[Carry #2280] Add support for CDI devices to device flag by djdongjin · Pull Request #4170 · containerd/nerdctl

djdongjin · 2025-04-27T01:15:50Z

Copilot

Pull Request Overview

This PR adds support for Container Device Interface (CDI) by introducing new functions to obtain CDI specification directories across supported platforms, updating configuration and CLI flags accordingly, and integrating CDI device injection into container creation.

Adds CDISpecDirs functions in the defaults package for Windows, Linux, FreeBSD, and Darwin.
Updates the config structure, CLI flags, and tests to support CDI device injection.
Integrates CDI devices handling into container run and create commands.

Reviewed Changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
pkg/defaults/defaults_windows.go	Adds a stub CDISpecDirs function returning an empty slice for Windows.
pkg/defaults/defaults_linux.go	Implements CDISpecDirs with distinct behavior based on rootless mode.
pkg/defaults/defaults_freebsd.go	Provides a CDISpecDirs function returning default CDI spec directories.
pkg/defaults/defaults_darwin.go	Provides a CDISpecDirs function returning default CDI spec directories.
pkg/config/config.go	Introduces a new field for CDI spec directories in the configuration.
pkg/cmd/container/run_cdi.go	Adds a function to configure CDI devices during container run.
pkg/cmd/container/create.go	Injects the CDI devices option into the container creation process.
pkg/api/types/container_types.go	Adds a CDIDevices field to the container create options structure.
docs/config.md	Documents the new --cdi-spec-dirs CLI flag and configuration.
cmd/nerdctl/testdata/cdi/vendor1.yaml	Provides an example CDI vendor file for testing purposes.
cmd/nerdctl/main.go	Introduces the CLI flag for CDI spec directories.
cmd/nerdctl/helpers/flagutil.go	Processes the new cdi-spec-dirs flag and integrates it into global options.
cmd/nerdctl/container/container_run_linux_test.go	Adds tests to verify the CDI device injection functionality.
cmd/nerdctl/container/container_create.go	Differentiates between standard devices and CDI devices during creation.
.github/workflows/test.yml	Temporarily enables CDI support in the Docker daemon for integration tests.

Files not reviewed (1)

go.mod: Language not supported

AkihiroSuda · 2025-04-28T11:41:21Z

 | `host_gateway_ip`   | `--host-gateway-ip`                | `NERDCTL_HOST_GATEWAY_IP` | IP address that the special 'host-gateway' string in --add-host resolves to. Defaults to the IP address of the host. It has no effect without setting --add-host | Since 1.3.0      |
 | `bridge_ip`         | `--bridge-ip`                      | `NERDCTL_BRIDGE_IP`       | IP address for the default nerdctl bridge network, e.g., 10.1.100.1/24                                                                                           | Since 2.0.1      |
 | `kube_hide_dupe`    | `--kube-hide-dupe`                 |                           | Deduplicate images for Kubernetes with namespace k8s.io, no more redundant <none> ones are displayed    | Since 2.0.3      |
+| `cdi_spec_dirs`     | `--cdi-spec-dirs`                   |                          | The folders to use when searching for CDI specifications.    | Since 2.0.5 |


CDI needs explanation

apostasie

@djdongjin one minor nit on help

apostasie · 2025-04-28T16:36:23Z

 	helpers.AddPersistentStringFlag(rootCmd, "host-gateway-ip", nil, nil, nil, aliasToBeInherited, cfg.HostGatewayIP, "NERDCTL_HOST_GATEWAY_IP", "IP address that the special 'host-gateway' string in --add-host resolves to. Defaults to the IP address of the host. It has no effect without setting --add-host")
 	helpers.AddPersistentStringFlag(rootCmd, "bridge-ip", nil, nil, nil, aliasToBeInherited, cfg.BridgeIP, "NERDCTL_BRIDGE_IP", "IP address for the default nerdctl bridge network")
 	rootCmd.PersistentFlags().Bool("kube-hide-dupe", cfg.KubeHideDupe, "Deduplicate images for Kubernetes with namespace k8s.io")
+	rootCmd.PersistentFlags().StringSlice("cdi-spec-dirs", cfg.CDISpecDirs, "The directories to search for CDI spec files. Defaults to /etc/cdi,/var/run/cdi")


We do not mention the defaults for CNI for eg - maybe we can remove them here as well? (or alternatively, mention the rootless defaults as well)

perhaps another pr..

mikebrow · 2025-04-29T16:20:57Z

 | `host_gateway_ip`   | `--host-gateway-ip`                | `NERDCTL_HOST_GATEWAY_IP` | IP address that the special 'host-gateway' string in --add-host resolves to. Defaults to the IP address of the host. It has no effect without setting --add-host | Since 1.3.0      |
 | `bridge_ip`         | `--bridge-ip`                      | `NERDCTL_BRIDGE_IP`       | IP address for the default nerdctl bridge network, e.g., 10.1.100.1/24                                                                                           | Since 2.0.1      |
 | `kube_hide_dupe`    | `--kube-hide-dupe`                 |                           | Deduplicate images for Kubernetes with namespace k8s.io, no more redundant <none> ones are displayed    | Since 2.0.3      |
+| `cdi_spec_dirs`     | `--cdi-spec-dirs`                   |                          | The folders to use when searching for CDI ([container-device-interface](https://github.com/cncf-tags/container-device-interface)) specifications.    | Since 2.1.0 |


folders :-)

mikebrow

LGTM

AkihiroSuda

Thanks, will merge after releasing v2.0.5

AkihiroSuda · 2025-05-01T03:24:12Z

Needs rebase

This change adds support for specifying fully-qualified CDI device names in the --device flag. This allows the Container Device Interface (CDI) to be used to inject devices into container being run. Signed-off-by: Evan Lezar <[email protected]> Enable cdi feature for Docker Signed-off-by: Evan Lezar <[email protected]> Update go mod Signed-off-by: Evan Lezar <[email protected]> Signed-off-by: Jin Dong <[email protected]> use containerd cdi.WithCDIDevices Signed-off-by: Jin Dong <[email protected]>

djdongjin force-pushed the carry-8525-add-cdi-support branch 4 times, most recently from 548458e to 449537e Compare April 27, 2025 02:47

djdongjin requested a review from Copilot April 27, 2025 02:48

Copilot AI reviewed Apr 27, 2025

View reviewed changes

djdongjin changed the title ~~[Carry 8525] add cdi support~~ [Carry 8525] Add support for CDI devices to device flag Apr 27, 2025

djdongjin changed the title ~~[Carry 8525] Add support for CDI devices to device flag~~ [Carry #8525] Add support for CDI devices to device flag Apr 27, 2025

djdongjin changed the title ~~[Carry #8525] Add support for CDI devices to device flag~~ [Carry #2280] Add support for CDI devices to device flag Apr 27, 2025

djdongjin requested review from AkihiroSuda and mikebrow April 27, 2025 02:51

djdongjin marked this pull request as ready for review April 27, 2025 02:58

djdongjin force-pushed the carry-8525-add-cdi-support branch 4 times, most recently from 9af8317 to efe0417 Compare April 27, 2025 16:06

apostasie reviewed Apr 27, 2025

View reviewed changes

Comment thread cmd/nerdctl/container/container_run_linux_test.go Outdated

apostasie reviewed Apr 27, 2025

View reviewed changes

Comment thread cmd/nerdctl/container/container_run_linux_test.go

apostasie reviewed Apr 27, 2025

View reviewed changes

Comment thread cmd/nerdctl/container/container_run_linux_test.go Outdated

djdongjin force-pushed the carry-8525-add-cdi-support branch from efe0417 to 8e649bd Compare April 27, 2025 19:07

djdongjin requested a review from apostasie April 27, 2025 19:58

AkihiroSuda reviewed Apr 28, 2025

View reviewed changes

Comment thread pkg/defaults/defaults_linux.go Outdated

AkihiroSuda reviewed Apr 28, 2025

View reviewed changes

Comment thread docs/config.md Outdated

AkihiroSuda reviewed Apr 28, 2025

View reviewed changes

djdongjin force-pushed the carry-8525-add-cdi-support branch from 8e649bd to c34d651 Compare April 28, 2025 13:38

djdongjin requested a review from AkihiroSuda April 28, 2025 13:39

djdongjin added this to the v2.1.0 (tentative) milestone Apr 28, 2025

apostasie approved these changes Apr 28, 2025

View reviewed changes

mikebrow reviewed Apr 29, 2025

View reviewed changes

mikebrow approved these changes Apr 29, 2025

View reviewed changes

AkihiroSuda approved these changes Apr 29, 2025

View reviewed changes

djdongjin force-pushed the carry-8525-add-cdi-support branch from c34d651 to 71c646f Compare May 1, 2025 14:18

AkihiroSuda approved these changes May 1, 2025

View reviewed changes

AkihiroSuda merged commit 8d7539a into containerd:main May 1, 2025
83 of 87 checks passed

djdongjin mentioned this pull request May 1, 2025

Add support for CDI devices to device flag #2280

Closed

This was referenced May 2, 2025

[FIX]: #4012 follow-ups (fixes and target test binaries) #4180

Merged

Update the list of the unimplemented Docker features (and implement them) #3867

Open

mook-as mentioned this pull request May 12, 2025

rddepman: bump WSLDistro from 0.82 to 0.83 rancher-sandbox/rancher-desktop#8624

Merged

Conversation

djdongjin commented Apr 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AkihiroSuda Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

djdongjin Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

apostasie left a comment

Choose a reason for hiding this comment

Uh oh!

apostasie Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

mikebrow Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

mikebrow Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented May 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

djdongjin commented Apr 27, 2025 •

edited

Loading