Skip to content

fix(ascend): guard nil Requests in MutateAdmission - #2416

Merged
hami-robot[bot] merged 1 commit into
Project-HAMi:masterfrom
keshav9926:fix-ascend-nil-requests-panic
Aug 10, 2026
Merged

fix(ascend): guard nil Requests in MutateAdmission#2416
hami-robot[bot] merged 1 commit into
Project-HAMi:masterfrom
keshav9926:fix-ascend-nil-requests-panic

Conversation

@keshav9926

@keshav9926 keshav9926 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

What this PR does / why we need it

MutateAdmission in the ascend backend writes the trimmed memory quantity
into ctr.Resources.Requests unconditionally. When a container declares only
limits (no requests block), Requests is a nil map and the write panics
(assignment to entry in nil map) inside the admission webhook.

This initializes Requests before the write, mirroring the guard the kunlun
backend already uses for the identical write
(pkg/device/kunlun/vdevice.go), and the pattern merged for mthreads in
#2254. After this, no backend under pkg/device/ writes to
Resources.Requests unguarded.

Also adds Test_MutateAdmission_NilRequests: two regression cases (memory
limit without a requests block; count-only without a requests block), both of
which panic on master today.

Which issue(s) this PR fixes

Fixes #2415

Special notes for your reviewer

The existing Test_MutateAdmission never hits this path with nil Requests:
the only case reaching the write is the only fixture that explicitly sets a
Requests block. The new test covers exactly the gap.

AI assistance disclosure

This PR was developed with AI assistance (Claude); the bug was found by an
automated audit of unguarded map writes across pkg/device/, and the fix and
tests were human-reviewed before submission.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed pod admission when resource requests are missing but memory limits are specified.
    • Memory requests are now populated correctly without errors or crashes, including when default card allocation applies.
    • Improved handling for pods that define only memory limits.
  • Tests

    • Added coverage for admission scenarios with absent resource requests.
    • Verified both explicit memory limits and default card allocation behavior.

@hami-robot
hami-robot Bot requested a review from DSFans2014 August 6, 2026 17:53
@hami-robot
hami-robot Bot requested a review from wawa0210 August 6, 2026 17:53
@hami-robot

hami-robot Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

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

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

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review Change Stack

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: 6728615d-2959-45ce-b81d-e4d5800abaa3

📥 Commits

Reviewing files that changed from the base of the PR and between c5e99b6 and 6b8752b.

📒 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

MutateAdmission now initializes nil resource request maps before assigning memory requests. Regression tests cover explicit memory limits and the default full-card allocation.

Changes

Ascend admission mutation

Layer / File(s) Summary
Handle missing resource requests
pkg/device/ascend/device.go, pkg/device/ascend/device_test.go
MutateAdmission initializes nil request maps before writing memory requests. Tests verify explicit-limit handling and fallback to MemoryAllocatable.

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

Possibly related PRs

Suggested reviewers: archlitchi

Poem

I’m a rabbit guarding requests in a map,
No nil holes remain in the admission trap.
Limits guide memory, defaults fill the card,
Tests watch the paths and keep errors barred.
Hop, hop—Ascend mutation is safe!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the nil Requests guard added to Ascend MutateAdmission.
Linked Issues check ✅ Passed The changes initialize nil Requests before writing memory and add regression tests, satisfying issue #2415.
Out of Scope Changes check ✅ Passed All code and test changes directly address the nil Requests panic described in issue #2415.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ 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 archlitchi August 6, 2026 17:54
@github-actions github-actions Bot added the kind/bug Something isn't working label Aug 6, 2026
@archlitchi

Copy link
Copy Markdown
Member

please sync with master to pass the CI

A container declaring only limits (no requests block) reaches the
trimmed-memory write with ctr.Resources.Requests == nil and panics
(assignment to entry in nil map) in the admission webhook. Initialize
the map before writing, as kunlun vdevice already does for the same
write and as merged for mthreads in Project-HAMi#2254.

Add Test_MutateAdmission_NilRequests covering both panicking shapes:
memory limit present without a requests block, and count-only without
a requests block.

Signed-off-by: keshav9926 <kkakani160@gmail.com>
@keshav9926
keshav9926 force-pushed the fix-ascend-nil-requests-panic branch from afc148c to 6b8752b Compare August 7, 2026 07:58
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@coderabbitai coderabbitai Bot removed the enhancement label Aug 7, 2026
@keshav9926

Copy link
Copy Markdown
Contributor Author

Done — rebased onto current master (now based on c5e99b6). Thanks for taking a look @archlitchi!

@Shouren Shouren left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@hami-robot hami-robot Bot added the lgtm label Aug 9, 2026
@hami-robot

hami-robot Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: keshav9926, Shouren

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:

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 added the approved label Aug 9, 2026
@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
unittests 64.10% <100.00%> (+<0.01%) ⬆️

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 84.75% <100.00%> (+0.07%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hami-robot
hami-robot Bot merged commit 59cbf76 into Project-HAMi:master Aug 10, 2026
17 checks passed
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 MutateAdmission panics on nil Requests (assignment to nil map)

3 participants