fix(ascend): do not overwrite ASCEND_VISIBLE_DEVICES for Ascend containers

[peachest]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Fixes a bug in the ascend overwriteEnv injection (introduced in #1738):
when multiple Ascend chip types are registered, a container that requests one of them still gets ASCEND_VISIBLE_DEVICES clobbered by empty values injected by the other chip instances, so ascend-docker-runtime mounts no device at all.

Which issue(s) this PR fixes:

Fixes #2954

Special notes for your reviewer:

Each registered Ascend chip type has its own Device instance, and the scheduler webhook calls MutateAdmission once per instance. #1738 modeled the injection on the nvidia implementation, which has a single instance, so the per-instance aspect was missed: for a container that requests one chip, the other N-1 instances each take the !ok branch and each append an empty ASCEND_VISIBLE_DEVICES to container.Env.

Why that breaks mounting, based on #1738 and re-validation on a real Ascend
cluster — when Kubernetes starts a container, env entries are appended to
Config.Env in this order:

• variables from the original image;
• variables injected by the device plugin via kubelet;
• variables defined in pod.spec.containers[].env.

ascend-docker-runtime then interprets them as follows:

• ASCEND_VISIBLE_DEVICES: parsed from the end backwards — a later value overrides an earlier one;
• all other variables (including ASCEND_VNPU_SPECS): parsed from the beginning — values defined in the image win.

So the extra empty ASCEND_VISIBLE_DEVICES entries from the other Ascend instances override the device plugin's real value: the pod still schedules, but ascend-docker-runtime does not actually mount the Ascend devices.

The fix (two guards in the !ok branch):

1. skip the injection when the container requests any registered Ascend resource (limits or requests) — the clearing value is only meant for containers that request nothing;
2. make the injection idempotent: skip it when the last ASCEND_VISIBLE_DEVICES entry already equals the value we would inject, so even a multi-instance loop appends at most one entry.

Containers that request no Ascend resource still get exactly one empty ASCEND_VISIBLE_DEVICES — that is the intended overwriteEnv behavior, unchanged by this PR.

Hardware validation:

• Huawei Ascend 910B4 (single physical card), ascend-docker-runtime, k3s v1.33.4, containerd 2.0.5, 7 chip types registered with overwriteEnv: true.
• Before: a pod requesting huawei.com/Ascend910B4: 1 + Ascend910B4-memory: 8192 (vNPU slice vir05_1c_8g) schedules and runs, pod spec carries 6 empty ASCEND_VISIBLE_DEVICES entries, container env shows ASCEND_VNPU_SPECS=vir05_1c_8g but empty ASCEND_VISIBLE_DEVICES, and no /dev/davinci* is mounted.
• After: same pod gets ASCEND_VISIBLE_DEVICES=<phyid> (single value, no duplicates), ASCEND_VNPU_SPECS=vir05_1c_8g, and the assigned /dev/davinci* is mounted.

Does this PR introduce a user-facing change?:

Yes — with multiple Ascend chip types registered and overwriteEnv: true, containers that request an Ascend resource now correctly keep the device plugin's ASCEND_VISIBLE_DEVICES value and get their devices mounted.

---

AI assistance disclosure: this fix was developed with AI assistance for code editing and this description's English wording. The root-cause analysis, reproduction, and hardware validation on a real 910B cluster are my own work.

Summary by CodeRabbit

• Bug Fixes
  • Improved Ascend device environment handling during admission.
  • Prevented empty ASCEND_VISIBLE_DEVICES values from being injected when a container requests Ascend resources.
  • Ensured non-Ascend containers receive the expected empty value when overwrite is enabled.
  • Corrected handling of repeated environment entries and values supplied through references.

[hami-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: peachest
Once this PR has been reviewed and has the lgtm label, please assign dsfans2014 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: c7979e22-5c1f-4c03-9aed-8be3ed041f9f

📥 Commits

Reviewing files that changed from the base of the PR and between be3d184 and 80fa1ca.

📒 Files selected for processing (2)

• pkg/device/ascend/device.go
• pkg/device/ascend/device_test.go

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

---

📝 Walkthrough

Walkthrough

The Ascend device configuration now records all resource names. Admission mutation detects Ascend resource requests and evaluates the last environment-variable entry. Regression tests cover overwrite behavior for Ascend and non-Ascend containers.

Changes

Ascend overwrite-env behavior

Layer / File(s)	Summary
Resource-aware mutation and regression coverage pkg/device/ascend/device.go, pkg/device/ascend/device_test.go	Devices stores all configured Ascend resource names. MutateAdmission skips empty ASCEND_VISIBLE_DEVICES injection for Ascend containers and existing empty literals. Tests cover multiple device passes, last-value behavior, and ValueFrom entries.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 1180a

Ascend-requesting containers retain the device plugin's assigned visibility value, while non-Ascend containers continue to receive a single empty visibility value. No current merge-blocking risk remains.

Poem

A rabbit checks the Ascend gate
No empty value seals a device’s fate
Last entries guide the way
Tests keep stray injections at bay
NPU paths now stay awake

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main fix: preventing ASCEND_VISIBLE_DEVICES from being overwritten for Ascend containers.
Linked Issues check	✅ Passed	The changes address issue #2954. MutateAdmission now detects requests for any registered Ascend resource and skips empty ASCEND_VISIBLE_DEVICES injection for those containers. The added tests cove…
Out of Scope Changes check	✅ Passed	The implementation changes and regression tests are directly related to the ASCEND_VISIBLE_DEVICES injection defect described in issue #2954. No unrelated code changes are indicated.

Full details: Linked Issues check

Explanation

The changes address issue #2954. MutateAdmission now detects requests for any registered Ascend resource and skips empty ASCEND_VISIBLE_DEVICES injection for those containers. The added tests cover multiple chip types, duplicate prevention, last-value behavior, and existing ValueFrom entries.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[DSFans2014]

please fix the lint error

[DSFans2014]

hi @peachest , I noticed that runtimeClassName and overwriteEnv were added under config in #1738. From my perspective, these configurations apply uniformly to all ascend devices. If that's the case, it might be better to move them under vnpu. Please let me know if there are other considerations.

HAMi/charts/hami/templates/scheduler/device-configmap.yaml

Lines 114 to 127 in f47cd28

	vnpus:
	hamiVnpuCore: {{ .Values.devices.ascend.hamiVnpuCore | default false }}
	configs:
	- chipName: 910A
	commonWord: Ascend910A
	resourceName: huawei.com/Ascend910A
	resourceMemoryName: huawei.com/Ascend910A-memory
	resourceCoreName: huawei.com/Ascend910A-core
	memoryAllocatable: 32768
	memoryCapacity: 32768
	memoryFactor: 1
	aiCore: 30
	runtimeClassName: "{{ .Values.devices.ascend.runtimeClassName }}"
	overwriteEnv: {{ .Values.scheduler.overwriteEnv | default "false" }}

[peachest]

hi @peachest , I noticed that runtimeClassName and overwriteEnv were added under config in #1738. From my perspective, these configurations apply uniformly to all ascend devices. If that's the case, it might be better to move them under vnpu. Please let me know if there are other considerations.

HAMi/charts/hami/templates/scheduler/device-configmap.yaml

Lines 114 to 127 in f47cd28

	vnpus:
	hamiVnpuCore: {{ .Values.devices.ascend.hamiVnpuCore | default false }}
	configs:
	- chipName: 910A
	commonWord: Ascend910A
	resourceName: huawei.com/Ascend910A
	resourceMemoryName: huawei.com/Ascend910A-memory
	resourceCoreName: huawei.com/Ascend910A-core
	memoryAllocatable: 32768
	memoryCapacity: 32768
	memoryFactor: 1
	aiCore: 30
	runtimeClassName: "{{ .Values.devices.ascend.runtimeClassName }}"
	overwriteEnv: {{ .Values.scheduler.overwriteEnv | default "false" }}

I think this is a good idea. At the time of #1738, there is no global config for all ascend chips so I put these two fields under each ascend chip. There is another pr #2966 to enable more granular control over the behavior of overwriting env

I am wondering is there a better method to do this only once instead of multiple times in each chips for each container in the MutateAdmission path.

HAMi/pkg/scheduler/webhook.go

Lines 97 to 104 in 9e75c87

	}
	}
	if hasPrivileged && hasResource {
	klog.Warningf(template+" - Denying admission as container %s is privileged", pod.Namespace, pod.Name, pod.UID, privilegedName)
	return admission.Denied(fmt.Sprintf("container %s is privileged", privilegedName))
	}
	if !hasResource {

[codecov]

Codecov Report

❌ Patch coverage is 86.95652% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/device/ascend/device.go	86.95%	3 Missing ⚠️

Flag	Coverage Δ
unittests	70.92% <86.95%> (+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	89.62% <86.95%> (+0.65%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

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

[DSFans2014]

/lgtm