-
Notifications
You must be signed in to change notification settings - Fork 439
Add support for drop-in config files in a container #1303
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
Conversation
cd51b1e to
71a4612
Compare
This change adds support for drop-in files in a container. Here the user needs to set RUNTIME_DROP_IN_CONFIG_HOST_PATH in addition to RUNTIME_DROP_IN_CONFIG. In this case when updating the top-level containerd config, the imports will use the host path instead of the container path. Signed-off-by: Evan Lezar <[email protected]>
71a4612 to
0f11fb2
Compare
Pull Request Test Coverage Report for Build 17861099938Details
💛 - Coveralls |
There was a problem hiding this 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 adds support for drop-in configuration files when running in a container environment by allowing users to specify both container and host paths for drop-in configs. This enables correct path mapping when updating top-level containerd configurations that need to reference host-relative paths.
- Introduces a new
RUNTIME_DROP_IN_CONFIG_HOST_PATHenvironment variable and corresponding CLI flag - Adds path mapping functionality to translate container paths to host paths in configuration imports
- Updates the containerd config builder to accept and use container-to-host path mappings
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/config/engine/containerd/option.go | Adds new builder field and option function for container-to-host path mapping |
| pkg/config/engine/containerd/containerd.go | Updates config creation to pass path mapping to NewConfigWithDropIn |
| pkg/config/engine/containerd/config_drop_in.go | Implements path translation logic for import patterns and adds host path mapping methods |
| cmd/nvidia-ctk-installer/container/runtime/runtime.go | Adds new CLI flag for drop-in config host path |
| cmd/nvidia-ctk-installer/container/runtime/containerd/containerd.go | Integrates path mapping into runtime config creation logic |
| cmd/nvidia-ctk-installer/container/runtime/containerd/config_test.go | Adds test case to verify host path mapping functionality |
| cmd/nvidia-ctk-installer/container/container.go | Adds DropInConfigHostPath field to Options struct |
| func (c *topLevelConfig) asHostPath(path string) string { | ||
| if c.containerToHostPathMap == nil { | ||
| return path | ||
| } | ||
| if hostPath, ok := c.containerToHostPathMap[path]; ok { | ||
| return hostPath | ||
| } | ||
| return path | ||
| } |
Copilot
AI
Sep 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This function performs exact path matching only. Consider if prefix matching would be more robust for nested paths within mapped directories, or document that only exact directory matches are supported.
ArangoGutierrez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass looks good, or maybe my eyes are tired.
LGTM
I'll give the next round early on Monday
ArangoGutierrez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get this in so we can test it on NVIDIA/gpu-operator#1710
This change adds support for drop-in files in a container. Here the user needs to set
RUNTIME_DROP_IN_CONFIG_HOST_PATHin addition toRUNTIME_DROP_IN_CONFIG. In this case when updating the top-level containerd config, the imports will use the host path instead of the container path.