Fix/metax int32 overflow - #2446
Closed
im-Toqeer-506 wants to merge 2 commits into
Closed
Conversation
Every hardware backend implements the same device.Devices interface (pkg/device/devices.go), but each is otherwise tested in isolation, so the same class of contract violation has repeatedly been fixed one backend at a time (e.g. nil-map / nil-pointer panics on the admission and Fit paths in Project-HAMi#2254 and Project-HAMi#2294). Add pkg/device/conformance_test.go: a backend-agnostic suite that runs one shared set of contract assertions against every constructible backend, so a regression in any of them fails here immediately instead of shipping and being rediscovered vendor-by-vendor. Invariants asserted for all 14 constructible backends: - registry guard: every case has a name and a non-nil backend - GetResourceNames() advertises at least one non-empty resource name (a backend with none is unreachable by the scheduler) - a container requesting none of a backend's resources yields Nums == 0 - Fit against nil and empty candidate lists returns false without panicking - MutateAdmission on a pod with no accelerator request does not panic The suite lives in the external device_test package on purpose: the backend sub-packages import pkg/device, so an internal test importing them back would create an import cycle. The ascend and iluvatar backends (slice-returning constructors gated behind enable flags) and the int32-overflow invariant for GenerateResourceRequests are intentionally deferred to a follow-up, so this first pass stays green while the underlying fixes land (Project-HAMi#2278, Project-HAMi#2284). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: M Toqeer Zia <muhammadtoqeerzia586694@gmail.com>
Signed-off-by: M Toqeer Zia <muhammadtoqeerzia586694@gmail.com>
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: im-Toqeer-506 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 |
github-actions
Bot
added
kind/bug
📝 WalkthroughWalkthroughThis change adds shared conformance tests for constructible device backends and validates registration, resource handling, fitting, admission mutation, and overflow behavior. Metax now rejects memory requests outside the valid ChangesDevice conformance and resource validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 `@pkg/device/metax/sdevice.go`:
- Around line 259-265: The no-unit memory value is scaled before validation,
allowing int64 overflow to produce a small valid value. In
pkg/device/metax/sdevice.go lines 259-265, update the validation before
MemoryFactor multiplication to reject negatives and values above
math.MaxInt32/MemoryFactor, while retaining the post-conversion range check for
unit-based input. In pkg/device/conformance_test.go lines 322-348, add a no-unit
value such as (1<<54)+1 and assert GenerateResourceRequests rejects it or
returns only non-negative fields.
🪄 Autofix
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: 4ed7372c-6f76-480e-8216-647d6c008059
📒 Files selected for processing (2)
pkg/device/conformance_test.gopkg/device/metax/sdevice.go
Comment on lines
+259
to
+265
| // Reject out-of-range memory requests that would overflow int32. | ||
| // MemoryFactor is 1024, so a Gi request without a unit multiplies by that; | ||
| // a request above math.MaxInt32 wraps negative on the unchecked cast below. | ||
| if mem < 0 || mem > math.MaxInt32 { | ||
| klog.Errorf("container<%s> metax-sgpu memory request %d MiB is out of range", ctr.Name, mem) | ||
| return device.ContainerDeviceRequest{} | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate no-unit memory before scaling.
The check runs after v * MemoryFactor. For v = (1<<54)+1 and MemoryFactor = 1024, the int64 multiplication wraps to 1024. The request then passes this check and returns a small Memreq for a very large request.
pkg/device/metax/sdevice.go#L259-L265: Reject negative values and values greater thanmath.MaxInt32 / MemoryFactorbefore multiplication. Keep the post-conversion range check for unit-based input.pkg/device/conformance_test.go#L322-L348: Add a no-unit value such as(1<<54)+1and assert thatGenerateResourceRequestsrejects it or returns only non-negative fields.
📍 Affects 2 files
pkg/device/metax/sdevice.go#L259-L265(this comment)pkg/device/conformance_test.go#L322-L348
🤖 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/metax/sdevice.go` around lines 259 - 265, The no-unit memory value
is scaled before validation, allowing int64 overflow to produce a small valid
value. In pkg/device/metax/sdevice.go lines 259-265, update the validation
before MemoryFactor multiplication to reject negatives and values above
math.MaxInt32/MemoryFactor, while retaining the post-conversion range check for
unit-based input. In pkg/device/conformance_test.go lines 322-348, add a no-unit
value such as (1<<54)+1 and assert GenerateResourceRequests rejects it or
returns only non-negative fields.
Contributor
|
AI generated |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind feature
/kind failing-test
What this PR does / why we need it:
All 14 HAMi device backends implement the same
device.Devicesinterface(
pkg/device/devices.go), but each is tested in isolation — so the same class ofcontract violation keeps being rediscovered and fixed one backend at a time
(int32 overflow in
GenerateResourceRequests: #2278/#2284/#2336; nil-map / nil-pointerpanics on the admission and Fit paths: #2254/#2294).
This PR lands phase 1 of a shared, table-driven conformance suite that runs one
set of contract assertions against every constructible backend, so a regression in
any of them fails CI immediately instead of shipping.
Included:
conformanceCases()) that constructs 14 backendvariants from plain in-memory configs mirroring the production resource names wired
in
pkg/scheduler/config.InitDevicesWithConfig.GetResourceNames()returns at least one non-empty name (else the backend isunreachable by the scheduler).
Nums == 0.Fit(nil, …)andFit([]*DeviceUsage{}, …)returnfalsewithout panicking.MutateAdmissiondoes not panic on a pod that requests none of the resources.Nums/Memreq/CoresreqfromGenerateResourceRequests— theint32-overflow guard.
metax-sgpu,which this PR fixes inline (reject out-of-range memory before the unchecked
int32(mem)cast) — exactly the value the suite is meant to deliver.cambricon[bug]: cambricon int32 overflow in GenerateResourceRequests silently drops memory request #2278,mthreadsbug: int32 overflow in GenerateResourceRequests silently drops memory request (iluvatar, mthreads) #2284) go on anexplicit, commented skip list linked to their tracking issues, so the gap is visible,
not silent.
Kept additive and phased per the issue:
ascendandiluvatar(slice / enable-flagconstructors) and further invariants (scoring monotonicity,
PatchAnnotationsround-trip, lock idempotency) are documented as follow-up.
Which issue(s) this PR fixes:
Fixes # #2379
Special notes for your reviewer:
cambriconandmthreadsfail andmetax-sgpupasses (confirming the metax fix is exercised).MemoryFactor > 0, so it targets thescaling-multiplication overflow that the bug reports describe; backends that don't
scale still get the in-range non-negativity check (case 1).
device_testpackage on purpose — the backendsub-packages import
pkg/device, so an internal test importing them back wouldcreate an import cycle.
go test -race ./pkg/device/...and./pkg/scheduler/...all pass;gofmt,goimports(local-prefix), andgo vetare clean.Does this PR introduce a user-facing change?: