fix(cambricon): clamp oversized memory request before int32 conversion - #2339
fix(cambricon): clamp oversized memory request before int32 conversion#2339shivv23 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shivv23 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 |
📝 WalkthroughWalkthroughCambricon memory request conversion now clamps invalid and oversized values to ChangesCambricon memory request bounds
Estimated code review effort: 2 (Simple) | ~10 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 |
Signed-off-by: shivv23 <shivamkumar0423@gmail.com>
6d5dca2 to
2de1da6
Compare
![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
🧹 Nitpick comments (1)
pkg/device/cambricon/device_test.go (1)
291-328: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover both invalid-input branches.
These cases cover only positive, integer quantities. Add a negative quantity to exercise
memnums < 0, and add a whole-number decimal quantity such as10000000.0to exerciseAsInt64() == false. Assert that both cases produceMemreq == int32(math.MaxInt32)and preserve the other request fields. (github.com)🤖 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/cambricon/device_test.go` around lines 291 - 328, Add test cases in the device request parsing tests covering a negative vmemory quantity to exercise the memnums < 0 branch and a whole-number decimal such as 10000000.0 to exercise the AsInt64() == false branch. For both cases, assert Memreq is int32(math.MaxInt32) while preserving the expected Nums, Type, MemPercentagereq, and Coresreq fields, following the existing cases near the vmemory tests.
🤖 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/cambricon/device.go`:
- Around line 255-260: Update the memory-request conversion logic surrounding
memnums and memnum to compare the Quantity against the intended lower and upper
bounds before calling AsInt64(). If the request is out of range, clamp memnum to
math.MaxInt32; otherwise convert the validated quantity to the scaled integer,
ensuring AsInt64() failures cannot leave memnum at zero and trigger percentage
accounting.
---
Nitpick comments:
In `@pkg/device/cambricon/device_test.go`:
- Around line 291-328: Add test cases in the device request parsing tests
covering a negative vmemory quantity to exercise the memnums < 0 branch and a
whole-number decimal such as 10000000.0 to exercise the AsInt64() == false
branch. For both cases, assert Memreq is int32(math.MaxInt32) while preserving
the expected Nums, Type, MemPercentagereq, and Coresreq fields, following the
existing cases near the vmemory tests.
🪄 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: 3a40d61c-587c-49ad-8e19-dfcb88d30b8b
📒 Files selected for processing (2)
pkg/device/cambricon/device.gopkg/device/cambricon/device_test.go
| if memnums > math.MaxInt32/256 || memnums < 0 { | ||
| klog.ErrorS(nil, "cambricon memory request is out of int32 range, clamping to max int32", "container", ctr.Name, "request", memnums) | ||
| memnum = math.MaxInt32 | ||
| } else { | ||
| memnum = int(memnums * 256) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'device.go' pkg/device/cambricon || true
echo "== git diff stat =="
git diff --stat || true
echo "== relevant imports/context =="
sed -n '1,120p' pkg/device/cambricon/device.go
echo "== target lines =="
sed -n '220,300p' pkg/device/cambricon/device.go
echo "== quantity usage in cambricon/device.go =="
rg -n "mem\.AsInt64\(|PercentageRequest|MemPercentagereq|Fit\(|Quantity|ResourceList|ResourceRequests" pkg/device/cambricon/device.go
echo "== module kubernetes/apimachinery version =="
if [ -f go.mod ]; then
rg -n "apimachinery|k8s.io/apimachinery" go.mod
fiRepository: Project-HAMi/HAMi
Length of output: 7983
🌐 Web query:
kubernetes apimachinery resource Quantity AsInt64 negative scale MilliValue validation
💡 Result:
In Kubernetes apimachinery, the resource.Quantity type uses a fixed-point representation to avoid floating-point errors [1]. When handling Quantity values, specifically regarding AsInt64, MilliValue, and negative scales, the following behaviors and validation considerations apply: 1. Rounding and Negative Values: Kubernetes Quantity operations such as MilliValue() and ScaledValue() are designed to round toward "positive infinity" (away from zero) for positive numbers and toward "negative infinity" (away from zero) for negative numbers [2]. Historically, bugs in the implementation (e.g., scaledValue) caused some small negative values to round incorrectly toward positive infinity [2]. Recent fixes have ensured these operations correctly round away from zero for negative inputs [2]. 2. AsInt64 Behavior: The AsInt64() method returns an int64 at scale 0 [3]. It will return false if the value cannot be represented as an int64 (e.g., it exceeds math.MaxInt64 or math.MinInt64) or if the conversion would result in a loss of precision (i.e., the value is fractional) [3]. 3. Validation: Because Value() and MilliValue() project through int64, they can overflow if the Quantity is very large [2][4]. Validating these values by checking against resource.MaxMilliValue is considered unreliable because MaxMilliValue itself is a constant that may not account for fractional quantities or the full range of Quantity [4]. Instead, the recommended approach is to use Quantity.Cmp() to compare against a defined bound, such as the resource.MaxMilliQuantity() (introduced to provide a safe, exact bound for MilliValue checks) [4]. 4. Negative Scales: The internal int64Amount structure handles scales, where a negative scale (e.g., scale = -3 for milli-quantities) effectively represents fractional values [5][6]. When performing operations with these scales, the implementation ensures that quantities remain representable as fixed-point numbers [1][3]. For developers, it is critical to perform sign-aware checks (e.g., q.Sign() >= 0) and use Cmp() for bounds checking to avoid overflows and incorrect rounding behavior when working with large or negative quantities [4].
Citations:
- 1: https://kubernetes.io/docs/reference/kubernetes-api/definitions/quantity-resource/
- 2: [Quantity] Fix scaledValue to round negative Quantity values away from zero kubernetes/kubernetes#138510
- 3: https://github.com/kubernetes/apimachinery/blob/5a348c53/pkg/api/resource/amount.go
- 4: [Quantity] Add resource.MaxMilliQuantity and fix the Quantity docs kubernetes/kubernetes#140674
- 5: https://github.com/kubernetes/apimachinery/blob/v0.24.2/pkg/api/resource/quantity.go
- 6: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/api/resource/quantity.go
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Go package availability =="
go list -m -f '{{.Version}}' k8s.io/apimachinery 2>/dev/null || true
go env GOPATH 2>/dev/null || true
echo "== inspect vendored/source Quantity.AsInt64 if present =="
find "$(go env GOPATH 2>/dev/null || true)/pkg/mod/k8s.io/apimachinery" -path '*pkg/api/resource/amount.go' -type f 2>/dev/null | head -20 || true
python3 - <<'PY'
from pathlib import Path
import re, subprocess
base = subprocess.check_output(["go", "env", "GOPATH"], text=True).strip()
paths = subprocess.check_output(
["find", f"{base}/pkg/mod/k8s.io/apimachinery", "-path", "*pkg/api/resource/amount.go", "-type", "f"],
text=True, stderr=subprocess.DEVNULL
).splitlines()[:1]
for p in paths:
print(f"PATH={p}")
txt = Path(p).read_text()
idx = txt.find("func (i *int64Amount) AsInt64() (int64, bool)")
if idx != -1:
snippet = txt[idx:]
print(snippet[:snippet.find("\n\nfunc ")+500] if "\n\nfunc " in snippet[idx+1:] else snippet[:800])
PY
python3 - <<'PY'
import subprocess, textwrap, re
try:
out = subprocess.check_output(["go", "env", "GOPATH"], text=True).strip()
except Exception as e:
print(f"go env failed: {e}")
raise SystemExit(0)
paths = subprocess.check_output(
["find", f"{out}/pkg/mod/k8s.io/apimachinery-v0.36.3", "-path", "*pkg/api/resource/quantity.go", "-type", "f"],
text=True, stderr=subprocess.DEVNULL
).splitlines()
for p in paths:
txt = Path(p).read_text()
for fn in ["func (q Quantity) AsInt64()", "func (q Quantity) MilliValue()", "func ValidateResourceFieldPath"]:
idx = txt.find(fn)
if idx != -1:
end = txt.find("\nfunc ", idx+1)
if end == -1: end = idx+3000
print(f"=== {p} :: {fn} ===")
print(txt[idx:end])
PY
echo "== validate Quantity negative scale behavior by compiling vendor source into temporary package? =="
# Avoid executing repository code. Only inspect generated code/import behavior if tooling exists.
tmpdir="$(mktemp -d)"
cat > "$tmpdir/main.go" <<'GO'
package main
import (
"fmt"
"k8s.io/apimachinery/pkg/api/resource"
)
func main() {
for _, s := range []string{"10000000.0", "1.1", "-1"} {
q, err := resource.ParseQuantity(s)
if err != nil {
fmt.Println(s, "ERROR", err)
continue
}
v, ok := q.AsInt64()
fmt.Println(s, "AsInt64(ok)=", ok, "value=", v, "format=", q.Format, "internal=", q.Value(), "Milli=", q.MilliValue())
}
}
GOCACHE="$tmpdir/gocache" go env -w GOMODCACHE="$tmpdir/modules"
GOCACHE="$tmpdir/gocache" go -C "$tmpdir" run main.go || true
GOCACHE="$tmpdir/gocache" go env -w GOMODCACHE= 2>/dev/null || true
echo "== Fit behavior =="
sed -n '310,390p' pkg/device/cambricon/device.goRepository: Project-HAMi/HAMi
Length of output: 782
🏁 Script executed:
#!/bin/bash
set -u
echo "== GOPATH and module cache =="
go env GOPATH GOMODCACHE 2>/dev/null || true
echo "== all apimachinery package files =="
find "$HOME/go/pkg/mod" -path '*k8s.io/apimachinery*' -type f 2>/dev/null | head -50 || true
python3 - <<'PY'
from pathlib import Path
import subprocess, re
gop = Path(subprocess.check_output(["go", "env", "GOPATH"], text=True, stderr=subprocess.DEVNULL).strip())
candidates = sorted(glob := (gop / "pkg/mod" / "k8s.io/apimachinery-v0.36.3").glob("*/pkg/api/resource/amount.go")) if gop.exists() else []
print("candidates:", candidates)
for p in candidates:
txt = p.read_text()
print(f"FILE {p} AsInt64:")
idx = txt.find("func (i *int64Amount) AsInt64() (intValue int64, ok bool)")
if idx == -1:
idx = txt.find("func (i *int64Amount) AsInt64()")
if idx == -1:
continue
lines = txt[idx:].splitlines()[:80]
for line in lines:
print(line)
py
python3 - <<'PY'
import subprocess
try:
out = subprocess.check_output(["go", "env", "GOMODCACHE"], text=True).strip()
except Exception as e:
print("GOMODCACHE unavailable:", e)
raise SystemExit
print("GOMODCACHE=", out)
pats = [
"pkg/api/resource/amount.go",
"pkg/api/resource/quantity.go",
]
for path in pats:
ps = subprocess.check_output(["find", out, "-path", f"*k8s.io/apimachinery-v0.36.3/{path}", "-type", "f"], text=True, stderr=subprocess.DEVNULL).strip().splitlines()
print("===", path, len(ps), ps)
if not ps:
continue
text = Path(ps[0]).read_text()
for fn in ["func (i *int64Amount) AsInt64() (intValue int64, ok bool)", "func ValidateQuantity(q Quantity, fldPath *Path) *FieldError"]:
idx = text.find(fn)
if idx != -1:
end = text.find("\nfunc ", idx+1)
if end == -1:
end = idx + 2500
print("====", fn, "====")
print(text[idx:end])
PY
echo "== module cache Go files =="
if [ -d "$HOME/go/pkg/mod/k8s.io" ]; then ls -R "$HOME/go/pkg/mod/k8s.io" | head -100; fiRepository: Project-HAMi/HAMi
Length of output: 607
🌐 Web query:
k8s.io/apimachinery v0.36.3 resource Amount AsInt64 negative scale source
💡 Result:
In k8s.io/apimachinery (including version v0.36.3), the AsInt64() method on int64Amount is designed to return the amount as an int64 at scale 0 [1][2]. When dealing with a negative scale, the method explicitly returns 0, false [1][2]. The implementation follows this logic: 1. If the internal scale is 0, it returns the value and true [1][2]. 2. If the internal scale is negative (a.scale < 0), it returns 0, false [1][2]. This behavior reflects that a negative scale represents fractional components (e.g., milli, micro, nano), which cannot be represented as an int64 at scale 0 without potential loss of precision [1][2]. 3. If the internal scale is positive, it uses positiveScaleInt64 to calculate the result [1][2]. The AsInt64() method on the higher-level Quantity type similarly uses this underlying int64Amount logic [3][4]. It returns false if the Quantity is internally represented as a decimal (inf.Dec), or if the underlying int64Amount cannot perform the conversion (which includes cases where the scale is negative) [3][4][5]. In contexts where rounding occurs—such as in RoundUp or when converting scaled values—the library consistently enforces "rounding away from zero" (e.g., -9 at scale 1 rounds to -10) [3][6][4][7]. While the specific AsInt64() method simply fails (returns false) for negative scales, related rounding functions ensure that negative values are treated correctly by moving them further from zero [8][6][7].
Citations:
- 1: https://github.com/kubernetes/apimachinery/blob/v0.35.3/pkg/api/resource/amount.go
- 2: https://gerrit.benkard.de/plugins/gitiles/kubeia/+/832a54ed59c5650748e68062e2f609987921ff2c%5E%21/metrics-server/vendor/k8s.io/apimachinery/pkg/api/resource
- 3: https://github.com/kubernetes/apimachinery/blob/master/pkg/api/resource/quantity.go
- 4: https://github.com/kubernetes/apimachinery/blob/v0.24.2/pkg/api/resource/quantity.go
- 5: Kubernetes fails to allocate PV pretending storage request overflows int64 kubernetes/kubernetes#100091
- 6: https://pkg.go.dev/k8s.io/KUBERNETES/staging/src/k8s.io/apimachinery/pkg/api/resource
- 7: [Quantity] Fix scaledValue to round negative Quantity values away from zero kubernetes/kubernetes#138510
- 8: https://github.com/kubernetes/apimachinery/blob/v0.35.3/pkg/api/resource/math.go
🌐 Web query:
Kubernetes Quantity negative scale valid as string "10000000.0" validation validation.go MilliValue 1000
💡 Result:
In Kubernetes, the Quantity type is a fixed-point representation used for resources like CPU and memory [1]. When parsing a string as a Quantity, the format allows for an optional sign ("+" or "-"), a mantissa (integer part), and optional exponents/suffixes [1]. Regarding your specific technical points: 1. Negative Scale in Quantity Logic: The internal implementation of Kubernetes quantities involves handling scales to represent fixed-point math accurately [2][1]. While string inputs can include negative numbers (e.g., "-100m"), a "negative scale" in the context of the underlying math library (like inf.Dec or resource.Scale) is an implementation detail for exponent representation [2][3]. Within the public API, "negative scale" is generally not a valid or exposed concept for users; the parser simply handles the numeric value provided [1]. Explicit validation functions, such as ValidateNonnegativeQuantity, are often used to enforce that specific resource fields remain positive, as negative values are logically invalid for most resource definitions [4]. 2. MilliValue and Validation: The MilliValue() function is an accessor that returns the value of a quantity in millicores (or equivalent units, i.e., value * 1000) [2][5]. It is implemented as q.ScaledValue(Milli), where Milli is defined with a scale of -3 [2]. Because MilliValue() performs an internal scaling operation, it can overflow an int64 if the underlying quantity is extremely large [2][5]. Kubernetes code often contains warnings that callers should verify the magnitude using Value() before calling MilliValue() if overflow is a concern, though there are ongoing discussions about improving this safety in the codebase [5]. 3. String Validation: When validating quantities in Kubernetes (e.g., via admission controllers or CEL), the standard practice is to use the native quantity(string) parsing logic, which rejects malformed strings [6][7][8]. String values like "10000000.0" are generally parsed as valid quantities, provided they do not exceed the internal architectural limits (magnitude constraints) imposed by the library, which rounds or caps values that are too large or too precise [1]. In summary, the Quantity library handles numeric strings, including fractional ones [1]. The "negative scale" mentioned is an internal mechanism for representing decimals and exponents [2], while MilliValue is a specific utility that requires caution regarding int64 overflow [5]. Validation logic for these fields typically relies on standard parsing or explicit constraints (like checking for non-negative values) to ensure data integrity [4][6].
Citations:
- 1: https://dev-k8sref-io.web.app/docs/common-definitions/quantity-/
- 2: https://github.com/kubernetes/apimachinery/blob/4427f8f31dfbac65d3a044d0168f84c51bfda440/pkg/api/resource/quantity.go
- 3: https://github.com/kubeflow/crd-validation/blob/master/vendor/k8s.io/apimachinery/pkg/api/resource/quantity.go
- 4: https://github.com/kubernetes/kubernetes/blob/f16bfb069a22241a5501f6fe530f5d4e2a82cf0e/pkg/apis/core/validation/validation.go
- 5: No overflow validation when using MilliValue() kubernetes/kubernetes#128684
- 6: https://kubernetes.io/docs/reference/using-api/cel/
- 7: https://docs.pynenc.org/projects/piceli/en/latest/kubernetes_model/piceli_templates/auxiliary/quantity.html
- 8: https://oneuptime.com/blog/post/2026-02-09-cel-complex-field-validation/view
🌐 Web query:
kubernetes apimachinery v0.36.3 Int64Amount Scale negative AsInt64 code
💡 Result:
In Kubernetes apimachinery (including v0.36.x), the int64Amount type uses the AsInt64() method to attempt a fast conversion of a fixed-point value into an int64 at scale 0 [1][2]. When the scale of an int64Amount is negative, the AsInt64() method explicitly returns 0, false [1][2]: func (a int64Amount) AsInt64 (int64, bool) { if a.scale == 0 { return a.value, true } if a.scale < 0 { // TODO: attempt to reduce factors, although it is assumed that factors are reduced prior // to the int64Amount being created. return 0, false } return positiveScaleInt64(a.value, a.scale) } The code includes a TODO comment indicating that while the method currently returns false for negative scales, it could theoretically attempt to reduce factors; however, it assumes that factors are already reduced before the int64Amount is created [1][2]. Consequently, if a consumer encounters a negative scale, the method signals that the value cannot be represented as an int64 at scale 0 without potential loss of precision or simply because the fast-path optimization does not support that transformation [1][2]. If AsInt64() returns false, callers are generally expected to use the inf.Dec representation of the Quantity instead [3][4].
Citations:
- 1: https://github.com/kubernetes/apimachinery/blob/v0.35.3/pkg/api/resource/amount.go
- 2: https://github.com/kubernetes/apimachinery/blob/5a348c53/pkg/api/resource/amount.go
- 3: https://github.com/kubernetes/apimachinery/blob/master/pkg/api/resource/quantity.go
- 4: https://github.com/kubernetes/apimachinery/blob/v0.24.2/pkg/api/resource/quantity.go
Handle AsInt64() failures before falling back to percentage accounting.
Negative internal scales make AsInt64() return false, so whole-number decimal quantities such as 10000000.0 leave memnum == 0. This makes MemPercentagereq default to 100, causing Fit to use percentage accounting instead of the clamped request. Compare the Quantity with the intended bounds first, then convert only after the bounds check passes.
🤖 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/cambricon/device.go` around lines 255 - 260, Update the
memory-request conversion logic surrounding memnums and memnum to compare the
Quantity against the intended lower and upper bounds before calling AsInt64().
If the request is out of range, clamp memnum to math.MaxInt32; otherwise convert
the validated quantity to the scaled integer, ensuring AsInt64() failures cannot
leave memnum at zero and trigger percentage accounting.
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/cambricon/device_test.go`:
- Around line 291-328: Add a negative vmemory regression case to the
resource-request table using resource.MustParse("-1") for
cambricon.com/mlu.smlu.vmemory. In Test_GenerateResourceRequests, assert Memreq
is math.MaxInt32 while preserving the existing Nums, Type, and Coresreq
expectations.
🪄 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: 7fd82570-9067-48eb-bad4-41a27d902156
📒 Files selected for processing (2)
pkg/device/cambricon/device.gopkg/device/cambricon/device_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/device/cambricon/device.go
| { | ||
| name: "vmemory expressed in Gi units no longer wraps to zero", | ||
| args: corev1.Container{ | ||
| Resources: corev1.ResourceRequirements{ | ||
| Limits: corev1.ResourceList{ | ||
| "cambricon.com/mlu": resource.MustParse("1"), | ||
| "cambricon.com/mlu.smlu.vmemory": resource.MustParse("16Gi"), | ||
| "cambricon.com/mlu.smlu.vcore": resource.MustParse("2"), | ||
| }, | ||
| }, | ||
| }, | ||
| want: device.ContainerDeviceRequest{ | ||
| Nums: int32(1), | ||
| Type: CambriconMLUDevice, | ||
| Memreq: int32(math.MaxInt32), | ||
| MemPercentagereq: int32(0), | ||
| Coresreq: int32(2), | ||
| }, | ||
| }, | ||
| { | ||
| name: "oversized plain vmemory value clamps to max int32", | ||
| args: corev1.Container{ | ||
| Resources: corev1.ResourceRequirements{ | ||
| Limits: corev1.ResourceList{ | ||
| "cambricon.com/mlu": resource.MustParse("1"), | ||
| "cambricon.com/mlu.smlu.vmemory": resource.MustParse("10000000"), | ||
| "cambricon.com/mlu.smlu.vcore": resource.MustParse("2"), | ||
| }, | ||
| }, | ||
| }, | ||
| want: device.ContainerDeviceRequest{ | ||
| Nums: int32(1), | ||
| Type: CambriconMLUDevice, | ||
| Memreq: int32(math.MaxInt32), | ||
| MemPercentagereq: int32(0), | ||
| Coresreq: int32(2), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a regression case for negative memory values.
These cases cover 16Gi and an oversized positive value. They do not cover the negative-input branch described by this PR. Add a case with resource.MustParse("-1") for cambricon.com/mlu.smlu.vmemory. Assert Memreq == math.MaxInt32 and the existing Nums, Type, and Coresreq values. Otherwise, the negative clamp can regress without failing Test_GenerateResourceRequests.
As per PR objective, negative requests must be clamped to math.MaxInt32.
Suggested test case
{
+ name: "negative vmemory value clamps to max int32",
+ args: corev1.Container{
+ Resources: corev1.ResourceRequirements{
+ Limits: corev1.ResourceList{
+ "cambricon.com/mlu": resource.MustParse("1"),
+ "cambricon.com/mlu.smlu.vmemory": resource.MustParse("-1"),
+ "cambricon.com/mlu.smlu.vcore": resource.MustParse("2"),
+ },
+ },
+ },
+ want: device.ContainerDeviceRequest{
+ Nums: int32(1),
+ Type: CambriconMLUDevice,
+ Memreq: int32(math.MaxInt32),
+ MemPercentagereq: int32(0),
+ Coresreq: int32(2),
+ },
+ },📝 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.
| { | |
| name: "vmemory expressed in Gi units no longer wraps to zero", | |
| args: corev1.Container{ | |
| Resources: corev1.ResourceRequirements{ | |
| Limits: corev1.ResourceList{ | |
| "cambricon.com/mlu": resource.MustParse("1"), | |
| "cambricon.com/mlu.smlu.vmemory": resource.MustParse("16Gi"), | |
| "cambricon.com/mlu.smlu.vcore": resource.MustParse("2"), | |
| }, | |
| }, | |
| }, | |
| want: device.ContainerDeviceRequest{ | |
| Nums: int32(1), | |
| Type: CambriconMLUDevice, | |
| Memreq: int32(math.MaxInt32), | |
| MemPercentagereq: int32(0), | |
| Coresreq: int32(2), | |
| }, | |
| }, | |
| { | |
| name: "oversized plain vmemory value clamps to max int32", | |
| args: corev1.Container{ | |
| Resources: corev1.ResourceRequirements{ | |
| Limits: corev1.ResourceList{ | |
| "cambricon.com/mlu": resource.MustParse("1"), | |
| "cambricon.com/mlu.smlu.vmemory": resource.MustParse("10000000"), | |
| "cambricon.com/mlu.smlu.vcore": resource.MustParse("2"), | |
| }, | |
| }, | |
| }, | |
| want: device.ContainerDeviceRequest{ | |
| Nums: int32(1), | |
| Type: CambriconMLUDevice, | |
| Memreq: int32(math.MaxInt32), | |
| MemPercentagereq: int32(0), | |
| Coresreq: int32(2), | |
| }, | |
| }, | |
| { | |
| name: "vmemory expressed in Gi units no longer wraps to zero", | |
| args: corev1.Container{ | |
| Resources: corev1.ResourceRequirements{ | |
| Limits: corev1.ResourceList{ | |
| "cambricon.com/mlu": resource.MustParse("1"), | |
| "cambricon.com/mlu.smlu.vmemory": resource.MustParse("16Gi"), | |
| "cambricon.com/mlu.smlu.vcore": resource.MustParse("2"), | |
| }, | |
| }, | |
| }, | |
| want: device.ContainerDeviceRequest{ | |
| Nums: int32(1), | |
| Type: CambriconMLUDevice, | |
| Memreq: int32(math.MaxInt32), | |
| MemPercentagereq: int32(0), | |
| Coresreq: int32(2), | |
| }, | |
| }, | |
| { | |
| name: "negative vmemory value clamps to max int32", | |
| args: corev1.Container{ | |
| Resources: corev1.ResourceRequirements{ | |
| Limits: corev1.ResourceList{ | |
| "cambricon.com/mlu": resource.MustParse("1"), | |
| "cambricon.com/mlu.smlu.vmemory": resource.MustParse("-1"), | |
| "cambricon.com/mlu.smlu.vcore": resource.MustParse("2"), | |
| }, | |
| }, | |
| }, | |
| want: device.ContainerDeviceRequest{ | |
| Nums: int32(1), | |
| Type: CambriconMLUDevice, | |
| Memreq: int32(math.MaxInt32), | |
| MemPercentagereq: int32(0), | |
| Coresreq: int32(2), | |
| }, | |
| }, | |
| { | |
| name: "oversized plain vmemory value clamps to max int32", | |
| args: corev1.Container{ | |
| Resources: corev1.ResourceRequirements{ | |
| Limits: corev1.ResourceList{ | |
| "cambricon.com/mlu": resource.MustParse("1"), | |
| "cambricon.com/mlu.smlu.vmemory": resource.MustParse("10000000"), | |
| "cambricon.com/mlu.smlu.vcore": resource.MustParse("2"), | |
| }, | |
| }, | |
| }, | |
| want: device.ContainerDeviceRequest{ | |
| Nums: int32(1), | |
| Type: CambriconMLUDevice, | |
| Memreq: int32(math.MaxInt32), | |
| MemPercentagereq: int32(0), | |
| Coresreq: int32(2), | |
| }, | |
| }, |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 304-304: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(math.MaxInt32)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 323-323: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(math.MaxInt32)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🤖 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/cambricon/device_test.go` around lines 291 - 328, Add a negative
vmemory regression case to the resource-request table using
resource.MustParse("-1") for cambricon.com/mlu.smlu.vmemory. In
Test_GenerateResourceRequests, assert Memreq is math.MaxInt32 while preserving
the existing Nums, Type, and Coresreq expectations.
2 participants
What this PR does
Clamps the cambricon memory request in
GenerateResourceRequeststo the int32 range instead of letting theint32cast silently wrap.Why
memnumsis computed as a 64-bit int and then narrowed withint32(), which wraps to 0 for any request abovemath.MaxInt32/256. Requestingcambricon.com/mlu.smlu.vmemoryin Gi units (e.g. 16Gi) therefore producedMemreq=0, soFit()scheduled the pod with no memory accounting and could oversubscribe an already-full MLU. Same class of bug fixed for enflame (#2145/#2190) and addressed for the other backends in #2338.See #2278.
What changed
pkg/device/cambricon/device.go: if the memory request exceedsmath.MaxInt32/256(or is negative), clampmemnumtomath.MaxInt32and log an error; otherwise apply the scaling.pkg/device/cambricon/device_test.goverifying the clamp and in-range preservation.Verification
go test ./pkg/device/cambricon/ -count=1.AI assistance disclosure
The PR description was generated with the help of AI assistance.
Fixes #2278
Summary by CodeRabbit