Repository: kubernetes-sigs/cluster-api
Pull request #5381 (2021-10-04 19:04:09)

🐛 KCP should cleanup OwnerRemediated condition if the machine goes back to healthy
**What this PR does / why we need it**:
This PR allows KCP to properly cleanup OwnerRemediated condition if the machine goes back to healthy and remediation did not happened for any reason

**Which issue(s) this PR fixes**:
Fixes https://github.com/kubernetes-sigs/cluster-api/issues/5169


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please ask for approval from **fabriziopandini** after the PR has been reviewed.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fcluster-api).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-sigs/cluster-api/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["fabriziopandini"]} -->
sbueringer: lgtm +/- squash

Patch:
diff --git a/cmd/clusterctl/cmd/upgrade_apply.go b/cmd/clusterctl/cmd/upgrade_apply.go
index 6dbc7be04..2a01c9628 100644
--- a/cmd/clusterctl/cmd/upgrade_apply.go
+++ b/cmd/clusterctl/cmd/upgrade_apply.go
@@ -86,9 +86,6 @@ func runUpgradeApply() error {
 		(len(ua.controlPlaneProviders) > 0) ||
 		(len(ua.infrastructureProviders) > 0)
 
-	if ua.contract == "" && !hasProviderNames {
-		return errors.New("Either the --contract flag or at least one of the following flags has to be set: --core, --bootstrap, --control-plane, --infrastructure")
-	}
 	if ua.contract != "" && hasProviderNames {
 		return errors.New("The --contract flag can't be used in combination with --core, --bootstrap, --control-plane, --infrastructure")
 	}
diff --git a/controlplane/kubeadm/controllers/remediation.go b/controlplane/kubeadm/controllers/remediation.go
index a3c818dbe..312a0df74 100644
--- a/controlplane/kubeadm/controllers/remediation.go
+++ b/controlplane/kubeadm/controllers/remediation.go
@@ -22,6 +22,7 @@ import (
 
 	"github.com/blang/semver"
 	"github.com/pkg/errors"
+	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1"
 	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal"
@@ -37,6 +38,33 @@ import (
 func (r *KubeadmControlPlaneReconciler) reconcileUnhealthyMachines(ctx context.Context, controlPlane *internal.ControlPlane) (ret ctrl.Result, retErr error) {
 	log := ctrl.LoggerFrom(ctx)
 
+	// Cleanup pending remediation actions not completed for any reasons (e.g. number of current replicas is less or equal to 1)
+	// if the underlying machine is now back to healthy / not deleting.
+	errList := []error{}
+	healthyMachines := controlPlane.HealthyMachines()
+	for _, m := range healthyMachines {
+		if conditions.IsTrue(m, clusterv1.MachineHealthCheckSuccededCondition) &&
+			conditions.IsFalse(m, clusterv1.MachineOwnerRemediatedCondition) &&
+			m.DeletionTimestamp.IsZero() {
+			patchHelper, err := patch.NewHelper(m, r.Client)
+			if err != nil {
+				errList = append(errList, errors.Wrapf(err, "failed to get PatchHelper for machine %s", m.Name))
+				continue
+			}
+
+			conditions.Delete(m, clusterv1.MachineOwnerRemediatedCondition)
+
+			if err := patchHelper.Patch(ctx, m, patch.WithOwnedConditions{Conditions: []clusterv1.ConditionType{
+				clusterv1.MachineOwnerRemediatedCondition,
+			}}); err != nil {
+				errList = append(errList, errors.Wrapf(err, "failed to patch machine %s", m.Name))
+			}
+		}
+	}
+	if len(errList) > 0 {
+		return ctrl.Result{}, kerrors.NewAggregate(errList)
+	}
+
 	// Gets all machines that have `MachineHealthCheckSucceeded=False` (indicating a problem was detected on the machine)
 	// and `MachineOwnerRemediated` present, indicating that this controller is responsible for performing remediation.
 	unhealthyMachines := controlPlane.UnhealthyMachines()
diff --git a/go.mod b/go.mod
index 6a83ddf22..63180ed4a 100644
--- a/go.mod
+++ b/go.mod
@@ -38,6 +38,6 @@ require (
 	k8s.io/klog/v2 v2.9.0
 	k8s.io/kubectl v0.21.4
 	k8s.io/utils v0.0.0-20210819203725-bdf08cb9a70a
-	sigs.k8s.io/controller-runtime v0.10.2
+	sigs.k8s.io/controller-runtime v0.10.1
 	sigs.k8s.io/yaml v1.2.0
 )
diff --git a/go.sum b/go.sum
index ab7aee5c6..9adc754a6 100644
--- a/go.sum
+++ b/go.sum
@@ -1125,8 +1125,8 @@ rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8
 rsc.io/quote/v3 v3.1.0/go.mod h1:yEA65RcK8LyAZtP9Kv3t0HmxON59tX3rD+tICJqUlj0=
 rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA=
 sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.0.22/go.mod h1:LEScyzhFmoF5pso/YSeBstl57mOzx9xlU9n85RGrDQg=
-sigs.k8s.io/controller-runtime v0.10.2 h1:jW8qiY+yMnnPx6O9hu63tgcwaKzd1yLYui+mpvClOOc=
-sigs.k8s.io/controller-runtime v0.10.2/go.mod h1:CQp8eyUQZ/Q7PJvnIrB6/hgfTC1kBkGylwsLgOQi1WY=
+sigs.k8s.io/controller-runtime v0.10.1 h1:+eLHgY/VrJWnfg6iXUqhCUqNXgPH1NZeP9drNAAgWlg=
+sigs.k8s.io/controller-runtime v0.10.1/go.mod h1:CQp8eyUQZ/Q7PJvnIrB6/hgfTC1kBkGylwsLgOQi1WY=
 sigs.k8s.io/kustomize/api v0.8.8/go.mod h1:He1zoK0nk43Pc6NlV085xDXDXTNprtcyKZVm3swsdNY=
 sigs.k8s.io/kustomize/cmd/config v0.9.10/go.mod h1:Mrby0WnRH7hA6OwOYnYpfpiY0WJIMgYrEDfwOeFdMK0=
 sigs.k8s.io/kustomize/kustomize/v4 v4.1.2/go.mod h1:PxBvo4WGYlCLeRPL+ziT64wBXqbgfcalOS/SXa/tcyo=


Test patch:
diff --git a/controlplane/kubeadm/controllers/remediation_test.go b/controlplane/kubeadm/controllers/remediation_test.go
index 9d14c62ac..769dcf530 100644
--- a/controlplane/kubeadm/controllers/remediation_test.go
+++ b/controlplane/kubeadm/controllers/remediation_test.go
@@ -52,6 +52,32 @@ func TestReconcileUnhealthyMachines(t *testing.T) {
 		g.Expect(env.Cleanup(ctx, ns)).To(Succeed())
 	}()
 
+	t.Run("Remediation cleans up stuck remediation on previously unhealthy machines", func(t *testing.T) {
+		g := NewWithT(t)
+
+		m := createMachine(ctx, g, ns.Name, "m1-unhealthy-", withStuckRemediation())
+
+		controlPlane := &internal.ControlPlane{
+			KCP:      &controlplanev1.KubeadmControlPlane{},
+			Cluster:  &clusterv1.Cluster{},
+			Machines: collections.FromMachines(m),
+		}
+		ret, err := r.reconcileUnhealthyMachines(context.TODO(), controlPlane)
+
+		g.Expect(ret.IsZero()).To(BeTrue()) // Remediation skipped
+		g.Expect(err).ToNot(HaveOccurred())
+
+		g.Eventually(func() error {
+			if err := env.Get(ctx, client.ObjectKey{Namespace: m.Namespace, Name: m.Name}, m); err != nil {
+				return err
+			}
+			c := conditions.Get(m, clusterv1.MachineOwnerRemediatedCondition)
+			if c == nil {
+				return nil
+			}
+			return errors.Errorf("condition %s still exists", clusterv1.MachineOwnerRemediatedCondition)
+		}, 10*time.Second).Should(Succeed())
+	})
 	t.Run("Remediation does not happen if there are no unhealthy machines", func(t *testing.T) {
 		g := NewWithT(t)
 
@@ -759,6 +785,13 @@ func withMachineHealthCheckFailed() machineOption {
 	}
 }
 
+func withStuckRemediation() machineOption {
+	return func(machine *clusterv1.Machine) {
+		conditions.MarkTrue(machine, clusterv1.MachineHealthCheckSuccededCondition)
+		conditions.MarkFalse(machine, clusterv1.MachineOwnerRemediatedCondition, clusterv1.WaitingForRemediationReason, clusterv1.ConditionSeverityWarning, "")
+	}
+}
+
 func withHealthyEtcdMember() machineOption {
 	return func(machine *clusterv1.Machine) {
 		conditions.MarkTrue(machine, controlplanev1.MachineEtcdMemberHealthyCondition)
diff --git a/test/go.mod b/test/go.mod
index f35f7421c..7fb7aa21c 100644
--- a/test/go.mod
+++ b/test/go.mod
@@ -24,7 +24,7 @@ require (
 	k8s.io/klog/v2 v2.9.0
 	k8s.io/utils v0.0.0-20210819203725-bdf08cb9a70a
 	sigs.k8s.io/cluster-api v0.0.0-00010101000000-000000000000
-	sigs.k8s.io/controller-runtime v0.10.2
+	sigs.k8s.io/controller-runtime v0.10.1
 	sigs.k8s.io/kind v0.11.1
 	sigs.k8s.io/yaml v1.2.0
 )
diff --git a/test/go.sum b/test/go.sum
index 41da39b4f..2f3fc3da4 100644
--- a/test/go.sum
+++ b/test/go.sum
@@ -1469,8 +1469,8 @@ rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA=
 sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.0.14/go.mod h1:LEScyzhFmoF5pso/YSeBstl57mOzx9xlU9n85RGrDQg=
 sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.0.15/go.mod h1:LEScyzhFmoF5pso/YSeBstl57mOzx9xlU9n85RGrDQg=
 sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.0.22/go.mod h1:LEScyzhFmoF5pso/YSeBstl57mOzx9xlU9n85RGrDQg=
-sigs.k8s.io/controller-runtime v0.10.2 h1:jW8qiY+yMnnPx6O9hu63tgcwaKzd1yLYui+mpvClOOc=
-sigs.k8s.io/controller-runtime v0.10.2/go.mod h1:CQp8eyUQZ/Q7PJvnIrB6/hgfTC1kBkGylwsLgOQi1WY=
+sigs.k8s.io/controller-runtime v0.10.1 h1:+eLHgY/VrJWnfg6iXUqhCUqNXgPH1NZeP9drNAAgWlg=
+sigs.k8s.io/controller-runtime v0.10.1/go.mod h1:CQp8eyUQZ/Q7PJvnIrB6/hgfTC1kBkGylwsLgOQi1WY=
 sigs.k8s.io/kind v0.11.1 h1:pVzOkhUwMBrCB0Q/WllQDO3v14Y+o2V0tFgjTqIUjwA=
 sigs.k8s.io/kind v0.11.1/go.mod h1:fRpgVhtqAWrtLB9ED7zQahUimpUXuG/iHT88xYqEGIA=
 sigs.k8s.io/kustomize/api v0.8.8/go.mod h1:He1zoK0nk43Pc6NlV085xDXDXTNprtcyKZVm3swsdNY=