ctr: implement CDI support by bart0sh · Pull Request #6204 · containerd/containerd

bart0sh · 2021-11-04T13:11:50Z

Implement support of the Container Device Interface (CDI) in ctr.

CDI goal is to update device-related information (devices, mounts, hooks, environment variables etc) in the OCI spec from the JSON files provided by device vendors. That would allow workloads to access properly configured devices.

The plan is to have CDI support in at least containerd, cri-o and podman. It's already implemented in podman: containers/podman#10081

Here is a typical usage scenario:

vendor provides a spec update in the CDI JSON format, e.g:

$ cat /etc/cdi/test.json 
{
  "cdiVersion": "0.2.0",
  "kind": "vendor.com/device",
  "devices": [
    {
      "name": "myDevice",
      "containerEdits": {
        "deviceNodes": [
          {"path": "/dev/device1", "type": "b", "major": 7, "minor": 0, "fileMode": 432, "permissions": "rw", "uid": 1000, "gid": 1000}
        ]
      }
    }
  ],
  "containerEdits": {
    "env": [
      "FOO=VALID_SPEC",
      "BAR=BARVALUE1"
    ],
    "deviceNodes": [
      {"path": "/dev/device1", "type": "b", "major": 7, "minor": 0, "fileMode": 432, "permissions": "rw", "uid": 1000, "gid": 1000}
    ],
    "mounts": [
       {"hostPath": "/bin/vendorBin", "containerPath": "/bin/vendorBin", "options": ["bind"]}
    ],
    "hooks": [
       {"hookName": "startContainer", "path": "/bin/vendorHook", "args": ["/bin/vendorHook", "param1", "param2"]}
    ]
  }
}

OCI spec gets updated by the ctr/containerd/other runtime and passed to the OCI runtime
workload can access the device inside a container

k8s-ci-robot · 2021-11-04T13:11:59Z

Hi @bart0sh. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

theopenlab-ci · 2021-11-04T13:19:58Z

Build succeeded.

containerd-build-arm64 : FAILURE in 1m 37s (non-voting)
containerd-test-arm64 : FAILURE in 1m 41s (non-voting)
containerd-integration-test-arm64 : FAILURE in 5m 02s (non-voting)

bart0sh · 2021-11-04T13:26:36Z

I can't reproduce any of above CI failures locally. Can anybody point me out how to fix those?

bart0sh · 2021-11-04T13:33:00Z

/cc @mikebrow @kad @klihub

k8s-ci-robot · 2021-11-04T13:33:02Z

@bart0sh: GitHub didn't allow me to request PR reviews from the following users: kad, klihub.

Note that only containerd members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @mikebrow @kad @klihub

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

AkihiroSuda · 2021-11-04T16:28:16Z

Please explain the goal and usage

bart0sh · 2021-11-05T11:31:21Z

@AkihiroSuda done, please review.

AkihiroSuda · 2021-11-05T11:42:46Z

#6195

this is an alternative approach to NRI: https://github.com/klihub/nri/blob/5f41c0fbfab8c026f94db5e3f3dc91ce9ff509bf/v2alpha1/plugins/cdi-resolver/cdi-resolver.go#L48-L60

@bart0sh
Could you explain why we can't use NRI?

bart0sh · 2021-11-05T12:59:43Z

@AkihiroSuda We can, but it's much longer term plan, I'd say.

bart0sh · 2021-11-05T15:16:03Z

@AkihiroSuda More details on this.
This code https://github.com/klihub/nri/blob/5f41c0fbfab8c026f94db5e3f3dc91ce9ff509bf/v2alpha1/plugins/cdi-resolver/cdi-resolver.go#L48-L60 is not merged yet and it would take some time to get it merged.

Even if it's merged it would cover only CRI use case. Covering other types of clients (ctr, Docker, etc) would require even more work and there is no guarantee when/if it will be done.

klihub · 2021-11-08T13:49:39Z

 	},
