✨ KCP remediates unhealthy machines #3830

fabriziopandini · 2020-10-20T15:18:10Z

What this PR does / why we need it:
This PR adds to KCP support for remediating unhealthy machines according to KCP proposal changes defined by #3676

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

fabriziopandini · 2020-10-20T15:20:12Z

The implementation should be completed so people can review (thanks in advance for feedback)
however, I prefer to keep this on hold while doing some additional local testing
/hold

srm09

minor nit suggestions

api/v1alpha3/condition_consts.go

controlplane/kubeadm/internal/workload_cluster_etcd.go

controlplane/kubeadm/internal/control_plane_test.go

controlplane/kubeadm/internal/machinefilters/machine_filters.go

controlplane/kubeadm/controllers/remediation.go

controlplane/kubeadm/controllers/remediation_test.go

controlplane/kubeadm/controllers/remediation.go

controlplane/kubeadm/controllers/remediation_test.go

controlplane/kubeadm/controllers/remediation.go

controlplane/kubeadm/controllers/controller.go

controlplane/kubeadm/controllers/remediation.go

vincepri · 2020-10-21T19:48:48Z

controlplane/kubeadm/controllers/remediation.go

+		logger.Info("A control plane machine needs remediation, but the current number of replicas is currently lower that expected. Skipping remediation", "UnhealthyMachine", machineToBeRemediated.Name, "Replicas", desiredReplicas, "CurrentReplicas", c.Machines.Len())
+		conditions.MarkFalse(machineToBeRemediated, clusterv1.MachineOwnerRemediatedCondition, clusterv1.WaitingForRemediationReason, clusterv1.ConditionSeverityWarning, "KCP waiting for having at least %d control plane machines before triggering remediation", desiredReplicas)


Sidenote, it'd be great to have the conditions functions to log things out for us

vincepri · 2020-10-21T19:50:55Z

/milestone v0.3.11

fabriziopandini · 2020-10-21T20:27:30Z

@vincepri @sedefsavas thanks for the feedback!
I have added some small improvements after today's tests, each of them in separate commits for helping in the review.

/hold cancel
Given the positive results of the additional local tests

fabriziopandini · 2020-10-22T08:43:59Z

/test pull-cluster-api-test-release-0-3

fabriziopandini · 2020-10-23T19:52:40Z

/hold
for #3863 to merge

sedefsavas

Some nits.

I will LGTM after it is approved.

api/v1alpha3/condition_consts.go

controlplane/kubeadm/internal/control_plane.go

sedefsavas · 2020-10-26T22:04:08Z

controlplane/kubeadm/controllers/remediation.go

