Feat: Allow mode-agnostic Ascend pods to schedule across both vNPU-template and HAMi-core nodes - #2035
Conversation
📝 WalkthroughWalkthroughAscend runtime annotations now include memory and core values for non- ChangesAscend mode-agnostic scheduling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: 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 allows mode-agnostic (annotation-less) pods to be scheduled on hami-core nodes by removing the strict mode-fit filtering, and updates the container lifecycle injection to use a self-detecting PostStart hook. It also updates annotation patching to include memory and core allocations. The review feedback highlights a critical issue where setting info.Core to 0 for these mode-agnostic pods can prevent the limiter process from enforcing resource limits. It is recommended to fallback to the template's or device's default AICore value when Usedcores is 0, and to update the corresponding unit tests to assert this behavior.
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.
94d61e6 to
a075fae
Compare
![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/device/ascend/device.go`:
- Around line 151-168: The new PostStart hook in device.go unconditionally uses
a sh-based ExecAction for Ascend containers, which can break shell-less or
distroless images when the limiter is absent. Update the hook logic around the
self-detecting hami-vnpu-core limiter injection so it is only added when the
container image can actually execute a shell, or replace the shell command with
a shell-free entrypoint; keep the no-op path safe by guarding the setup in the
same area that checks VNPUModeAnnotation, VNPUModeHamiCore, and
ctr.Lifecycle.PostStart.
🪄 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: 908a767d-51d0-409f-9840-e2d1adc16cfc
📒 Files selected for processing (2)
pkg/device/ascend/device.gopkg/device/ascend/device_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:
|
|
please resolve ai-comments and conflicts |
|
the branch is out of sync with current master. also: line 185 in the PR branch has the 'what this PR does / why we need it' section in the PR description is empty. no ai assistance disclosure is present. if any ai tool was used, it must be disclosed per CONTRIBUTING.md: https://github.com/Project-HAMi/HAMi/blob/master/CONTRIBUTING.md#ai-assistance-notice |
a075fae to
a7c8881
Compare
…te and HAMi-core nodes Make an annotation-less Ascend pod mode-agnostic so its effective mode follows the node it lands on, while pods pinned via huawei.com/vnpu-mode keep today's behavior: - Fit: only reject a hami-core-requested pod on a node that does not support it; unannotated (legacy vNPU) pods are allowed on both node types. Composes with the existing whole-card / memory-less mode gate. - PatchAnnotations: emit a superset RuntimeInfo (template name plus raw memory/core) so the device plugin picks the right shape per node. Limiter startup is no longer injected here: the scheduler-side PostStart hook was removed upstream in Project-HAMi#2062, so this change is scheduling-only. Fixes Project-HAMi#2034 Signed-off-by: ouyangluwei(riseunion) <ouyangluwei@riseunion.io>
a7c8881 to
73a86b3
Compare
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/ascend/device_test.go (1)
915-946: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMissing self-detecting PostStart hook.
The PR objectives explicitly require injecting a "self-detecting PostStart hook that starts the limiter only when its binary is present". However, the AI summary and the updated test cases (expecting
wantPostStart: falseforhami-core) indicate that thePostStartlifecycle injection was completely removed fromMutateAdmissioninstead of being updated.If the injection was removed to bypass the previous review feedback regarding shell-less images, this leaves the limiter unstarted on hami-core nodes. Please restore the
PostStarthook injection and update it to safely handle shell-less images (e.g., by using a shell-free entrypoint or checking for shell availability) to ensure the PR's core requirements are fulfilled.🤖 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_test.go` around lines 915 - 946, Restore PostStart lifecycle injection in MutateAdmission for hami-core containers, with self-detection that starts the limiter only when its binary is present. Ensure the hook remains safe for shell-less images by using a shell-independent execution path or explicitly handling shell absence, and update the related test expectation from wantPostStart false to the required injected-hook behavior.
🤖 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/ascend/device_test.go`:
- Around line 915-946: Restore PostStart lifecycle injection in MutateAdmission
for hami-core containers, with self-detection that starts the limiter only when
its binary is present. Ensure the hook remains safe for shell-less images by
using a shell-independent execution path or explicitly handling shell absence,
and update the related test expectation from wantPostStart false to the required
injected-hook behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 06c32351-f07c-459b-a85a-77c92852068f
📒 Files selected for processing (2)
pkg/device/ascend/device.gopkg/device/ascend/device_test.go
|
Two pods scheduled onto the same Ascend 910B4, both running the same continuous matmul load: Pod A:
huawei.com/Ascend910B4: 1
huawei.com/Ascend910B4-memory: 16384
# no -core requested
Pod B :
huawei.com/Ascend910B4: 1
huawei.com/Ascend910B4-memory: 16384
huawei.com/Ascend910B4-core: 30With both pods saturating the shared card (npu-smi AICore 100%), measured throughput is ~1:1 (~49 vs ~49 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, ouyangluwei163 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 |
4 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #2034
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Summary by CodeRabbit
hami-coreimmediately when the node can’t support it.hami-corefitting/admission behavior.