fix(scheduler): scope gpujoborder to victim selection only via new VictimOrderFn extension point

Fixes a sign-inversion bug where prefer-larger evicted the smaller job
(the victim queue's !JobOrderFn inversion flipped the same sign used
for pending-job ordering). Adds Session.VictimOrderFn/AddVictimOrderFn
as a dedicated, non-inverted composition path for victim-specific
comparators, falling back to the existing !JobOrderFn behavior when
none are registered, so no other plugin's behavior changes. gpujoborder
now registers exclusively via AddVictimOrderFn and no longer affects
pending-job allocation ordering at all.

Addresses @gshaibi's review comment on #1995.

Signed-off-by: CoolingCube <CoolingCubeInfo@proton.me>

Author: CoolingCube

pkg/scheduler/actions/utils/job_order_by_queue.go

@@ -255,7 +255,7 @@ func (jo *JobsOrderByQueues) createLeafNode(queue *queue_info.QueueInfo) *queueN
 		queue: queue,
 		children: scheduler_util.NewPriorityQueue(func(l, r interface{}) bool {
 			if jo.options.VictimQueue {
-				return !jo.ssn.JobOrderFn(l, r)
+				return jo.ssn.VictimOrderFn(l, r)
 			}
 			return jo.ssn.JobOrderFn(l, r)
 		}, jo.options.MaxJobsQueueDepth),

pkg/scheduler/framework/session.go

