fix(vgpu-init): correct source and destination path handling#2018
fix(vgpu-init): correct source and destination path handling#2018haitwang-cloud wants to merge 1 commit into
Conversation
…init.sh Signed-off-by: Tim <tim.wang03@sap.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: haitwang-cloud The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThe docker/vgpu-init.sh script now normalizes SOURCE_DIR and DEST_DIR by stripping trailing slashes, with a fallback to "/" for an empty DEST_DIR. It skips copying ld.so.preload from the source tree and regenerates it at DEST_DIR pointing to DEST_DIR/libvgpu.so. Changesvgpu-init.sh Path Handling Fix
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Script as vgpu-init.sh
participant Source as SOURCE_DIR
participant Dest as DEST_DIR
Script->>Source: Traverse files, compute relative_path
Script->>Script: Skip ld.so.preload from Source
Script->>Dest: Copy remaining files to DEST_DIR/relative_path
Script->>Dest: Write ld.so.preload pointing to DEST_DIR/libvgpu.so
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docker/vgpu-init.sh (1)
31-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueQuote the expansion inside the parameter substitution.
${source_file#$SOURCE_DIR/}treats$SOURCE_DIRas a glob pattern. It's harmless today sinceSOURCE_DIRis a fixed literal path, but quoting is the correct idiom and silences SC2295.♻️ Proposed fix
- relative_path="${source_file#$SOURCE_DIR/}" + relative_path="${source_file#"$SOURCE_DIR"/}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker/vgpu-init.sh` at line 31, The parameter expansion in the relative_path assignment should quote the SOURCE_DIR prefix inside the substitution to avoid treating it as a glob pattern. Update the shell expansion in vgpu-init.sh so the source_file stripping uses the quoted form of SOURCE_DIR within the parameter substitution, keeping the existing logic intact while following the correct shell idiom and silencing SC2295.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docker/vgpu-init.sh`:
- Line 31: The parameter expansion in the relative_path assignment should quote
the SOURCE_DIR prefix inside the substitution to avoid treating it as a glob
pattern. Update the shell expansion in vgpu-init.sh so the source_file stripping
uses the quoted form of SOURCE_DIR within the parameter substitution, keeping
the existing logic intact while following the correct shell idiom and silencing
SC2295.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9cfffe14-e26f-4df9-aed4-34427f6bad95
📒 Files selected for processing (1)
docker/vgpu-init.sh
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates docker/vgpu-init.sh to dynamically regenerate ld.so.preload at runtime based on the actual DEST_DIR path, rather than copying a static version from the source directory. It also strips trailing slashes from the input destination directory to prevent double slashes during path composition. The review feedback suggests further robust path normalization using tr -s '/' to handle potential duplicate slashes in both the destination directory configuration and the generated ld.so.preload file.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| DEST_DIR="${1%/}" | ||
|
|
||
| if [ -z "$DEST_DIR" ]; then | ||
| DEST_DIR="/" | ||
| fi |
There was a problem hiding this comment.
To make the path handling extremely robust against any malformed inputs, duplicate slashes, or trailing slashes passed from Helm values, we can normalize the input path using tr -s '/' before stripping the trailing slash.
| DEST_DIR="${1%/}" | |
| if [ -z "$DEST_DIR" ]; then | |
| DEST_DIR="/" | |
| fi | |
| DEST_DIR=$(echo "$1" | tr -s '/') | |
| DEST_DIR="${DEST_DIR%/}" | |
| if [ -z "$DEST_DIR" ]; then | |
| DEST_DIR="/" | |
| fi |
| # libvgpu.so under DEST_DIR. Treating this file as runtime-generated (rather | ||
| # than a static packaged asset) is what keeps the preload path aligned with | ||
| # whatever runtime directory the chart passes in. | ||
| printf '%s/libvgpu.so\n' "$DEST_DIR" > "$DEST_DIR/ld.so.preload" |
There was a problem hiding this comment.
If DEST_DIR is /, composing the path as "$DEST_DIR/libvgpu.so" will produce a double slash (//libvgpu.so). While the OS kernel generally handles duplicate slashes gracefully, it is much cleaner and more readable to write a fully normalized path to ld.so.preload.
| printf '%s/libvgpu.so\n' "$DEST_DIR" > "$DEST_DIR/ld.so.preload" | |
| preload_path=$(echo "${DEST_DIR}/libvgpu.so" | tr -s '/') | |
| printf '%s\n' "$preload_path" > "$DEST_DIR/ld.so.preload" |
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@haitwang-cloud As the content of |
3 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
When the HAMi vGPU runtime directory is customized away from the default
/usr/local/vgpu(for example, changed to/var/lib/hami/vgpuvia Helm values),docker/vgpu-init.shstill writes anld.so.preloadwhose content points to the old hardcoded path/usr/local/vgpu/libvgpu.so. The dynamic linker fails to preloadlibvgpu.soat the actual mounted location, and vGPU memory/core isolation is silently not enforced.Root cause:
docker/vgpu-init.shtreatsld.so.preloadas a static packaged file and copies it verbatim from/k8s-vgpu/lib/nvidia/ld.so.preload, whose content is hardcoded to/usr/local/vgpu/libvgpu.so. No matter what$DEST_DIRis passed to the script, the resulting preload content never changes.Which issue(s) this PR fixes:
Fixes # #2017
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
NO
Summary by CodeRabbit