ClusterAPI: Report machine phases to improve cluster-autoscaler decisions #7989
Conversation
|
|
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @loick111! |
k8s-ci-robot
added
needs-ok-to-test
|
Hi @loick111. Thanks for your PR. I'm waiting for a kubernetes 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. 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. |
k8s-ci-robot
added
the
area/provider/cluster-api
k8s-ci-robot
added
size/L
| // This helps cluster-autoscaler make better scaling decisions. | ||
| switch { | ||
| case isFailedMachineProviderID(providerIDNormalized): | ||
| klog.V(4).Infof("Machine failed in %s (%s)", ng.Id(), providerID) |
There was a problem hiding this comment.
nit: can we say Machine failed in node group %s (%s)...
for all 3 log instances here
| case isFailedMachineProviderID(providerIDNormalized): | ||
| klog.V(4).Infof("Machine failed in %s (%s)", ng.Id(), providerID) | ||
| instance.Status = &cloudprovider.InstanceStatus{ | ||
| State: cloudprovider.InstanceCreating, |
There was a problem hiding this comment.
Is there really no other possible instance state than "InstanceCreating" when the machine is failed?
There was a problem hiding this comment.
You're right, we can have machine failed for multiple reasons.
cf. https://cluster-api.sigs.k8s.io/developer/core/controllers/machine
I handled the reporting of Status only when machine are Failed during Provisioning & Deleting phases as I think cluster-autoscaler doesn't have anything to do when Running machine goes into Failed state, it will be more under machine-controller responsibility. (by keeping the same behavior than before, not reporting any Status to core cluster-autoscaler)
There was a problem hiding this comment.
This seems like a fair evolution from the current behaviors, thx!
Add improved error handling for machines phase in the ClusterAPI node group implementation. When a machine is in Deleting/Failed/Pending phase, mark the cloudprovider.Instance with a status for cluster-autoscaler recovery actions. The changes: - Enhance Nodes listing to allow reporting the machine phase in Instance status - Add error status reporting for failed machines This change helps identify and manage failed machines more effectively, allowing the autoscaler to make better scaling decisions.
loick111
force-pushed
the
feature/clusterapi-instances-status
branch
from
7e3c844 to
005a42b
Compare
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis, loick111 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What type of PR is this?
/kind bug
What this PR does / why we need it
Following the changes done in #7950, this PR improves the node listing process in the
cluster-autoscalerClusterAPI provider by reporting the machine phase in theInstancestatus. This allows the cluster-autoscaler to make better scaling decisions without having to wait for a timeout. Specifically:Deleting,Failed, orPendingstates are now reported properly.priority expander, as it prevents unnecessary delays before scaling the next priority group.Which issue(s) this PR fixes
Related to #6128
Related to #7928
Special notes for your reviewer
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: