fix(ascend): skip unhealthy devices in Fit() by Eshiv-Pandey · Pull Request #2241 · Project-HAMi/HAMi

Eshiv-Pandey · 2026-07-31T15:42:58Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

The Ascend device plugin writes Health: false into node annotations when GetDeviceHealth() reports an unhealthy NPU. The scheduler decodes this viaUnMarshalNodeDevices()and uildNodeUsage() copies it to DeviceUsage.Health. However, Ascend.Fit() never checks dev.Health, so unhealthy NPUs remain eligible for allocation.

NVIDIA's Fit() already skips unhealthy devices with common.CardNotHealth. This PR adds the same check to the Ascend backend, placed at the top of the device loop before checkType() matching the NVIDIA pattern exactly, in short adding consistency here :)

Which issue(s) this PR fixes:

None filed.

Special notes for your reviewer:

Scoped to Ascend only ... no changes to serialization, common helpers, or other backends.
The Ascend device plugin always initializes Health for every device (manager.go: Health: health == 0), so this check is safe.

Does this PR introduce a user-facing change?

No. Unhealthy Ascend NPUs are now skipped during scheduling instead of being allocated.

AI assistance disclosure:

I used an AI tool for codebase navigation and review support.

Summary by CodeRabbit

Bug Fixes

Unhealthy devices are now excluded from allocation.
Device selection reports a clear health-related rejection reason when a device is skipped.
Added logging to improve visibility into devices rejected due to health status.
Added coverage to verify that unhealthy devices are not allocated and receive the appropriate rejection reason.

coderabbitai · 2026-07-31T15:43:22Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3f13028f-c994-43f7-b0d8-2f80d8a1536c

📥 Commits

Reviewing files that changed from the base of the PR and between c8cb33a and 88c09ce.

📒 Files selected for processing (2)

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

🚧 Files skipped from review as they are similar to previous changes (2)

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

📝 Walkthrough

Walkthrough

Devices.Fit now filters unhealthy Ascend devices before allocation checks, records CardNotHealth, logs each skipped device, and continues scanning. A test verifies that no unhealthy device is allocated.

Changes

Ascend device health filtering

Layer / File(s)	Summary
Reject unhealthy devices `pkg/device/ascend/device.go`, `pkg/device/ascend/device_test.go`	`Devices.Fit` skips unhealthy devices, records `CardNotHealth`, and logs the device status. The test verifies allocation failure and no selected devices.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Possibly related PRs

Project-HAMi/HAMi#2238: Both changes modify Ascend Devices.Fit failure handling.
Project-HAMi/HAMi#2260: Both changes reject unhealthy devices with CardNotHealth.
Project-HAMi/HAMi#2270: Both changes modify Ascend Devices.Fit validation.

Suggested reviewers: ouyangluwei163

Poem

A rabbit checks each device with care,
Unhealthy cards receive no share.
CardNotHealth records the trail,
Logs explain the allocation fail.
The healthy path remains aware.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	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 change: Ascend Fit() skips unhealthy devices.
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.

✨ Finishing Touches

🧪 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

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

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

pkg/device/ascend/device_test.go (1)
2032-2057: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add a mixed-health allocation case.

This case verifies rejection when every device is unhealthy. It does not verify that Fit skips an unhealthy device and continues scanning. Add a case with an unhealthy device followed by a healthy device, and assert that the healthy device is allocated.
🤖 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 2032 - 2057, Add a
mixed-health test case alongside the existing CardNotHealth scenario, with an
unhealthy device followed by a healthy device that satisfies the request. Assert
Fit succeeds, allocates exactly one device, selects the healthy device ID, and
reports the corresponding successful reason while preserving the existing
all-unhealthy rejection case.

🤖 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/nvidia/calculate_score.go`:
- Around line 218-219: Update pkg/device/nvidia/calculate_score.go lines 218-219
in the P2P link validation to compare normalized P2PLink.Type values or
multiplicity counts, log a mismatch, and return zero when asymmetric link types
are detected. Update pkg/device/nvidia/calculate_score_test.go line 128 to make
the reverse direction contain one TwoNVLINKLinks entry while preserving the
existing zero-score assertions.

---

Nitpick comments:
In `@pkg/device/ascend/device_test.go`:
- Around line 2032-2057: Add a mixed-health test case alongside the existing
CardNotHealth scenario, with an unhealthy device followed by a healthy device
that satisfies the request. Assert Fit succeeds, allocates exactly one device,
selects the healthy device ID, and reports the corresponding successful reason
while preserving the existing all-unhealthy rejection case.

🪄 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: 57acf54b-8a5f-4183-ad86-73d4bc80961c

📥 Commits

Reviewing files that changed from the base of the PR and between c7891de and f0a3fc3.

📒 Files selected for processing (4)

pkg/device/ascend/device.go
pkg/device/ascend/device_test.go
pkg/device/nvidia/calculate_score.go
pkg/device/nvidia/calculate_score_test.go

The Ascend device plugin writes Health: false into node annotations when GetDeviceHealth() reports an unhealthy NPU. The scheduler decodes this and buildNodeUsage() copies it to DeviceUsage.Health. However, ascend.Fit() never checked dev.Health, allowing unhealthy NPUs to remain eligible for allocation. Add a dev.Health check at the top of the device loop, mirroring the existing nvidia.Fit() pattern. Unhealthy devices are skipped with reason CardNotHealth. Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>

mesutoezdil

checked that only the nvidia backend has this check today so the gap is real, and the fix mirrors that pattern exactly.

mesutoezdil · 2026-08-02T12:02:31Z

/lgtm

The nvidia backend already checks dev.Health before allocating a device. The same guard was missing from all other backends: amd, awsneuron, biren, cambricon, enflame (device.go + gcu.go), hygon, iluvatar, kunlun (device.go + vdevice.go), metax, mthreads, and vastai. For loop-based backends, add the dev.Health check at the top of the device loop before any other checks, mirroring the nvidia pattern. For graph-select backends (kunlun), add the check inside the fitness functions (FitXPU, FitVXPU) so unhealthy devices are excluded from graph selection. For awsneuron, add the check in both the single-device loop path and inside continuousDeviceAvailable() for multi-device topology-aware selection. Add a CardNotHealth test case for each touched backend that already has a Fit test table (amd, awsneuron, biren, cambricon, enflame/gcu, hygon, iluvatar, kunlun/vdevice, metax, mthreads, vastai). Ascend is handled separately in Project-HAMi#2241. Follows up on Project-HAMi#2241 which fixed the same gap for ascend. Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>

pkg/device/ascend/device.go and device_test.go changes duplicate PR Project-HAMi#2241 which already has LGTM. Reverting to master to avoid conflict. Signed-off-by: Eshiv-Pandey <eshivpandey@gmail.com>

pkg/device/ascend/device.go and device_test.go changes duplicate PR Project-HAMi#2241 which already has LGTM. Reverting to master to avoid conflict. Signed-off-by: Eshiv-Pandey <eshivpandey@gmail.com> Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>

codecov · 2026-08-03T04:03:04Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag	Coverage Δ
unittests	`60.78% <100.00%> (+0.10%)`	⬆️

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.15%)`	⬆️

... 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.

archlitchi · 2026-08-03T04:32:33Z

please resolve these conflicts

The nvidia backend already checks dev.Health before allocating a device. The same guard was missing from all other backends: amd, awsneuron, biren, cambricon, enflame (device.go + gcu.go), hygon, iluvatar, kunlun (device.go + vdevice.go), metax, mthreads, and vastai. For loop-based backends, add the dev.Health check at the top of the device loop before any other checks, mirroring the nvidia pattern. For graph-select backends (kunlun), add the check inside the fitness functions (FitXPU, FitVXPU) so unhealthy devices are excluded from graph selection. For awsneuron, add the check in both the single-device loop path and inside continuousDeviceAvailable() for multi-device topology-aware selection. Add a CardNotHealth test case for each touched backend that already has a Fit test table (amd, awsneuron, biren, cambricon, enflame/gcu, hygon, iluvatar, kunlun/vdevice, metax, mthreads, vastai). Ascend is handled separately in Project-HAMi#2241. Follows up on Project-HAMi#2241 which fixed the same gap for ascend. Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>

