Kubernetes / NRI communication path

[aojea]

This was found by @gauravkghildiyal , see doc with more details.

in DRANET, a DRA driver for kubernetes networking, we implement an architecture with 2 legs for the driver:

1. Kubelet via DRA
2. Container runtime via NRI

This architecture allow to handle complex workloads that require to execute at different stages of a pod creation. Also, this guarantees that the kubernetes system will not schedule nothing to the node until the DRA driver in the node is running, the driver can store the state in memory during the kubelet DRA NodePrepareResources() callback, and use the NRI callback to execute the actions on the Pod during the container runtime stage.

However, there is still a race in this process, if for any case the runtime is restarted, the DRA driver will not be able to notice it and the Pod can be running without the NRI call back called ... we need to ensure that a specific driver is always called for a specific pod

NRI added the concept of required plugins #278 , this indicates that drivers can implement this behavior globally modifying the configuration or via annotations.

Since global is an admin option that requires changing containerd config, is hard for independent drivers to modify it, so we need to use the annotations path, however, the DRA hook in kubelet does not have a way to plumb dumb annotations (Device plugin does have)

I commented this during kubecon with NRI maintainers @klihub @kad they mentioned it should be possible to use the existing NodePrepareResources() hook to be able to use CDI to allow this functionality, but may require some work in some places to enable it.

@klihub @gauravkghildiyal @samuelkarp @chrishenzie

[klihub]

Since global is an admin option that requires changing containerd config, is hard for independent drivers to modify it, so we need to use the annotations path, however, the DRA hook in kubelet does not have a way to plumb dumb annotations (Device plugin does have)

I commented this during kubecon with NRI maintainers @klihub @kad they mentioned it should be possible to use the existing NodePrepareResources() hook to be able to use CDI to allow this functionality, but may require some work in some places to enable it.

@aojea @kad I took a look at this, to see 1) what exactly is there in CDI for annotations, 2) if it could be used for this, and 3) what is the extent of changes required to make this work.

It was pretty easy to shoestring together for a working proto. Fundamentally two things were needed:

• allow CDI to annotate a container when a CDI device is injected
• teach the builtin default NRI validator plugin to also take into account required plugins annotated on a container (IOW in addition to the global configuration, and any potential user provided required plugins annotated on the pod)

While this works, I think it would be a bit problematic to retrofit existing CDI annotations to double as container annotations, since AFAIK they are already in use by some vendors and their current semantics are 'annotations for the infra which uses CDI to inject devices into a container' (or actually a VM I suspect, but @zvonkok probably can tell us a bit more details).

So I took also a look at how it would look like if we added more complete support to CDI for injecting container annotations during device injection. So the exact same idea, just not re-/mis-using the existing 'infra-annotations'. I have a working tree here: https://github.com/klihub/container-device-interface/tree/devel/container-annotations. I also have an updated NRI tree with a trivial patch to the builtin validator picking container-annotated required plugins using the/a well-known CDI annotation and then failing container creation if necessary, and a containerd tree divering CDI and NRI against those for testing.

I'd like to pull in @elezar to hear his opinion about adding something (more generic than an absolutely necessary minimum) like this to CDI. And I'd like to hear other folks opinion about whether lose-coupling NRI required plugin checking with CDI this way is frowned upon or lose enough ?

[zvonkok]

@aojea We (@kad FYI) face a similar dilemma with Kata and Confidential Containers, where we, for now, rely on annotations and the pod resources api to deduce what callbacks to run for a specific device that is added to the sandbox and execute different actions depending on the Pod lifecycle.
We proposed to extend the CRI API as well with information that is needed before sandbox create. At KubeCon, we spoke with Dawn, and she was open to reconsidering the change to the CRI API.
For now, I do not know whether it makes sense to merge those two use cases, but it is worth considering consolidation.

I am currently planning and thinking about the proper DRA support with Kata/Confidential Containers and/or sandboxed environments in general.

[klihub]

@aojea We (@kad FYI) face a similar dilemma with Kata and Confidential Containers, where we, for now, rely on annotations and the pod resources api to deduce what callbacks to run for a specific device that is added to the sandbox and execute different actions depending on the Pod lifecycle. We proposed to extend the CRI API as well with information that is needed before sandbox create. At KubeCon, we spoke with Dawn, and she was open to reconsidering the change to the CRI API. For now, I do not know whether it makes sense to merge those two use cases, but it is worth considering consolidation.

I am currently planning and thinking about the proper DRA support with Kata/Confidential Containers and/or sandboxed environments in general.

