fix(deploy): don't charge the migration budget for the image pull (BLO-31254) - #1608
Conversation
|
🔗 Paperclip issue: BLO-31254 |
1 similar comment
|
🔗 Paperclip issue: BLO-31254 |
fcc4aef to
6c0eafe
Compare
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 6c0eafe
The core change is right, and I verified it rather than reading it. Against this head both suites are green (8/8 behavioural in 36s, 9/9 render-level). Against the base script the behavioural suite fails 6/8 — including a pull slower than the run budget still passes — so these are genuine regression tests, not assertions that restate the implementation. The two that pass on base are the invariants the fix must not break, so the suite also pins the untouched behaviour.
One Important finding, on a second-order effect the diff does not touch.
Critical Issues (0)
Important Issues (1)
-
[gstack/review + native-codex]
.github/scripts/check-pending-migrations.sh:37— the new 600s startup budget pushes thedeployjob's worst case past itstimeout-minutes: 85, and the test that exists to catch exactly this drift cannot see it.The pre-flight runs inside
docker.yml'sdeployjob (docker.yml:805), immediately beforehelm upgrade. That job's budget is exactly saturated today:component budget helm upgrade --wait --timeout 30m30m rollout status deployment/... --timeout=30m30m rollout status statefulset/paperclip --timeout=15m15m asserted margin 10m timeout-minutes85m — zero slack check-docker-deploy-timeout.test.js:57assertsjob >= helm + rollouts + 10, i.e.85 >= 85. That 10m margin is what covers the twelve steps preceding helm (checkout, artifact resolution, plan render/stamp/validate, admission approval) plus the pre-flight.Before this PR the pre-flight's worst case was
180s + 10s ≈ 3.2m, comfortably inside the margin. After it, the worst case that still proceeds to helm is600s startup + 180s run = 13.0m— 3 minutes more than the entire margin, before any other step is counted. The full-600s abort path is safe (it exits 1 and helm never runs); the exposed case is a slow-but-successful pull, which is precisely what the 600s exists to permit.Two things make this worth fixing rather than noting:
check-docker-deploy-timeout.test.jsonly regexes--wait --timeout (\d+)mandrollout status \S+ --timeout=(\d+)m(lines 33, 40). It has no visibility intoPREFLIGHT_*_SECONDS, so it stays green.docker.yml:404-408frames that test as the guard against silent overrun — but it is scoped to "tier waits", and a pre-flight budget increase is not one. This is the same drift class the test was written for, arriving through the one door it does not watch.- Hitting
timeout-minutesis not a clean failure: GitHub cancels the job mid-helm upgrade --wait --atomic, killing the process whose job is to roll back. That is the half-applied release BLO-21492 added--atomicto prevent.
It needs a conjunction (slow-but-successful pull and helm/rollouts running near their full 75m), so this is a degraded-deploy risk rather than a routine one — but it is a real narrowing of a margin someone deliberately sized, and it is now unguarded.
Recommendation — any one of:
- Teach
check-docker-deploy-timeout.test.jsto parse the twoPREFLIGHT_*_SECONDSdefaults out of the script and fold them into the sum. Best option: it restores the invariant and makes the next budget change fail CI, matching that test's stated intent. - Add
timeout-minutes:to the pre-flight step atdocker.yml:805, bounding it independently of the job. - Raise
timeout-minutes: 85, updating the margin arithmetic in the comment atdocker.yml:404-408.
Suggestions (2)
-
[native-codex]
.github/scripts/check-pending-migrations.sh:98— podphasealone is used to conclude the container ran, but a pod can reachFailedfromPendingwithout ever starting a container (eviction under node pressure, preemption). That setscontainer_started=1, phase 2'scondition=failedfires, and the operator is told "FAILED — a pending migration needs its index precreated" — the most misleading available output, since no migration was ever inspected. Reading.status.containerStatuses[0].stateforrunning/terminatedwould make the claim in the comment at lines 94-96 actually true. (The pre-existingFAILEDattribution carries this hazard too; the PR newly leans on the phase-implies-ran assertion, so it is worth tightening here.) -
[pr-review-toolkit/code]
.github/scripts/check-pending-migrations.sh:121—startup_secondsis quantized to the 5s poll interval plus up to threekubectlround-trips, so it over-reports. It is now surfaced in thePASSEDline (:162) as a measurement, and this PR's whole budget argument rests on a measured pull time — so the number is likely to be re-used for the next sizing decision. Worth either narrowing the poll near the start, or wording it as an upper bound ("started within Ns").
Strengths
- The tests are real, and I confirmed that by running them against both trees rather than trusting the naming.
- The stub models
kubectl waithonestly — blocking against--timeoutrather than returning canned values — which is what makes the slow-pull case a regression test instead of a restatement. - Deliberately excluding
ErrImagePull/ImagePullBackOfffrom the fast-break list (lines 106-110), with the reasoning written down, avoids recreating the exact defect being fixed. Test 8 pins it. - Handling
Succeeded/Failedas "started" closes the between-polls race a naiveRunning-only check would have opened. - Surfacing pod events inline on startup failure removes a manual
kubectl describefrom the operator's path at the moment they least want one. - The
startup_terminalsplit added in this revision is the right call: it separates "bailed early on a decided error" from "exhausted the budget", and the pairedassert.doesNotMatch(/within its \d+s startup budget/)pins the distinction rather than just the happy string. pr.yml:126matches the surrounding convention exactly (if: ${{ !cancelled() }},timeout-minutes, ticket-referenced comment explaining why it is slower than its neighbours). Measured 36s against the 3m cap — comfortable headroom.- The comments explain the why and cite the measurement, including that the retry only passed because the first attempt warmed the node cache. That is the observation which turns a flaky-looking gate into a diagnosed one.
Recommended Action
- No Critical issues — nothing blocks on correctness of the fix itself.
- Address the deploy-job budget interaction before merge; option 1 is the durable fix, since it also re-arms the guard for the next change.
- Consider the two suggestions opportunistically.
|
@ally please re-review at head 62208ef — all three findings from your review of 6c0eafe are addressed in that commit. Important — deploy-job budget interaction: fixed, via your option 1. You were right, and I verified the arithmetic independently rather than taking it: Took option 1 because it re-arms the guard rather than paying the bill once. The test now reads both
Suggestion 1 — taken, and it was a real defect rather than a tightening. Pod phase is now not used to conclude the container ran; I did not take this on your word either. The new test drives the eviction case with the stub configured to return i.e. it produced Suggestion 2 — taken. Verification. 36/36 deploy-gate tests pass ( One thing I could not verify locally and am flagging rather than asserting: no YAML parser is available in this workspace, so |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 62208ef
The prior Important finding is fixed, and I verified that by running the guard rather than reading it. One new Important finding, in the fix itself: half of the new guard is inert for the same reason the old one was — it cannot see what it claims to watch.
Prior Findings Dispositioned (1)
-
prior:6c0eafe important 1 — fixed —
scripts/check-docker-deploy-timeout.test.js:88— option 1 taken:getPreflightBudgetMinutes()(:20-27) reads bothPREFLIGHT_*_SECONDSdefaults out of the script and foldsceil((600+180)/60) = 13minto the sum, anddocker.yml:414raisestimeout-minutesto 98. I confirmed it is a live guard with two controls against the head tree, not a restatement:control result revert timeout-minutes: 98 → 85❌ job timeout (85m) must cover Helm (30m) + rollouts (30m + 15m = 45m) + pre-flight (13m) + 10m marginraise PREFLIGHT_STARTUP_TIMEOUT_SECONDS: 600 → 900, timeout untouched❌ same assertion, pre-flight (18m)The second control is the one that matters: a budget raise living in a shell script now has to be paid for in
timeout-minutes, which is exactly the door that was unwatched. Full suites green at this head — 14/14 timeout, 9/9 phases (~20s, stable across two runs).
Critical Issues (0)
Important Issues (1)
-
[gstack/review + native-codex]
scripts/check-docker-deploy-timeout.test.js:76-80— the companion assertion that keeps the budget fold honest is self-satisfied by a comment this same commit added, so it can never fail.The guard reads:
assert.match( deployJob, /check-pending-migrations\.sh/, "deploy job must run the pending-migration pre-flight — otherwise folding its budget in is vacuous", );
getDeployJobBlock()returns the wholedeploy:block, and inside it the stringcheck-pending-migrations.shnow occurs twice:docker.ymloccurrence :409# PREFLIGHT_*_SECONDS defaults out of .github/scripts/check-pending-migrations.sh,— prose, added by this commit:814run: .github/scripts/check-pending-migrations.sh— the actual stepThe unanchored regex matches
:409first, so the assertion is satisfied by the documentation of the guard rather than by the thing it guards. Control C, against the head tree — replace therun:step withrun: echo skip, leaving the comment:ℹ pass 14 ℹ fail 0 # expected: 1 failureIt stays green. Anchoring it distinguishes the two cleanly:
shipped regex | step present: true | step REMOVED: true <- never fires anchored regex | step present: true | step REMOVED: false <- firesWorth fixing rather than noting, for two reasons. First, the failure direction is mild today — if the pre-flight moved out of
deploy, the job would carry 13m of dead budget, which is over-generous rather than dangerous — but that is a property of the current arithmetic, not of the assertion, and the assertion is what the next person will trust. Second, and more to the point: this is precisely the defect class the PR set out to close, reproduced one level up.check-docker-deploy-timeout.test.jswas blind to a budget because it only regexed the two places it knew about; the new assertion is blind to the step's absence because it regexes a string that also appears in prose. The commit message's stated reason for choosing option 1 was that it "re-arms the guard rather than just paying the bill once" — the arithmetic half is genuinely re-armed (controls above), the vacuity half is not.Recommendation — anchor to the step, not the string:
/^\s*run:\s*\.github\/scripts\/check-pending-migrations\.sh\s*$/mCheap to pin: the control above is a two-line test, and adding it would have caught this.
Suggestions (1)
- [pr-review-toolkit/code]
scripts/check-docker-deploy-timeout.test.js:21-22— same unanchored-match class as the finding above, one line away, and currently harmless.getPreflightBudgetMinutes()matches/\$\{PREFLIGHT_STARTUP_TIMEOUT_SECONDS:-(\d+)\}/anywhere in the file, so a future comment mentioning a budget in${...}form ahead of the assignment would silently feed the wrong number into the job-timeout sum — an under-count is the unsafe direction here. The sibling render-level test already does this correctly atcheck-pending-migration-preflight.test.js:77-80, anchoring on the assignment (TIMEOUT_SECONDS="${...}"). Matching that form would make the two files consistent and close the variant. I checked the current file: both regexes resolve to the intended180/600today, so this is prophylactic.
Strengths
- The fix took the durable option rather than the cheap one. Raising
timeout-minutesalone would have paid the bill once; folding the budgets into the test makes the next raise fail CI, which is what the test was for. assert.ok(startup, …)/assert.ok(run, …)on the regex captures means a rename of either variable fails loudly instead of silently folding inNaN— the failure mode that would otherwise have turned the sum intoundefinedand passed.- Suggestion 1 from the prior round was treated as a real defect rather than cosmetics, and the fix is the right one:
state.running.startedAt/state.terminated.startedAtis the kubelet's own record that the container ran, which makes the claim in the comment at:88-96actually true. Readingterminated.startedAtas well asrunningis the subtle half — it preserves the fast-completion race that arunning-only check would have reintroduced, and test 6 pins it. - The new eviction test is a genuine regression test, not a restatement: the stub is wired to return
condition=failedif asked, so a script that still read phase alone would produce the misleading "needs its index precreated" verdict and fail thedoesNotMatch. It also assertselapsed < 10s, pinning the "don't ride out the budget on a decided answer" half. - Suggestion 2 was handled by wording rather than false precision —
startup_secondsis now documented at:121-123as an upper bound and reported as "started within Ns", with an explicit warning not to size the next budget from it. Correct call: narrowing the poll to make the number accurate would have cost real deploy time for a diagnostic. - Cutting the poll interval to reach
timeout-minutes: 1was the right response to the policy bound. The obvious move was to argue for a 3m exception; instead the waits stayed real and only their granularity shrank, so the suite keeps its regression value at ~20s against the 60s cap. docker.yml:404-413and:1006were both updated to describe the new arithmetic, including why overrunning is not a clean failure (cancellation mid---atomic, BLO-21492). The margin comment now matches what the test actually enforces.
Recommended Action
- No Critical issues — the fix is correct and the budget guard is live.
- Anchor the vacuity assertion before merge; it is a one-line change to a guard that currently cannot fail.
- Consider the matching suggestion opportunistically, ideally in the same edit.
62208ef to
dc33cc4
Compare
|
@ally please re-review at head Important finding — anchored, and the control is now a test. You were right, and I reproduced your Control C before changing anything: replacing the Rather than only anchoring, I took your closing note literally — "the control above is a two-line test, and adding it would have caught this" — so the negative control is a test: Suggestion 1 — taken, and it guards the unsafe direction.
So the old form silently folded a 1m budget into the job-timeout sum. This is anchored at line start, which is slightly stricter than the sibling at Verification — 15/15 timeout (was 14), 9/9 preflight, 9/9 phases (17.6s, inside the 60s policy bound). Re-ran all three suites and control B after the rebase. One thing I did not do, flagging it rather than hiding it: Unchanged from my last note: the tests model |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: dc33cc4
Looks good. The prior Important finding is fixed, and I confirmed that with the same control that exposed it rather than by reading the diff. No new blocking findings.
Prior Findings Dispositioned (1)
-
prior:62208ef important 1 — fixed —
scripts/check-docker-deploy-timeout.test.js:17— the vacuity assertion is anchored on the executed step (PREFLIGHT_STEP_PATTERN = /^\s*run:\s*\.github\/scripts\/check-pending-migrations\.sh\s*$/m), and the anchoring is itself now pinned by a negative-control test at:110. Three controls against the head tree:control prior head (62208ef) this head (dc33cc4) C — replace the run:step withecho skip, leave the margin comment atdocker.yml:409pass 14 / fail 0— guard satisfied by prosepass 13 / fail 2— fold assertion + fixture check both fireE — revert PREFLIGHT_STEP_PATTERNto the unanchored/check-pending-migrations\.sh/n/a fail 1— "removing the pre-flight step must falsify the pattern"Control E is the one that matters: the new test does not merely restate the anchor, it fails when the anchor is loosened back to the shipped-at-62208ef1 form. Suites green at this head — 15/15 timeout (was 14), 9/9 phases (19.1s), 9/9 render.
The companion suggestion was taken too and is equally live. With a decoy
${PREFLIGHT_STARTUP_TIMEOUT_SECONDS:-9999}planted in a comment ahead of the assignment, the shipped anchored regex holds the fold at 13m (pass 15); unanchoring it feeds the decoy in and the sum becomespre-flight (170m)→fail 1. My first attempt at this control used a decoy of99, which both readings survive — worth recording that the discriminating decoy has to be large enough to move the verdict.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
-
[pr-review-toolkit/tests]
scripts/check-pending-migration-preflight-phases.test.js:126— "a started-but-unfinished check is reported as migrations in trouble" does not actually exhaust the run budget it is named for. WithSTUB_JOB_RESULT: "timeout"andSTUB_READY_AFTERdefaulted to0, the stub'scondition=completebranch (:52-53) satisfieselapsed >= 0on the first iteration and exits1immediately, sokubectl waitreturns instantly rather than blocking to the 2s deadline at:50.What the test asserts — message selection on the
completed != 0path — is exercised correctly, and the script's behaviour is identical either way, so nothing is uncovered. I checked the stronger version of this and it does not hold:--timeout="${TIMEOUT_SECONDS}s"is pinned, by the render-level test atcheck-pending-migration-preflight.test.js:68. So this is test-name fidelity, not a coverage gap. Honouring the deadline forSTUB_JOB_RESULT=timeoutwould cost 2s and make the test do what it says; leaving it is defensible given the suite already runs 19.1s against a 60s cap.
Strengths
- The fix is the right shape. The finding was "this guard cannot fail", and the response was not just to anchor the regex but to add the negative control that would have caught it —
:110-133synthesises the step's absence and requires the pattern to notice. Control E confirms that control is load-bearing, which is precisely what the previous round's assertion was not. - Sharing one
PREFLIGHT_STEP_PATTERNconstant between the fold assertion and its control (:16-17, "so the two cannot drift apart") is what makes the control durable rather than a snapshot. A copy in each place would have let the guarded pattern loosen while the control kept testing the old one. - The
withoutStep.includes("check-pending-migrations.sh")precondition at:128is the detail I'd have expected to be missing: it asserts the control is still hard, i.e. that the margin comment still names the script. Without it the control would silently become trivial the day that comment is reworded. - The budget-regex anchor picked the stricter form (
/^STARTUP_TIMEOUT_SECONDS="\$\{...\}"/m) over matching the sibling render test's convention, and the comment at:25-28says why — an under-count is the unsafe direction. Choosing strictness over consistency, and writing down which way the failure leans, is the right call. - The commit message reports the controls with their numbers (
14/14 green→fail 2,13m -> 1munder the decoy) rather than asserting the fix works. That is what let me re-run them instead of re-deriving them. - The wider change still holds up:
98 >= 30 + (30+15) + 13 + 10is exactly saturated by design, no stale85mreference survives anywhere inscripts/,docs/or.github/, and the phases suite is wired intopr.yml:128rather than being an orphan file. - Excluding the
condition=failed --timeout=10swait from the fold is correct and non-obvious: it only runs on a path that exits 1, so Helm never starts and it cannot overrun the job.
Recommended Action
- No Critical or Important issues — the prior finding is closed with a verified control, and the fix is mergeable as-is.
- Consider the suggestion opportunistically; it is cosmetic and the coverage it appears to threaten is held elsewhere.
ff47c67 to
eafe8f6
Compare
|
@ally please re-review at head 1. Took the Suggestion ( 2. Rebased onto master Review focus — please check my reasoning on the step bound, since I nearly "fixed" a non-problem here. You noted the phases suite runs 19.1s against the 60s cap. I stress-tested that:
The 81.2s crosses the One thing I could not explain, recorded rather than hidden: the very first local run of the session reported For contrast, the |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: eafe8f6
Looks good. Since the last reviewed head the branch was rebased, and exactly one PR file changed — I checked that by blob SHA rather than by reading the range diff, because the rebase makes compare show four unrelated master commits. The other five files are byte-identical, so the earlier rounds' verdicts carry over; I re-ran their controls anyway to confirm the rebase did not quietly loosen them. No new blocking findings.
The one new commit takes the prior round's suggestion, and it holds up: reverting only the stub guard fails exactly one test, with the message the commit says it produces.
| control | result |
|---|---|
A — revert only the != timeout stub guard |
pass 8 / fail 1 — "the run budget must actually be spent, not short-circuited (took 0.2s)" |
C — docker.yml timeout-minutes: 98 → 85 |
❌ "job timeout (85m) must cover Helm (30m) + rollouts (45m) + pre-flight (13m) + 10m margin" |
D — unanchor PREFLIGHT_STEP_PATTERN |
❌ "removing the pre-flight step must falsify the pattern" |
E — startup budget 600 → 900, timeout untouched |
❌ same assertion, pre-flight (18m) |
C/D/E are the prior rounds' guards, still live after the rebase. Suites green at this head: 15/15 timeout, 9/9 phases (21.9s local), 9/9 render. The named test goes 0.19s → 2.21s, which is the budget actually being spent.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
-
[native-codex]
scripts/check-pending-migration-preflight-phases.test.js:53-54— the new comment claims a kubectl parity that does not hold: "failedstill resolves as soon as the container starts, which is what kubectl does." Realkubectl wait --for=condition=completedoes not short-circuit when a Job fails —Completenever goes true,Faileddoes, andwaithas no notion of giving up on a second condition, so it blocks the full--timeoutand then exits non-zero (kubernetes/kubernetes#89273). The stub exits immediately instead.Nothing is uncovered, and I confirmed that rather than assuming it. Control B — make
failedride out the--timeoutas kubectl actually would (!= timeout→= complete):✔ a failing check still reports the migration verdict (2199ms) # was 229ms ℹ pass 9 fail 0All nine still pass, so the early resolve is purely a 2s saving with no behavioural consequence — the script's verdict is identical either way, and
--timeout="${TIMEOUT_SECONDS}s"is pinned separately atcheck-pending-migration-preflight.test.js:68. Worth a word only because this commit exists to delete a stub fidelity claim that wasn't true, and it adds one in the same edit. Either drop the "which is what kubectl does" clause, or say the divergence is deliberate and costs 2s — the operational corollary being that a genuineFAILEDverdict in production takes the full 180s run budget before it prints, which the 13m fold already covers. -
[pr-review-toolkit/tests]
scripts/check-pending-migration-preflight-phases.test.js:144— the1.5floor is tied toPREFLIGHT_TIMEOUT_SECONDS: "2"at:82by a comment rather than by construction, so the two can drift apart silently. Control F — lower only the budget to1, leave the floor:ℹ pass 8 fail 1 AssertionError: the run budget must actually be spent, not short-circuited (took 1.2s)A budget reduction breaks the suite spuriously; a raise makes the assertion progressively weaker while staying green. This PR already established the fix pattern one file over —
PREFLIGHT_STEP_PATTERNis shared between the fold assertion and its control "so the two cannot drift apart" — and hoisting the run budget to a constant used by both the env and the floor (e.g.RUN_BUDGET_SECONDS = 2, floorRUN_BUDGET_SECONDS * 0.75) would apply the same treatment. Prophylactic: both numbers are correct today.
Strengths
- The control in the commit message reproduces exactly —
fail 1, the run-budget test, "took 0.2s". Reporting a negative control with its literal output is what let me re-run it in a minute instead of re-deriving it, and it is the reason this round is short. - Asserting
elapsedrather than only fixing the stub is the right half to insist on. A stub-only fix would have been silently reversible by a later edit; the assertion makes the fidelity self-enforcing, and control A shows it is the assertion, not the stub, that fails when the guard goes. - The change is correctly scoped to
STUB_JOB_RESULT=timeout.failedandcompletekeep their old path, so the other eight tests are untouched — I verified that rather than assuming it, and the twotimeoutconsumers (:132,:151) are the only ones whose timings move. - Taking a suggestion this seriously is the right instinct. The prior round explicitly said nothing was uncovered and the coverage was held elsewhere; fixing it anyway, on the grounds that a test which does not do what its name says is the same rot the PR exists to remove, is consistent with the standard the rest of the PR sets.
- The cost is stated honestly and lands where claimed: 23s on the real runner against the step's 60s cap (
pr.yml:129), a 2.6x margin, matching 21.9s locally. The suite is wall-clock-dominated by fixed budgets rather than by runner speed, so that margin is stable rather than lucky. - The rebase is clean — five of six files byte-identical to the last reviewed head, and controls C/D/E confirm the budget fold and the anchor survived it intact.
Recommended Action
- No Critical or Important issues — mergeable as-is.
- Consider the two suggestions opportunistically; both are cosmetic, and the first is a one-clause comment edit.
- CI is red, but not from this diff — see the note below.
CI note (not a finding against this PR). verify is failing, and none of it is attributable to these six files.
- The blocking failure is
policy→ "Test bounded PR-check polling skills" →prcheckloop: a sub-minute interval is clamped to 60s/AssertionError: expected at least one sleep. That test is not in this diff. verifythen fails only downstream: "Upstream lane(s) did not run … GitHub Actions skips a lane when a job it needs (typically 'policy') did not succeed."- The shard-manifest drift in the same log is a red herring — its step is
continue-on-errorand reportssuccessat job level. The manifest is byte-identical on master (443/443, last topped up 2026-08-24) and this PR touches neither it norserver/. - The previous head of this same branch failed
policyat a different unrelated step (Test Helm chart render suite), which passed as its own job this run. Two different out-of-diff failures across two runs of one branch, while sibling PRs pass both steps, reads as flake rather than breakage here.
This PR's own three steps all pass: Validate Docker deploy timeout margin, Validate pending-migration deploy pre-flight, and Test pending-migration pre-flight phase budgets (23s).
|
@ally please re-review at head b8dfa19 — takes both suggestions from your review at
Your control F is now green ( |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: b8dfa19
One new commit since the last reviewed head, a clean fast-forward (ahead_by: 1, behind_by: 0 — no rebase this time), touching one file. The other five are byte-identical by blob SHA, so the earlier rounds' verdicts carry over; I re-ran their controls anyway.
Both of the prior round's suggestions were taken, and both of the commit message's controls reproduce exactly. The run-budget derivation is a genuine strengthening rather than a tidy-up — I checked the half a green suite cannot show. One Important finding, and it is mine: the commit faithfully copied a citation I gave it, and the citation is wrong.
| control | result |
|---|---|
F — RUN_BUDGET_SECONDS 2 → 1 |
✅ pass 9 / fail 0, run-budget test 1201ms (was a spurious fail 1) |
A — revert the != timeout stub guard |
❌ pass 8 / fail 1 — "the run budget must actually be spent, not short-circuited (took 0.2s)" |
| G (new) — budget 8s, stub short-circuits at ~2s | ❌ fail 1 "(took 2.3s)" against the derived 6.0 floor — the old hardcoded 1.5 passes the identical tree |
C — timeout-minutes 98 → 85 |
❌ "job timeout (85m) must cover … pre-flight (13m) + 10m margin" |
D — unanchor PREFLIGHT_STEP_PATTERN |
❌ "removing the pre-flight step must falsify the pattern" |
| E — startup budget 600 → 900 | ❌ same assertion, pre-flight (18m) |
Control G is the one worth the time. F and A show the change is inert-free and self-enforcing, but neither discriminates the derivation from the old constant — at a budget of 2 the derived floor evaluates to exactly the 1.5 it replaced. G separates them: same tree, same injected partial short-circuit, opposite verdicts. So this closed a real hole rather than restating a number. Suites green at this head: 15/15 timeout, 9/9 phases (21.6s), 9/9 render.
Critical Issues (0)
Important Issues (1)
-
[native-codex]
scripts/check-pending-migration-preflight-phases.test.js:63— the upstream citation is wrong, and I am the source of it — my review ateafe8f6b2citedkubernetes/kubernetes#89273and this commit copied it in good faith. That issue is "serverside apply: delete dropped fields" (lavalamp, 2020-03-19, sig/api-machinery). It has nothing to do withkubectl wait, Jobs, or conditions.The claim the comment makes is correct — I re-verified it rather than just retracting the number.
kubectl wait --for=condition=completewatches one condition; on a failed JobCompletenever goes true, so it blocks the full--timeoutand then exits non-zero. The accurate references are kubernetes/kubernetes#100248 ("Support waiting on multiple conditions in kubectl wait" — the canonical request, and the closest match to the comment's "has no second condition to give up on") and kubernetes/kubectl#1629 ("how to kubectl wait for job completion regardless of success or failure" — the same problem hit in CI, still open).This is severity-by-context, not by risk: zero runtime effect, no test or behaviour depends on it, and it is a one-token fix. I am raising it as Important rather than filing it with the cosmetics for one reason — this commit exists specifically to delete a false claim from this comment, and says so: "This PR exists to delete a false claim from a comment; it should not add one." Merging it with a fresh false claim in the same comment inverts its own stated standard. The comment is also load-bearing: it is the written justification for a deliberate stub-vs-kubectl divergence, so the next reader who follows the reference to audit that reasoning lands on an unrelated SSA issue and has to redo the work.
Recommendation — swap
#89273for#100248(optionally both). Triage it as documentation, not risk; it should not hold the PR up beyond the edit itself.
Suggestions (2)
-
[gstack/review + pr-review-toolkit/tests]
scripts/check-pending-migration-preflight-phases.test.js:219,235— the same drift the commit just fixed is still present three lines over, on the startup budget.assert.ok(elapsed < 10, …)is tied toPREFLIGHT_STARTUP_TIMEOUT_SECONDS: "60"(:200,:229) by the assertion message — which hardcodes the string"the whole 60s startup budget"— rather than by construction.The failure direction is the weakening one, and it is silent. Control H, against the head tree: in the config-error test lower the startup budget to
8and swap the reason to a recoverable one so the script rides out the entire budget instead of bailing:ceiling-fired-count: 0 # `elapsed < 10` did not fireThe one failure was a different assertion (the
CreateContainerConfigErrormatch); the timing ceiling passed a full ride-out. At any startup budget under 10s these two tests can no longer distinguish a fast bail from riding out the budget — which is precisely what their names claim to prove — while staying green and keeping a message that now says something false.Worth more than idle prophylaxis because the pressure is demonstrated, not hypothetical: this file's own comment at
:96-99records that its budgets were already cut once to fit the policy job's 60s step bound. "Someone lowers a timing constant here to save wall-clock" is the thing that has already happened. The fix is the treatment this commit just applied one screen up — aSTARTUP_BUDGET_SECONDSconstant feeding the env, the ceiling (e.g.STARTUP_BUDGET_SECONDS / 6) and the message. Both numbers are correct today. -
[pr-review-toolkit/comments]
scripts/check-pending-migration-preflight-phases.test.js:93— leftover duplicate. "Deliberately tiny so a budget being charged the wrong phase is loud." now appears at both:18(on the new constant, with the fuller sharing rationale) and:93(on the env entry that just becameString(RUN_BUDGET_SECONDS)). The:93copy is the stale half — the value is no longer written there. Deleting it points the reader at the constant, which is where the reasoning now lives.
Strengths
- Control G is the check I most expected to fail, and it held. It would have been easy to ship a derivation that merely reproduced
1.5and called it hygiene; the derived floor actually catches a partial short-circuit the constant waved through. That is the difference between refactoring a number and fixing a guard. - Both controls in the commit message reproduce to the literal string, including
"took 0.2s". That is now three rounds running, and it is the reason these reviews take minutes instead of hours — I re-run the author's controls rather than re-deriving them. - The stub's timeout loop is robust in a way that is easy to get wrong: because
sleep 1preserves the sub-second offset, elapsed converges on the budget regardless of where the second boundary falls, so control F's 1s budget yields ~1.2s rather than a race. Adate-arithmetic-only version would have been flaky at small budgets. - Taking a suggestion the prior round explicitly labelled cosmetic, and taking it properly — the comment rewrite states the divergence, its cost (~2s), and its operational corollary (a genuine
FAILEDin production spends the whole run budget before printing, which the 13m fold covers). Naming the production consequence of a test-stub shortcut is the part almost nobody writes down. - Scope discipline: one file, +19/−5, no drive-by edits, and the five untouched files verified byte-identical rather than merely "not in the diff".
- The prior rounds' guards all survive the change — the budget fold, the step anchor, and the startup-budget arithmetic each still fail when reverted. The PR's central claim (a budget raise in a shell script must be paid for in
timeout-minutes) is intact at this head. RUN_BUDGET_SECONDS's comment explains the coupling rather than the value — why the two uses must not drift, and which direction each failure leans. That is the same reasoning-in-the-comment standard the rest of the PR sets.
Recommended Action
- No Critical issues. The fix is correct, the guards are live, and the deploy-budget arithmetic is unchanged and still enforced.
- Correct the
#89273citation before merge — a one-token edit, and the commit's own standard asks for it. - Consider the startup-budget derivation opportunistically; it completes the pattern this commit established, and control H shows what it protects.
CI note (not a finding against this PR). The policy failure I flagged at eafe8f6b2 has cleared — policy, review, Helm chart, Vendored claude_k8s adapter and review/ally-comment all report SUCCESS at this head, confirming that was flake rather than breakage. No check is in a failure state; 13 remain queued or running. security-review is NEUTRAL and Storybook visual regression SKIPPED, both routine here.
mergeStateStatus is BEHIND (master has moved), so this will want an update before merge — mechanical, and unrelated to the diff. reviewDecision is empty: no required-review protection applies to this branch, so nothing about approval identity is blocking.
b8dfa19 to
95db583
Compare
|
@ally please re-review at head Your Important finding is fixed, and I verified all three references directly rather than swapping one unchecked number for another. One correction to your replacement: Also took both Suggestions — the Your control H, isolated. Worth your attention because my first construction of it was confounded and I have recorded that in the commit message: making
Suites: phases 9/9 (23.4s / 60s bound), preflight 9/9, docker-deploy-timeout 15/15. Rebased onto Focus: (1) is the |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 95db583
Clean. The Important finding is fixed, and I verified the replacement citations against the live GitHub API rather than accepting the swap — both check out, and your correction to me was right.
Prior Findings Dispositioned (1)
- prior:b8dfa19 important 1 — fixed —
scripts/check-pending-migration-preflight-phases.test.js:75—#89273is gone. The line now readskubernetes/kubectl#1629; the underlying gap is kubernetes/kubernetes#100248, "wait on multiple conditions", closed rotten rather than fixed. Verified both against the API at this head, not just that the token changed:kubectl#1629is open,triage/accepted, titled "how to kubectl wait for job completion regardless of success or failure" — the exact scenario.k/k#100248is closed 2021-09-03 withlifecycle/rottenon it, titled "Support waiting on multiple conditions in kubectl wait". Leading with the open exact-match issue and demoting mine to "the underlying gap" is the better ordering, and your unprompted correction of my "canonical request" framing is the fix I should have made myself.
Critical Issues (0)
Important Issues (0)
Suggestions (3)
-
[pr-review-toolkit/tests]
scripts/check-pending-migration-preflight-phases.test.js:282— the3.5floor is hardcoded against an inlinePREFLIGHT_STARTUP_TIMEOUT_SECONDS: "5"at:273. This is the same drift class you just fixed one instance of at:169(RUN_BUDGET_SECONDS * 0.75), and it fails in both directions: drop that5to3and the floor sits above the budget, so the test fails on a correct script; raise it and the assertion silently weakens. It is also the third budget in this file, and the only one not declared at the top — which is exactly the condition your own:24-32note warns about. Suggest a named constant plusPULL_ERROR_BUDGET_SECONDS * 0.7.- Worth stating precisely why this is a Suggestion and not a repeat Important: the assertion is currently correct and the failure mode is a spurious red on a future edit, not a false green. Your
:169fix already removed the one instance that could have gone vacuous.
- Worth stating precisely why this is a Suggestion and not a repeat Important: the assertion is currently correct and the failure mode is a spurious red on a future edit, not a false green. Your
-
[pr-review-toolkit/tests]
scripts/check-pending-migration-preflight-phases.test.js:176—"the two INCONCLUSIVE causes are distinguishable from the log alone"re-runs both slow scenarios end to end, which costs roughly 7s of the file's measured 23.4s — the single most expensive test here, in a file you are deliberately holding under a 60s step bound. The property is already established transitively::147assertsmatch(/never started/)on the startup path and:164assertsdoesNotMatch(/never started/)on the run path, and that phrase occurs only in the INCONCLUSIVE line. If you want the stronger line-level claim kept explicitly, it is cheap to get it from outputs the other tests already produce rather than from two fresh runs. -
[gstack/review]
scripts/check-pending-migration-preflight.test.js:77,79— these two budget regexes are unanchored, whilecheck-docker-deploy-timeout.test.js:25-31deliberately anchors the same extraction with^...+/mand documents why. No false match exists today — I checked the new comment block at.github/scripts/check-pending-migrations.sh:21-40, and it namesTIMEOUT_SECONDSin prose but never in="${PREFLIGHT_...:-N}"form, so nothing matches ahead of:33/:37. Aligning them costs two characters and removes the asymmetry where one file's justification does not apply to its neighbour.
On your two focus questions
(1) The #100248 "closed rotten" framing — keep it, it is accurate and load-bearing. Verified above. It earns its words precisely because the issue is closed: a reader who follows a bare closed reference reasonably infers the gap was fixed, concludes the divergence comment is stale, and "helpfully" deletes the stub's deliberate blocking behaviour. Naming it closed rotten rather than fixed forecloses that. One precision note for anyone re-verifying: the API reports state_reason: completed on #100248, because the k8s lifecycle bot closes through the ordinary path — the lifecycle/rotten label is the actual evidence, not the state reason. Your prose is right; the API field is the misleading one.
(2) The / 6 derivation — principled, and the control proves it. The property that matters is that the ceiling can never exceed the budget it is asserting against, because a ceiling above its own budget is unfalsifiable and stays green forever. A fraction guarantees that structurally; the hardcoded 10 did not. That it reproduces 10 at today's budget is the right way to pick the divisor — it makes the change a pure refactor at the shipped value, with the tracking behaviour as the only delta.
Your control isolates exactly that delta, and the confounding you recorded is the reason it is worth trusting: flipping CreateContainerConfigError to non-terminal changes classification, which trips the doesNotMatch(/within its \d+s startup budget/) assertion at :261 as well, so both arms fail 1 and the ceiling is never the variable. Injecting latency at constant classification leaves the ceiling as the only thing that can differ, and it does — fail 1 derived vs pass 9 hardcoded. Stating in the commit message that your first construction was confounded is the part I would keep.
The one soft edge, and it does not change the recommendation: the divisor ties the ceiling to the budget, but the quantity actually being bounded is how fast an early bail returns — three kubectl round-trips with no sleep, since the loop breaks before it, so ~0.2s independent of the budget. / 6 clears that comfortably anywhere from the shipped 60s down to ~6s, and only degrades if the budget were ever driven to ~2s (ceiling 0.33s against a 0.2s bail). That is well outside the range this file plausibly moves in, and the alternative — deriving from POLL_SECONDS — would not track the budget, which is the failure mode you were actually fixing. Leave it.
Strengths
- The budget fold is arithmetically exact and I re-derived it independently:
ceil((600 + 180) / 60) = 13, and30 (Helm) + 30 + 15 (rollouts) + 13 + 10 = 98, matchingtimeout-minutes: 98. Reading both defaults out of the shell script rather than restating them means the next budget raise is forced to be paid for here. docker.yml:405-413explains why overrunning is not a clean failure — GitHub cancels the job mid-helm upgrade --atomic, killing the process whose job is to roll back. That converts an arbitrary-looking number into a stated hazard.- The
running.startedAt/terminated.startedAtconcatenation at.github/scripts/check-pending-migrations.sh:106-111is checked before the terminal-phase branch, so a job that completed between polls is read as started rather than discarded.:192covers it. Getting that ordering wrong is how the "never started" path would eat real answers. - Excluding
ErrImagePull/ImagePullBackOfffrom the terminal set at:117-120, with:282asserting the wait continues, is the discipline that keeps the fix from re-creating the defect it exists to remove. - Both INCONCLUSIVE messages now say which phase spent the budget and explicitly disclaim a migration verdict. The distinction between "bailed on a decided error" and "exhausted the budget" is carried in the message rather than left for the operator to infer.
Recommended Action
- No Critical issues.
- No Important issues.
- Suggestions are all test-maintainability and none should hold the PR.
:282is the one I would take before merge, since it is the same class you just fixed one line-group away and costs a constant.
…O-31254)
The pending-migration pre-flight bounded `pull + run` with a budget sized as
if it only bounded `run`. A cold pull of the ~1.7 GB candidate image measured
3m3s against the single 180s budget, so on run 33601878894 the container
started ~3s past the deadline and was killed before emitting anything. The
gate read INCONCLUSIVE and skipped `helm upgrade` — the correct refusal, on a
perfectly good build. It only passed on retry because the first attempt had
warmed the node cache, which makes the first deploy of any freshly built
image — the case that always pulls cold — a coin flip.
Split the wait into two phases. Phase 1 polls until the pod leaves Pending,
under its own `PREFLIGHT_STARTUP_TIMEOUT_SECONDS` (600s, ~3x the measured
cold pull). Only then is `PREFLIGHT_TIMEOUT_SECONDS` (unchanged at 180s)
armed, so it now measures what it was always sized for. `Succeeded`/`Failed`
count as started: with backoffLimit 0 a fast check can finish between two
polls, and reading that as "never started" would discard a real verdict.
The INCONCLUSIVE causes now read differently, so the log alone separates slow
infrastructure from migrations in trouble. All three name the phase they
belong to, disclaim any migration verdict where none was reached, and the
startup branch dumps the pod events carrying the pull duration and image size
that previously forced an operator to go describe the pod by hand.
`InvalidImageName`/`CreateContainerConfigError` break out early rather than
riding out the startup budget, since neither self-heals, and are reported as
a terminal container error rather than as budget exhaustion — saying "never
started within its 600s budget" after 0s would point the operator at a budget
that was never the constraint. Pull errors are deliberately excluded from
that list: ErrImagePull/ImagePullBackOff routinely recover, and failing fast
on them would recreate this very defect.
Tests: check-pending-migration-preflight-phases.test.js drives the script
against a stub kubectl that models `kubectl wait` honestly, so the phase
ordering and budget accounting are exercised rather than grepped. Confirmed
as a real regression test — the pre-fix script fails its slow-pull case with
the production symptom verbatim ("INCONCLUSIVE — job did not finish within
Ns"). Wired into pr.yml; the render-level suite keeps the shipped defaults
honest, which the behavioural suite cannot since it overrides both budgets.
…bound The policy job requires every `node --test` step to carry `timeout-minutes: 1`; the new pre-flight step asked for 3 and failed `policy`, which skipped every downstream lane and took `verify` red with it. Raising the bound was the wrong direction — the step needed 3 minutes only because the tests wait on the script's hardcoded 5s poll, costing ~32s of real time. So make the poll injectable (`PREFLIGHT_POLL_SECONDS`, default 5, set by nothing but the tests) and drive the tests at 1s. The waits stay real, only their granularity shrinks: 32s -> 19s, a 3.1x margin under the bound instead of 1.9x. Also loosens the transient-pull assertion from `>= 8` to `>= 3.5`. The script compares its deadline in whole seconds, so it can return ~1s early against a wall clock — the exact bound was measuring second-granularity slop, not behaviour. The claim under test is "kept polling, did not bail on sight", and the terminal-error path returns in ~0.2s, so seconds settle it. Verified the file is still a real regression test after the retune: run against the pre-fix script, 6/8 fail and the slow-pull case reproduces the reported symptom verbatim — `INCONCLUSIVE — job did not finish within 2s`. 35/35 deploy-gate tests and the policy timeout gate pass on the fix. BLO-31254
…d container start from the kubelet (BLO-31254) Addresses Ally's review of 6c0eafe. Important — the 600s startup budget added in 6c0eafe pushed the deploy job's worst case past its `timeout-minutes: 85`, and the test that exists to catch that drift could not see it. The job budget was exactly saturated: helm 30m + rollouts 30m + 15m + the asserted 10m margin = 85m, zero slack. That 10m had to cover the twelve steps ahead of helm, and the pre-flight's worst case that still proceeds to helm (a slow-but-successful pull: 600s startup + 180s run) is 13m on its own. `check-docker-deploy-timeout.test.js` only regexed the helm and rollout waits, so a budget living in a shell script was invisible to it. Took Ally's option 1, since it re-arms the guard rather than just paying the bill once: the test now reads both `PREFLIGHT_*_SECONDS` defaults out of check-pending-migrations.sh and folds them into the sum, and asserts the deploy job actually runs that script so the fold cannot go vacuous. Confirmed it is a real guard — against the unchanged workflow it fails with `job timeout (85m) must cover Helm (30m) + rollouts (30m + 15m = 45m) + pre-flight (13m) + 10m margin`. Raised timeout-minutes to 98 to satisfy it. Overrunning was never a clean failure: GitHub cancels the job mid-`helm upgrade --atomic`, killing the process whose job is to roll back, which is the half-applied release BLO-21492 added --atomic to prevent. Suggestion 1 (a real defect, not cosmetic) — phase alone was used to conclude the container ran. A pod can reach Failed straight from Pending via eviction or preemption without ever starting a container; that set container_started=1, phase 2's `condition=failed` fired, and the operator was told "FAILED — a pending migration needs its index precreated": a migration verdict from a check that never inspected a migration, which is the most misleading output available here. Now reads `.status.containerStatuses[0].state.{running,terminated} .startedAt`, which is the fact the comment already claimed. terminated.startedAt still covers the fast-check-between-polls race, so no conclusive answer is discarded. A terminal phase with no start stamp now breaks early instead of riding out the budget — backoffLimit is 0, so nothing will replace the pod. Suggestion 2 — startup_seconds is quantized to the poll interval plus kubectl round-trips, so it over-reports. Reworded "started in Ns" to "started within Ns" at both report sites and said so at the assignment, since this PR's own budget argument rests on a measured pull time and the number invites reuse. Verified: new stub case models the start stamp honestly, and the eviction test is a genuine regression test — run against the pre-fix script it produces the "needs its index precreated" verdict instead of INCONCLUSIVE, exactly the symptom described. 36/36 deploy-gate tests pass; the phases file runs 18.6s, 3.2x under the policy job's one-minute step bound. BLO-31254
…O-31254) Ally's Important finding at 62208ef was correct, and I verified it with the control rather than reading it: the assertion added in 62208ef to keep the budget fold honest was self-satisfied by a comment that same commit added. `getDeployJobBlock()` returns the whole `deploy:` block, and after 62208ef `check-pending-migrations.sh` occurs twice inside it — the margin prose at docker.yml:409 and the real step at :814. The unanchored regex matched the prose first, so the guard was satisfied by its own documentation. Control on the head tree: replacing the `run:` step with `echo skip` left the suite 14/14 green. Anchoring on the step makes that control fail 2. This is the defect class the PR set out to close, reproduced one level up: check-docker-deploy-timeout was blind to a budget because it only regexed the places it knew about; the new assertion was blind to the step's absence because it regexed a string that also appears in prose. So the fix is not just the anchor — the negative control is now a test, which is what would have caught this in the first place. Also took the matching suggestion, which is the same class one line away and guards the unsafe direction. `getPreflightBudgetMinutes()` matched the `${...}` expansion anywhere in the file, so a comment mentioning a budget ahead of the assignment fed the wrong number into the job-timeout sum. Measured with an under-counting decoy: unanchored folds 13m -> 1m, anchored holds at 13m. Now anchored on the start-of-line assignment, which is stricter than the sibling render-level test at check-pending-migration-preflight.js:77. Verification: 15/15 timeout (was 14), 9/9 preflight, 9/9 phases (17.6s, inside the 60s policy step bound). Controls B and C above both behave correctly. Not verified locally: no YAML parser here, so docker.yml stays eyeballed plus CI.
…(BLO-31254)
Ally's review (Suggestion 1) found that "a started-but-unfinished check is
reported as migrations in trouble" did not exhaust the budget it is named for.
With STUB_JOB_RESULT=timeout and STUB_READY_AFTER defaulted to 0, the stub's
condition=complete branch satisfied `elapsed >= 0` on its first iteration and
exited 1 immediately, so `kubectl wait` returned in ~0.2s instead of blocking
to the 2s deadline.
Nothing was uncovered -- the message-selection path under test is identical
either way, and `--timeout="${TIMEOUT_SECONDS}s"` is pinned separately by
check-pending-migration-preflight.test.js:68. But a test that does not do what
its name says is the same rot this PR exists to remove, so make the stub
honest: STUB_JOB_RESULT=timeout now models a job that starts and never settles,
riding out the whole --timeout. "failed" still resolves as soon as the
container starts, which is what kubectl actually does.
Also assert the elapsed time, so the fidelity is self-enforcing rather than a
property of the stub that a later edit could silently drop. Negative control:
revert only the stub guard and exactly one test fails -- the run-budget one --
with "the run budget must actually be spent, not short-circuited (took 0.2s)".
Suite cost 19.1s -> 24.3s against the policy step's 60s bound (2.5x margin);
the named test goes 0.19s -> 2.25s. 33/33 deploy-gate tests and 3/3 policy
timeout-gate tests green.
BLO-31254
…BLO-31254) Takes both suggestions from Ally's review at eafe8f6. The stub's comment said `failed` resolves as soon as the container starts "which is what kubectl does". It does not: `wait --for=condition=complete` never sees Complete go true on a failed Job and has no second condition to give up on, so it blocks the full --timeout (kubernetes/kubernetes#89273). The early resolve is a deliberate ~2s saving with no effect on the verdict -- now said that way, along with its real operational corollary, that a genuine FAILED in production spends the whole run budget before printing. This PR exists to delete a false claim from a comment; it should not add one. The 1.5s run-budget floor was tied to PREFLIGHT_TIMEOUT_SECONDS: "2" by a comment rather than by construction, so lowering the budget alone failed the suite spuriously and raising it alone weakened the assertion while staying green. Both now derive from RUN_BUDGET_SECONDS, the same treatment PREFLIGHT_STEP_PATTERN already gets one file over. Controls: - F, RUN_BUDGET_SECONDS 2 -> 1: pass 9 fail 0 (was fail 1, spurious) - A, revert the `!= timeout` stub guard: pass 8 fail 1, "the run budget must actually be spent, not short-circuited (took 0.2s)" - suite at head: 9/9, 22.5s (unchanged; step bound is 60s)
Ally's review at b8dfa19 raised one Important finding against the comment the previous commit rewrote, and it was Ally's own citation that was wrong: kubernetes/kubernetes#89273 is "serverside apply: delete dropped fields" (2020, sig/api-machinery) and has nothing to do with `kubectl wait`. Verified all three references directly rather than swapping one unchecked number for another: k/k#89273 "serverside apply: delete dropped fields" closed 2020 k/k#100248 "Support waiting on multiple conditions in ..." closed, rotten kubectl#1629 "how to kubectl wait for job completion open regardless of success or failure" The claim itself holds, so this is a citation fix and not a retraction. kubectl#1629 leads because it is the exact scenario and still open; #100248 is named as the underlying gap, with "closed rotten rather than fixed" stated so nobody reads a closed issue as a resolved one. That mattered here: Ally called #100248 "canonical" without noting it was closed. Also takes both Suggestions, since the push costs a review cycle either way: - Derives the early-bail ceiling from the startup budget it is a fraction of (`STARTUP_BUDGET_SECONDS / 6`), replacing two hardcoded `10`s, three literal `"60"`s and a "60s" message string. Same drift the previous commit fixed for the run budget, in the direction that fails green: a ceiling above its own budget cannot be exceeded, so the fail-fast tests would keep passing while proving nothing. - Deletes the stale duplicate comment on the env entry, whose value now comes from the constant that carries the fuller rationale. Control H (isolated) -- budget 60 -> 6, and the terminal-error bail regressed to ~3s so it is still classified correctly and only its latency changes: derived ceiling (6/6 = 1s) fail 1 "must not burn the whole 6s startup budget on a terminal error (took 3.141s)" old hardcoded 10s ceiling pass 9 <- the hole My first attempt at this control was confounded and I am recording that: it made CreateContainerConfigError non-terminal, which also flipped the message to budget exhaustion and tripped a different assertion, so both variants failed and the ceiling was never isolated. Injecting latency without changing classification is what separates them. Suites at this head: phases 9/9 (23.4s against the 60s step bound), preflight 9/9, docker-deploy-timeout 15/15. Rebased onto f0d69fe (was BEHIND by 2); clean, no conflicts. `check:test-undefined-symbols` fails locally on 20 pre-existing `setImmediate` /`BufferEncoding` references in server/src -- this worktree has no node_modules, so Node globals do not resolve. Zero overlap with this diff and `policy` is green on CI where deps are installed. Co-Authored-By: Claude <noreply@anthropic.com>
…on (BLO-31254) Both of Ally's remaining test-maintainability suggestions. The recoverable-pull-error test hardcoded a 3.5s floor against an inline 5s budget -- the third budget in the file and the only one not declared with the others, i.e. the same drift class already fixed at the run-budget floor. Declare PULL_ERROR_BUDGET_SECONDS and derive the floor as PULL_ERROR_BUDGET_SECONDS * 0.7, which reproduces 3.5 exactly, so this is a pure refactor at the shipped value with tracking as the only delta. Control isolating that delta: with the budget lowered 5 -> 3, the derived floor passes (1/1) and a floor hardcoded back to 3.5 fails (0/1) on the same correct script. That is the spurious-red direction Ally named, demonstrated rather than asserted. Also anchor the two budget regexes in the render-level suite with ^...$/m, matching check-docker-deploy-timeout.test.js, which already anchors the same two extractions and documents why. No false match exists today, so this makes correctness a property of the regex rather than of the script's current prose. The Number.isFinite guards confirm both still extract. Not taken: rebuilding the "distinguishable from the log alone" test out of other tests' outputs to save ~7s. The property is established transitively, but sourcing assertions from sibling tests couples them, and the file runs 21.5s against a 60s bound -- 38s of headroom does not buy that trade. node --test on both suites: 24 pass render-level, 9 pass behavioural.
…31254) Found by stress-running the suite rather than by CI, which passed all 21 checks at 95db583 and had passed the earlier heads too. Both failures are mine, from commits on this branch, and both are the same defect this PR exists to fix -- a timing budget sized as if a clock were continuous. The stub and the script both compare a whole-second `date +%s` against a deadline built from a whole-second start stamp, so a budget of N is satisfiable after as little as N-1 seconds of wall clock: the truncated origin is up to 1s in the past. 1. The run-budget floor (0a6c43c) was RUN_BUDGET_SECONDS * 0.75 = 1.5s against a 2s budget whose worst case is ~1.0s. Reproduced at 1.2s, ~1 run in 6 under load. A fraction of the budget cannot be right here -- subtract the granularity first, then take the margin. Now spentFloor(N) = (N - 1) * 0.75, shared with the pull-error floor so a third instance cannot appear. 2. `a pull slower than the run budget still passes` asserted started > 2 with STUB_READY_AFTER=3. The stub's clock origin is its first call -- the `apply` the script makes BEFORE stamping startup_began -- so when those two truncate into different seconds the reported figure reads 1s short, i.e. exactly 2, and `2 > 2` fails. Reproduced twice, ~1 run in 20. STUB_READY_AFTER is now derived as RUN_BUDGET_SECONDS + granularity + 1, so the under-report still clears the budget. Every bound in the file is now derived and has structural margin, not empirical luck -- the worst case is a guaranteed lower bound on elapsed and each floor sits strictly below it: run-budget floor 0.75s vs worst case 1s margin 0.25s pull-error floor 3.0s vs worst case 4s margin 1.00s slow-pull started in {3,4} vs > 2 margin 1 whole second early-bail ceil 10s upper bound on a ~0.2s bail Costs ~1s (SLOW_PULL_SECONDS 3 -> 4); the file is ~22s against its 60s bound. No production script change -- test-only.
95db583 to
5d50a41
Compare
…d at (BLO-31254) Comment only; the bound stays at 1 minute and the policy-node-test-timeouts gate still passes 3/3. This step waits on real budgets, so it is the one in the policy job most likely to be re-tuned by someone counting sleep seconds. Leaving the bound undocumented is how BLO-31405 happened one step below -- that suite sits at 59s against the same 60s bound, which I re-measured independently in the same job while checking this one (run 33714085042: this step 04:14:38 -> 04:15:01 = 23s, Helm render 04:17:02 -> 04:18:01 = 59s). Records the measurement and, more importantly, why this step does NOT need the 3x margin BLO-31405 asks of its neighbour: the two have different variance profiles. This file's cost is almost entirely fixed `sleep`, which does not dilate under runner contention -- 21.5s local vs 23s on a runner, a 7% spread, so ~2.5x margin. The Helm suite forks `helm template` 18 times and ranges 21.8s local to 32-73s on CI, where 3x is the right ask. Applying one multiple to both would either strand this bound or under-size that one.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 414bf88
Looks good. All three findings from my earlier reviews are retired, and this head is clean on every lens.
The branch advanced from 5d50a410 to 414bf880 while I was reviewing. I re-resolved rather than attesting the tree I started on: the delta is one commit touching pr.yml only, +10/-0, comment-only, and I read it in full. All five code files under review are byte-identical across the two heads (same blob SHAs for check-pending-migrations.sh, both pre-flight test files, check-docker-deploy-timeout.test.js, and docker.yml), so the evidence below is evidence about this head.
I verified the change rather than reading it. Both suites are green at this head — 9/9 behavioural in 23.9s, 24/24 render-level — and, more importantly, they fail where they should:
| control | result |
|---|---|
behavioural suite vs. the pre-fix script (master) |
7/9 fail |
| the 2 that pass on base | a failing check still reports the migration verdict, a check that completes between polls… — exactly the invariants the fix must not break |
| new render-level budget test vs. base | fails (base declares no STARTUP_TIMEOUT_SECONDS) |
So the suite is a genuine regression test and pins the untouched behaviour, which is the pair that matters for a control-flow-only defect.
I also mutation-tested the budget fold, since a fold that cannot fail is the failure mode two of my prior findings were about:
| mutation | result |
|---|---|
timeout-minutes: 98 → 97 |
1 fail — 98 is exactly the asserted minimum |
startup budget 600 → 700, job timeout untouched |
1 fail — the drift class prior:6c0eafe asked for, now closed |
remove the pre-flight run: step |
2 fail — the fold and the anchoring guard |
That third one is the one I care about: it confirms PREFLIGHT_STEP_PATTERN's ^\s*run:…$ anchoring does what be1e81ba claimed, and that the load-bearing test is not itself satisfied by the margin comment at docker.yml:409.
The arithmetic checks out end to end. ceil((600+180)/60) = 13, and 98 ≥ 30 + 45 + 13 + 10. The subtle part is right and correctly justified in the comment at check-docker-deploy-timeout.test.js:81-87: the worst case that reaches Helm is startup + run, because the extra --for=condition=failed --timeout=10s only executes on the abort path where the script exits non-zero and Helm never runs. The 10m margin covering the other pre-helm steps is also strictly better than before — pre-PR the pre-flight's 180s ate into that same 10m; it now has its own tier.
Two things I checked specifically for gaps and found none: check-pending-migrations.sh has exactly one invocation site (docker.yml:814), inside the deploy job whose timeout was raised, so no second job inherits a budget nobody folded; and under set -euo pipefail every phase-1 kubectl is || true-guarded inside an assignment or condition, so a transient API error cannot kill the loop early and silently skip to phase 2.
The startedAt-not-pod-phase decision (108db00a) is the right call and the reasoning in the comment is exact — a pod evicted straight out of Pending would otherwise hand phase 2 a job whose condition=failed is already true, printing "a pending migration needs its index precreated" from a check that never opened the database. That is the worst available output here, and it now has a dedicated test that fails on base.
Excluding ErrImagePull/ImagePullBackOff from the fast-fail list is also correct, and it is the non-obvious half of the fix: fast-failing those would reconstruct BLO-31254 through a different door. The remaining unlisted-but-terminal reasons (CreateContainerError, ErrImageNeverPull) ride out the budget and fail closed — the safe direction, and ErrImageNeverPull cannot arise here anyway since the job pins a digest under the default pull policy.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
-
[pr-review-toolkit: tests]
scripts/check-pending-migration-preflight.test.js:93—366is now the only un-derived budget literal left in this branch, and the message beside it already names the number it is twice:startup budget must clear the measured 183s cold pull with margin. Every other bound in the behavioural file was converted to a derived constant over the last three commits (SLOW_PULL_SECONDS,EARLY_BAIL_CEILING_SECONDS,spentFloor,PULL_ERROR_BUDGET_SECONDS), so this is the last instance of the pattern those commits were closing.const MEASURED_COLD_PULL_SECONDS = 183;thenstartupBudget >= 2 * MEASURED_COLD_PULL_SECONDS. Same value, but the relationship between the floor and the measurement stops living in prose — and if the pull is ever re-measured, one edit moves both the floor and the message.- Genuinely optional: unlike the drift cases, nothing here can go vacuous, because the floor is compared against a value read out of the script.
-
[native-codex] PR description — stale in three places relative to this head, all from later commits on the branch. The body is the durable record for BLO-31254 and outlives the diff, so it is worth a pass before merge:
- "
timeout-minutes: 3" for the newpr.ymlstep — it is 1 (pr.yml:139), lowered byd716752ato match its neighbours. This is the one most likely to mislead, and414bf880sharpens it: that step now carries a ten-line comment justifying the 1-minute bound by measurement, so the body and the file it describes disagree in a place someone re-tuning the bound will land on first. - "(new, 8 tests)" — it is 9.
- "
17/17 pass" — that command now yields 18 (9 behavioural + 9 render-level incheck-pending-migration-preflight.test.js).
- "
Strengths
- The fix targets the right thing. Splitting the budget is a smaller and more honest change than raising 180s to 600s would have been: raising one number would have made the symptom rarer while leaving image transfer chargeable to the migration allowance, so the next registry slowdown reproduces it. Arming the run clock only after the kubelet stamps a start time makes the budget measure what its name says.
- The two INCONCLUSIVE messages now carry their own attribution, including the pod events inline. Reading the original failure required knowing to go
describethe pod;Successfully pulled image … in 3m3.14s. Image size: 1695082158 bytesin the job log is the difference between a five-minute diagnosis and the one that took a ticket. 5d50a410is the commit I would most want to see here. Both flakes were found by stress-running rather than by CI — which passed all 21 checks at95db5837and at every earlier head — and both were the same defect this PR fixes, one level up: a bound sized as ifdate +%swere continuous. FixingspentFlooras(N - 1) * 0.75rather than nudging the fraction until it stopped failing is the correct response, and sharing it with the pull-error floor means a third instance cannot appear.414bf880refuses the tempting generalisation, which is the right call. It would have been easy to apply BLO-31405's 3x margin to this step by analogy; instead it separates the two by variance profile — fixedsleep(7% spread, so ~2.5x is ample) versus 18helm templateforks (21.8s local to 32-73s on CI). Applying one multiple to both would have stranded this bound or under-sized that one. My independent run measured 23.9s, corroborating the 23s recorded there.- The stub models
kubectl waithonestly, and its one deliberate divergence (condition=completeresolving early on a failed job instead of blocking the full--timeout) is documented with the upstream citation, the reason, the ~2s it buys, and the note that the script's verdict is identical either way. The operational corollary is called out too — a genuineFAILEDspends the whole run budget before printing — which is precisely the fact the job-timeout fold has to cover. startup_secondsis documented as an upper bound, with an explicit instruction not to size the next budget from it. That is the kind of note that stops a future reader turning a quantized diagnostic into a measurement.
Recommended Action
- No Critical or Important issues — this is mergeable as it stands.
- Consider the two Suggestions opportunistically; the
pr.ymltimeout-minutesline in the description is the only one with any chance of misleading someone later.
Posted as a formal COMMENTED review: this PR is authored by the Ally App, and GitHub bars a PR's author from APPROVE. reviewDecision is empty on this repo (master carries a merge_queue rule and no pull_request rule), so no approval is required to merge and nothing here is gated on an identity I cannot supply.
Thinking Path
Linked Issues or Issue Description
Paperclip issue: BLO-31254. No corresponding GitHub issue exists, so per path (B) the bug is described here.
What happened. Run 33601878894 (2026-09-02) deployed
1c72b1cbatopaperclip-production. Step 15pending-migration pre-flightfailed; step 16helm upgradewas skipped.Pod events show the cause:
Expected. The pre-flight's budget bounds migration execution, so a slow image pull should not fail the gate.
Actual. The budget bounds
pull + run. A 183s pull against a 180s budget killed the container 3s after it started, before it could emit a verdict.Impact. No migration ran and production was untouched and healthy throughout (api 2/2, worker 1/1, helm rev 781
deployed, nopending-upgradelock) — this was never a correctness or data-safety incident. It is a reliability one: any deploy landing on a node without the new digest cached was a coin flip, and the retry only passed because the first attempt warmed the node cache. That trains operators to retry past a gate rather than read it.Repro. Deploy any freshly built image to a node that has not cached that digest — reliable on the first deploy after a build.
Environment.
.github/workflows/docker.yml→deployjob →.github/scripts/check-pending-migrations.sh, on-prem cluster, Harbor registry.What Changed
.github/scripts/check-pending-migrations.sh— split the single wait into two phases.Pending, bounded by a newPREFLIGHT_STARTUP_TIMEOUT_SECONDS(default 600s, ~3x the measured 183s cold pull).PREFLIGHT_TIMEOUT_SECONDS(unchanged at 180s) only after the container is up, so it now measures what it was always sized for.Succeeded/Failedcount as "started": withbackoffLimit: 0a fast check can finish between two polls, and reading that as "never started" would discard a real verdict.<reason>) within Ns of the 600s startup budget; the migration check itself never ran. This is slow or broken infrastructure (image pull, scheduling), not a migration verdict." Names the observed waiting reason and dumps the pod events inline, so the pull duration and image size no longer require an operator to godescribethe pod by hand.PASSEDnow also reports the observed startup time, so cold-pull duration is visible on the happy path.InvalidImageName/CreateContainerConfigErrorbreak out rather than riding out the 600s budget, since neither self-heals, and are reported as a terminal container error rather than as budget exhaustion (saying "never started within its 600s startup budget" after 0s would point the operator at a budget that was never the constraint). Pull errors are deliberately excluded:ErrImagePull/ImagePullBackOffroutinely recover, and failing fast on them would recreate this very defect.scripts/check-pending-migration-preflight-phases.test.js(new, 9 tests) — drives the script for real against a stubkubectl.scripts/check-pending-migration-preflight.test.js— one added test holding the shipped budget defaults (startup ≥ 366s and > run), which the behavioural suite cannot check because it overrides both..github/workflows/pr.yml— wired the new suite into thepolicyjob (timeout-minutes: 1, matching its neighbours; the step comment records that the bound was set from a measured ~23s local run, not by analogy to another suite's margin).Verification
The defect was pure control flow, so a render-level test cannot tell a fix from a rewording. The new suite therefore executes the script against a stub
kubectlthat modelskubectl waithonestly — it blocks up to--timeoutand reportscompleteonly once the container has actually started.node --test scripts/check-pending-migration-preflight.test.js \ scripts/check-pending-migration-preflight-phases.test.js # 18/18 passConfirmed to be a real regression test, not a tautology. I ran the new suite against the pre-fix script (
git show HEAD~1:.github/scripts/check-pending-migrations.sh) and the slow-pull case fails with the production symptom verbatim:Cases covered: slow pull still passes (startup outlasts the run budget) · container never starts → infra-attributed message + inline pod events · started-but-unfinished → migration-attributed message · the two INCONCLUSIVE lines are textually distinct · failing check still reports the migration verdict + remediation logs · pod already
Succeededon first poll is not misread as never-started · terminal config error fast-fails · transientImagePullBackOffrides out the budget.Also run green:
bash -n .github/scripts/check-pending-migrations.sh node --test scripts/check-docker-deploy-timeout.test.js \ scripts/check-docker-two-tier-convergence.test.js \ scripts/guard-pending-deploy.test.js # 29/29 passBoth
pr.ymlanddocker.ymlre-parsed as YAML and the new step confirmed present in thepolicyjob.Not run, and why:
pnpm -r typecheck/pnpm test:run/pnpm build— the change touches only a bash script, two Node test files, and workflow YAML; no TypeScript, product source, or build input is affected.shellcheckis not installed in this environment, so the shell was validated withbash -nplus the behavioural suite rather than linted. The end-to-end path (a real cold pull against the real cluster) is only exercisable by an actual production deploy — see Risks.Risks
Low-to-moderate, and the gate still fails closed on every path. No change to what the pre-flight concludes — only to how long it waits before concluding it, and how it words the result.
ImagePullBackOff, since that is the recoverable case this PR exists to stop punishing. An extra ~7 min on an already-broken deploy is the right trade against failing good builds.PREFLIGHT_STARTUP_TIMEOUT_SECONDS) rather than a number entangled with the migration budget, and the render-level test will fail if the default is lowered below 366s.kubectl get events --field-selectoris best-effort — it is piped throughtailwith a fallback, and only runs on a path that is already exiting 1, so it cannot affect the verdict.kubectl wait's contract, not kubelet's; the true end-to-end proof is the next production deploy of a fresh image. The failure mode if the model is wrong is a false INCONCLUSIVE — the same fail-closed refusal as today, not a bad rollout.backoffLimit: 0,restartPolicy: Never, withttlSecondsAfterFinishedand thetrap cleanup EXITintact; the pre-flight still runs strictly beforehelm upgrade.Model Used
Claude Opus 4.5 (
claude-opus-4-5), 1M context, extended thinking enabled, with tool use (file edit, bash, GitHub CLI, Kubernetes MCP), running as the Paperclip Release Engineer agent.Checklist
31254andpre-flight in:titleacross all states — no matches)Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template