feat(tests): track and ratchet the control-plane request footprint (backend#2870) - #944
Conversation
…ackend#2870) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… closed on a failed render (backend#2870) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
…unparseable (backend#2870) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
|
Addressed the 'Unparsable memory units count as zero' Medium (thread went outdated after the push): |
There was a problem hiding this comment.
✅ 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 cf410f0. Configure here.
# Conflicts: # Makefile
The fixture DaemonSet had no replicas, so the special case and the missing-replicas fallback both yielded 1 and deleting the arm changed nothing. A stray replicas: 3 makes the two disagree. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
aptracebloc
left a comment
There was a problem hiding this comment.
First-pass code review — the guard itself is excellent, but this can't land yet.
Blocker: the PR is currently CONFLICTING with develop (mergeable=CONFLICTING/DIRTY). It needs a develop-merge to resolve the conflict before it can be approved or merged.
On the code, the house bar for a ratchet is met:
- Derived, not restated: the footprint is summed from
helm templateacross everyclient/ci/*-values.yamlprofile, and the reserve is parsed live from the installer's own_TB_ENVELOPE_OVERHEAD_MEM_BYTES/_TB_ENVELOPE_OVERHEAD_CPU_MILLIconstants — verified those exist as literal integers, so a rename reddens rather than silently passing. The only hand-written numbers are the ratchet ceilings (3136 MiB / 900 m), which are the bounds to hold under. - Ratchet direction is correct: it fails when the footprint GROWS past the ceiling; the bats suite pins this (green at ceiling, reddens at
TB_CP_FOOTPRINT_MEM_CEIL=3000). - Fail-closed throughout: non-zero/partial render, empty render, zero-request sum, unparseable quantity, and unreadable reserve all refuse rather than undercount. Deterministic render → low flake risk.
Two low, non-blocking nits: (1) init-container requests are folded into the "steady-state" sum even though init containers aren't resident and the scheduler treats their request as max() not sum() — conservative for a ratchet, but worth a comment or exclusion to match the "STEADY STATE ONLY" note; (2) the already-Bugbot-flagged predictable /tmp/cp-footprint-helm.$$ stderr path — the manifest path a few lines down already uses mktemp, so mirror that.
Resolve the conflict with a develop-merge and this is in good shape.
— drafted with Claude Code
# Conflicts: # Makefile
… path (backend#2870) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
|
Rebased onto develop (the DRIFT_GUARDS conflict was #933's one-guard-per-line reformat vs my single-line addition — resolved to develop's format with |
…d them (backend#2870) @aptracebloc nit 1, and it is more than a nit. A pod effective request per resource is max( sum(app containers), max(init containers) ): init containers run to completion ONE AT A TIME before the app containers start, so they are never resident alongside them. This summed both, which CONTRADICTED the guard own "STEADY STATE ONLY" note and inflated the figure the ceiling is compared against. Conservative is not harmless here -- this number is the input to backend#2460/#2461, so an inflated one sends that work after MiB that no scheduler ever reserves. PER-RESOURCE, not per-pod: k8s takes the max independently for memory and cpu. THE NUMBER DOES NOT MOVE: 3136 MiB / 900 m either way, because this chart init containers carry no requests. So the "64 MiB OVER" finding STANDS -- the method was wrong, the conclusion was not. Worth saying rather than leaving the doubt open. WHICH MEANS THE FIX IS INERT ON THE REAL RENDER, and a green run proves nothing about the formula. So the new fixture is built where the two answers disagree: app 300Mi/200m, init max 1000Mi/500m -> scheduler 1000 MiB / 500 m against sum-everything 1350 MiB / 750 m. BOTH numbers are asserted rather than "under the ceiling", which would pass on either formula. The 1350/750 figure was MEASURED by running the previous code in place, not computed by hand. Mutation-proved: max -> + reddens the fixture test. Only the formula, rebased onto ab756ea. My own mktemp fix for the same Bugbot Low is DISCARDED -- the co-driver got there first and theirs is better, using mktemp "${TMPDIR:-/tmp}/cp-footprint-helm.XXXXXX" rather than a bare mktemp. control-plane-footprint.bats 9 passing. gen-manifest --check clean, shellcheck -S warning -x clean, bash -n clean. The ~1600-test client bats suite is CI job. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
All three closed. Your nit 1 turned out to be the substantive one. Blocker — conflict resolvedThe Nit 1 — not a nit, and you were right about the directionA pod's effective request per resource is Conservative isn't harmless here, which is why I didn't just add the comment you offered as the alternative: this guard's number is the input to backend#2460/#2461, so an inflated one sends that work chasing MiB no scheduler ever reserves. The number does not move: 3136 MiB / 900 m either way, because this chart's init containers carry no requests. So the "64 MiB OVER" finding stands — the method was wrong, the conclusion wasn't. I'd rather say that explicitly than leave you wondering whether the gap was an artifact. And that means the fix is inert on the real render, so a green run proves nothing about the formula. The new fixture is therefore built where the two answers disagree: Both numbers asserted, not "under the ceiling" — a total would pass on either formula. The Nit 2 / Bugbot Low — superseded, and I discarded mineA co-driver landed Evidence
Re-requesting review. |
|
bugbot run |
There was a problem hiding this comment.
✅ 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 9f5f103. Configure here.
aptracebloc
left a comment
There was a problem hiding this comment.
Re-review at 9f5f1033 — the develop-merge cleared the conflict and the guard came through fully intact. Approving.
Re-verified against origin/develop:
- Ceilings unchanged / ratchet non-weakened: still
3136MiB /900m (TB_CP_FOOTPRINT_{MEM,CPU}_CEILdefaults), fails when the footprint grows past either. Footprint still summed fromhelm templateacross everyclient/ci/*-values.yamlprofile; reserve still parsed live from the installer's_TB_ENVELOPE_OVERHEAD_MEM_BYTES/_TB_ENVELOPE_OVERHEAD_CPU_MILLI, fail-closed if either moves. - Drift wiring survived #933's rework:
control-plane-footprint.shis the final entry of the new multilineDRIFT_GUARDS, so the requiredSource-of-truth driftjob (make drift) runs it — green. - Bats still pins the reddening: green at the ceiling, red (exit 1, "exceed the recorded ceiling") once it's breached, plus the fail-closed branches.
- No conflict markers, no dropped hunks (diff is exactly Makefile + the two guard files).
Both earlier low notes are now closed: the /tmp/…$$ stderr path is mktemp'd, and the init-container handling is now the scheduler's real max(sum(app), max(init)) rather than a sum — with a falsifiable bats fixture (asserts 1000 MiB / 500 m, not 1350/750) proving the formula, and the chart number confirmed unchanged either way. Nice.
— drafted with Claude Code
# Conflicts: # Makefile
aptracebloc
left a comment
There was a problem hiding this comment.
Re-review after the develop-merge that dismissed my prior approval. The merge is clean and additive: the guard's scripts/tests/control-plane-footprint.sh and .bats are byte-identical to the head I approved at 9f5f1033, and the only Makefile change since is develop inserting a sibling guard above the footprint entry in DRIFT_GUARDS — the footprint entry itself is untouched and still wired into the required Source-of-truth drift job. Ceilings unchanged (3136 MiB / 900 m), footprint still summed from helm template across every client/ci/*-values.yaml profile, reserve still parsed live from _TB_ENVELOPE_OVERHEAD_MEM_BYTES / _TB_ENVELOPE_OVERHEAD_CPU_MILLI, fail-closed on empty profiles / non-zero render / unreadable reserve, and the bats suite still pins green-at-ceiling, reddening-when-exceeded, the falsifiable init-container max(), and the fail-closed branches. No conflict markers, no dropped hunks. CI green, no open Bugbot threads. Re-approving.
— drafted with Claude Code
The measurement-free part of backend#2870, landed green and safe. It does not touch the fleet envelope contract, does not change any install behavior, and does not assert schedulability (which is red today by construction). It closes the ticket's core finding — "nothing sums the real chart footprint … grep: zero hits" — and stops that footprint silently growing worse.
What it does
scripts/tests/control-plane-footprint.shrenders the chart across everyclient/ci/*-values.yamlprofile and sums the steady-state control-plane requests (Deployments × replicas + DaemonSets × 1; Job/CronJob hooks excluded — they're one-shot conformance runs, not resident beside a training pod). Measured, consistent across all four profiles:It ratchets, it does not assert schedulability. DoD part 2 wants
envelope + control_plane ≤ allocatable, but that is red today (the 64–76 MiB overshoot this ticket is about), and making it green needs measured per-platform kubelet reservations (backend#2460) and/or a control-plane trim under load (backend#2461) — both empirical campaigns, neither a code change, and landing a red gate trains people to skip the tier (org rule 4). So instead it holds the footprint at or under a recorded ceiling: a chart change that adds a component or raises a request — making an already-tight fit worse, silently, on every edge — reddens the requiredSource-of-truth driftjob and forces the author to see the cost and bump the ceiling deliberately. And it reports the current gap against the reserve, so the number backend#2460/#2461 must close is tracked rather than rediscovered.Why this and not more
The full #2870 fix (installer refuses/reduces using the real footprint) can't be self-contained: the runtime installer guarantees neither
jqnorpython3(line 163: "jq is not a guaranteed prerequisite"), which is why every envelope number is pre-computed into embedded constants. So the footprint clamp would have to go into the same generated, drift-gated fleet contract asoverhead— and closing the gap there is arithmetically backend#2460 (a measured, per-platform kubelet reservation), whose own DoD warns "right-sizing from idle numbers is how you get an OOMKilled control plane under load." I won't ship guessed reservations that canexit 137a customer's control plane. This PR is the part that needs no live measurement.It derives, it does not restate (backend#1729)
helm template(the ground truth of what installs), not a hand-copied total;_TB_ENVELOPE_OVERHEAD_*constant, not a second copy — a rename reddens rather than passing;Evidence
sum_control_plane_requests.pysplit out so the arithmetic is unit-tested without helm —control-plane-footprint.batsproves Gi→MiB, Deployment×replicas, DaemonSet×1, Job excluded (2176/550/2 from a mixed fixture).make drift— all 39 guards green (this is Develop #39, now in the requiredSource-of-truth driftjob).check-style,check-facts, shellcheck,py_compileclean.Part of tracebloc/backend#2870 (does not close it — the schedulability assertion and the installer clamp wait on backend#2460/#2461's measurements).
Note
Low Risk
Observability and CI gating only—no chart or installer behavior changes—but merge-blocking drift now depends on helm, python3, and PyYAML for footprint math.
Overview
Adds a required drift guard that measures steady-state control-plane memory/CPU requests from
helm templateacross everyclient/ci/*-values.yamlprofile, compares them to recorded ceilings (3136 MiB / 900m), and reports overshoot vs installer-derived_TB_ENVELOPE_OVERHEAD_*reserves—without asserting schedulability (that gap stays informational until backend#2460/#2461).The summation logic is inlined in
control-plane-footprint.sh: only Deployments/StatefulSets/DaemonSets (Jobs excluded), DaemonSets counted ×1, Kubernetes scheduler mathmax(sum(apps), max(inits))per resource, full quantity parsing with fail-closed on bad units or failed/partial renders, and reserve constants parsed frominstall-client-helm.shrather than duplicated.control-plane-footprint.batsexercises arithmetic viaTB_CP_FOOTPRINT_FIXTURE, ratchet breach, and fail-closed paths. The guard is wired intoDRIFT_GUARDSin the Makefile (with the prior line continuation fix forjobs-manager-waits-for-mysql.sh).Reviewed by Cursor Bugbot for commit 5829271. Bugbot is set up for automated code reviews on this repo. Configure here.