Skip to content

docs(resources): stop calling requests == limits "the chart contract" (backend#2872) - #609

Merged
LukasWodka merged 2 commits into
developfrom
docs/2872-cli-qos-claims
Aug 31, 2026
Merged

docs(resources): stop calling requests == limits "the chart contract" (backend#2872)#609
LukasWodka merged 2 commits into
developfrom
docs/2872-cli-qos-claims

Conversation

@LukasWodka

@LukasWodka LukasWodka commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Part of tracebloc/backend#2872 — the cli half. Sibling: tracebloc/client#922.

Comment-only; no behaviour change.

The correction is narrower than the ticket implies — and I want that on the record

#2872 lists three cli sites among "ten false Guaranteed QoS claims". Checked against the code: the requests == limits part is accurate here. BuildEnvSpec writes RESOURCE_REQUESTS and RESOURCE_LIMITS as literally the same string, both dimensions, and the spawned job template carries no init containers — so a CPU training pod really does come out Guaranteed.

What was false is the framing around it:

claim why it's wrong
"the chart contract" The chart's derive path (DERIVE_JOB_ENVELOPE) has written no cpu limit at all since backend#2418. Equal requests and limits is this package's choice for the explicit envelope, not an invariant of the chart. Calling it a contract is precisely what let the claim survive after #2418 falsified it everywhere else.
Guaranteed, unqualified A GPU pod is BestEffort whatever this writes: client-runtime._get_gpu_resources sets only nvidia.com/gpu and ephemeral-storage, and neither counts toward the QoS class — ComputePodQOS skips anything isSupportedQoSComputeResource rejects (backend#2871).
"cpu=2,memory=8Gi" named as RESOURCE_LIMITS Stale. The built-in fallback has been the contract floor cpu=1,memory=2Gi since backend#2254 — DefaultTraining() in this very file already derives it from the contract.

Four sites: resources.go package doc, resources.go ParseTraining doc (a fourth instance the ticket didn't list), set.go BuildEnvSpec doc, and provenance_test.go's test failure message — which is where a wrong explanation does the most damage, since it is read only by whoever just broke the thing, at the moment they are deciding what the rule was.

Left alone: resources.go:223 "the format is a stable chart contract" — that's about the cpu=X,memory=Y string format, which is accurate and unrelated to QoS.

Test plan

go build ./... && go vet ./... && go test ./... -count=1 — green; gofmt clean. Comment-only, so there is nothing new to assert here; the assertion that can fail lives in the sibling PR (client#922 adds pod-qos-class.{py,bats}, which derives each pod's class with the kubelet's own rule and includes the gpu-only → BestEffort case that grounds the second row above).

Checklist

  • Targets develop
  • No behaviour change
  • The narrower scope vs the ticket is documented above rather than silently applied

🤖 Generated with Claude Code


Note

Low Risk
Comment and test-message edits only; no logic or API changes.

Overview
Comment-only documentation fix for internal/resources (CLI half of backend#2872); bumps VERSION to 0.10.18. No runtime behavior changes.

Docs and one test failure message no longer describe equal RESOURCE_REQUESTS / RESOURCE_LIMITS as “the chart contract” or as universally Guaranteed QoS. They now state that matching requests and limits is BuildEnvSpec’s choice for the explicit envelope (Guaranteed for CPU training pods), while the chart derive path omits CPU limits since backend#2418, and GPU pods stay BestEffort regardless (backend#2871).

The package doc also replaces the stale cpu=2,memory=8Gi example with the real contract floor cpu=1,memory=2Gi (backend#2254). Updates land in resources.go (package + ParseTraining), set.go (BuildEnvSpec), and provenance_test.go.

Reviewed by Cursor Bugbot for commit 52fefba. Bugbot is set up for automated code reviews on this repo. Configure here.

… (backend#2872)

Four comments in this package explained the envelope by asserting a QoS class or
a chart-wide contract. The requests == limits part is accurate here --
BuildEnvSpec writes RESOURCE_REQUESTS and RESOURCE_LIMITS as the same string, so
a CPU training pod does come out Guaranteed. What was false is the framing:

  * "the chart contract" -- the chart DERIVE path (DERIVE_JOB_ENVELOPE) has
    written no cpu limit at all since backend#2418, so equal requests and limits
    is this package's choice for the explicit envelope, not an invariant of the
    chart. Calling it a contract is what let the claim survive after #2418
    falsified it everywhere else.
  * a GPU pod is BestEffort no matter what this writes: client-runtime's GPU path
    sets only nvidia.com/gpu and ephemeral-storage, and neither counts toward the
    QoS class (backend#2871).
  * the "cpu=2,memory=8Gi" literal named as RESOURCE_LIMITS is stale -- the
    built-in fallback has been the contract floor cpu=1,memory=2Gi since
    backend#2254.

Includes a test FAILURE MESSAGE (provenance_test.go), which is where a wrong
explanation does the most damage: it is read only by whoever just broke the
thing, at the moment they are deciding what the rule was.

Comment-only; no behaviour change.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka LukasWodka self-assigned this Aug 31, 2026
@LukasWodka

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 3fdbf5f. Configure here.

@saqlainsyed007 saqlainsyed007 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 52fefba8. A comment/message accuracy fix — no behaviour change, and the correction is the right one. Approving.

What it is (backend#2872). Several comments (and one test-failure message) in internal/resources asserted that RESOURCE_REQUESTS == RESOURCE_LIMITS is "the chart contract". That overclaims: since backend#2418 the chart's own derive path (DERIVE_JOB_ENVELOPE) writes no cpu limit at all, and a GPU pod is BestEffort regardless (backend#2871). So requests==limits is this package's choice for the explicit envelope (yielding a Guaranteed CPU pod), not a chart-wide invariant — and calling it a contract is exactly what let the claim survive after #2418 falsified it elsewhere. The fallback floor is also corrected to the real cpu=1,memory=2Gi (backend#2254), not the stale cpu=2,memory=8Gi.

The assertion itself is unchanged (if env["RESOURCE_REQUESTS"] != env["RESOURCE_LIMITS"]) — only its failure message is reworded, so the test still checks the same thing. VERSION bump 0.10.17→0.10.18 is the repo's required version-bump-gate action (green). CI green, MERGEABLE, no open threads. LGTM.

@LukasWodka
LukasWodka merged commit 06f48cf into develop Aug 31, 2026
31 of 33 checks passed
@LukasWodka
LukasWodka deleted the docs/2872-cli-qos-claims branch August 31, 2026 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants