From 6c5f4eea632b9ebacbec062228d41fc0e3dc09c4 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Fri, 27 Sep 2024 13:19:23 +0200 Subject: [PATCH] Remove support for config overrides Signed-off-by: Evan Lezar --- cmd/nvidia-ctk/runtime/configure/configure.go | 28 -------- pkg/config/engine/api.go | 2 +- pkg/config/engine/containerd/config_v1.go | 12 +--- .../engine/containerd/config_v1_test.go | 39 ++-------- pkg/config/engine/containerd/config_v2.go | 7 +- .../engine/containerd/config_v2_test.go | 38 ++-------- pkg/config/engine/containerd/toml.go | 56 --------------- pkg/config/engine/crio/crio.go | 2 +- pkg/config/engine/crio/crio_test.go | 13 ++-- pkg/config/engine/docker/docker.go | 2 +- tools/container/container.go | 8 +-- tools/container/containerd/containerd.go | 20 +----- tools/container/containerd/containerd_test.go | 72 ------------------- 13 files changed, 28 insertions(+), 271 deletions(-) delete mode 100644 pkg/config/engine/containerd/toml.go delete mode 100644 tools/container/containerd/containerd_test.go diff --git a/cmd/nvidia-ctk/runtime/configure/configure.go b/cmd/nvidia-ctk/runtime/configure/configure.go index 6dc95913..e783a4e2 100644 --- a/cmd/nvidia-ctk/runtime/configure/configure.go +++ b/cmd/nvidia-ctk/runtime/configure/configure.go @@ -17,7 +17,6 @@ package configure import ( - "encoding/json" "fmt" "path/filepath" @@ -156,13 +155,6 @@ func (m command) build() *cli.Command { Usage: "Enable CDI in the configured runtime", Destination: &config.cdi.enabled, }, - &cli.StringFlag{ - Name: "runtime-config-override", - Destination: &config.runtimeConfigOverrideJSON, - Usage: "specify additional runtime options as a JSON string. The paths are relative to the runtime config.", - Value: "", - EnvVars: []string{"RUNTIME_CONFIG_OVERRIDE"}, - }, } return &configure @@ -252,16 +244,10 @@ func (m command) configureConfigFile(c *cli.Context, config *config) error { return fmt.Errorf("unable to load config for runtime %v: %v", config.runtime, err) } - runtimeConfigOverride, err := config.runtimeConfigOverride() - if err != nil { - return fmt.Errorf("unable to parse config overrides: %w", err) - } - err = cfg.AddRuntime( config.nvidiaRuntime.name, config.nvidiaRuntime.path, config.nvidiaRuntime.setAsDefault, - runtimeConfigOverride, ) if err != nil { return fmt.Errorf("unable to update config: %v", err) @@ -314,20 +300,6 @@ func (c *config) getOuputConfigPath() string { return c.resolveConfigFilePath() } -// runtimeConfigOverride converts the specified runtimeConfigOverride JSON string to a map. -func (o *config) runtimeConfigOverride() (map[string]interface{}, error) { - if o.runtimeConfigOverrideJSON == "" { - return nil, nil - } - - runtimeOptions := make(map[string]interface{}) - if err := json.Unmarshal([]byte(o.runtimeConfigOverrideJSON), &runtimeOptions); err != nil { - return nil, fmt.Errorf("failed to read %v as JSON: %w", o.runtimeConfigOverrideJSON, err) - } - - return runtimeOptions, nil -} - // configureOCIHook creates and configures the OCI hook for the NVIDIA runtime func (m *command) configureOCIHook(c *cli.Context, config *config) error { err := ocihook.CreateHook(config.hookFilePath, config.nvidiaRuntime.hookPath) diff --git a/pkg/config/engine/api.go b/pkg/config/engine/api.go index 78b1549c..b074dadf 100644 --- a/pkg/config/engine/api.go +++ b/pkg/config/engine/api.go @@ -19,7 +19,7 @@ package engine // Interface defines the API for a runtime config updater. type Interface interface { DefaultRuntime() string - AddRuntime(string, string, bool, ...map[string]interface{}) error + AddRuntime(string, string, bool) error Set(string, interface{}) RemoveRuntime(string) error Save(string) (int64, error) diff --git a/pkg/config/engine/containerd/config_v1.go b/pkg/config/engine/containerd/config_v1.go index 5522a227..b6a06a01 100644 --- a/pkg/config/engine/containerd/config_v1.go +++ b/pkg/config/engine/containerd/config_v1.go @@ -30,7 +30,7 @@ type ConfigV1 Config var _ engine.Interface = (*ConfigV1)(nil) // AddRuntime adds a runtime to the containerd config -func (c *ConfigV1) AddRuntime(name string, path string, setAsDefault bool, configOverrides ...map[string]interface{}) error { +func (c *ConfigV1) AddRuntime(name string, path string, setAsDefault bool) error { if c == nil || c.Tree == nil { return fmt.Errorf("config is nil") } @@ -85,16 +85,6 @@ func (c *ConfigV1) AddRuntime(name string, path string, setAsDefault bool, confi } config.SetPath([]string{"plugins", "cri", "containerd", "default_runtime", "options", "BinaryName"}, path) config.SetPath([]string{"plugins", "cri", "containerd", "default_runtime", "options", "Runtime"}, path) - - defaultRuntimeSubtree := subtreeAtPath(config, "plugins", "cri", "containerd", "default_runtime") - if err := defaultRuntimeSubtree.applyOverrides(configOverrides...); err != nil { - return fmt.Errorf("failed to apply config overrides to default_runtime: %w", err) - } - } - - runtimeSubtree := subtreeAtPath(config, "plugins", "cri", "containerd", "runtimes", name) - if err := runtimeSubtree.applyOverrides(configOverrides...); err != nil { - return fmt.Errorf("failed to apply config overrides: %w", err) } *c.Tree = config diff --git a/pkg/config/engine/containerd/config_v1_test.go b/pkg/config/engine/containerd/config_v1_test.go index 774500d5..62c3be9c 100644 --- a/pkg/config/engine/containerd/config_v1_test.go +++ b/pkg/config/engine/containerd/config_v1_test.go @@ -27,12 +27,11 @@ import ( func TestAddRuntimeV1(t *testing.T) { logger, _ := testlog.NewNullLogger() testCases := []struct { - description string - config string - setAsDefault bool - configOverrides []map[string]interface{} - expectedConfig string - expectedError error + description string + config string + setAsDefault bool + expectedConfig string + expectedError error }{ { description: "empty config not default runtime", @@ -53,32 +52,6 @@ func TestAddRuntimeV1(t *testing.T) { `, expectedError: nil, }, - { - description: "empty config not default runtime with overrides", - configOverrides: []map[string]interface{}{ - { - "options": map[string]interface{}{ - "SystemdCgroup": true, - }, - }, - }, - expectedConfig: ` - version = 1 - [plugins] - [plugins.cri] - [plugins.cri.containerd] - [plugins.cri.containerd.runtimes] - [plugins.cri.containerd.runtimes.test] - privileged_without_host_devices = false - runtime_engine = "" - runtime_root = "" - runtime_type = "" - [plugins.cri.containerd.runtimes.test.options] - BinaryName = "/usr/bin/test" - Runtime = "/usr/bin/test" - SystemdCgroup = true - `, - }, { description: "options from runc are imported", config: ` @@ -236,7 +209,7 @@ func TestAddRuntimeV1(t *testing.T) { Tree: config, } - err = c.AddRuntime("test", "/usr/bin/test", tc.setAsDefault, tc.configOverrides...) + err = c.AddRuntime("test", "/usr/bin/test", tc.setAsDefault) require.NoError(t, err) require.EqualValues(t, expectedConfig.String(), config.String()) diff --git a/pkg/config/engine/containerd/config_v2.go b/pkg/config/engine/containerd/config_v2.go index ee9d33f7..ef113e96 100644 --- a/pkg/config/engine/containerd/config_v2.go +++ b/pkg/config/engine/containerd/config_v2.go @@ -25,7 +25,7 @@ import ( ) // AddRuntime adds a runtime to the containerd config -func (c *Config) AddRuntime(name string, path string, setAsDefault bool, configOverrides ...map[string]interface{}) error { +func (c *Config) AddRuntime(name string, path string, setAsDefault bool) error { if c == nil || c.Tree == nil { return fmt.Errorf("config is nil") } @@ -70,11 +70,6 @@ func (c *Config) AddRuntime(name string, path string, setAsDefault bool, configO config.SetPath([]string{"plugins", "io.containerd.grpc.v1.cri", "containerd", "default_runtime_name"}, name) } - runtimeSubtree := subtreeAtPath(config, "plugins", "io.containerd.grpc.v1.cri", "containerd", "runtimes", name) - if err := runtimeSubtree.applyOverrides(configOverrides...); err != nil { - return fmt.Errorf("failed to apply config overrides: %w", err) - } - *c.Tree = config return nil } diff --git a/pkg/config/engine/containerd/config_v2_test.go b/pkg/config/engine/containerd/config_v2_test.go index 598222f8..f10da804 100644 --- a/pkg/config/engine/containerd/config_v2_test.go +++ b/pkg/config/engine/containerd/config_v2_test.go @@ -27,12 +27,11 @@ import ( func TestAddRuntime(t *testing.T) { logger, _ := testlog.NewNullLogger() testCases := []struct { - description string - config string - setAsDefault bool - configOverrides []map[string]interface{} - expectedConfig string - expectedError error + description string + config string + setAsDefault bool + expectedConfig string + expectedError error }{ { description: "empty config not default runtime", @@ -52,31 +51,6 @@ func TestAddRuntime(t *testing.T) { `, expectedError: nil, }, - { - description: "empty config not default runtime with overrides", - configOverrides: []map[string]interface{}{ - { - "options": map[string]interface{}{ - "SystemdCgroup": true, - }, - }, - }, - expectedConfig: ` - version = 2 - [plugins] - [plugins."io.containerd.grpc.v1.cri"] - [plugins."io.containerd.grpc.v1.cri".containerd] - [plugins."io.containerd.grpc.v1.cri".containerd.runtimes] - [plugins."io.containerd.grpc.v1.cri".containerd.runtimes.test] - privileged_without_host_devices = false - runtime_engine = "" - runtime_root = "" - runtime_type = "" - [plugins."io.containerd.grpc.v1.cri".containerd.runtimes.test.options] - BinaryName = "/usr/bin/test" - SystemdCgroup = true - `, - }, { description: "options from runc are imported", config: ` @@ -234,7 +208,7 @@ func TestAddRuntime(t *testing.T) { Tree: config, } - err = c.AddRuntime("test", "/usr/bin/test", tc.setAsDefault, tc.configOverrides...) + err = c.AddRuntime("test", "/usr/bin/test", tc.setAsDefault) require.NoError(t, err) require.EqualValues(t, expectedConfig.String(), config.String()) diff --git a/pkg/config/engine/containerd/toml.go b/pkg/config/engine/containerd/toml.go deleted file mode 100644 index 85c20d0e..00000000 --- a/pkg/config/engine/containerd/toml.go +++ /dev/null @@ -1,56 +0,0 @@ -/** -# Copyright 2024 NVIDIA CORPORATION -# -# 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 containerd - -import ( - "fmt" - - "github.com/pelletier/go-toml" -) - -// tomlTree is an alias for toml.Tree that allows for extensions. -type tomlTree toml.Tree - -func subtreeAtPath(c toml.Tree, path ...string) *tomlTree { - tree := c.GetPath(path).(*toml.Tree) - return (*tomlTree)(tree) -} - -func (t *tomlTree) insert(other map[string]interface{}) error { - - for key, value := range other { - if insertsubtree, ok := value.(map[string]interface{}); ok { - subtree := (*toml.Tree)(t).Get(key).(*toml.Tree) - return (*tomlTree)(subtree).insert(insertsubtree) - } - (*toml.Tree)(t).Set(key, value) - } - return nil -} - -func (t *tomlTree) applyOverrides(overrides ...map[string]interface{}) error { - for _, override := range overrides { - subconfig, err := toml.TreeFromMap(override) - if err != nil { - return fmt.Errorf("invalid toml config: %w", err) - } - if err := t.insert(subconfig.ToMap()); err != nil { - return err - } - } - return nil -} diff --git a/pkg/config/engine/crio/crio.go b/pkg/config/engine/crio/crio.go index 47320d15..422d2de6 100644 --- a/pkg/config/engine/crio/crio.go +++ b/pkg/config/engine/crio/crio.go @@ -44,7 +44,7 @@ func New(opts ...Option) (engine.Interface, error) { } // AddRuntime adds a new runtime to the crio config -func (c *Config) AddRuntime(name string, path string, setAsDefault bool, _ ...map[string]interface{}) error { +func (c *Config) AddRuntime(name string, path string, setAsDefault bool) error { if c == nil { return fmt.Errorf("config is nil") } diff --git a/pkg/config/engine/crio/crio_test.go b/pkg/config/engine/crio/crio_test.go index a43d3a94..17572673 100644 --- a/pkg/config/engine/crio/crio_test.go +++ b/pkg/config/engine/crio/crio_test.go @@ -27,12 +27,11 @@ import ( func TestAddRuntime(t *testing.T) { logger, _ := testlog.NewNullLogger() testCases := []struct { - description string - config string - setAsDefault bool - configOverrides []map[string]interface{} - expectedConfig string - expectedError error + description string + config string + setAsDefault bool + expectedConfig string + expectedError error }{ { description: "empty config not default runtime", @@ -137,7 +136,7 @@ func TestAddRuntime(t *testing.T) { Tree: config, } - err = c.AddRuntime("test", "/usr/bin/test", tc.setAsDefault, tc.configOverrides...) + err = c.AddRuntime("test", "/usr/bin/test", tc.setAsDefault) require.NoError(t, err) require.EqualValues(t, expectedConfig.String(), config.String()) diff --git a/pkg/config/engine/docker/docker.go b/pkg/config/engine/docker/docker.go index e6edbe81..831360f6 100644 --- a/pkg/config/engine/docker/docker.go +++ b/pkg/config/engine/docker/docker.go @@ -49,7 +49,7 @@ func New(opts ...Option) (engine.Interface, error) { } // AddRuntime adds a new runtime to the docker config -func (c *Config) AddRuntime(name string, path string, setAsDefault bool, _ ...map[string]interface{}) error { +func (c *Config) AddRuntime(name string, path string, setAsDefault bool) error { if c == nil { return fmt.Errorf("config is nil") } diff --git a/tools/container/container.go b/tools/container/container.go index 8d09e734..c2c50c5b 100644 --- a/tools/container/container.go +++ b/tools/container/container.go @@ -67,8 +67,8 @@ func ParseArgs(c *cli.Context, o *Options) error { } // Configure applies the options to the specified config -func (o Options) Configure(cfg engine.Interface, configOverrides ...map[string]interface{}) error { - err := o.UpdateConfig(cfg, configOverrides...) +func (o Options) Configure(cfg engine.Interface) error { + err := o.UpdateConfig(cfg) if err != nil { return fmt.Errorf("unable to update config: %v", err) } @@ -98,14 +98,14 @@ func (o Options) flush(cfg engine.Interface) error { } // UpdateConfig updates the specified config to include the nvidia runtimes -func (o Options) UpdateConfig(cfg engine.Interface, configOverrides ...map[string]interface{}) error { +func (o Options) UpdateConfig(cfg engine.Interface) error { runtimes := operator.GetRuntimes( operator.WithNvidiaRuntimeName(o.RuntimeName), operator.WithSetAsDefault(o.SetAsDefault), operator.WithRoot(o.RuntimeDir), ) for name, runtime := range runtimes { - err := cfg.AddRuntime(name, runtime.Path, runtime.SetAsDefault, configOverrides...) + err := cfg.AddRuntime(name, runtime.Path, runtime.SetAsDefault) if err != nil { return fmt.Errorf("failed to update runtime %q: %v", name, err) } diff --git a/tools/container/containerd/containerd.go b/tools/container/containerd/containerd.go index b5dff882..d1d227fb 100644 --- a/tools/container/containerd/containerd.go +++ b/tools/container/containerd/containerd.go @@ -17,7 +17,6 @@ package main import ( - "encoding/json" "fmt" "os" @@ -198,11 +197,7 @@ func Setup(c *cli.Context, o *options) error { return fmt.Errorf("unable to load config: %v", err) } - runtimeConfigOverride, err := o.runtimeConfigOverride() - if err != nil { - return fmt.Errorf("unable to parse config overrides: %w", err) - } - err = o.Configure(cfg, runtimeConfigOverride) + err = o.Configure(cfg) if err != nil { return fmt.Errorf("unable to configure containerd: %v", err) } @@ -260,16 +255,3 @@ func (o *options) containerAnnotationsFromCDIPrefixes() []string { return annotations } - -func (o *options) runtimeConfigOverride() (map[string]interface{}, error) { - if o.runtimeConfigOverrideJSON == "" { - return nil, nil - } - - runtimeOptions := make(map[string]interface{}) - if err := json.Unmarshal([]byte(o.runtimeConfigOverrideJSON), &runtimeOptions); err != nil { - return nil, fmt.Errorf("failed to read %v as JSON: %w", o.runtimeConfigOverrideJSON, err) - } - - return runtimeOptions, nil -} diff --git a/tools/container/containerd/containerd_test.go b/tools/container/containerd/containerd_test.go deleted file mode 100644 index 7240a4dc..00000000 --- a/tools/container/containerd/containerd_test.go +++ /dev/null @@ -1,72 +0,0 @@ -/** -# Copyright 2024 NVIDIA CORPORATION -# -# 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 main - -import ( - "testing" - - "github.com/stretchr/testify/require" -) - -func TestRuntimeOptions(t *testing.T) { - testCases := []struct { - description string - options options - expected map[string]interface{} - expectedError error - }{ - { - description: "empty is nil", - }, - { - description: "empty json", - options: options{ - runtimeConfigOverrideJSON: "{}", - }, - expected: map[string]interface{}{}, - expectedError: nil, - }, - { - description: "SystemdCgroup is true", - options: options{ - runtimeConfigOverrideJSON: "{\"SystemdCgroup\": true}", - }, - expected: map[string]interface{}{ - "SystemdCgroup": true, - }, - expectedError: nil, - }, - { - description: "SystemdCgroup is false", - options: options{ - runtimeConfigOverrideJSON: "{\"SystemdCgroup\": false}", - }, - expected: map[string]interface{}{ - "SystemdCgroup": false, - }, - expectedError: nil, - }, - } - - for _, tc := range testCases { - t.Run(tc.description, func(t *testing.T) { - runtimeOptions, err := tc.options.runtimeConfigOverride() - require.ErrorIs(t, tc.expectedError, err) - require.EqualValues(t, tc.expected, runtimeOptions) - }) - } -}