✨ Add MHC remediation logic

[JoelSpeed]

What this PR does / why we need it:

This PR adds the final part of the MHC logic which is responsible to short circuiting remediation if the maxUnhealthy field would be violated by remediating the node and otherwise, remediating the node.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Machine health check remediation logic from #1990

[JoelSpeed]

/cc @vincepri @ncdc for continuity

[k8s-ci-robot]

@JoelSpeed: GitHub didn't allow me to request PR reviews from the following users: for, continuity.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @vincepri @ncdc for continuity

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.

[JoelSpeed]

controllers/machinehealthcheck_targets.go:279:76: `r` can be `k8s.io/client-go/tools/leaderelection/resourcelock.EventRecorder` (interfacer)
func (t *healthCheckTarget) remediate(logger logr.Logger, c client.Client, r record.EventRecorder) error {

The interfacer linter is complaining, but I don't think its suggestion is a good one. Is there a known workaround to tell the linter to ignore this line?

Alternatively, it seems to be deprecated, should it be disabled altogether?

[ncdc]

👀

[chuckha]

super clean code, the logic looks great and thank you for readable tests! There are a few points of feedback to be addressed, but I don't expect them to be very involved

[ncdc]

At some point, we need to decide if MHC should handle e.g. KubeadmControlPlane-owned Machines. Maybe it should handle all Machines that have a controller owner?

[JoelSpeed]

I think this depends on the status of autorecovery of the KubeadmControlPlane owned machines, I had a play with it last week and it seemed if I lost a node, the etcd cluster didn't recover itself? So I decided to leave this like this for now

Once the control plane machines can recover from losing a member, I think this makes sense to add as a feature

[ncdc]

@detiber I know you & I discussed KCP and remediation previously, although I don't recall where we left things. WDYT?

[detiber]

I think it's probably safer to avoid auto remediation on KCP for now, either that or put it behind a feature gate.

As @JoelSpeed mentioned, we would need to likely beef up things with respect to removing etcd members that no longer exist if their backing Machine is removed without a scale down operation.

[ncdc]

A control plane machine should never be owned by a MachineSet. Having it owned by a KubeadmControlPlane would be a more accurate example.

[JoelSpeed]

I've updated this. One point I note now is, the check for a machineset owner comes before the check for a control plane machine (which I think is correct). It made me realise however the control plane check is a little redundant, it will only ever protect against, if it ever happened, a control plane machine being owned by a machineset, which is a very non-standard way of running.

Perhaps the control plane check should be dropped? Though I can also see argument for leaving it in in-case we change the MachineSet requirement in the future, WDYT? (@enxebre do you have any opinions on this?)

[ncdc]

I think I agree with this. We could start by only remediating Machines that belong to a MachineSet (that would be the only check). And pending the decision on KubeadmControlPlane, we could expand it to only remediating Machines that have a controller owner (this would cover MachineSets and presumably all control plane implementations).

[enxebre]

I agree. I think it make sense to address KubeadmControlPlane separately and discuss all sensitive possible scenarios for control plane.

[JoelSpeed]

@ncdc How would you like me to proceed on this comment, should I get rid of the isControlPlane checks and just rely on MachineSet ownership for now?

[ncdc]

I think it's fine to keep the check that excludes control plane machines until we're ready for control plane/etcd remediation.

[ncdc]

The interfacer linter is complaining, but I don't think its suggestion is a good one. Is there a known workaround to tell the linter to ignore this line?

Alternatively, it seems to be deprecated, should it be disabled altogether?

I'm 👍 to disabling. @vincepri WDYT?

[ncdc]

/approve
@chuckha do you want to rereview or would you like @vincepri to take a look?

/hold
so we remember to squash before we lgtm 😄

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed, ncdc

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 [ncdc]

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

[JoelSpeed]

/retest

Now the linter is disabled I can check these tests work

[vincepri]

/milestone v0.3.0

[rudoi]

Will the CRD be added to kustomize / the controller added to main.go in a separate PR?

[JoelSpeed]

Will the CRD be added to kustomize / the controller added to main.go in a separate PR?

Yep, this is all coming soon!

@ncdc Rebased and squashed

[ncdc]

/lgtm

[ncdc]

/hold cancel