fix: require Ascend core resource for soft slicing by fyuan1316 · Pull Request #2024 · Project-HAMi/HAMi

fyuan1316 · 2026-07-07T16:00:14Z

Summary

This PR adds an admission-time validation for Ascend soft slicing.

When a pod explicitly selects HAMi soft slicing with:

metadata:
  annotations:
    huawei.com/vnpu-mode: hami-core

and the Ascend device config defines a resourceCoreName, the workload must request the corresponding core resource together with the memory resource. Otherwise the admission webhook now rejects the pod with a clear error.

This prevents memory-only soft-slicing requests from silently entering the template-matching path where the actual allocated memory can be greater than the requested value.

Changes

Validate hami-core Ascend workloads in MutateAdmission.
Require the configured resourceCoreName when soft slicing is requested.
Keep the validation scoped to configs that define a core resource, so legacy configs without resourceCoreName are not affected.
Add a unit test for the memory-only rejection case.

Test

go test ./pkg/device/ascend
go test ./pkg/device
go test ./pkg/scheduler
git diff --check

Summary by CodeRabbit

Bug Fixes
- Strengthened admission checks for vNPU core mode: containers are now rejected unless they specify a positive quantity for the configured core resource in either resource limits or requests (not just memory).
- Updated the rejection message to require requesting the configured core resource together with the corresponding memory resource.
Tests
- Expanded vNPU core mode tests with new failure cases for missing core requests and core requests set to zero, including assertions on the expected error content.

hami-robot · 2026-07-07T16:00:24Z

Welcome @fyuan1316! It looks like this is your first PR to Project-HAMi/HAMi 🎉

coderabbitai · 2026-07-07T16:01:22Z

📝 Walkthrough

Walkthrough

This PR adds hami-core vNPU admission validation that rejects requests missing a positive configured core resource while requesting memory. It also updates tests to cover the new rejection paths and error matching.

Changes

hami-core vNPU validation

Layer / File(s)	Summary
Admission core check `pkg/device/ascend/device.go`	`MutateAdmission` now rejects hami-core admissions when the configured core resource is missing or non-positive, and a helper checks `Limits` and `Requests` for that resource.
Admission tests `pkg/device/ascend/device_test.go`	The vNPU admission test cases add expected error matching, cover memory-only and zero-core failures, and assert `false` plus the error substring for rejection cases.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

Project-HAMi/HAMi#2027 — Matches the same Ascend hami-core admission rule requiring an explicit core resource request.
Project-HAMi/HAMi#2025 — The change tightens MutateAdmission validation for vNPU core requests.
Project-HAMi/ascend-device-plugin#101 — The change enforces the “request both memory and core” rule for Ascend workloads.

Suggested labels: kind/bug, lgtm, approved

Suggested reviewers: FouoF, archlitchi

Poem

A rabbit hops with careful grace,
Core and memory now share the space.
If core is missing, tests say nay,
The gate stays shut, no run today.
Hop-hop! The checks are in their place 🐇

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: requiring the Ascend core resource for soft slicing admission.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

gemini-code-assist

Code Review

This pull request introduces a validation check for Ascend soft slicing in HAMi-core mode, ensuring that the core resource is requested alongside the memory resource. It also adds a helper method isCoreResourceRequested to verify these resource limits and requests, and includes corresponding unit tests to validate the rejection of memory-only requests. There are no review comments, so no additional feedback is provided.

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.

Signed-off-by: Yuan Fang <yuanfang@alauda.io>

hami-robot · 2026-07-07T16:44:34Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: fyuan1316

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

DSFans2014

/lgtm

codecov · 2026-07-08T09:00:59Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag	Coverage Δ
unittests	`59.55% <100.00%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
pkg/device/ascend/device.go	`83.52% <100.00%> (+0.30%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

archlitchi · 2026-07-08T09:46:45Z

I don't think this is the right way to do so, because we will support compete mode for HAMi-core vNPU mode, like we do for NVIDIA, task without 'core' resource will compete on computing cores with each other

fyuan1316 · 2026-07-09T00:23:43Z

I don't think this is the right way to do so, because we will support compete mode for HAMi-core vNPU mode, like we do for NVIDIA, task without 'core' resource will compete on computing cores with each other

@archlitchi:
Thanks for the clarification. I agree with your point.

After checking the design and implementation of other device types, especially the NVIDIA path, I agree that this PR is too strict. It would reject valid hami-core workloads that intentionally omit *-core and rely on compute competition.

What I still want to address is the memory-only hami-core case: when a user sets huawei.com/vnpu-mode: hami-core and requests *-memory, the requested memory should be used as the hami-core memory quota, while *-core remains optional.

I think there are two possible ways to implement this:

Extend GenerateResourceRequests to receive Pod context, so Ascend can check the hami-core annotation directly.
Keep the existing interface and add an optional pod-aware request generation path only for devices that need Pod context, falling back to the current GenerateResourceRequests for others.

In both cases:

hami-core + memory only keeps Coresreq=0 and supports compute-compete mode
hami-core + memory + core uses an explicit core quota
non-hami-core workloads keep the existing template-based behavior

Do you think either of these directions is acceptable, or would you suggest a better approach?

archlitchi · 2026-07-09T08:36:18Z

I don't think this is the right way to do so, because we will support compete mode for HAMi-core vNPU mode, like we do for NVIDIA, task without 'core' resource will compete on computing cores with each other

@archlitchi: Thanks for the clarification. I agree with your point.

After checking the design and implementation of other device types, especially the NVIDIA path, I agree that this PR is too strict. It would reject valid hami-core workloads that intentionally omit *-core and rely on compute competition.

What I still want to address is the memory-only hami-core case: when a user sets huawei.com/vnpu-mode: hami-core and requests *-memory, the requested memory should be used as the hami-core memory quota, while *-core remains optional.

I think there are two possible ways to implement this:

Extend GenerateResourceRequests to receive Pod context, so Ascend can check the hami-core annotation directly.

Keep the existing interface and add an optional pod-aware request generation path only for devices that need Pod context, falling back to the current GenerateResourceRequests for others.

In both cases:

hami-core + memory only keeps Coresreq=0 and supports compute-compete mode

hami-core + memory + core uses an explicit core quota

non-hami-core workloads keep the existing template-based behavior

Do you think either of these directions is acceptable, or would you suggest a better approach?

yeah, i think we can use Option 1, passing the pod context is reasonable for this method

archlitchi · 2026-07-09T08:37:51Z

/lgtm cancel

mesutoezdil · 2026-07-09T10:20:27Z


 	if isHAMiCore {
+		if dev.config.ResourceCoreName != "" && !dev.hasPositiveCoreResourceRequest(ctr) {
+			return false, fmt.Errorf("ascend soft slicing requires positive core resource %s to be requested together with %s", dev.config.ResourceCoreName, dev.config.ResourceMemoryName)


pr #2035 removes the entire if isHAMiCore block. if #2035 merges after this, this check will be silently dropped. both prs touch the same region and need to be coordinated before either merges?

hami-robot Bot added the dco-signoff: yes label Jul 7, 2026

hami-robot Bot requested review from chaunceyjiang and lengrongfu July 7, 2026 16:00

hami-robot Bot added the size/M label Jul 7, 2026

gemini-code-assist Bot reviewed Jul 7, 2026

View reviewed changes

coderabbitai Bot added the kind/bug Something isn't working label Jul 7, 2026

fyuan1316 force-pushed the fix/ascend-soft-slice-core-validation branch 3 times, most recently from 108921c to 58b10cd Compare July 7, 2026 16:06

fyuan1316 mentioned this pull request Jul 7, 2026

Clarify Ascend soft-slicing resource semantics when hami-core is set #2027

Open

fix: require Ascend core for soft slicing

b3cee93

Signed-off-by: Yuan Fang <yuanfang@alauda.io>

fyuan1316 force-pushed the fix/ascend-soft-slice-core-validation branch from 58b10cd to b3cee93 Compare July 7, 2026 16:43

coderabbitai Bot added approved lgtm labels Jul 7, 2026

DSFans2014 mentioned this pull request Jul 8, 2026

docs: clarify Ascend soft slicing resource requests Project-HAMi/ascend-device-plugin#102

Open

DSFans2014 reviewed Jul 8, 2026

View reviewed changes

hami-robot Bot assigned DSFans2014 Jul 8, 2026

fyuan1316 temporarily deployed to nvidia July 8, 2026 09:03 — with GitHub Actions Inactive

hami-robot Bot assigned archlitchi Jul 9, 2026

hami-robot Bot removed the lgtm label Jul 9, 2026

mesutoezdil reviewed Jul 9, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: require Ascend core resource for soft slicing#2024

fix: require Ascend core resource for soft slicing#2024
fyuan1316 wants to merge 1 commit into
Project-HAMi:masterfrom
fyuan1316:fix/ascend-soft-slice-core-validation

fyuan1316 commented Jul 7, 2026 •

edited

Loading

Uh oh!

hami-robot Bot commented Jul 7, 2026

Uh oh!

coderabbitai Bot commented Jul 7, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

hami-robot Bot commented Jul 7, 2026

Uh oh!

DSFans2014 left a comment

Uh oh!

codecov Bot commented Jul 8, 2026

Uh oh!

archlitchi commented Jul 8, 2026

Uh oh!

fyuan1316 commented Jul 9, 2026 •

edited

Loading

Uh oh!

archlitchi commented Jul 9, 2026

Uh oh!

archlitchi commented Jul 9, 2026

Uh oh!

mesutoezdil Jul 9, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

fyuan1316 commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Related

Test

Summary by CodeRabbit

Uh oh!

hami-robot Bot commented Jul 7, 2026

Uh oh!

coderabbitai Bot commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

hami-robot Bot commented Jul 7, 2026

Uh oh!

DSFans2014 left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jul 8, 2026

Codecov Report

Uh oh!

archlitchi commented Jul 8, 2026

Uh oh!

fyuan1316 commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

archlitchi commented Jul 9, 2026

Uh oh!

archlitchi commented Jul 9, 2026

Uh oh!

mesutoezdil Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fyuan1316 commented Jul 7, 2026 •

edited

Loading

coderabbitai Bot commented Jul 7, 2026 •

edited

Loading

fyuan1316 commented Jul 9, 2026 •

edited

Loading

mesutoezdil Jul 9, 2026 •

edited

Loading