Stabilize predicates plugin execution order and rollback semantics. by wangyang0616 · Pull Request #5259 · volcano-sh/volcano

wangyang0616 · 2026-04-28T16:00:26Z

What type of PR is this?

This change updates the predicates plugin to use explicit execution order instead of map iteration:

Add per-stage plugin order slices in PredicatesPlugin (Filter / StableFilter / PreFilter / Reserve / PreBind / Score).
Record plugin execution order during InitPlugin() registration via unified addXxxPlugin helpers.
Replace direct range map execution with ordered-slice iteration across plugin stages.
Update rollback paths (runUnReservePlugins, PreBindRollBack) to call Unreserve in reverse reserveOrder, ensuring symmetric reserve/rollback behavior.

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #5249

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Signed-off-by: wangyang0616 <wangyang8126@gmail.com>

gemini-code-assist

Code Review

This pull request introduces deterministic execution order for scheduler plugins by adding explicit order slices to the PredicatesPlugin struct and updating the plugin initialization and execution logic to use these slices. Feedback was provided regarding the new unit tests, which initialize plugin maps but fail to populate the corresponding order slices, potentially leading to incorrect test behavior.

gemini-code-assist · 2026-04-28T16:03:17Z

+	pp.StableFilterPlugins = map[string]k8sframework.FilterPlugin{
+		nodeunschedulable.Name: &fakeFilterPlugin{name: nodeunschedulable.Name, message: "stable-nodeunschedulable"},
+		nodeaffinity.Name:      &fakeFilterPlugin{name: nodeaffinity.Name, message: "stable-nodeaffinity"},
+		tainttoleration.Name:   &fakeFilterPlugin{name: tainttoleration.Name, message: "stable-tainttoleration"},
+	}
+	pp.FilterPlugins = map[string]k8sframework.FilterPlugin{
+		nodeunschedulable.Name: &fakeFilterPlugin{name: nodeunschedulable.Name, message: "stable-nodeunschedulable"},
+		nodeaffinity.Name:      &fakeFilterPlugin{name: nodeaffinity.Name, message: "stable-nodeaffinity"},
+		nodeports.Name:         &fakeFilterPlugin{name: nodeports.Name, message: "normal-nodeports"},
+		tainttoleration.Name:   &fakeFilterPlugin{name: tainttoleration.Name, message: "stable-tainttoleration"},
+	}


The test TestPredicateFailureReasonAggregationOrderStable initializes FilterPlugins and StableFilterPlugins maps directly, but it doesn't initialize the corresponding filterPluginOrder and stableFilterOrder slices. The Predicate function now iterates over these slices to ensure stable execution order. Without these slices being populated, the predicate plugins won't be executed, and the test will not behave as intended.

To fix this, you should also initialize the order slices to match the intended execution order you want to test.

pp.StableFilterPlugins = map[string]k8sframework.FilterPlugin{ nodeunschedulable.Name: &fakeFilterPlugin{name: nodeunschedulable.Name, message: "stable-nodeunschedulable"}, nodeaffinity.Name: &fakeFilterPlugin{name: nodeaffinity.Name, message: "stable-nodeaffinity"}, tainttoleration.Name: &fakeFilterPlugin{name: tainttoleration.Name, message: "stable-tainttoleration"}, } pp.FilterPlugins = map[string]k8sframework.FilterPlugin{ nodeunschedulable.Name: &fakeFilterPlugin{name: nodeunschedulable.Name, message: "stable-nodeunschedulable"}, nodeaffinity.Name: &fakeFilterPlugin{name: nodeaffinity.Name, message: "stable-nodeaffinity"}, nodeports.Name: &fakeFilterPlugin{name: nodeports.Name, message: "normal-nodeports"}, tainttoleration.Name: &fakeFilterPlugin{name: tainttoleration.Name, message: "stable-tainttoleration"}, } pp.stableFilterOrder = []string{ nodeunschedulable.Name, nodeaffinity.Name, tainttoleration.Name, } pp.filterPluginOrder = []string{ nodeunschedulable.Name, nodeaffinity.Name, nodeports.Name, tainttoleration.Name, }

gemini-code-assist · 2026-04-28T16:03:17Z

+	pp.ReservePlugins = map[string]k8sframework.ReservePlugin{
+		vbcap.Name:            &fakeReservePlugin{name: vbcap.Name, calls: &calls},
+		dynamicresources.Name: &fakeReservePlugin{name: dynamicresources.Name, calls: &calls},
+	}


Similar to the TestPredicateFailureReasonAggregationOrderStable test, TestReserveRollbackOrderStable initializes the ReservePlugins map but does not set the reserveOrder slice. The runReservePlugins and PreBindRollBack functions rely on this slice for ordered execution and rollback. Without it, no reserve or unreserve operations will be performed, and the test will fail its assertion.

Please initialize reserveOrder to ensure the test correctly verifies the reservation and rollback logic.

Suggested change

pp.ReservePlugins = map[string]k8sframework.ReservePlugin{

vbcap.Name: &fakeReservePlugin{name: vbcap.Name, calls: &calls},

dynamicresources.Name: &fakeReservePlugin{name: dynamicresources.Name, calls: &calls},

}

pp.ReservePlugins = map[string]k8sframework.ReservePlugin{

vbcap.Name: &fakeReservePlugin{name: vbcap.Name, calls: &calls},

dynamicresources.Name: &fakeReservePlugin{name: dynamicresources.Name, calls: &calls},

}

pp.reserveOrder = []string{vbcap.Name, dynamicresources.Name}

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR makes predicates plugin execution deterministic by replacing Go map iteration with explicit, recorded per-stage plugin orders, and ensures rollback semantics match reserve semantics by unreserving in reverse order.

Changes:

Add per-stage order slices (Filter/StableFilter/PreFilter/Reserve/PreBind/Score) to PredicatesPlugin and populate them during InitPlugin().
Switch all plugin-stage executions to iterate in the recorded order rather than ranging over maps.
Update rollback paths to unreserve in reverse reserveOrder, aligning reserve/unreserve behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.

File	Description
pkg/scheduler/plugins/predicates/predicates.go	Introduces explicit plugin ordering for deterministic execution and reverses unreserve order for symmetric rollback.
pkg/scheduler/plugins/predicates/predicates_test.go	Adds tests intended to validate stable failure-reason aggregation order and reverse unreserve rollback order.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+	}
+	pp.FilterPlugins = map[string]k8sframework.FilterPlugin{
+		nodeunschedulable.Name: &fakeFilterPlugin{name: nodeunschedulable.Name, message: "stable-nodeunschedulable"},
+		nodeaffinity.Name:      &fakeFilterPlugin{name: nodeaffinity.Name, message: "stable-nodeaffinity"},
+		nodeports.Name:         &fakeFilterPlugin{name: nodeports.Name, message: "normal-nodeports"},
+		tainttoleration.Name:   &fakeFilterPlugin{name: tainttoleration.Name, message: "stable-tainttoleration"},
+	}


+		"normal-nodeports",
+	}
+	for i := 0; i < 20; i++ {
+		err := pp.Predicate(task, volcanoNode, schedframework.NewCycleState())


+	pp.ReservePlugins = map[string]k8sframework.ReservePlugin{
+		vbcap.Name:            &fakeReservePlugin{name: vbcap.Name, calls: &calls},
+		dynamicresources.Name: &fakeReservePlugin{name: dynamicresources.Name, calls: &calls},
+	}


+	task := api.NewTaskInfo(pod)
+
+	event := &framework.Event{Task: task}
+	pp.runReservePlugins(&framework.Session{}, event)


 	"k8s.io/client-go/informers"
 	k8sfake "k8s.io/client-go/kubernetes/fake"
 	k8sframework "k8s.io/kube-scheduler/framework"
+	schedframework "k8s.io/kubernetes/pkg/scheduler/framework"


+			},
+		},
+	}
+	k8sNodeInfo := schedframework.NewNodeInfo()


