From 4cd719692e315abb63c17cd5a91c2908a17c1d8e Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Wed, 25 May 2022 20:09:18 +0200 Subject: [PATCH 1/4] Use BinaryName for v1 containerd runtime config This fixes a bug where the runtime path for v1 containerd configs was specified in the options.Runtime setting (which is used for the default runtime) instead of options.BinaryName. Signed-off-by: Evan Lezar --- tools/container/containerd/config.go | 15 ++++-------- tools/container/containerd/config_v1.go | 9 ++++---- tools/container/containerd/config_v1_test.go | 24 ++++++++++++++++---- tools/container/containerd/config_v2.go | 7 +++--- 4 files changed, 32 insertions(+), 23 deletions(-) diff --git a/tools/container/containerd/config.go b/tools/container/containerd/config.go index 8182eb9e..629ec7f3 100644 --- a/tools/container/containerd/config.go +++ b/tools/container/containerd/config.go @@ -28,9 +28,8 @@ type UpdateReverter interface { type config struct { *toml.Tree - version int64 - cri string - binaryKey string + version int64 + cri string } // update adds the specified runtime class to the the containerd config. @@ -48,7 +47,7 @@ func (config *config) update(runtimeClass string, runtimeType string, runtimeBin config.SetPath(runtimeClassPath, runc) } - config.initRuntime(runtimeClassPath, runtimeType, runtimeBinary) + config.initRuntime(runtimeClassPath, runtimeType, "BinaryName", runtimeBinary) if setAsDefault { defaultRuntimeNamePath := config.defaultRuntimeNamePath() @@ -83,7 +82,7 @@ func (config *config) revert(runtimeClass string) { // initRuntime creates a runtime config if it does not exist and ensures that the // runtimes binary path is specified. -func (config *config) initRuntime(path []string, runtimeType string, binary string) { +func (config *config) initRuntime(path []string, runtimeType string, binaryKey string, binary string) { if config.GetPath(path) == nil { config.SetPath(append(path, "runtime_type"), runtimeType) config.SetPath(append(path, "runtime_root"), "") @@ -91,7 +90,7 @@ func (config *config) initRuntime(path []string, runtimeType string, binary stri config.SetPath(append(path, "privileged_without_host_devices"), false) } - binaryPath := append(path, "options", config.binaryKey) + binaryPath := append(path, "options", binaryKey) config.SetPath(binaryPath, binary) } @@ -99,10 +98,6 @@ func (config config) runcPath() []string { return config.runtimeClassPath("runc") } -func (config config) runtimeClassBinaryPath(runtimeClass string) []string { - return append(config.runtimeClassPath(runtimeClass), "options", config.binaryKey) -} - func (config config) runtimeClassPath(runtimeClass string) []string { return append(config.containerdPath(), "runtimes", runtimeClass) } diff --git a/tools/container/containerd/config_v1.go b/tools/container/containerd/config_v1.go index e46ec21b..1d98776f 100644 --- a/tools/container/containerd/config_v1.go +++ b/tools/container/containerd/config_v1.go @@ -31,10 +31,9 @@ type configV1 struct { func newConfigV1(cfg *toml.Tree) UpdateReverter { c := configV1{ config: config{ - Tree: cfg, - version: 1, - cri: "cri", - binaryKey: "Runtime", + Tree: cfg, + version: 1, + cri: "cri", }, } @@ -68,7 +67,7 @@ func (config *configV1) Update(o *options) error { log.Warnf("Setting default_runtime is deprecated") defaultRuntimePath := append(config.containerdPath(), "default_runtime") - config.initRuntime(defaultRuntimePath, o.runtimeType, runtimeBinary) + config.initRuntime(defaultRuntimePath, o.runtimeType, "Runtime", runtimeBinary) } return nil } diff --git a/tools/container/containerd/config_v1_test.go b/tools/container/containerd/config_v1_test.go index 246d5274..b1f39677 100644 --- a/tools/container/containerd/config_v1_test.go +++ b/tools/container/containerd/config_v1_test.go @@ -110,7 +110,7 @@ func TestUpdateV1ConfigDefaultRuntime(t *testing.T) { if tc.expectedDefaultRuntimeBinary == nil { require.Nil(t, defaultRuntime, "%d: %v", i, tc) } else { - expected, err := runtimeTomlConfigV1(tc.expectedDefaultRuntimeBinary.(string)) + expected, err := defaultRuntimeTomlConfigV1(tc.expectedDefaultRuntimeBinary.(string)) require.NoError(t, err, "%d: %v", i, tc) configContents, _ := toml.Marshal(defaultRuntime.(*toml.Tree)) @@ -291,7 +291,7 @@ func TestRevertV1Config(t *testing.T) { "nvidia": runtimeMapV1("/test/runtime/dir/nvidia-container-runtime"), "nvidia-experimental": runtimeMapV1("/test/runtime/dir/nvidia-container-runtime-experimental"), }, - "default_runtime": runtimeMapV1("/test/runtime/dir/nvidia-container-runtime"), + "default_runtime": defaultRuntimeV1("/test/runtime/dir/nvidia-container-runtime"), "default_runtime_name": "nvidia", }, }, @@ -325,7 +325,11 @@ func runtimeTomlConfigV1(binary string) (*toml.Tree, error) { return toml.TreeFromMap(runtimeMapV1(binary)) } -func runtimeMapV1(binary string) map[string]interface{} { +func defaultRuntimeTomlConfigV1(binary string) (*toml.Tree, error) { + return toml.TreeFromMap(defaultRuntimeV1(binary)) +} + +func defaultRuntimeV1(binary string) map[string]interface{} { return map[string]interface{}{ "runtime_type": runtimeType, "runtime_root": "", @@ -337,6 +341,18 @@ func runtimeMapV1(binary string) map[string]interface{} { } } +func runtimeMapV1(binary string) map[string]interface{} { + return map[string]interface{}{ + "runtime_type": runtimeType, + "runtime_root": "", + "runtime_engine": "", + "privileged_without_host_devices": false, + "options": map[string]interface{}{ + "BinaryName": binary, + }, + } +} + func runcConfigMapV1(binary string) map[string]interface{} { return map[string]interface{}{ "plugins": map[string]interface{}{ @@ -359,7 +375,7 @@ func runcRuntimeConfigMapV1(binary string) map[string]interface{} { "privileged_without_host_devices": true, "options": map[string]interface{}{ "runc-option": "value", - "Runtime": binary, + "BinaryName": binary, }, } } diff --git a/tools/container/containerd/config_v2.go b/tools/container/containerd/config_v2.go index 4d0ef4d9..fac96b0f 100644 --- a/tools/container/containerd/config_v2.go +++ b/tools/container/containerd/config_v2.go @@ -28,10 +28,9 @@ type configV2 struct { func newConfigV2(cfg *toml.Tree) UpdateReverter { c := configV2{ config: config{ - Tree: cfg, - version: 2, - cri: "io.containerd.grpc.v1.cri", - binaryKey: "BinaryName", + Tree: cfg, + version: 2, + cri: "io.containerd.grpc.v1.cri", }, } From 15cbd54d1c4dc8a84b199aee1e6bcef91c097e41 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Wed, 25 May 2022 20:10:28 +0200 Subject: [PATCH 2/4] Also set Runtime file v1 containerd runtime config This ensures that older versions of containerd that may be expecting this over options.BinaryName should continue to work. Signed-off-by: Evan Lezar --- tools/container/containerd/config.go | 3 +++ tools/container/containerd/config_v1_test.go | 2 ++ 2 files changed, 5 insertions(+) diff --git a/tools/container/containerd/config.go b/tools/container/containerd/config.go index 629ec7f3..2e61be5d 100644 --- a/tools/container/containerd/config.go +++ b/tools/container/containerd/config.go @@ -48,6 +48,9 @@ func (config *config) update(runtimeClass string, runtimeType string, runtimeBin } config.initRuntime(runtimeClassPath, runtimeType, "BinaryName", runtimeBinary) + if config.version == 1 { + config.initRuntime(runtimeClassPath, runtimeType, "Runtime", runtimeBinary) + } if setAsDefault { defaultRuntimeNamePath := config.defaultRuntimeNamePath() diff --git a/tools/container/containerd/config_v1_test.go b/tools/container/containerd/config_v1_test.go index b1f39677..0b69f5aa 100644 --- a/tools/container/containerd/config_v1_test.go +++ b/tools/container/containerd/config_v1_test.go @@ -349,6 +349,7 @@ func runtimeMapV1(binary string) map[string]interface{} { "privileged_without_host_devices": false, "options": map[string]interface{}{ "BinaryName": binary, + "Runtime": binary, }, } } @@ -376,6 +377,7 @@ func runcRuntimeConfigMapV1(binary string) map[string]interface{} { "options": map[string]interface{}{ "runc-option": "value", "BinaryName": binary, + "Runtime": binary, }, } } From dad3e855b5c2b62cca645f10d82b36b4bf10e5d3 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Fri, 27 May 2022 16:18:57 +0200 Subject: [PATCH 3/4] Also cleanup v1 default_runtime if BinaryName is set Signed-off-by: Evan Lezar --- tools/container/containerd/config_v1.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tools/container/containerd/config_v1.go b/tools/container/containerd/config_v1.go index 1d98776f..f3121c18 100644 --- a/tools/container/containerd/config_v1.go +++ b/tools/container/containerd/config_v1.go @@ -84,6 +84,14 @@ func (config *configV1) Revert(o *options) error { } } } + if runtime, ok := config.GetPath(append(defaultRuntimeOptionsPath, "BinaryName")).(string); ok { + for _, runtimeBinary := range o.getRuntimeBinaries() { + if path.Base(runtimeBinary) == path.Base(runtime) { + config.DeletePath(append(defaultRuntimeOptionsPath, "BinaryName")) + break + } + } + } if options, ok := config.GetPath(defaultRuntimeOptionsPath).(*toml.Tree); ok { if len(options.Keys()) == 0 { From 530d66b5c7e9882965ea383f93d9c48d3c46fc21 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Fri, 27 May 2022 16:20:06 +0200 Subject: [PATCH 4/4] Also set default_runtime.options.BinaryName Signed-off-by: Evan Lezar --- tools/container/containerd/config_v1.go | 1 + tools/container/containerd/config_v1_test.go | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/tools/container/containerd/config_v1.go b/tools/container/containerd/config_v1.go index f3121c18..e744cd7f 100644 --- a/tools/container/containerd/config_v1.go +++ b/tools/container/containerd/config_v1.go @@ -68,6 +68,7 @@ func (config *configV1) Update(o *options) error { log.Warnf("Setting default_runtime is deprecated") defaultRuntimePath := append(config.containerdPath(), "default_runtime") config.initRuntime(defaultRuntimePath, o.runtimeType, "Runtime", runtimeBinary) + config.initRuntime(defaultRuntimePath, o.runtimeType, "BinaryName", runtimeBinary) } return nil } diff --git a/tools/container/containerd/config_v1_test.go b/tools/container/containerd/config_v1_test.go index 0b69f5aa..993d4f1d 100644 --- a/tools/container/containerd/config_v1_test.go +++ b/tools/container/containerd/config_v1_test.go @@ -336,7 +336,8 @@ func defaultRuntimeV1(binary string) map[string]interface{} { "runtime_engine": "", "privileged_without_host_devices": false, "options": map[string]interface{}{ - "Runtime": binary, + "BinaryName": binary, + "Runtime": binary, }, } }