Skip to content

Add support for specifying the config file path in an environment variable #763

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 24, 2025

Conversation

elezar
Copy link
Member

@elezar elezar commented Oct 30, 2024

This change allows the config file used by the nvidia-container-runtime and nvidia-container-runtime-hook to be explicitly set using an NVIDIA_CTK_CONFIG_FILE_PATH environment variable. This removes the need to inject a -config option when installing the nvidia-contianer-runtime-hook wrapper from the toolkit container and allows the two components to treat this consistently.

This would allow the changes of #700 to be integrated more easily.

@elezar elezar self-assigned this Oct 30, 2024
@elezar elezar force-pushed the allow-config-override-by-envvar branch from 91aadb7 to 3a304b0 Compare June 18, 2025 12:02
elezar added 2 commits June 18, 2025 15:01
This change adds support for explicitly specifying the path
to the config file through an environment variable.
The NVIDIA_CTK_CONFIG_FILE_PATH variable is used for both the nvidia-container-runtime
and nvidia-container-runtime-hook.

Signed-off-by: Evan Lezar <[email protected]>
@elezar elezar force-pushed the allow-config-override-by-envvar branch from 3a304b0 to 77457b4 Compare June 18, 2025 13:03
@coveralls
Copy link

coveralls commented Jun 18, 2025

Pull Request Test Coverage Report for Build 15733658255

Details

  • 36 of 46 (78.26%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 33.635%

Changes Missing Coverage Covered Lines Changed/Added Lines %
cmd/nvidia-ctk-installer/toolkit/installer/directory.go 0 1 0.0%
cmd/nvidia-ctk-installer/toolkit/installer/installer.go 5 7 71.43%
cmd/nvidia-container-runtime-hook/hook_config.go 14 17 82.35%
cmd/nvidia-ctk-installer/toolkit/installer/options.go 0 4 0.0%
Totals Coverage Status
Change from base Build 15731241720: 0.002%
Covered Lines: 4345
Relevant Lines: 12918

💛 - Coveralls

@elezar elezar force-pushed the allow-config-override-by-envvar branch from 77457b4 to 849691d Compare June 18, 2025 13:06
@elezar elezar marked this pull request as ready for review June 18, 2025 21:40
@elezar elezar added this to the v1.18.0 milestone Jun 18, 2025
@ArangoGutierrez ArangoGutierrez requested a review from Copilot June 24, 2025 14:36
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enables overriding the location of the nvidia-container-runtime config file via the NVIDIA_CTK_CONFIG_FILE_PATH environment variable for both the runtime and hook, and refactors related installer APIs to propagate the explicit path.

  • Introduce FilePathOverrideEnvVar and RelativeFilePath constants and update GetConfigFilePath to respect NVIDIA_CTK_CONFIG_FILE_PATH
  • Refactor installer (ToolkitInstaller) to accept and propagate a concrete config file path
  • Update tests and hook code to use the new override logic and remove direct -config flag usage

Reviewed Changes

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

Show a summary per file
File Description
internal/config/config.go Added FilePathOverrideEnvVar/RelativeFilePath and updated GetConfigFilePath logic
internal/config/config_test.go Adjusted tests to cover both root override and file-path override
cmd/nvidia-ctk-installer/toolkit/toolkit.go Pass explicit config path into installToolkitConfig
cmd/nvidia-ctk-installer/toolkit/installer/options.go Renamed toolkitInstaller receiver to ToolkitInstaller in options
cmd/nvidia-ctk-installer/toolkit/installer/libraries.go Updated receiver type for collectLibraries
cmd/nvidia-ctk-installer/toolkit/installer/installer_test.go Switched to ToolkitInstaller in test setup and updated env vars
cmd/nvidia-ctk-installer/toolkit/installer/installer.go Renamed type, made New return *ToolkitInstaller, added ConfigFilePath
cmd/nvidia-ctk-installer/toolkit/installer/executables_test.go Removed Args-related tests following wrapper refactor
cmd/nvidia-ctk-installer/toolkit/installer/executables.go Dropped CLI-arg support, now uses env-var override in wrappers
cmd/nvidia-ctk-installer/toolkit/installer/directory.go Renamed receiver on createDirectory
cmd/nvidia-container-runtime-hook/hook_config.go Simplified loadConfig, now honors flag/env/default path
Comments suppressed due to low confidence (6)

internal/config/config.go:34

  • Add a GoDoc comment for the exported constant FilePathOverrideEnvVar to explain its purpose and usage.
	FilePathOverrideEnvVar = "NVIDIA_CTK_CONFIG_FILE_PATH"

internal/config/config.go:35

  • Add a GoDoc comment for the exported constant RelativeFilePath to clarify that it represents the default relative path to the configuration file.
	RelativeFilePath       = "nvidia-container-runtime/config.toml"

internal/config/config_test.go:28

  • [nitpick] The test name TestGetConfigWithCustomConfig does not clearly reflect that it verifies behavior under the XDG_CONFIG_HOME override. Consider renaming to TestGetConfigWithConfigRootOverride.
func TestGetConfigWithCustomConfig(t *testing.T) {

internal/config/config_test.go:34

  • [nitpick] This comment appears misleading because the test actually writes a debug path into the file contents. Update or remove it to accurately describe what the test is verifying.
	// By default debug is disabled

cmd/nvidia-ctk-installer/toolkit/installer/installer.go:49

  • [nitpick] Changing the signature of New to return *ToolkitInstaller instead of Installer is a breaking API change. Ensure all consumers are updated or consider providing an overload to maintain backward compatibility.
func New(opts ...Option) (*ToolkitInstaller, error) {

cmd/nvidia-container-runtime-hook/hook_config.go:21

  • The previous fallback to loading the config from the driver directory (/run/nvidia/driver/etc) was removed. If that behavior is still required for legacy installs, consider reintroducing the driverPath fallback logic.
func loadConfig() (*config.Config, error) {

if configFilePathOverride := os.Getenv(FilePathOverrideEnvVar); configFilePathOverride != "" {
return configFilePathOverride
}
configRoot := "/etc"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe a newline before configRoot :=...

Copy link
Collaborator

Choose a reason for hiding this comment

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

non blocker

@ArangoGutierrez
Copy link
Collaborator

@elezar I think the PR title should be updated to better reflect the intent of the PR.
Allow config override by envvar kind of tells me that I can override the config itself (like things inside the config), but the PR is just for the config PATH.

@elezar elezar changed the title Allow config override by envvar Allow config file override envvar Jun 24, 2025
@elezar
Copy link
Member Author

elezar commented Jun 24, 2025

@ArangoGutierrez is the change you require the PR title, or are you blocking on the nit?

@elezar elezar changed the title Allow config file override envvar Add support for specifying the config file path in an environment variable Jun 24, 2025
Copy link
Collaborator

@ArangoGutierrez ArangoGutierrez left a comment

Choose a reason for hiding this comment

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

LGTM

@elezar elezar merged commit d1286bc into NVIDIA:main Jun 24, 2025
16 checks passed
@elezar elezar deleted the allow-config-override-by-envvar branch June 24, 2025 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants