Feature - #2412
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>
|
[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 |
|
Welcome @im-Toqeer-506! It looks like this is your first PR to Project-HAMi/HAMi 🎉 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdded a backend-agnostic conformance suite for registered device backends. The suite checks construction, resource names, empty inputs, admission mutation, and overflow handling. Metax now rejects memory values outside the ChangesDevice backend conformance
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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-266: Update the memory-request conversion flow before the
scaling assignment: reject failed AsInt64() conversions, validate the original v
against the allowable range after accounting for MemoryFactor, and only then
compute mem = v * MemoryFactor. Preserve the existing final mem range check in
the surrounding request-validation logic as defense in depth, and ensure invalid
or overflowing inputs do not produce a zero-valued request.
🪄 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: cda5f0ca-a473-481a-9eba-7d4c1ded1a71
📒 Files selected for processing (2)
pkg/device/conformance_test.gopkg/device/metax/sdevice.go
| // 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.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
factor = 1024
value = 1 << 54
wrapped = ((value * factor + (1 << 63)) % (1 << 64)) - (1 << 63)
assert 0 <= wrapped <= (1 << 31) - 1
print(f"wrapped int64 result: {wrapped}")
PYRepository: Project-HAMi/HAMi
Length of output: 179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate sdevice.go =="
fd -a 'sdevice\.go$' . | sed 's#^\./##'
echo "== relevant file info =="
file="$(fd 'sdevice\.go$' . | head -n1)"
wc -l "$file"
echo "== outline =="
ast-grep outline "$file" --view compact || true
echo "== relevant lines =="
sed -n '220,285p' "$file" | cat -n | sed 's/^/L/;s/L/ /'
echo "== imports header =="
sed -n '1,40p' "$file" | cat -nRepository: Project-HAMi/HAMi
Length of output: 4335
Validate the memory quantity before scaling.
Line 262 runs after mem = v * MemoryFactor. With MemoryFactor == 1024, a value such as v == 1<<54 overflows int64 to 0, so this check accepts an out-of-range request and returns Memreq: 0. Failed AsInt64() conversions also leave mem at 0, which is interpreted as MemPercentagereq: 100. Reject failed AsInt64() conversions and validate v before multiplying; keep the final mem check as defense in depth.
🤖 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 - 266, Update the
memory-request conversion flow before the scaling assignment: reject failed
AsInt64() conversions, validate the original v against the allowable range after
accounting for MemoryFactor, and only then compute mem = v * MemoryFactor.
Preserve the existing final mem range check in the surrounding
request-validation logic as defense in depth, and ensure invalid or overflowing
inputs do not produce a zero-valued request.
|
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. |
Signed-off-by: M Toqeer Zia <muhammadtoqeerzia586694@gmail.com>
|
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. |
|
You need to edit the title, to reflect the feature you intend to implement |
2 participants
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?: