Skip to content

Commit

Permalink
Undo most of the changes that were incorrect, except for the new unit…
Browse files Browse the repository at this point in the history 
… test with fixes.

Signed-off-by: Ray Burgemeestre <rayb@nvidia.com>
  • Loading branch information
@rayburgemeestre
rayburgemeestre committed Feb 14, 2024
1 parent ca76cdb commit d6fad3d
Show file tree
Hide file tree
Showing 4 changed files with 104 additions and 95 deletions.
31 changes: 9 additions & 22 deletions cmd/containerd/command/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,11 @@ import (
"github.com/urfave/cli"
)

func outputConfig(ctx gocontext.Context, userConfig *srvconfig.Config) error {
return generateConfig(ctx, userConfig, os.Stdout)
func outputConfig(ctx gocontext.Context, config *srvconfig.Config) error {
return generateConfig(ctx, config, os.Stdout)
}

func generateConfig(ctx gocontext.Context, userConfig *srvconfig.Config, output io.Writer) error {
// Make sure we have a 'base' config with all available plugins set
config := &srvconfig.Config{}
func generateConfig(ctx gocontext.Context, config *srvconfig.Config, output io.Writer) error {
plugins, err := server.LoadPlugins(ctx, config)
if err != nil {
return err
Expand Down Expand Up @@ -78,11 +76,6 @@ func generateConfig(ctx gocontext.Context, userConfig *srvconfig.Config, output
// this command, generate the max configuration version
config.Version = srvconfig.CurrentConfigVersion

// Now merge on top of this, the actual user config that is present on disk
if err = srvconfig.MergeConfig(config, userConfig); err != nil {
return err
}

return toml.NewEncoder(output).SetIndentTables(true).Encode(config)
}

Expand Down Expand Up @@ -118,24 +111,23 @@ var configCommand = cli.Command{
Name: "migrate",
Usage: "Migrate the current configuration file to the latest version (does not migrate subconfig files)",
Action: func(context *cli.Context) error {
userConfig := defaultConfig()
config := defaultConfig()
ctx := gocontext.Background()
if err := srvconfig.LoadConfig(ctx, context.GlobalString("userConfig"), userConfig); err != nil && !os.IsNotExist(err) {
if err := srvconfig.LoadConfig(ctx, context.GlobalString("config"), config); err != nil && !os.IsNotExist(err) {
return err
}

if userConfig.Version < srvconfig.CurrentConfigVersion {
plugins := registry.Graph(srvconfig.V2DisabledFilter(userConfig.DisabledPlugins))
if config.Version < srvconfig.CurrentConfigVersion {
plugins := registry.Graph(srvconfig.V2DisabledFilter(config.DisabledPlugins))
for _, p := range plugins {
if p.ConfigMigration != nil {
if err := p.ConfigMigration(ctx, userConfig.Version, userConfig.Plugins); err != nil {
if err := p.ConfigMigration(ctx, config.Version, config.Plugins); err != nil {
return err
}
}
}
}

config := &srvconfig.Config{}
plugins, err := server.LoadPlugins(ctx, config)
if err != nil {
return err
Expand All @@ -160,12 +152,7 @@ var configCommand = cli.Command{

config.Version = srvconfig.CurrentConfigVersion

// Now merge on top of this, the actual user config that is present on disk
if err = srvconfig.MergeConfig(userConfig, userConfig); err != nil {
return err
}

return toml.NewEncoder(os.Stdout).SetIndentTables(true).Encode(userConfig)
return toml.NewEncoder(os.Stdout).SetIndentTables(true).Encode(config)
},
},
},
Expand Down
152 changes: 87 additions & 65 deletions cmd/containerd/command/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,93 +23,109 @@ import (
"path/filepath"
"testing"

srvconfig "github.com/containerd/containerd/v2/cmd/containerd/server/config"
"github.com/pelletier/go-toml/v2"

srvconfig "github.com/containerd/containerd/v2/cmd/containerd/server/config"
// without the following two includes the behavior of this unit test would be different
_ "github.com/containerd/containerd/v2/plugins/cri"
_ "github.com/containerd/containerd/v2/plugins/cri/runtime"
"github.com/stretchr/testify/assert"
)

func TestCommandConfig(t *testing.T) {
// this will fail to map to structs though in containerd 2.x, but for testing we can still use it
data1 := `
version = 2
[plugins."io.containerd.grpc.v1.cri".registry.configs."registry-1.docker.io".auth]
username = "${username}"
password = "${password}"
`
// sandbox_image also won't map
data2 := `
[plugins."io.containerd.grpc.v1.cri"]
sandbox_image = "${sandbox_image}"
`
// from now on all should be okay
// deprecated but still accepted at the time of writing
data1 := ` version = 2
[plugins."io.containerd.grpc.v1.runtime".registry.configs."registry-1.docker.io".auth]
username = "my-username"
password = "my-password"
`
// the old location, should not be accepted at all
data2 := ` version = 2
[plugins."io.containerd.grpc.v1.cri".registry.configs."registry-1.docker.io".auth]
username = "should-not-be-present"
password = "should-not-be-present"
`
data3 := `
[plugins."io.containerd.grpc.v1.cri".registry]
config_path = "/cm/local/apps/containerd/var/etc/certs.d"
`
[plugins."io.containerd.grpc.v1.runtime"]
sandbox_image = "my-sandbox-image:1.0"
`
data4 := `
[plugins."io.containerd.grpc.v1.cri".containerd.runtimes.runc]
runtime_type = "io.containerd.runc.v2"
[plugins."io.containerd.grpc.v1.cri".containerd.runtimes.runc.options]
SystemdCgroup = true
`
[plugins."io.containerd.grpc.v1.runtime".registry]
config_path = "/my-custom-certs.d-config-path"
`
data5 := `
[plugins."io.containerd.grpc.v1.cri".cni]
bin_dir = "/cm/local/apps/kubernetes/current/bin/cni"
`
[plugins."io.containerd.grpc.v1.runtime".containerd.runtimes.runc]
runtime_type = "io.containerd.runc.v2"
[plugins."io.containerd.grpc.v1.runtime".containerd.runtimes.runc.options]
SystemdCgroup = true
`
data6 := `
[plugins]
[plugins."io.containerd.grpc.v1.cri"]
[plugins."io.containerd.grpc.v1.cri".containerd]
default_runtime_name = "nvidia"
[plugins."io.containerd.grpc.v1.cri".containerd.runtimes]
[plugins."io.containerd.grpc.v1.cri".containerd.runtimes.nvidia]
privileged_without_host_devices = false
runtime_type = "io.containerd.runc.v2"
[plugins."io.containerd.grpc.v1.cri".containerd.runtimes.nvidia.options]
BinaryName = "/usr/bin/nvidia-container-runtime"
SystemdCgroup = true
[plugins.'io.containerd.grpc.v1.cri'.cni]
bin_dir = '/should-not-be-present'
conf_dir = '/should-not-be-present'
`
expected := `
[plugins]
[plugins.'io.containerd.grpc.v1.cri']
sandbox_image = '${sandbox_image}'
[plugins.'io.containerd.grpc.v1.cri'.cni]
bin_dir = '/cm/local/apps/kubernetes/current/bin/cni'
[plugins.'io.containerd.grpc.v1.cri'.containerd]
data7 := `
[plugins.'io.containerd.grpc.v1.runtime'.cni]
bin_dir = '/custom-bin-dir'
`
data8 := `
[plugins.'io.containerd.grpc.v1.runtime'.cni]
conf_dir = '/custom-conf-dir'
`
data9 := `
[plugins]
[plugins."io.containerd.grpc.v1.runtime"]
[plugins."io.containerd.grpc.v1.runtime".containerd]
default_runtime_name = "nvidia"
[plugins."io.containerd.grpc.v1.runtime".containerd.runtimes]
[plugins."io.containerd.grpc.v1.runtime".containerd.runtimes.nvidia]
privileged_without_host_devices = false
runtime_type = "io.containerd.runc.v2"
[plugins."io.containerd.grpc.v1.runtime".containerd.runtimes.nvidia.options]
BinaryName = "/usr/bin/nvidia-container-runtime"
SystemdCgroup = true
`
expected_runtimes := `

Check warning on line 88 in cmd/containerd/command/config_test.go

View workflow job for this annotation

GitHub Actions / Linters (ubuntu-22.04) 

var-naming: don't use underscores in Go names; var expected_runtimes should be expectedRuntimes (revive)
[plugins.'io.containerd.grpc.v1.runtime'.containerd]
default_runtime_name = 'nvidia'
[plugins.'io.containerd.grpc.v1.cri'.containerd.runtimes]
[plugins.'io.containerd.grpc.v1.cri'.containerd.runtimes.nvidia]
[plugins.'io.containerd.grpc.v1.runtime'.containerd.runtimes]
[plugins.'io.containerd.grpc.v1.runtime'.containerd.runtimes.nvidia]
privileged_without_host_devices = false
runtime_type = 'io.containerd.runc.v2'
[plugins.'io.containerd.grpc.v1.cri'.containerd.runtimes.nvidia.options]
[plugins.'io.containerd.grpc.v1.runtime'.containerd.runtimes.nvidia.options]
BinaryName = '/usr/bin/nvidia-container-runtime'
SystemdCgroup = true
[plugins.'io.containerd.grpc.v1.cri'.containerd.runtimes.runc]
[plugins.'io.containerd.grpc.v1.runtime'.containerd.runtimes.runc]
runtime_type = 'io.containerd.runc.v2'
[plugins.'io.containerd.grpc.v1.cri'.containerd.runtimes.runc.options]
[plugins.'io.containerd.grpc.v1.runtime'.containerd.runtimes.runc.options]
SystemdCgroup = true
[plugins.'io.containerd.grpc.v1.cri'.registry]
config_path = '/cm/local/apps/containerd/var/etc/certs.d'
[plugins.'io.containerd.grpc.v1.cri'.registry.configs]
[plugins.'io.containerd.grpc.v1.cri'.registry.configs.'registry-1.docker.io']
[plugins.'io.containerd.grpc.v1.cri'.registry.configs.'registry-1.docker.io'.auth]
password = '${password}'
username = '${username}'
`
// currently we cannot invoke more than one times, due to:
// currently we cannot invoke testMergeConfig() more than once, due to:
// panic: io.containerd.content.v1.content: plugin: id already registered
testMergeConfig(t, []string{data1, data2, data3, data4, data5, data6}, expected)
asserts := []CheckAsserts{
{Expected: false, Value: "should-not-be-present"},
{Expected: true, Value: "/custom-bin-dir"},
{Expected: true, Value: "/custom-conf-dir"},
{Expected: true, Value: "my-username"},
{Expected: true, Value: "my-password"},
{Expected: true, Value: "my-sandbox-image:1.0"},
{Expected: true, Value: "my-sandbox-image:1.0"},
{Expected: true, Value: "/my-custom-certs.d-config-path"},
{Expected: true, Value: expected_runtimes},
}
testMergeConfig(t, []string{data1, data2, data3, data4, data5, data6, data7, data8, data9}, asserts)
}

func testMergeConfig(t *testing.T, inputs []string, expected string) {
type CheckAsserts struct {
Expected bool
Value string
}

func testMergeConfig(t *testing.T, inputs []string, asserts []CheckAsserts) {
tempDir := t.TempDir()
var result srvconfig.Config

Expand All @@ -127,11 +143,10 @@ func testMergeConfig(t *testing.T, inputs []string, expected string) {
mainFilepath := filepath.Join(tempDir, "containerd.toml")
resultString, err := toml.Marshal(result)
assert.NoError(t, err)

err = os.WriteFile(mainFilepath, resultString, 0600)
assert.NoError(t, err)

// now load this config, and see if all the imports it will do results in a config containing the expected string
// now load this config, and see if all the imports results in what we expect
config := defaultConfig()
ctx := context.Background()
err = srvconfig.LoadConfig(ctx, mainFilepath, config)
Expand All @@ -140,5 +155,12 @@ func testMergeConfig(t *testing.T, inputs []string, expected string) {
var buf bytes.Buffer
err = generateConfig(ctx, config, &buf)
assert.NoError(t, err)
assert.Contains(t, buf.String(), expected)

for _, item := range asserts {
if item.Expected {
assert.Contains(t, buf.String(), item.Value)
} else {
assert.NotContains(t, buf.String(), item.Value)
}
}
}
6 changes: 3 additions & 3 deletions cmd/containerd/server/config/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ func LoadConfig(ctx context.Context, path string, out *Config) error {
return err
}

if err := MergeConfig(out, config); err != nil {
if err := mergeConfig(out, config); err != nil {
return err
}

Expand Down Expand Up @@ -395,7 +395,7 @@ func resolveImports(parent string, imports []string) ([]string, error) {
return out, nil
}

// MergeConfig merges Config structs with the following rules:
// mergeConfig merges Config structs with the following rules:
// 'to' 'from' 'result'
// "" "value" "value"
// "value" "" "value"
Expand All @@ -406,7 +406,7 @@ func resolveImports(parent string, imports []string) ([]string, error) {
// []{"1", "2"} []{"1"} []{"1","2"}
// []{} []{"2"} []{"2"}
// Maps merged by keys, but values are replaced entirely.
func MergeConfig(to, from *Config) error {
func mergeConfig(to, from *Config) error {
err := mergo.Merge(to, from, mergo.WithOverride, mergo.WithTransformers(sliceTransformer{}))
if err != nil {
return err
Expand Down
10 changes: 5 additions & 5 deletions cmd/containerd/server/config/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ func TestMergeConfigs(t *testing.T) {
StreamProcessors: map[string]StreamProcessor{"1": {Path: "3"}},
}

err := MergeConfig(a, b)
err := mergeConfig(a, b)
assert.NoError(t, err)

assert.Equal(t, 2, a.Version)
Expand All @@ -77,13 +77,13 @@ func TestMergeConfigs(t *testing.T) {
// https://github.com/containerd/containerd/blob/v1.6.0/services/server/config/config.go#L322-L323
a = &Config{Version: 2, OOMScore: 1}
b = &Config{Version: 2, OOMScore: 0} // OOMScore "not set / default"
err = MergeConfig(a, b)
err = mergeConfig(a, b)
assert.NoError(t, err)
assert.Equal(t, 1, a.OOMScore)

a = &Config{Version: 2, OOMScore: 1}
b = &Config{Version: 2, OOMScore: 0} // OOMScore "not set / default"
err = MergeConfig(a, b)
err = mergeConfig(a, b)
assert.NoError(t, err)
assert.Equal(t, 1, a.OOMScore)
}
Expand Down Expand Up @@ -292,7 +292,7 @@ func TestMergingTwoPluginConfigsMerge(t *testing.T) {
testMergeConfig(t, []string{data1, data2}, expected, "io.containerd.grpc.v1.cri")
}

func TestMergingTwoPluginConfigsRecursively(t *testing.T) {
func TestMergingTwoPluginConfigsWithNesting(t *testing.T) {
// Configuration that configures runtime: runc
data1 := `
[plugins."io.containerd.grpc.v1.cri".containerd.runtimes.runc]
Expand Down Expand Up @@ -368,7 +368,7 @@ func testMergeConfig(t *testing.T, inputs []string, expected string, comparePlug
if i == 0 {
result = tempOut
} else {
err = MergeConfig(&result, &tempOut)
err = mergeConfig(&result, &tempOut)
assert.NoError(t, err)
}
}
Expand Down

0 comments on commit d6fad3d

Please sign in to comment.