+		"normal-nodeports",
+	}
+	for i := 0; i < 20; i++ {
+		err := pp.Predicate(task, volcanoNode, schedframework.NewCycleState())


+
+	bindCtx := &cache.BindContext{
+		TaskInfo:   task,
+		Extensions: map[string]cache.BindContextExtension{pp.Name(): &BindContextExtension{State: schedframework.NewCycleState()}},


 	// Run all Filter plugins (except those in StableFilterPlugins)
-	for name, plugin := range pp.FilterPlugins {
+	for _, name := range pp.filterPluginOrder {
+		plugin, exists := pp.FilterPlugins[name]
+		if !exists {
+			continue
+		}


Normalize the predicates plugin field names for external access, align PreFilter-related naming with Kubernetes plugin terminology, and centralize reverse unreserve iteration in a shared helper to keep rollback behavior consistent. Signed-off-by: wangyang0616 <wangyang8126@gmail.com>

Signed-off-by: wangyang0616 <wangyang8126@gmail.com>

JesseStutler · 2026-05-06T07:40:56Z

/cc
Will take a look this week, thanks

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

JesseStutler · 2026-05-08T01:42:50Z

/approve
/lgtm
Thanks!

volcano-sh-bot · 2026-05-08T01:42:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JesseStutler

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/scheduler/plugins/OWNERS~~ [JesseStutler]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

JesseStutler · 2026-05-08T01:43:02Z

/cherrypick release-1.14

volcano-sh-bot · 2026-05-08T01:43:04Z

@JesseStutler: once the present PR merges, I will cherry-pick it on top of release-1.14 in a new PR and assign it to you.

Details

In response to this:

/cherrypick release-1.14

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/test-infra repository.

volcano-sh-bot · 2026-05-08T01:44:54Z

@JesseStutler: #5259 failed to apply on top of branch "release-1.14":

Applying: Stabilize predicates plugin execution order and rollback semantics.
Using index info to reconstruct a base tree...
M	pkg/scheduler/plugins/predicates/predicates.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/scheduler/plugins/predicates/predicates.go
Applying: Unify exported predicates plugin field naming.
Using index info to reconstruct a base tree...
M	pkg/scheduler/plugins/predicates/predicates.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/scheduler/plugins/predicates/predicates.go
CONFLICT (content): Merge conflict in pkg/scheduler/plugins/predicates/predicates.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Unify exported predicates plugin field naming.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Details

In response to this:

/cherrypick release-1.14

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/test-infra repository.

volcano-sh-bot · 2026-05-08T01:44:56Z

@JesseStutler: new issue created for failed cherrypick: #5281

Details

In response to this:

/cherrypick release-1.14

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/test-infra repository.

Stabilize predicates plugin execution order and rollback semantics.

3ba2679

Signed-off-by: wangyang0616 <wangyang8126@gmail.com>

Copilot AI review requested due to automatic review settings April 28, 2026 16:00

volcano-sh-bot requested review from dafu-wu and merryzhou April 28, 2026 16:00

volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 28, 2026

gemini-code-assist Bot reviewed Apr 28, 2026

View reviewed changes

Copilot AI reviewed Apr 28, 2026

View reviewed changes

Copilot started reviewing on behalf of wangyang0616 April 28, 2026 16:19 View session

wangyang0616 added 2 commits April 29, 2026 17:08

fix(scheduler): align predicates unit tests with plugin order slices

b242d2b

Signed-off-by: wangyang0616 <wangyang8126@gmail.com>

JesseStutler requested a review from Copilot May 6, 2026 07:40

volcano-sh-bot requested a review from JesseStutler May 6, 2026 07:40

Copilot started reviewing on behalf of JesseStutler May 6, 2026 07:41 View session

Copilot AI reviewed May 6, 2026

View reviewed changes

volcano-sh-bot assigned JesseStutler May 8, 2026

volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label May 8, 2026

volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2026

volcano-sh-bot merged commit 27f49b2 into volcano-sh:master May 8, 2026
29 checks passed

volcano-sh-bot mentioned this pull request May 8, 2026

[release-1.14] Stabilize predicates plugin execution order and rollback semantics. #5281

Open

This was referenced May 8, 2026

[WIP] Resolve merge conflicts and stabilize predicates plugin execution order #5285

Closed

[release-1.14] Stabilize predicates plugin execution order and rollback semantics #5286

Merged

Conversation

wangyang0616 commented Apr 28, 2026

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

wangyang0616 May 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

wangyang0616 May 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

JesseStutler commented May 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

JesseStutler commented May 8, 2026

Uh oh!

volcano-sh-bot commented May 8, 2026

Uh oh!

JesseStutler commented May 8, 2026

Uh oh!

volcano-sh-bot commented May 8, 2026

Uh oh!

Uh oh!

volcano-sh-bot commented May 8, 2026

Uh oh!

volcano-sh-bot commented May 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants