feat(ascend): support 910C vNPU templates vir05_1c_16g and vir10_3c_32g#2005
Conversation
Ascend HDK 26.0.RC1 enables NPU virtualization (vNPU) on the 910C. The 910C was previously registered without any vNPU templates, so it could only be allocated as a whole card and fractional (memory/core) requests could not be scheduled. Add the vir05_1c_16g (16GB/5 aiCore/1 aiCPU) and vir10_3c_32g (32GB/10 aiCore/3 aiCPU) templates to the scheduler device config, and add examples/ascend/job-910C.yaml demonstrating a vNPU request. Signed-off-by: ouyangluwei(riseunion) <ouyangluwei@riseunion.io>
📝 WalkthroughWalkthroughAdds ChangesAscend 910C vNPU Template Support
Estimated code review effort: 3 (Moderate) | ~20 minutes 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 vNPU templates for Ascend devices in the scheduler configuration and provides an example job configuration for Ascend910C. Feedback points out a critical bug where vNPU requests for Ascend910C are incorrectly rounded up to 2 NPUs during admission mutation, and suggests code modifications to bypass this rounding and adjust the scheduling logic for fractional vNPU requests.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
examples/ascend/job-910C.yaml (2)
12-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComment "requesting 1 NPU" is misleading given auto-adjustment behavior.
Per
pkg/device/ascend/device.goMutateAdmission, whencommonWord == Ascend910CTypeand the request is 1, HAMi silently rounds it up to 2 (minimum allocation unit is one physical module = 2 NPUs). The example's comment says "requesting 1 NPU" without mentioning this auto-adjustment, which could confuse users about the actual resulting allocation.📝 Suggested comment clarification
- huawei.com/Ascend910C: 1 # requesting 1 NPU + huawei.com/Ascend910C: 1 # requesting 1 NPU (auto-adjusted to 2, the minimum allocation unit)🤖 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 `@examples/ascend/job-910C.yaml` around lines 12 - 16, Update the example comment in the Ascend 910C job manifest so it reflects the behavior in MutateAdmission for Ascend910CType: a request of 1 is automatically rounded up to 2 NPUs. Clarify the note near the huawei.com/Ascend910C resource to mention the minimum physical module allocation and that the scheduler will adjust the request, so users understand the actual allocated count.
1-16: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueStatic analysis flags root/privilege-escalation defaults on example manifest.
Checkov flags missing
allowPrivilegeEscalation: falseand non-rootsecurityContext(CKV_K8S_20, CKV_K8S_23). Since this is a demo/example manifest, this is low priority, but adding a minimalsecurityContextwould align with security best practices for example docs that users may copy verbatim.🤖 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 `@examples/ascend/job-910C.yaml` around lines 1 - 16, The example Pod manifest is missing the basic container hardening settings flagged by Checkov. Update the ubuntu-container spec to include a minimal securityContext with allowPrivilegeEscalation set to false and runAsNonRoot enabled (and, if needed, a non-root runAsUser) so the example aligns with safer defaults while keeping the existing Pod and resources fields intact.Source: Linters/SAST tools
🤖 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 `@examples/ascend/job-910C.yaml`:
- Around line 12-16: Update the example comment in the Ascend 910C job manifest
so it reflects the behavior in MutateAdmission for Ascend910CType: a request of
1 is automatically rounded up to 2 NPUs. Clarify the note near the
huawei.com/Ascend910C resource to mention the minimum physical module allocation
and that the scheduler will adjust the request, so users understand the actual
allocated count.
- Around line 1-16: The example Pod manifest is missing the basic container
hardening settings flagged by Checkov. Update the ubuntu-container spec to
include a minimal securityContext with allowPrivilegeEscalation set to false and
runAsNonRoot enabled (and, if needed, a non-root runAsUser) so the example
aligns with safer defaults while keeping the existing Pod and resources fields
intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 014964da-43df-4e85-8dfe-f8e1bede193f
📒 Files selected for processing (2)
charts/hami/templates/scheduler/device-configmap.yamlexamples/ascend/job-910C.yaml
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 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
The 910C round-up-to-2 logic assumed every 910C is a SuperPod module (minimum allocation unit of 2 NPUs). This forced fractional/odd vNPU requests up to 2 NPUs, making vir05_1c_16g and vir10_3c_32g slices unschedulable on non-SuperPod cards. Add a superPod flag to VNPUConfig (default false) and only apply the round-up when it is set, so vNPU memory slices and odd NPU counts are preserved. Enable superPod on the 910C entry in the scheduler device config and cover both paths with Test_MutateAdmission910C_VNPUSplit. Signed-off-by: ouyangluwei(riseunion) <ouyangluwei@riseunion.io>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/device/ascend/device.go (1)
134-149: 🎯 Functional Correctness | 🟠 MajorGating logic looks correct in isolation, but see cross-file config concern.
The added
dev.config.SuperPodcondition correctly matches the intent described in the commit message (round-up/odd-reject only applies when SuperPod is enabled) and is consistent with all updated/new tests indevice_test.go. However, this behavior directly depends on howsuperPodis set per-entry incharts/hami/templates/scheduler/device-configmap.yaml— see the companion comment onpkg/device/ascend/vnpu.goLine 40 regarding a potential contradiction betweensuperPod: trueand the newly added fractional templates on the same Ascend910C entry.🤖 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.go` around lines 134 - 149, The Ascend910C gating in the device allocator is fine, but it depends on the per-entry SuperPod setting being consistent with the chart config. Update the device config generation path so the Ascend910C entry does not combine dev.config.SuperPod=true with fractional templates on the same entry; make the rendered config match the intended SuperPod-only rounding/rejection behavior used by the logic in the Ascend device handling. Verify the config source that populates dev.config.SuperPod and the templates feeding it stay aligned for the Ascend910C resource entry.
🤖 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.
Duplicate comments:
In `@pkg/device/ascend/device.go`:
- Around line 134-149: The Ascend910C gating in the device allocator is fine,
but it depends on the per-entry SuperPod setting being consistent with the chart
config. Update the device config generation path so the Ascend910C entry does
not combine dev.config.SuperPod=true with fractional templates on the same
entry; make the rendered config match the intended SuperPod-only
rounding/rejection behavior used by the logic in the Ascend device handling.
Verify the config source that populates dev.config.SuperPod and the templates
feeding it stay aligned for the Ascend910C resource entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 13c4f117-da55-4a82-8e10-c60efa86dd65
📒 Files selected for processing (4)
charts/hami/templates/scheduler/device-configmap.yamlpkg/device/ascend/device.gopkg/device/ascend/device_test.gopkg/device/ascend/vnpu.go
🚧 Files skipped from review as they are similar to previous changes (1)
- charts/hami/templates/scheduler/device-configmap.yaml
|
[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 |
3 participants
Ascend HDK 26.0.RC1 enables NPU virtualization (vNPU) on the 910C. The 910C was previously registered without any vNPU templates, so it could only be allocated as a whole card and fractional (memory/core) requests could not be scheduled.
Add the vir05_1c_16g (16GB/5 aiCore/1 aiCPU) and vir10_3c_32g (32GB/10 aiCore/3 aiCPU) templates to the scheduler device config, and add examples/ascend/job-910C.yaml demonstrating a vNPU request.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #2004
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Summary by CodeRabbit