refactor(nvidia): split Fit into composable check pipeline - #2090
refactor(nvidia): split Fit into composable check pipeline#2090yxxhero wants to merge 3 commits into
Conversation
The 137-line Fit function in pkg/device/nvidia/device.go interleaved 13
distinct per-card failure checks in a single for-loop, making the
reasoning hard to follow and per-branch testing impractical.
This refactor extracts the per-card checks into a pipeline of small,
independently testable functions in fit_checks.go:
checkCardHealth (runs in the Fit loop, before checkType)
checkCardUUID
checkCardTimeSlicing
checkCardQuota
checkCardMemory
checkCardCore
checkCardExclusive
checkCardComputeExhausted
checkCardCustomRule
Each returns the common.ReasonXxx string on failure or empty on pass.
runCardChecks walks the pipeline and short-circuits on the first
failure, preserving the original check order.
NUMA reset, checkType, Coresreq normalization and memreq computation
remain in the Fit loop because they carry cross-card or mutation
side-effects.
Zero breaking changes
---------------------
* Fit method signature is unchanged.
* device.Devices interface is unchanged.
* No exported symbols added or removed.
* All 13 pre-existing TestDevices_Fit cases and the four topology/
NUMA/CustomFilterRule tests in device_test.go still pass unchanged.
* pkg/scheduler/score_test.go's indirect Fit invocations still pass.
Coverage
--------
* Fit: 70% -> 100%.
* All 11 check functions: 100%.
* New tests include regression guards for ordering pitfalls discovered
during self-review (health precedence over type mismatch, MIG
multi-slot selection via the i++ stay-loop, Coresreq>100 clamping).
* BenchmarkFit on 8 cards: ~2000 ns/op, 25 allocs/op.
Known trade-off (disclosed)
---------------------------
normalizeCoresreq now runs before UUID/TimeSlicing instead of after.
Because it is idempotent and reads no fields consumed by the UUID or
TimeSlicing checks, all 13 reason counts, the returned tmpDevs and the
reason string remain identical. The only observable difference is that
klog.ErrorS("core limit can't exceed 100") may fire once even when
every card fails UUID/TimeSlicing (original code would not fire at all
in that edge case). This is log-only; functional behavior is identical.
Signed-off-by: yxxhero <aiopsclub@163.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: yxxhero 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 |
📝 WalkthroughWalkthrough
ChangesNVIDIA fit pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant NvidiaGPUDevices
participant runCardChecks
participant CustomFilterRule
Request->>NvidiaGPUDevices: Fit request and device usages
NvidiaGPUDevices->>runCardChecks: Build cardCheckCtx
runCardChecks->>CustomFilterRule: Evaluate custom card rule
CustomFilterRule-->>runCardChecks: Pass or failure reason
runCardChecks-->>NvidiaGPUDevices: First failed check or success
NvidiaGPUDevices-->>Request: Selected devices and aggregate reason
Possibly related PRs
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 |
![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 refactors the GPU allocation logic in pkg/device/nvidia/device.go by extracting card-specific checks into a pipeline of check functions in a new file pkg/device/nvidia/fit_checks.go, improving code maintainability. It also introduces comprehensive unit tests and benchmarks to verify the correctness and performance of the refactored checks. The review feedback suggests enhancing the robustness of the new functions in fit_checks.go by adding defensive nil checks for parameters like dev, ctx, ctx.pod, and ctx.request to prevent potential nil pointer dereferences.
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.
| func checkCardHealth(dev *device.DeviceUsage, pod *corev1.Pod) string { | ||
| if !dev.Health { | ||
| klog.V(5).InfoS(common.CardNotHealth, "pod", klog.KObj(pod), "device", dev.ID, "health", dev.Health) | ||
| return common.CardNotHealth | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
To adhere to defensive programming best practices and prevent potential nil pointer dereferences, we should add a nil check for the dev parameter at the beginning of checkCardHealth.
func checkCardHealth(dev *device.DeviceUsage, pod *corev1.Pod) string {
if dev == nil {
return common.CardNotHealth
}
if !dev.Health {
klog.V(5).InfoS(common.CardNotHealth, "pod", klog.KObj(pod), "device", dev.ID, "health", dev.Health)
return common.CardNotHealth
}
return ""
}References
- Enforce defensive programming by ensuring appropriate null/nil checks exist before object property accesses.
There was a problem hiding this comment.
Intentionally not added. These four are unexported helpers whose only callers are:
NvidiaGPUDevices.Fit— the production path. Inside the main loopdev := devices[i]cannot be nil unless the caller ofFitputs nil entries in the input slice (which would be a caller bug we want surfaced, not silently swallowed).ctxis constructed inline in the same loop, andctx.pod/ctx.requestare assigned fromFit's own parameters.- The unit tests in
fit_checks_test.go/fit_extra_test.go, which build the inputs explicitly and would not benefit from a nil guard either.
Adding nil checks on private helpers would convert caller bugs into silent failures (e.g. returning common.CardNotHealth for a nil dev would mislabel the failure mode and skew reason counters). It also does not match the prevailing defensive-programming style elsewhere in pkg/device (e.g. checkType, CustomFilterRule, AddResourceUsage all dereference their pointer parameters without nil guards).
If the maintainers prefer the defensive style project-wide, happy to revisit — but it would be a broader change across pkg/device, not specific to this refactor.
| func runCardChecks(dev *device.DeviceUsage, ctx *cardCheckCtx) string { | ||
| for _, check := range cardCheckPipeline { | ||
| if reason := check(dev, ctx); reason != "" { | ||
| return reason | ||
| } | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
To enforce defensive programming and prevent potential nil pointer dereferences across the entire check pipeline, we should validate that dev, ctx, ctx.pod, and ctx.request are not nil before executing the checks.
func runCardChecks(dev *device.DeviceUsage, ctx *cardCheckCtx) string {
if dev == nil || ctx == nil || ctx.pod == nil || ctx.request == nil {
return "InvalidContext"
}
for _, check := range cardCheckPipeline {
if reason := check(dev, ctx); reason != "" {
return reason
}
}
return ""
}References
- Enforce defensive programming by ensuring appropriate null/nil checks exist before object property accesses.
There was a problem hiding this comment.
Intentionally not added. These four are unexported helpers whose only callers are:
NvidiaGPUDevices.Fit— the production path. Inside the main loopdev := devices[i]cannot be nil unless the caller ofFitputs nil entries in the input slice (which would be a caller bug we want surfaced, not silently swallowed).ctxis constructed inline in the same loop, andctx.pod/ctx.requestare assigned fromFit's own parameters.- The unit tests in
fit_checks_test.go/fit_extra_test.go, which build the inputs explicitly and would not benefit from a nil guard either.
Adding nil checks on private helpers would convert caller bugs into silent failures (e.g. returning common.CardNotHealth for a nil dev would mislabel the failure mode and skew reason counters). It also does not match the prevailing defensive-programming style elsewhere in pkg/device (e.g. checkType, CustomFilterRule, AddResourceUsage all dereference their pointer parameters without nil guards).
If the maintainers prefer the defensive style project-wide, happy to revisit — but it would be a broader change across pkg/device, not specific to this refactor.
| func computeMemreq(req device.ContainerDeviceRequest, dev *device.DeviceUsage) int32 { | ||
| if req.Memreq > 0 { | ||
| return req.Memreq | ||
| } | ||
| if req.MemPercentagereq != 101 && req.Memreq == 0 { | ||
| return dev.Totalmem * req.MemPercentagereq / 100 | ||
| } | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
To prevent potential nil pointer dereferences when dev is nil, we should add a guard check for dev != nil before accessing dev.Totalmem.
| func computeMemreq(req device.ContainerDeviceRequest, dev *device.DeviceUsage) int32 { | |
| if req.Memreq > 0 { | |
| return req.Memreq | |
| } | |
| if req.MemPercentagereq != 101 && req.Memreq == 0 { | |
| return dev.Totalmem * req.MemPercentagereq / 100 | |
| } | |
| return 0 | |
| } | |
| func computeMemreq(req device.ContainerDeviceRequest, dev *device.DeviceUsage) int32 { | |
| if req.Memreq > 0 { | |
| return req.Memreq | |
| } | |
| if req.MemPercentagereq != 101 && req.Memreq == 0 && dev != nil { | |
| return dev.Totalmem * req.MemPercentagereq / 100 | |
| } | |
| return 0 | |
| } |
References
- Enforce defensive programming by ensuring appropriate null/nil checks exist before object property accesses.
There was a problem hiding this comment.
Intentionally not added. These four are unexported helpers whose only callers are:
NvidiaGPUDevices.Fit— the production path. Inside the main loopdev := devices[i]cannot be nil unless the caller ofFitputs nil entries in the input slice (which would be a caller bug we want surfaced, not silently swallowed).ctxis constructed inline in the same loop, andctx.pod/ctx.requestare assigned fromFit's own parameters.- The unit tests in
fit_checks_test.go/fit_extra_test.go, which build the inputs explicitly and would not benefit from a nil guard either.
Adding nil checks on private helpers would convert caller bugs into silent failures (e.g. returning common.CardNotHealth for a nil dev would mislabel the failure mode and skew reason counters). It also does not match the prevailing defensive-programming style elsewhere in pkg/device (e.g. checkType, CustomFilterRule, AddResourceUsage all dereference their pointer parameters without nil guards).
If the maintainers prefer the defensive style project-wide, happy to revisit — but it would be a broader change across pkg/device, not specific to this refactor.
| func normalizeCoresreq(req *device.ContainerDeviceRequest, pod *corev1.Pod, dev *device.DeviceUsage) { | ||
| if req.Coresreq > 100 { | ||
| klog.ErrorS(nil, "core limit can't exceed 100", "pod", klog.KObj(pod), "device", dev.ID) | ||
| req.Coresreq = 100 | ||
| } | ||
| } |
There was a problem hiding this comment.
To prevent potential nil pointer dereferences when req or dev is nil, we should add defensive checks at the beginning of normalizeCoresreq.
func normalizeCoresreq(req *device.ContainerDeviceRequest, pod *corev1.Pod, dev *device.DeviceUsage) {
if req == nil {
return
}
if req.Coresreq > 100 {
deviceID := ""
if dev != nil {
deviceID = dev.ID
}
klog.ErrorS(nil, "core limit can't exceed 100", "pod", klog.KObj(pod), "device", deviceID)
req.Coresreq = 100
}
}References
- Enforce defensive programming by ensuring appropriate null/nil checks exist before object property accesses.
There was a problem hiding this comment.
Intentionally not added. These four are unexported helpers whose only callers are:
NvidiaGPUDevices.Fit— the production path. Inside the main loopdev := devices[i]cannot be nil unless the caller ofFitputs nil entries in the input slice (which would be a caller bug we want surfaced, not silently swallowed).ctxis constructed inline in the same loop, andctx.pod/ctx.requestare assigned fromFit's own parameters.- The unit tests in
fit_checks_test.go/fit_extra_test.go, which build the inputs explicitly and would not benefit from a nil guard either.
Adding nil checks on private helpers would convert caller bugs into silent failures (e.g. returning common.CardNotHealth for a nil dev would mislabel the failure mode and skew reason counters). It also does not match the prevailing defensive-programming style elsewhere in pkg/device (e.g. checkType, CustomFilterRule, AddResourceUsage all dereference their pointer parameters without nil guards).
If the maintainers prefer the defensive style project-wide, happy to revisit — but it would be a broader change across pkg/device, not specific to this refactor.
![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 (2)
pkg/device/nvidia/fit_bench_test.go (2)
50-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore global state to prevent cross-test pollution.
registerBenchmarkDeviceunconditionally replaces the globaldevice.DevicesMap. While this works in isolated benchmark runs, it can cause unexpected state pollution and failures if other tests or benchmarks that rely on the original map run subsequently.Consider accepting
b *testing.Band usingb.Cleanup()to restore the previous state.♻️ Proposed refactor
-func registerBenchmarkDevice(nv *NvidiaGPUDevices) { +func registerBenchmarkDevice(b *testing.B, nv *NvidiaGPUDevices) { + b.Helper() + old := device.DevicesMap device.DevicesMap = map[string]device.Devices{NvidiaGPUDevice: nv} + b.Cleanup(func() { + device.DevicesMap = old + }) }(Note: If you apply this, you will need to update the four call sites to pass
b:registerBenchmarkDevice(b, nv))🤖 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/fit_bench_test.go` around lines 50 - 52, Update registerBenchmarkDevice to accept *testing.B, save the existing device.DevicesMap before replacing it, and register b.Cleanup to restore that original map after the benchmark. Update all four call sites to pass b when invoking registerBenchmarkDevice.
65-67: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPre-allocate arguments outside the benchmark loop.
Allocating
&device.NodeInfo{}and&device.PodDevices{}inside theb.Nloop introduces garbage collection and allocation overhead, which skews the benchmark's measurement ofFit. Declare these variables outside the loop to ensure you are only measuring the target function's performance.
pkg/device/nvidia/fit_bench_test.go#L65-L67: Extract allocations tonodeInfo := &device.NodeInfo{}andpodDevices := &device.PodDevices{}before the loop, and passnodeInfoandpodDevicesinside the loop.pkg/device/nvidia/fit_bench_test.go#L81-L83: Apply the same extraction outside the loop.pkg/device/nvidia/fit_bench_test.go#L101-L103: Apply the same extraction outside the loop.🤖 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/fit_bench_test.go` around lines 65 - 67, Move the NodeInfo and PodDevices allocations outside each benchmark loop in pkg/device/nvidia/fit_bench_test.go at lines 65-67, 81-83, and 101-103, then pass the reused variables to nv.Fit inside the respective loops. Apply the same change at all three sites.
🤖 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/nvidia/fit_checks_test.go`:
- Around line 242-243: Preserve the package-global device.DevicesMap in all
affected tests by snapshotting its current value before overwriting it and
restoring that snapshot during cleanup. Apply this to both setup blocks in
pkg/device/nvidia/fit_checks_test.go at lines 242-243 and 261-262, and add
equivalent snapshot-and-restore cleanup in pkg/device/nvidia/fit_extra_test.go
at lines 72-73.
---
Nitpick comments:
In `@pkg/device/nvidia/fit_bench_test.go`:
- Around line 50-52: Update registerBenchmarkDevice to accept *testing.B, save
the existing device.DevicesMap before replacing it, and register b.Cleanup to
restore that original map after the benchmark. Update all four call sites to
pass b when invoking registerBenchmarkDevice.
- Around line 65-67: Move the NodeInfo and PodDevices allocations outside each
benchmark loop in pkg/device/nvidia/fit_bench_test.go at lines 65-67, 81-83, and
101-103, then pass the reused variables to nv.Fit inside the respective loops.
Apply the same change at all three sites.
🪄 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: 8c1c577e-5d2f-4c39-8aa5-1399f283d1bf
📒 Files selected for processing (5)
pkg/device/nvidia/device.gopkg/device/nvidia/fit_bench_test.gopkg/device/nvidia/fit_checks.gopkg/device/nvidia/fit_checks_test.gopkg/device/nvidia/fit_extra_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.
🚀 New features to boost your workflow:
|
… PR review Address CodeRabbit feedback on PR Project-HAMi#2090: 1. Snapshot-and-restore device.DevicesMap in three test setup blocks (fit_checks_test.go x2, fit_extra_test.go x1) instead of replacing it with nil on cleanup. This avoids order-dependent state leakage when other tests in the package have previously populated the map. 2. registerBenchmarkDevice now accepts *testing.B and registers b.Cleanup to restore the previous DevicesMap, mirroring the test isolation pattern. 3. Hoist nodeInfo and allocated out of the inner benchmark loops in BenchmarkFit_SingleCardFromEight / FourCardsFromEight / AllCardsFail since they are loop-invariant; this removes ~3 redundant allocations per iteration from the measured path. Gemini code-assist suggested adding defensive nil checks to checkCardHealth, runCardChecks, computeMemreq, and normalizeCoresreq. These were intentionally not added: all four are unexported helpers whose only callers are the Fit method (which constructs ctx in place and iterates a non-nil []*DeviceUsage) and unit tests that build the inputs explicitly. Adding nil guards on private helpers would silently swallow caller bugs that should surface as panics, and does not match HAMi's prevailing style elsewhere in pkg/device. Happy to revisit if the maintainers prefer the defensive style. Signed-off-by: yxxhero <aiopsclub@163.com>
|
@yxxhero The benchmark is useful, but I think we can make it more effective. Currently, it only reports execution time without comparing the results against a baseline, which makes it difficult to identify performance regressions. It would be helpful to store benchmark results from the main branch and compare them with the results from each pull request, showing the percentage difference. We could also introduce a threshold—for example, flagging changes that cause a regression of more than 5%—to help contributors ensure that their changes do not introduce significant performance overhead. The exact threshold and how to account for benchmark variability can be discussed and refined later. |
Conflicts only in pkg/device/nvidia/device.go. Master added a new mutex GPU scheduler policy (hami.io/gpu-scheduler-policy: mutex) that rejects any GPU with dev.Used > 0. Resolution keeps the refactored check pipeline in fit_checks.go and absorbs the new logic as a new pipeline stage: - device.go: add gpuPolicy/isMutex locals; thread isMutex into cardCheckCtx. - fit_checks.go: add isMutex field to cardCheckCtx; add checkCardMutex (returns common.ExclusiveDeviceAllocateConflict when the policy is mutex and the card is in use) and insert it in cardCheckPipeline right after checkCardTimeSlicing to match master's evaluation order. - fit_checks_test.go: add TestCheckCardMutex covering policy-on/policy- off and zero/non-zero Used. Tests pass with -race. gofmt, goimports, license header, and golangci-lint clean. Signed-off-by: yxxhero <aiopsclub@163.com>
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 (1)
pkg/device/nvidia/device.go (1)
838-841: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the slice length instead of the map length to count allocated devices.
tmpDevsis a map of strings to slices (map[string]device.ContainerDevices). Callinglen(tmpDevs)returns the number of keys in the map (which is at most 1 fork.Type), rather than the number of devices actually allocated. This causes the reason count and log messages to incorrectly report1instead of the true count of allocated cards.
pkg/device/nvidia/device.go#L838-L841: Uselen(tmpDevs[k.Type])to correctly record and log the number of devices allocated when the request cannot be fully satisfied.pkg/device/nvidia/device.go#L771-L772: Uselen(tmpDevs[k.Type])to correctly add the number of discarded devices to theNumaNotFitreason count.🛠️ Proposed fixes for the map length evaluations
For
pkg/device/nvidia/device.go#L838-L841:- if len(tmpDevs) > 0 { - reasons[common.AllocatedCardsInsufficientRequest] = len(tmpDevs) - klog.V(5).InfoS(common.AllocatedCardsInsufficientRequest, "pod", klog.KObj(pod), "request", originReq, "allocated", len(tmpDevs)) + allocatedCount := len(tmpDevs[k.Type]) + if allocatedCount > 0 { + reasons[common.AllocatedCardsInsufficientRequest] = allocatedCount + klog.V(5).InfoS(common.AllocatedCardsInsufficientRequest, "pod", klog.KObj(pod), "request", originReq, "allocated", allocatedCount) }For
pkg/device/nvidia/device.go#L771-L772:if k.Nums != originReq { - reasons[common.NumaNotFit] += len(tmpDevs) + reasons[common.NumaNotFit] += len(tmpDevs[k.Type]) klog.V(5).InfoS(common.NumaNotFit, "pod", klog.KObj(pod), "device", dev.ID, "k.nums", k.Nums, "numa", numa, "prevnuma", prevnuma, "device numa", dev.Numa) }🤖 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 838 - 841, The allocated-device counts use the map size instead of the device-slice size. In pkg/device/nvidia/device.go lines 838-841, update the reason count and log field in the insufficient-request handling to use len(tmpDevs[k.Type]); in lines 771-772, update the NumaNotFit discarded-device count to use the same slice length.
🤖 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/nvidia/device.go`:
- Around line 838-841: The allocated-device counts use the map size instead of
the device-slice size. In pkg/device/nvidia/device.go lines 838-841, update the
reason count and log field in the insufficient-request handling to use
len(tmpDevs[k.Type]); in lines 771-772, update the NumaNotFit discarded-device
count to use the same slice length.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bfbd9fe2-6e46-4f8a-8912-bba274b8cadc
📒 Files selected for processing (3)
pkg/device/nvidia/device.gopkg/device/nvidia/fit_checks.gopkg/device/nvidia/fit_checks_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/device/nvidia/fit_checks.go
- pkg/device/nvidia/fit_checks_test.go
| } | ||
|
|
||
| func checkCardCustomRule(dev *device.DeviceUsage, ctx *cardCheckCtx) string { | ||
| if !ctx.nv.CustomFilterRule(ctx.allocated, *ctx.request, ctx.tmpDevsMap[ctx.deviceType], dev) { |
There was a problem hiding this comment.
note: this passes the mutated k copy instead of the original request, harmless now since customfilterrule only reads memreq which never changes here, but revisit if it ever needs nums or coresreq
|
Thx for PR @yxxhero! Reminder: Answers must be written by human being. You can view the relevant rule here. "4. Review replies. The reply you post must be written by you and must address the specific point raised. Verbatim or canned AI replies, or replies that do not engage the comment, lead to the PR being closed." |
| } | ||
|
|
||
| func checkCardCustomRule(dev *device.DeviceUsage, ctx *cardCheckCtx) string { | ||
| if !ctx.nv.CustomFilterRule(ctx.allocated, *ctx.request, ctx.tmpDevsMap[ctx.deviceType], dev) { |
There was a problem hiding this comment.
the old code passed the original request to CustomFilterRule but this passes the mutated k with clamped Coresreq and decremented Nums, harmless today because only Memreq is read but it silently changes what a future rule would see in a pr that claims no behavior change.
|
closed bcs of inactivity of pr owner, it can be reopened if necessary |
3 participants
Summary
The 137-line
Fitfunction inpkg/device/nvidia/device.gointerleaved 13 distinct per-card failure checks in a single for-loop, making the reasoning hard to follow and per-branch unit testing impractical.This refactor extracts the per-card checks into a pipeline of small, independently testable functions in
fit_checks.go. Each returns acommon.ReasonXxxstring on failure or empty on pass.runCardCheckswalks the pipeline and short-circuits on the first failure, preserving the original check order.Motivation
if ... continuebranches compressed into a declarative pipeline.Fitcoverage went from ~70% to 100%; all 11 check functions are at 100%.Changes
pkg/device/nvidia/device.goFitbody went from 137 lines to ~80. The main loop now:checkCardHealthfirst (precedence preserved overcheckType).nv.checkType+ NUMA reset (kept in loop because of cross-card state).normalizeCoresreq+computeMemreq(kept in loop because they mutatek).cardCheckCtxand callsrunCardChecksfor the remaining 8 checks.pkg/device/nvidia/fit_checks.go(new)cardCheckCtx: per-iteration context (request, pod, allocated, tmpDevsMap, deviceIndex, memreq, ...).cardChecktype +cardCheckPipelineslice.runCardChecksorchestrator.checkCardXxxfunctions +checkCardHealth+computeMemreq+normalizeCoresreq.Tests
fit_checks_test.go: unit tests for every check function with table-driven positive/negative cases.fit_extra_test.go: integration tests for previously-uncovered Fit branches (NumaNotFit, ResourceQuotaNotFit, CardNotFoundCustomFilterRule, MIG multi-slot selection, Coresreq>100 clamping, health precedence over type mismatch, etc).fit_bench_test.go: benchmarks for Fit (3 scenarios) and runCardChecks.Zero breaking changes
Fitmethod signaturedevice.DevicesinterfaceTestDevices_Fit(13 cases)pkg/scheduler/score_test.go(indirect Fit caller)The check order is also preserved:
health → checkType → NUMA → UUID → TimeSlicing → normalize → memreq → quota → memory → core → exclusive → computeExhausted → customRule.Known trade-off (disclosed)
normalizeCoresreqnow runs before UUID/TimeSlicing instead of after them. This is functionally safe because:normalizeCoresreqonly readsreq.Coresreqand is idempotent (clamp to 100).computeMemreqonly reads(req.Memreq, req.MemPercentagereq, dev.Totalmem)— it does not depend onCoresreq.k.Coresreq/ctx.memreq; their final values are identical in both orderings.Only observable difference: when every card fails UUID/TimeSlicing and
Coresreq > 100, the original code would never logcore limit can't exceed 100, while the refactored code logs it once. Log-only, no functional change.Coverage report
Benchmarks (8-card node, production-path DevicesMap)
Review process
This PR went through six rounds of self-review before submission. Twenty real issues were caught and fixed, including two genuine bugs (health-check double-invocation, checkType precedence), four fake-green tests (regression guards that did not actually exercise the failure mode their names claimed), and several test-isolation / benchmark-fidelity improvements. Details are documented in the commit message.
AI Assistance Disclosure
This PR was authored with AI assistance (Claude Code) and is disclosed per CONTRIBUTING.md.
Test Plan
go build ./pkg/device/nvidia/...go vet ./pkg/device/nvidia/...go test -short --race ./pkg/device/... ./pkg/scheduler/...— all 19 packages passhack/verify-license.shhack/verify-import-aliases.shhack/verify-staticcheck.sh(golangci-lint v2.8.0)go test -benchruns without regressionSummary by CodeRabbit
Bug Fixes
Tests