Implement Drop-In Lifecycle Support for Containerd and Crio #1272

ArangoGutierrez · 2025-08-27T09:41:51Z

No description provided.

coveralls · 2025-08-27T09:44:08Z

Pull Request Test Coverage Report for Build 17823407768

Details

6 of 188 (3.19%) changed or added relevant lines in 10 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.5%) to 36.229%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/config/engine/containerd/config_v1.go	0	1	0.0%
cmd/nvidia-ctk/runtime/configure/configure.go	0	2	0.0%
pkg/config/engine/containerd/option.go	0	4	0.0%
pkg/config/engine/crio/option.go	0	4	0.0%
cmd/nvidia-ctk-installer/container/runtime/runtime.go	0	6	0.0%
pkg/config/engine/crio/crio.go	3	58	5.17%
pkg/config/engine/containerd/config.go	0	110	0.0%

Totals
Change from base Build 17802483936:	-0.5%
Covered Lines:	4804
Relevant Lines:	13260

💛 - Coveralls

elezar

As discussed offline, I think the approach here is differs from what my first instinct was to more clearly split SOURCE and TARGET / NVIDIA runtimes but to keep the logic pretty much as is. Let us assume that we make the changes as described in #1279, then the switch to drop in files SHOULD reduce to implementing GetDefaultRuntimeOptions to pull these from the SOURCE config, whereas any functions that MODIFY the config should act on the TARGET config.

It may be useful to list the various combinations more clearly in the design doc or some kind of test here (please point it out if it is there).

elezar · 2025-08-27T14:02:12Z

cmd/nvidia-ctk/runtime/configure/configure.go


+	defaultContainerdDropInDir = "/etc/containerd/conf.d"
+	defaultCrioDropInDir       = "/etc/crio/conf.d"
+	defaultDropInFileName      = "99.nvidia.toml"


Suggested change

defaultDropInFileName = "99.nvidia.toml"

defaultDropInFileName = "99-nvidia.toml"

elezar · 2025-08-27T14:09:31Z

cmd/nvidia-ctk/runtime/configure/configure.go

What about the following:

nvidia-ctk runtime configure \ --runtime=containerd \ --config=/etc/containerd/config.toml \ --nvidia-config=/etc/containerd/conf.d/99-nvidia.toml

and

nvidia-ctk runtime configure \ --runtime=crio \ --config=/etc/crio/crio.conf \ --nvidia-config=/etc/crio/conf.d/99-nvidia.toml

the drop in dirs can be inferred as the PARENT dir of the nvidia-config file.

Note that IFF --nvidia-config="" we could trigger the same behaviour as --dry-run in which case no config modifications are made to the specified config files.

Does it make sense to set up tests for the various combinations here and then use that to drive development?

I have implemented this approach. PTAL

elezar · 2025-08-27T14:10:18Z

cmd/nvidia-ctk/runtime/configure/configure.go

+			&cli.StringFlag{
+				Name:        "drop-in-config-path",
+				Usage:       "the path to the drop-in configuration file for the containerd runtime",
+				Destination: &config.dropInConfigPath,
+			},


As a note: Here we use the same option and in the toolkit container logic we only expose a flag for containerd.

elezar · 2025-08-27T14:26:05Z

pkg/config/engine/containerd/config.go

+		return 0, fmt.Errorf("failed to create drop-in directory %s: %w", c.dropInConfigPath, err)
+	}
+
+	return c.NVConfig.Save(path)


I believe this may be saving to the wrong path. Why is this not the drop in file?

elezar · 2025-08-27T14:29:21Z

pkg/config/engine/containerd/config.go

+	}
+
+	// Main config exists, ensure it has imports directive
+	return d.ensureImportsDirective()


Should the only difference between these two cases not be whether the original config is empty or not. Updating the imports should not require different logic.

elezar · 2025-08-27T14:34:50Z

pkg/config/engine/containerd/containerd.go


+	var dropInConfigPath string
+	if b.dropInDir != "" {
+		dropInConfigPath = b.dropInDir


There seems to be an inconsistency here. Is this a directory or a file path?

elezar · 2025-08-27T14:38:14Z

pkg/config/engine/crio/crio.go

 	}

