[no-relnote] add Dropins flag to nvidia-ctk cmd #1300

ArangoGutierrez · 2025-09-19T10:14:43Z

Builds on #1280

Copilot

Pull Request Overview

This PR adds support for drop-in configuration files to the nvidia-ctk command by introducing a --drop-in-config flag. This allows NVIDIA-specific runtime configurations to be saved to separate files instead of modifying the main configuration files.

Adds DropInConfig field to container runtime options and related CLI flags
Refactors containerd and crio config builders to use separate source and destination configs
Implements drop-in config support for containerd with import management

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pkg/config/engine/engine.go	Changes function signatures to use more specific interface instead of generic Interface
pkg/config/engine/crio/option.go	Renames path field to topLevelConfigPath for clarity
pkg/config/engine/crio/crio.go	Updates config structure to support source/destination pattern
pkg/config/engine/containerd/option.go	Renames path field to topLevelConfigPath for clarity
pkg/config/engine/containerd/containerd.go	Implements drop-in config support with source/destination pattern
pkg/config/engine/containerd/config_drop_in.go	New file implementing drop-in config functionality for containerd
pkg/config/engine/containerd/config.go	Renames function for better clarity
pkg/config/engine/config.go	New file defining the Config struct for source/destination pattern
cmd/nvidia-ctk/runtime/configure/configure.go	Adds drop-in-config flag and updates function calls
cmd/nvidia-ctk-installer/container/runtime/runtime.go	Adds drop-in config validation and flags
cmd/nvidia-ctk-installer/container/container.go	Updates flush logic to use drop-in config path when specified
Test files	Updates test expectations for new drop-in config behavior

pkg/config/engine/containerd/config_drop_in.go

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

coveralls · 2025-09-19T10:17:04Z

Pull Request Test Coverage Report for Build 18010993246

Details

14 of 37 (37.84%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.9%) to 37.18%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
cmd/nvidia-ctk/runtime/configure/configure.go	10	18	55.56%
pkg/config/engine/containerd/config_drop_in.go	3	18	16.67%

Totals
Change from base Build 17981864462:	0.9%
Covered Lines:	4958
Relevant Lines:	13335

💛 - Coveralls

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

Copilot

Pull Request Overview

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

Copilot · 2025-09-19T11:49:55Z

pkg/config/engine/containerd/containerd.go

+		topLevelConfig := &Config{
+			Tree: func() *toml.Tree {
+				t, _ := toml.FromFile(b.topLevelConfigPath).Load()
+				return t
+			}(),
+			configOptions: sourceConfigOptions,
+		}


Error from toml.FromFile(b.topLevelConfigPath).Load() is being silently ignored with _. This could lead to unexpected behavior if the file loading fails. The error should be handled appropriately.

Copilot · 2025-09-19T11:49:55Z

pkg/config/engine/containerd/config_drop_in.go

+	// TODO: Only do this if we've actually modified the config.
+	if err := c.topLevelConfig.flush(); err != nil {
+		return 0, fmt.Errorf("failed to save top-level config: %w", err)
+	}


The TODO comment indicates that the top-level config is being flushed unconditionally, even when no modifications were made. This could result in unnecessary file I/O operations.

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

Copilot · 2025-09-19T11:49:56Z

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

 	}

 	// Apply the runtime-specific config changes.
+	// TODO: Add the runtime-specific DropInConfigs here.


This TODO comment suggests incomplete implementation. The runtime-specific drop-in config logic should be implemented or the TODO should be addressed.

Copilot

Pull Request Overview

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

pkg/config/engine/containerd/config_drop_in.go

Copilot · 2025-09-19T14:16:52Z

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

+		}
+	}
+
+	if !filepath.IsAbs(config.dropInConfigPath) {


This validation will fail when dropInConfigPath is empty for docker runtime, since an empty string is not an absolute path. The validation should only run when dropInConfigPath is not empty.

Suggested change

if !filepath.IsAbs(config.dropInConfigPath) {

if config.dropInConfigPath != "" && !filepath.IsAbs(config.dropInConfigPath) {

elezar · 2025-09-24T13:15:43Z

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

 	defaultCrioConfigFilePath       = "/etc/crio/crio.conf"
 	defaultDockerConfigFilePath     = "/etc/docker/daemon.json"

+	defaultContainerdDropInConfigFilePath = "/etc/containerd/config.d/99-nvidia.toml"


Nit: Did we want to use the /run/nvidia/{{ .something }} here or are we hoping that we could convince Containerd to start honoring this path?

Since this is for interactive usage, I thought it was ok to set it like that, and yes, with the Hope that we can push the community towards this direction in the future.

PR containerd/containerd#12323 was merged, so we can expect FUTURE containerd versions to have "/etc/containerd/conf.d/*.toml" by default

OK. That's fine then. Let's also check the paths we specify in the GPU Operator as part of NVIDIA/gpu-operator#1710 as well.

The GPU Operator is using the right default

https://github.com/NVIDIA/gpu-operator/blob/1dda24966ee5e793a5eb4aa1f7abe477ef410f3c/controllers/object_controls.go#L62-L63

DefaultContainerdDropInConfigFile = "/etc/containerd/conf.d/99-nvidia.toml" // DefaultContainerdSocketFile indicates default containerd socket file

elezar · 2025-09-24T13:17:17Z

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

 		}
 	}

+	if config.dropInConfigPath == "" && (config.runtime == "containerd" || config.runtime == "crio") {


Why are we checking the runtime here AND in the switch statement?

Suggested change

if config.dropInConfigPath == "" && (config.runtime == "containerd" || config.runtime == "crio") {

if config.dropInConfigPath == "" {

suggestion adopted

elezar · 2025-09-24T13:19:59Z

pkg/config/engine/containerd/config_drop_in.go

-		return fmt.Errorf("failed to flush config to %q: %w", c.path, err)
+func (c *topLevelConfig) Save(dropInPath string) (int64, error) {
+	saveToPath := c.path
+	if dropInPath == "" {


We may want toa dd a comment indicating that the path "" indicates that we're writing to STDOUT.

comment added

elezar · 2025-09-24T13:22:37Z

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

+		}
+	}
+
+	if config.dropInConfigPath != "" && !filepath.IsAbs(config.dropInConfigPath) {


Did we also want to raise an error if docker specifies a drop-in config path?

Error check added for the mentioned scenario

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>

pkg/config/engine/containerd/config_drop_in.go

third_party/libnvidia-container

Signed-off-by: Evan Lezar <[email protected]>

ArangoGutierrez requested review from elezar and Copilot September 19, 2025 10:14

ArangoGutierrez self-assigned this Sep 19, 2025

Copilot AI reviewed Sep 19, 2025

View reviewed changes

pkg/config/engine/containerd/config_drop_in.go Outdated Show resolved Hide resolved

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

elezar reviewed Sep 19, 2025

View reviewed changes

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

elezar reviewed Sep 19, 2025

View reviewed changes

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

elezar added this to the v1.18.0 milestone Sep 19, 2025

elezar linked an issue Sep 19, 2025 that may be closed by this pull request

Use drop-in files to configure containerd or cri-o #1260

Closed

ArangoGutierrez force-pushed the dropins_ctk_cmd branch 2 times, most recently from e7ef820 to 8738572 Compare September 19, 2025 11:47

ArangoGutierrez requested review from elezar and Copilot September 19, 2025 11:48

Copilot AI reviewed Sep 19, 2025

View reviewed changes

elezar force-pushed the dropins_ctk_cmd branch from 8738572 to 9dc5067 Compare September 19, 2025 12:50

ArangoGutierrez marked this pull request as ready for review September 19, 2025 12:51

ArangoGutierrez force-pushed the dropins_ctk_cmd branch from 9dc5067 to 995866e Compare September 19, 2025 13:19

ArangoGutierrez requested a review from Copilot September 19, 2025 14:16

Copilot AI reviewed Sep 19, 2025

View reviewed changes

elezar reviewed Sep 24, 2025

View reviewed changes

ArangoGutierrez added 5 commits September 24, 2025 15:29

[no-relnote] Add drop-in-config flag to nvidia-ctk cmd

b322509

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>

[no-relnote] fix typo at config_drop_in.go

9aada78

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>

[no-relnote] drop-in-config must be absolute path

8ef4fb2

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>

[no-relnote] rename flush to save

7386f78

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>

[no-relnote] check if dropin path is absolute

e98dc0a

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>

ArangoGutierrez force-pushed the dropins_ctk_cmd branch from df47315 to 8fec2e6 Compare September 24, 2025 13:30

ArangoGutierrez requested a review from elezar September 24, 2025 13:31

ArangoGutierrez force-pushed the dropins_ctk_cmd branch 4 times, most recently from cce3ebe to b41bd12 Compare September 24, 2025 13:52

[no-relnote] add unit tests for cmd/nvidia-ctk/runtime/configure

a6e6d1a

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>

ArangoGutierrez force-pushed the dropins_ctk_cmd branch from b41bd12 to a6e6d1a Compare September 24, 2025 14:53

ArangoGutierrez commented Sep 25, 2025

View reviewed changes

pkg/config/engine/containerd/config_drop_in.go Show resolved Hide resolved

ArangoGutierrez commented Sep 25, 2025

View reviewed changes

third_party/libnvidia-container Show resolved Hide resolved

[no-relnote] Minor comments

d4ff926

Signed-off-by: Evan Lezar <[email protected]>

elezar force-pushed the dropins_ctk_cmd branch from a4ff053 to d4ff926 Compare September 25, 2025 14:34

elezar approved these changes Sep 25, 2025

View reviewed changes

elezar merged commit c51b2ea into NVIDIA:main Sep 26, 2025
16 checks passed

	if !filepath.IsAbs(config.dropInConfigPath) {
	if config.dropInConfigPath != "" && !filepath.IsAbs(config.dropInConfigPath) {

	if config.dropInConfigPath == "" && (config.runtime == "containerd" \|\| config.runtime == "crio") {
	if config.dropInConfigPath == "" {

[no-relnote] add Dropins flag to nvidia-ctk cmd #1300

[no-relnote] add Dropins flag to nvidia-ctk cmd #1300

Uh oh!

Conversation

ArangoGutierrez commented Sep 19, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 18010993246

Details

💛 - Coveralls

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Copilot AI Sep 19, 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!

ArangoGutierrez Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Uh oh!

coveralls commented Sep 19, 2025 •

edited

Loading

ArangoGutierrez Sep 25, 2025 •

edited

Loading