feat: ca: do not backoff scale up on specified errors #7777
Conversation
k8s-ci-robot
added
kind/feature
|
|
k8s-ci-robot
added
the
cncf-cla: no
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Chase-Marino 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 |
|
Welcome @Chase-Marino! |
|
Hi @Chase-Marino. 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
needs-ok-to-test
Chase-Marino
changed the title
k8s-ci-robot
added
cncf-cla: yes
| maxNodeProvisionTime = flag.Duration("max-node-provision-time", 15*time.Minute, "The default maximum time CA waits for node to be provisioned - the value can be overridden per node group") | ||
| maxPodEvictionTime = flag.Duration("max-pod-eviction-time", 2*time.Minute, "Maximum time CA tries to evict a pod before giving up") | ||
| nodeGroupsFlag = multiStringFlag( | ||
| maxTotalUnreadyPercentage = flag.Float64("max-total-unready-percentage", 45, "Maximum percentage of unready nodes in the cluster. After this is exceeded, CA halts operations") |
There was a problem hiding this comment.
the whitespace change here is a bit sus and could result in merge conflicts, see if that can be avoided
There was a problem hiding this comment.
weird doesn't show up on my local will check it out later
| backoff := true | ||
| for _, part := range e.autoscalingContext.AutoscalingOptions.ScaleUpIgnoreBackoffErrors { | ||
| if strings.Contains(err.Error(), part) { | ||
| e.autoscalingContext.LogRecorder.Eventf(apiv1.EventTypeWarning, "ScaledUpGroup", "Scale-up: retriable error %s", aerr.Error()) |
There was a problem hiding this comment.
would fit better into the else of if backoff
|
|
||
| if backoff { | ||
| e.autoscalingContext.LogRecorder.Eventf(apiv1.EventTypeWarning, "FailedToScaleUpGroup", "Scale-up failed for group %s: %v", info.Group.Id(), err) | ||
| e.scaleStateNotifier.RegisterFailedScaleUp(info.Group, string(aerr.Type()), aerr.Error(), gpuResourceName, gpuType, now) |
There was a problem hiding this comment.
FYI we might want to still call this to get these bumped:
csr.scaleUpFailures[nodeGroup.Id()] = append(csr.scaleUpFailures[nodeGroup.Id()], ScaleUpFailure{NodeGroup: nodeGroup, Reason: reason, Time: currentTime})
metrics.RegisterFailedScaleUp(reason, gpuResourceName, gpuType)
but not call the csr.backoffNodeGroup
... RegisterFailedScaleUp says "It will mark this group as not safe to autoscale", so maybe avoiding it completely is the right call, but not sure how the metrics and failures are used
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
What type of PR is this?
/kind feature
Which component this PR applies to?
cluster-autoscaler
What this PR does / why we need it:
This pr adds an option to provide errors to ignore during scaleup. When my cluster needs to scale up and there is a throttling error, I want to continue retrying.
Currently this problem can result in asgs scaling inbalanced or even refusing to scale at all if it hits all asgs, and aws throttling can block scaleup for 10m+
Which issue(s) this PR fixes:
#5271
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
-->