fix: drop containerd config mounts when NRI is enabled#2514
Open
frezbo wants to merge 1 commit into
Open
Conversation
frezbo
requested review from
cdesiniotis,
karthikvetrivel,
rahulait,
rajathagasthya,
shivamerla and
tariq1890
as code owners
Author
|
Relates to: NVIDIA/nvidia-container-toolkit#1861 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds NRI-plugin-aware runtime transformation so the toolkit installer avoids mounting/modifying runtime config on read-only hosts (e.g., Talos), and updates unit tests to cover the new behavior.
Changes:
- Add early-return NRI plugin path in
transformForRuntimethat mounts only the NRI socket. - Extract runtime config/socket mounting logic into
transformRuntimeConfigAndSocketMounts. - Extend
TestTransformForRuntimewith per-test ClusterPolicy input and a new “containerd with NRI plugin enabled” case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| controllers/transforms_test.go | Adds a new NRI-enabled containerd test case and allows per-test ClusterPolicy inputs. |
| controllers/object_controls.go | Skips runtime config/socket mounts when NRI plugin is enabled; factors mount logic into a helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cdesiniotis
reviewed
Jun 3, 2026
frezbo
force-pushed
the
fix/drop-containerd-config-mounts-when-nri-enabled
branch
from
7d78a7c to
3383f08
Compare
Contributor
|
/ok to test 3383f08 |
Contributor
|
@tariq1890 requesting your review |
cdesiniotis
reviewed
Jun 4, 2026
frezbo
force-pushed
the
fix/drop-containerd-config-mounts-when-nri-enabled
branch
from
3383f08 to
c3feb2e
Compare
Contributor
|
/ok to test c3feb2e |
tariq1890
reviewed
Jun 4, 2026
tariq1890
reviewed
Jun 4, 2026
Contributor
|
Thank you very much @frezbo for your contribution here! |
When NRI is enabled there's no need to setup containerd config file path mounts as the installer is a no-op. This fixes issues on immutable hosts where the containerd paths are non-writable and prevents the NRI pod from coming up since kubelet tries to create mount-points for those. Part of upstream discusssion at: NVIDIA#2385 Signed-off-by: Noel Georgi <git@frezbo.dev>
frezbo
force-pushed
the
fix/drop-containerd-config-mounts-when-nri-enabled
branch
from
c3feb2e to
543bf7a
Compare
tariq1890
reviewed
Jun 5, 2026
Comment on lines
+1416
to
+1420
| // transformRuntimeConfigAndSocketMounts configures the toolkit container with the | ||
| // mounts and environment required for the toolkit installer to update the container | ||
| // runtime configuration (top-level config, drop-in config, and runtime socket). This | ||
| // is not required when the NRI plugin is enabled, since the installer does not modify | ||
| // the runtime configuration in that mode. |
Contributor
There was a problem hiding this comment.
Let's leave out the parts that are more or less obvious.
Suggested change
| // transformRuntimeConfigAndSocketMounts configures the toolkit container with the | |
| // mounts and environment required for the toolkit installer to update the container | |
| // runtime configuration (top-level config, drop-in config, and runtime socket). This | |
| // is not required when the NRI plugin is enabled, since the installer does not modify | |
| // the runtime configuration in that mode. | |
| // transformRuntimeConfigAndSocketMounts configures the toolkit container with the | |
| // mounts and environment required for the toolkit installer to update the container | |
| // runtime configuration (top-level config, drop-in config, and runtime socket). |
| // hostPath mounts (e.g. /etc/containerd/conf.d with DirectoryOrCreate) break on | ||
| // read-only hosts such as Talos. Only the NRI socket mount (below) is required. | ||
| if config.CDI.IsNRIPluginEnabled() { | ||
| // setup mounts for the runtime NRI socket file |
Contributor
There was a problem hiding this comment.
Why not move L1389-L1401 to a private the same way as you have done in transformRuntimeConfigAndSocketMounts?
| testCases := []struct { | ||
| description string | ||
| runtime gpuv1.Runtime | ||
| clusterPolicy *gpuv1.ClusterPolicySpec |
Contributor
There was a problem hiding this comment.
Instead of modifying the test case struct here, you can add a new test case to the TestTransformToolkit method in object_controls_test.go. The test case struct over there already has the ClusterPolicySpec object as a struct field.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When NRI is enabled there's no need to setup containerd config file path mounts as the installer is a no-op. This fixes issues on immutable hosts where the containerd paths are non-writable and prevents the NRI pod from coming up since kubelet tries to create mount-points for those.
Part of upstream discusssion at: #2385