Skip to content

Conversation

elezar
Copy link
Member

@elezar elezar commented Aug 19, 2025

By default container extracts the current config by running the containerd config dump command and if that fails we read the existing config file.

This change allows ordering of these sources to be defined as we do for the nvidia-ctk runtime configure command.

See #1222 where setting RUNTIME_CONFIG_SOURCES=file or RUNTIME_CONFIG_SOURCES=file,command should allow the current config to be preferred over the containerd config dump command.

By default container extracts the current config by running
the `containerd config dump` command and if that fails
we read the existing config file.

This change allows ordering of these sources to be defined as
we do for the nvidia-ctk runtime configure command.

Signed-off-by: Evan Lezar <[email protected]>
@elezar elezar added the bug Issue/PR to expose/discuss/fix a bug label Aug 19, 2025
@coveralls
Copy link

Pull Request Test Coverage Report for Build 17078747183

Details

  • 0 of 18 (0.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.04%) to 35.494%

Changes Missing Coverage Covered Lines Changed/Added Lines %
cmd/nvidia-ctk-installer/container/runtime/containerd/containerd.go 0 9 0.0%
cmd/nvidia-ctk-installer/container/runtime/runtime.go 0 9 0.0%
Totals Coverage Status
Change from base Build 17078280554: -0.04%
Covered Lines: 4583
Relevant Lines: 12912

💛 - Coveralls

@elezar elezar added must-backport The changes in PR need to be backported to at least one stable release branch. and removed bug Issue/PR to expose/discuss/fix a bug labels Aug 19, 2025
@elezar elezar requested review from klueska and cdesiniotis August 19, 2025 18:46
switch source {
case "file":
loaders = append(loaders, toml.FromFile(o.Config))
case "command":
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to self: Since command here implies containerd config dump command we may want to make this more expressive. For now I have used the values that we support in the nvidia-ctk runtime configure command:

&cli.StringFlag{
Name: "config-source",
Usage: "the source to retrieve the container runtime configuration; one of [command, file]\"",
Destination: &config.configSource,
Value: defaultConfigSource,
},

Comment on lines +106 to +110
&cli.StringSliceFlag{
Name: "config-source",
Usage: "specify the config sources",
Destination: &opts.ConfigSources,
Sources: cli.EnvVars("RUNTIME_CONFIG_SOURCES"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Flag to be called config-source-order (or similar)

specify the order of config sources

As the user can do --config-source command,file or --config-source file,command

same for the ENV VAR RUNTIME_CONFIG_SOURCES_ORDER

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't agree that the flag name should be changed. The flag specifies the sources and the ordering is implied by the order in which the sources are specified. Valid values for this argument are currently:

  • ""
  • "file"
  • "command"
  • "command,file" (Default)
  • "file,command"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then let's expand the Usage to be more explicit, "specify the config sources [file, command]" as nvidia-ctk runtime configure has a flag with the same name but this one is to point to a path. We should provide guidance to the user via Usage with at least the valid options or the default behav of this flag if not set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
must-backport The changes in PR need to be backported to at least one stable release branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants