Is effect: None + kubectl describe by itself an adequate safeguard against erroneous taints? Or are there different kinds of mistakes that only rate limiting eviction would mitigate? In general I like the idea of effect: None essentially being a dry run instead of trying to determine in real-time whether a taint is working while it is or is not actively evicting pods. Wondering if that's a worthwhile way we could narrow the scope here.

I don't think the rate limiting would uncover anything that kubectl describe may have missed, except perhaps a race (one pods shown as to be evicted, large number of additional such pods created, then turning on eviction and deleting all of them).

But primarily it is that kubectl describe is optional, some admin might forget to double-check. Then the rate limiting may help as a second line of defense.

I'm undecided about this field. As it stands, this is a lower bound: "at least this number of pods were evicted, but maybe also more". 99% of the times it'll be accurate, but guaranteeing that would imply evicting a pod and updating this field in a single atomic transaction. If the kube-controller-manager dies and restarts, it cannot figure out whether all evicted pods were accounted for.

When using conditions, determining the old count becomes icky (need to parse string on startup, which may not always work because IMHO we shouldn't make the string format part of the official API).

Is the idea with the PodsEvicted field that it will monotonically increase forever? Would it make more sense to expose that as a metric tied to the lifetime of a kube-controller-manager instance? Then Prometheus would make it easier to add up the total evictions for a given DeviceTaintRule among all past and present kube-controller-manager pods without Kubernetes having to handle that itself.

I think the API is still a good place to at least keep some sign of "in progress"/"done" but maybe doesn't need to detail the exact progress. For that I think a condition makes sense.

Is the idea with the PodsEvicted field that it will monotonically increase forever?

Yes, but per DeviceTaintRule. Basically some feedback to the admin that "yes, this did something".

Would it make more sense to expose that as a metric tied to the lifetime of a kube-controller-manager instance? Then Prometheus would make it easier to add up the total evictions for a given DeviceTaintRule among all past and present kube-controller-manager pods without Kubernetes having to handle that itself.

Such a metric makes sense for "overall work done". But it cannot be per DeviceTaintRule because using e.g. the DeviceTaintRule UID as dimension would lead to unlimited growth of that metric.

I think the API is still a good place to at least keep some sign of "in progress"/"done" but maybe doesn't need to detail the exact progress. For that I think a condition makes sense.

So you think we can drop PodsEvicted and also not represent it in the condition description?

Such a metric makes sense for "overall work done".

Actually, it already exists: "Total number of Pods deleted by DeviceTaintEvictionController since its start."

I think as part of condition description it can be included like this "10 pods are pending eviction, 100 evicted already since starting the controller". That makes it clear that the count resets when restarting the controller and thus avoids potential confusion when the number is too low.

Resolved by switching to conditions. The exact wording of the description can be decided during the implementation phase.

While implement the type updates I was reminded that bumping the spec increments the Generation. This was meant as preparation for conditions which can record the observed generation to document what they are based on.

But now these new status fields aren't using conditions. Should they? There's no field to store data like "pods pending eviction" in machine-readable form in https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Condition. The numbers could be part of the message field:

Condition {
   Type: "InProgress",
   Status: ConditionTrue,
   ObservedGeneration: rule.spec.Generation,
   LastTransitionTime: time.Now,
  Reason: "PodsPendingEviction",
  Message: fmt.Sprintf("%d pods are pending eviction, %d evicted already", numPodsPending, numPodsEvicted),
}`

This becomes `Status: ConditionFalse` when there are no pods which need to be evicted.

The advantage of conditions is that kubectl wait --for=condition=InProgress=false DeviceTaintRule/my-taint-rule can be used, which is nicer than kubectl wait --for=jsonpath='{.status.podsPendingEviction}'=0.

I've switched to Conditions []metav1.Condition.

While implement the type updates I was reminded that bumping the spec increments the Generation.

It can (and should), but must be specifically done in the strategy PrepareForUpdate method

I've switched to Conditions []metav1.Condition.

+1 for using conditions

It can (and should), but must be specifically done in the strategy PrepareForUpdate method

I wasn't clear: I meant to point out that I already implemented it like that in the initial implementation in 1.33 😁 .

I agree that managing Generation should be considered best practice for any type with a spec.

Since slices can now be “devices” or “taints”, can we call out:

• How the scheduler/DRA controllers index taint-slices vs device-slices.
• Cache invalidation and event fan-out when taint-slices update, to avoid O(N×M) reprocessing.

Short answer: the ResourceSlice tracker keeps track of that state and tries to minimize the changes that the consumers (scheduler and eviction controller) see.

The long answer is a bit more involved and I'd like to punt on that now while still in alpha. It needs to be added for beta when discussing scalability.

How do we handle the races between scheduling and post-allocation taints? For example:

• Taint appears after scheduling but before kubelet admission.
• Taint appears while pod is running.

Is that question related to this user story?

Taint appears after scheduling but before kubelet admission.

If the effect is NoSchedule, then nothing happens. The pod is allowed to run.
If the effect is NoExecute, then the pod gets evicted.

Taint appears while pod is running.

Same here.

What I meant that it would nice to mention in the Safe Pod Eviction section that:

• NoSchedule only affects scheduling decisions. Once a pod is bound, adding the taint later has no impact.
• NoExecute applies retroactively. If the taint appears either right after scheduling but before kubelet admission or while the pod is already running, the pod is subject to eviction.

That way readers don’t misinterpret this as a race with undefined behavior.

IMHO NoSchedule does not belong into this section because the use case is about eviction.

I've extended the story with additional comments about that race.

I'm intentionally not using the NoExecute constant here because at this point I don't want to educate readers about the API details.

I would add Ready or Completed condition state.

Can you describe what the semantic of those states would be?

"Completed" could replace "EvictionInProgress", but it could be a bit misleading: it might be "Completed" now, but in the unlikely case that some pod pops up which is scheduled (for example, the scheduler missed the taint or a user manually scheduled a pod), then it's possible to go back from "Completed: True" to "Completed: False". It's not really a final state.

I agree. How about that?

• EvictionInProgress=True → pods are currently being evicted.
• EvictionInProgress=False, Reason=Idle → no pods are pending eviction at the moment.

I updated the comment in the API docs and added reason: Idle in the explanations beneath it.

Can we add test plan for the case when multiple rules hit the same device set with different rates?

We may need to add another test case to validate rate limiting and controller back-pressure.

Ack.

I'm still finding this pretty weird to reason about. I think this is the max evictions/second this taint would drive, but it's pretty best-effort / there's no guarantee pods would actually be evicted at this rate, right?

• The eviction controller sync loop might be busy with other things
• The eviction controller API client might be rate-limited client-side or server-side
• The eviction controller API client might not be able to keep up with evictions
• The attempts to evict pods could be rejected due to PDB (does the controller use the /evict subresource that respects PDB?)

If we keep this here, the docs need to make it really clear that this is not a strong guarantee pods will be deleted this fast

Yes, it would be "max rate with best effort".

does the controller use the /evict subresource that respects PDB?

No. I copied the node eviction controller, which just deletes the pod. I don't have a strong opinion about what's best, but following that established pattern might be less surprising because it's consistent.

Maybe two separate effects? One to "forcibly evict" and one to "evict respecting policy (PDB)"?

With rate limiting gone there is no need for this field.

Correct but I think the idea of an effect that honors PDBs vs and effect that does not is a separate discussion.

True. I'll keep this open for the PDB aspect. Like you I prefer to treat support for PDB as a future extension. It raises several questions that are similar to the rate limit (who gets to decide whether it should be used or not) and the API isn't obvious (new effect or a separate field).

But let's prepare for it now and explicitly allow adding more effects:

    // The effect of the taint on claims that do not tolerate the taint
    // and through such claims on the pods using them.
    //
    // Valid effects are None, NoSchedule and NoExecute. PreferNoSchedule as used for
    // nodes is not valid here. More effects may get added in the future.
    // Consumers must treat unknown effects like None.
    //
    // +required
    Effect DeviceTaintEffect

    // ^^^^
    //
    // Implementing PreferNoSchedule would depend on a scoring solution for DRA.
    // It might get added as part of that.
    //
    // A possible future new effect is NoExecuteWithPodDisruptionBudget:
    // honor the pod disruption budget instead of simply deleting pods.
    // This is currently undecided, it could also be a separate field.
    //
    // Validation must be prepared to allow unknown enums in stored objects,
    // which will enable adding new enums within a single release without
    // ratcheting.

Define how clients (schedulers, evictors, etc) should treat unrecognized taint effects (safest option is probably to treat them as None). If we contemplate adding more effects in the future, we have to ensure the oldest clients still present which do things with taint effects handle unrecognized effects safely. A 1.x API server can have a 1.(x-1) scheduler or kube-controller-manager running against it.

Define how clients (schedulers, evictors, etc) should treat unrecognized taint effects (safest option is probably to treat them as None).

I already did above in the API docs ("Consumers must treat unknown effects like None."). There it is visible to anyone who may want to react to the API.

This note here is more geared towards "how do we implement this type" and thus not part of the official API.

While implement the type updates I was reminded that bumping the spec increments the Generation.

It can (and should), but must be specifically done in the strategy PrepareForUpdate method

I've switched to Conditions []metav1.Condition.

+1 for using conditions

Fixed.

Define how clients (schedulers, evictors, etc) should treat unrecognized taint effects (safest option is probably to treat them as None).

I already did above in the API docs ("Consumers must treat unknown effects like None."). There it is visible to anyone who may want to react to the API.

This note here is more geared towards "how do we implement this type" and thus not part of the official API.

It can (and should), but must be specifically done in the strategy PrepareForUpdate method

I wasn't clear: I meant to point out that I already implemented it like that in the initial implementation in 1.33 😁 .

I agree that managing Generation should be considered best practice for any type with a spec.

Fixed.

Is there a plan to close that race window at some point by blocking new allocations while a taint is created?

IIUC currently the SoT for NoSchedule case is in scheduler, but NoExecute in Device Driver, which triggers eviction.

Theoretically if the NoSchedule taint is created DRA driver could reject binding request for pods which does not have matching toleration, so in fact it could have an immediate effect.

Is there a plan to close that race window at some point by blocking new allocations while a taint is created?

No. I don't see how that could be done. The scheduler is always going to observe the taint creation with some delay.

IIUC currently the SoT for NoSchedule case is in scheduler, but NoExecute in Device Driver, which triggers eviction.

Sorry, I'm not following. The SoT for a taint is in the object which defines the taint (DeviceTaintRule or ResourceSlice). Different consumers (scheduler, controller) react to that once they observe those. There's no difference regarding that between NoScheduler and NoExecute.

IIUC NoSchedule is enforced in scheduler, but NoExecute in scheduler, but eventually pods are evicted by the taint controller (not the Driver as said).

I wonder if the Driver should/could block new allocations for RCs which does not have necessary tolerations and reject them as well (assuming it can reject allocation, but I'm not sure). If we don't have this mechanism, then Device Driver cannot guarantee that new pods don't appear once it published pods to evict.

NoSchedule is enforced in scheduler, but NoExecute in scheduler

Did you mean "NoSchedule is enforced in scheduler, but NoExecute in controller"?

The scheduler also enforced NoExecute by treating it like NoSchedule. It just doesn't evict, leaving that to the controller.

I wonder if the Driver should/could block new allocations for RCs which does not have necessary tolerations and reject them as well (assuming it can reject allocation, but I'm not sure).

It cannot. The driver is not consulted by the scheduler when allocating ResourceClaims.

Added.

Since there is a race with scheduler, it may be possible that initial set of conditions does not list any pod to evict. In that case kubectl wait may get completed but some pods may appear later and will become a subject for eviction not tracked with conditions.

True. We can make it very unlikely, though, by delaying setting the condition to False shortly after the DeviceTaintRule was created (5 to 10 seconds?). That should be long enough to propagate the new DeviceTaintRule to a running kube-scheduler. After a restart, the kube-scheduler first waits for cache sync before scheduling pods.

will become a subject for eviction not tracked with conditions

As discussed elsewhere with @helayoty, this condition can go from "False" to "True". The controller must reconcile to catch this.

So I guess there is nothing better we could do than adding this best effort delay to mitigate this race. I think we can discuss it again before beta promotion. I bet long term we need to have some way to address this sort of races, especially since the same problem is between scheduler and kubelet, so some unification would be welcome.