Resolve merge conflict in pkg/device/ascend/device_test.go by keeping both test cases: - 'fit fail: CardNotHealth' (from PR branch) - tests skipping unhealthy devices in Fit() - 'fit fail: partial allocation AllocatedCardsInsufficientRequest for multiple cards' (from master) - tests partial allocation Both test cases pass successfully.

mesutoezdil · 2026-08-03T12:34:50Z

for future: please refrain from force pushing once you have started getting reviews, it makes it much harder to tell what has changed from review to review

mesutoezdil · 2026-08-03T12:34:55Z

/lgtm

Eshiv-Pandey · 2026-08-03T13:28:55Z

Would keep in mind. Thanks!

archlitchi

/lgtm

archlitchi

/lgtm

hami-robot · 2026-08-04T02:32:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: archlitchi, Eshiv-Pandey

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~~ [archlitchi]

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

* fix: skip unhealthy devices in Fit() for all non-nvidia backends The nvidia backend already checks dev.Health before allocating a device. The same guard was missing from all other backends: amd, awsneuron, biren, cambricon, enflame (device.go + gcu.go), hygon, iluvatar, kunlun (device.go + vdevice.go), metax, mthreads, and vastai. For loop-based backends, add the dev.Health check at the top of the device loop before any other checks, mirroring the nvidia pattern. For graph-select backends (kunlun), add the check inside the fitness functions (FitXPU, FitVXPU) so unhealthy devices are excluded from graph selection. For awsneuron, add the check in both the single-device loop path and inside continuousDeviceAvailable() for multi-device topology-aware selection. Add a CardNotHealth test case for each touched backend that already has a Fit test table (amd, awsneuron, biren, cambricon, enflame/gcu, hygon, iluvatar, kunlun/vdevice, metax, mthreads, vastai). Ascend is handled separately in #2241. Follows up on #2241 which fixed the same gap for ascend. Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com> * fix: address CodeRabbit suggestions and CI lint failures - Fix duplicate Health field in awsneuron DeviceInfo struct literal - Remove invalid Health field from ContainerDevice/ContainerDeviceRequest literals (not valid fields on those types) - Fix undefined 'reason' var in iluvatar TestPatchAnnotations - Add wantReason assertion to iluvatar Test_Fit loop - Add Health: true to kunlun positive FitVXPU fixtures (Go zero-inits omitted bool to false, causing health check to reject valid fixtures) - Add CardNotHealth test cases for amd, biren, cambricon, enflame, hygon, iluvatar, kunlun, metax, mthreads, vastai, awsneuron backends Signed-off-by: Eshiv-Pandey <eshivpandey@gmail.com> Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com> * test: fix broken tests after health check added to Fit() After adding the !dev.Health early-exit to Fit() across all non-nvidia backends, two test files had devices without Health: true set: - enflame/device_test.go: TestFit_SelectProfileByRequest, TestFit_SelectProfileByMemoryCoreRequest, and TestFit_MutexRejectsUsedDevice used DeviceUsage structs with Health defaulting to false, causing the health gate to fire before the logic under test. Set Health: true on those devices. - iluvatar/device_test.go: Test_Fit existing failure-path cases had no wantReason set (empty string), but the test loop now checks reason unconditionally. Added the correct expected reason strings: CardInsufficientMemory, CardTypeMismatch, ExclusiveDeviceAllocateConflict. Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com> * test: add Health: true to graphSelect test fixtures after health check added to Fit() The !dev.Health early-exit added to Fit() and continuousDeviceAvailable() caused graphSelect test fixtures to fail because DeviceUsage.Health defaults to false in Go, making every device appear unhealthy. Add Health: true to all DeviceUsage fixtures in: - awsneuron/device_test.go: Test_graphSelect - kunlun/device_test.go: Test_graphSelect, Test_graphSelectVXPU, TestKunlunVDevices_Fit_Mutex Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com> * test: add CardNotHealth coverage for enflame and restore health check lost in rebase The rebase onto latest master dropped two changes from the original PR: 1. The !dev.Health check in enflame/device.go Fit() was lost 2. Health: true on test fixtures in enflame/device_test.go was lost This commit restores both and adds a TestFit_UnhealthyDeviceRejected test to bring enflame/device.go patch coverage to 100%. Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com> * refactor(awsneuron): use helper functions for test device construction Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com> * fix(awsneuron): resolve golangci-lint gofmt and modernize issues - Run gofmt -w to fix formatting - Replace for i := 0; i < total; i++ with for i := range 16 to satisfy the modernize linter (range over int, Go 1.22+) Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com> --------- Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com> Signed-off-by: Eshiv-Pandey <eshivpandey@gmail.com>

