KEP-3063: dra: pre-scheduled pods

[pohly]

• One-line PR description: pods with spec.nodeName set while claims are not reserved is an error scenario, but dealing with it automatically when it arises is still useful and not too hard, so it should be worthwhile.

• Issue link: DRA: control plane controller ("classic DRA") #3063

• Other comments:
  See dra: pre-scheduled pods kubernetes#118209 for the kube-controller-manager implementation

/cc @alculquicondor

[alculquicondor]

overall lgtm, but I think this falls under sig-node more than sig-scheduling.

[pohly]

sig-node more than sig-scheduling

Yes. Both kubelet and pkg/controller/resourceclaim are SIG Node. Do you want me to find a different reviewer also for kubernetes/kubernetes#118209? If I remember correctly, you are also moonlighting as reviewer for kcm, so I wasn't sure how much you might want to be involved in that.

[alculquicondor]

I'm not an approver in kcm, so you would still need another person. But also the changes you proposed still belong to SIG Node https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/resourceclaim/OWNERS

[pohly]

Any thoughts on whether such a condition is useful? I'm undecided myself.

@klueska, @smarterclayton ?

[sftim]

My thinking: it's all-or-nothing. So, if something is interested in a particular claim, it's not helpful. If you want the side effect of a metric changing to see how long it's taking Pods to get to this point, it might be.

Maybe we can provide another instrumentation approach though. For example, one or more of:

• increment a per-node counter when a Pod is accepted with pending claims, and increment a different counter when claims are resolved
• put the .status in the ResourceClaim instead, with an entry for each associated Pod;
  teach tooling to look there
• fire an Event [per claim, per Pod] to note that a resource claim has succeeded and is ready

[sftim]

(thinking ahead)
Could we ever have a situation where a Pod that is already running might make a resource claim?
For example, a Pod that is using one cryptographic key identifies that it needs to make another (key rotation) and so requests a second one, by directly calling create for the ResourceClaim API.

In that case, we wouldn't expect to update .status for the Pod; we might not even have the associated Pod object identity available, depending on how it has authenticated.

[pohly]

I believe the only real use case that we have right now is "notify the user why the pod is not starting". An event satisfies that requirement.

A condition or dedicated fields in the claim status seem more about "notify some component so that it can take some action" - we don't have use cases for that right now. My preference is to not add any conditions or fields until we know more about how they would be used.

Could we ever have a situation where a Pod that is already running might make a resource claim?

I suspect that this would be very tricky to implement. Right now, the pod.spec.resourceClaims are fixed. We would have to allow modifying that and then find a way how the container runtime can be told to update the containers, ideally without having to restart them.

Mutating some existing claim is already possible, to some extend. For example, if the claim causes some directory to be mounted, the resource driver can change the content of what was mounted. However, the same caveat applies: modifying the container sandbox in response to some claim changes is not supported.

[pohly]

I removed the condition part. That needs more thought about use cases.

[dchen1107]

One of the challenges on the debuggability today is the user / dev don't know why a pod being scheduled but not running. The scenario of an unallocated or unreserved claims makes the situation even more complicated. But I agreed this need more thoughts.

[swatisehgal]

/cc

[pohly]

/assign @klueska

[Atharva-Shinde]

Would this need another PRR @johnbelamaric ?

[pohly]

My two cents: this is a relatively minor change that doesn't impact any of the PRR aspects. The same API calls are made as before, they just come from kube-controller-manager instead of kube-scheduler. The actual content (= size of PodSchedulingContext) is smaller than normal.

[klueska]

Should we also include the updates to the kubelet plugin API in this or open a separate PR for that?

[pohly]

Given the focus of this PR I prefer a separate PR for kubelet changes.

[klueska]

@bart0sh or @byako Can one of you open a PR to update the KEP with the kubeletplugin API changes? I'd prefer to present the full set of updates all at once in the sig-node meeting (rather than present this PR now and another one later).

[byako]

@klueska , yep, on it. I somehow thought this was updated already.

[byako]

@bart0sh or @byako Can one of you open a PR to update the KEP with the kubeletplugin API changes? I'd prefer to present the full set of updates all at once in the sig-node meeting (rather than present this PR now and another one later).

#4164

[dchen1107]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, pohly

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

The pull request process is described here

Needs approval from an approver in each of these files:

• keps/sig-node/OWNERS [dchen1107]

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