Fix/ascend pod device index out of range - #119
Conversation
Add podContainerCount boundary check and empty slice placeholder for Ascend/310P device annotations to match Nvidia/DCU/Metax patterns, preventing 'index out of range [1] with length 1' panic when pod has more containers than annotation entries.
Add podContainerCount boundary check and empty slice placeholder for Ascend/310P device annotations to match Nvidia/DCU/Metax patterns, preventing 'index out of range [1] with length 1' panic when pod has more containers than annotation entries.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: silvasong The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits. 📝 Please follow instructions in the contributing guide to update your commits with the DCO Full details of the Developer Certificate of Origin can be found at developercertificate.org. The list of commits missing DCO signoff:
DetailsInstructions 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. I understand the commands that are listed here. |
|
Welcome @silvasong! It looks like this is your first PR to Project-HAMi/HAMi-WebUI 🎉 |
📝 WalkthroughWalkthrough
ChangesAscend device decoding
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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. 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: 1
🤖 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 `@server/internal/provider/util/util.go`:
- Around line 329-342: Update the Ascend container-device parsing loop around
DecodeNpuContainerDevices to ensure pd[devType] contains exactly
podContainerCount(pod) entries, appending ContainerDevices{} for missing
trailing split segments while retaining existing handling for empty and invalid
segments. Add a regression test covering Ascend/310P annotations with fewer
segments, using the relevant test setup instead of the NVIDIA-only case in
util_test.go.
🪄 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: 3787b080-8279-4650-a72d-a9cac45b03ef
📒 Files selected for processing (1)
server/internal/provider/util/util.go
| for i, s := range strings.Split(str, OnePodMultiContainerSplitSymbol) { | ||
| if i >= podContainerCount(pod) { | ||
| break | ||
| } | ||
| if s == "" { | ||
| pd[devType] = append(pd[devType], ContainerDevices{}) | ||
| continue | ||
| } | ||
| cd, err := DecodeNpuContainerDevices(s) | ||
| if err != nil { | ||
| return PodDevices{}, nil | ||
| } | ||
| if len(cd) == 0 { | ||
| continue | ||
| } | ||
| pd[devType] = append(pd[devType], cd) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Pad missing Ascend container slots.
When the annotation contains fewer split segments than podContainerCount(pod), this loop appends only the segments that exist. The index check limits excess input, but it does not add missing trailing ContainerDevices{} entries. fetchContainerInfo can still index a shorter PodSingleDevice slice and panic.
Iterate through the container count and treat absent segments as empty, or pad the slice after the loop. Add an Ascend/310P regression test. server/internal/provider/util/util_test.go:353-403 selects NVIDIA, so it does not execute this branch.
Proposed fix
- for i, s := range strings.Split(str, OnePodMultiContainerSplitSymbol) {
- if i >= podContainerCount(pod) {
- break
- }
+ segments := strings.Split(str, OnePodMultiContainerSplitSymbol)
+ containerCount := podContainerCount(pod)
+ for i := 0; i < containerCount; i++ {
+ s := ""
+ if i < len(segments) {
+ s = segments[i]
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for i, s := range strings.Split(str, OnePodMultiContainerSplitSymbol) { | |
| if i >= podContainerCount(pod) { | |
| break | |
| } | |
| if s == "" { | |
| pd[devType] = append(pd[devType], ContainerDevices{}) | |
| continue | |
| } | |
| cd, err := DecodeNpuContainerDevices(s) | |
| if err != nil { | |
| return PodDevices{}, nil | |
| } | |
| if len(cd) == 0 { | |
| continue | |
| } | |
| pd[devType] = append(pd[devType], cd) | |
| } | |
| segments := strings.Split(str, OnePodMultiContainerSplitSymbol) | |
| containerCount := podContainerCount(pod) | |
| for i := 0; i < containerCount; i++ { | |
| s := "" | |
| if i < len(segments) { | |
| s = segments[i] | |
| } | |
| if s == "" { | |
| pd[devType] = append(pd[devType], ContainerDevices{}) | |
| continue | |
| } | |
| cd, err := DecodeNpuContainerDevices(s) | |
| if err != nil { | |
| return PodDevices{}, nil | |
| } | |
| pd[devType] = append(pd[devType], cd) | |
| } |
🤖 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 `@server/internal/provider/util/util.go` around lines 329 - 342, Update the
Ascend container-device parsing loop around DecodeNpuContainerDevices to ensure
pd[devType] contains exactly podContainerCount(pod) entries, appending
ContainerDevices{} for missing trailing split segments while retaining existing
handling for empty and invalid segments. Add a regression test covering
Ascend/310P annotations with fewer segments, using the relevant test setup
instead of the NVIDIA-only case in util_test.go.
|
Thanks for the fix. This is now covered by merged #95, which preserves Ascend empty container slots, bounds decoded entries to the Pod container count, and adds the aggregation fix and regression coverage. Closing as superseded. |
2 participants
Title
fix: prevent index out of range panic in Ascend pod device annotation decodingDescription
Problem
When a pod with Ascend (910B/310P) GPU devices has fewer device annotation entries than the total container count, the
DecodePodDevicesfunction produces aPodSingleDeviceslice shorter than expected. This causes aruntime error: index out of range [1] with length 1panic infetchContainerInfoatpod.go.Root Cause
The Ascend/310P device branch in
DecodePodDeviceswas missing three safeguards that Nvidia, DCU, and Metax already have:podContainerCountboundary check — noif i >= podContainerCount(pod) { break }continueto skip empty entries instead of appending an emptyContainerDevices{}, which misaligned the indexfor _, s := rangeinstead offor i, s := rangeFix
Align the Ascend/310P branch with the Nvidia/DCU/Metax pattern:
Verification
go vetpassesTestDecodePodDevicesWithInitContainerspassesSummary by CodeRabbit