Add option in toolkit container to enable CDI in runtime #838
Conversation
cdesiniotis
force-pushed
the
enable-cdi-toolkit-container
branch
from
ee99755 to
182d161
Compare
Signed-off-by: Christopher Desiniotis <[email protected]>
…rom engine.Interface Signed-off-by: Christopher Desiniotis <[email protected]>
Signed-off-by: Christopher Desiniotis <[email protected]>
cdesiniotis
force-pushed
the
enable-cdi-toolkit-container
branch
from
182d161 to
0aaafb4
Compare
cdesiniotis
requested review from
elezar,
tariq1890,
klueska and
guptaNswati
| @@ -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", | |||
There was a problem hiding this comment.
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
| Name: "runtime-enable-cdi", | |
| Name: "enable-cdi", |
There was a problem hiding this comment.
Let's try to be consistent with the option for the nvidia-ctk runtime configure command.
There was a problem hiding this comment.
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
Any ideas on what the name of the new flag should be?
| @@ -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", | |||
There was a problem hiding this comment.
| Usage: "Enable CDI in the configured runtime", | |
| Usage: "Enable Container Device Interface (CDI) in the configured runtime", |
| @@ -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) | |||
There was a problem hiding this comment.
I think we should be able to cast c to a Config type and call EnableCDI here instead of reimplementing it.
| @@ -24,6 +24,7 @@ type Interface interface { | |||
| RemoveRuntime(string) error | |||
| Save(string) (int64, error) | |||
| GetRuntimeConfig(string) (RuntimeConfig, error) | |||
| EnableCDI() | |||
There was a problem hiding this comment.
I think Set is only used to enable CDI. Let's remove that from the interface if that's the case.
| SetAsDefault bool | ||
| RestartMode string | ||
| HostRootMount string | ||
| RuntimeEnableCDI bool |
There was a problem hiding this comment.
I don't agree with the Runtime* prefix here. Runtime in this context refers to the NVIDIA runtime. I would just use EnableCDI here.
No description provided.