Inactive workloads are occasionally admitted

[dhenkel92]

What happened:

We've observed that inactive workloads (.spec.active=false) occasionally get admitted and are then immediately evicted.

After investigation, we found this is triggered by runtime class reconciliation, which pushes all pending workloads back onto the internal heap, regardless of their active status. This causes the scheduler to consider them, even though they're inactive.

The main workload controller correctly checks .spec.active before adding workloads to the heap. However, this check is missing in other situations.

What you expected to happen:

We’d expect Kueue to consistently avoid scheduling inactive workloads, regardless of the trigger.

How to reproduce it (as minimally and precisely as possible):

1. Apply the test setup below.
2. Annotate the RuntimeClass:

   kubectl annotate runtimeclass test-kueue-class foo=bar
   

This will trigger the code mentioned above, and you’ll see the following events in the workload:

  Type    Reason                   Age   From                       Message
  ----    ------                   ----  ----                       -------
  Normal  QuotaReserved            2s    kueue-admission            Quota reserved in ClusterQueue test-cluster-queue, wait time since queued was 14s
  Normal  Admitted                 2s    kueue-admission            Admitted by ClusterQueue test-cluster-queue, wait time since reservation was 0s
  Normal  EvictedDueToDeactivated  2s    kueue-workload-controller  The workload is deactivated

Example spec:

---
apiVersion: node.k8s.io/v1
kind: RuntimeClass
metadata:
  name: test-kueue-class
handler: myconfiguration
---
apiVersion: kueue.x-k8s.io/v1beta1
kind: ResourceFlavor
metadata:
  name: test-rf
---
apiVersion: kueue.x-k8s.io/v1beta1
kind: ClusterQueue
metadata:
  name: test-cluster-queue
spec:
  namespaceSelector: {} # match all.
  resourceGroups:
    - coveredResources: ["cpu", "memory"]
      flavors:
        - name: "test-rf"
          resources:
            - name: "cpu"
              nominalQuota: 10
            - name: "memory"
              nominalQuota: 2Gi
---
apiVersion: kueue.x-k8s.io/v1beta1
kind: LocalQueue
metadata:
  name: test-local-queue
spec:
  clusterQueue: test-cluster-queue
---
apiVersion: kueue.x-k8s.io/v1beta1
kind: Workload
metadata:
  name: test-wl-1
spec:
  active: false
  podSets:
    - count: 1
      name: main
      template:
        spec:
          runtimeClassName: test-kueue-class
          containers:
            - name: demo
              resources:
                limits:
                  cpu: "1"
                  memory: 128Mi
                requests:
                  cpu: "1"
                  memory: 128Mi
  queueName: test-local-queue

Environment:

• Kubernetes version (use kubectl version): v1.31.6
• Kueue version (use git describe --tags --dirty --always): 0.13.0

[mimowo]

Thank you for reporting!

[jyotilakra92]

Hey, I am new to this open source community and would love to pick this up as my first task.

[olekzabl]

/assign

[olekzabl]

Hello. I've looked at #6737, in particular at this review comment and I'm thinking how to address that generally.
Let me share my findings & thoughts.

1. Looking through all calls to AddOrUpdateWorkloadWithoutLock, I see:

   • 3 places which do pre-check IsActive
     (as well as "has no quota reservation", or alternatively "has status pending", which implies the former):

     kueue/pkg/controller/core/workload_controller.go

     Lines 259 to 264 in c4143a4

     	if workload.IsActive(&wl) && !workload.HasQuotaReservation(&wl) {
     	if err := r.queues.AddOrUpdateWorkload(&wl, queueOptions...); err != nil {
     	log.V(2).Info("Failed to add DRA workload to queue", "error", err)
     	return ctrl.Result{}, err
     	}
     	} else {

     kueue/pkg/controller/core/workload_controller.go

     Line 300 in c4143a4

     if err := r.queues.AddOrUpdateWorkload(wl.DeepCopy()); err != nil {

     kueue/pkg/controller/core/workload_controller.go

     Lines 795 to 800 in c4143a4

     	if workload.IsActive(e.Object) && !workload.HasQuotaReservation(e.Object) {
     	if err := r.queues.AddOrUpdateWorkload(wlCopy); err != nil {
     	log.V(2).Info("ignored an error for now", "error", err)
     	}
     	return true
     	}

     Note that, in snippets # 1 and 3, IsActive influences not only whether we'll call AddOrUpdateWorkload(), but also the subsequent flow of the code.
     Which means that we can't just move the check deeper down the call stack.

   • 3 places which don't pre-check IsActive
     (still, they all do pre-check "having no quota reservation" in some way):

     • In WorkloadReconciler.Update():

       kueue/pkg/controller/core/workload_controller.go

       Line 893 in c4143a4

       err := r.queues.UpdateWorkload(log, e.ObjectOld, wlCopy)

       kueue/pkg/controller/core/workload_controller.go

       Line 928 in c4143a4

       if err := r.queues.AddOrUpdateWorkloadWithoutLock(wlCopy); err != nil {

     • In queueReconcileForPending() (the one originally discussed in this issue)

       kueue/pkg/controller/core/workload_controller.go

       Line 1111 in c4143a4

       if err = h.r.queues.AddOrUpdateWorkload(wlCopy); err != nil {

2. For the latter 3 cases, we have a choice between:

   • A: adding a pre-check check for IsActive, like for the former 3 cases, and like it's coded in Fix: Inactive workloads occasionally getting admitted #6737;
   • B: putting a pre-check for IsActive (and no quota reserved as well?) down the stack, as proposed in the comment.

   At first, I was stylistically tempted to propose A - so that the code is more self-consistent overall.

   However, now I'm thinking B may be more correct - in that, if a Workload just got updated by switching its Spec.Active from true (or empty) to false, and if it was present in r.queues so far, B will result in removing it from there, while A would just make no action.

3. Then, the question of "nice code" strikes back - if we just do B, then - on some code paths - we'll be checking IsActive twice. The performance cost is (I think) negligible; still, this could be considered not very nice.
   However, I don't see a nicer (and simple) way.

Summing up, I'd do just B.
Plus returning an error if "no quota reserved" is not met. (IIUC this will never happen for current code - so this is to warn against potential future regressions).
Plus an integration test as requested.
Plus maybe another integration test to make sure that workload deactivation removes it from the queue heap. (Unless I find out that it's hard to test, or already tested).

Does it make sense?

@mimowo @dhenkel92

[mimowo]

Summing up, I'd do just B.

Ideally, yes, but if you encounter issues then the benefit of A is the minimal impact, which might be in plus just before release

Plus an integration test as requested.

Yes, I would even start here with the TDD approach, even if the test is flaky (say 1/20 repeats) it will already give us signal that we fixed the issue - once fixed.

Does it make sense?

yes

[olekzabl]

Update: my speculation about A being incorrect turned out false.

Both problematic cases (inside WorkloadReconciler.Update()) are in fact also guarded by a check of IsActive, which happens much higher up:

kueue/pkg/controller/core/workload_controller.go

Lines 870 to 871 in b47e06e

	switch {
	case status == workload.StatusFinished || !active:

If a workload is not active, we'll enter the switch branch above, and hence not enter the other branches containing the calls to [AddOr]UpdateWorkload().

This is also confirmed by an already existing integration test:

kueue/test/integration/singlecluster/controller/core/workload_controller_test.go

Line 167 in b47e06e

ginkgo.It("should clear workload requeue state after deactivation", func() {

So, at this point, my preference goes back to solution A.

[Singularity23x0]

However, now I'm thinking B may be more correct - in that, if a Workload just got updated by switching its Spec.Active from true (or empty) to false, and if it was present in r.queues so far, B will result in removing it from there, while A would just make no action.

From the perspective of #5310 this would complicate moving the logic to the Reconciler due to increasing the amount of places which rely on the past state of the workload.

Based on that I recommend going with option A.