🌱 Add conditions to the Machine object #3136

fabriziopandini · 2020-06-03T12:44:26Z

What this PR does / why we need it:
This PR adds conditions to the Machine object

Which issue(s) this PR fixes:
Fixes #3112

controllers/machine_controller_phases.go

detiber · 2020-06-03T14:16:19Z

controllers/machine_controller_phases.go

+	// Report a summary of current status of the bootstrap object defined for this machine.
+	bootstrapReadyCondition := conditions.Mirror(conditions.UnstructuredGetter(bootstrapConfig), clusterv1.BootstrapReadyCondition)
+	conditions.Set(m, bootstrapReadyCondition)
+


I wouldn't necessarily expect it to happen, but if we plan on eventually relying on conditions rather than the existing status fields for logic down the line, should we attempt to detect any discrepancies between conditions and the status fields and throw an error if they do not align?

🤔 I really hope that doesn't happen, seems it would be definitely an implementation issue although not sure if this is the right time/place for conditions to solve that problem, wdyt?

I'm just wondering what potential downstream consequences we are looking at if semantics start varying widely between our current contract and conditions-based contracts.

If we pro-actively try to align semantics (maybe through warning instead of erroring to start with), then avoid having to try to reconcile later after implementations are already in place.

Alternatively, we could allow for new providers to implement only conditions rather than both the status fields and conditions, and update the logic accordingly:

If status fields exist, but not conditions, use those

If conditions exist, but not status fields, use those

If both status fields and conditions exist, test for alignment and warn/error

For the last bullet, would we have to write special logic in every controller?

For the last bullet, would we have to write special logic in every controller?

I think there are possibly two aspects to it:

Producing controllers: Ensure tests in place verifying that they don't generate conflicting signals

Consuming controllers (assuming warning rather than error): Verify if both status fields and conditions present that they do not contradict each other, warn (so that hopefully the bug report makes it up to fix the behavior) in the producing controller, then proceed with logic based on status fields (to not break existing behavior)

We could probably find a way to generalize the consistency check, but at the end of the day matching contractual behaviors between consumed status fields vs consumed conditions will likely be controller specific.

These are definitely valid points / concerns, let's chat later all of us and make sure to capture action items in an issue. It's also a matter of documentation for folks implementing conditions, pretty important to call it out explicitly.

My major concern here is that conditions are intended to express the operational state of a component in the cluster, while flags were designed for driving the internal state machines of controllers.
In other words, IMO conditions provide an API designed for a consumer external to Cluster API, while flags are designed for a consumer internal to Cluster API.

I have opened #3153 to start the discussion about what we want to do about conditions-based contracts vs flags.

controllers/machine_controller_phases.go

fabriziopandini · 2020-06-03T19:36:34Z

@vincepri @detiber

I have implemented conditions fall back
I was forced to rebase in order to get CI passing with the new linters
lets sync in slack or zoom for flags vs conditions consistency

util/conditions/getter.go

controllers/machine_controller_phases.go

controllers/machine_controller.go

benmoss · 2020-06-04T14:22:36Z

util/conditions/getter_test.go

-			got := Mirror(tt.from, tt.t)
+			got := mirror(tt.from, tt.t)


We should move these tests to be on the setters now that these methods are private

I think that the current code organization also makes sense because the part of code that gets a Mirror condition is in getter.go/getter_test.go (no matter if they are private), and the set helpers using the mirror condition in setter.go

Yeah but the general reason you don't test private methods is that they are just an implementation detail of the public methods. They can essentially be inlined into the setters, since they effectively don't exist except for callers within the same package.

I think we're also missing coverage of the setters

@benmoss do you think that is something that we can handle as a followup or do you consider it a blocker for this PR?

I've asked Fabrizio to make these methods private because they might be misleading to users, or hard to use; I don't necessarily mind keeping the tests for now, we can clean it up later

the general reason you don't test private methods is that they are just an implementation detail of the public methods.

This is a general concern, not strictly related to this PR, but however IMO tests are always a good thing, no matter if for private or public func, and AFAIK we already have a lot of tests on private methods. Also testing private methods helps for separation of concern in unit tests.

I think we're also missing coverage of the setters

Done

Yeah it's not a blocker, just a conversation I thought was worth having

benmoss · 2020-06-04T18:56:48Z

api/v1alpha3/condition_consts.go

+// Conditions and condition Reasons for the Machine object
+
+// MachineConditionsCount defines the total number of conditions on the Machine object.
+const MachineConditionsCount = 2


Is this expected to be the total number of conditions defined anywhere that get put onto Machines, or just the number that are being used for moving towards "ready" in the summary? We are about to run into this problem with MHC, where we're adding two more conditions to Machine. I think we shouldn't, but then we should probably clarify what this constant is. Maybe like SummaryConditionsCount? Not great, but something like that?

This expected to be the total number of conditions defined anywhere that get put onto Machines;
FYI. I'm considering changing the 1 of x behavior because it works well during provisioning but it is not ideal when the machine is up and running. But for now, this is enough for getting us started

So we will want to update this to 4 when the MHC changes get merged? Won't it always be stuck at 2/4 in the progress summary?

Might be good to track this as a separate issue for cleanup

I have opened #3151 to track the problem and #3152 to implement a first fix to avoid misleading messages

controllers/machine_controller_phases.go

fabriziopandini · 2020-06-05T19:16:33Z

/retest

fabriziopandini · 2020-06-08T12:02:04Z

@vincepri I managed to get test passing, PTAL

test/helpers/envtest.go

fabriziopandini · 2020-06-08T15:01:45Z

/hold
for #3125 to merge

detiber · 2020-06-08T16:04:04Z

util/patch/fake/client.go

