Add support for CDI devices to ctr run command#8525
Conversation
|
Hi @elezar. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
elezar
force-pushed
the
ctr-add-cdi-devices
branch
from
e3d1eef to
0de9fcc
Compare
|
/cc @bart0sh |
|
@elezar: GitHub didn't allow me to request PR reviews from the following users: bart0sh. Note that only containerd members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
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. |
elezar
force-pushed
the
ctr-add-cdi-devices
branch
2 times, most recently
from
cd1db00 to
9b039f2
Compare
| if limit != 0 { | ||
| opts = append(opts, oci.WithMemoryLimit(limit)) | ||
| } | ||
| var cdiDeviceIDs []string |
There was a problem hiding this comment.
A question to the reviewers: Does CTR read the containerd config, and if so, do we want to use the same feature flag to control this behaviour here. We may also, for example, want to configure the CDI registry with the configured CDI paths -- although this is not configurable for existing CLI-based implementations.
There was a problem hiding this comment.
I think in general, it may be a good idea to instantiate an instance of the CDI registry in the CLI code path explicily instead of relying on the instantiation in the (now) OCI package using the defaults.
There was a problem hiding this comment.
Does CTR read the containerd config
No
There was a problem hiding this comment.
Thanks for the clarification. I will update the PR to instantiate the singleton CDI registry with auto-refresh disabled and also provide a mechanism to configure CDI spec folders if desired at a later stage.
|
@mikebrow @AkihiroSuda @dmcgowan could you please take a look. |
|
/cc @mikebrow |
|
Please consider squashing commits |
OK. Let me rebase and do so. |
This change adds support for CDI devices to the ctr --device flag. If a fully-qualified CDI device name is specified, this is injected into the OCI specification before creating the container. Note that the CDI specifications and the devices that they represent are local and mirror the behaviour of linux devices in the ctr command. Signed-off-by: Evan Lezar <[email protected]>
elezar
force-pushed
the
ctr-add-cdi-devices
branch
from
9b039f2 to
d3887b2
Compare
Commits have been squashed. Thanks for the review. |
|
Did this make it into 1.7.2 (released Jun 3, whereas this was merged in May 31)? Or a later 1.7.x version (and if so, which one)? |
|
Or maybe the flags changed? I tried following the example in the OP comment: # ctr run --rm --device=nvidia.com/gpu=all docker.io/library/hello-world:latest helloworld /hello
ctr: error stating device path: stat nvidia.com/gpu=all: no such file or directoryVersion: # ctr version
Client:
Version: v1.7.5
Revision: fe457eb99ac0e27b3ce638175ef8e68a7d2bc373
Go version: go1.20.7
Server:
Version: v1.7.5
Revision: fe457eb99ac0e27b3ce638175ef8e68a7d2bc373
UUID: ee13c59e-d92f-45de-8169-24ae9e6865d5I am moderately sure I configured it correctly in [plugins]
[plugins."io.containerd.grpc.v1.cri"]
enable_cdi = true
cdi_spec_dirs = ["/etc/cdi", "/var/run/cdi"]and dump shows it enabled: # containerd config dump | grep cdi
cdi_spec_dirs = ["/etc/cdi", "/var/run/cdi"]
enable_cdi = trueEither way, can I request that this be part of the docs somewhere? |
|
@deitch I'm happy to update the docs. Do you know which ones should be updated for this? /cc @AkihiroSuda |
|
@elezar I do not. I sometimes struggle with finding the right docs. Are there docs for the I also imagine we should indicate minimum version(s) for using it. |
No, this is for v2.0. |
|
Ah that explains why I could not get it to work. What are my options for now, then? Build off master branch? When is 2.0 ETA? |
|
@deitch have you been able to verify this functionality with a v2.0 release such as https://github.com/containerd/containerd/releases/tag/v2.0.0-rc.1 |
|
Nope, haven't been on my device with special devices (that sounds odd) in a while. But it should be possible to test it even without? Create any old CDI file, run it with the command, it should create the config. Will get some errors on starting, because the actual devices do not exist, but the process of creating the runc container config should work? |
5 participants
This change adds support for requesting CDI devices to
ctrcommand line.This means that, assuming the relevant CDI specification exists, the following command can be used to run a container with access to the required device:
The requested devices are injected directly into the generated OCI runtime specification as
ctrdoes not use the CRI.This mirrors how device nodes are handled in that they are resolved locally and the edits are applied to the container config before calling the Create API.
Some minor cleanup / refactoring that makes this addition simpler was also done.
This builds on the work to add CDI support to the containerd server:
And replaces previous attempts here: