From 1081cecea97d60a9c4e715c3a9ecaa5323d7cdec Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Fri, 7 Jul 2023 12:34:43 +0200 Subject: [PATCH] Return empty requirements if NVIDIA_DISABLE_REQUIRE is true Signed-off-by: Evan Lezar --- CHANGELOG.md | 1 + .../container_config.go | 14 ++-- .../container_config_test.go | 25 +------ cmd/nvidia-container-runtime-hook/main.go | 6 +- internal/config/image/builder.go | 73 +++++++++++++++++++ internal/config/image/cuda_image.go | 27 ++----- internal/config/image/cuda_image_test.go | 10 +++ 7 files changed, 104 insertions(+), 52 deletions(-) create mode 100644 internal/config/image/builder.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 751986b6..19a564f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ * Create ouput folders if required when running `nvidia-ctk runtime configure` * Generate default config as post-install step. * Added support for detecting GSP firmware at custom paths when generating CDI specifications. +* Added logic to skip the extraction of image requirements if NVIDIA_DISABLE_REQUIRES is set to true. * [libnvidia-container] Support OpenSSL 3 with the Encrypt/Decrypt library diff --git a/cmd/nvidia-container-runtime-hook/container_config.go b/cmd/nvidia-container-runtime-hook/container_config.go index 36278dc0..2bf371aa 100644 --- a/cmd/nvidia-container-runtime-hook/container_config.go +++ b/cmd/nvidia-container-runtime-hook/container_config.go @@ -38,8 +38,10 @@ type nvidiaConfig struct { MigConfigDevices string MigMonitorDevices string DriverCapabilities string - Requirements []string - DisableRequire bool + // Requirements defines the requirements DSL for the container to run. + // This is empty if no specific requirements are needed, or if requirements are + // explicitly disabled. + Requirements []string } type containerConfig struct { @@ -327,15 +329,12 @@ func getNvidiaConfig(hookConfig *HookConfig, image image.CUDA, mounts []Mount, p log.Panicln("failed to get requirements", err) } - disableRequire := image.HasDisableRequire() - return &nvidiaConfig{ Devices: devices, MigConfigDevices: migConfigDevices, MigMonitorDevices: migMonitorDevices, DriverCapabilities: driverCapabilities, Requirements: requirements, - DisableRequire: disableRequire, } } @@ -353,7 +352,10 @@ func getContainerConfig(hook HookConfig) (config containerConfig) { s := loadSpec(path.Join(b, "config.json")) - image, err := image.NewCUDAImageFromEnv(s.Process.Env) + image, err := image.New( + image.WithEnv(s.Process.Env), + image.WithDisableRequire(hook.DisableRequire), + ) if err != nil { log.Panicln(err) } diff --git a/cmd/nvidia-container-runtime-hook/container_config_test.go b/cmd/nvidia-container-runtime-hook/container_config_test.go index a52adebc..2131c768 100644 --- a/cmd/nvidia-container-runtime-hook/container_config_test.go +++ b/cmd/nvidia-container-runtime-hook/container_config_test.go @@ -40,7 +40,6 @@ func TestGetNvidiaConfig(t *testing.T) { Devices: "all", DriverCapabilities: allDriverCapabilities.String(), Requirements: []string{"cuda>=9.0"}, - DisableRequire: false, }, }, { @@ -54,7 +53,6 @@ func TestGetNvidiaConfig(t *testing.T) { Devices: "all", DriverCapabilities: allDriverCapabilities.String(), Requirements: []string{"cuda>=9.0"}, - DisableRequire: false, }, }, { @@ -86,7 +84,6 @@ func TestGetNvidiaConfig(t *testing.T) { Devices: "", DriverCapabilities: allDriverCapabilities.String(), Requirements: []string{"cuda>=9.0"}, - DisableRequire: false, }, }, { @@ -100,7 +97,6 @@ func TestGetNvidiaConfig(t *testing.T) { Devices: "gpu0,gpu1", DriverCapabilities: allDriverCapabilities.String(), Requirements: []string{"cuda>=9.0"}, - DisableRequire: false, }, }, { @@ -115,7 +111,6 @@ func TestGetNvidiaConfig(t *testing.T) { Devices: "gpu0,gpu1", DriverCapabilities: defaultDriverCapabilities.String(), Requirements: []string{"cuda>=9.0"}, - DisableRequire: false, }, }, { @@ -130,7 +125,6 @@ func TestGetNvidiaConfig(t *testing.T) { Devices: "gpu0,gpu1", DriverCapabilities: allDriverCapabilities.String(), Requirements: []string{"cuda>=9.0"}, - DisableRequire: false, }, }, { @@ -145,7 +139,6 @@ func TestGetNvidiaConfig(t *testing.T) { Devices: "gpu0,gpu1", DriverCapabilities: "video,display", Requirements: []string{"cuda>=9.0"}, - DisableRequire: false, }, }, { @@ -162,7 +155,6 @@ func TestGetNvidiaConfig(t *testing.T) { Devices: "gpu0,gpu1", DriverCapabilities: "video,display", Requirements: []string{"cuda>=9.0", "req0=true", "req1=false"}, - DisableRequire: false, }, }, { @@ -179,8 +171,7 @@ func TestGetNvidiaConfig(t *testing.T) { expectedConfig: &nvidiaConfig{ Devices: "gpu0,gpu1", DriverCapabilities: "video,display", - Requirements: []string{"cuda>=9.0", "req0=true", "req1=false"}, - DisableRequire: true, + Requirements: []string{}, }, }, { @@ -211,7 +202,6 @@ func TestGetNvidiaConfig(t *testing.T) { Devices: "all", DriverCapabilities: defaultDriverCapabilities.String(), Requirements: []string{"cuda>=9.0"}, - DisableRequire: false, }, }, { @@ -243,7 +233,6 @@ func TestGetNvidiaConfig(t *testing.T) { Devices: "", DriverCapabilities: defaultDriverCapabilities.String(), Requirements: []string{"cuda>=9.0"}, - DisableRequire: false, }, }, { @@ -257,7 +246,6 @@ func TestGetNvidiaConfig(t *testing.T) { Devices: "gpu0,gpu1", DriverCapabilities: defaultDriverCapabilities.String(), Requirements: []string{"cuda>=9.0"}, - DisableRequire: false, }, }, { @@ -272,7 +260,6 @@ func TestGetNvidiaConfig(t *testing.T) { Devices: "gpu0,gpu1", DriverCapabilities: defaultDriverCapabilities.String(), Requirements: []string{"cuda>=9.0"}, - DisableRequire: false, }, }, { @@ -287,7 +274,6 @@ func TestGetNvidiaConfig(t *testing.T) { Devices: "gpu0,gpu1", DriverCapabilities: allDriverCapabilities.String(), Requirements: []string{"cuda>=9.0"}, - DisableRequire: false, }, }, { @@ -302,7 +288,6 @@ func TestGetNvidiaConfig(t *testing.T) { Devices: "gpu0,gpu1", DriverCapabilities: "video,display", Requirements: []string{"cuda>=9.0"}, - DisableRequire: false, }, }, { @@ -319,7 +304,6 @@ func TestGetNvidiaConfig(t *testing.T) { Devices: "gpu0,gpu1", DriverCapabilities: "video,display", Requirements: []string{"cuda>=9.0", "req0=true", "req1=false"}, - DisableRequire: false, }, }, { @@ -336,8 +320,7 @@ func TestGetNvidiaConfig(t *testing.T) { expectedConfig: &nvidiaConfig{ Devices: "gpu0,gpu1", DriverCapabilities: "video,display", - Requirements: []string{"cuda>=9.0", "req0=true", "req1=false"}, - DisableRequire: true, + Requirements: []string{}, }, }, { @@ -351,7 +334,6 @@ func TestGetNvidiaConfig(t *testing.T) { Devices: "all", DriverCapabilities: defaultDriverCapabilities.String(), Requirements: []string{}, - DisableRequire: false, }, }, { @@ -367,7 +349,6 @@ func TestGetNvidiaConfig(t *testing.T) { MigConfigDevices: "mig0,mig1", DriverCapabilities: defaultDriverCapabilities.String(), Requirements: []string{"cuda>=9.0"}, - DisableRequire: false, }, }, { @@ -393,7 +374,6 @@ func TestGetNvidiaConfig(t *testing.T) { MigMonitorDevices: "mig0,mig1", DriverCapabilities: defaultDriverCapabilities.String(), Requirements: []string{"cuda>=9.0"}, - DisableRequire: false, }, }, { @@ -525,7 +505,6 @@ func TestGetNvidiaConfig(t *testing.T) { require.Equal(t, tc.expectedConfig.DriverCapabilities, config.DriverCapabilities) require.ElementsMatch(t, tc.expectedConfig.Requirements, config.Requirements) - require.Equal(t, tc.expectedConfig.DisableRequire, config.DisableRequire) }) } } diff --git a/cmd/nvidia-container-runtime-hook/main.go b/cmd/nvidia-container-runtime-hook/main.go index 99583aa5..2db60247 100644 --- a/cmd/nvidia-container-runtime-hook/main.go +++ b/cmd/nvidia-container-runtime-hook/main.go @@ -137,10 +137,8 @@ func doPrestart() { args = append(args, capabilityToCLI(cap)) } - if !hook.DisableRequire && !nvidia.DisableRequire { - for _, req := range nvidia.Requirements { - args = append(args, fmt.Sprintf("--require=%s", req)) - } + for _, req := range nvidia.Requirements { + args = append(args, fmt.Sprintf("--require=%s", req)) } args = append(args, fmt.Sprintf("--pid=%s", strconv.FormatUint(uint64(container.Pid), 10))) diff --git a/internal/config/image/builder.go b/internal/config/image/builder.go new file mode 100644 index 00000000..b11f6c74 --- /dev/null +++ b/internal/config/image/builder.go @@ -0,0 +1,73 @@ +/** +# Copyright (c) NVIDIA CORPORATION. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +**/ + +package image + +import ( + "fmt" + "strings" +) + +type builder struct { + env []string + disableRequire bool +} + +// New creates a new CUDA image from the input options. +func New(opt ...Option) (CUDA, error) { + b := &builder{} + for _, o := range opt { + o(b) + } + + return b.build() +} + +// build creates a CUDA image from the builder. +func (b builder) build() (CUDA, error) { + c := make(CUDA) + + for _, e := range b.env { + parts := strings.SplitN(e, "=", 2) + if len(parts) != 2 { + return nil, fmt.Errorf("invalid environment variable: %v", e) + } + c[parts[0]] = parts[1] + } + + if b.disableRequire { + c[envNVDisableRequire] = "true" + } + + return c, nil +} + +// Option is a functional option for creating a CUDA image. +type Option func(*builder) + +// WithDisableRequire sets the disable require option. +func WithDisableRequire(disableRequire bool) Option { + return func(b *builder) { + b.disableRequire = disableRequire + } +} + +// WithEnv sets the environment variables to use when creating the CUDA image. +func WithEnv(env []string) Option { + return func(b *builder) { + b.env = env + } +} diff --git a/internal/config/image/cuda_image.go b/internal/config/image/cuda_image.go index 4a255cb5..1fe51c54 100644 --- a/internal/config/image/cuda_image.go +++ b/internal/config/image/cuda_image.go @@ -42,27 +42,18 @@ type CUDA map[string]string // NewCUDAImageFromSpec creates a CUDA image from the input OCI runtime spec. // The process environment is read (if present) to construc the CUDA Image. func NewCUDAImageFromSpec(spec *specs.Spec) (CUDA, error) { - if spec == nil || spec.Process == nil { - return NewCUDAImageFromEnv(nil) + var env []string + if spec != nil && spec.Process != nil { + env = spec.Process.Env } - return NewCUDAImageFromEnv(spec.Process.Env) + return New(WithEnv(env)) } // NewCUDAImageFromEnv creates a CUDA image from the input environment. The environment // is a list of strings of the form ENVAR=VALUE. func NewCUDAImageFromEnv(env []string) (CUDA, error) { - c := make(CUDA) - - for _, e := range env { - parts := strings.SplitN(e, "=", 2) - if len(parts) != 2 { - return nil, fmt.Errorf("invalid environment variable: %v", e) - } - c[parts[0]] = parts[1] - } - - return c, nil + return New(WithEnv(env)) } // IsLegacy returns whether the associated CUDA image is a "legacy" image. An @@ -77,11 +68,9 @@ func (i CUDA) IsLegacy() bool { // GetRequirements returns the requirements from all NVIDIA_REQUIRE_ environment // variables. func (i CUDA) GetRequirements() ([]string, error) { - // TODO: We need not process this if disable require is set, but this will be done - // in a single follow-up to ensure that the behavioural change is accurately captured. - // if i.HasDisableRequire() { - // return nil, nil - // } + if i.HasDisableRequire() { + return nil, nil + } // All variables with the "NVIDIA_REQUIRE_" prefix are passed to nvidia-container-cli var requirements []string diff --git a/internal/config/image/cuda_image_test.go b/internal/config/image/cuda_image_test.go index a330b70c..371a0ccc 100644 --- a/internal/config/image/cuda_image_test.go +++ b/internal/config/image/cuda_image_test.go @@ -106,6 +106,16 @@ func TestGetRequirements(t *testing.T) { env: []string{"CUDA_VERSION=11.6", "NVIDIA_REQUIRE_BRAND=brand=tesla"}, requirements: []string{"cuda>=11.6", "brand=tesla"}, }, + { + description: "NVIDIA_DISABLE_REQUIRE ignores requirements", + env: []string{"NVIDIA_REQUIRE_CUDA=cuda>=11.6", "NVIDIA_REQUIRE_BRAND=brand=tesla", "NVIDIA_DISABLE_REQUIRE=true"}, + requirements: []string{}, + }, + { + description: "NVIDIA_DISABLE_REQUIRE ignores legacy image requirements", + env: []string{"CUDA_VERSION=11.6", "NVIDIA_REQUIRE_BRAND=brand=tesla", "NVIDIA_DISABLE_REQUIRE=true"}, + requirements: []string{}, + }, } for _, tc := range testCases {