Skip to content

[no-relnote] Refactor AddRuntime #1279

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 8, 2025
Merged

[no-relnote] Refactor AddRuntime #1279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 24 additions & 17 deletions pkg/config/engine/containerd/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,29 +28,37 @@ func (c *Config) AddRuntime(name string, path string, setAsDefault bool) error {
if c == nil || c.Tree == nil {
return fmt.Errorf("config is nil")
}
config := *c.Tree

config.Set("version", c.Version)
defaultRuntimeOptions := c.GetDefaultRuntimeOptions()
return c.AddRuntimeWithOptions(name, path, setAsDefault, defaultRuntimeOptions)
}

func (c *Config) GetDefaultRuntimeOptions() interface{} {
runtimeNamesForConfig := engine.GetLowLevelRuntimes(c)
for _, r := range runtimeNamesForConfig {
options := config.GetSubtreeByPath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "runtimes", r})
if options == nil {
continue
options := c.GetSubtreeByPath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "runtimes", r})
if options != nil {
c.Logger.Debugf("Using options from runtime %v: %v", r, options)
return options.Copy()
}
c.Logger.Debugf("using options from runtime %v: %v", r, options)
config.SetPath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "runtimes", name}, options.Copy())
break
}
c.Logger.Warningf("Could not infer options from runtimes %v", runtimeNamesForConfig)
Copy link
Preview

Copilot AI Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The log message capitalization should be consistent. Consider using lowercase 'could' to match the pattern in the original code which used 'could not infer options'.

Suggested change
c.Logger.Warningf("Could not infer options from runtimes %v", runtimeNamesForConfig)
c.Logger.Warningf("could not infer options from runtimes %v", runtimeNamesForConfig)

Copilot uses AI. Check for mistakes.

options, _ := toml.TreeFromMap(map[string]interface{}{
"runtime_type": c.RuntimeType,
"runtime_root": "",
"runtime_engine": "",
"privileged_without_host_devices": false,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if, on this fallback, we should add the SystemdCgroup = true|false based on a function that looks if the system is running systemd.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent of this change is not to add additional behaviour. It is a refactor.

})
return options
}

if config.GetPath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "runtimes", name}) == nil {
c.Logger.Warningf("could not infer options from runtimes %v; using defaults", runtimeNamesForConfig)
config.SetPath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "runtimes", name, "runtime_type"}, c.RuntimeType)
config.SetPath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "runtimes", name, "runtime_root"}, "")
config.SetPath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "runtimes", name, "runtime_engine"}, "")
config.SetPath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "runtimes", name, "privileged_without_host_devices"}, false)
}
func (c *Config) AddRuntimeWithOptions(name string, path string, setAsDefault bool, options interface{}) error {
config := *c.Tree

config.Set("version", c.Version)

if options != nil {
config.SetPath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "runtimes", name}, options)
}
if len(c.ContainerAnnotations) > 0 {
annotations, err := c.getRuntimeAnnotations([]string{"plugins", c.CRIRuntimePluginName, "containerd", "runtimes", name, "container_annotations"})
if err != nil {
Expand All @@ -65,7 +73,6 @@ func (c *Config) AddRuntime(name string, path string, setAsDefault bool) error {
if setAsDefault {
config.SetPath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "default_runtime_name"}, name)
}

*c.Tree = config
return nil
}
Expand Down
69 changes: 23 additions & 46 deletions pkg/config/engine/containerd/config_v1.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,59 +33,36 @@ func (c *ConfigV1) AddRuntime(name string, path string, setAsDefault bool) error
if c == nil || c.Tree == nil {
return fmt.Errorf("config is nil")
}
defaultRuntimeOptions := c.GetDefaultRuntimeOptions()
return c.AddRuntimeWithOptions(name, path, setAsDefault, defaultRuntimeOptions)
}

config := *c.Tree

config.Set("version", int64(1))

runtimeNamesForConfig := engine.GetLowLevelRuntimes(c)
for _, r := range runtimeNamesForConfig {
options := config.GetSubtreeByPath([]string{"plugins", "cri", "containerd", "runtimes", r})
if options == nil {
continue
}
c.Logger.Debugf("using options from runtime %v: %v", r, options)
config.SetPath([]string{"plugins", "cri", "containerd", "runtimes", name}, options.Copy())
break

}

