Allow update-ldcache hook to work when pivot-root is not supported #1174
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
Pull Request Test Coverage Report for Build 16070359030Details
💛 - Coveralls |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for containers using --no-pivot by detecting when pivot_root is unavailable and falling back to a move-mount (msMoveRoot) strategy; it also refactors argument parsing and the root mount logic.
- Implement
msMoveRootinldconfig_linux.goand stub it on non-Linux platforms. - Introduce a
--no-pivotflag,noPivotRootdetection, and switch in the pivot logic. - Refactor
mountProcto useSecureJoinandutils.MkdirAllInRoot, and streamline handler argument parsing.
Reviewed Changes
Copilot reviewed 6 out of 104 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/ldconfig/ldconfig_other.go | Add msMoveRoot stub for non-Linux builds |
| internal/ldconfig/ldconfig_linux.go | Implement msMoveRoot, add mount masking, chroot helper, and noPivotRoot logic; refactor mountProc |
| internal/ldconfig/ldconfig.go | Add --no-pivot flag in runner, new NewFromArgs constructor, and use l.directories fields |
| go.mod | Add dependencies: github.com/moby/sys/mountinfo, github.com/prometheus/procfs |
| cmd/nvidia-cdi-hook/update-ldcache/update-ldcache.go | Replace manual args unpacking with NewFromArgs and simplified call |
| cmd/nvidia-cdi-hook/create-soname-symlinks/soname-symlinks.go | Replace manual args unpacking with NewFromArgs and simplified call |
Comments suppressed due to low confidence (3)
internal/ldconfig/ldconfig_linux.go:104
- [nitpick] The doc comment for
msMoveRootis incomplete. Consider expanding it to explain what the function does (e.g. "msMoveRoot moves the root filesystem in place when pivot_root isn't available").
// msMoveRoot is used
internal/ldconfig/ldconfig_linux.go:255
- [nitpick] The new
noPivotRootdetection logic andmsMoveRootfallback are not covered by tests. Consider adding unit tests for different rootfs types and the fallback path to ensure behavior across environments.
if err := utils.MkdirAllInRoot(newroot, target, 0755); err != nil {
internal/ldconfig/ldconfig_linux.go:145
- The code references
unix.Mount,unix.Unmount, anderrors.Iswithout importinggolang.org/x/sys/unixanderrors. Add the missing imports to ensure compilation.
if err := unix.Mount("", p, "", unix.MS_SLAVE|unix.MS_REC, ""); err != nil {
internal/ldconfig/ldconfig_linux.go
Outdated
|
|
||
| if err := os.MkdirAll(target, 0755); err != nil { | ||
| return fmt.Errorf("error creating directory: %w", err) | ||
| target, err := securejoin.SecureJoin(newroot, "/proc") |
There was a problem hiding this comment.
Using an absolute path ("/proc") with SecureJoin will discard newroot. Use a relative subpath like "proc" to ensure the directory is created under newroot.
Suggested change
| target, err := securejoin.SecureJoin(newroot, "/proc") | |
| target, err := securejoin.SecureJoin(newroot, "proc") |
Copilot uses AI. Check for mistakes.
ArangoGutierrez
requested changes
Jul 2, 2025
internal/ldconfig/ldconfig.go
Outdated
| l := &Ldconfig{ | ||
| ldconfigPath: ldconfigPath, | ||
| inRoot: inRoot, | ||
| // NewFromRunnerArgs creates an Ldconfig struct from the args passed to the Cmd |
There was a problem hiding this comment.
Suggested change
| // NewFromRunnerArgs creates an Ldconfig struct from the args passed to the Cmd | |
| // NewFromArgs creates an Ldconfig struct from the args passed to the cmd |
Or edit func name
This change updates the proc mount when updating ldconfig to use secure join to align with the other functions. Signed-off-by: Evan Lezar <[email protected]>
Signed-off-by: Evan Lezar <[email protected]>
This change updates the update-ldcache logic to use an alternative to pivot-root when this is not supported. This includes cases where the root filesystem is in a ramfs (e.g. when running from the kata-agent). Signed-off-by: Evan Lezar <[email protected]>
Signed-off-by: Evan Lezar <[email protected]>
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.
The changes made to run the update-ldcache hook in an isolated namespace were implemented using a pivot_root. This is, however, not supported in all use cases -- most notably in kata-containers where runc is typically invoked with the --no-pivot option.
This change attempts to detect when pivot_root is not supported and use the same move mount operation as implemented by runc in this case.