hami-robot Bot added kind/bug Something isn't working dco-signoff: yes labels Jul 31, 2026

hami-robot Bot requested review from lengrongfu and ouyangluwei163 July 31, 2026 15:43

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

coderabbitai Bot requested a review from mesutoezdil July 31, 2026 15:43

coderabbitai Bot reviewed Jul 31, 2026

View reviewed changes

Comment thread pkg/device/nvidia/calculate_score.go Outdated

Eshiv-Pandey force-pushed the fix/ascend-fit-health-check branch from f0a3fc3 to 14e4e98 Compare July 31, 2026 17:12

mesutoezdil reviewed Jul 31, 2026

View reviewed changes

Comment thread pkg/device/ascend/device.go

Eshiv-Pandey mentioned this pull request Aug 1, 2026

fix: skip unhealthy devices in Fit() for all non-nvidia backends #2260

Merged

coderabbitai Bot mentioned this pull request Aug 1, 2026

silent under-allocation and topology bypass in Ascend 910C device allocation #2268

Closed

hami-robot Bot assigned mesutoezdil Aug 2, 2026

hami-robot Bot added the lgtm label Aug 2, 2026

Eshiv-Pandey temporarily deployed to nvidia August 3, 2026 04:06 — with GitHub Actions Inactive

coderabbitai Bot mentioned this pull request Aug 3, 2026

fix(device): GenReason uses len(reason) instead of len(devices) in kunlun/awsneuron #2291

Closed

hami-robot Bot removed the lgtm label Aug 3, 2026

mesutoezdil reviewed Aug 3, 2026

View reviewed changes

Comment thread pkg/device/ascend/device_test.go Outdated

Eshiv-Pandey force-pushed the fix/ascend-fit-health-check branch from c8cb33a to 88c09ce Compare August 3, 2026 12:00

Eshiv-Pandey temporarily deployed to nvidia August 3, 2026 12:09 — with GitHub Actions Inactive

hami-robot Bot added the lgtm label Aug 3, 2026

archlitchi approved these changes Aug 4, 2026

View reviewed changes

hami-robot Bot assigned archlitchi Aug 4, 2026

hami-robot Bot added the approved label Aug 4, 2026

archlitchi approved these changes Aug 4, 2026

View reviewed changes

hami-robot Bot merged commit b55ab85 into Project-HAMi:master Aug 4, 2026
17 checks passed

princexpoddar mentioned this pull request Aug 4, 2026

fix (ascend) - validate module pair count and prevent under allocation for ascend 910C #2369

Merged

Uh oh!

Conversation

Eshiv-Pandey commented Jul 31, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Bug Fixes

Uh oh!

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

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mesutoezdil left a comment • 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!

Uh oh!

mesutoezdil commented Aug 2, 2026

Uh oh!

codecov Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

archlitchi commented Aug 3, 2026

Uh oh!

Uh oh!

mesutoezdil commented Aug 3, 2026

Uh oh!

mesutoezdil commented Aug 3, 2026

Uh oh!

Eshiv-Pandey commented Aug 3, 2026

Uh oh!

archlitchi left a comment

Choose a reason for hiding this comment

Uh oh!

archlitchi left a comment

Choose a reason for hiding this comment

Uh oh!

hami-robot Bot commented Aug 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Eshiv-Pandey commented Jul 31, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 31, 2026 •

edited

Loading

mesutoezdil left a comment •

edited

Loading

codecov Bot commented Aug 3, 2026 •

edited

Loading