+
+		if accessor.GetResourceVersion() == "" {
+			accessor.SetResourceVersion("1")
+		}


I'm wondering if it might be a good idea to also create a deep copy of any passed in resources as well, which could help avoid potential issues where someone forgets to make a deep copy of a resource that is used in multiple tests across mutliple fake clients?

Uhhhmm
when I call client.Create I do expect side effects, and NewFakeClientWithScheme is just an helper for doing NewClient and client.Create....

It gets complicated, though... If you call client.Create you expect the side effect to be that the resource reflects the state immediately after the Create call.

With the Fake client, and passing in the resources to NewFakeClientWithScheme I believe you may also get changes made from other operations against the backing resource if you don't provide it a deep copy. So if you designed a test where you:

create a resource to use for initializing and comparison later, let's call it preResource

pass the pointer to preResource resource in directly to NewFakeClientWiithScheme

call a function/method that is meant to modify the resource represented by preResource, let's call it storedResource

get a copy of storedResource using client.Get, let's call this copy postResource`

Because storedResource and preResource are pointing to the same underlying resource, someone building a test could mistakenly attempt to make comparisons using preResource against postResource under the assumption that preResource actually represents the state of the resource prior to calling the function that modifies storedResource, however it actually represents the state of storedResource.

Not sure if that helps with clarifying my concern.

Yeah, @detiber is correct here — we need to make sure to not modify the caller object during init. I hope in the future we only use Create and avoid using any init objects.

Makefile

vincepri

/approve
/hold cancel
/milestone v0.3.7

k8s-ci-robot · 2020-06-08T18:29:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [fabriziopandini,vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vincepri · 2020-06-08T18:31:57Z

The conditions API diff is fine because we haven't released yet. The ones for test CRDs in the external package might be breaking though. Can we use DeepCopy() instead of a function?

fabriziopandini · 2020-06-08T18:45:07Z

@vincepri done
I will open an issue so we keep track to switch to func as soon as possible

k8s-ci-robot · 2020-06-08T18:50:38Z

@fabriziopandini: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-apidiff	`5113f80`	link	`/test pull-cluster-api-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

detiber · 2020-06-08T19:36:33Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 3, 2020

k8s-ci-robot requested review from CecileRobertMichon and ncdc June 3, 2020 12:44

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 3, 2020

detiber reviewed Jun 3, 2020

View reviewed changes

fabriziopandini force-pushed the add-conditions-to-machine branch from 7bec0b0 to bf3d451 Compare June 3, 2020 19:25

vincepri reviewed Jun 3, 2020

View reviewed changes

util/conditions/getter.go Outdated Show resolved Hide resolved

detiber reviewed Jun 3, 2020

View reviewed changes

controllers/machine_controller_phases.go Outdated Show resolved Hide resolved

fabriziopandini mentioned this pull request Jun 4, 2020

🌱 Add conditions to the KubeadmControlPlane object #3139

Merged

benmoss reviewed Jun 4, 2020

View reviewed changes

fabriziopandini mentioned this pull request Jun 5, 2020

Define roadmap for a condition based contract #3153

Closed

detiber reviewed Jun 5, 2020

View reviewed changes

controllers/machine_controller_phases.go Outdated Show resolved Hide resolved

gab-satchi mentioned this pull request Jun 5, 2020

Add conditions to AWSCluster kubernetes-sigs/cluster-api-provider-aws#1742

Closed

fabriziopandini force-pushed the add-conditions-to-machine branch from 4d4f17a to 307e3ec Compare June 8, 2020 11:40

fabriziopandini commented Jun 8, 2020

View reviewed changes

test/helpers/envtest.go Outdated Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2020

detiber reviewed Jun 8, 2020

View reviewed changes

fabriziopandini force-pushed the add-conditions-to-machine branch from 307e3ec to 2fbb91c Compare June 8, 2020 18:02

vincepri reviewed Jun 8, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

fabriziopandini force-pushed the add-conditions-to-machine branch from 2fbb91c to 11bd893 Compare June 8, 2020 18:28

vincepri approved these changes Jun 8, 2020

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2020

k8s-ci-robot added this to the v0.3.7 milestone Jun 8, 2020

fabriziopandini added 3 commits June 8, 2020 20:43

Fix to allow multiple test environments

88d43aa

Refactor condition helpers for Mirror, Summary and Aggregate

6494b2d

Add conditions to the Machine object

5113f80

fabriziopandini force-pushed the add-conditions-to-machine branch from 11bd893 to 5113f80 Compare June 8, 2020 18:45

k8s-ci-robot assigned detiber Jun 8, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2020

k8s-ci-robot merged commit 03cff23 into kubernetes-sigs:master Jun 8, 2020

fabriziopandini mentioned this pull request Jun 9, 2020

Refactor test CRDs in controller/external #3168

Closed

fabriziopandini deleted the add-conditions-to-machine branch June 9, 2020 11:55

fabriziopandini mentioned this pull request Jun 9, 2020

🌱 fix test/helpers.NewFakeClientWithScheme #3171

Merged

🌱 Add conditions to the Machine object #3136

🌱 Add conditions to the Machine object #3136

Conversation

fabriziopandini commented Jun 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

fabriziopandini Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

fabriziopandini commented Jun 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini commented Jun 5, 2020

fabriziopandini commented Jun 8, 2020

fabriziopandini commented Jun 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 8, 2020

vincepri commented Jun 8, 2020

fabriziopandini commented Jun 8, 2020

k8s-ci-robot commented Jun 8, 2020

detiber commented Jun 8, 2020

fabriziopandini Jun 3, 2020 •

edited

Loading

fabriziopandini Jun 5, 2020 •

edited

Loading