fix(device): guard nil maps in mthreads MutateAdmission by imantaba · Pull Request #2254 · Project-HAMi/HAMi

imantaba · 2026-08-01T05:24:05Z

What this PR does

MthreadsDevices.MutateAdmission writes to ctr.Resources.Limits and
p.Annotations without checking they are non-nil. Writing to a nil map panics
in Go and crashes the mutating-webhook goroutine. Two ordinary inputs trigger
it:

a pod that requests more than one mthreads.com/vgpu but carries no
annotations (p.Annotations == nil) panics on the request-gpu-num write;
a container that specifies the vgpu count under requests only, with no
limits block (ctr.Resources.Limits == nil), panics on the cores/memory
write.

Because the webhook runs MutateAdmission for every registered device on every
pod, this takes down admission for affected mthreads pods.

This initializes ctr.Resources.Limits and p.Annotations before writing,
matching the nil guard already used in the metax backend
(pkg/device/metax/sdevice.go).

Fixes

Fixes #2253

Testing

Added two cases to Test_MutateAdmission covering both inputs. Each produces
panic: assignment to entry in nil map before this change and passes after.
Verified red→green; full package passes under -race; gofmt clean.

AI assistance disclosure

This change was developed with AI assistance (per CONTRIBUTING.md).

Summary by CodeRabbit

Bug Fixes
- Improved admission handling when resource limits or pod annotations are not initially configured.
- vGPU requests with multiple devices are now admitted successfully in additional request and limit scenarios.

Note: a requests-only extended-resource pod is eventually rejected by API validation, but mutating admission runs before validation, so the nil-Limits panic is still reachable.

MthreadsDevices.MutateAdmission writes to ctr.Resources.Limits and p.Annotations without checking they are non-nil. Two normal inputs trigger "assignment to entry in nil map" and crash the mutating webhook goroutine: - a pod that requests more than one mthreads.com/vgpu but carries no annotations (p.Annotations is nil) panics on the request-gpu-num write; - a container that specifies the vgpu count under requests only, with no limits block (ctr.Resources.Limits is nil), panics on the cores/memory write. Because the webhook runs MutateAdmission for every registered device on every pod, this takes down admission for affected mthreads pods. Initialize ctr.Resources.Limits and p.Annotations before writing, matching the nil guard already used in the metax backend. Add table cases covering both inputs (each panics before this change, passes after). Signed-off-by: imantaba <itn.taba@gmail.com>

coderabbitai · 2026-08-01T05:24:27Z

📝 Walkthrough

Walkthrough

MutateAdmission initializes nil resource limits and pod annotations before writing derived values. Tests cover multi-vGPU admission when limits or annotations are absent, including a request-only vGPU count of one.

Changes

mthreads admission

Layer / File(s)	Summary
Initialize admission maps `pkg/device/mthreads/device.go`, `pkg/device/mthreads/device_test.go`	`MutateAdmission` initializes nil limits and annotations. Tests cover vGPU counts of one and two with missing annotations or limits.

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

Possibly related PRs

Project-HAMi/HAMi#2134: Both PRs modify MthreadsDevices.MutateAdmission and related tests.
Project-HAMi/HAMi#2176: Both PRs initialize nil resource maps before writing admission-derived values.

Suggested reviewers: ouyangluwei163

Poem

A rabbit checks each map,
Before values take their place.
Nil limits become ready,
Annotations hold their data,
And vGPUs pass admission safely.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes initialize both nil maps required by issue `#2253` and add tests for the reported admission paths.
Out of Scope Changes check	✅ Passed	The changes are limited to the requested nil-map guards and focused regression tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: guarding nil maps in mthreads MutateAdmission.

✨ 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

🧹 Nitpick comments (1)

pkg/device/mthreads/device_test.go (1)
67-83: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assert the values written by MutateAdmission.

These cases only check the return value and error state. They would pass if the function stopped writing the derived resource limits or mthreads.com/request-gpu-num. Assert the expected core limit, memory limit, annotation value, and non-nil maps in both cases.

Also applies to: 84-104
🤖 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/mthreads/device_test.go` around lines 67 - 83, Strengthen the test
cases around MutateAdmission, including both referenced scenarios, to verify the
mutation outputs rather than only its return value and error. Assert the
expected core and memory resource limits, the mthreads.com/request-gpu-num
annotation, and that the mutated resource and annotation maps are non-nil.

🤖 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 `@pkg/device/mthreads/device_test.go`:
- Around line 67-83: Strengthen the test cases around MutateAdmission, including
both referenced scenarios, to verify the mutation outputs rather than only its
return value and error. Assert the expected core and memory resource limits, the
mthreads.com/request-gpu-num annotation, and that the mutated resource and
annotation maps are non-nil.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 39af774c-26e7-4d70-ae8a-96907ebf7c25

📥 Commits

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

📒 Files selected for processing (2)

pkg/device/mthreads/device.go
pkg/device/mthreads/device_test.go

codecov · 2026-08-01T05:28:50Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag	Coverage Δ
unittests	`64.22% <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/mthreads/device.go	`86.61% <100.00%> (+0.22%)`	⬆️

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

