fix(ascend): reject -core on hard-split vNPU and fix "nor"->"not" typo - #2026
Conversation
On Ascend hard split the compute is fixed by the driver template, so a requested -core is reserved by the scheduler (Fit) but dropped by PatchAnnotations and never delivered to the container. When the request also exceeds the physical core count the pod stays Pending with CardInsufficientCore for no visible reason. Reject the -core request at admission when the pod is not in hami-core (soft split) mode, so the invalid combination fails fast with a clear message instead of being silently mis-accounted. Signed-off-by: wangmin <wangmin@riseunion.io>
…ssage 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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAscend admission now rejects non-zero core resource requests unless the pod uses hami-core mode, checks both requests and limits, updates one rejection message, and adds unit coverage for hard-split and hami-core outcomes. ChangesCore resource admission validation
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Pod
participant MutateAdmission
participant Container
Pod->>MutateAdmission: Submit pod with -core request
MutateAdmission->>Container: Inspect Limits and Requests for core resource
alt not hami-core mode and core > 0
MutateAdmission-->>Pod: Reject admission with error
else hami-core mode or core == 0
MutateAdmission-->>Pod: Allow admission
end
Possibly related issues
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 adds validation to reject -core requests when not in hami-core (soft split) mode, as they are only supported in that mode. It also fixes a typo in an error message and adds a corresponding unit test. The feedback suggests removing redundant local constants in the new test since they are already defined at the package level.
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:
|
Test_MutateAdmission_HardSplitCoreRejected redeclared VNPUModeAnnotation and VNPUModeHamiCore locally; use the package-level constants from device.go instead. Signed-off-by: wangmin <wangmin@riseunion.io>
Signed-off-by: wangmin <wangmin@riseunion.io>
6e832c2 to
85806d3
Compare
|
/retest |
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, FouoF, 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 |
5 participants
Fixes #2025
What this changes
On Ascend hard split the compute of a vNPU is fixed by the driver template, so a per-container
-corerequest cannot be honored. Today it is dropped byPatchAnnotationsbut still reserved by the scheduler (Fit), and when it exceeds the physical core count the pod staysPendingwithCardInsufficientCorefor no visible reason.This PR rejects the invalid combination at admission:
MutateAdmission: when the pod is not inhami-core(soft split) mode and a<resource>-corelimit/request > 0 is present, admission fails with<resource>-core is only supported in hami-core (soft split) mode.hami-core) with-coreis unchanged.-coreis unchanged.Also folds in a one-word typo fix: the multi-device rejection message
"vNPU nor supported for multiple devices"→"not".Commits
fix(ascend): reject -core request on hard-split vNPU— fix + unit testTest_MutateAdmission_HardSplitCoreRejected.fix(ascend): correct typo "nor" -> "not" in multi-device rejection message.Testing
Unit (
go test ./pkg/device/ascend/...): all pass, incl. the new table test covering hard-split core (rejected), hard-split core=0 / no-core (allowed), and hami-core soft split with core (allowed).Hardware validation (CONTRIBUTING.md gate 2):
hami.io/node-register-Ascend910B4is read fine by the open-source scheduler).mem 16384 + core 10(≤20), hard splithuawei.com/Ascend910B4-core is only supported in hami-core (soft split) modemem 16384 + core 25(>20), hard splitCardInsufficientCoremem 8192, hard split, no corevir05_1c_8g)temp=vir05_1c_8g(unchanged)mem + core,hami-coreannotation...nor...vNPU not supported for multiple devicesServer-side webhook log:
AI assistance disclosure
Per CONTRIBUTING.md: this change was investigated and implemented with the assistance of Claude Code. The root-cause analysis is based on my own hardware testing on an 8×910B4 node (6 rounds). I understand the code and can explain and defend it. AI helped navigate the codebase, draft the fix, and write the tests; I reviewed and verified everything, including the hardware validation above.
Summary by CodeRabbit
Behavior change & compatibility note
hami-coremode, or pods that omit the core request or set it to 0, are unaffected.Bug Fixes
hami-core.Tests
hami-corecore-resource admission outcomes.