@zvonkok What do you think about piggybacking some metadata for some use cases on the existing top level and per device Annotations fields, the same ones you currently use for other purposes ? I'm considering things like using them to declare at the CDI level extra attributes for devices which would then be transparently put on corresponding DRA-exposed resources as attributes by a DRA driver, probably using a well-defined common prefix for such attribute annotations. Maybe something like 'resource.attribute.xyzzy.foo: bar' which would then show up as 'xyzzy.foo: bar' on a resource ?

And in principle the annotations that we'd like to propagate to containers could be handled with similar semantic conventions without syntactic changes required to the current Spec itself, as opposed to the latest incarnation of what I have been playing around with now for a day or two.

[klihub]

@aojea @elezar I have a containerd tree patched to use

• the above mentioned CDI additions, and
• an NRI with the default validator plugin patched to look for CDI annotated required plugins

If you now have a CDI Spec with a global or device-level extra container annotation like this

cdiVersion: "1.2.0"
kind: "vendor.com/test"
containerEdits:
  annotations:
    required-plugins:
      value: |
        - template
      format: stringSlice
      onConflict: append
devices:
  - name: dev0
    containerEdits:
      deviceNodes:
        - path: /dev/test-dev0
          hostPath: /dev/null
      env:
        - TEST_DEV0=true

then if you try to claim it for a container with something like

apiVersion: resource.k8s.io/v1beta1
kind:  ResourceClaim
metadata:
  name: claim-dev0
spec:
  devices:
    requests:
    - name: req-dev0
      deviceClassName: dra-matic-cdi
      count: 1
---
apiVersion: v1
kind: Pod
metadata:
  name: test-pod
spec:
  containers:
  - name: ctr0
    image: busybox
    imagePullPolicy: IfNotPresent
    command:
      - sh
      - -c
      - sleep 3600
    resources:
      claims:
        - name: claim-dev0
  resourceClaims:
  - name: claim-dev0
    resourceClaimName: claim-dev0
  terminationGracePeriodSeconds: 1

without running the required template plugin, then you'll get a container creation error like this

[root@n4c16-fedora-43-containerd ~]# kubectl get pod test-pod -ojson | jq '.status.containerStatuses[0].state'                                                                                                                                
{                                                                                                                                                                                                                                             
  "waiting": {                                                                                                                                                                                                                                
    "message": "failed to create containerd container: failed to get NRI adjustment for container: validator \"00-default-validator\" rejected container adjustment, reason: validation error: required plugin \"template\" not present",     
    "reason": "CreateContainerError"                                                                                                                                                                                                          
  }                                                                                                                                                                                                                                           
} 

But if you start up the template plugin, the creation of the container should succeed.

So I think this approach would work, so if folks agree that we should go forward and implement this then it mostly comes down to implementation details. Related to that my biggest reservation/question about the PoC CDI changes is whether it is really justifiable to extend the CDI Spec with such generic container annotation injection support (IOW, do we expect some other real use cases for it) ?

If not, then we could try to tap in to the already existing global and device-level annotations in CDI spec to carry the same information.

[gauravkghildiyal]

@klihub This is great progress, thanks!

I have a couple of questions focused on verifying that this would cover our use case described at Proposal: Ensuring NRI Plugin Execution for DRA Workloads via CRI Annotations (You can ignore the solution described there for now and focus on just the problem)

I have 2 questions:

