🐛 Add MachinesUpToDate condition handling in MachinePool controller#13234
🐛 Add MachinesUpToDate condition handling in MachinePool controller#13234ramessesii2 wants to merge 1 commit intokubernetes-sigs:mainfrom
Conversation
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @ramessesii2. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
70ae825 to
0952e72
Compare
| conditions.Set(mp, metav1.Condition{ | ||
| Type: clusterv1.MachinesUpToDateCondition, | ||
| Status: metav1.ConditionFalse, | ||
| Reason: clusterv1.NotUpToDateReason, |
There was a problem hiding this comment.
instead of this, would suggest using MachinePool specific constant. I already see MachineDeploymentMachinesNotUpToDateReason and other existing. If something is not there, you can add a new constant to keep it consistent with others.
There was a problem hiding this comment.
aha! Nice catch! But it looks like it's going to be a separate work as per https://github.com/kubernetes-sigs/cluster-api/blob/main/api/core/v1beta2/machinepool_types.go#L32-L65
bnallapeta
left a comment
There was a problem hiding this comment.
Minor comments. Looks good.
internal/controllers/machinepool/machinepool_controller_status.go
Outdated
Show resolved
Hide resolved
| if !hasMachinePoolMachines { | ||
| // Check if infrastructure is ready by looking at the initialization status | ||
| if mp.Status.Initialization.InfrastructureProvisioned == nil || !*mp.Status.Initialization.InfrastructureProvisioned { | ||
| conditions.Set(mp, metav1.Condition{ |
There was a problem hiding this comment.
I wonder if its also worth adding a message to state that this pool is "machine less" and the infrastructure is not ready. Just to give more context.
There was a problem hiding this comment.
That's a user-facing condition and so, isn't that a provider-specific implementation detail?
It might be useful for devs debugging the controller itself, but then they can go to logs as opposed to users going to the infrastructure resource when they see the infra not provisioned message.
There was a problem hiding this comment.
In that case adding a log with a high verbosity level may help with the debugging then
0952e72 to
adee449
Compare
|
/ok-to-test |
- Updated the updateStatus method to include the new condition
- Added unit tests
- Fixes 10059
Signed-off-by: Satyam Bhardwaj <sbhardwaj@mirantis.com>
adee449 to
fd0e7eb
Compare
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: 942c1eb5fd09168e53e5dddeb3cb8d301e37ab11 |
|
/lgtm |
|
Need help getting this merged now that it has lgtm. cc @fabriziopandini |
What this PR does / why we need it:
MachinePool lacks clear status signal during upgrades. When spec.version changes (generation increments), status.observedGeneration updates immediately to match, but Ready/InfrastructureReady conditions only flip to False ~10s later. See the linked Github issue for more details.
Following the pattern established in
MachineDeployment, this PR adds aMachinesUpToDatecondition toMachinePoolthat provides an immediate, reliable signal for upgrade status:MachineUpToDateConditionfrom individual MachinesInfrastructureProvisionedstatusSee the new condition in the MachinePool status
status: availableReplicas: 1 conditions: - lastTransitionTime: "2026-01-14T12:39:46Z" message: "" observedGeneration: 9 reason: UpToDate status: "True" type: MachinesUpToDate - lastTransitionTime: "2026-01-14T11:36:09Z" message: "" observedGeneration: 9 reason: NotPaused status: "False" type: Paused deprecated: v1beta1: availableReplicas: 1 conditions: - lastTransitionTime: "2026-01-14T12:39:46Z" status: "True" type: Ready - lastTransitionTime: "2026-01-14T11:36:09Z" status: "True" type: BootstrapReady - lastTransitionTime: "2026-01-14T12:39:46Z" status: "True" type: InfrastructureReady - lastTransitionTime: "2026-01-14T11:45:50Z" status: "True" type: ReplicasReady readyReplicas: 1 initialization: bootstrapDataSecretCreated: true infrastructureProvisioned: true nodeRefs: - apiVersion: v1 kind: Node name: aks-pool2-34400930-vmss000000 uid: 953a05d8-8b81-421e-bfca-4c500f3c0f68 observedGeneration: 9 phase: Running readyReplicas: 1 replicas: 1 upToDateReplicas: 1Fixes #10059
/area machinepool