Add option in toolkit container to enable CDI in runtime #838

cdesiniotis · 2024-12-18T20:20:35Z

These changes add an option to enable CDI in container engines (containerd, crio, docker) from the toolkit contaienr.

This can be triggered through the --enable-cdi-in-runtime command line option or RUNTIME_ENABLE_CDI environment variable.

tariq1890 · 2024-12-19T23:30:22Z

tools/container/runtime/runtime.go

@@ -89,6 +90,13 @@ func Flags(opts *Options) []cli.Flag {
 			EnvVars:     []string{"NVIDIA_RUNTIME_SET_AS_DEFAULT", "CONTAINERD_SET_AS_DEFAULT", "DOCKER_SET_AS_DEFAULT"},
 			Hidden:      true,
 		},
+		&cli.BoolFlag{
+			Name:        "runtime-enable-cdi",


Let the flag be passed as --enable-cdi. The envar and the Option variable can have the Runtime/RUNTIME prefix. This is consistent with the other flags like socket, restart-mode etc

Suggested change

Name: "runtime-enable-cdi",

Name: "enable-cdi",

Let's try to be consistent with the option for the nvidia-ctk runtime configure command.

The problem is that we already have a cdi-enabled / enable-cdi flag defined in the toolkit options which triggers the generation of a CDI spec:

nvidia-container-toolkit/tools/container/toolkit/toolkit.go

Lines 173 to 179 in 8d869ac

&cli.BoolFlag{

Name: "cdi-enabled",

Aliases: []string{"enable-cdi"},

Usage: "enable the generation of a CDI specification",

Destination: &opts.cdiEnabled,

EnvVars: []string{"CDI_ENABLED", "ENABLE_CDI"},

},

Any ideas on what the name of the new flag should be?

Thanks for the clarification.

One question I would have is whether we need a separate flag? Would it not make sense for the same flag / envvar (e.g. CDI_ENABLED) to be use to also trigger enabling CDI in the runtime? Do older versions of containerd complain about invalid config options?

If we feel we want finer control, then I'm ok to use RUNTIME_ENABLE_CDI or ENABLE_CDI_IN_RUNTIME to be used. (maybe the latter).

tariq1890 · 2024-12-19T23:30:59Z

tools/container/runtime/runtime.go

@@ -89,6 +90,13 @@ func Flags(opts *Options) []cli.Flag {
 			EnvVars:     []string{"NVIDIA_RUNTIME_SET_AS_DEFAULT", "CONTAINERD_SET_AS_DEFAULT", "DOCKER_SET_AS_DEFAULT"},
 			Hidden:      true,
 		},
+		&cli.BoolFlag{
+			Name:        "runtime-enable-cdi",
+			Usage:       "Enable CDI in the configured runtime",


Suggested change

Usage: "Enable CDI in the configured runtime",

Usage: "Enable Container Device Interface (CDI) in the configured runtime",

elezar · 2024-12-20T14:35:00Z

pkg/config/engine/containerd/config_v1.go

@@ -163,3 +163,7 @@ func (c *ConfigV1) GetRuntimeConfig(name string) (engine.RuntimeConfig, error) {
 		tree: runtimeData,
 	}, nil
 }
+
+func (c *ConfigV1) EnableCDI() {
+	c.Set("enable_cdi", true)


I think we should be able to cast c to a Config type and call EnableCDI here instead of reimplementing it.

elezar · 2024-12-20T14:36:07Z

pkg/config/engine/api.go

@@ -24,6 +24,7 @@ type Interface interface {
 	RemoveRuntime(string) error
 	Save(string) (int64, error)
 	GetRuntimeConfig(string) (RuntimeConfig, error)
+	EnableCDI()


I think Set is only used to enable CDI. Let's remove that from the interface if that's the case.

elezar · 2024-12-20T14:36:52Z

tools/container/container.go

+	SetAsDefault     bool
+	RestartMode      string
+	HostRootMount    string
+	RuntimeEnableCDI bool


I don't agree with the Runtime* prefix here. Runtime in this context refers to the NVIDIA runtime. I would just use EnableCDI here.

elezar

Some minor comments.

elezar · 2025-01-16T13:59:26Z

tools/container/runtime/docker/docker.go

+	if o.RuntimeEnableCDI {
+		cfg.Set("features", map[string]bool{"cdi": true})
+	}
+


Note that we're already enabling CDI in the Configure call, so there is no need to do it here too.

Suggested change

if o.RuntimeEnableCDI {

cfg.Set("features", map[string]bool{"cdi": true})

}

elezar · 2025-01-16T14:00:49Z

tools/container/runtime/runtime.go

+		if opts.RuntimeEnableCDI {
+			opts.RuntimeEnableCDI = false
+		}


We already handle the case that EnableCDI is a no-op in the case of CRI-O.

Suggested change

if opts.RuntimeEnableCDI {

opts.RuntimeEnableCDI = false

}

elezar · 2025-01-16T14:01:41Z

pkg/config/engine/docker/docker.go

@@ -103,6 +103,11 @@ func (c Config) DefaultRuntime() string {
 	return r
 }

+// EnableCDI sets features.cdi to true in the docker config.
+func (c *Config) EnableCDI() {
+	c.Set("features", map[string]bool{"cdi": true})


Out of scope of this PR: I've just thought about the fact that we would overrwrite existing features here.

This change adds an EnableCDI method to the container engine config files and Updates the 'nvidia-ctk runtime configure' command to use this new method. Signed-off-by: Christopher Desiniotis <[email protected]>

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

Signed-off-by: Christopher Desiniotis <[email protected]>

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

Changes applied.

elezar · 2025-01-27T15:02:11Z

@klueska as discussed today, I have rebased this PR.

cdesiniotis force-pushed the enable-cdi-toolkit-container branch 2 times, most recently from 182d161 to 0aaafb4 Compare December 18, 2024 20:28

cdesiniotis requested review from elezar, tariq1890, klueska and guptaNswati December 18, 2024 20:31

tariq1890 reviewed Dec 19, 2024

View reviewed changes

elezar reviewed Dec 20, 2024

View reviewed changes

elezar previously requested changes Dec 20, 2024

View reviewed changes

elezar force-pushed the enable-cdi-toolkit-container branch from 0aaafb4 to d4b8bc5 Compare January 16, 2025 13:58

elezar reviewed Jan 16, 2025

View reviewed changes

elezar force-pushed the enable-cdi-toolkit-container branch from d4b8bc5 to edeeaa4 Compare January 16, 2025 14:39

elezar force-pushed the enable-cdi-toolkit-container branch from edeeaa4 to 67d6718 Compare January 27, 2025 12:29

cdesiniotis and others added 4 commits January 27, 2025 15:51

Add EnableCDI() method to engine.Interface

df73db7

This change adds an EnableCDI method to the container engine config files and Updates the 'nvidia-ctk runtime configure' command to use this new method. Signed-off-by: Christopher Desiniotis <[email protected]>

Remove Set from engine config API

f625242

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

Add option in toolkit container to enable CDI in runtime

e89be14

Signed-off-by: Christopher Desiniotis <[email protected]>

Fix overwriting docker feature flags

2b417c1

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

elezar force-pushed the enable-cdi-toolkit-container branch from 67d6718 to 2b417c1 Compare January 27, 2025 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option in toolkit container to enable CDI in runtime #838

Add option in toolkit container to enable CDI in runtime #838

cdesiniotis commented Dec 18, 2024 •

edited by elezar

Loading

tariq1890 Dec 19, 2024

elezar Dec 20, 2024

cdesiniotis Dec 20, 2024

elezar Jan 16, 2025

tariq1890 Dec 19, 2024

elezar Dec 20, 2024

elezar Dec 20, 2024

elezar Dec 20, 2024

elezar left a comment

elezar Jan 16, 2025

elezar Jan 16, 2025

elezar Jan 16, 2025

elezar commented Jan 27, 2025

	&cli.BoolFlag{
	Name: "cdi-enabled",
	Aliases: []string{"enable-cdi"},
	Usage: "enable the generation of a CDI specification",
	Destination: &opts.cdiEnabled,
	EnvVars: []string{"CDI_ENABLED", "ENABLE_CDI"},
	},

	Usage: "Enable CDI in the configured runtime",
	Usage: "Enable Container Device Interface (CDI) in the configured runtime",

	if o.RuntimeEnableCDI {
	cfg.Set("features", map[string]bool{"cdi": true})
	}

Add option in toolkit container to enable CDI in runtime #838

Are you sure you want to change the base?

Add option in toolkit container to enable CDI in runtime #838

Conversation

cdesiniotis commented Dec 18, 2024 • edited by elezar 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

elezar left a 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 Jan 27, 2025

cdesiniotis commented Dec 18, 2024 •

edited by elezar

Loading