+	var dropInConfigPath string
+	if b.dropInDir != "" {


I would argue that IF the dropIn config path is "" then we should have the same behviour as we currently have.

cmd/nvidia-ctk-installer/container/runtime/containerd/config_v2_test.go

ArangoGutierrez · 2025-09-09T10:01:07Z

Fixes #1260

Copilot

Pull Request Overview

This PR implements drop-in lifecycle support for container runtimes (containerd and CRI-O) by adding support for NVIDIA-specific configuration files that can be managed separately from the main runtime configuration files.

Adds a new WithNvidiaConfig option for both containerd and CRI-O configs to specify a separate NVIDIA configuration file path
Implements specialized save logic that creates NVIDIA-specific config files containing only runtime configurations
Updates the configure command and installer to support the new --nvidia-config flag

Reviewed Changes

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

Show a summary per file

File	Description
pkg/config/engine/crio/option.go	Adds WithNvidiaConfig option to specify NVIDIA-specific config file path
pkg/config/engine/crio/crio.go	Implements NVIDIA config file support with specialized save/remove logic
pkg/config/engine/containerd/option.go	Adds WithNvidiaConfig option for containerd configuration
pkg/config/engine/containerd/containerd.go	Adds nvidiaConfig field to Config struct
pkg/config/engine/containerd/config_v1.go	Updates Save method call to use pointer receiver
pkg/config/engine/containerd/config.go	Implements NVIDIA config file support with import handling for containerd
cmd/nvidia-ctk/runtime/configure/configure.go	Adds --nvidia-config flag and passes it to runtime configs
cmd/nvidia-ctk-installer/container/runtime/runtime.go	Adds nvidia-config flag to installer runtime options
cmd/nvidia-ctk-installer/container/runtime/crio/crio.go	Updates CRI-O installer to use nvidia config option
cmd/nvidia-ctk-installer/container/runtime/containerd/containerd.go	Updates containerd installer to use nvidia config option
cmd/nvidia-ctk-installer/container/container.go	Adds NvidiaConfig field to shared container options

Copilot · 2025-09-09T16:22:48Z

pkg/config/engine/crio/crio.go

+	// If using NVIDIA-specific configuration, handle file cleanup
+	if c.nvidiaConfig != "" {
+		// Check if all NVIDIA runtimes are being removed
+		remainingNvidiaRuntimes := 0
+		if runtimes := c.GetPath([]string{"crio", "runtime", "runtimes"}); runtimes != nil {
+			if runtimesTree, ok := runtimes.(*toml.Tree); ok {
+				for _, runtimeName := range runtimesTree.Keys() {
+					if c.isNvidiaRuntime(runtimeName) && runtimeName != name {
+						remainingNvidiaRuntimes++
+					}
+				}
+			}
+		}


The runtime counting logic iterates through all runtimes every time a runtime is removed. Consider caching the NVIDIA runtime count or using a more efficient approach when removing multiple runtimes in sequence.

Copilot · 2025-09-09T16:22:48Z

pkg/config/engine/containerd/config.go

+	// If using NVIDIA-specific configuration, handle file cleanup
+	if c.nvidiaConfig != "" {
+		// Check if all NVIDIA runtimes are being removed
+		remainingNvidiaRuntimes := 0
+		if runtimes := c.GetPath([]string{"plugins", c.CRIRuntimePluginName, "containerd", "runtimes"}); runtimes != nil {
+			if runtimesTree, ok := runtimes.(*toml.Tree); ok {
+				for _, runtimeName := range runtimesTree.Keys() {
+					if c.isNvidiaRuntime(runtimeName) && runtimeName != name {
+						remainingNvidiaRuntimes++
+					}
+				}
+			}
+		}


This runtime counting logic is duplicated between CRI-O and containerd implementations. Consider extracting this into a shared helper function to reduce code duplication.

Copilot · 2025-09-09T16:22:48Z

pkg/config/engine/containerd/config.go

+	// Update main config with imports directive
+	if err := c.updateMainConfigImports(path); err != nil {
+		// Try to clean up the NVIDIA config file on error
+		os.Remove(c.nvidiaConfig)


The error from os.Remove is silently ignored during cleanup. Consider logging the cleanup failure or handling it appropriately, as this could indicate permission issues or other problems.

Suggested change

os.Remove(c.nvidiaConfig)

if removeErr := os.Remove(c.nvidiaConfig); removeErr != nil {

c.Logger.Errorf("failed to remove NVIDIA config file during cleanup: %v", removeErr)

}

Copilot · 2025-09-09T16:22:49Z

pkg/config/engine/containerd/config.go

+	if imports == nil {
+		mainConfig.Set("imports", []string{importPattern})
+	} else if importsList, ok := imports.([]interface{}); ok {


The imports handling logic has complex type checking with multiple nested conditions. Consider refactoring this into separate helper functions to improve readability and maintainability.

ArangoGutierrez · 2025-09-09T16:22:59Z

As discussed offline, I think the approach here is differs from what my first instinct was to more clearly split SOURCE and TARGET / NVIDIA runtimes but to keep the logic pretty much as is. Let us assume that we make the changes as described in #1279, then the switch to drop in files SHOULD reduce to implementing GetDefaultRuntimeOptions to pull these from the SOURCE config, whereas any functions that MODIFY the config should act on the TARGET config.

It may be useful to list the various combinations more clearly in the design doc or some kind of test here (please point it out if it is there).

This PR now rebases #1279 and builds on the concepts introduced there; now this PR is smaller and focused.
The test bit's are also in an independent PR #1293

cmd/nvidia-ctk/runtime/configure/configure.go

Copilot

Pull Request Overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Copilot · 2025-09-18T09:07:17Z

pkg/config/engine/containerd/config_v1.go

 func (c ConfigV1) Save(path string) (int64, error) {
-	return (Config)(c).Save(path)
+	return (*Config)(&c).Save(path)


This type conversion creates a pointer to a copy of the struct instead of using the existing pointer. It should be return ((*Config)(c)).Save(path) to avoid creating an unnecessary copy.

Copilot · 2025-09-18T09:07:17Z

pkg/config/engine/crio/crio.go

+func (c *Config) isNvidiaRuntime(name string) bool {
+	return name == "nvidia" || name == "nvidia-cdi" || name == "nvidia-legacy"
+}


The hardcoded runtime names are duplicated between containerd and crio implementations. Consider extracting these to a shared constant or configuration to avoid inconsistencies.

Copilot · 2025-09-18T09:07:17Z

pkg/config/engine/containerd/config.go

+// isNvidiaRuntime checks if the runtime name is an NVIDIA runtime
+func (c *Config) isNvidiaRuntime(name string) bool {
+	return name == "nvidia" || name == "nvidia-cdi" || name == "nvidia-legacy"


The hardcoded runtime names are duplicated between containerd and crio implementations. Consider extracting these to a shared constant or configuration to avoid inconsistencies.

Suggested change

// isNvidiaRuntime checks if the runtime name is an NVIDIA runtime

func (c *Config) isNvidiaRuntime(name string) bool {

return name == "nvidia" || name == "nvidia-cdi" || name == "nvidia-legacy"

// nvidiaRuntimeNames contains the recognized NVIDIA runtime names.

var nvidiaRuntimeNames = []string{"nvidia", "nvidia-cdi", "nvidia-legacy"}

// isNvidiaRuntime checks if the runtime name is an NVIDIA runtime

func (c *Config) isNvidiaRuntime(name string) bool {

for _, n := range nvidiaRuntimeNames {

if name == n {

return true

}

}

return false

Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>

elezar · 2025-09-18T10:10:25Z

cmd/nvidia-ctk-installer/container/runtime/runtime.go

+			Name:        "drop-in-config",
+			Usage:       "Path to the NVIDIA-specific config file to create. When specified, runtime configurations are saved to this file instead of modifying the main config file",
+			Destination: &opts.NvidiaConfig,
+			Value:       defaultNVIDIARuntimeConfigFilePath,


Question: How does this work for crio? Do we also have the ability to update the imports there? Is this something that we want to do?

It is possible to point to a diff location than the default one (see https://www.ibm.com/docs/en/zoscp/1.1.0?topic=options-crio-commands-flags#crio-commands__title__4)
--config-dir | Path to the configuration drop-in directory.

So we could offer the same drop-in-config flag on the crio side.
Let me see how that would look like

To be clear. I'm ok for runtime-specific defaults here. We already do this for other settings here:

nvidia-container-toolkit/cmd/nvidia-ctk-installer/container/runtime/runtime.go

Lines 128 to 159 in 470b841

// Apply the runtime-specific config changes.

switch runtime {

case containerd.Name:

if opts.Config == runtimeSpecificDefault {

opts.Config = containerd.DefaultConfig

}

if opts.Socket == runtimeSpecificDefault {

opts.Socket = containerd.DefaultSocket

}

if opts.RestartMode == runtimeSpecificDefault {

opts.RestartMode = containerd.DefaultRestartMode

}

case crio.Name:

if opts.Config == runtimeSpecificDefault {

opts.Config = crio.DefaultConfig

}

if opts.Socket == runtimeSpecificDefault {

opts.Socket = crio.DefaultSocket

}

if opts.RestartMode == runtimeSpecificDefault {

opts.RestartMode = crio.DefaultRestartMode

}

case docker.Name:

if opts.Config == runtimeSpecificDefault {

opts.Config = docker.DefaultConfig

}

if opts.Socket == runtimeSpecificDefault {

opts.Socket = docker.DefaultSocket

}

if opts.RestartMode == runtimeSpecificDefault {

opts.RestartMode = docker.DefaultRestartMode

}

Ok, I think is ok to go with defaults for now

ArangoGutierrez requested review from elezar, tariq1890, cdesiniotis and Copilot August 27, 2025 09:41

ArangoGutierrez self-assigned this Aug 27, 2025

This comment was marked as outdated.

Sign in to view

ArangoGutierrez force-pushed the dropins branch from e7d8b8b to c8c059c Compare August 27, 2025 09:51

elezar reviewed Aug 28, 2025

View reviewed changes

elezar reviewed Aug 29, 2025

View reviewed changes

cmd/nvidia-ctk-installer/container/runtime/containerd/config_v2_test.go Outdated Show resolved Hide resolved

elezar mentioned this pull request Sep 9, 2025

[no-relnote] Refactor cmd/installer/container/containerd unit test #1293

Merged

ArangoGutierrez force-pushed the dropins branch from c8c059c to aafd1c4 Compare September 9, 2025 16:20

ArangoGutierrez requested review from Copilot and elezar September 9, 2025 16:21

Copilot AI reviewed Sep 9, 2025

View reviewed changes

elezar reviewed Sep 18, 2025

View reviewed changes

cmd/nvidia-ctk/runtime/configure/configure.go Outdated Show resolved Hide resolved

ArangoGutierrez force-pushed the dropins branch from aafd1c4 to 9a43829 Compare September 18, 2025 08:53

ArangoGutierrez requested a review from Copilot September 18, 2025 09:05

Copilot AI reviewed Sep 18, 2025

View reviewed changes

Implement Drop-In Lifecycle Support for Containerd Configuration

516cac1

Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>

ArangoGutierrez force-pushed the dropins branch from 9a43829 to 516cac1 Compare September 18, 2025 09:09

elezar reviewed Sep 18, 2025

View reviewed changes

ArangoGutierrez closed this Sep 19, 2025

elezar mentioned this pull request Sep 19, 2025

Add drop-in file support for containerd and crio #1280

Merged

	defaultDropInFileName = "99.nvidia.toml"
	defaultDropInFileName = "99-nvidia.toml"

-		os.Remove(c.nvidiaConfig)
+		if removeErr := os.Remove(c.nvidiaConfig); removeErr != nil {
+			c.Logger.Errorf("failed to remove NVIDIA config file during cleanup: %v", removeErr)
+		}

-// isNvidiaRuntime checks if the runtime name is an NVIDIA runtime
-func (c *Config) isNvidiaRuntime(name string) bool {
-	return name == "nvidia" || name == "nvidia-cdi" || name == "nvidia-legacy"
+// nvidiaRuntimeNames contains the recognized NVIDIA runtime names.
+var nvidiaRuntimeNames = []string{"nvidia", "nvidia-cdi", "nvidia-legacy"}
+// isNvidiaRuntime checks if the runtime name is an NVIDIA runtime
+func (c *Config) isNvidiaRuntime(name string) bool {
+	for _, n := range nvidiaRuntimeNames {
+		if name == n {
+			return true
+		}
+	}
+	return false

	// Apply the runtime-specific config changes.
	switch runtime {
	case containerd.Name:
	if opts.Config == runtimeSpecificDefault {
	opts.Config = containerd.DefaultConfig
	}
	if opts.Socket == runtimeSpecificDefault {
	opts.Socket = containerd.DefaultSocket
	}
	if opts.RestartMode == runtimeSpecificDefault {
	opts.RestartMode = containerd.DefaultRestartMode
	}
	case crio.Name:
	if opts.Config == runtimeSpecificDefault {
	opts.Config = crio.DefaultConfig
	}
	if opts.Socket == runtimeSpecificDefault {
	opts.Socket = crio.DefaultSocket
	}
	if opts.RestartMode == runtimeSpecificDefault {
	opts.RestartMode = crio.DefaultRestartMode
	}
	case docker.Name:
	if opts.Config == runtimeSpecificDefault {
	opts.Config = docker.DefaultConfig
	}
	if opts.Socket == runtimeSpecificDefault {
	opts.Socket = docker.DefaultSocket
	}
	if opts.RestartMode == runtimeSpecificDefault {
	opts.RestartMode = docker.DefaultRestartMode
	}

Implement Drop-In Lifecycle Support for Containerd and Crio #1272

Implement Drop-In Lifecycle Support for Containerd and Crio #1272

Uh oh!

Conversation

ArangoGutierrez commented Aug 27, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

coveralls commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 17823407768

Details

💛 - Coveralls

Uh oh!

elezar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ArangoGutierrez commented Sep 9, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

ArangoGutierrez commented Sep 9, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Aug 27, 2025 •

edited

Loading