fix(scheduler): make NUMA sort opt-in for binpack/spread policies#1874
fix(scheduler): make NUMA sort opt-in for binpack/spread policies#1874mesutoezdil wants to merge 7 commits into
Conversation
![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 introduces the hami.io/topology-aware-scoring annotation, allowing users to disable NUMA-aware GPU scheduling. It also enhances error handling by ensuring that failures in decoding pod devices are logged and handled gracefully without stopping the monitoring or scheduling processes. Corresponding unit tests were added for these changes. Feedback was provided to optimize performance in the scheduler by moving loop-invariant calculations for GPU policy and NUMA settings outside of the node iteration loop.
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:
|
f84e65a to
0477f77
Compare
0477f77 to
b4d12ae
Compare
deae042 to
206023b
Compare
any news @archlitchi |
206023b to
ccde489
Compare
ccde489 to
eedf4f3
Compare
hami-robot
Bot
added
dco-signoff: no
size/XXL
and removed
dco-signoff: yes
size/M
labels
cd18c31 to
dddef02
Compare
hami-robot
Bot
added
dco-signoff: yes
size/M
and removed
dco-signoff: no
size/XXL
labels
Same root cause as the other branches: upstream refactored inline node-usage construction into buildNodeUsage(), this branch computed NumaIgnore from the hami.io/topology-aware-scoring annotation at the call site. Moved that computation into buildNodeUsage() so both are kept. Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
|
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:
📝 WalkthroughWalkthroughGPU device usage now defaults to NUMA-first sorting, with an annotation-controlled mode that orders solely by score. The new setting is copied across usage lists, wired from pod annotations, and covered by policy and NUMA selection tests. ChangesGPU NUMA sorting
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Pod
participant buildNodeUsage
participant DeviceUsageList
participant Fit
Pod->>buildNodeUsage: topology-aware annotation
buildNodeUsage->>DeviceUsageList: set NumaIgnore
DeviceUsageList->>DeviceUsageList: sort by score or NUMA
Fit->>DeviceUsageList: select sorted devices
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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/scheduler/policy/gpu_policy_test.go`:
- Around line 124-129: Update the fully populated test case in
TestDeviceUsageListDeepCopy to set NumaIgnore: true in its input and expected
values, ensuring DeepCopy verifies propagation of the non-zero field while
preserving the existing assertions.
🪄 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: ea443c14-8257-4123-bdc0-5d6ee05a812c
📒 Files selected for processing (4)
pkg/scheduler/policy/gpu_policy.gopkg/scheduler/policy/gpu_policy_test.gopkg/scheduler/scheduler.gopkg/util/types.go
Fully populated case did not set NumaIgnore, so the deep copy check could not catch a regression in that field. Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
…or-binpack-spread
|
✅ Action performedReview finished.
|
|
/assign @archlitchi |
Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mesutoezdil 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 |
Per Project-HAMi#1806 discussion, the default stays NUMA-primary; hami.io/topology-aware-scoring: "false" opts out to pure Score ordering. NumaBind is kept for compatibility but no longer changes Less's behavior, since NUMA grouping is now unconditional. Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/scheduler/policy/gpu_policy_test.go (1)
245-265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test cases for the NUMA tiebreaker.
If you implement the NUMA tiebreaker for equal scores in
gpu_policy.go, consider adding the following test cases to ensure the tiebreaker behavior is explicitly verified and protected from future regressions.🧪 Proposed test cases
}, expectedLess: false, }, + { + name: "Binpack NumaIgnore score tiebreaker", + policy: "binpack", + numaIgnore: true, + deviceLists: []*DeviceListsScore{ + {Device: &device.DeviceUsage{Numa: 1}, Score: 10}, + {Device: &device.DeviceUsage{Numa: 0}, Score: 10}, + }, + expectedLess: true, + }, + { + name: "Spread NumaIgnore score tiebreaker", + policy: "spread", + numaIgnore: true, + deviceLists: []*DeviceListsScore{ + {Device: &device.DeviceUsage{Numa: 0}, Score: 10}, + {Device: &device.DeviceUsage{Numa: 1}, Score: 10}, + }, + expectedLess: 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/scheduler/policy/gpu_policy_test.go` around lines 245 - 265, Add test cases in the GPU policy comparison test table covering equal Score values with differing NUMA nodes, verifying the NUMA tiebreaker for both binpack and spread policies. Keep the existing NumaIgnore cases unchanged and assert the expected ordering through expectedLess.
🤖 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/policy/gpu_policy.go`:
- Around line 66-72: Update the NumaIgnore branch in the relevant score-ordering
comparator to use NUMA ordering when si and sj are equal, while preserving the
existing score-first behavior for unequal scores and the binpack direction.
Ensure equal-score comparisons return a deterministic NUMA-based result rather
than false.
---
Nitpick comments:
In `@pkg/scheduler/policy/gpu_policy_test.go`:
- Around line 245-265: Add test cases in the GPU policy comparison test table
covering equal Score values with differing NUMA nodes, verifying the NUMA
tiebreaker for both binpack and spread policies. Keep the existing NumaIgnore
cases unchanged and assert the expected ordering through expectedLess.
🪄 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: d0a98096-3b11-4c63-af69-409b4e6620fe
📒 Files selected for processing (3)
pkg/scheduler/numa_sort_test.gopkg/scheduler/policy/gpu_policy.gopkg/scheduler/policy/gpu_policy_test.go
…sort sort.Sort is not stable, so returning false for both Less(i,j) and Less(j,i) on equal Score left device order for tied scores nondeterministic across scheduling cycles. Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
3 participants
What type of PR is this?
/kind bug
When spread or binpack policy is selected, Less() in pkg/scheduler/policy/gpu_policy.go uses NUMA as the primary sort key.
This overrides the policy intent: a busy NUMA node gets picked over an idle one on the other NUMA node.
Fix: add NumaIgnore bool to DeviceUsageList.
When the pod annotation hami.io/topology-aware-scoring: "false" is set, Less() sorts by Score only.
Default is unchanged (NUMA-first, backward compatible).
Which issue(s) this PR fixes: Fixes #1806
Does this PR introduce a user-facing change?
Yes. Users can now set hami.io/topology-aware-scoring: "false" on a pod to make spread/binpack sort by Score without NUMA grouping.
Summary by CodeRabbit
hami.io/topology-aware-scoringannotation to control NUMA-aware GPU ordering per workload."false", device ordering ignores NUMA locality and sorts by score only (direction depends on binpack vs spread); default ordering remains NUMA-first, using score only within the same NUMA node.