Skip to content

fix: treat volcano nominations as non-blocking for provisioning #58

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

Merged
pfernandes21 merged 2 commits into from
Aug 31, 2026
+79 −1
Merged

fix: treat volcano nominations as non-blocking for provisioning #58

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
001aaa0
Treat volcano nominations as non-blocking for provisioning; stop stam…
pfernandes21 Aug 28, 2026
033a92c
Revert Exists-requirement label skip; keep upstream random-value beha…
pfernandes21 Aug 28, 2026
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
6 changes: 6 additions & 0 deletions pkg/controllers/provisioning/scheduling/nodeclaimtemplate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,12 @@ func (i *NodeClaimTemplate) resolveCustomLabelsFromRequirements() map[string]str
if v1.WellKnownLabels.Has(key) || v1.RestrictedLabels.Has(key) || schedulingSimulationKeys.Has(key) {
continue
}
// An Exists requirement left unnarrowed by any pod requirement has no concrete value;
// Any() would invent a random one (e.g. `efa=1490613278757040451`), mislabeling the node
// and matching nothing that selects on a real value. Leave the label unset instead.
if requirement.Operator() == corev1.NodeSelectorOpExists {
continue
}
Comment thread
exa-heron-staging[bot] marked this conversation as resolved.
Outdated
if value := requirement.Any(); value != "" {
labels[key] = value
}
Expand Down
56 changes: 56 additions & 0 deletions pkg/controllers/provisioning/scheduling/nodeclaimtemplate_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
Copyright The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package scheduling

import (
"testing"

corev1 "k8s.io/api/core/v1"

"sigs.k8s.io/karpenter/pkg/scheduling"
)

func TestResolveCustomLabelsFromRequirements(t *testing.T) {
nct := &NodeClaimTemplate{
Requirements: scheduling.NewRequirements(
scheduling.NewRequirement("efa", corev1.NodeSelectorOpExists),
scheduling.NewRequirement("team", corev1.NodeSelectorOpIn, "training"),
),
}

labels := nct.resolveCustomLabelsFromRequirements()

if value, ok := labels["efa"]; ok {
t.Fatalf("expected no label for unnarrowed Exists requirement, got efa=%q", value)
}
if labels["team"] != "training" {
t.Fatalf("expected team=training, got %v", labels)
}
}

func TestResolveCustomLabelsFromRequirementsNarrowedExists(t *testing.T) {
requirements := scheduling.NewRequirements(scheduling.NewRequirement("efa", corev1.NodeSelectorOpExists))
// A pod requirement `efa In [true]` intersects the pool's `efa Exists` down to a concrete value.
requirements.Add(scheduling.NewRequirement("efa", corev1.NodeSelectorOpIn, "true"))
nct := &NodeClaimTemplate{Requirements: requirements}

labels := nct.resolveCustomLabelsFromRequirements()

if labels["efa"] != "true" {
t.Fatalf("expected efa=true from narrowed requirement, got %v", labels)
}
}
12 changes: 11 additions & 1 deletion pkg/utils/pod/scheduling.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,18 @@ func IsScheduled(pod *corev1.Pod) bool {
return pod.Spec.NodeName != ""
}

// VolcanoSchedulerName is the schedulerName Volcano-managed pods carry.
const VolcanoSchedulerName = "volcano"

// IsPreempting checks if a pod is about to schedule onto existing capacity freed by preemption.
// kube-scheduler sets NominatedNodeName only after selecting preemption victims whose deletion
// will free enough capacity for the pod, and clears it when the nomination becomes invalid.
// Volcano also sets it on gang members pipelined behind an eviction while the gang as a whole
// cannot bind (minAvailable unmet), and never clears it, so for volcano-scheduled pods the field
// carries no will-soon-schedule guarantee — treating it as one deadlocks partially-satisfiable
// gangs (the nominated pods never appear provisionable, so the missing nodes are never launched).
func IsPreempting(pod *corev1.Pod) bool {
return pod.Status.NominatedNodeName != ""
return pod.Status.NominatedNodeName != "" && pod.Spec.SchedulerName != VolcanoSchedulerName
Comment on lines 148 to +149

@exa-heron-staging exa-heron-staging Bot Aug 28, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Volcano preemption still has a pre-delete race that launches an extra NodeClaim

🛑 blocking · rule unsafe-side-effect · confidence 0.95

The reply's simulation argument is only true after the preempted victim's deletionTimestamp is visible; this predicate creates a real race before that point. Volcano's source at https://github.com/volcano-sh/volcano/blob/d57d10f4/pkg/scheduler/cache/cache.go starts Evictor.Evict in a goroutine, and that evictor performs a separate status update followed by the pod delete. The same source's taskUnschedulable path then publishes the preemptor's Unschedulable condition and nomination, so there is no atomic guarantee that the victim is already marked terminating when Karpenter observes the nomination. In that interval, the changed line makes the preemptor pass IsProvisionable; pkg/controllers/provisioning/controller.go:67 triggers the batcher, while pkg/controllers/provisioning/provisioner.go:377,380 snapshots the nodes before collecting pending pods. pkg/controllers/provisioning/scheduling/existingnode.go:111 therefore still accounts for the live victim and rejects the nominated node, causing CreateNodeClaims at pkg/controllers/provisioning/provisioner.go:161 to launch capacity. The author's claim that an empty claim is immediately consolidated does not make this safe: consolidation can be disabled (pkg/apis/v1/nodepool.go:93 accepts a nil value and pkg/controllers/disruption/consolidation.go:141 skips it), may be blocked by budgets/validation, and the preemptor can land on the new node before the victim disappears. Thus the acknowledged pre-delete window can leave an unnecessary node (or move the preemptor onto it) during every genuine Volcano preemption, not merely the gang-pipelining case. Preserve the distinction at a caller with access to the nominated node/victim state (or consume an explicit Volcano preemption signal), and add a test covering nomination plus a still-active victim; the current tests only assert the blanket Volcano exception at pkg/utils/pod/suite_test.go:262-267,289-297.

Suggested fix:

Do not make every Volcano nomination provisionable. Gate the exception on scheduler-specific evidence that the nomination is a gang pipeline rather than a live preemption, or defer provisioning until the nominated node's actual preemption victim is observed terminating; validate this with an end-to-end test where the nomination is observed before the victim delete.

Heron review global-review-orchestrator-guardian · fingerprint c3da29da0549 · reply @heron dismiss <reason> to dismiss

@devin-ai-integration devin-ai-integration Bot Aug 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The duplicate-provisioning window during a genuine Volcano preemption is covered by Karpenter's own scheduling simulation rather than by this predicate: eviction sets the victim's deletionTimestamp, terminating pods fail IsReschedulable (IsActive is false), so they don't occupy capacity when the provisioner simulates existing nodes — the preemptor simulates as fitting the nominated node and no NodeClaim is created. The residual race is only the gap between Volcano writing the nomination and the victim's delete landing, bounded by the provisioner batch window; a spurious node from that race is empty and consolidated immediately on our pools (consolidateAfter: 0s).

Distinguishing "real preemption" from "gang-pipelined" nominations from pod status alone isn't possible: Volcano writes the same field in both flows (taskUnschedulable only gates on EvictionOccurred, which is true for both) and never clears it, so any Volcano nomination may be stale regardless of how it originated. The upstream guard's assumption (nomination ⇒ will bind there without new capacity) simply doesn't hold for this scheduler.

@devin-ai-integration devin-ai-integration Bot Aug 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The pre-delete race window is real but bounded and accepted: it lasts from the nomination status write until the victim's deletionTimestamp is visible (Volcano's evictor issues the delete in the same goroutine flow, so this is sub-second in practice, and the provisioner batcher adds its own window before snapshotting). The worst case is one extra node during a genuine preemption; on the clusters that run Volcano (delphi) every pool has consolidation enabled with consolidateAfter: 0s, so a spurious node is reaped — the generic "consolidation can be disabled" caveat doesn't apply to this fork's deployment.