@@ -76,6 +76,7 @@ type Session struct {
 	NodePreOrderFns                       []api.NodePreOrderFn
 	NodeOrderFns                          []api.NodeOrderFn
 	JobOrderFns                           []common_info.CompareFn
+	VictimOrderFns                        []common_info.CompareFn
 	SubGroupOrderFns                      []common_info.CompareFn
 	TaskOrderFns                          []common_info.CompareFn
 	QueueOrderFns                         []api.CompareQueueFn
@@ -434,6 +435,7 @@ func (ssn *Session) clear() {
 	ssn.NodePreOrderFns = nil
 	ssn.NodeOrderFns = nil
 	ssn.JobOrderFns = nil
+	ssn.VictimOrderFns = nil
 	ssn.SubGroupOrderFns = nil
 	ssn.TaskOrderFns = nil
 	ssn.QueueOrderFns = nil

pkg/scheduler/framework/session_plugins.go

@@ -68,6 +68,15 @@ func (ssn *Session) AddJobOrderFn(jof common_info.CompareFn) {
 	ssn.JobOrderFns = append(ssn.JobOrderFns, jof)
 }
 
+// AddVictimOrderFn registers a comparator that applies ONLY when ordering
+// candidates for eviction (the victim queue), not for regular pending-job
+// allocation ordering. Unlike JobOrderFn, no external inversion is applied
+// to this comparator's result: a negative return means "l is the BETTER
+// victim (should be evicted first)", directly.
+func (ssn *Session) AddVictimOrderFn(vof common_info.CompareFn) {
+	ssn.VictimOrderFns = append(ssn.VictimOrderFns, vof)
+}
+
 func (ssn *Session) AddTaskOrderFn(tof common_info.CompareFn) {
 	ssn.TaskOrderFns = append(ssn.TaskOrderFns, tof)
 }
@@ -285,6 +294,21 @@ func (ssn *Session) JobOrderFn(l, r interface{}) bool {
 	}
 }
 
+// VictimOrderFn composes registered victim-specific comparators directly --
+// a negative result means "l is the BETTER victim", with no external
+// inversion needed or applied (unlike the old !JobOrderFn(l, r) pattern).
+// If no victim-specific comparators are registered, this falls back to the
+// existing !JobOrderFn(l, r) behavior, preserving exact current behavior
+// for any plugin that never needed the ordering/victim distinction.
+func (ssn *Session) VictimOrderFn(l, r interface{}) bool {
+	for _, vof := range ssn.VictimOrderFns {
+		if v := vof(l, r); v != 0 {
+			return v < 0
+		}
+	}
+	return !ssn.JobOrderFn(l, r)
+}
+
 func (ssn *Session) TaskOrderFn(l, r interface{}) bool {
 	for _, compareTasks := range ssn.TaskOrderFns {
 		if comparison := compareTasks(l, r); comparison != 0 {

pkg/scheduler/plugins/gpujoborder/gpujoborder.go

@@ -31,11 +31,19 @@ func (rp *gpuJobOrderPlugin) Name() string {
 	return "gpujoborder"
 }
 
+// OnSessionOpen registers this plugin's comparator via AddVictimOrderFn,
+// NOT AddJobOrderFn. This plugin is scoped to victim/eviction selection
+// only, per the real, confirmed distinction between the ordering and
+// victim-selection paths (see VictimOrderFn in session_plugins.go) --
+// pending-job allocation ordering is intentionally left untouched.
 func (rp *gpuJobOrderPlugin) OnSessionOpen(ssn *framework.Session) {
-	ssn.AddJobOrderFn(rp.JobOrderFn)
+	ssn.AddVictimOrderFn(rp.VictimOrderFn)
 }
 
-func (rp *gpuJobOrderPlugin) JobOrderFn(l, r interface{}) int {
+// VictimOrderFn returns -1 when l is the BETTER victim (should be evicted
+// first), matching VictimOrderFn's direct (non-inverted) contract -- no
+// external sign flip is applied or needed here.
+func (rp *gpuJobOrderPlugin) VictimOrderFn(l, r interface{}) int {
 	lv := l.(*podgroup_info.PodGroupInfo)
 	rv := r.(*podgroup_info.PodGroupInfo)
 

pkg/scheduler/plugins/gpujoborder/gpujoborder_test.go

@@ -38,48 +38,48 @@ func newPlugin(t *testing.T, mode string) *gpuJobOrderPlugin {
 	return rp
 }
 
-func TestJobOrderFn_PriorityDiffers_Defers(t *testing.T) {
+func TestVictimOrderFn_PriorityDiffers_Defers(t *testing.T) {
 	vm := resource_info.NewResourceVectorMap()
 	a := makeGPUPodGroup("a", 50, 1, vm)
 	b := makeGPUPodGroup("b", 10, 1, vm)
 	rp := newPlugin(t, "")
-	if got := rp.JobOrderFn(a, b); got != 0 {
+	if got := rp.VictimOrderFn(a, b); got != 0 {
 		t.Errorf("expected 0 when priorities differ, got %d", got)
 	}
 }
 
-func TestJobOrderFn_SamePriority_PrefersLarger_Default(t *testing.T) {
+func TestVictimOrderFn_SamePriority_PrefersLarger_Default(t *testing.T) {
 	vm := resource_info.NewResourceVectorMap()
 	small := makeGPUPodGroup("small", 10, 1, vm)
 	large := makeGPUPodGroup("large", 10, 2, vm)
 	rp := newPlugin(t, "")
-	if got := rp.JobOrderFn(large, small); got != -1 {
+	if got := rp.VictimOrderFn(large, small); got != -1 {
 		t.Errorf("expected -1 (larger job preferred as victim), got %d", got)
 	}
-	if got := rp.JobOrderFn(small, large); got != 1 {
+	if got := rp.VictimOrderFn(small, large); got != 1 {
 		t.Errorf("expected 1, got %d", got)
 	}
 }
 
-func TestJobOrderFn_SamePriority_SameGPU_FallsThrough(t *testing.T) {
+func TestVictimOrderFn_SamePriority_SameGPU_FallsThrough(t *testing.T) {
 	vm := resource_info.NewResourceVectorMap()
 	a := makeGPUPodGroup("a", 10, 1, vm)
 	b := makeGPUPodGroup("b", 10, 1, vm)
 	rp := newPlugin(t, "")
-	if got := rp.JobOrderFn(a, b); got != 0 {
+	if got := rp.VictimOrderFn(a, b); got != 0 {
 		t.Errorf("expected 0 (equal GPU falls through), got %d", got)
 	}
 }
 
-func TestJobOrderFn_PreferSmallerMode(t *testing.T) {
+func TestVictimOrderFn_PreferSmallerMode(t *testing.T) {
 	vm := resource_info.NewResourceVectorMap()
 	small := makeGPUPodGroup("small", 10, 1, vm)
 	large := makeGPUPodGroup("large", 10, 2, vm)
 	rp := newPlugin(t, "prefer-smaller")
-	if got := rp.JobOrderFn(small, large); got != -1 {
+	if got := rp.VictimOrderFn(small, large); got != -1 {
 		t.Errorf("expected -1 (smaller job preferred as victim in prefer-smaller mode), got %d", got)
 	}
-	if got := rp.JobOrderFn(large, small); got != 1 {
+	if got := rp.VictimOrderFn(large, small); got != 1 {
 		t.Errorf("expected 1, got %d", got)
 	}
 }
@@ -89,7 +89,44 @@ func TestNew_UnrecognizedMode_FallsBackToPreferLarger(t *testing.T) {
 	small := makeGPUPodGroup("small", 10, 1, vm)
 	large := makeGPUPodGroup("large", 10, 2, vm)
 	rp := newPlugin(t, "totally-not-a-real-mode")
-	if got := rp.JobOrderFn(large, small); got != -1 {
+	if got := rp.VictimOrderFn(large, small); got != -1 {
 		t.Errorf("expected fallback to prefer-larger behavior, got %d", got)
 	}
 }
+
+// TestGpujoborder_DoesNotAffect_PendingJobOrdering is the specific test
+// gshaibi asked for: confirms that registering gpujoborder's comparator
+// has NO EFFECT on ssn.JobOrderFn (pending-job allocation ordering),
+// since the plugin now registers exclusively via AddVictimOrderFn.
+func TestGpujoborder_DoesNotAffect_PendingJobOrdering(t *testing.T) {
+	rp := newPlugin(t, "")
+
+	vm := resource_info.NewResourceVectorMap()
+	small := makeGPUPodGroup("small", 10, 1, vm)
+	large := makeGPUPodGroup("large", 10, 2, vm)
+
+	// Session WITHOUT gpujoborder registered: falls through to the
+	// existing CreationTimestamp/UID fallback for JobOrderFn.
+	ssnWithout := &framework.Session{}
+	baselineResult := ssnWithout.JobOrderFn(small, large)
+
+	// Session WITH gpujoborder registered via OnSessionOpen (the real
+	// registration path), same two jobs, same JobOrderFn call.
+	ssnWith := &framework.Session{}
+	rp.OnSessionOpen(ssnWith)
+	withPluginResult := ssnWith.JobOrderFn(small, large)
+
+	if len(ssnWith.JobOrderFns) != 0 {
+		t.Errorf("expected gpujoborder to register 0 JobOrderFns (it should only "+
+			"register via AddVictimOrderFn now), got %d", len(ssnWith.JobOrderFns))
+	}
+	if len(ssnWith.VictimOrderFns) != 1 {
+		t.Errorf("expected gpujoborder to register exactly 1 VictimOrderFn, got %d",
+			len(ssnWith.VictimOrderFns))
+	}
+	if baselineResult != withPluginResult {
+		t.Errorf("pending-job ordering changed after registering gpujoborder: "+
+			"without=%v, with=%v -- expected identical, since gpujoborder must not "+
+			"affect JobOrderFn at all", baselineResult, withPluginResult)
+	}
+}