From 7566eb124a7dcb89128d511148b18b9839c9f542 Mon Sep 17 00:00:00 2001 From: Evan Lezar <7723350-elezar@users.noreply.gitlab.com> Date: Thu, 23 Nov 2023 12:35:09 +0000 Subject: [PATCH] Merge branch 'fix-config-update-command' into 'main' Switch to reflect package for config updates See merge request nvidia/container-toolkit/container-toolkit!500 --- CHANGELOG.md | 1 + cmd/nvidia-ctk/config/config.go | 88 +++++++++++++----- cmd/nvidia-ctk/config/config_test.go | 133 +++++++++------------------ 3 files changed, 109 insertions(+), 113 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bde84c17..dc74bf23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## v1.14.4 * Include `nvidia/nvoptix.bin` in list of graphics mounts. * Include `vulkan/icd.d/nvidia_layers.json` in list of graphics mounts. +* Fixed bug in `nvidia-ctk config` command when using `--set`. The types of applied config options are now applied correctly. ## v1.14.3 * [toolkit-container] Bump CUDA base image version to 12.2.2. diff --git a/cmd/nvidia-ctk/config/config.go b/cmd/nvidia-ctk/config/config.go index ee5832b6..fc73b97d 100644 --- a/cmd/nvidia-ctk/config/config.go +++ b/cmd/nvidia-ctk/config/config.go @@ -19,14 +19,16 @@ package config import ( "errors" "fmt" + "reflect" "strconv" "strings" + "github.com/urfave/cli/v2" + createdefault "github.com/NVIDIA/nvidia-container-toolkit/cmd/nvidia-ctk/config/create-default" "github.com/NVIDIA/nvidia-container-toolkit/cmd/nvidia-ctk/config/flags" "github.com/NVIDIA/nvidia-container-toolkit/internal/config" "github.com/NVIDIA/nvidia-container-toolkit/internal/logger" - "github.com/urfave/cli/v2" ) type command struct { @@ -103,7 +105,7 @@ func run(c *cli.Context, opts *options) error { } for _, set := range opts.sets.Value() { - key, value, err := (*configToml)(cfgToml).setFlagToKeyValue(set) + key, value, err := setFlagToKeyValue(set) if err != nil { return fmt.Errorf("invalid --set option %v: %w", set, err) } @@ -126,50 +128,86 @@ func run(c *cli.Context, opts *options) error { return nil } -type configToml config.Toml - var errInvalidConfigOption = errors.New("invalid config option") +var errUndefinedField = errors.New("undefined field") var errInvalidFormat = errors.New("invalid format") // setFlagToKeyValue converts a --set flag to a key-value pair. // The set flag is of the form key[=value], with the value being optional if key refers to a // boolean config option. -func (c *configToml) setFlagToKeyValue(setFlag string) (string, interface{}, error) { - if c == nil { - return "", nil, errInvalidConfigOption - } - +func setFlagToKeyValue(setFlag string) (string, interface{}, error) { setParts := strings.SplitN(setFlag, "=", 2) key := setParts[0] - v := (*config.Toml)(c).Get(key) - if v == nil { - return key, nil, errInvalidConfigOption - } - switch v.(type) { - case bool: - if len(setParts) == 1 { - return key, true, nil - } + field, err := getField(key) + if err != nil { + return key, nil, fmt.Errorf("%w: %w", errInvalidConfigOption, err) } + kind := field.Kind() if len(setParts) != 2 { + if kind == reflect.Bool { + return key, true, nil + } return key, nil, fmt.Errorf("%w: expected key=value; got %v", errInvalidFormat, setFlag) } value := setParts[1] - switch vt := v.(type) { - case bool: + switch kind { + case reflect.Bool: b, err := strconv.ParseBool(value) if err != nil { return key, value, fmt.Errorf("%w: %w", errInvalidFormat, err) } return key, b, err - case string: + case reflect.String: return key, value, nil - case []string: - return key, strings.Split(value, ","), nil - default: - return key, nil, fmt.Errorf("unsupported type for %v (%v)", setParts, vt) + case reflect.Slice: + valueParts := strings.Split(value, ",") + switch field.Elem().Kind() { + case reflect.String: + return key, valueParts, nil + case reflect.Int: + var output []int64 + for _, v := range valueParts { + vi, err := strconv.ParseInt(v, 10, 0) + if err != nil { + return key, nil, fmt.Errorf("%w: %w", errInvalidFormat, err) + } + output = append(output, vi) + } + return key, output, nil + } } + return key, nil, fmt.Errorf("unsupported type for %v (%v)", setParts, kind) +} + +func getField(key string) (reflect.Type, error) { + s, err := getStruct(reflect.TypeOf(config.Config{}), strings.Split(key, ".")...) + if err != nil { + return nil, err + } + return s.Type, err +} + +func getStruct(current reflect.Type, paths ...string) (reflect.StructField, error) { + if len(paths) < 1 { + return reflect.StructField{}, fmt.Errorf("%w: no fields selected", errUndefinedField) + } + tomlField := paths[0] + for i := 0; i < current.NumField(); i++ { + f := current.Field(i) + v, ok := f.Tag.Lookup("toml") + if !ok { + continue + } + if v != tomlField { + continue + } + if len(paths) == 1 { + return f, nil + } + return getStruct(f.Type, paths[1:]...) + } + return reflect.StructField{}, fmt.Errorf("%w: %q", errUndefinedField, tomlField) } diff --git a/cmd/nvidia-ctk/config/config_test.go b/cmd/nvidia-ctk/config/config_test.go index bab1cb4d..eca474e9 100644 --- a/cmd/nvidia-ctk/config/config_test.go +++ b/cmd/nvidia-ctk/config/config_test.go @@ -19,152 +19,109 @@ package config import ( "testing" - "github.com/NVIDIA/nvidia-container-toolkit/internal/config" - "github.com/pelletier/go-toml" "github.com/stretchr/testify/require" ) func TestSetFlagToKeyValue(t *testing.T) { + // TODO: We need to enable this test again since switching to reflect. testCases := []struct { description string - config map[string]interface{} setFlag string expectedKey string expectedValue interface{} expectedError error }{ { - description: "empty config returns an error", - setFlag: "anykey=value", - expectedKey: "anykey", - expectedError: errInvalidConfigOption, - }, - { - description: "option not present returns an error", - config: map[string]interface{}{ - "defined": "defined-value", - }, + description: "option not present returns an error", setFlag: "undefined=new-value", expectedKey: "undefined", expectedError: errInvalidConfigOption, }, { - description: "boolean option assumes true", - config: map[string]interface{}{ - "boolean": false, - }, - setFlag: "boolean", - expectedKey: "boolean", + description: "undefined nexted option returns error", + setFlag: "nvidia-container-cli.undefined", + expectedKey: "nvidia-container-cli.undefined", + expectedError: errInvalidConfigOption, + }, + { + description: "boolean option assumes true", + setFlag: "disable-require", + expectedKey: "disable-require", expectedValue: true, }, { - description: "boolean option returns true", - config: map[string]interface{}{ - "boolean": false, - }, - setFlag: "boolean=true", - expectedKey: "boolean", + description: "boolean option returns true", + setFlag: "disable-require=true", + expectedKey: "disable-require", expectedValue: true, }, { - description: "boolean option returns false", - config: map[string]interface{}{ - "boolean": false, - }, - setFlag: "boolean=false", - expectedKey: "boolean", + description: "boolean option returns false", + setFlag: "disable-require=false", + expectedKey: "disable-require", expectedValue: false, }, { - description: "invalid boolean option returns error", - config: map[string]interface{}{ - "boolean": false, - }, - setFlag: "boolean=something", - expectedKey: "boolean", + description: "invalid boolean option returns error", + setFlag: "disable-require=something", + expectedKey: "disable-require", expectedValue: "something", expectedError: errInvalidFormat, }, { - description: "string option requires value", - config: map[string]interface{}{ - "string": "value", - }, - setFlag: "string", - expectedKey: "string", + description: "string option requires value", + setFlag: "swarm-resource", + expectedKey: "swarm-resource", expectedValue: nil, expectedError: errInvalidFormat, }, { - description: "string option returns value", - config: map[string]interface{}{ - "string": "value", - }, - setFlag: "string=string-value", - expectedKey: "string", + description: "string option returns value", + setFlag: "swarm-resource=string-value", + expectedKey: "swarm-resource", expectedValue: "string-value", }, { - description: "string option returns value with equals", - config: map[string]interface{}{ - "string": "value", - }, - setFlag: "string=string-value=more", - expectedKey: "string", + description: "string option returns value with equals", + setFlag: "swarm-resource=string-value=more", + expectedKey: "swarm-resource", expectedValue: "string-value=more", }, { - description: "string option treats bool value as string", - config: map[string]interface{}{ - "string": "value", - }, - setFlag: "string=true", - expectedKey: "string", + description: "string option treats bool value as string", + setFlag: "swarm-resource=true", + expectedKey: "swarm-resource", expectedValue: "true", }, { - description: "string option treats int value as string", - config: map[string]interface{}{ - "string": "value", - }, - setFlag: "string=5", - expectedKey: "string", + description: "string option treats int value as string", + setFlag: "swarm-resource=5", + expectedKey: "swarm-resource", expectedValue: "5", }, { - description: "[]string option returns single value", - config: map[string]interface{}{ - "string": []string{"value"}, - }, - setFlag: "string=string-value", - expectedKey: "string", + description: "[]string option returns single value", + setFlag: "nvidia-container-cli.environment=string-value", + expectedKey: "nvidia-container-cli.environment", expectedValue: []string{"string-value"}, }, { - description: "[]string option returns multiple values", - config: map[string]interface{}{ - "string": []string{"value"}, - }, - setFlag: "string=first,second", - expectedKey: "string", + description: "[]string option returns multiple values", + setFlag: "nvidia-container-cli.environment=first,second", + expectedKey: "nvidia-container-cli.environment", expectedValue: []string{"first", "second"}, }, { - description: "[]string option returns values with equals", - config: map[string]interface{}{ - "string": []string{"value"}, - }, - setFlag: "string=first=1,second=2", - expectedKey: "string", + description: "[]string option returns values with equals", + setFlag: "nvidia-container-cli.environment=first=1,second=2", + expectedKey: "nvidia-container-cli.environment", expectedValue: []string{"first=1", "second=2"}, }, } for _, tc := range testCases { t.Run(tc.description, func(t *testing.T) { - tree, _ := toml.TreeFromMap(tc.config) - cfgToml := (*config.Toml)(tree) - k, v, err := (*configToml)(cfgToml).setFlagToKeyValue(tc.setFlag) + k, v, err := setFlagToKeyValue(tc.setFlag) require.ErrorIs(t, err, tc.expectedError) require.EqualValues(t, tc.expectedKey, k) require.EqualValues(t, tc.expectedValue, v)