✨ Remediate unhealthy MachinePool machines #11392
Conversation
k8s-ci-robot
added
area/machinepool
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
needs-rebase
|
|
||
| // Calculate how many in flight machines we should remediate. | ||
| // By default, we allow all machines to be remediated at the same time. | ||
| maxInFlight := len(unhealthyMachines) |
There was a problem hiding this comment.
You'll find this mostly copied from the MachineSet remediation code, except that I don't like changing the meaning of a variable throughout the function, so I split up inFlight/maxInFlight variables.
| @@ -1299,7 +1299,10 @@ func (r *Reconciler) reconcileUnhealthyMachines(ctx context.Context, s *scope) ( | |||
| for _, m := range machinesToRemediate { | |||
| log.Info("Deleting unhealthy Machine", "Machine", klog.KObj(m)) | |||
| patch := client.MergeFrom(m.DeepCopy()) | |||
| if err := r.Client.Delete(ctx, m); err != nil && !apierrors.IsNotFound(err) { | |||
| if err := r.Client.Delete(ctx, m); err != nil { | |||
There was a problem hiding this comment.
Small bugfix that I took over from my code: don't go on with conditions.MarkTrue/Patch if the object doesn't exist
AndiDog
force-pushed
the
machine-pool-machine-remediation
branch
from
792b24a
to
a7fc58f
Compare
k8s-ci-robot
removed
the
needs-rebase
|
Not sure what's up with the build. I ran |
k8s-ci-robot
added
the
needs-rebase
|
Before looking into this PR It would be great to better understand what is the overall status of MachinePool machines.
Also, probably worth to think about E2E test coverage of this feature; however, the key decision to be taken is if we want to keep adding MP tests to existing test pipelines or if we want to have dedicated test pipelines for MP. |
|
I have concerns that adding separate dedicated test pipelines for MP will increase the maintenance effort for all of us. Instead I would simply disable MachinePools for e2e tests where it leads to flakiness (until the flakes are fixed). |
Ideally those should be maintained by folks taking care of MP, but I agree with you that without a clear commitment from a small set of new maintainers, this could become an additional burden for the existing maintainers. |
AndiDog
force-pushed
the
machine-pool-machine-remediation
branch
from
a7fc58f
to
2f706f2
Compare
k8s-ci-robot
removed
the
needs-rebase
|
@AndiDog: The following tests failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
|
@fabriziopandini @sbueringer can we start a chat thread about flaky tests? I couldn't find exactly which ones those are. Regarding this PR, I get strange differences from a local |
What this PR does / why we need it:
With infra providers implementing the MachinePool machines specification, users want to use
MachineHealthCheckto select, check and remediate single machines within a machine pool. Right now, onlyKubeadmControlPlaneandMachineSetmachines get deleted once they are marked unhealthy by theMachineHealthCheckcontroller.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Part of #9005 because this is a major gap of machine pool machines support.
Related to CAPA implementation for AWSMachinePool AWSMachines (PR), since that's the provider where we want to use the remediation feature first, as soon as possible. If both PRs have good chances of getting accepted, my company will integrate them first into their fork and test them extensively.
/area machinepool