if config.GetPath([]string{"plugins", "cri", "containerd", "runtimes", name}) == nil {
c.Logger.Warningf("could not infer options from runtimes %v; using defaults", runtimeNamesForConfig)
config.SetPath([]string{"plugins", "cri", "containerd", "runtimes", name, "runtime_type"}, c.RuntimeType)
config.SetPath([]string{"plugins", "cri", "containerd", "runtimes", name, "runtime_root"}, "")
config.SetPath([]string{"plugins", "cri", "containerd", "runtimes", name, "runtime_engine"}, "")
config.SetPath([]string{"plugins", "cri", "containerd", "runtimes", name, "privileged_without_host_devices"}, false)
}
func (c *ConfigV1) GetDefaultRuntimeOptions() interface{} {
return (*Config)(c).GetDefaultRuntimeOptions()
}

if len(c.ContainerAnnotations) > 0 {
annotations, err := (*Config)(c).getRuntimeAnnotations([]string{"plugins", "cri", "containerd", "runtimes", name, "container_annotations"})
if err != nil {
return err
}
annotations = append(c.ContainerAnnotations, annotations...)
config.SetPath([]string{"plugins", "cri", "containerd", "runtimes", name, "container_annotations"}, annotations)
func (c *ConfigV1) AddRuntimeWithOptions(name string, path string, setAsDefault bool, options interface{}) error {
if err := (*Config)(c).AddRuntimeWithOptions(name, path, setAsDefault && !c.UseLegacyConfig, options); err != nil {
return err
}

config.SetPath([]string{"plugins", "cri", "containerd", "runtimes", name, "options", "BinaryName"}, path)
config := *c.Tree
config.SetPath([]string{"plugins", "cri", "containerd", "runtimes", name, "options", "Runtime"}, path)
*c.Tree = config

if setAsDefault {
if !c.UseLegacyConfig {
config.SetPath([]string{"plugins", "cri", "containerd", "default_runtime_name"}, name)
} else {
// Note: This is deprecated in containerd 1.4.0 and will be removed in 1.5.0
if config.GetPath([]string{"plugins", "cri", "containerd", "default_runtime"}) == nil {
config.SetPath([]string{"plugins", "cri", "containerd", "default_runtime", "runtime_type"}, c.RuntimeType)
config.SetPath([]string{"plugins", "cri", "containerd", "default_runtime", "runtime_root"}, "")
config.SetPath([]string{"plugins", "cri", "containerd", "default_runtime", "runtime_engine"}, "")
config.SetPath([]string{"plugins", "cri", "containerd", "default_runtime", "privileged_without_host_devices"}, false)
}
config.SetPath([]string{"plugins", "cri", "containerd", "default_runtime", "options", "BinaryName"}, path)
config.SetPath([]string{"plugins", "cri", "containerd", "default_runtime", "options", "Runtime"}, path)
}
if !c.UseLegacyConfig || !setAsDefault {
return nil
}

config = *c.Tree
// Note: This is deprecated in containerd 1.4.0 and will be removed in 1.5.0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we edit the note, now that 1.5.0 is released? and add a warning log.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent of this change is not to add additional behaviour. It is a refactor.

if config.GetPath([]string{"plugins", "cri", "containerd", "default_runtime"}) == nil {
config.SetPath([]string{"plugins", "cri", "containerd", "default_runtime", "runtime_type"}, c.RuntimeType)
config.SetPath([]string{"plugins", "cri", "containerd", "default_runtime", "runtime_root"}, "")
config.SetPath([]string{"plugins", "cri", "containerd", "default_runtime", "runtime_engine"}, "")
config.SetPath([]string{"plugins", "cri", "containerd", "default_runtime", "privileged_without_host_devices"}, false)
}
config.SetPath([]string{"plugins", "cri", "containerd", "default_runtime", "options", "BinaryName"}, path)
config.SetPath([]string{"plugins", "cri", "containerd", "default_runtime", "options", "Runtime"}, path)
*c.Tree = config
return nil
}
Expand Down
31 changes: 21 additions & 10 deletions pkg/config/engine/crio/crio.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,31 +74,42 @@ func New(opts ...Option) (engine.Interface, error) {
return &cfg, nil
}

// AddRuntime adds a new runtime to the crio config
// AddRuntime adds a new runtime to the crio config.
// The runtime options are extracted from the default runtime and the applicable
// settings are overridden.
func (c *Config) AddRuntime(name string, path string, setAsDefault bool) error {
if c == nil {
if c == nil || c.Tree == nil {
return fmt.Errorf("config is nil")
}
defaultRuntimeOptions := c.GetDefaultRuntimeOptions()
return c.AddRuntimeWithOptions(name, path, setAsDefault, defaultRuntimeOptions)
Comment on lines +84 to +85
Copy link
Preview

Copilot AI Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The variable name defaultRuntimeOptions could be shortened to options for better readability, as the context already makes it clear these are default options.

Suggested change
defaultRuntimeOptions := c.GetDefaultRuntimeOptions()
return c.AddRuntimeWithOptions(name, path, setAsDefault, defaultRuntimeOptions)
options := c.GetDefaultRuntimeOptions()
return c.AddRuntimeWithOptions(name, path, setAsDefault, options)

Copilot uses AI. Check for mistakes.

}

config := *c.Tree

func (c *Config) GetDefaultRuntimeOptions() interface{} {
runtimeNamesForConfig := engine.GetLowLevelRuntimes(c)
for _, r := range runtimeNamesForConfig {
if options, ok := config.GetPath([]string{"crio", "runtime", "runtimes", r}).(*toml.Tree); ok {
c.Logger.Debugf("using options from runtime %v: %v", r, options.String())
options, _ = toml.Load(options.String())
config.SetPath([]string{"crio", "runtime", "runtimes", name}, options)
break
options := c.GetSubtreeByPath([]string{"crio", "runtime", "runtimes", r})
if options != nil {
c.Logger.Debugf("Using options from runtime %v: %v", r, options)
Copy link
Preview

Copilot AI Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The log message capitalization should be consistent with other log messages in the codebase. Consider using lowercase 'using' to match the pattern in the original code.

Suggested change
c.Logger.Debugf("Using options from runtime %v: %v", r, options)
c.Logger.Debugf("using options from runtime %v: %v", r, options)

Copilot uses AI. Check for mistakes.

return options.Copy()
}
}
c.Logger.Warningf("Could not infer options from runtimes %v", runtimeNamesForConfig)
return nil
}

func (c *Config) AddRuntimeWithOptions(name string, path string, setAsDefault bool, options interface{}) error {
config := *c.Tree

if options != nil {
config.SetPath([]string{"crio", "runtime", "runtimes", name}, options)
}
config.SetPath([]string{"crio", "runtime", "runtimes", name, "runtime_path"}, path)
config.SetPath([]string{"crio", "runtime", "runtimes", name, "runtime_type"}, "oci")

if setAsDefault {
config.SetPath([]string{"crio", "runtime", "default_runtime"}, name)
}

*c.Tree = config
return nil
}
Expand Down
23 changes: 10 additions & 13 deletions pkg/config/engine/crio/crio_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,20 +127,19 @@ func TestAddRuntime(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
cfg, err := toml.Load(tc.config)
require.NoError(t, err)
expectedConfig, err := toml.Load(tc.expectedConfig)
require.NoError(t, err)

c := &Config{
Logger: logger,
Tree: cfg,
}
c, err := New(
WithLogger(logger),
WithConfigSource(toml.FromString(tc.config)),
)
require.NoError(t, err)

err = c.AddRuntime("test", "/usr/bin/test", tc.setAsDefault)
require.NoError(t, err)

require.EqualValues(t, expectedConfig.String(), cfg.String())
require.EqualValues(t, expectedConfig.String(), c.String())
})
}
}
Expand Down Expand Up @@ -188,14 +187,12 @@ monitor_path = "/usr/libexec/crio/conmon"
}
for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
cfg, err := toml.Load(config)
c, err := New(
WithLogger(logger),
WithConfigSource(toml.FromString(config)),
)
require.NoError(t, err)

c := &Config{
Logger: logger,
Tree: cfg,
}

rc, err := c.GetRuntimeConfig(tc.runtime)
require.Equal(t, tc.expectedError, err)
require.Equal(t, tc.expected, rc.GetBinaryPath())
Expand Down
Loading