Trigger an explicit scale up error when expander filters out all scale-up options #7512
Conversation
k8s-ci-robot
added
kind/cleanup
|
Hi @mtrqq. 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
size/S
|
/assign towca |
| return &status.ScaleUpStatus{ | ||
| Result: status.ScaleUpNoOptionsAvailable, | ||
| PodsRemainUnschedulable: GetRemainingPods(podEquivalenceGroups, skippedNodeGroups), | ||
| Result: status.ScaleUpError, |
There was a problem hiding this comment.
I think we should keep the ScaleUpNoOptionsAvailable status here. The Expander semantics aren't particularly well defined, and we know of existing implementations (at least in the GKE fork) that filter all options out in certain expected scenarios.
We do need more debugability in this scenario though, so the additional logging and correctly setting PodsRemainUnschedulable are very worthwhile additions.
There was a problem hiding this comment.
+1, I think the improvement should more explicitly inform the user of what happened, not necessarily handle it differently. Also no expander option might be an expected outcome, so we should not really log an error here.
| @@ -177,9 +177,10 @@ func (o *ScaleUpOrchestrator) ScaleUp( | |||
| // Pick some expansion option. | |||
| bestOption := o.autoscalingContext.ExpanderStrategy.BestOption(options, nodeInfos) | |||
| if bestOption == nil || bestOption.NodeCount <= 0 { | |||
| klog.Errorf("Expander filtered out all options, valid options: %d (this shouldn't happen)", len(options)) | |||
There was a problem hiding this comment.
Following the other comment, can we actually change the severity to Info here and remove the "this shouldn't happen" part?
| podInfos := []status.NoScaleUpInfo{} | ||
| for _, eg := range egs { | ||
| for _, pod := range eg.Pods { | ||
| noScaleUpInfo := status.NoScaleUpInfo{ |
There was a problem hiding this comment.
NoScaleUpInfo should present a complete explanation why each of the node groups couldn't be scaled up to accommodate a given pod. Normally, each node group should be either in SkippedNodeGroups (if the node group is skipped for all pods) or RejectedNodeGroups (if the node group doesn't pass scheduling predicates for a particular group of pods).
In this case, there are some node groups that aren't in either field (because they weren't skipped and the predicates passed for them). We need to add these node groups to one of the fields. We already do that for the atomic scale-up logic, I think we can just reuse markAllGroupsAsUnschedulable with a new Reason. WDYT?
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mtrqq 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 |
…ons.
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Cluster autoscaler will silently fail the scale-up in case expander was not able to pick the best option, this change adds logs to capture this behavior as well as changes the status of the scale up from ScaleUpNoOptionsAvailable to ScaleUpError
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
This should not affect expanders as BestOption semantics tells us that this interface is supposed to pick the best option out of available ones and not to filter them out completely even if any are available
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: