Fix dockerfile git worktree - #2171
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shellyco-code 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 |
📝 WalkthroughWalkthroughDocker build entry points now support Git worktrees by relocating ChangesWorktree-safe Docker builds
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@pkg/device-plugin/nvidiadevice/nvinternal/rm/health_checkhealth_test.go`:
- Around line 29-33: Update the nvmlResourceManager initialization in the
health-check test to set healthCheckXIDs using getHealthCheckXids("all", "")
instead of the nonexistent resourceManager.disableHealthChecks field, preserving
the early-return behavior before accessing the nil NVML interface.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 19d9c30d-9e00-484b-b23a-a67b1ab24d25
📒 Files selected for processing (9)
docker/Dockerfiledocker/Dockerfile.hamicorepkg/device-plugin/nvidiadevice/nvinternal/plugin/factory.gopkg/device-plugin/nvidiadevice/nvinternal/rm/health.gopkg/device-plugin/nvidiadevice/nvinternal/rm/health_checkhealth_test.gopkg/device-plugin/nvidiadevice/nvinternal/rm/health_test.gopkg/device-plugin/nvidiadevice/nvinternal/rm/nvml_manager.gopkg/device-plugin/nvidiadevice/nvinternal/rm/rm.gopkg/device-plugin/nvidiadevice/nvinternal/rm/tegra_manager.go
💤 Files with no reviewable changes (2)
- docker/Dockerfile
- docker/Dockerfile.hamicore
| r := &nvmlResourceManager{ | ||
| resourceManager: resourceManager{ | ||
| disableHealthChecks: "all", | ||
| }, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Initialize the declared XID-selection field.
Line 31 references nonexistent resourceManager.disableHealthChecks, so this test does not compile. Set healthCheckXIDs: getHealthCheckXids("all", "") instead; that also preserves the intended early return before using the nil NVML interface.
Proposed fix
resourceManager: resourceManager{
- disableHealthChecks: "all",
+ healthCheckXIDs: getHealthCheckXids("all", ""),
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| r := &nvmlResourceManager{ | |
| resourceManager: resourceManager{ | |
| disableHealthChecks: "all", | |
| }, | |
| } | |
| r := &nvmlResourceManager{ | |
| resourceManager: resourceManager{ | |
| healthCheckXIDs: getHealthCheckXids("all", ""), | |
| }, | |
| } |
🤖 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 `@pkg/device-plugin/nvidiadevice/nvinternal/rm/health_checkhealth_test.go`
around lines 29 - 33, Update the nvmlResourceManager initialization in the
health-check test to set healthCheckXIDs using getHealthCheckXids("all", "")
instead of the nonexistent resourceManager.disableHealthChecks field, preserving
the early-return behavior before accessing the nil NVML interface.
There was a problem hiding this comment.
this branch carries #2151 too, c4847d0 and 3cd40b7 are both in here and only aec9056 is the docker fix. worth rebasing onto master so this pr is one commit.
related: 3cd40b7 is titled "address PR feedback for health check config", which is the commit u said u pushed to #2151 on the 28th. it landed here instead, #2151 is still on c4847d0 and still fails lint on the unused os import.
| RUN rm -rf /libvgpu/.git && echo "gitdir: /libvgpu-git" > /libvgpu/.git | ||
| WORKDIR /libvgpu | ||
| RUN rm -rf /libvgpu/build | ||
| RUN bash ./build.sh |
There was a problem hiding this comment.
dropping the copy means git describe fails in every build, not just worktrees, so CI_COMMIT_SHA falls back to unknown for released images too. #2042 tried to keep it by resolving the real gitdir, what does libvgpu report as its version after this?
| }() | ||
|
|
||
| return rm.NewNVMLResourceManagers(o.infolib, o.nvmllib, o.devicelib, o.config.Config) | ||
| return rm.NewNVMLResourceManagers(o.infolib, o.nvmllib, o.devicelib, o.config.Config, disableHealthChecks, enableHealthChecks) |
There was a problem hiding this comment.
Please remove changes that are unrelated to the pr title
Signed-off-by: shellyco-code <shellychahar57@gmail.com>
aec9056 to
40e2192
Compare
|
@mesutoezdil ,@DSFans2014 PTAL |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Makefile (1)
42-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCentralize the repeated worktree setup.
docker,dockerwithlib, andhack/build.sh:docker_buildeach mutate.git. Their quoting and failure handling already differ. Move relocation and restoration into one helper to prevent future divergence and reduce thedockerwithlibtarget length.🤖 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 `@Makefile` around lines 42 - 48, Centralize the `.git` worktree relocation and restoration currently duplicated across the `docker`, `dockerwithlib`, and `docker_build` flows into a shared Makefile helper or callable function. Update each caller to use that helper, preserving the existing worktree detection, `.git/modules/libvgpu` setup, quoting, and failure-handling behavior while shortening the `dockerwithlib` target.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.
Inline comments:
In `@Makefile`:
- Around line 24-30: Make the worktree metadata relocation failure-safe in
Makefile entries at Makefile lines 24-39 and 42-58, and in hack/build.sh lines
37-54: before renaming .git, register an idempotent cleanup handler that can
restore it, separately validate git rev-parse and the real_gitdir value, and
check mkdir and metadata-copy operations. Ensure every setup failure invokes
cleanup and restores metadata within the same shell.
- Line 29: Update the cp commands in the docker and dockerwithlib targets to
quote both expansions of REAL_GITDIR, including the source path to
modules/libvgpu, so worktree paths containing whitespace remain a single
argument.
---
Nitpick comments:
In `@Makefile`:
- Around line 42-48: Centralize the `.git` worktree relocation and restoration
currently duplicated across the `docker`, `dockerwithlib`, and `docker_build`
flows into a shared Makefile helper or callable function. Update each caller to
use that helper, preserving the existing worktree detection,
`.git/modules/libvgpu` setup, quoting, and failure-handling behavior while
shortening the `dockerwithlib` target.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dab7ba3b-02d2-4171-a01d-f66a9adbc3c8
📒 Files selected for processing (2)
Makefilehack/build.sh
| @if [ -f .git ]; then \ | ||
| echo "Detected git worktree, preparing .git/modules/libvgpu"; \ | ||
| REAL_GITDIR=$$(git rev-parse --git-common-dir); \ | ||
| mv .git .git.bak; \ | ||
| mkdir -p .git/modules; \ | ||
| cp -r $$REAL_GITDIR/modules/libvgpu .git/modules/libvgpu; \ | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)Makefile$|hack/build\.sh$' || true
echo "== Makefile context =="
if [ -f Makefile ]; then
nl -ba Makefile | sed -n '1,70p'
fi
echo "== build.sh context =="
if [ -f hack/build.sh ]; then
nl -ba hack/build.sh | sed -n '1,90p'
fi
echo "== git status/diff =="
git status --short || true
git diff --stat || trueRepository: Project-HAMi/HAMi
Length of output: 269
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | grep -E '(^|/)Makefile$|hack/build\.sh$' || true
echo "== Makefile relevant context =="
awk 'NR>=1 && NR<=80 { printf "%4d\t%s\n", NR, $0 }' Makefile
echo "== charts Makefile relevant context =="
awk 'NR>=1 && NR<=80 { printf "%4d\t%s\n", NR, $0 }' charts/Makefile
echo "== hack/build.sh relevant context =="
awk 'NR>=1 && NR<=90 { printf "%4d\t%s\n", NR, $0 }' hack/build.sh
echo "== git status/diff =="
git status --short || true
git diff --stat || trueRepository: Project-HAMi/HAMi
Length of output: 7841
Make worktree metadata relocation failure-safe in every entry point.
Each implementation moves .git before cleanup is guaranteed. If git rev-parse, mkdir -p .git/modules, or the metadata copy fails, the checkout can be left without .git. Install an idempotent cleanup handler before the rename, check every setup command and variable, and restore metadata from the same shell.
Makefile#L24-L39andMakefile#L42-L58: Register cleanup beforemv .git .git.bak, then restore on setup failure.hack/build.sh#L37-L54: Declare and checkreal_gitdirseparately, install cleanup beforemv, and restore metadata from the same shell.
📍 Affects 2 files
Makefile#L24-L30(this comment)Makefile#L42-L48hack/build.sh#L37-L44
🤖 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 `@Makefile` around lines 24 - 30, Make the worktree metadata relocation
failure-safe in Makefile entries at Makefile lines 24-39 and 42-58, and in
hack/build.sh lines 37-54: before renaming .git, register an idempotent cleanup
handler that can restore it, separately validate git rev-parse and the
real_gitdir value, and check mkdir and metadata-copy operations. Ensure every
setup failure invokes cleanup and restores metadata within the same shell.
Source: Linters/SAST tools
| REAL_GITDIR=$$(git rev-parse --git-common-dir); \ | ||
| mv .git .git.bak; \ | ||
| mkdir -p .git/modules; \ | ||
| cp -r $$REAL_GITDIR/modules/libvgpu .git/modules/libvgpu; \ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Makefiles:\n'
git ls-files | grep -E '(^|/)Makefile$|(^|/)makefile$' || true
printf '\nRelevant Makefile excerpts:\n'
if [ -f Makefile ]; then
nl -ba Makefile | sed -n '1,70p'
fi
printf '\nSearch REAL_GITDIR usages:\n'
rg -n '\bREAL_GITDIR\b|\bdockerwithlib\b|\blibvgpu\b' -S .Repository: Project-HAMi/HAMi
Length of output: 260
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant Makefile excerpts:\n'
awk '{printf "%6d\t%s\n", NR, $0}' Makefile | sed -n '1,70p'
printf '\nSearch REAL_GITDIR usages:\n'
rg -n '\bREAL_GITDIR\b|\bdockerwithlib\b|\blibvgpu\b' -S .Repository: Project-HAMi/HAMi
Length of output: 6405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp_dir="$(mktemp -d)"
worktree_parent="$tmp_dir/parent with spaces"
real_gitdir="$worktree_parent/gitdir with space"
mkdir -p "$worktree_parent/.git/modules"
mkdir -p "$real_gitdir/modules/libvgpu/subdir"
printf 'hello\n' > "$real_gitdir/modules/libvgpu/subdir/test.txt"
bash -c '
unset REAL_GITDIR; export REAL_GITDIR="'"$real_gitdir"'"; rm -f test_file; cp -r "$REAL_GITDIR/modules/libvgpu" .git/modules/libvgpu
' bashworkdir "$worktree_parent"
find "$worktree_parent" -type f -print | sort | while read -r f; do
rel="${f#$worktree_parent/}"
printf '%s\t' "$rel"
if [ -f "$f" ]; then cat -- "$f"; else printf '[dir]\n'; fi
done
rm -rf "$tmp_dir"Repository: Project-HAMi/HAMi
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp_dir="$(mktemp -d)"
worktree_parent="$tmp_dir/parent with spaces"
real_gitdir="$worktree_parent/gitdir with space"
mkdir -p "$worktree_parent/.git/modules"
mkdir -p "$real_gitdir/modules/libvgpu/subdir"
printf 'hello\n' > "$real_gitdir/modules/libvgpu/subdir/test.txt"
bash -c '
unset REAL_GITDIR; export REAL_GITDIR="'"$real_gitdir"'"; rm -f test_file; mkdir -p .git/modules; cp -r "$REAL_GITDIR/modules/libvgpu" .git/modules/libvgpu
' bashworkdir "$worktree_parent"
find "$worktree_parent" -mindepth 1 -print | sort | while read -r f; do
rel="${f#$worktree_parent/}"
printf '%s\t' "$rel"
if [ -f "$f" ]; then cat -- "$f"; else printf '[dir]\n'; fi
done
rm -rf "$tmp_dir"Repository: Project-HAMi/HAMi
Length of output: 384
Quote both REAL_GITDIR expansions.
A git worktree under a path with whitespace splits the unquoted cp source into multiple arguments. Quote the source in both docker and dockerwithlib.
Proposed fix
- cp -r $$REAL_GITDIR/modules/libvgpu .git/modules/libvgpu; \
+ cp -r "$$REAL_GITDIR/modules/libvgpu" .git/modules/libvgpu; \
...
- cp -r $$REAL_GITDIR/modules/libvgpu .git/modules/libvgpu; \
+ cp -r "$$REAL_GITDIR/modules/libvgpu" .git/modules/libvgpu; \🤖 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 `@Makefile` at line 29, Update the cp commands in the docker and dockerwithlib
targets to quote both expansions of REAL_GITDIR, including the source path to
modules/libvgpu, so worktree paths containing whitespace remain a single
argument.
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:
|
|
Thanks for working on Git worktree builds. The current approach temporarily renames the checkout's .git entry to .git.bak. That can leave the repository metadata displaced when the copy fails or the process is interrupted, and it is unsafe when builds run concurrently. The existing major review concern is therefore a correctness issue, not a minor cleanup request. We are closing this implementation. A replacement must leave the working tree metadata untouched and include a real worktree-build regression test. |
4 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR resolves an issue where the Docker builds (
docker/Dockerfileanddocker/Dockerfile.hamicore) would fail early when building from a linked Git worktree. The build context assumed that.git/modules/libvgpuwould always exist as a directory, which is false in worktree environments where.gitis a file.By removing the hardcoded
COPY .git/modules/libvgpusteps, we make the Dockerfiles portable across all supported Git working tree layouts. The internallibvgpu/build.shscript already handles missing git metadata gracefully by defaultingCI_COMMIT_SHAto"unknown".Which issue(s) this PR fixes:
Fixes #2041
Special notes for your reviewer:
This ensures compatibility with developers who use
git worktreefor maintaining multiple branches simultaneously without needing multiple full clones.Does this PR introduce a user-facing change?: