fix(amd): empty nouse-gputype annotation should not exclude every device - #2395
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe AMD type filter now ignores empty or whitespace-only annotation values and empty comma-separated tokens. Tests cover the filtering cases. Scheduler tests now close channels, remove a temporary device registration, and mark quota limits as explicitly configured. ChangesAMD annotation filtering and scheduler test updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🧹 Nitpick comments (1)
pkg/device/amd/device_test.go (1)
475-477: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd whitespace-only regression cases.
The implementation uses
strings.TrimSpace, but the table tests only the empty string. Add" "cases for bothAMDInUseandAMDNoUseto verify the whitespace-only behavior.Proposed test additions
{"empty nouse annotation keeps card", map[string]string{AMDNoUse: ""}, "MI300X", true}, {"empty use annotation keeps card", map[string]string{AMDInUse: ""}, "MI300X", true}, + {"whitespace-only nouse annotation keeps card", map[string]string{AMDNoUse: " "}, "MI300X", true}, + {"whitespace-only use annotation keeps card", map[string]string{AMDInUse: " "}, "MI300X", true},🤖 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/amd/device_test.go` around lines 475 - 477, Add whitespace-only table cases alongside the existing empty annotation cases in the relevant test table, covering both AMDInUse and AMDNoUse with a value of spaces and expecting the MI300X card to remain included.
🤖 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/amd/device.go`:
- Around line 167-175: Update the AMDInUse matching logic around the useTypes
predicate to trim each comma-separated member and skip empty entries before
calling strings.Contains. Preserve the existing rejection behavior when no
non-empty member matches, and apply the same empty-member handling without
changing AMDNoUse processing.
---
Nitpick comments:
In `@pkg/device/amd/device_test.go`:
- Around line 475-477: Add whitespace-only table cases alongside the existing
empty annotation cases in the relevant test table, covering both AMDInUse and
AMDNoUse with a value of spaces and expecting the MI300X card to remain
included.
🪄 Autofix
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: 9ffefe8b-f816-4464-a52f-c49788e2f4f3
📒 Files selected for processing (2)
pkg/device/amd/device.gopkg/device/amd/device_test.go
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.
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
689e7d4 to
83fb305
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
please resolve the code-rabbit comment |
strings.Split on a trailing/leading/double comma (e.g. "MI250,") leaves an empty member, and strings.Contains(cardType, "") is always true, so that member matched every card. This let a nouse-gputype annotation like "MI250," exclude all cards instead of just MI250, and let a use-gputype annotation like "MI300," match cards it shouldn't. Trim and skip empty members before matching, in both branches. Also add whitespace-only and trailing/leading-comma regression cases to TestCheckAMDType, per review feedback on PR Project-HAMi#2395. Signed-off-by: Aditya Raut <araut7798@gmail.com>
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/scheduler/scheduler_test.go`:
- Line 2070: Update the test setup around device.SupportDevices to capture the
prior "TEST" mapping and whether the key was present before assigning the
temporary value. In the t.Cleanup callback, restore the saved mapping when it
previously existed; otherwise delete "TEST", preserving the original
package-level state.
🪄 Autofix
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: 009fb84f-56bb-4873-a6ac-33c059320f31
📒 Files selected for processing (1)
pkg/scheduler/scheduler_test.go
checkAMDType only guarded the in-use annotation against an empty value; the no-use annotation had no such guard. If amd.com/nouse-gputype was present but set to an empty string, strings.Split returned a single empty-string element, and strings.Contains(cardType, "") is always true in Go, so every AMD card was treated as excluded and no AMD device could be scheduled. Apply the same "ok && non-empty" guard used for the in-use annotation to the no-use annotation as well, matching the shared device.CheckType helper used by other vendor backends. Signed-off-by: Aditya Raut <araut7798@gmail.com>
…ling Signed-off-by: Aditya Raut <araut7798@gmail.com>
strings.Split on a trailing/leading/double comma (e.g. "MI250,") leaves an empty member, and strings.Contains(cardType, "") is always true, so that member matched every card. This let a nouse-gputype annotation like "MI250," exclude all cards instead of just MI250, and let a use-gputype annotation like "MI300," match cards it shouldn't. Trim and skip empty members before matching, in both branches. Also add whitespace-only and trailing/leading-comma regression cases to TestCheckAMDType, per review feedback on PR Project-HAMi#2395. Signed-off-by: Aditya Raut <araut7798@gmail.com>
Signed-off-by: Aditya Raut <araut7798@gmail.com>
Several tests call informerFactory.Start(s.stopCh) but never close s.stopCh, so the shared informer's event-processing goroutine keeps running after the test finishes. Test_Filter and Test_ResourceQuota also wire that informer's UpdateFunc to s.onUpdatePod, so a pod update processed during the test can still be in flight on that leaked goroutine once later tests start, racing their unsynchronized writes to global state (e.g. device.SupportDevices in Test_onAddPod_BadDeviceAnnotation) against DecodePodDevices' reads. Reproduced with the race detector via the same command CI's hack/unit-test.sh runs (go test $(go list ./pkg/... ./cmd/...) -short --race -count=1); confirmed unrelated to the pkg/device/amd change in this PR since it reproduces without it. Add t.Cleanup to close each stopCh (and to remove the "TEST" entry added to device.SupportDevices) so no test outlives its own goroutines. Signed-off-by: Aditya Raut <araut7798@gmail.com>
Project-HAMi#2313 added a LimitSet flag that FitQuota now gates on instead of Limit != 0, and updated TestFitResourceQuota's fixture accordingly, but missed three sibling tests in the same file that also build device.Quota directly: TestFitResourceQuotaNonNvidia, TestFitResourceQuotaCountsEveryDevice, and TestFitResourceQuotaAscendMemoryFactor. Their fixtures defaulted to LimitSet: false, so FitQuota treated the configured limits as unset and admitted every pod, failing the denial assertions in all three tests on current master. Set LimitSet: true on the five affected fixture entries to match what AddQuota produces, mirroring the fix already applied to TestFitResourceQuota. Signed-off-by: Aditya Raut <araut7798@gmail.com>
0eeb3e9 to
ab4a621
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adity1raut, archlitchi The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2 participants
What this fixes
checkAMDTypeinpkg/device/amd/device.godecides whether an AMD card is eligible for a pod based on theamd.com/use-gputype/amd.com/nouse-gputypeannotations. The in-use check is guarded against an empty annotation value:but the no-use check was not:
If
amd.com/nouse-gputypeis present but set to an empty string,strings.Split("", ",")returns[""], andstrings.Contains(cardType, "")is alwaystruein Go (every string contains the empty string). SoContainsFuncreturnstrue, and the function returnsfalsefor every card — an empty exclusion annotation ends up excluding all AMD devices instead of excluding none, and the pod becomes unschedulable.The shared
device.CheckTypehelper used by the other vendor backends (nvidia, hygon, ...) already guards against this withok && strings.TrimSpace(value) != "". The AMD backend has its own local copy of this logic and was missing that guard on the no-use branch.Fix
Add the same
ok && strings.TrimSpace(...) != ""guard to the no-use branch, mirroring the existing guard on the in-use branch and the shared helper.Testing
TestCheckAMDTypecovering empty/non-empty use and no-use annotations, including the regression case. Verified it fails against the old code and passes with the fix.go test ./pkg/device/amd/... -race -count=1passes.Fit()), not device-plugin/container isolation code, so per the contribution guide's testing policy, unit tests are the appropriate validation here — I don't have AMD GPU hardware to validate against.AI disclosure
This PR was written primarily by Geminy, used to investigate the codebase, identify the bug, implement the fix, and write the regression test. I reviewed and verified the change before submitting.
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests