Repository navigation
feat(ascend,hygon): register AI-core/dcucores and support new and legacy vnpus config - #18
Conversation
RunManager called device.Register synchronously, which blocks in its 30s refresh loop and prevents the dpm manager (mockmanager.Run) from ever starting, so Hygon DCU resources were never registered. nvidia and ascend already use 'go device.Register'. (kunlun has the same latent bug but is not currently wired up.) Signed-off-by: wangmin <wangmin@riseunion.io>
…config
- Add ResourceCoreName to VNPUConfig and register huawei.com/Ascend{X}-core
in GetResource: sum of per-card devcore, falling back to the chip-level
aiCore when the annotation omits it; MemoryFactor is not applied to core.
- Add the VNPUs wrapper {hamiVnpuCore, configs} mirroring upstream HAMi,
with a dual-format UnmarshalYAML: prefer the new nested object, fall back
to the legacy flat list, otherwise return the original error.
- Wire config.go to the nested VNPUs and InitDevices(config.VNPUs.Configs).
- Add unit tests for dual-format parsing and core registration.
- README: per-vendor usage guide; Makefile: common dev/build/deploy targets.
Signed-off-by: wangmin <wangmin@riseunion.io>
HAMi consumes hygon.com/dcucores for DCU compute partitioning (GenerateResourceRequests reads it and the scheduler fits on Coresreq vs Totalcore), and the device-config sets resourceCoreName by default, but the mock only registered dcumem. Sum per-card devcore into the core resource when resourceCoreName is configured, mirroring nvidia/ascend. Signed-off-by: wangmin <wangmin@riseunion.io>
The image build runs 'go build' inside the container, which downloads all modules from the default proxy and can be slow on some networks. Add a GOPROXY build-arg (default unchanged) so it can be overridden, e.g.: make docker-build GOPROXY=https://goproxy.cn,direct Signed-off-by: wangmin <wangmin@riseunion.io>
|
@Wangmin362: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Welcome @Wangmin362! It looks like this is your first PR to Project-HAMi/mock-device-plugin 🎉 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds optional core resource registration for Ascend and Hygon devices, introduces dual-format ChangesCore Resource Support and VNPUs Dual-Format
Makefile, Dockerfile GOPROXY, and README
Estimated code review effort: 3 (Moderate) | ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@Makefile`:
- Around line 33-39: The Makefile is missing a standard all target alias, which
causes compatibility issues with tooling that expects make all to work. Add a
thin all target that delegates to the existing build target while keeping
.DEFAULT_GOAL := help unchanged, and place it near the other top-level targets
so it is easy to find alongside help and build.
In `@README.md`:
- Around line 107-122: The Hygon DCU docs in the README still only describe
memory and count resources, so update the Hygon walkthrough and related summary
text to include the new core resource name `hygon.com/dcucores` and clarify when
it is exposed. Adjust the Hygon section that mentions `hygon.com/dcumem`, the
config/annotation notes around `hami.io/node-dcu-register`, and any summary
table entries so they reflect both memory and core resource behavior
consistently.
- Around line 14-20: The diagram block in README.md uses an unlabeled fenced
code block, which markdownlint-cli2 flags. Update the fenced block that contains
the node/register diagram to include a language tag, using the existing diagram
content and marking the fence as text so the README stays lint-clean.
🪄 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: ea4a3401-51c2-4815-a387-75c2d9cd955d
📒 Files selected for processing (11)
.gitignoreDockerfileMakefileREADME.mdinternal/pkg/api/device/ascend/device.gointernal/pkg/api/device/ascend/device_core_test.gointernal/pkg/api/device/ascend/vnpu.gointernal/pkg/api/device/ascend/vnpu_test.gointernal/pkg/api/device/hygon/device.gointernal/pkg/api/device/hygon/device_test.gointernal/pkg/config/config.go
- README: document hygon.com/dcucores in the Hygon usage section, the verify line, and the ManagedResources table (the dcucores change added the resource but not the docs). - README: tag the "How it works" diagram fence as text (markdownlint MD040). - Makefile: add an 'all: build' alias for standard tooling (checkmake). Signed-off-by: wangmin <wangmin@riseunion.io>
|
Thanks for the review. Addressed the three CodeRabbit comments in e138184:
The remaining Disclosure per the AI Assistance Notice: this response and the above changes were prepared with AI assistance and reviewed by me before posting. |
|
@archlitchi PTAL! |
huawei.com/<chip>-core is percentage-based: a whole card is 100. HAMi caps a core request at 100 and, in hami-vnpu-core soft mode, treats a card's total core as 100 regardless of the physical AI-core count. Register 100 per physical card instead of the annotation devcore / chip aiCore. Update unit tests and README accordingly. Also bump the copyright year on this PR's new files to 2026. Signed-off-by: wangmin <wangmin@riseunion.io>
huawei.com/<chip>-core is the soft-partition (hami-vnpu-core) core resource. Previously the mock registered it whenever the chip config set resourceCoreName, but the default config sets resourceCoreName for every Ascend chip while vnpus.hamiVnpuCore defaults to false. As a result the mock advertised -core on hard/template nodes, where the HAMi scheduler filters out any pod that requests -core (ModeNotFit) -- a dead resource. Gate -core registration on whether the node runs in hami-vnpu-core mode, mirroring the real ascend-device-plugin (IsHamiVnpuCore) and the HAMi scheduler's nodeSupportHamiCore: the node hami-vnpu-core annotation takes precedence over the global vnpus.hamiVnpuCore config. Thread the global flag into each ascend Devices via InitDevices. Add unit tests for global-off, node-annotation override on, and override off. Update the README to document the mode gating. Signed-off-by: wangmin <wangmin@riseunion.io>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/pkg/api/device/ascend/device_test.go (1)
96-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the new
HamiVnpuCorewiring here.This still only exercises
Configs, so a regression whereInitDevicesstops copyingVNPUs.HamiVnpuCoreintoDevices.hamiVnpuCorewould pass unnoticed even thoughGetResourcenow depends on that field.Suggested test extension
tests := []struct { name string + hamiVnpuCore bool configs []VNPUConfig expectedCount int expectedCommon []string }{ { name: "single device config", + hamiVnpuCore: true, configs: []VNPUConfig{ { @@ t.Run(tt.name, func(t *testing.T) { - devices := InitDevices(VNPUs{Configs: tt.configs}) + devices := InitDevices(VNPUs{ + HamiVnpuCore: tt.hamiVnpuCore, + Configs: tt.configs, + }) @@ for i, dev := range devices { + if dev.hamiVnpuCore != tt.hamiVnpuCore { + t.Fatalf("device %d: hamiVnpuCore = %v, want %v", i, dev.hamiVnpuCore, tt.hamiVnpuCore) + } if tt.expectedCommon[i] != dev.CommonWord() {🤖 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 `@internal/pkg/api/device/ascend/device_test.go` at line 96, Extend the InitDevices test coverage to include VNPUs.HamiVnpuCore in addition to Configs, so the wiring into Devices.hamiVnpuCore is validated. Update the test setup around InitDevices and the related assertions on Devices to confirm the HamiVnpuCore value is preserved, ensuring GetResource-dependent regressions are caught. Use the InitDevices, VNPUs, and Devices symbols to locate the code path.
🤖 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 `@internal/pkg/api/device/ascend/device_test.go`:
- Line 96: Extend the InitDevices test coverage to include VNPUs.HamiVnpuCore in
addition to Configs, so the wiring into Devices.hamiVnpuCore is validated.
Update the test setup around InitDevices and the related assertions on Devices
to confirm the HamiVnpuCore value is preserved, ensuring GetResource-dependent
regressions are caught. Use the InitDevices, VNPUs, and Devices symbols to
locate the code path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d7622a72-9a43-46c1-afd4-fd28d1f78d42
📒 Files selected for processing (5)
README.mdinternal/pkg/api/device/ascend/device.gointernal/pkg/api/device/ascend/device_core_test.gointernal/pkg/api/device/ascend/device_test.gointernal/pkg/config/config.go
✅ Files skipped from review due to trivial changes (1)
- README.md
DSFans2014
left a comment
DSFans2014
left a comment
There was a problem hiding this comment.
hi @Wangmin362 Have you tested it locally to see if xxx-core resources get reported correctly?
Yes. I deployed the open-source HAMi scheduler together with this mock-device-plugin on a Kubernetes cluster and mocked an Ascend node (no real NPU) via the hami.io/node-register- annotation + count resource, then checked node.status.allocatable and the device-plugin logs. Results: -core value (percentage-based, 100/card): a 2-card register annotation reports …/-memory=65536 and …/-core=200 (2 × 100), independent of the annotation's devcore. ✅ -core gated on hami-vnpu-core mode (node annotation over global vnpus.hamiVnpuCore):
dual-format vnpus: verified on both HAMi v2.9.0 (nested {hamiVnpuCore, configs}) → -core reported in soft mode, and v2.8.1 (legacy flat list, no resourceCoreName) → memory only, no -core, with the "parsed as legacy flat-list format" warning logged. Scheduling: a pod with huawei.com/vnpu-mode: hami-core requesting -core schedules onto a hami-vnpu-core node and starts; on a non-soft node (no -core advertised) the scheduler correctly rejects it with ModeNotFit. NVIDIA/Hygon resources are unaffected. Unit tests (device_core_test.go) cover all four mode combinations. |
Extend TestInitDevices to pass VNPUs.HamiVnpuCore and assert it is copied into each Devices.hamiVnpuCore, so a regression where InitDevices stops propagating the flag (which GetResource now depends on) is caught. Addresses a CodeRabbit review nitpick. Signed-off-by: wangmin <wangmin@riseunion.io>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, 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 |
Part of the HAMi v2.10 roadmap: Project-HAMi/HAMi#1889 (Device Support -- mock-device-plugin support new NPU Template).
What this PR does / why we need it
The mock device plugin had drifted from the device config that the HAMi
scheduler now consumes, so several resources could not be mocked and the
new Ascend
vnpusconfig failed to load. This PR brings it back in line:Ascend AI-core resource. Newer vnpu configs declare
resourceCoreName(e.g.
huawei.com/Ascend910B4-core) and the scheduler fits vNPU pods onthe core request, but the mock only registered
*-memory. Register theAI-core resource as the sum of per-card
devcore, falling back to thechip-level
aiCorewhen the annotation omits it.memoryFactoris notapplied to the core resource.
New + legacy
vnpusconfig compatibility. HAMi movedvnpusfrom aflat list to a nested object
{hamiVnpuCore, configs: [...]}. The mockonly understood the flat list and errored out on the new config. Add a
VNPUswrapper with a dual-formatUnmarshalYAML: prefer the new nestedobject, fall back to the legacy flat list, otherwise return the original
error. Clusters that have not yet upgraded their device config keep working.
Hygon
dcucoresresource. HAMi consumeshygon.com/dcucoresfor DCUcompute partitioning and the device config sets
resourceCoreNamebydefault, but the mock only registered
dcumem. Register the core resource,mirroring nvidia/ascend.
Fix: the Hygon DCU manager never started.
RunManagercalleddevice.Registersynchronously, which blocks in its 30s refresh loop andprevents the dpm manager (
mockmanager.Run) from ever starting, so DCUresources were never registered. Run it in a goroutine, matching the
nvidia and ascend managers.
Also included: unit tests, a per-vendor usage section in the README, a
Makefile with common dev/build/deploy targets, and an optional
GOPROXYDocker build-arg for faster image builds.
How was this validated
go test ./...passes): dual-formatvnpusparsing (newnested, legacy flat, invalid scalar, empty), and core-resource registration
for ascend and hygon (including the no-
resourceCoreNamecase).versions to exercise both
vnpuslayouts, for NVIDIA / Ascend / Hygon:vnpus, ascend entry hasresourceCoreName): the mockregisters both
*-memoryand*-core; Hygon registersdcumemanddcucores; NVIDIA registersgpumem/gpucores/gpumem-percentage.vnpus, noresourceCoreName): the mock parsesthe legacy format (logging the fallback) and registers
*-memoryonly,with no
*-core— confirming backward compatibility.node.status.allocatable,and a pod requesting them is scheduled and starts.
Type of change
/kind feature
Release note
Summary by CodeRabbit