1. So far in DRANET, we don't use CDI at all and solely rely on NRI. I understand that with the current idea in this thread, we will need to use CDI solely for the purpose of making sure the NRI plugin gets executed. I don't consider this to be bad, but just confirming that I understood things correctly.

   As an example, this is what that CDI spec would look like that has a dummy placeholder device to just enforce the NRI invocation. (We don't require any other container edits or any other thing, but solely that NRI plugin execution is enforced)

# Example CDI config to enforce NRI plugin execution
cdiVersion: "1.2.0"
kind: "dra.net/nri-enforcement"
devices:
  - name: placeholder-for-making-nri-required
containerEdits:
  annotations:
    required-plugins:
      value: |
        - dra.net
      format: stringSlice
      onConflict: append

2. Will the above also be sufficient to make sure our NRI hooks on RunPodSandbox are also necessarily invoked. The confusion that I'm trying to clear up is that this is not just limited to the Container related hooks (which is usually the impression I get from an API termed ContainerDeviceInterface).

If the answer to question 2 above is that "NO it doesn't already work with RunPodSandbox" and requires significant effort, I think at that point we should start questioning whether using CDI to enforce NRI is indeed the right approach, or if we perhaps need to solve this by making DRA/kubelet pass in NRI required annotations through CRI (which is the angle I was thinking of in the doc Proposal: Ensuring NRI Plugin Execution for DRA Workloads via CRI Annotations

[SergeyKanzhelev]

We have a discussion here: https://docs.google.com/document/d/1bssPQW4L_RVyVtGhq1rWidxFSVmvN2iwEHG0t5KN7S4/edit?disco=AAAB29IroeQ

I think the problem of DRANET is not a DRA problem, it is a NRI framework problem. I think it is not the first time NRI authors will ask for a persistent self-registration of a plugin with the container runtime. If we will solve this problem for DRANET, there will be another similar ask for some other functionality.

I think some sort of persistent self-registration of NRI plugins with the container runtime to declare themselves "required" is a better feature to build. Since it is potentially dangerous, it can be built with some TTL so container runtime can self-heal after a bad termination of a "required" self-registered NRI plugin

[SergeyKanzhelev]

@zvonkok what properties are needed to be passed from DRA to NRI for confidential compute? Please bring it on. Also, do confidential compute Pods adopting pod-level resources? I feel like pod level resources are very-well suited for those

[aojea]

I think the problem of DRANET is not a DRA problem, it is a NRI framework problem. I think it is not the first time NRI authors will ask for a persistent self-registration of a plugin with the container runtime. If we will solve this problem for DRANET, there will be another similar ask for some other functionality.

@SergeyKanzhelev is more nuanced, the overall problem of priorities is solved, the dra drivers connects both to Kubernetes system via DRA and to the container runtime system via NRI, so nothing will get scheduled to the node until the drivers publish the ResourceSlice, and there is also a NodePrepareResources hook that helps the driver to wait for any runtime problem ... the problem is that there is a tricky race condition from where the DRA NodePrepareResources hook is executed to when the Pod is started to be created by the runtime ... and this seems triggered by an update on the controller runtime (config change or similar) ... we just need to be able to close that race

@gauravkghildiyal the NodePrepareResources hook has a way to send a CDI value in its response, the plan is to use that, CDI and NRI are not exclusive, what we want is for the system to ensure that the race between the NodePrepareResource hook and the Pod creation in the runtime does not happen ... and for that we have a system via the required plugin annotation added recently in NRI , so the proposal will be to signal through CDI (NodePrepareResources response) that PodX has the required DRANET plugin annotation, so the runtime ensure DRANET is called back on Pod creation

Ok, I misread #282 (comment), so we are waiting for confirmation from @klihub about that approach working

[klihub]

@klihub This is great progress, thanks!

I have a couple of questions focused on verifying that this would cover our use case described at Proposal: Ensuring NRI Plugin Execution for DRA Workloads via CRI Annotations (You can ignore the solution described there for now and focus on just the problem)

I have 2 questions:

1. So far in DRANET, we don't use CDI at all and solely rely on NRI. I understand that with the current idea in this thread, we will need to use CDI solely for the purpose of making sure the NRI plugin gets executed. I don't consider this to be bad, but just confirming that I understood things correctly.
   As an example, this is what that CDI spec would look like that has a dummy placeholder device to just enforce the NRI invocation. (We don't require any other container edits or any other thing, but solely that NRI plugin execution is enforced)

Example CDI config to enforce NRI plugin execution

cdiVersion: "1.2.0"
kind: "dra.net/nri-enforcement"
devices:

• name: placeholder-for-making-nri-required
  containerEdits:
  annotations:
  required-plugins:
  value: |
  - dra.net
  format: stringSlice
  onConflict: append

Yes, this is the idea with the CDI injection approach. To do a CDI device injection which would inject the annotation for the required plugins. And I think that your proposal (to allow injecting container annotations from the DRA driver) is better in the sense, that all other details/aspects being equal it does not require a DRA driver/NRI plugin combo to use CDI for device (or pseudo-device) injection if the only extra thing it needs is annotating a required plugin.

2. Will the above also be sufficient to make sure our NRI hooks on RunPodSandbox are also necessarily invoked. The confusion that I'm trying to clear up is that this is not just limited to the Container related hooks (which is usually the impression I get from an API termed ContainerDeviceInterface).

If the answer to question 2 above is that "NO it doesn't already work with RunPodSandbox" and requires significant effort, I think at that point we should start questioning whether using CDI to enforce NRI is indeed the right approach, or if we perhaps need to solve this by making DRA/kubelet pass in NRI required annotations through CRI (which is the angle I was thinking of in the doc Proposal: Ensuring NRI Plugin Execution for DRA Workloads via CRI Annotations

No, it would not ensure that the NRI plugin is around during a RunPodSandbox. And we might need to extend our current plumbing if we want to be able to do that.

[SergeyKanzhelev]

I think the problem of DRANET is not a DRA problem, it is a NRI framework problem. I think it is not the first time NRI authors will ask for a persistent self-registration of a plugin with the container runtime. If we will solve this problem for DRANET, there will be another similar ask for some other functionality.

@SergeyKanzhelev is more nuanced, the overall problem of priorities is solved, the dra drivers connects both to Kubernetes system via DRA and to the container runtime system via NRI, so nothing will get scheduled to the node until the drivers publish the ResourceSlice, and there is also a NodePrepareResources hook that helps the driver to wait for any runtime problem ... the problem is that there is a tricky race condition from where the DRA NodePrepareResources hook is executed to when the Pod is started to be created by the runtime ... and this seems triggered by an update on the controller runtime (config change or similar) ... we just need to be able to close that race

I understand the problem. I am just saying that I do not see this as a problem of coordinating DRA and NRI. I see it as a problem of NRI plugins in general to ensure "guaranteed execution".

[pravk03]

/cc

[pravk03]

Hi, I just wanted to bring in the dra-driver-cpu use case as it is susceptible to the same race condition, but the scenario is slightly different. We use a similar DRA + NRI plugin approach as DRANet, but in our case, we actually do use CDI.

The current implementation looks like this:

• For containers with claims: We generate a CDI spec with environment variables. The NRI plugin reads this and applies the allocated CPUs.
• For containers without claims: These do not reference any CDI devices. However, the NRI plugin must still process them to exclude the CPUs allocated to claims and assign them the shared pool CPUs.

@klihub, looking at your proposed CDI annotations mitigation, it seems it might not fully cover our use case. Since the proposed approach injects container annotations when a CDI device is injected, the containers without claims (and hence no CDI devices) will miss this annotation. This means we do not exclude claim allocated CPUs if the NRI plugin is not running during container creation. I think Proposal: Ensuring NRI Plugin Execution for DRA Workloads via CRI Annotations has a similar limitation unless we introduce a way to apply the annotations to all the pods.

I'd love to get your thoughts on how we might handle this kind of node-wide NRI requirement.

@ffromani FYI

[gauravkghildiyal]

@pravk03 -- I think your use case for CPU DRA Driver fits perfectly within a "Global Required NRI Plugin". See https://github.com/containerd/nri/blob/main/docs/validation-plugins.md#1-globally-required-always (Screenshot below)

[SergeyKanzhelev]

I am trying to summarize a few discussions we had on how to best address this.

May work today

global required path

Mark the DRANET as global required NRI plugin. Solves the problem, but creates unnecessary installation and clean up burden as it requires changing of containerd configuration file.

webhook/explicit set of Pod annotation

Outside of DRA to NRI - for other features - there will be a need to specify that the feature is only supported when a specific NRI is present. Since there are no other callbacks, the way to ensure that the feature will work is explicitly or via webhook specify that specific NRI plugin is required. Doing the same for DRANET is a option that will work today.

Require changes

new way for NRI plugins to declare themselves "required"

The proposal is to allow any NRI plugin to self-register itself as "required" and ask container runtime to preserve this information across restarts. So NRI plugin will guaranteed to be used for all Pods while it is active. The benefit of this approach is that it will allow a more reliable registration of NRI plugins, even outside of DRA and NRI combination. But it also may render Pod unusable if DRA plugin was deleted while Pod using it is still running. The Pod will run without NRI callback execution (note, this is not really a supported scenario as DRA plugin is required for the duration of Pod lifecycle for cases of node restart and to unprepare resources).

extend cdi devices to allow adding annotations

Improve CDI devices to support adding annotations AND allow to use CDI devices for sandbox creation. This path is most aligned with other DRA plugins. However requires some design work on how CDI devices apply to sandboxes and how to add annotations to CDI devices and align with existing annotations. This path is promising as it's best aligned with other DRA plugins and the CDI purpose.

@pravk03 my understanding of this proposal is the the annotation will be added to the Pod and thus be used for ALL containers inside the Pod.

support adding annotation from NodePrepareResources

The proposal is to return annotations from NodePrepareResources require kubelet to add those to Sandbox (and containers?). Some research will be needed on whether just passing those to sandbox will be enough or they will need to be preserved for the duration of a pod lifecycle for all subsequent containers creations. This is the most generic approach, that enables future experimentation. But it also requires to carry this collection alongside the CDI devices, while the purpose of CDI devices was to encapsulate all changes needed to enable a container.