Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trigger an explicit scale up error when expander filters out all scale-up options #7512

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Trigger an explicit scale up error when expander filters out all scale-up options #7512

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions cluster-autoscaler/core/scaleup/orchestrator/orchestrator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the other comment, can we actually change the severity to Info here and remove the "this shouldn't happen" part?

return &status.ScaleUpStatus{
Result: status.ScaleUpNoOptionsAvailable,
PodsRemainUnschedulable: GetRemainingPods(podEquivalenceGroups, skippedNodeGroups),
Result: status.ScaleUpError,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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.

PodsRemainUnschedulable: getAllPods(podEquivalenceGroups, skippedNodeGroups),
ConsideredNodeGroups: nodeGroups,
}, nil
}
Expand Down Expand Up @@ -814,6 +815,23 @@ func GetRemainingPods(egs []*equivalence.PodGroup, skipped map[string]status.Rea
return remaining
}

// ExpandPodGrops flattens all equivalence groups into a list of NoScaleUpInfo
func getAllPods(egs []*equivalence.PodGroup, skipped map[string]status.Reasons) []status.NoScaleUpInfo {
podInfos := []status.NoScaleUpInfo{}
for _, eg := range egs {
for _, pod := range eg.Pods {
noScaleUpInfo := status.NoScaleUpInfo{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Pod: pod,
RejectedNodeGroups: eg.SchedulingErrors,
SkippedNodeGroups: skipped,
}
podInfos = append(podInfos, noScaleUpInfo)
}
}

return podInfos
}

// GetPodsAwaitingEvaluation returns list of pods for which CA was unable to help
// this scale up loop (but should be able to help).
func GetPodsAwaitingEvaluation(egs []*equivalence.PodGroup, bestOption string) []*apiv1.Pod {
Expand Down
Loading