From a21e379b69916558c1771b086a4a17b0a58f7cc1 Mon Sep 17 00:00:00 2001 From: Ray Burgemeestre Date: Thu, 21 Mar 2024 12:00:24 +0100 Subject: [PATCH 1/2] Allow sections of Plugins to be merged, and not overwritten as entire sections. See for rationale the Pull Request description. Added unit test to demonstrate the difference of this change. Signed-off-by: Ray Burgemeestre --- cmd/containerd/server/config/config.go | 4 -- cmd/containerd/server/config/config_test.go | 73 +++++++++++++++++++++ 2 files changed, 73 insertions(+), 4 deletions(-) diff --git a/cmd/containerd/server/config/config.go b/cmd/containerd/server/config/config.go index c77f165e4..24687b873 100644 --- a/cmd/containerd/server/config/config.go +++ b/cmd/containerd/server/config/config.go @@ -412,10 +412,6 @@ func mergeConfig(to, from *Config) error { } // Replace entire sections instead of merging map's values. - for k, v := range from.Plugins { - to.Plugins[k] = v - } - for k, v := range from.StreamProcessors { to.StreamProcessors[k] = v } diff --git a/cmd/containerd/server/config/config_test.go b/cmd/containerd/server/config/config_test.go index 8240aad01..d76d462f0 100644 --- a/cmd/containerd/server/config/config_test.go +++ b/cmd/containerd/server/config/config_test.go @@ -17,12 +17,17 @@ package config import ( + "bytes" "context" + "fmt" "os" "path/filepath" "sort" + "strings" "testing" + "github.com/pelletier/go-toml/v2" + "github.com/stretchr/testify/assert" "github.com/containerd/containerd/v2/version" @@ -260,3 +265,71 @@ func TestDecodePluginInV1Config(t *testing.T) { assert.NoError(t, err) assert.Equal(t, true, pluginConfig["shim_debug"]) } + +func TestMergingPluginsWithTwoCriDropInConfigs(t *testing.T) { + data1 := ` +[plugins."io.containerd.grpc.v1.cri".cni] + bin_dir = "/cm/local/apps/kubernetes/current/bin/cni" +` + data2 := ` +[plugins."io.containerd.grpc.v1.cri".registry] + config_path = "/cm/local/apps/containerd/var/etc/certs.d" +` + expected := ` +[cni] + bin_dir = '/cm/local/apps/kubernetes/current/bin/cni' + +[registry] + config_path = '/cm/local/apps/containerd/var/etc/certs.d' +` + + testMergeConfig(t, []string{data1, data2}, expected, "io.containerd.grpc.v1.cri") + testMergeConfig(t, []string{data2, data1}, expected, "io.containerd.grpc.v1.cri") +} + +func TestMergingPluginsWithTwoCriCniDropInConfigs(t *testing.T) { + data1 := ` +[plugins."io.containerd.grpc.v1.cri".cni] + bin_dir = "/cm/local/apps/kubernetes/current/bin/cni" +` + data2 := ` +[plugins."io.containerd.grpc.v1.cri".cni] + conf_dir = "/tmp" +` + expected := ` +[cni] + bin_dir = '/cm/local/apps/kubernetes/current/bin/cni' + conf_dir = '/tmp' +` + testMergeConfig(t, []string{data1, data2}, expected, "io.containerd.grpc.v1.cri") +} + +func testMergeConfig(t *testing.T, inputs []string, expected string, comparePlugin string) { + tempDir := t.TempDir() + var result Config + + for i, data := range inputs { + filename := fmt.Sprintf("data%d.toml", i+1) + filepath := filepath.Join(tempDir, filename) + err := os.WriteFile(filepath, []byte(data), 0600) + assert.NoError(t, err) + + var tempOut Config + err = LoadConfig(context.Background(), filepath, &tempOut) + assert.NoError(t, err) + + if i == 0 { + result = tempOut + } else { + err = mergeConfig(&result, &tempOut) + assert.NoError(t, err) + } + } + + criPlugin := result.Plugins[comparePlugin] + var buf bytes.Buffer + if err := toml.NewEncoder(&buf).SetIndentTables(true).Encode(criPlugin); err != nil { + panic(err) + } + assert.Equal(t, strings.TrimLeft(expected, "\n"), buf.String()) +} From 530db2e8d1b5d3d503db0e4dcc11ad94c5978cd2 Mon Sep 17 00:00:00 2001 From: Ray Burgemeestre Date: Tue, 2 Apr 2024 21:19:43 +0200 Subject: [PATCH 2/2] Introduce two additional unit tests for two runtimes and pod annotations. Signed-off-by: Ray Burgemeestre --- cmd/containerd/server/config/config_test.go | 94 +++++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/cmd/containerd/server/config/config_test.go b/cmd/containerd/server/config/config_test.go index d76d462f0..fe999c2a5 100644 --- a/cmd/containerd/server/config/config_test.go +++ b/cmd/containerd/server/config/config_test.go @@ -304,6 +304,100 @@ func TestMergingPluginsWithTwoCriCniDropInConfigs(t *testing.T) { testMergeConfig(t, []string{data1, data2}, expected, "io.containerd.grpc.v1.cri") } +func TestMergingPluginsWithTwoCriRuntimeDropInConfigs(t *testing.T) { + runcRuntime := ` +[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 +` + nvidiaRuntime := ` +[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_engine = "" + runtime_root = "" + runtime_type = "io.containerd.runc.v2" + [plugins."io.containerd.grpc.v1.cri".containerd.runtimes.nvidia.options] + BinaryName = "/usr/bin/nvidia-container-runtime" + SystemdCgroup = true +` + expected := ` +[containerd] + default_runtime_name = 'nvidia' + + [containerd.runtimes] + [containerd.runtimes.nvidia] + privileged_without_host_devices = false + runtime_engine = '' + runtime_root = '' + runtime_type = 'io.containerd.runc.v2' + + [containerd.runtimes.nvidia.options] + BinaryName = '/usr/bin/nvidia-container-runtime' + SystemdCgroup = true + + [containerd.runtimes.runc] + runtime_type = 'io.containerd.runc.v2' + + [containerd.runtimes.runc.options] + SystemdCgroup = true +` + testMergeConfig(t, []string{runcRuntime, nvidiaRuntime}, expected, "io.containerd.grpc.v1.cri") + + // Merging a third config that customizes only the default_runtime_name should result in mostly identical result + runcDefault := ` + [plugins."io.containerd.grpc.v1.cri".containerd] + default_runtime_name = "runc" +` + // This will then be the only difference in our expected TOML + expected2 := strings.Replace(expected, "default_runtime_name = 'nvidia'", "default_runtime_name = 'runc'", 1) + + testMergeConfig(t, []string{runcRuntime, nvidiaRuntime, runcDefault}, expected2, "io.containerd.grpc.v1.cri") + + // Mixing up the order will again result in 'nvidia' being the default runtime + testMergeConfig(t, []string{runcRuntime, runcDefault, nvidiaRuntime}, expected, "io.containerd.grpc.v1.cri") +} + +func TestMergingPluginsWithTwoCriRuntimeWithPodAnnotationsDropInConfigs(t *testing.T) { + runc1 := ` +[plugins."io.containerd.grpc.v1.cri".containerd.runtimes.runc] + runtime_type = "io.containerd.runc.v2" + cni_conf_dir = "/foo" + pod_annotations = ["a", "b", "c"] +` + runc2 := ` +[plugins."io.containerd.grpc.v1.cri".containerd.runtimes.runc] + runtime_type = "io.containerd.runc.v2" + cni_conf_dir = "/bar" + pod_annotations = ["d", "e", "f"] +` + expected := ` +[containerd] + [containerd.runtimes] + [containerd.runtimes.runc] + cni_conf_dir = '/bar' + pod_annotations = ['d', 'e', 'f'] + runtime_type = 'io.containerd.runc.v2' +` + testMergeConfig(t, []string{runc1, runc2}, expected, "io.containerd.grpc.v1.cri") + + // The other way around: runc1 over runc2 + expected = ` +[containerd] + [containerd.runtimes] + [containerd.runtimes.runc] + cni_conf_dir = '/foo' + pod_annotations = ['a', 'b', 'c'] + runtime_type = 'io.containerd.runc.v2' +` + testMergeConfig(t, []string{runc2, runc1}, expected, "io.containerd.grpc.v1.cri") +} + func testMergeConfig(t *testing.T, inputs []string, expected string, comparePlugin string) { tempDir := t.TempDir() var result Config