Cleanup default runtime in runtime config when setAsDefault=false

[cdesiniotis]

This commit updates the AddRuntime() implementation for containerd and cri-o to clear the default runtime field if the runtime we are adding is currently set as the default AND setAsDefault=false.

[coveralls]

Pull Request Test Coverage Report for Build 17224664637

Details

• 10 of 10 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.07%) to 35.819%

---

Totals
Change from base Build 17110221655:	0.07%
Covered Lines:	4669
Relevant Lines:	13035

---

💛 - Coveralls

[Copilot]

Pull Request Overview

This PR updates the AddRuntime() implementation for containerd and cri-o to properly handle default runtime configuration when setAsDefault=false. The change ensures that if a runtime being added is currently set as the default runtime AND setAsDefault=false, the default runtime field is cleared from the configuration.

• Added logic to clear default runtime configuration when setAsDefault=false and the runtime matches the current default
• Enhanced test coverage with new test cases for both containerd and cri-o configurations

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
pkg/config/engine/crio/crio.go	Added else clause to clear default_runtime when setAsDefault=false and runtime matches current default
pkg/config/engine/containerd/config.go	Added else clause to clear default_runtime_name when setAsDefault=false and runtime matches current default
pkg/config/engine/crio/crio_test.go	Added test cases for setting runtime as default and clearing default when setAsDefault=false
pkg/config/engine/containerd/config_test.go	Added test cases for setting runtime as default and clearing default when setAsDefault=false

[elezar]

What about the following to clean up intendation a bit:

Suggested change

	if defaultRuntime, ok := config.GetPath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "default_runtime_name"}).(string); ok {
	if defaultRuntime == name {
	config.DeletePath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "default_runtime_name"})
	}
	}
	if defaultRuntime, ok := config.GetPath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "default_runtime_name"}).(string); ok && defaultRuntime == name {
	config.DeletePath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "default_runtime_name"})
	}

alternatively, it may be more readable if we do:

Suggested change

	if defaultRuntime, ok := config.GetPath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "default_runtime_name"}).(string); ok {
	if defaultRuntime == name {
	config.DeletePath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "default_runtime_name"})
	}
	}
	defaultRuntime, ok := config.GetPath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "default_runtime_name"}).(string)
	if ok && defaultRuntime == name {
	config.DeletePath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "default_runtime_name"})
	}

which also decreases the nesting but makes the conditional easier to read.

[tariq1890]

I prefer the latter suggestion; definitely more readable

[cdesiniotis]

I agree the latter suggestion is more readable. I have updated this now.

[elezar]

I didn't check if it's covered explicilty, but what about the case where default_runtime_name exists but is not set to test?

[cdesiniotis]

Yes, I believe this is covered in a prior test case on line 117 with description: options from default runtime are imported. For this test case, the original config has default_runtime_name: default and the setAsDefault setting is configured as false.

[elezar]

Thanks @cdesiniotis.

I have proposed a minor cleanup for readability (and a possibly missed test case).

The other question I have is whether we should backport this?

[elezar]

@cdesiniotis I am giving my approval on this pending my comments being considered / addressed.