🌱 Add Node related condition to Machine conditions #3670

sedefsavas · 2020-09-21T16:32:58Z

What this PR does / why we need it:
This PR adds a condition for Node health to Machine conditions that will be managed by Machine controller.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes # #3138

k8s-ci-robot · 2020-09-21T16:32:59Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

sedefsavas · 2020-09-21T16:33:08Z

cc @fabriziopandini

controllers/machine_controller_noderef.go

fabriziopandini

first pass

api/v1alpha3/condition_consts.go

controllers/machine_controller.go

controllers/machine_controller_noderef.go

sedefsavas · 2020-09-25T06:54:44Z

Unit test fails because CacheTracker panics in the tests that doesn't use testEnv. Probably will need to use testEnv for the tests using CacheTracker.

controllers/machine_controller_test.go

controllers/machine_controller_phases_test.go

api/v1alpha3/condition_consts.go

controllers/machine_controller_node.go

controllers/machine_controller_node_test.go

controllers/machine_controller_node.go

vincepri · 2020-09-29T18:03:58Z

/milestone v0.4.0

sedefsavas · 2020-09-30T05:29:46Z

/test pull-cluster-api-test

sedefsavas · 2020-10-20T05:23:37Z

This was ready to go, just solved conflicts.
cc @vincepri @fabriziopandini

fabriziopandini · 2020-10-20T19:53:47Z

/lgtm

vincepri

Need to take a bit more time to review but it seems we're potentially changing some behaviors that we should first chat about

vincepri · 2020-10-20T21:10:20Z

controllers/machine_controller.go

+	}
+	conditions.MarkFalse(m, clusterv1.MachineNodeHealthyCondition, clusterv1.DeletingReason, clusterv1.ConditionSeverityInfo, "")
+	if err := patchMachine(ctx, patchHelper, m); err != nil {
+		conditions.MarkFalse(m, clusterv1.MachineNodeHealthyCondition, clusterv1.DeletionFailedReason, clusterv1.ConditionSeverityInfo, "")


I would expect this to fail as well if the patch doesn't go through above

Machines are patched in Reconcile after returning from reconcileDelete, so setting this in case it won't fail there.

controllers/machine_controller.go

vincepri · 2020-10-20T21:13:35Z

controllers/machine_controller_node.go

+	node, err := r.getNode(remoteClient, providerID)
+	if err != nil {
+		if err == ErrNodeNotFound {
+			// While a NodeRef is set in the status, failing to get that node means the node is deleted.


What if it's a transient error? The node might not be deleted

We are setting the condition and reconciling, if it is transient it will be fixed right away. Otherwise how to detect the transient error?

vincepri · 2020-10-20T21:16:47Z

controllers/machine_controller_node.go

+	if err != nil {
+		if err == ErrNodeNotFound {
+			// While a NodeRef is set in the status, failing to get that node means the node is deleted.
+			// If Status.NodeRef is not set before, node still can be in the provisioning state.


This comment is a little confusing, don't we set the node reference a few lines down?

Before this PR we were returning directly if the NodeRef is set.

cluster-api/controllers/machine_controller_noderef.go

Line 45 in fe28e9d

if machine.Status.NodeRef != nil {

Removed that check to be able to check the health of the node.

And here, if node cannot be found when the NodeRef is already assigned, that means the node is deleted. But if NodeRef is not yet set, that means there was never a node before as NodeRef is nil, and hence it is not a NodeNotFoundReason issue.

controllers/machine_controller.go

vincepri · 2020-10-20T23:38:10Z

controllers/machine_controller_node.go

+			Kind:       node.Kind,
+			APIVersion: node.APIVersion,


Are these properly populated? Usually they're not, and they need to be filled in manually

This is same with what was used before:

cluster-api/controllers/machine_controller_noderef.go

Lines 103 to 107 in fe28e9d

return &apicorev1.ObjectReference{

Kind: node.Kind,

APIVersion: node.APIVersion,

Name: node.Name,

UID: node.UID,

sedefsavas · 2020-10-21T00:33:23Z

I had renamed machine_controller_noderef to machine_controller_node after addressing the previous comments.
But since there are new comments, reverted it back for better visibility.

cc @vincepri

k8s-ci-robot · 2020-10-21T00:34:07Z

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

Test name	Commit	Details	Rerun command
pull-cluster-api-apidiff	`6a11470`	link	`/test pull-cluster-api-apidiff`
pull-cluster-api-make-main	`07e4fc3`	link	`/test pull-cluster-api-make-main`
pull-cluster-api-test-main	`07e4fc3`	link	`/test pull-cluster-api-test-main`
pull-cluster-api-apidiff-main	`07e4fc3`	link	`/test pull-cluster-api-apidiff-main`
pull-cluster-api-build-main	`07e4fc3`	link	`/test pull-cluster-api-build-main`
pull-cluster-api-e2e-main	`07e4fc3`	link	`/test pull-cluster-api-e2e-main`

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.

controllers/machine_controller_noderef.go

vincepri · 2020-10-21T19:53:17Z

/hold cancel

fabriziopandini · 2020-10-26T13:11:46Z

@sedefsavas if you squash commits, then this one can be merged

fabriziopandini · 2020-10-26T13:13:50Z

api/v1alpha3/condition_consts.go

+	MachineNodeHealthyCondition ConditionType = "NodeHealthy"
+
+	// WaitingForNodeRefReason (Severity=Info) documents a machine.spec.providerId is not assigned yet.
+	WaitingForNodeRefReason = "WaitingForNodeRef"


nit: what about

Suggested change

WaitingForNodeRefReason = "WaitingForNodeRef"

WaitingForNodeProvisioningReason = "WaitingForNodeProvisioning"

NodeRef is a bit too technical for a user that does not know CAPI internals

There is already a NodeProvisioningReason, I think it will be confusing.
WaitingForNodeRefReason is waiting for machine.Spec.ProviderID and
NodeProvisioningReason is waiting for machine.Status.NodeRef when there is no node found.

controllers/machine_controller_noderef.go

fabriziopandini · 2020-10-26T17:14:44Z

/approve

k8s-ci-robot · 2020-10-26T17:14:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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]

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

fabriziopandini · 2020-10-26T20:32:38Z

/lgtm

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 21, 2020

k8s-ci-robot requested review from detiber and JoelSpeed September 21, 2020 16:33

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 21, 2020

vincepri reviewed Sep 21, 2020

View reviewed changes

controllers/machine_controller_noderef.go Outdated Show resolved Hide resolved

vincepri reviewed Sep 21, 2020

View reviewed changes

controllers/machine_controller_noderef.go Outdated Show resolved Hide resolved

sedefsavas mentioned this pull request Sep 22, 2020

🌱Add Machine and KCP conditions to KCP controller #3674

Closed

fabriziopandini reviewed Sep 22, 2020

View reviewed changes

sedefsavas commented Sep 23, 2020

View reviewed changes

controllers/machine_controller_noderef.go Outdated Show resolved Hide resolved

sedefsavas force-pushed the machine_conditiononly branch from 6a2d6b7 to eb2fff6 Compare September 23, 2020 06:19

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 23, 2020

sedefsavas marked this pull request as ready for review September 25, 2020 06:43

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 25, 2020

sedefsavas force-pushed the machine_conditiononly branch from eb2fff6 to e2c6e86 Compare September 25, 2020 06:56

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 28, 2020

sedefsavas commented Sep 28, 2020

View reviewed changes

controllers/machine_controller_test.go Outdated Show resolved Hide resolved

vincepri reviewed Sep 28, 2020

View reviewed changes

controllers/machine_controller_phases_test.go Outdated Show resolved Hide resolved

vincepri reviewed Sep 29, 2020

View reviewed changes

k8s-ci-robot added this to the v0.4.0 milestone Sep 29, 2020

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 29, 2020

sedefsavas force-pushed the machine_conditiononly branch from 995c571 to 663a3eb Compare September 29, 2020 19:46

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2020

sedefsavas force-pushed the machine_conditiononly branch from 42b6249 to 52b6450 Compare October 20, 2020 05:22

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 20, 2020

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

vincepri requested changes Oct 20, 2020

View reviewed changes

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

vincepri reviewed Oct 20, 2020

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2020

vincepri reviewed Oct 21, 2020

View reviewed changes

controllers/machine_controller_noderef.go Outdated Show resolved Hide resolved

vincepri reviewed Oct 21, 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 Oct 21, 2020

fabriziopandini reviewed Oct 26, 2020

View reviewed changes

Add Node related condition to Machine conditions

9506423

sedefsavas force-pushed the machine_conditiononly branch from 394b4bb to 9506423 Compare October 26, 2020 15:56

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2020

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

k8s-ci-robot merged commit cb3095b into kubernetes-sigs:release-0.3 Oct 26, 2020

fabriziopandini mentioned this pull request Oct 27, 2020

MachineDeployment status is not easily understood #3865

Closed

sedefsavas mentioned this pull request Nov 2, 2020

🌱 Add Node related condition to Machine conditions #3890

Merged

fabriziopandini mentioned this pull request Dec 2, 2020

Add MachineNodeHealthyCondition to Machine’s Ready condition #3974

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Add Node related condition to Machine conditions #3670

🌱 Add Node related condition to Machine conditions #3670

sedefsavas commented Sep 21, 2020

k8s-ci-robot commented Sep 21, 2020

sedefsavas commented Sep 21, 2020

fabriziopandini left a comment

sedefsavas commented Sep 25, 2020

vincepri commented Sep 29, 2020

sedefsavas commented Sep 30, 2020

sedefsavas commented Oct 20, 2020

fabriziopandini commented Oct 20, 2020

vincepri left a comment

vincepri Oct 20, 2020

sedefsavas Oct 21, 2020

vincepri Oct 20, 2020

sedefsavas Oct 21, 2020

vincepri Oct 20, 2020

sedefsavas Oct 21, 2020

vincepri Oct 20, 2020

sedefsavas Oct 21, 2020

sedefsavas commented Oct 21, 2020

k8s-ci-robot commented Oct 21, 2020 •

edited

Loading

vincepri commented Oct 21, 2020

fabriziopandini commented Oct 26, 2020

fabriziopandini Oct 26, 2020

sedefsavas Oct 26, 2020

fabriziopandini commented Oct 26, 2020

k8s-ci-robot commented Oct 26, 2020

fabriziopandini commented Oct 26, 2020

	return &apicorev1.ObjectReference{
	Kind: node.Kind,
	APIVersion: node.APIVersion,
	Name: node.Name,
	UID: node.UID,

	WaitingForNodeRefReason = "WaitingForNodeRef"
	WaitingForNodeProvisioningReason = "WaitingForNodeProvisioning"

🌱 Add Node related condition to Machine conditions #3670

🌱 Add Node related condition to Machine conditions #3670

Conversation

sedefsavas commented Sep 21, 2020

k8s-ci-robot commented Sep 21, 2020

sedefsavas commented Sep 21, 2020

fabriziopandini left a comment

Choose a reason for hiding this comment

sedefsavas commented Sep 25, 2020

vincepri commented Sep 29, 2020

sedefsavas commented Sep 30, 2020

sedefsavas commented Oct 20, 2020

fabriziopandini commented Oct 20, 2020

vincepri left a 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

sedefsavas commented Oct 21, 2020

k8s-ci-robot commented Oct 21, 2020 • edited Loading

vincepri commented Oct 21, 2020

fabriziopandini commented Oct 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini commented Oct 26, 2020

k8s-ci-robot commented Oct 26, 2020

fabriziopandini commented Oct 26, 2020

k8s-ci-robot commented Oct 21, 2020 •

edited

Loading