+	cli.StringSliceFlag{
+		Name:  "cdi",
+		Usage: "CDI device in the vendor.com/device=<device name> format to add to the container",


nitpick: The usage string seems to imply that the accepted CDI devices names are restricted to the format mentioned. However, I don't see this being checked in the code and AFAIK the current CDI implementation does not implement such a restriction either.

It should be done by CDI in my opinion. I'll create an issue about it.

klihub · 2021-11-08T14:00:18Z

I can't reproduce any of above CI failures locally. Can anybody point me out how to fix those?

Isn't at least one of those failures related to cross-builds (darwin) not finding the newly added WithCDIdevices() function because it is put in spec_opts_linux.go ?

theopenlab-ci · 2021-11-09T11:25:24Z

Build succeeded.

containerd-build-arm64 : FAILURE in 2m 43s (non-voting)
containerd-test-arm64 : FAILURE in 3m 35s (non-voting)
containerd-integration-test-arm64 : FAILURE in 5m 37s (non-voting)

bart0sh · 2021-11-09T11:26:01Z

Isn't at least one of those failures related to cross-builds (darwin) not finding the newly added WithCDIdevices() function because it is put in spec_opts_linux.go ?

Moved it to the spec_opts_unix.go, but it didn't help for some reason.

klihub · 2021-11-09T18:28:01Z

Isn't at least one of those failures related to cross-builds (darwin) not finding the newly added WithCDIdevices() function because it is put in spec_opts_linux.go ?

Moved it to the spec_opts_unix.go, but it didn't help for some reason.

Yes, but now it fails on linux because spec_opts_unix.go is tagged // +build !linux,!windows. I guess you need to have a real implementation in both spec_opts_{linux,unix}.go, and then probably have a stub that just errors out for spec_opts_windows.go.

AkihiroSuda · 2021-11-11T05:01:55Z

@AkihiroSuda More details on this. This code https://github.com/klihub/nri/blob/5f41c0fbfab8c026f94db5e3f3dc91ce9ff509bf/v2alpha1/plugins/cdi-resolver/cdi-resolver.go#L48-L60 is not merged yet and it would take some time to get it merged.

The cdi-resolver PR seems marked as "ready for review" recently. containerd/nri#16

Even if it's merged it would cover only CRI use case. Covering other types of clients (ctr, Docker, etc) would require even more work and there is no guarantee when/if it will be done.

We can easily extend ctr (and/or nerdctl) to support NRI.
I'm not sure when Docker/Moby can support NRI, but that question applies to CDI too :)

theopenlab-ci · 2021-11-11T10:46:18Z

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

theopenlab-ci · 2021-11-11T10:56:42Z

Build succeeded.

containerd-build-arm64 : FAILURE in 1m 59s (non-voting)
containerd-test-arm64 : FAILURE in 2m 08s (non-voting)
containerd-integration-test-arm64 : FAILURE in 5m 35s (non-voting)

theopenlab-ci · 2021-11-11T13:46:13Z

Build succeeded.

containerd-build-arm64 : FAILURE in 2m 00s (non-voting)
containerd-test-arm64 : FAILURE in 2m 00s (non-voting)
containerd-integration-test-arm64 : FAILURE in 21m 30s (non-voting)

Signed-off-by: Ed Bartosh <[email protected]>

theopenlab-ci · 2021-11-11T16:23:23Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 18s (non-voting)
containerd-test-arm64 : SUCCESS in 6m 02s (non-voting)
containerd-integration-test-arm64 : SUCCESS in 24m 40s (non-voting)

adrianchiris · 2022-02-17T11:16:23Z

I didnt find any related issue in containerd to support CDI just this PR to comment so posting here.

Just wanted to add a +1 for adding support for CDI in containerd.

we have a use-case to inject RDMA [1] resources to containers to allow applications to use RDMA.
using CDI we could provide additional mounts to expose those resources (linux character devices)

[1] https://community.mellanox.com/s/article/What-is-RDMA

kzys · 2022-03-11T19:34:56Z

Both #6654 and this PR adds CDI into containerd's dependencies. We probably should review one of them first. Maybe #6654?

bart0sh · 2022-03-20T21:15:14Z

@kzys yes, #6654 is more important. I'm closing this one to avoid confusion.

k8s-ci-robot added the needs-ok-to-test label Nov 4, 2021

k8s-ci-robot requested a review from mikebrow November 4, 2021 13:33

AkihiroSuda added the status/more-info-needed Awaiting contributor information label Nov 4, 2021

AkihiroSuda added status/needs-discussion Needs discussion and decision from maintainers and removed status/more-info-needed Awaiting contributor information labels Nov 5, 2021

klihub reviewed Nov 8, 2021

View reviewed changes

bart0sh force-pushed the PR002-ctr-support-CDI branch from 1f41629 to 8a77bcc Compare November 9, 2021 11:18

bart0sh force-pushed the PR002-ctr-support-CDI branch from 8a77bcc to a504135 Compare November 11, 2021 10:46

bart0sh force-pushed the PR002-ctr-support-CDI branch from a504135 to 5e80a68 Compare November 11, 2021 10:49

bart0sh force-pushed the PR002-ctr-support-CDI branch from 5e80a68 to 8864a9c Compare November 11, 2021 13:22

ctr: implement CDI support

9855017

Signed-off-by: Ed Bartosh <[email protected]>

bart0sh force-pushed the PR002-ctr-support-CDI branch from 8864a9c to 9855017 Compare November 11, 2021 15:57

klihub mentioned this pull request Nov 28, 2021

Proposal to Clean up/Update the CDI API. cncf-tags/container-device-interface#38

Closed

bart0sh closed this Mar 20, 2022

bart0sh mentioned this pull request May 17, 2023

Add CDI support to ctr command line cncf-tags/container-device-interface#142

Closed

elezar mentioned this pull request May 17, 2023

Add support for CDI devices to ctr run command #8525

Merged

Conversation

bart0sh commented Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Nov 4, 2021

Uh oh!

theopenlab-ci Bot commented Nov 4, 2021

Uh oh!

bart0sh commented Nov 4, 2021

Uh oh!

bart0sh commented Nov 4, 2021

Uh oh!

k8s-ci-robot commented Nov 4, 2021

Uh oh!

AkihiroSuda commented Nov 4, 2021

Uh oh!

bart0sh commented Nov 5, 2021

Uh oh!

AkihiroSuda commented Nov 5, 2021

Uh oh!

bart0sh commented Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bart0sh commented Nov 5, 2021

Uh oh!

Uh oh!

klihub Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

bart0sh Nov 9, 2021

Choose a reason for hiding this comment

Uh oh!

klihub commented Nov 8, 2021

Uh oh!

theopenlab-ci Bot commented Nov 9, 2021

Uh oh!

bart0sh commented Nov 9, 2021

Uh oh!

klihub commented Nov 9, 2021

Uh oh!

AkihiroSuda commented Nov 11, 2021

Uh oh!

theopenlab-ci Bot commented Nov 11, 2021

Uh oh!

theopenlab-ci Bot commented Nov 11, 2021

Uh oh!

theopenlab-ci Bot commented Nov 11, 2021

Uh oh!

theopenlab-ci Bot commented Nov 11, 2021

Uh oh!

adrianchiris commented Feb 17, 2022

Uh oh!

kzys commented Mar 11, 2022

Uh oh!

bart0sh commented Mar 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bart0sh commented Nov 4, 2021 •

edited

Loading

bart0sh commented Nov 5, 2021 •

edited

Loading