fix(scheduler): treat an empty use/nouse gpuuuid annotation as no constraint - #2045
Conversation
CheckUUID split the annotation value on ',' and required a device id to equal one of the parts. An empty value yielded [""], which matches no real device, so an empty nvidia.com/use-gpuuuid (or the per-vendor equivalent) filtered out every device and made the node unschedulable with CardUuidMismatch. Skip the check when the value is empty/whitespace. Signed-off-by: wangmin <wangmin@riseunion.io>
|
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 (2)
📝 WalkthroughWalkthroughUUID, Hygon DCU type, and NVIDIA GPU type checks now ignore empty or whitespace-only annotation values instead of applying restrictive filters. Tests cover the updated behavior. ChangesDevice annotation constraints
Estimated code review effort: 2 (Simple) | ~10 minutes 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 updates the CheckUUID function in pkg/device/devices.go to treat empty or whitespace-only UUID annotations as 'no constraint' rather than attempting to match them, preventing devices from being incorrectly filtered out. It also adds corresponding unit tests in pkg/device/devices_test.go to verify this behavior. There are no review comments, and I have no feedback to provide.
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.
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 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
|
checkGPUtype in pkg/device/nvidia/device.go has the same bug on the nouse-gputype branch, strings.Contains(cardtype, "") is always true so an empty nvidia.com/nouse-gputype annotation excludes every gpu type |
|
checkDCUtype in pkg/device/hygon/device.go has the same bug on the nouse-dcutype branch, strings.Contains(cardtype, "") is always true so an empty hygon.com/nouse-dcutype annotation excludes every dcu type |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/device/hygon/device.go (1)
98-131: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard for whole-annotation emptiness, but split still yields empty tokens on malformed lists.
Lines 102/116 correctly guard against a fully empty/whitespace
DCUInUse/DCUNoUsevalue. However, for the multi-value branch (strings.SplitSeq(inuse, ",")/strings.SplitSeq(nouse, ",")), a trailing/double comma (e.g."dcu,"or"dcu,,test") produces an empty token. Sincestrings.Contains(cardtype, "")is alwaystrue, that empty token will match every card type — reintroducing the same bug this PR fixes, just via a malformed list instead of a fully empty string.🐛 Proposed fix to skip empty tokens
} else { for val := range strings.SplitSeq(inuse, ",") { + if strings.TrimSpace(val) == "" { + continue + } if strings.Contains(strings.ToUpper(cardtype), strings.ToUpper(val)) { return 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/hygon/device.go` around lines 98 - 131, Skip empty or whitespace-only tokens in the comma-separated branches of checkDCUtype for both DCUInUse and DCUNoUse before performing case-insensitive substring matching; trim each token, ignore it when empty, and only compare non-empty values against cardtype.pkg/device/nvidia/device.go (1)
487-509: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSame trailing/empty-token issue as the Hygon fix, via
strings.Split.The whole-value guards on Lines 492 and 500 correctly handle fully empty/whitespace annotations, but
strings.Split(inuse, ",")/strings.Split(unuse, ",")will still produce an empty string token for malformed values like"A100,"or"A100,,V100". Sincestrings.Contains(cardtype, "")is alwaystrue, that empty token silently matches every card type in theslices.ContainsFunccheck, reintroducing the "match everything" bug forGPUNoUse(or "allow everything" forGPUInUse) on this specific input shape.🐛 Proposed fix to filter empty tokens after split
if inuse, ok := annos[GPUInUse]; ok && strings.TrimSpace(inuse) != "" { useTypes := strings.Split(inuse, ",") - if !slices.ContainsFunc(useTypes, func(useType string) bool { - return strings.Contains(cardtype, strings.ToUpper(useType)) - }) { + if !slices.ContainsFunc(useTypes, func(useType string) bool { + useType = strings.TrimSpace(useType) + return useType != "" && strings.Contains(cardtype, strings.ToUpper(useType)) + }) { return false } } if unuse, ok := annos[GPUNoUse]; ok && strings.TrimSpace(unuse) != "" { unuseTypes := strings.Split(unuse, ",") - if slices.ContainsFunc(unuseTypes, func(unuseType string) bool { - return strings.Contains(cardtype, strings.ToUpper(unuseType)) - }) { + if slices.ContainsFunc(unuseTypes, func(unuseType string) bool { + unuseType = strings.TrimSpace(unuseType) + return unuseType != "" && strings.Contains(cardtype, strings.ToUpper(unuseType)) + }) { return false } }🤖 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/nvidia/device.go` around lines 487 - 509, Filter out empty or whitespace-only tokens after splitting GPUInUse and GPUNoUse values in checkGPUtype, before the slices.ContainsFunc checks. Trim each token and ignore blank entries so malformed values such as trailing or repeated commas cannot match every card type; preserve the existing behavior for valid tokens and fully empty annotations.
🤖 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.
Outside diff comments:
In `@pkg/device/hygon/device.go`:
- Around line 98-131: Skip empty or whitespace-only tokens in the
comma-separated branches of checkDCUtype for both DCUInUse and DCUNoUse before
performing case-insensitive substring matching; trim each token, ignore it when
empty, and only compare non-empty values against cardtype.
In `@pkg/device/nvidia/device.go`:
- Around line 487-509: Filter out empty or whitespace-only tokens after
splitting GPUInUse and GPUNoUse values in checkGPUtype, before the
slices.ContainsFunc checks. Trim each token and ignore blank entries so
malformed values such as trailing or repeated commas cannot match every card
type; preserve the existing behavior for valid tokens and fully empty
annotations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 914eefa5-5f18-43d4-a0d9-68ddd2c5e900
📒 Files selected for processing (4)
pkg/device/hygon/device.gopkg/device/hygon/device_test.gopkg/device/nvidia/device.gopkg/device/nvidia/device_test.go
…aint checkGPUtype (nvidia) and checkDCUtype (hygon) match a card type with strings.Contains, which treats an empty string as a substring of every type. An empty nouse-gputype / nouse-dcutype annotation therefore made Contains(cardtype, "") always true and excluded every device, leaving the node unschedulable. Skip the check when the value is empty or whitespace so an empty type allow/deny list means no constraint, mirroring the CheckUUID fix in this PR. Signed-off-by: wangmin <wangmin@riseunion.io>
d41f242 to
c0c2a08
Compare
Good catch — you're right. Fixed both nvidia (checkGPUtype) and hygon (checkDCUtype); the other vendors don't have this issue. Verified the nvidia path on a real GTX 1080 Ti (empty nouse-gputype now schedules; real type filtering preserved). Thanks! |
…ltering checkGPUtype (nvidia) and checkDCUtype (hygon) duplicated the same use/nouse type-filter shape, including the empty-value guard added in the previous commit. Extract a CheckType helper in devices.go, parallel to CheckUUID, so both vendors and the empty-value guard live in one place. hygon previously returned early once the use annotation was set and never evaluated nouse; CheckType evaluates both, matching CheckUUID/nvidia. No existing test set both, and the behaviour only differs for the contradictory case where a card is in both the use and nouse list. Signed-off-by: wangmin <wangmin@riseunion.io>
reminder for future: per the contribution guidelines (gate 6), replies in pr review threads must be written directly by the author, not generated by ai tools. |
|
/lgtm |
It wasn't generated entirely by AI. I wrote it in Chinese first, then had the AI translate it into English and polish it up a bit. My English isn't very good. |
no problem mate, it was just a reminder :) thanks for your works, you do all amazing, i like your prs :) always direct lgtm, all clear :) have a great day! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, Wangmin362 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 |
KunlunUseUUID and KunlunNoUseUUID were defined but never read, so a pod asking for baidu.com/use-gpuuuid was scheduled onto any XPU and nouse-gpuuuid could not exclude a card. Every other backend calls device.CheckUUID in Fit(); kunlun was missed when the shared helper landed in Project-HAMi#1622 and again when Project-HAMi#2045 changed it. graghSelect derives topology from a device's position in the slice, so the check goes into the fitFn it already takes rather than filtering the slice, which would shift devices between wings. Both KunlunDevices and KunlunVDevices are covered, and an allocation that fails only because of the annotations now reports CardUuidMismatch instead of NumaNotFit. Signed-off-by: Lakshya77089 <lakshyasharma7708@gmail.com>
* fix(kunlun): honour use-gpuuuid and nouse-gpuuuid annotations KunlunUseUUID and KunlunNoUseUUID were defined but never read, so a pod asking for baidu.com/use-gpuuuid was scheduled onto any XPU and nouse-gpuuuid could not exclude a card. Every other backend calls device.CheckUUID in Fit(); kunlun was missed when the shared helper landed in #1622 and again when #2045 changed it. graghSelect derives topology from a device's position in the slice, so the check goes into the fitFn it already takes rather than filtering the slice, which would shift devices between wings. Both KunlunDevices and KunlunVDevices are covered, and an allocation that fails only because of the annotations now reports CardUuidMismatch instead of NumaNotFit. Signed-off-by: Lakshya77089 <lakshyasharma7708@gmail.com> * fix(kunlun): reuse the xpu uuid annotations and dedupe the mismatch count graghSelect can call fitFn more than once for the same device, so the plain counter reported the same card twice. Track mismatched ids in a map instead. The physical path now reads the same hami.io/use-xpu-uuid and hami.io/no-use-xpu-uuid keys vdevice.go already defines, rather than a new pair, so both kunlun paths honour one documented annotation. baidu.com/ stays as the legacy alias. Added a test that uses the literal key so a rename of the constant cannot silently break it. Signed-off-by: Lakshya77089 <lakshyasharma7708@gmail.com> --------- Signed-off-by: Lakshya77089 <lakshyasharma7708@gmail.com>
4 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
CheckUUIDsplits theuse-gpuuuid/nouse-gpuuuidannotation on,and keeps a device only if its id equals one of the parts. When the annotation is present but its value is empty,strings.Split("", ",")returns[""]and no real device id equals"", so an emptynvidia.com/use-gpuuuid(or the per-vendor equivalent, since all vendors shareCheckUUID) makes every device fail the check and the node becomes unschedulable withCardUuidMismatch. This is easy to hit when the annotation value is templated and renders empty; an empty allow-list should mean "no constraint", not "match nothing".This PR skips the check when the value is empty or whitespace. The
usebranch is the actual bug; thenousebranch gets the same guard only for symmetry (an empty exclude-list already excludes nothing, so its behaviour is unchanged).Which issue(s) this PR fixes:
NONE
Special notes for your reviewer:
Extended the existing
TestCheckUUIDwith empty / whitespace-onlyuse-gpuuuidand emptynouse-gpuuuidcases; theusecases fail on master (device filtered out) and pass with the fix.Also verified end to end on real hardware — NVIDIA GeForce GTX 1080 Ti, driver 550.144.03, Kubernetes v1.29 — running the scheduler (master before / fixed after) as a separate scheduler with its own
schedulerNamein an isolated namespace:nvidia.com/use-gpuuuid: "": before -> Pod stays Pending withCardUuidMismatch; after -> Pod is scheduled.CardUuidMismatch, a matching UUID schedules — real UUID filtering is preserved.This PR was written with AI assistance (analysis and drafting); the diagnosis, the fix, and the on-hardware testing were done and reviewed by me.
Does this PR introduce a user-facing change?:
Yes — a pod with an empty
use-gpuuuid/nouse-gpuuuidannotation is no longer wrongly rejected withCardUuidMismatch; an empty value is now treated as no constraint.Summary by CodeRabbit