fix (ascend) - validate module pair count and prevent under allocation for ascend 910C - #2369
Conversation
…for Ascend 910C Signed-off-by: Prabal Poddar <prabalpoddar73@gmail.com>
… fix reason reporting Always run computeBestCombination910C for multi-device Ascend 910C requests even when NetworkID is absent or needTopology is false. Update AllocatedCardsInsufficientRequest reason to report the selected NPU count and pass originReq to GenReason. Signed-off-by: Prabal Poddar <prabalpoddar73@gmail.com>
Verify that computeBestCombination910C returns empty slice and Fit returns fit=false when no candidate NPUs share a full module and NetworkID is absent. Signed-off-by: Prabal Poddar <prabalpoddar73@gmail.com>
Signed-off-by: Prabal Poddar <prabalpoddar73@gmail.com>
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR enforces complete module-pair allocation for multi-device Ascend 910C requests. It rejects short selections, validates exact-count candidates, updates card sorting, and adds regression tests for pairing metadata and no-pair cases. ChangesAscend 910C Pairing Fix
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 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.
🧹 Nitpick comments (1)
pkg/device/ascend/device_910c_pairing_test.go (1)
56-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the insufficient-allocation reason.
This test only asserts
fit == false. It passes ifFitreturns a different failure reason or an empty reason. Assert thatreasoncontainscommon.AllocatedCardsInsufficientRequest.Proposed test update
import ( + "strings" "testing" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/Project-HAMi/HAMi/pkg/device" + "github.com/Project-HAMi/HAMi/pkg/device/common" ) @@ if fit { t.Errorf("expected fit=false when only 2 of 4 requested NPUs form full module pairs, got fit=true, allocated=%d, reason=%q", len(tmpDevs[Ascend910CType]), reason) } + if !strings.Contains(reason, common.AllocatedCardsInsufficientRequest) { + t.Errorf("expected reason to contain %q, got %q", common.AllocatedCardsInsufficientRequest, reason) + }🤖 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/ascend/device_910c_pairing_test.go` around lines 56 - 61, Update the failure assertions in the test around dev.Fit to also verify that reason contains common.AllocatedCardsInsufficientRequest, while preserving the existing fit=false and allocation checks.
🤖 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.
Nitpick comments:
In `@pkg/device/ascend/device_910c_pairing_test.go`:
- Around line 56-61: Update the failure assertions in the test around dev.Fit to
also verify that reason contains common.AllocatedCardsInsufficientRequest, while
preserving the existing fit=false and allocation checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f6408c61-bf15-453b-9cba-ce58d597555c
📒 Files selected for processing (2)
pkg/device/ascend/device.gopkg/device/ascend/device_910c_pairing_test.go
Apply De Morgan's law to QF1001 staticcheck findings in Fit(). Add AllocatedCardsInsufficientRequest reason checks to 910C pairing tests. Signed-off-by: Prabal Poddar <prabalpoddar73@gmail.com>
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 20 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
/assign |
…nd tests Signed-off-by: Prabal Poddar <prabalpoddar73@gmail.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, princexpoddar 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 type of PR is this?
/kind bug
What this PR does / why we need it:
while going through ascend 910c device allocation logic in pkg/device/ascend/device.go...I found a issue in how multi NPU requests are validated during scheduling:
-when a pod requested multiple NPUs ( lets say 4 NPUs) on a node with 1 full card pair(2 NPUs) and other partial single NPU available... computeBestCombination910C filtered out partial cards and returned only the full pair found(2NPUs) and fit() assigned this result to tmpDevs and unconditionally returned true,thus causing under allocation of 2NPUs for a 4 NPU request while reporting success
-when the raw candidature NPU count happened to equal originReq (lets say asking for 2 NPUs on a node with 2 available NPUs on separate cards), Fit() returned true early without invoking computeBestCombination910C() whenever needTopology was false or NetworkID was missing..thus allowing candidate NPUs on separate cards to bypass module pairing validation completely
-In computeBestCombination910C() cards were sorted in ascending order by available NPU count which contradicted the comment directly above it stating full cards should be preferred first
To resolve these,
I have updated Fit() to verify that len(combination) == int(originReq) after module filtering and if sufficient complete module pairs cannot satsfy the request it now returns fit=false with AllocatedCardsInsufficientRequest instead of reporting success on a partial allocation
Also ensured module-pair validation always runs for multi-device Ascend910CType requests regardless of whether NetworkID is present
Updated common.GenReason parameter passing and failure tracking so Kubernetes logs and reason strings accurately report requested versus allocated NPU counts.
Changed card sorting in computeBestCombination910C() to descending order so fully available cards are evaluated before partially available ones.
Added regression tests in pkg/device/ascend/device_910c_pairing_test.go covering
-partial allocation rejection (TestAscend910C_FitPartialAllocationBug)
-candidate-count bypass prevention (TestAscend910C_FitExactCountBypassBug)
-successful full-pair allocation (TestAscend910C_FitFullPairSucceeds)
-missing NetworkID handling (TestAscend910C_FitWithoutNetworkID_ValidatesPairing)
-empty combination handling when no candidate NPUs form a complete module (TestComputeBestCombination910C_NoFullPairsReturnsEmpty).
Which issue(s) this PR fixes:
Fixes #2268
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
No
Summary by CodeRabbit
Bug Fixes
Tests