+
+	defer func() {
+		// Always attempt to Patch the Machine conditions after each reconcileUnhealthyMachines.
+		if err := patchHelper.Patch(ctx, machineToBeRemediated, patch.WithOwnedConditions{Conditions: []clusterv1.ConditionType{


nit.
WithOwnedConditions is deprecated, is there another way to use here?

For now, I'm keeping the same approach we are using in the other controllers (WithOwnedConditions), but soon or later we should switch everything to WithForceOverwriteConditions unless we finally switch to server side apply.

We should un-deprecate it, we shouldn't use WithForceOverwriteConditions here

ncdc · 2020-10-27T20:50:10Z

@sedefsavas

I will LGTM after it is approved.

FYI, the documented process in CONTRIBUTING.md is that LGTM is supposed to come before approval.

fabriziopandini · 2020-10-29T15:44:36Z

/hold cancel

fabriziopandini · 2020-11-04T20:20:46Z

/ hold
for #3900 to merge
only the latest commit is relevant for this PR

fabriziopandini · 2020-11-11T14:05:55Z

/test pull-cluster-api-e2e-full-release-0-3

fabriziopandini · 2020-11-11T15:31:26Z

/retest

k8s-ci-robot · 2020-11-11T16:11:41Z

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

Test name	Commit	Details	Rerun command
pull-cluster-api-e2e-full-release-0-3	`fbd23d4`	link	`/test pull-cluster-api-e2e-full-release-0-3`

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/machinehealthcheck_controller.go

controlplane/kubeadm/controllers/remediation.go

vincepri · 2020-11-11T21:40:54Z

controlplane/kubeadm/controllers/remediation.go

+		return ctrl.Result{}, errors.Wrapf(err, "failed to delete unhealthy machine %s", machineToBeRemediated.Name)
+	}
+
+	logger.Info("Remediating unhealthy machine", "UnhealthyMachine", machineToBeRemediated.Name)


Hasn't remediation already happened at this point? Or is it in progress?

Yes, the machine object is deleted at this point and the KCP job is completed, but the actual machine deletion will take some time to happen, so from a user PoV I think it is correct to say that remediation is in progress (and it shows up in the same way in the conditions as well).

controlplane/kubeadm/controllers/remediation.go

controlplane/kubeadm/controllers/remediation_test.go

vincepri · 2020-11-12T16:25:19Z

LGTM, squash? :)

fabriziopandini · 2020-11-12T16:48:05Z

/test pull-cluster-api-e2e-full-release-0-3

vincepri

/approve
/lgtm

k8s-ci-robot · 2020-11-12T16:58:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [vincepri]

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

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

k8s-ci-robot requested review from detiber and JoelSpeed October 20, 2020 15:18

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 20, 2020

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

srm09 reviewed Oct 20, 2020

View reviewed changes

api/v1alpha3/condition_consts.go Outdated Show resolved Hide resolved

controlplane/kubeadm/internal/workload_cluster_etcd.go Outdated Show resolved Hide resolved

sedefsavas reviewed Oct 21, 2020

View reviewed changes

controlplane/kubeadm/controllers/remediation.go Show resolved Hide resolved

sedefsavas reviewed Oct 21, 2020

View reviewed changes

vincepri reviewed Oct 21, 2020

View reviewed changes

k8s-ci-robot added this to the v0.3.11 milestone Oct 21, 2020

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 mentioned this pull request Oct 22, 2020

Improve how do we determine the control plane machine to be remediated #3845

Closed

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

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 23, 2020

fabriziopandini force-pushed the kcp-remediation branch from 364c34b to 8d76ecc Compare October 26, 2020 13:24

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

sedefsavas reviewed Oct 26, 2020

View reviewed changes

fabriziopandini linked an issue Oct 27, 2020 that may be closed by this pull request

KCP unhealthy remediation support #2976

Closed

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

fabriziopandini force-pushed the kcp-remediation branch from 8d76ecc to 269290a Compare October 28, 2020 11:01

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

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 29, 2020

fabriziopandini force-pushed the kcp-remediation branch from 269290a to 4d6b7d7 Compare November 4, 2020 20:19

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 4, 2020

fabriziopandini force-pushed the kcp-remediation branch 3 times, most recently from 32b4bde to fbd23d4 Compare November 11, 2020 14:04

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 11, 2020

vincepri reviewed Nov 11, 2020

View reviewed changes

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 12, 2020

fabriziopandini mentioned this pull request Nov 12, 2020

Add E2E test for KCP remediation #3913

Closed

KCP remediates unhealthy machines

b34a457

fabriziopandini force-pushed the kcp-remediation branch from 69abfe1 to b34a457 Compare November 12, 2020 16:47

vincepri approved these changes Nov 12, 2020

View reviewed changes

k8s-ci-robot assigned vincepri Nov 12, 2020

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

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

k8s-ci-robot merged commit 24f10f8 into kubernetes-sigs:release-0.3 Nov 12, 2020

fabriziopandini mentioned this pull request Nov 12, 2020

Forward port KCP remediation #3914

Closed

fabriziopandini deleted the kcp-remediation branch November 17, 2020 12:34

fabriziopandini mentioned this pull request Nov 30, 2020

✨ KCP remediation #3956

Merged

joekr mentioned this pull request Sep 9, 2022

Add MachineHealthCheck to our templates oracle/cluster-api-provider-oci#144

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ KCP remediates unhealthy machines #3830

✨ KCP remediates unhealthy machines #3830

fabriziopandini commented Oct 20, 2020

fabriziopandini commented Oct 20, 2020

srm09 left a comment

vincepri Oct 21, 2020

vincepri commented Oct 21, 2020

fabriziopandini commented Oct 21, 2020

fabriziopandini commented Oct 22, 2020

fabriziopandini commented Oct 23, 2020

sedefsavas left a comment

sedefsavas Oct 26, 2020

fabriziopandini Oct 28, 2020

vincepri Nov 11, 2020

ncdc commented Oct 27, 2020

fabriziopandini commented Oct 29, 2020

fabriziopandini commented Nov 4, 2020

fabriziopandini commented Nov 11, 2020

fabriziopandini commented Nov 11, 2020

k8s-ci-robot commented Nov 11, 2020

vincepri Nov 11, 2020

fabriziopandini Nov 12, 2020

vincepri commented Nov 12, 2020

fabriziopandini commented Nov 12, 2020

vincepri left a comment

k8s-ci-robot commented Nov 12, 2020

		logger.Info("A control plane machine needs remediation, but the current number of replicas is currently lower that expected. Skipping remediation", "UnhealthyMachine", machineToBeRemediated.Name, "Replicas", desiredReplicas, "CurrentReplicas", c.Machines.Len())
		conditions.MarkFalse(machineToBeRemediated, clusterv1.MachineOwnerRemediatedCondition, clusterv1.WaitingForRemediationReason, clusterv1.ConditionSeverityWarning, "KCP waiting for having at least %d control plane machines before triggering remediation", desiredReplicas)

✨ KCP remediates unhealthy machines #3830

✨ KCP remediates unhealthy machines #3830

Conversation

fabriziopandini commented Oct 20, 2020

fabriziopandini commented Oct 20, 2020

srm09 left a comment

Choose a reason for hiding this comment

vincepri Oct 21, 2020

Choose a reason for hiding this comment

vincepri commented Oct 21, 2020

fabriziopandini commented Oct 21, 2020

fabriziopandini commented Oct 22, 2020

fabriziopandini commented Oct 23, 2020

sedefsavas left a comment

Choose a reason for hiding this comment

sedefsavas Oct 26, 2020

Choose a reason for hiding this comment

fabriziopandini Oct 28, 2020

Choose a reason for hiding this comment

vincepri Nov 11, 2020

Choose a reason for hiding this comment

ncdc commented Oct 27, 2020

fabriziopandini commented Oct 29, 2020

fabriziopandini commented Nov 4, 2020

fabriziopandini commented Nov 11, 2020

fabriziopandini commented Nov 11, 2020

k8s-ci-robot commented Nov 11, 2020

vincepri Nov 11, 2020

Choose a reason for hiding this comment

fabriziopandini Nov 12, 2020

Choose a reason for hiding this comment

vincepri commented Nov 12, 2020

fabriziopandini commented Nov 12, 2020

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 12, 2020