fix: resolve libvgpu git metadata path for linked worktree Docker builds - #2042
fix: resolve libvgpu git metadata path for linked worktree Docker builds#2042pingxin403 wants to merge 2 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pingxin403 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change modifies ChangesDocker build worktree workaround
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 addresses pod leaks during scheduling failures by introducing a restorePod helper in pkg/scheduler/scheduler.go to restore pod and quota manager states, and adds a corresponding unit test. It also updates hack/build.sh to handle linked git worktrees during Docker builds. The review feedback highlights two critical issues: first, the trap in hack/build.sh should be registered immediately after copying .git to prevent a broken repository state if subsequent setup steps fail; second, if util.PatchPodAnnotations fails, the newly added placement must be explicitly cleaned up before calling restorePod to avoid resource leaks.
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.
| local _restore_git="" | ||
| if [ -f .git ]; then | ||
| local _git_common | ||
| _git_common=$(git rev-parse --git-common-dir 2>/dev/null) | ||
| if [ -n "$_git_common" ] && [ -d "$_git_common/modules/libvgpu" ]; then | ||
| _restore_git=$(mktemp) | ||
| cp .git "$_restore_git" | ||
| rm -f .git | ||
| mkdir -p .git/modules | ||
| cp -r "$_git_common/modules/libvgpu" .git/modules/ | ||
| fi | ||
| fi | ||
| if [ -n "$_restore_git" ]; then | ||
| # shellcheck disable=SC2064 | ||
| trap "rm -rf .git && cp '$_restore_git' .git && rm -f '$_restore_git'" EXIT | ||
| fi |
There was a problem hiding this comment.
In the current implementation, if any command inside the if [ -f .git ] block fails after .git is deleted (such as mkdir -p .git/modules or cp -r ...), the script will exit immediately due to set -e. Since the trap is registered after the if block, it will never be registered, leaving the repository in a broken state with .git deleted.
Registering the trap immediately after copying .git to the temporary file ensures that the cleanup and restoration are guaranteed to run if any subsequent setup step fails.
| local _restore_git="" | |
| if [ -f .git ]; then | |
| local _git_common | |
| _git_common=$(git rev-parse --git-common-dir 2>/dev/null) | |
| if [ -n "$_git_common" ] && [ -d "$_git_common/modules/libvgpu" ]; then | |
| _restore_git=$(mktemp) | |
| cp .git "$_restore_git" | |
| rm -f .git | |
| mkdir -p .git/modules | |
| cp -r "$_git_common/modules/libvgpu" .git/modules/ | |
| fi | |
| fi | |
| if [ -n "$_restore_git" ]; then | |
| # shellcheck disable=SC2064 | |
| trap "rm -rf .git && cp '$_restore_git' .git && rm -f '$_restore_git'" EXIT | |
| fi | |
| local _restore_git="" | |
| if [ -f .git ]; then | |
| local _git_common | |
| _git_common=$(git rev-parse --git-common-dir 2>/dev/null) | |
| if [ -n "$_git_common" ] && [ -d "$_git_common/modules/libvgpu" ]; then | |
| _restore_git=$(mktemp) | |
| cp .git "$_restore_git" | |
| # shellcheck disable=SC2064 | |
| trap "rm -rf .git && cp '$_restore_git' .git && rm -f '$_restore_git'" EXIT | |
| rm -f .git | |
| mkdir -p .git/modules | |
| cp -r "$_git_common/modules/libvgpu" .git/modules/ | |
| fi | |
| fi |
| err = util.PatchPodAnnotations(args.Pod, annotations) | ||
| if err != nil { | ||
| s.recordScheduleFilterResultEvent(args.Pod, EventReasonFilteringFailed, "", err) | ||
| if added { | ||
| s.quotaManager.RmUsage(args.Pod, m.Devices) | ||
| } | ||
| s.podManager.DelPod(args.Pod) | ||
| restorePod() | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
If util.PatchPodAnnotations fails, the scheduler attempts to restore the old pod placement by calling restorePod(). However, the new placement has already been added to podManager and quotaManager (via s.podManager.AddPod and s.quotaManager.AddUsage). Calling restorePod() without first removing the new placement will result in a resource leak in quotaManager (since the new placement's usage is never removed) and potentially podManager (if removed was false, the new pod is never deleted).
To fix this, we should explicitly remove the new placement from both managers if added was true, before calling restorePod().
| err = util.PatchPodAnnotations(args.Pod, annotations) | |
| if err != nil { | |
| s.recordScheduleFilterResultEvent(args.Pod, EventReasonFilteringFailed, "", err) | |
| if added { | |
| s.quotaManager.RmUsage(args.Pod, m.Devices) | |
| } | |
| s.podManager.DelPod(args.Pod) | |
| restorePod() | |
| return nil, err | |
| } | |
| err = util.PatchPodAnnotations(args.Pod, annotations) | |
| if err != nil { | |
| s.recordScheduleFilterResultEvent(args.Pod, EventReasonFilteringFailed, "", err) | |
| if added { | |
| s.quotaManager.RmUsage(args.Pod, m.Devices) | |
| s.podManager.DelPod(args.Pod) | |
| } | |
| restorePod() | |
| return nil, err | |
| } |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/scheduler/scheduler.go (1)
821-831: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRollback the tentative pod/quota add before
restorePod()
restorePod()only restores the state from beforeTakeAndDeletePod(...). IfPatchPodAnnotations(...)fails afterAddPod(...)/AddUsage(...), the fresh-pod path leaks cache/quota state, and the re-schedule path leaves the entry onm.NodeIDwhile onlyDevicesgets overwritten back. Undo the tentative add first, then restore the prior state.🤖 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/scheduler/scheduler.go` around lines 821 - 831, The post-patch failure path in scheduler logic leaves tentative pod and quota state behind. In the scheduling flow around AddPod, AddUsage, and util.PatchPodAnnotations, roll back the fresh AddPod/quota update (and the re-schedule cache entry on m.NodeID) before calling restorePod(), so the state is fully reverted prior to restoring the previous pod assignment.
🤖 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 `@hack/build.sh`:
- Around line 42-58: The EXIT restore trap in the git preservation logic is
registered too late, leaving a failure window after `.git` is removed and before
cleanup is guaranteed. In `build.sh` around the `_restore_git` handling, move
the `trap` setup so it is installed before any destructive `rm -f .git`, `mkdir
-p .git/modules`, or `cp -r` operations, and keep the restore behavior tied to
the same `_restore_git` variable so the worktree is always recoverable if any
step fails.
---
Outside diff comments:
In `@pkg/scheduler/scheduler.go`:
- Around line 821-831: The post-patch failure path in scheduler logic leaves
tentative pod and quota state behind. In the scheduling flow around AddPod,
AddUsage, and util.PatchPodAnnotations, roll back the fresh AddPod/quota update
(and the re-schedule cache entry on m.NodeID) before calling restorePod(), so
the state is fully reverted prior to restoring the previous pod assignment.
🪄 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: b5c18fe1-fcd1-45cc-93bd-871bcb5ad178
📒 Files selected for processing (3)
hack/build.shpkg/scheduler/scheduler.gopkg/scheduler/scheduler_test.go
In linked git worktrees, .git is a file (not a directory), so the path .git/modules/libvgpu does not exist in the Docker build context. This causes the Dockerfile COPY command to fail with 'not found'. Detect worktrees via [ -f .git ] and temporarily create the expected .git/modules/libvgpu directory from the real common gitdir, restoring the .git file on exit. Fixes Project-HAMi#2041 Signed-off-by: pingxin403 <pingxin403@163.com>
35e7f82 to
d1d9274
Compare
…uild Signed-off-by: pingxin403 <pingxin403@163.com>
d1d9274 to
7591a13
Compare
|
I think your fix won't work. Please check and test yourself or your agents, then create the PR. |
|
Close for now — the fix needs to be in docker_build() context preparation (pre-build), not in build.sh (runtime). Will revisit with correct approach. |
2 participants
What does this PR do?
Fixes #2041 — Docker build fails when the repository is a linked git worktree because
.git/modules/libvgpudoes not exist (in worktrees,.gitis a file, not a directory).Root Cause
The Dockerfile uses
COPY .git/modules/libvgpu /libvgpu-gitto preserve libvgpu's git metadata for version detection inside the container. In a linked worktree, the repository root.gitis a text file pointing to the real gitdir, so.git/modules/libvgpuis not a valid path.Fix
In
hack/build.sh'sdocker_build()function: detect worktrees via[ -f .git ], usegit rev-parse --git-common-dirto find the real modules path, and temporarily create.git/modules/libvgpufor the Docker build context. The.gitfile is restored on exit via a trap.Testing
[ -f .git ]is false, code path skipped — no change in behaviorgit rev-parse --git-common-dir.gitfile on both success and failureAI Assistance Disclosure
This PR was developed with the assistance of Claude Code (Anthropic) for code generation and debugging. The author reviewed, tested, and takes full responsibility for all changes.
Summary by CodeRabbit