[no-relnote] Rename Config to TopLevelConfigPath #1301
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR implements drop-in configuration file support for NVIDIA Container Toolkit, enabling containerized environments to write NVIDIA-specific runtime configurations to separate drop-in files instead of modifying main configuration files directly.
- Adds drop-in config file support for containerd and CRI-O container runtimes
- Introduces a new
engine.Configabstraction that separates source and destination configurations - Updates test cases to validate both top-level and drop-in configuration file handling
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/config/engine/engine.go | Introduces defaultRuntimesGetter interface to narrow function dependencies |
| pkg/config/engine/config.go | New abstraction for source/destination config separation |
| pkg/config/engine/containerd/containerd.go | Implements drop-in config support with import management |
| pkg/config/engine/containerd/config_drop_in.go | New drop-in config wrapper with import path management |
| pkg/config/engine/crio/crio.go | Updates CRI-O config to use source/destination pattern |
| cmd/nvidia-ctk-installer/container/runtime/runtime.go | Adds drop-in config path validation and default values |
| Multiple test files | Updated test expectations for drop-in configuration behavior |
Comment on lines
73
to
74
| // TODO: Only do this if we've actually modified the config. | ||
| if err := c.topLevelConfig.flush(); err != nil { |
There was a problem hiding this comment.
The TODO comment indicates that the top-level config is always flushed, even when unchanged. This could cause unnecessary file I/O operations. Consider implementing a change tracking mechanism to only flush when the config has been modified.
| opts.ExecutablePath = "" | ||
| } | ||
| if opts.DropInConfig != "" && opts.DropInConfig != runtimeSpecificDefault { | ||
| logger.Warningf("Ignorining drop-in-config=%q flag for %v", opts.DropInConfig, opts.RuntimeName) |
There was a problem hiding this comment.
There's a typo in the warning message. 'Ignorining' should be 'Ignoring'.
Suggested change
| logger.Warningf("Ignorining drop-in-config=%q flag for %v", opts.DropInConfig, opts.RuntimeName) | |
| logger.Warningf("Ignoring drop-in-config=%q flag for %v", opts.DropInConfig, opts.RuntimeName) |
elezar
force-pushed
the
dropins_ctk_installer_defaults
branch
from
531c96e to
bf68dc3
Compare
elezar
force-pushed
the
dropins_ctk_installer_defaults
branch
from
bf68dc3 to
58356af
Compare
This change renames the Config struct member to TopLevelConfigPath. This aids readability and better distinguishes between the top-level config file and a drop-in config file. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
elezar
force-pushed
the
dropins_ctk_installer_defaults
branch
from
58356af to
4a56d13
Compare
elezar
changed the title
elezar
approved these changes
Sep 19, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since we have introduced drop-in configs renaming
Configoption to theTopLevelConfigimproves readability.