The suggested gate ("defer until the victim is observed terminating") doesn't separate the cases: the gang-pipeline deadlock state also has a real evicted victim (EvictionOccurred is true in both flows) — after the victim exits, the gang still can't bind (minAvailable unmet) and the stale nomination persists forever. There is no pod-status signal that distinguishes "live preemption" from "stale/pipelined"; any victim-state gate reintroduces the deadlock, which strands multi-node gangs indefinitely — strictly worse than a transient extra node. Accepting as a deliberate tradeoff.

}

func IsPending(pod *corev1.Pod) bool {
Expand Down
68 changes: 68 additions & 0 deletions pkg/utils/pod/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,74 @@ var _ = Describe("IsDisruptable", func() {
})
})

var _ = Describe("IsPreempting", func() {
It("should return false when no node is nominated", func() {
p := &corev1.Pod{}
Expect(pod.IsPreempting(p)).To(BeFalse())
})

It("should return true for a kube-scheduler pod nominated to a node", func() {
p := &corev1.Pod{
Spec: corev1.PodSpec{SchedulerName: "default-scheduler"},
Status: corev1.PodStatus{NominatedNodeName: "node-a"},
}
Expect(pod.IsPreempting(p)).To(BeTrue())
})

It("should return false for a volcano-scheduled pod nominated to a node", func() {
p := &corev1.Pod{
Spec: corev1.PodSpec{SchedulerName: pod.VolcanoSchedulerName},
Status: corev1.PodStatus{NominatedNodeName: "node-a"},
}
Expect(pod.IsPreempting(p)).To(BeFalse())
})
})

var _ = Describe("IsProvisionable", func() {
unschedulable := corev1.PodCondition{
Type: corev1.PodScheduled,
Status: corev1.ConditionFalse,
Reason: corev1.PodReasonUnschedulable,
}

It("should exclude a kube-scheduler pod nominated to a node", func() {
p := &corev1.Pod{
Spec: corev1.PodSpec{SchedulerName: "default-scheduler"},
Status: corev1.PodStatus{
Conditions: []corev1.PodCondition{unschedulable},
NominatedNodeName: "node-a",
},
}
Expect(pod.IsProvisionable(p)).To(BeFalse())
})

It("should include a volcano-scheduled pod nominated to a node", func() {
p := &corev1.Pod{
Spec: corev1.PodSpec{SchedulerName: pod.VolcanoSchedulerName},
Status: corev1.PodStatus{
Conditions: []corev1.PodCondition{unschedulable},
NominatedNodeName: "node-a",
},
}
Expect(pod.IsProvisionable(p)).To(BeTrue())
})

It("should exclude a volcano-scheduled pod whose PodScheduled reason is not Unschedulable", func() {
p := &corev1.Pod{
Spec: corev1.PodSpec{SchedulerName: pod.VolcanoSchedulerName},
Status: corev1.PodStatus{
Conditions: []corev1.PodCondition{{
Type: corev1.PodScheduled,
Status: corev1.ConditionFalse,
Reason: "Schedulable",
}},
NominatedNodeName: "node-a",
},
}
Expect(pod.IsProvisionable(p)).To(BeFalse())
})
})

var _ = Describe("HasDRARequirements", func() {
It("should return false when the pod references no ResourceClaims", func() {
p := &corev1.Pod{
Expand Down
Loading