cubemaster: stop shadowing err when parsing container resource quantities

[dwin-gharibi]

Closes #1462.

Motivation

getReqResource has a named return err, but the loop body used := when calling
resource.ParseQuantity, which declares a new err scoped to the loop body. The
err = fmt.Errorf(...) assignments therefore wrote to that shadow and were discarded at break,
leaving the named return nil.

Consequence: a CreateCubeSandbox request with an unparseable cpu or mem string was admitted as
consuming cpu=0, mem=0. Those values flow through checkAndGetReqResource into
selctx.RequestResource, which is what the scheduler filters on — so the sandbox passed every
resource filter and could land on any node regardless of its real footprint.

The ctr.Resources == nil branch a few lines above uses plain = on the named return and worked
correctly, which is what made the bug easy to miss.

What this changes

CubeMaster/pkg/service/sandbox/util.go — the two ParseQuantity calls now use distinct error
variables (cpuErr, memErr) so the fmt.Errorf assignments land on the named return:

ctncpuQuantity, cpuErr := resource.ParseQuantity(ctr.Resources.Cpu)
if cpuErr != nil {
    err = fmt.Errorf("parse container %q cpu limit: %w", ctr.Name, cpuErr)
    break
}

Three lines, no behaviour change on the success path, no comment changes.

Testing

New: CubeMaster/pkg/service/sandbox/util_resource_parse_test.go

• TestGetReqResourceRejectsUnparseableQuantities — three sub-cases (bad cpu, bad mem, both); each
  asserts a non-nil error that names both the offending field and the container.
• TestGetReqResourceSumsValidQuantities — regression guard that valid multi-container quantities
  still sum correctly (750m / 512Mi).

$ docker run --rm ... -w /w/CubeMaster golang:1.26 go test -short -v -run TestGetReqResource ./pkg/service/sandbox/
--- PASS: TestGetReqResourceRejectsUnparseableQuantities
--- PASS: TestGetReqResourceSumsValidQuantities
--- PASS: TestGetReqResourceRejectsCPUOverflowWhenMemIsValid        (pre-existing)
--- PASS: TestGetReqResourceRejectsCPUOverflowBeforeMemOverflow     (pre-existing)
ok  github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/service/sandbox

CI gates checked locally:

• gofmt -l ./pkg — clean (fmt-check).
• GOOS=linux go build ./... — clean.
• staticcheck -checks 'SA*' ./pkg/service/sandbox/ — the SA4006 / SA4017 findings on
  util.go:168 and :173 are gone.

Pre-existing failure, unrelated to this change

Running the whole pkg/service/sandbox package in Linux (as make test does) shows one failure:

--- FAIL: TestValidatePauseResumeVolumesPresent

I confirmed this is pre-existing on master, not introduced here:

• passes in isolation (-run TestValidatePauseResumeVolumesPresent) on both master and this branch;
• fails when the full package runs, on master as well as this branch.

It looks like cross-test state pollution within the package. Not fixed here to keep this PR
single-purpose; worth its own issue.

(On macOS the package additionally cannot run at all — gomonkey cannot patch binaries on darwin —
so Linux is the only meaningful local target, matching CI.)

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[cubesandboxbot]

Minor: this success-path assertion depends on the package-global config's scheduler limits staying above the summed values. getReqResource rejects the request when cpu.Cmp(MaxMvmCPURes()) >= 0 or mem.Cmp(MaxMvmMemoryRes()) >= 0, and the defaults (max CPU=100, mem=300Gi) are what keep this test green. If conf.yaml ever tightens those limits below 750m/512Mi — or an earlier test mutates cfg.Scheduler without restoring it (the PR notes this package already has cross-test state pollution) — this test fails for an unrelated reason.

Suggest mirroring the sibling overflow tests: call ensureSandboxTestConfig(t) and pin cfg.Scheduler (e.g. max CPU "100", mem "300Gi") before calling getReqResource, restoring afterward, so the test asserts only the summing behaviour it intends.

[cubesandboxbot]

Review: cubemaster: stop shadowing err when parsing container resource quantities (#1463)

AI-generated review — verified against the base-branch workspace.

Verdict: Approve (one minor test-hardening suggestion)

This is a real bug, and the fix is correct and well-tested.

The bug (confirmed)

getReqResource (CubeMaster/pkg/service/sandbox/util.go:193) has a named return err. Inside the loop, the original code used ctncpuQuantity, err := resource.ParseQuantity(...). Because ctncpuQuantity was new, the short-variable declaration also introduced a fresh err scoped to the loop body, shadowing the named return. The subsequent err = fmt.Errorf(...) assignments and break wrote to that shadow, which was discarded on loop exit — leaving the named return nil.

Consequence: a CreateCubeSandbox request with an unparseable cpu or mem string passed through checkAndGetReqResource as cpu=0, mem=0, bypassing the scheduler's resource filter. The ctr.Resources == nil branch above used plain = on the named return and worked, which made the bug easy to miss.

The fix (correct)

The two ParseQuantity calls now bind distinct locals (cpuErr, memErr), so err = fmt.Errorf(...) lands on the named return:

• Error path: the wrapped error now propagates out of getReqResource → checkAndGetReqResource returns (nil, err), so the request is rejected instead of being admitted with zero resources.
• Success path: cpuErr/memErr are nil, err stays nil — no behaviour change.
• No new unused-variable or shadowing issues; the new locals are used exactly once each.

The diff matches the base tree at util.go:202-211 exactly, and the change is minimal (6+/6-).

Tests (correct)

• TestGetReqResourceRejectsUnparseableQuantities covers bad cpu, bad mem, and both-bad. "10 GB", "not-a-quantity", "abc", "def" are all rejected by k8s.io/apimachinery/pkg/api/resource.ParseQuantity (spaces and non-numeric prefixes don't match the quantity grammar). Each case asserts the error names both the offending field ("cpu limit"/"mem limit") and the container ("c1"), which is exactly what the bug previously suppressed.
• TestGetReqResourceSumsValidQuantities verifies 500m + 250m = 750m and 256Mi + 256Mi = 512Mi, guarding the success path. These sums are below the default scheduler caps (CPU=100, mem=300Gi), so the test passes in the current config.

Minor suggestion (posted inline)

TestGetReqResourceSumsValidQuantities implicitly depends on the package-global config's scheduler limits staying above 750m/512Mi. It is the only getReqResource test that doesn't pin cfg.Scheduler via ensureSandboxTestConfig(t) like the sibling overflow tests do. Since the PR itself notes this package already exhibits cross-test state pollution, I'd suggest pinning the scheduler config in the sum test so it asserts only the summing behaviour, not whatever the ambient config happens to be.

Pre-existing failure note

The PR's claim that TestValidatePauseResumeVolumesPresent fails on master independent of this change is consistent with the workspace: nothing in this diff touches pause/resume volume validation or shared state used by it.