mesutoezdil

pls fix the title scope, files are under pkg/device so it should be fix(device) not fix(device-plugin).

Add a table case for a container that sets the vgpu count under requests only, count=1, with no limits block. This exercises the memok-false branch, which also writes to ctr.Resources.Limits and panicked on a nil map before the guard added in the previous commit. Signed-off-by: imantaba <itn.taba@gmail.com>

imantaba · 2026-08-01T06:23:44Z

pls fix the title scope, files are under pkg/device so it should be fix(device) not fix(device-plugin).

Updated

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

/lgtm

archlitchi

/lgtm

archlitchi

/lgtm

hami-robot · 2026-08-03T03:57:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: archlitchi, imantaba

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

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>

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

hami-robot Bot added the dco-signoff: yes label Aug 1, 2026

hami-robot Bot requested review from ouyangluwei163 and wawa0210 August 1, 2026 05:24

github-actions Bot added the kind/bug Something isn't working label Aug 1, 2026

hami-robot Bot added the size/M label Aug 1, 2026

coderabbitai Bot requested a review from mesutoezdil August 1, 2026 05:24

coderabbitai Bot reviewed Aug 1, 2026

View reviewed changes

imantaba temporarily deployed to nvidia August 1, 2026 05:32 — with GitHub Actions Inactive

mesutoezdil reviewed Aug 1, 2026

View reviewed changes

Comment thread pkg/device/mthreads/device.go

Comment thread pkg/device/mthreads/device_test.go

imantaba changed the title ~~fix(device-plugin): guard nil maps in mthreads MutateAdmission~~ fix(device): guard nil maps in mthreads MutateAdmission Aug 1, 2026

imantaba temporarily deployed to nvidia August 1, 2026 06:25 — with GitHub Actions Inactive

coderabbitai Bot mentioned this pull request Aug 1, 2026

bug(quota): DelQuota uses length check instead of HasPrefix matching AddQuota #2271

Closed

hami-robot Bot assigned mesutoezdil Aug 2, 2026

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

archlitchi approved these changes Aug 3, 2026

View reviewed changes

hami-robot Bot assigned archlitchi Aug 3, 2026

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

archlitchi approved these changes Aug 3, 2026

View reviewed changes

archlitchi merged commit 368c983 into Project-HAMi:master Aug 3, 2026
19 checks passed

imantaba deleted the fix/mthreads-mutateadmission-nil-map branch August 3, 2026 06:15

blackdragoon26 mentioned this pull request Aug 3, 2026

fix(awsneuron): nil CustomInfo write can panic in Fit/AddResourceUsage #2294

Closed

im-Toqeer-506 mentioned this pull request Aug 5, 2026

test: add a shared conformance test suite for the device.Devices interface #2379

Closed

4 tasks

coderabbitai Bot mentioned this pull request Aug 5, 2026

Flaky: data race on global device.SupportDevices between leaked test informers and Test_onAddPod_BadDeviceAnnotation #2389

Closed

im-Toqeer-506 mentioned this pull request Aug 6, 2026

Feature #2412

Closed

This was referenced Aug 6, 2026

[bug]: ascend MutateAdmission panics on nil Requests (assignment to nil map) #2415

Closed

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

Merged

coderabbitai Bot mentioned this pull request Aug 7, 2026

fix(ascend): guard nil Requests map in MutateAdmission #2429

Closed

im-Toqeer-506 mentioned this pull request Aug 7, 2026

Fix/metax int32 overflow #2446

Closed

This was referenced Aug 15, 2026

test(pkg/device): add nil checks and unit tests for unparseable pod specs #2676

Closed

test(pkg/device): add nil guards and unit tests for pod and node parsing #2678

Closed

Navigation Menu

Uh oh!

fix(device): guard nil maps in mthreads MutateAdmission - #2254

fix(device): guard nil maps in mthreads MutateAdmission#2254
archlitchi merged 2 commits into
Project-HAMi:masterfrom
imantaba:fix/mthreads-mutateadmission-nil-map

imantaba commented Aug 1, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 1, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Uh oh!

codecov Bot commented Aug 1, 2026 •

edited

Loading

Uh oh!

mesutoezdil left a comment

Uh oh!

Uh oh!

Uh oh!

imantaba commented Aug 1, 2026

Uh oh!

mesutoezdil commented Aug 2, 2026

Uh oh!

archlitchi left a comment

Uh oh!

archlitchi left a comment

Uh oh!

hami-robot Bot commented Aug 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

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

Uh oh!

What this PR does

Fixes

Testing

AI assistance disclosure

Summary by CodeRabbit

Uh oh!

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

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

mesutoezdil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

imantaba commented Aug 1, 2026

Uh oh!

mesutoezdil commented Aug 2, 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 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

imantaba commented Aug 1, 2026 •

edited

Loading

coderabbitai Bot commented Aug 1, 2026 •

edited

Loading

codecov Bot commented Aug 1, 2026 •

edited

Loading