From 4dcaa611679a94dfe55a05e19bd4b128895eac10 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Thu, 10 Aug 2023 10:49:46 +0200 Subject: [PATCH] Use internal/config structs in hook This change ensures that the Config structs from internal.Config are used for the NVIDIA Container Runtime Hook config too. Signed-off-by: Evan Lezar --- CHANGELOG.md | 1 + .../container_config_test.go | 15 +++-- .../hook_config.go | 58 +++---------------- .../hook_config_test.go | 14 ++--- cmd/nvidia-container-runtime-hook/main.go | 37 ++++++------ internal/config/cli.go | 12 +++- internal/config/config.go | 12 ++-- 7 files changed, 53 insertions(+), 96 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 106741b0..83c2edbb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## v1.14.0-rc.3 * Added support for generating OCI hook JSON file to `nvidia-ctk runtime configure` command. * Remove installation of OCI hook JSON from RPM package. +* Refactored config for `nvidia-container-runtime-hook`. ## v1.14.0-rc.2 diff --git a/cmd/nvidia-container-runtime-hook/container_config_test.go b/cmd/nvidia-container-runtime-hook/container_config_test.go index 2131c768..f4b6c217 100644 --- a/cmd/nvidia-container-runtime-hook/container_config_test.go +++ b/cmd/nvidia-container-runtime-hook/container_config_test.go @@ -5,6 +5,7 @@ import ( "path/filepath" "testing" + "github.com/NVIDIA/nvidia-container-toolkit/internal/config" "github.com/NVIDIA/nvidia-container-toolkit/internal/config/image" "github.com/stretchr/testify/require" ) @@ -438,10 +439,9 @@ func TestGetNvidiaConfig(t *testing.T) { }, privileged: true, hookConfig: &HookConfig{ - SwarmResource: func() *string { - s := "DOCKER_SWARM_RESOURCE" - return &s - }(), + Config: config.Config{ + SwarmResource: "DOCKER_SWARM_RESOURCE", + }, SupportedDriverCapabilities: "video,display,utility,compute", }, expectedConfig: &nvidiaConfig{ @@ -457,10 +457,9 @@ func TestGetNvidiaConfig(t *testing.T) { }, privileged: true, hookConfig: &HookConfig{ - SwarmResource: func() *string { - s := "NOT_DOCKER_SWARM_RESOURCE,DOCKER_SWARM_RESOURCE" - return &s - }(), + Config: config.Config{ + SwarmResource: "NOT_DOCKER_SWARM_RESOURCE,DOCKER_SWARM_RESOURCE", + }, SupportedDriverCapabilities: "video,display,utility,compute", }, expectedConfig: &nvidiaConfig{ diff --git a/cmd/nvidia-container-runtime-hook/hook_config.go b/cmd/nvidia-container-runtime-hook/hook_config.go index cc66d3a4..c3e06d0b 100644 --- a/cmd/nvidia-container-runtime-hook/hook_config.go +++ b/cmd/nvidia-container-runtime-hook/hook_config.go @@ -22,64 +22,22 @@ var defaultPaths = [...]string{ configPath, } -// CLIConfig : options for nvidia-container-cli. -type CLIConfig struct { - Root *string `toml:"root"` - Path *string `toml:"path"` - Environment []string `toml:"environment"` - Debug *string `toml:"debug"` - Ldcache *string `toml:"ldcache"` - LoadKmods bool `toml:"load-kmods"` - NoPivot bool `toml:"no-pivot"` - NoCgroups bool `toml:"no-cgroups"` - User *string `toml:"user"` - Ldconfig *string `toml:"ldconfig"` -} - // HookConfig : options for the nvidia-container-runtime-hook. type HookConfig struct { - DisableRequire bool `toml:"disable-require"` - SwarmResource *string `toml:"swarm-resource"` - AcceptEnvvarUnprivileged bool `toml:"accept-nvidia-visible-devices-envvar-when-unprivileged"` - AcceptDeviceListAsVolumeMounts bool `toml:"accept-nvidia-visible-devices-as-volume-mounts"` - SupportedDriverCapabilities DriverCapabilities `toml:"supported-driver-capabilities"` - - NvidiaContainerCLI CLIConfig `toml:"nvidia-container-cli"` - NVIDIAContainerRuntime config.RuntimeConfig `toml:"nvidia-container-runtime"` - NVIDIAContainerRuntimeHook config.RuntimeHookConfig `toml:"nvidia-container-runtime-hook"` + config.Config + // TODO: We should also migrate the driver capabilities + SupportedDriverCapabilities DriverCapabilities `toml:"supported-driver-capabilities"` } func getDefaultHookConfig() (HookConfig, error) { - rtConfig, err := config.GetDefaultRuntimeConfig() - if err != nil { - return HookConfig{}, err - } - - rtHookConfig, err := config.GetDefaultRuntimeHookConfig() + defaultCfg, err := config.GetDefault() if err != nil { return HookConfig{}, err } c := HookConfig{ - DisableRequire: false, - SwarmResource: nil, - AcceptEnvvarUnprivileged: true, - AcceptDeviceListAsVolumeMounts: false, - SupportedDriverCapabilities: allDriverCapabilities, - NvidiaContainerCLI: CLIConfig{ - Root: nil, - Path: nil, - Environment: []string{}, - Debug: nil, - Ldcache: nil, - LoadKmods: true, - NoPivot: false, - NoCgroups: false, - User: nil, - Ldconfig: nil, - }, - NVIDIAContainerRuntime: *rtConfig, - NVIDIAContainerRuntimeHook: *rtHookConfig, + Config: *defaultCfg, + SupportedDriverCapabilities: allDriverCapabilities, } return c, nil @@ -142,11 +100,11 @@ func (c HookConfig) getConfigOption(fieldName string) string { // getSwarmResourceEnvvars returns the swarm resource envvars for the config. func (c *HookConfig) getSwarmResourceEnvvars() []string { - if c.SwarmResource == nil { + if c.SwarmResource == "" { return nil } - candidates := strings.Split(*c.SwarmResource, ",") + candidates := strings.Split(c.SwarmResource, ",") var envvars []string for _, c := range candidates { diff --git a/cmd/nvidia-container-runtime-hook/hook_config_test.go b/cmd/nvidia-container-runtime-hook/hook_config_test.go index 7cddafab..dddd1cdc 100644 --- a/cmd/nvidia-container-runtime-hook/hook_config_test.go +++ b/cmd/nvidia-container-runtime-hook/hook_config_test.go @@ -21,6 +21,7 @@ import ( "os" "testing" + "github.com/NVIDIA/nvidia-container-toolkit/internal/config" "github.com/stretchr/testify/require" ) @@ -110,10 +111,6 @@ func TestGetSwarmResourceEnvvars(t *testing.T) { value string expected []string }{ - { - value: "nil", - expected: nil, - }, { value: "", expected: nil, @@ -147,12 +144,9 @@ func TestGetSwarmResourceEnvvars(t *testing.T) { for i, tc := range testCases { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { c := &HookConfig{ - SwarmResource: func() *string { - if tc.value == "nil" { - return nil - } - return &tc.value - }(), + Config: config.Config{ + SwarmResource: tc.value, + }, } envvars := c.getSwarmResourceEnvvars() diff --git a/cmd/nvidia-container-runtime-hook/main.go b/cmd/nvidia-container-runtime-hook/main.go index b0d88ee1..30aad846 100644 --- a/cmd/nvidia-container-runtime-hook/main.go +++ b/cmd/nvidia-container-runtime-hook/main.go @@ -13,6 +13,7 @@ import ( "strings" "syscall" + "github.com/NVIDIA/nvidia-container-toolkit/internal/config" "github.com/NVIDIA/nvidia-container-toolkit/internal/info" "github.com/NVIDIA/nvidia-container-toolkit/internal/logger" "github.com/NVIDIA/nvidia-container-toolkit/internal/lookup" @@ -37,16 +38,12 @@ func exit() { os.Exit(0) } -func getCLIPath(config CLIConfig) string { - if config.Path != nil { - return *config.Path +func getCLIPath(config config.ContainerCLIConfig) string { + if config.Path != "" { + return config.Path } - var root string - if config.Root != nil { - root = *config.Root - } - if err := os.Setenv("PATH", lookup.GetPath(root)); err != nil { + if err := os.Setenv("PATH", lookup.GetPath(config.Root)); err != nil { log.Panicln("couldn't set PATH variable:", err) } @@ -76,7 +73,7 @@ func doPrestart() { if err != nil || hook == nil { log.Panicln("error getting hook config:", err) } - cli := hook.NvidiaContainerCLI + cli := hook.NVIDIAContainerCLIConfig container := getContainerConfig(*hook) nvidia := container.Nvidia @@ -85,15 +82,15 @@ func doPrestart() { return } - if !hook.NVIDIAContainerRuntimeHook.SkipModeDetection && info.ResolveAutoMode(&logInterceptor{}, hook.NVIDIAContainerRuntime.Mode, container.Image) != "legacy" { + if !hook.NVIDIAContainerRuntimeHookConfig.SkipModeDetection && info.ResolveAutoMode(&logInterceptor{}, hook.NVIDIAContainerRuntimeConfig.Mode, container.Image) != "legacy" { log.Panicln("invoking the NVIDIA Container Runtime Hook directly (e.g. specifying the docker --gpus flag) is not supported. Please use the NVIDIA Container Runtime (e.g. specify the --runtime=nvidia flag) instead.") } rootfs := getRootfsPath(container) args := []string{getCLIPath(cli)} - if cli.Root != nil && *cli.Root != "" { - args = append(args, fmt.Sprintf("--root=%s", *cli.Root)) + if cli.Root != "" { + args = append(args, fmt.Sprintf("--root=%s", cli.Root)) } if cli.LoadKmods { args = append(args, "--load-kmods") @@ -103,19 +100,19 @@ func doPrestart() { } if *debugflag { args = append(args, "--debug=/dev/stderr") - } else if cli.Debug != nil { - args = append(args, fmt.Sprintf("--debug=%s", *cli.Debug)) + } else if cli.Debug != "" { + args = append(args, fmt.Sprintf("--debug=%s", cli.Debug)) } - if cli.Ldcache != nil { - args = append(args, fmt.Sprintf("--ldcache=%s", *cli.Ldcache)) + if cli.Ldcache != "" { + args = append(args, fmt.Sprintf("--ldcache=%s", cli.Ldcache)) } - if cli.User != nil { - args = append(args, fmt.Sprintf("--user=%s", *cli.User)) + if cli.User != "" { + args = append(args, fmt.Sprintf("--user=%s", cli.User)) } args = append(args, "configure") - if cli.Ldconfig != nil { - args = append(args, fmt.Sprintf("--ldconfig=%s", *cli.Ldconfig)) + if cli.Ldconfig != "" { + args = append(args, fmt.Sprintf("--ldconfig=%s", cli.Ldconfig)) } if cli.NoCgroups { args = append(args, "--no-cgroups") diff --git a/internal/config/cli.go b/internal/config/cli.go index cb8d615c..f2a55a7a 100644 --- a/internal/config/cli.go +++ b/internal/config/cli.go @@ -19,7 +19,15 @@ package config // ContainerCLIConfig stores the options for the nvidia-container-cli type ContainerCLIConfig struct { Root string `toml:"root"` - LoadKmods bool `toml:"load-kmods"` - Ldconfig string `toml:"ldconfig"` + Path string `toml:"path"` Environment []string `toml:"environment"` + Debug string `toml:"debug"` + Ldcache string `toml:"ldcache"` + LoadKmods bool `toml:"load-kmods"` + // NoPivot disables the pivot root operation in the NVIDIA Container CLI. + // This is not exposed in the config if not set. + NoPivot bool `toml:"no-pivot,omitempty"` + NoCgroups bool `toml:"no-cgroups"` + User string `toml:"user"` + Ldconfig string `toml:"ldconfig"` } diff --git a/internal/config/config.go b/internal/config/config.go index c804f2ac..660c505c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -57,8 +57,11 @@ var ( // Config represents the contents of the config.toml file for the NVIDIA Container Toolkit // Note: This is currently duplicated by the HookConfig in cmd/nvidia-container-toolkit/hook_config.go type Config struct { - DisableRequire bool `toml:"disable-require"` - AcceptEnvvarUnprivileged bool `toml:"accept-nvidia-visible-devices-envvar-when-unprivileged"` + DisableRequire bool `toml:"disable-require"` + SwarmResource string `toml:"swarm-resource"` + AcceptEnvvarUnprivileged bool `toml:"accept-nvidia-visible-devices-envvar-when-unprivileged"` + AcceptDeviceListAsVolumeMounts bool `toml:"accept-nvidia-visible-devices-as-volume-mounts"` + // SupportedDriverCapabilities DriverCapabilities `toml:"supported-driver-capabilities"` NVIDIAContainerCLIConfig ContainerCLIConfig `toml:"nvidia-container-cli"` NVIDIACTKConfig CTKConfig `toml:"nvidia-ctk"` @@ -290,16 +293,13 @@ func (c Config) asCommentedToml() (*toml.Tree, error) { } func shouldComment(key string, defaultValue interface{}, setTo interface{}) bool { - if key == "nvidia-container-cli.root" && setTo == "" { - return true - } if key == "nvidia-container-cli.user" && !getCommentedUserGroup() { return false } if key == "nvidia-container-runtime.debug" && setTo == "/dev/null" { return true } - if setTo == nil || defaultValue == setTo { + if setTo == nil || defaultValue == setTo || setTo == "" { return true }