scheduler: read sla-waiting-time from member pods too - #5
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 4 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 51ac4f3. Configure here.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| candidateNodes := predicateNodes | ||
| if len(candidateNodes) == 0 { | ||
| unresolvable := fitErrors.GetUnschedulableAndUnresolvableNodes() | ||
| for _, n := range allNodes { | ||
| if _, skip := unresolvable[n.Name]; !skip { | ||
| candidateNodes = append(candidateNodes, n) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Preemption can evict running pods on nodes where the waiting pod can never run
When no node passes the preemption check, the fallback list of nodes to consider (built at pkg/scheduler/actions/preempt/preempt.go:254-262) keeps exactly the nodes that failed with an internal error or a "try later" verdict, so running pods can be killed on nodes that still cannot host the waiting pod.
Impact: Healthy workloads may be evicted for a pod that then still cannot be placed, losing work for no benefit.
Why the fallback selects error nodes instead of the intended busy nodes
predicateFn here is ssn.PredicateForPreemptAction, which returns an error only when the node status contains UnschedulableAndUnresolvable or ErrorSkipOrWait (pkg/scheduler/framework/session.go:441-460). Nodes that are merely resource-constrained (Unschedulable) return nil and therefore already land in predicateNodes.
Consequently len(candidateNodes) == 0 only happens when every node failed with unresolvable-or-error status. The fallback then filters out only the UnschedulableAndUnresolvable names via fitErrors.GetUnschedulableAndUnresolvableNodes(), so what remains are precisely the Error/Skip/Wait nodes — the opposite of the comment's stated intent. Those nodes are then scored and victims are evicted on them, while the final gate only checks that resources fit (preemptor.InitResreq.LessEqual(node.FutureIdle(), ...)), not that the predicate passes.
The same construction was added to reclaim (pkg/scheduler/actions/reclaim/reclaim.go:148-171), where nodes are tracked as unresolvable only when the error is a *api.FitError with ContainsUnschedulableAndUnresolvable(), so error/wait nodes are likewise promoted into the candidate list and their pods can be reclaimed.
Prompt for agents
In pkg/scheduler/actions/preempt/preempt.go and pkg/scheduler/actions/reclaim/reclaim.go a fallback candidate-node list was added for the case where no node passes the preemption predicate. The stated intent is to still consider nodes that are only resource-constrained (status Unschedulable). However ssn.PredicateForPreemptAction (pkg/scheduler/framework/session.go) already returns nil for Unschedulable-only nodes, so such nodes are always part of predicateNodes; the fallback list therefore ends up containing exactly the nodes that failed with UnschedulableAndUnresolvable-excluded reasons, i.e. Error/Skip/Wait nodes. Preempting/reclaiming on those nodes evicts victims although the pod still cannot be placed there (the final gate only checks resources fit, not the predicate). Please re-evaluate whether the fallback is needed at all; if some other filtering was intended (e.g. keeping nodes whose only failure is a resource shortage that preemption can fix), derive the set from the actual per-node statuses instead of 'everything that is not unresolvable', and make sure nodes with Error/Skip/Wait statuses are never used as preemption/reclaim targets.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Not part of this change. The branch was based on a stale exa/master, so the diff included unrelated fork commits (this preempt/reclaim fallback is one of them, already on exa/master). Rebased in f89902e — worth raising against the commit that introduced it if the analysis holds.
| if !reflect.DeepEqual(newPodGroup.Labels, podGroup.Labels) { | ||
| podGroup.Labels = newPodGroup.Labels | ||
| isUpdated = true | ||
| } | ||
|
|
||
| if !reflect.DeepEqual(newPodGroup.Annotations, podGroup.Annotations) { | ||
| podGroup.Annotations = newPodGroup.Annotations | ||
| isUpdated = true | ||
| } | ||
|
|
||
| return isUpdated | ||
| } |
There was a problem hiding this comment.
🟡 Scheduling information the scheduler stores on auto-created pod groups is repeatedly erased
Every field of an existing auto-created pod group is overwritten with a freshly derived copy (podGroup.Annotations = newPodGroup.Annotations at pkg/controllers/podgroup/pg_controller_handler.go:465-468) whenever the owning StatefulSet changes, so notes the scheduler wrote there are lost.
Impact: Network-topology placement decisions for StatefulSet workloads are dropped and rewritten repeatedly, causing needless churn; a queue assigned directly on the pod group is also reset.
How the wholesale overwrite loses scheduler state
addStatefulSet now calls createOrUpdateNormalPodPG (pkg/controllers/podgroup/pg_controller_handler.go:200), which for an existing PodGroup calls shouldUpdateExistingPodGroup. That helper rebuilds the PodGroup purely from the pod (buildPodGroupFromPod) and replaces Spec, Labels and Annotations in full when they differ.
The scheduler persists volcano.sh/job-allocated-hypernode onto the PodGroup in soft network-topology mode (pkg/scheduler/actions/allocate/allocate.go:297, persisted via pkg/scheduler/framework/job_updater.go:106-119). Since the rebuilt annotation map never contains that key, each StatefulSet add/update event (including frequent status-only updates) removes it, and the next scheduling session writes it back — a permanent ping-pong.
Similarly newPodGroup.Spec.Queue is empty unless the pod carries scheduling.QueueNameAnnotationKey, so a queue set directly on the PodGroup is reset to empty on the next StatefulSet event.
Prompt for agents
pgcontroller.shouldUpdateExistingPodGroup (pkg/controllers/podgroup/pg_controller_handler.go) rebuilds a PodGroup from the pod and replaces Spec/Labels/Annotations wholesale on the existing object. Fields that are owned by other components are therefore dropped: the scheduler writes volcano.sh/job-allocated-hypernode onto PodGroups in soft network-topology mode (see pkg/scheduler/actions/allocate/allocate.go and pkg/scheduler/framework/job_updater.go), and Spec.Queue may have been set directly on the PodGroup while the rebuilt value is empty when the pod has no queue annotation. Because addStatefulSet fires on every StatefulSet add/update (including status-only updates), this results in repeated stripping/rewriting. Consider reconciling only the fields the controller actually owns (e.g. MinMember, MinResources, PriorityClassName, and the specific annotations/labels it derives from the pod) and merging rather than replacing the annotation/label maps, leaving unknown keys and a non-empty existing queue untouched.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Not part of this change — stale base, since rebased in f89902e. This podgroup-controller reconcile is already on exa/master.
| - apiGroups: [ "" ] | ||
| resources: [ "nodes" ] | ||
| verbs: [ "list", "watch" ] |
There was a problem hiding this comment.
🟨 Controller service account granted cluster-wide node list/watch permissions
The controller ClusterRole gains cluster-wide list/watch on core nodes (installer/helm/chart/volcano/templates/controllers.yaml:91-93, mirrored in the generated installer/volcano-development.yaml:4488-4490). This is needed by the new HyperNode node-count logic (pkg/controllers/hypernode/hypernode_handler.go uses nodeLister), so the grant itself is justified, but it does broaden the controller's read surface to all node objects (labels, addresses, capacity) for any workload able to reach the controller's service account token.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Not part of this change — stale base, since rebased in f89902e. This RBAC grant comes with the hypernode node-count work already on exa/master.
The sla plugin lets a job declare a bounded waiting time after which it is permitted to hold its partial allocation instead of having it discarded, which is the only mechanism by which a wide gang can accumulate nodes that free up one at a time. Reaching it requires the annotation on the PodGroup, but PodGroups of operator-managed gangs (kubeflow PyTorchJob, and anything else that creates its own PodGroup) are not built from the pod template, so a workload that annotates its pods has no way to opt in. Resolve the waiting time from the job's member pods when the PodGroup does not declare one, in whichever order the two reach the cache. The PodGroup annotation stays authoritative and jobs that declare nothing keep a nil waiting time, so the sla plugin still abstains on them. Also fold the two duplicated per-key extraction paths into one helper that logs and skips unusable values rather than returning an error the caller discards. Tests cover the extraction precedence and, in allocate, the behavior this exists for: a four member gang facing two free nodes has its placements discarded and loses the nodes to a single pod job, unless an aged sla opt-in permits it from a tier ahead of gang. Assisted-by: devin:claude-sonnet-4.5
Caching the pod-declared waiting time on JobInfo.WaitingTime the first time a task was added had two problems: the cache handles a pod update as a delete followed by an add and nothing recomputed the field, so an updated annotation never took effect; and the value was whichever pod the task map happened to yield first, so members declaring different values gave a duration that could change between sessions. Keep the field meaning what it did upstream — the PodGroup's own annotation — and have the sla plugin read GetWaitingTime(), which falls back to the shortest waiting time declared by a member pod. Resolving on read cannot go stale, and taking the shortest makes it independent of iteration order. Assisted-by: devin:claude-sonnet-4.5 Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
51ac4f3 to
f89902e
Compare
|
CI notes, neither traceable to this change:
The rest of the scheduler and controller unit tests pass, including the new |
Runtime validation in a local kind clusterVolcano has no PR preview cluster, so I validated this in kind: 1 control-plane + 4 workers of 8 CPU, pods requesting 5 CPU so exactly one fits per worker. Scheduler/controller/webhook built from
110s is the optimum — the 4th node only frees at T+100. Run C is what isolates this change: putting Scheduler log: discard (before) vs permit (after)Before — the gang does place a member on each freed node, then loses it (once per freed node, 4×): After — the same situation ends in a permit, so the placement survives the session: The annotation really is pod-onlyMixed member waiting times: shortest wins (verified at runtime)Members declaring different values — the part that only had unit tests — is now covered in kind, with the loaded scheduler config verified live from inside the pod before each run and zero config reloads during any of them.
The all- Not covered: malformed / One "after" run initially starved — it was the test harness, not the schedulerIn the first round, 1 of 6 "after" runs starved because the
The failing run's own log shows With the config verified live before the run, Not verified anywhere in this round: GPUs, real node shapes, hypernode/network-topology paths, and quota plugins (single |
1 participant
What type of PR is this?
/kind feature
What this PR does / why we need it:
A gang whose
minMemberdoes not fit in a single scheduling session has its partial placements rolled back —allocatecallsstmt.Discard()unless someJobPipelinedFnpermits the job, and gang's own vote rejects anything belowminMember:So on a busy cluster a wide gang cannot accumulate nodes that free up one at a time: each session it grabs whatever is free, releases it, and the next single-node job takes it. The
slaplugin is the escape hatch — a job older than itssla-waiting-timegets aPermit, the discard is skipped, and the partial placement holds the nodes for the rest of the session — but only reachable if (a)slasits in a tier ahead of gang, sinceJobPipelinedstops at the first rejecting vote, and (b) the waiting time actually reaches the scheduler.(b) is what this PR fixes.
JobInfo.WaitingTimeis read only fromPodGroup.Annotations, and PodGroups of operator-managed gangs are not built from the pod template — the kubeflow training-operator constructs its own PodGroup, as does anything else implementing gang scheduling itself — so a workload that annotates its pods has no way to opt in. This resolves the waiting time from the job's member pods when the PodGroup declares none, in whichever order the PodGroup and pods reach the cache:The PodGroup annotation stays authoritative, and a job that declares nothing keeps a
nilwaiting time, soslastill abstains on it and nothing changes for workloads that do not opt in.The two duplicated per-key extraction paths (
volcano.sh/sla-waiting-timethen baresla-waiting-time) collapse into one helper. Behavior change: an unparseable or non-positive value on the prefixed key now falls through to the bare key instead of aborting the lookup, and is logged rather than returned as an error the caller discarded anyway.Which issue(s) this PR fixes:
NONE
Special notes for your reviewer:
Tests are the point of the change as much as the code is:
pkg/scheduler/api: extraction precedence — prefixed over bare key, PodGroup over pods, unusable values ignored, pods-before-PodGroup arrival order.pkg/scheduler/actions/allocate: the behavior above, end to end. A four member gang faces two free nodes plus a single pod job that fits one of them. Withoutslathe gang holds 0 nodes and the single pod job binds; with an agedslaopt-in one tier ahead of gang the gang holds both nodes and the single pod job waits; withslain gang's own tier gang's reject wins and the gang holds 0 again; a gang with no annotation is unaffected.Note the cost this makes visible: a reserving gang idles the nodes it has not filled yet, which is why the opt-in is per job and bounded by a waiting time rather than being on by default.
Does this PR introduce a user-facing change?
Link to Devin session: https://app.devin.ai/sessions/9af6fa5ec7b043caa9cdb0990e676419
Requested by: @jld-adriano
Note
Medium Risk
Changes gang scheduling reservation behavior for jobs that annotate pods for SLA; impact is opt-in but affects allocate pipelining and node holding on busy clusters.
Overview
SLA waiting time can now be picked up from member pod annotations when the PodGroup does not declare one, so operator-managed gangs (e.g. Kubeflow training-operator) can opt into the sla plugin via
volcano.sh/sla-waiting-timeon pods.JobInfogainsGetWaitingTime()(PodGroup wins; otherwise shortest valid duration among tasks, resolved on read).SetPodGrouponly stores the PodGroup value; waiting-time parsing is centralized inextractWaitingTime, which prefers the prefixed key over the bare key and falls through on unparseable or non-positive values instead of stopping at the first bad key. The sla plugin’s job ordering and pipelined/enqueue permits now callGetWaitingTime()instead of the cached field alone.New tests cover annotation precedence and arrival order in
job_info_sla_test, andallocate_sla_testexercises partial gang placement vs a smaller job with and without sla tier placement.Reviewed by Cursor Bugbot for commit f89902e. Bugbot is set up for automated code reviews on this repo. Configure here.