Skip to content

fix(util): preserve empty container slots in Ascend device index mapping - #122

Closed
manoj-1407 wants to merge 1 commit into
Project-HAMi:mainfrom
manoj-1407:fix/ascend-empty-container-index-mapping
Closed

fix(util): preserve empty container slots in Ascend device index mapping#122
manoj-1407 wants to merge 1 commit into
Project-HAMi:mainfrom
manoj-1407:fix/ascend-empty-container-index-mapping

Conversation

@manoj-1407

@manoj-1407 manoj-1407 commented Aug 9, 2026

Copy link
Copy Markdown

What type of PR is this?

/kind bug

Which issue(s) this PR fixes

Fixes #123

What this PR does

Ascend and Ascend310P device decoding silently skipped empty container entries, breaking the container-to-device index mapping for multi-container pods.

For a 2-container pod where container-0 has no Ascend device and container-1 does, the device was incorrectly mapped to container-0.

The Problem

// Before: silently skips empty entries, corrupts index mapping
if len(cd) == 0 {
    continue
}

NVIDIA, Hygon, and MetaX all correctly preserve empty slots. Ascend was the only device type missing this.

The Fix

Mirrors the existing pattern used by NVIDIA/Hygon/MetaX — check container count bound and append ContainerDevices{} for empty segments.

Testing

Added TestDecodePodDevicesAscendMultiContainer which proves the correct index mapping is preserved. All existing tests pass.

AI Disclosure

Used Claude for guidance. Did all implementation and testing myself. Understand the fix completely.

Summary by CodeRabbit

  • Bug Fixes

    • Improved Ascend device decoding for pods with multiple containers.
    • Preserved empty container positions and skipped empty device annotations.
    • Continued to report decoding failures safely without returning partial results.
  • Tests

    • Added coverage for multi-container pods where devices are assigned only to the appropriate container.

Ascend and Ascend310P device decoding silently skipped empty container
entries, breaking the index mapping for multi-container pods. When
container-0 has no Ascend device and container-1 does, the device was
mapped to container-0 instead.

Fix mirrors the existing NVIDIA/Hygon/MetaX pattern: check the container
count bound, and append an empty ContainerDevices{} for empty segments
to preserve the container-to-device index alignment.

Add TestDecodePodDevicesAscendMultiContainer to pin the correct behavior.

Signed-off-by: manoj-1407 <manojkumar148700@gmail.com>
@hami-robot

hami-robot Bot commented Aug 9, 2026

Copy link
Copy Markdown

@manoj-1407: The label(s) kind/bug cannot be applied, because the repository doesn't have them.

Details

In response to this:

What type of PR is this?

/kind bug

What this PR does

Ascend and Ascend310P device decoding silently skipped empty container entries, breaking the container-to-device index mapping for multi-container pods.

For a 2-container pod where container-0 has no Ascend device and container-1 does, the device was incorrectly mapped to container-0.

The Problem

// Before: silently skips empty entries, corrupts index mapping
if len(cd) == 0 {
   continue
}

NVIDIA, Hygon, and MetaX all correctly preserve empty slots. Ascend was the only device type missing this.

The Fix

Mirrors the existing pattern used by NVIDIA/Hygon/MetaX — check container count bound and append ContainerDevices{} for empty segments.

Testing

Added TestDecodePodDevicesAscendMultiContainer which proves the correct index mapping is preserved. All existing tests pass.

AI Disclosure

Used Claude for guidance. Did all implementation and testing myself. Understand the fix completely.

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.

@hami-robot

hami-robot Bot commented Aug 9, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: manoj-1407

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

Details Needs approval from an approver in each of these files:

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

@hami-robot

hami-robot Bot commented Aug 9, 2026

Copy link
Copy Markdown

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

@hami-robot hami-robot Bot added the size/M label Aug 9, 2026
@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Ascend pod-device decoding now bounds annotation processing by container count, preserves empty container positions, skips empty segments, and retains error handling. A multi-container test verifies the new mapping behavior.

Changes

Ascend container decoding

Layer / File(s) Summary
Preserve Ascend container slots
server/internal/provider/util/util.go, server/internal/provider/util/util_test.go
DecodePodDevices preserves empty container entries, limits processing to the pod container count, and decodes non-empty segments. The test verifies two container slots with a device only in the second slot.

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

Possibly related PRs

Suggested reviewers: fouof

Poem

A rabbit checks each container row,
Empty slots stay empty in the flow.
Devices hop where annotations say,
Errors still turn the result away.
Ascend’s mapping now stays in array!

🚥 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%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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 and concisely describes the main Ascend device mapping fix for empty container slots.
✨ 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
coderabbitai Bot requested a review from FouoF August 9, 2026 20:19
@Nimbus318

Copy link
Copy Markdown
Collaborator

Thanks for identifying and testing the container-slot alignment bug. The same fix is now included in merged #95, together with multi-device-type aggregation coverage, so this PR is superseded.

@Nimbus318 Nimbus318 closed this Aug 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug]: Ascend/Ascend310P device index mapping broken for multi-container pods

2 participants