Add support for CDI devices to device flag #2280

elezar · 2023-06-07T14:29:59Z

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.

This mirrors the support recently added to ctr (see containerd/containerd#8525) and under review for the Docker CLI (see docker/cli#4084).

Note that this assumes that the CLI is used to inject devices and additional work would be required for other mechanisms such as compose files.

elezar · 2023-06-07T15:18:57Z

/cc @mikebrow

Do you know who should have a look at this? Changes are similar to containerd/containerd#8525.

AkihiroSuda · 2023-06-08T00:20:05Z

go.mod

+	github.com/syndtr/gocapability v0.0.0-20200815063812-42c35b437635 // indirect
+	google.golang.org/genproto/googleapis/rpc v0.0.0-20230530153820-e85fd2cbaebc // indirect
+	gopkg.in/yaml.v2 v2.4.0 // indirect
+	sigs.k8s.io/yaml v1.3.0 // indirect


Can we minimize these dependencies?

The code for loading the CDI specifications reads YAML files -- and contains the functionaility to set up watchers on the filesystem that updates the spec in daemonized cases. We are looking to reduce the set of dependencies required, but that work has not yet been started.

Are there any dependencies in particular that you are concerned about? (To be clear, this should be a similar set to those for containerd which includes the same feature when using CRI).

AkihiroSuda · 2023-06-08T00:22:23Z

cmd/nerdctl/container_create.go

 	if err != nil {
 		return
 	}
+	for _, device := range allDevices {
+		if cdi.IsQualifiedName(device) {
+			opt.CDIDevices = append(opt.CDIDevices, device)


I guess we want to keep this experimental until the syntax gets accepted in Docker CLI

https://github.com/containerd/nerdctl/blob/main/docs/experimental.md?plain=1
https://github.com/search?q=repo%3Acontainerd%2Fnerdctl+checkExperimental&type=code

Can we have an integration test too?

Sure. I will add an integration test and also gate this behind an experimental flag.

The Docker CLI PR has been merged, so I will pick up this PR again in the next couple of days.

@elezar ping?

Sorry for the delay. I will look at this again this week.

@AkihiroSuda do we still need to make this feature experimental?

If it is consistent with Docker's behavior, it does not need to stay experimental

It is consistent, but requries an op-in feature flag in the docker config.

The --cdi-spec-dirs folder is also not a docker command line option, but I assume that that is ok.

AkihiroSuda · 2023-08-14T11:33:44Z

Needs rebase

AkihiroSuda · 2024-06-20T07:00:32Z

Sorry, needs another rebase

AkihiroSuda · 2024-07-11T19:41:29Z

ping @elezar

elezar · 2024-07-12T06:08:38Z

I will look into this today.

elezar · 2024-07-12T11:58:59Z

cmd/nerdctl/container_run_linux_test.go

+	assert.NilError(t, err)
+	cdiSpecDir := filepath.Join(cwd, "testdata", "cdi")
+
+	os.Setenv("CDI_SPEC_DIRS", cdiSpecDir)


I have added a basic test that sets the CDI_SPEC_DIRS option via an envvar. Do you want me to add one that sets it via the command line argument too?

Yes, that would be nice.
It should be also supported via TOML: https://github.com/containerd/nerdctl/blob/main/docs/config.md

Do you have an example of a test that sets the config via toml?

nerdctl/cmd/nerdctl/main_test.go

Lines 46 to 73 in 370cab6

// TestNerdctlConfig validates the configuration precedence [CLI, Env, TOML, Default].

func TestNerdctlConfig(t *testing.T) {

testutil.DockerIncompatible(t)

t.Parallel()

tomlPath := filepath.Join(t.TempDir(), "nerdctl.toml")

err := os.WriteFile(tomlPath, []byte(`

snapshotter = "dummy-snapshotter-via-toml"

`), 0400)

assert.NilError(t, err)

base := testutil.NewBase(t)

// [Default]

base.Cmd("info", "-f", "{{.Driver}}").AssertOutExactly(containerd.DefaultSnapshotter + "\n")

// [TOML, Default]

base.Env = append(base.Env, "NERDCTL_TOML="+tomlPath)

base.Cmd("info", "-f", "{{.Driver}}").AssertOutExactly("dummy-snapshotter-via-toml\n")

// [CLI, TOML, Default]

base.Cmd("info", "-f", "{{.Driver}}", "--snapshotter=dummy-snapshotter-via-cli").AssertOutExactly("dummy-snapshotter-via-cli\n")

// [Env, TOML, Default]

base.Env = append(base.Env, "CONTAINERD_SNAPSHOTTER=dummy-snapshotter-via-env")

base.Cmd("info", "-f", "{{.Driver}}").AssertOutExactly("dummy-snapshotter-via-env\n")

// [CLI, Env, TOML, Default]

base.Cmd("info", "-f", "{{.Driver}}", "--snapshotter=dummy-snapshotter-via-cli").AssertOutExactly("dummy-snapshotter-via-cli\n")

}

elezar · 2024-07-12T12:00:15Z

cmd/nerdctl/main.go

@@ -175,6 +175,7 @@ func initRootCmdFlags(rootCmd *cobra.Command, tomlPath string) (*pflag.FlagSet,
 	// Experimental enable experimental feature, see in https://github.com/containerd/nerdctl/blob/main/docs/experimental.md
 	AddPersistentBoolFlag(rootCmd, "experimental", nil, nil, cfg.Experimental, "NERDCTL_EXPERIMENTAL", "Control experimental: https://github.com/containerd/nerdctl/blob/main/docs/experimental.md")
 	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")
+	AddPersistentStringSliceFlag(rootCmd, "cdi-spec-dirs", nil, nil, cfg.CDISpecDirs, "CDI_SPEC_DIRS", "The directories to search for CDI spec files. Defaults to /etc/cdi,/var/run/cdi")


I'm happy to use the existing AddPersistentStringArrayFlag here instead, but then we need to change the command line argument to --cdi-spec-dir and make it clear that it's repeatable. I'm not sure whether the envvar would be split on commas then either.

Is this env var specific to the nerdctl implementation, or is it part of the CDI specification?
I guess the env var should use : on Linux and ; on Windows as the separator, just like $PATH (%PATH%)?

The envvar is not part of the CDI specification and is a nerdctl-specific implementation detail. I'm happy to defer to you on how to implement this though. Should both the command line argument --cdi-spec-dirs and the CDI_SPEC_DIRS envvar accept a : (or ;)-separated list of paths instead of a comma-separated list?

The CLI flag should not use colon separators.

Also I wonder if the env var is really necessary. Maybe it should be revisited in a separate PR?

OK. I can remove the envvar for now and only use the CLI flags. It is probably cleaner for the time being.

elezar · 2024-07-12T12:00:43Z

docs/config.md

@@ -45,6 +45,7 @@ experimental   = true
 | `hosts_dir`         | `--hosts-dir`                      |                           | `certs.d` directory                                                                                                                                              | Since 0.16.0     |
 | `experimental`      | `--experimental`                   | `NERDCTL_EXPERIMENTAL`    | Enable  [experimental features](experimental.md)                                                                                                                 | Since 0.22.3     |
 | `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      |
+| `cdi_spec_dirs`     | `--cdi-spec-dirs`                   | `CDI_SPEC_DIRS`               | The folders to use when searching for CDI specifications.| experimental |


We could drop the experimental.

elezar · 2024-07-12T12:02:02Z

go.mod

+	github.com/syndtr/gocapability v0.0.0-20200815063812-42c35b437635 // indirect
+	gopkg.in/yaml.v2 v2.4.0 // indirect
+	sigs.k8s.io/yaml v1.3.0 // indirect
+	tags.cncf.io/container-device-interface v0.8.0


Is see that there are multiple require sections in the module file. Are there specific sections where I should add these?

The sections are being unified:

Update go.mod #3216

elezar · 2024-07-12T13:20:42Z

Looking at the failing tests these are:

Rootless tests
Docker compatibility tests

For the first the question I would have is whether the testdata/cdi directory is NOT accessible in some way when running rootless.

For Docker compatibility, we would need to enable the cdi feature that has been available in Docker since v25. We would also need to configure the Docker daemon to use the same CDI folder as for the tests.

Do you have any guidance on how to address these failures?

AkihiroSuda · 2024-07-16T05:36:41Z

For the first the question I would have is whether the testdata/cdi directory is NOT accessible in some way when running rootless.

This might be being fixed in:

Makefile fixes and other spring cleanup #3213

For Docker compatibility, we would need to enable the cdi feature that has been available in Docker since v25. We would also need to configure the Docker daemon to use the same CDI folder as for the tests.

Yes, you can update /etc/docker/daemon.json and sudo systemctl restart docker here

nerdctl/.github/workflows/test.yml

Line 225 in 0c3c28a

test-integration-docker-compatibility:

AkihiroSuda · 2024-07-17T12:59:38Z

.github/workflows/test.yml

+      - name: Enable CDI in the Docker daemon.
+        run: |
+          sudo mkdir -p /etc/docker
+          echo '{ "features": {"cdi": true} }' | sudo tee /etc/docker/daemon.json


This should be merged into daemon.json without overwriting its existing content (if any)

AkihiroSuda · 2024-07-17T13:00:48Z

pkg/config/config.go

@@ -58,5 +60,6 @@ func New() *Config {
 		HostsDir:         ncdefaults.HostsDirs(),
 		Experimental:     true,
 		HostGatewayIP:    ncdefaults.HostGatewayIP(),
+		CDISpecDirs:      []string{"/etc/cdi", "/var/run/cdi"},


This should respect XDG dirs for rootless
https://github.com/containerd/nerdctl/blob/main/pkg/rootlessutil/xdg_linux.go

AkihiroSuda · 2024-07-17T13:01:08Z

pkg/config/config.go

@@ -39,6 +39,8 @@ type Config struct {
 	HostsDir         []string `toml:"hosts_dir"`
 	Experimental     bool     `toml:"experimental"`
 	HostGatewayIP    string   `toml:"host_gateway_ip"`
+	// CDISpecDirs is a list of directories in which CDI specifications can be found.
+	CDISpecDirs []string `toml:"cdi_spec_dirs,omitempty"`


bike shedding: "Dirs" vs "Path"

"CDIPath" might be more consistent with the existing "CNIPath"?

The point is that these are specifically directories and not files. Furthermore, the naming is consistent with this setting in other projects: containerd, cri-o, docker, singularity.

I'm happy to change if you feel strongly about it.

the naming is consistent with this setting in other projects: containerd, cri-o, docker, singularity.

👍

AkihiroSuda · 2024-08-23T02:37:46Z

Needs rebase

AkihiroSuda · 2024-09-04T15:18:03Z

ping @elezar

AkihiroSuda · 2024-10-03T07:13:16Z

@elezar Do you plan to rebase this ?

elezar · 2024-10-09T15:21:51Z

@elezar Do you plan to rebase this ?

Yes, I do need to get to it. I have been focussing on a security issue of late. I will pick this up again in the next couple of days.

AkihiroSuda · 2024-10-24T12:43:12Z

CI failing

AkihiroSuda · 2024-10-29T11:53:41Z

Needs rebase

AkihiroSuda · 2024-11-05T21:22:52Z

Moved to v2.1 milestone

djdongjin · 2025-02-18T22:58:41Z

Hi @elezar 👋 any plan to pick this up? If not I can probably carry it over (no rush).

elezar · 2025-02-25T21:00:12Z

Hi @djdongjin I have to pick this up again, but have been busy with a couple of high-priority tasks otherwise.

I will make time for this over the next month though.

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]>

Signed-off-by: Evan Lezar <[email protected]>

elezar mentioned this pull request Jun 7, 2023

Add CDI support to nerdctl command line cncf-tags/container-device-interface#143

Closed

elezar force-pushed the add-cdi-support branch 2 times, most recently from 73defce to bde1b0a Compare June 7, 2023 14:42

AkihiroSuda reviewed Jun 8, 2023

View reviewed changes

elezar force-pushed the add-cdi-support branch from bde1b0a to b6931e4 Compare January 23, 2024 10:16

This was referenced Jan 23, 2024

Update container-device-interface to v0.6.2 containerd/containerd#9677

Merged

Bump clients to v0.6.2 cncf-tags/container-device-interface#176

Closed

elezar force-pushed the add-cdi-support branch 4 times, most recently from c00e0f5 to 0d4ec2a Compare February 12, 2024 11:01

AkihiroSuda added this to the v2.0.0 milestone Feb 12, 2024

elezar mentioned this pull request Apr 2, 2024

Bump clients to v0.7.1 cncf-tags/container-device-interface#199

Closed

11 tasks

elezar force-pushed the add-cdi-support branch 2 times, most recently from e44d28d to 0fdf318 Compare July 12, 2024 11:53

elezar commented Jul 12, 2024

View reviewed changes

elezar force-pushed the add-cdi-support branch from 0fdf318 to 85444d9 Compare July 12, 2024 12:07

elezar requested a review from AkihiroSuda July 12, 2024 12:22

elezar force-pushed the add-cdi-support branch from 85444d9 to cbce6e7 Compare July 16, 2024 12:49

elezar force-pushed the add-cdi-support branch 2 times, most recently from 39182f4 to ffecb94 Compare July 16, 2024 13:09

AkihiroSuda reviewed Jul 17, 2024

View reviewed changes

elezar force-pushed the add-cdi-support branch from ffecb94 to 007d138 Compare October 15, 2024 12:27

AkihiroSuda modified the milestones: v2.0.0, v2.1.0 (tentative) Nov 5, 2024

AkihiroSuda modified the milestones: v2.1.0 (tentative), v2.1.0 Nov 8, 2024

AkihiroSuda modified the milestones: v2.0.1, v2.x.x (tentative) Dec 2, 2024

AkihiroSuda mentioned this pull request Feb 9, 2025

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

Open

7 tasks

AkihiroSuda modified the milestones: v2.0.4, v2.x.x Feb 24, 2025

elezar added 3 commits February 28, 2025 10:23

Enable cdi feature for Docker

8615ae1

Signed-off-by: Evan Lezar <[email protected]>

Update go mod

558eeed

Signed-off-by: Evan Lezar <[email protected]>

elezar force-pushed the add-cdi-support branch from 007d138 to 558eeed Compare February 28, 2025 08:28

	// TestNerdctlConfig validates the configuration precedence [CLI, Env, TOML, Default].
	func TestNerdctlConfig(t *testing.T) {
	testutil.DockerIncompatible(t)
	t.Parallel()
	tomlPath := filepath.Join(t.TempDir(), "nerdctl.toml")
	err := os.WriteFile(tomlPath, []byte(`
	snapshotter = "dummy-snapshotter-via-toml"
	`), 0400)
	assert.NilError(t, err)
	base := testutil.NewBase(t)

	// [Default]
	base.Cmd("info", "-f", "{{.Driver}}").AssertOutExactly(containerd.DefaultSnapshotter + "\n")

	// [TOML, Default]
	base.Env = append(base.Env, "NERDCTL_TOML="+tomlPath)
	base.Cmd("info", "-f", "{{.Driver}}").AssertOutExactly("dummy-snapshotter-via-toml\n")

	// [CLI, TOML, Default]
	base.Cmd("info", "-f", "{{.Driver}}", "--snapshotter=dummy-snapshotter-via-cli").AssertOutExactly("dummy-snapshotter-via-cli\n")

	// [Env, TOML, Default]
	base.Env = append(base.Env, "CONTAINERD_SNAPSHOTTER=dummy-snapshotter-via-env")
	base.Cmd("info", "-f", "{{.Driver}}").AssertOutExactly("dummy-snapshotter-via-env\n")

	// [CLI, Env, TOML, Default]
	base.Cmd("info", "-f", "{{.Driver}}", "--snapshotter=dummy-snapshotter-via-cli").AssertOutExactly("dummy-snapshotter-via-cli\n")
	}

Add support for CDI devices to device flag #2280

Are you sure you want to change the base?

Add support for CDI devices to device flag #2280

Conversation

elezar commented Jun 7, 2023 • edited Loading

elezar commented Jun 7, 2023

Choose a reason for hiding this comment

elezar Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Aug 14, 2023

AkihiroSuda commented Jun 20, 2024

AkihiroSuda commented Jul 11, 2024

elezar commented Jul 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elezar commented Jul 12, 2024

AkihiroSuda commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Aug 23, 2024

AkihiroSuda commented Sep 4, 2024

AkihiroSuda commented Oct 3, 2024

elezar commented Oct 9, 2024

AkihiroSuda commented Oct 24, 2024

AkihiroSuda commented Oct 29, 2024

AkihiroSuda commented Nov 5, 2024

djdongjin commented Feb 18, 2025

elezar commented Feb 25, 2025

elezar commented Jun 7, 2023 •

edited

Loading

elezar Jun 8, 2023 •

edited

Loading