Skip to content

test(installer): pin derived RESOURCE_LIMITS to the chart schema (client#836) - #850

Merged
aptracebloc merged 1 commit into
developfrom
fix/836-limits-schema-regression-test
Aug 26, 2026
Merged

test(installer): pin derived RESOURCE_LIMITS to the chart schema (client#836)#850
aptracebloc merged 1 commit into
developfrom
fix/836-limits-schema-regression-test

Conversation

@aptracebloc

@aptraceblocaptracebloc commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

What & why

Closes#836.

When TRACEBLOC_TRAINING_RESOURCES is unset, the VM-ceiling sizing (backend#2221 / #804) derives a training size like cpu=9,memory=12Gi. The L0.2 limits half (backend#2418 / #820) then drops cpu= on purpose — a cpu limit becomes a cpu.max quota that throttles even on an idle box — so RESOURCE_LIMITS ships memory-only (memory=12Gi). A pre-backend#2223 chart schema pinned RESOURCE_LIMITS to ^(cpu=\S+,memory=\S+)?$, which rejects a memory-only value and aborts helm install:

at '/env/RESOURCE_LIMITS': 'memory=12Gi' does not match pattern '^(cpu=\S+,memory=\S+)?$'

Root cause is already resolved in the chart. backend#2223 (#812) loosened the schema to admit any subset of the closed vocabulary, one day before #820 made limits memory-only — the two are a coordinated pair, and the current client/values.schema.json accepts memory=12Gi. The failure in the issue was against a stale pre-#812 chart. Reverting the memory-only design (to always emit cpu=+memory=) would re-introduce the idle-cpu throttling #820 fixed and break its tests, so this PR is regression tests only — no production code changes.

Changes (test-only)

  • scripts/tests/install-client-helm.bats — drives the real derivation (_resolve_training_size_training_limits) for the VM-ceiling repro (cpu=9,memory=12Gimemory=12Gi) and asserts the derived RESOURCE_LIMITS matches the pattern read fromclient/values.schema.json. A re-tightening back to the strict pattern reddens here instead of at a customer's helm install.
  • scripts/tests/chart-env-vocabulary.sh — renders the same memory=12Gi through the real chart schema via helm template (the authoritative validator, run in helm-ci), alongside the existing subset cases.

Verification

  • make lint clean (56 scripts parse; shellcheck 63 files).
  • Full install-client-helm.bats suite green (211/211).
  • Real-helm check confirmed: memory=12Gi renders; a memroy=12Gi typo is rejected (proves the schema is actually evaluated).
  • Confirmed the bats guard rejects the value under the old strict pattern, so it is a live guard, not a no-op.

🤖 Generated with Claude Code


Note

Low Risk
Only adds automated tests; no production, chart, or schema changes.

Overview
Test-only regression coverage for client#836 — no installer or chart logic changes.

VM-ceiling sizing plus L0.2 _training_limits can emit memory-onlyRESOURCE_LIMITS (e.g. memory=12Gi). These tests lock that derived value to the shipped chart vocabulary so a schema re-tightening fails in CI instead of at helm install.

  • install-client-helm.bats — New test runs _resolve_training_size_training_limits for the client#836 repro (cpu=9,memory=12Gimemory=12Gi) and checks the result against the RESOURCE_LIMITS pattern read from client/values.schema.json (Python re, since helm is mocked in bats).
  • chart-env-vocabulary.sh — Adds an accept case that renders memory=12Gi for both RESOURCE_REQUESTS and RESOURCE_LIMITS via real helm template / schema validation.

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

…ent#836)
When TRACEBLOC_TRAINING_RESOURCES is unset, the VM-ceiling sizing
(backend#2221 / #804) derives e.g. cpu=9,memory=12Gi, and the L0.2 limits
half (backend#2418 / #820) drops cpu so RESOURCE_LIMITS ships memory-only
(memory=12Gi). A pre-backend#2223 chart schema pinned RESOURCE_LIMITS to
`^(cpu=\S+,memory=\S+)?$`, which rejects that value, aborting `helm install`.
backend#2223 (#812) already loosened the schema to admit any subset, so the
current chart accepts memory-only limits — the two changes are a coordinated
pair. Rather than revert the memory-only design, this adds regression tests
that keep the derivation and the schema pinned together:
- install-client-helm.bats: drives the real derivation
(_resolve_training_size then _training_limits) for the VM-ceiling repro and
asserts the derived RESOURCE_LIMITS matches the pattern READ FROM
client/values.schema.json — so a re-tightening back to the strict pattern
reddens here, not at a customer's helm step.
- chart-env-vocabulary.sh: renders the same memory=12Gi through the REAL
chart schema via `helm template`, the authoritative validator.
No production code changes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@shujaatTraceblocshujaatTracebloc 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.

Approving. 51 checks pass, no open threads, and the tests hold the bar on both sides.

What I verified

Both halves are derived, not restated — which is the whole point of the PR and the thing that would have made it worthless if fudged:

  • The value comes from driving the real _resolve_training_size_training_limits, not from a literal "memory=12Gi" typed into the test. So it pins the derivation, not a snapshot of what the derivation happened to produce.
  • The pattern is read out of client/values.schema.json at runtime. A re-tightening back to the pre-#812^(cpu=\S+,memory=\S+)?$ reddens here rather than at a customer's helm install, which is exactly the failure client#836 describes.

The recursive pattern_for lookup is unambiguous today. I checked, because a recursive first-match search for a key name is the kind of thing that silently reads the wrong node: there is exactly one RESOURCE_LIMITS in the schema, at /properties/env/properties/RESOURCE_LIMITS, and one RESOURCE_REQUESTS. It also fails loud (sys.exit("RESOURCE_LIMITS pattern not found in schema")) rather than skipping if the constraint ever stops being expressed as a pattern — right direction.

The reasoning for tests-only is sound and I'd have wanted it stated exactly this way. backend#2223 (#812) loosened the schema one day before backend#2418 (#820) made limits memory-only; the issue's failure was against a stale pre-#812 chart. Reverting to always emitting cpu=+memory= would re-introduce the idle-CPU throttling #820 removed. So the production fix already exists and what was missing was the guard that keeps the two coordinated — that's a regression test, not a code change.

One observation, non-blocking and it errs safe

re.match(pattern, value) is anchored at the start only, whereas JSON Schema's pattern is an unanchored search under ECMA-262 semantics. Today the schema's pattern carries both ^ and $, so the two agree exactly. If it ever loses its anchors, the Python proxy becomes stricter than the real validator — a false failure, not a false pass, so it can't let a bad value through silently.

Worth noting only because it's the sort of divergence that's easy to assume runs the other way. And the PR already handles it structurally: chart-env-vocabulary.sh renders the same memory=12Gi through the actual chart schema via helm template, and the bats half openly says it's a proxy because helm is stubbed in that suite. Having the authoritative check somewhere is what makes the proxy acceptable.

Nice touch confirming a memroy=12Gi typo is rejected — that's the check that proves the schema is genuinely being evaluated rather than the render silently passing everything.

@aptracebloc
aptracebloc merged commit 5306604 into developAug 26, 2026
52 of 53 checks passed
@aptracebloc
aptracebloc deleted the fix/836-limits-schema-regression-test branch August 26, 2026 10:19
Sign up for freeto 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.

RESOURCE_LIMITS auto-derivation can emit schema-invalid value (memory without cpu) → Helm install fails

3 participants

@aptracebloc@shujaatTracebloc@LukasWodka