fix(kwok): deadline-derived sync-gate budgets + 18m tier timeout - #1708
Conversation
|
🌿 Preview your docs: https://nvidia-preview-fix-kwok-sync-gate-deadline.docs.buildwithfern.com/aicr |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a CI-derived absolute sync deadline with a 240-second diagnostic margin. A shared Bash helper computes bounded sync budgets with a 120-second floor, and Argo CD and Flux gates use those budgets. The action validates and exports the deadline, Tier 1–3 workflows pass the job timeout and use 18-minute limits, unit tests run during discovery, and contributor documentation describes the new CI-only variable. Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/kwok-test/action.yml:
- Around line 87-109: Validate in the “Derive sync-gate deadline” step that
JOB_TIMEOUT_MINUTES*60 minus the 240-second margin is at least
SYNC_BUDGET_FLOOR_SECONDS before calculating or exporting
KWOK_SYNC_DEADLINE_EPOCH; emit a clear ::error message and exit nonzero when the
budget is too small, while preserving the existing integer validation and
deadline setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 0577dab7-f3a3-46c9-af7d-6826503e83c7
📒 Files selected for processing (7)
.github/actions/kwok-test/action.yml.github/workflows/kwok-recipes.yaml.github/workflows/kwok-tier3-shard.yamldocs/contributor/tests.mdkwok/scripts/lib/sync-budget.shkwok/scripts/lib/sync-budget_test.shkwok/scripts/validate-scheduling.sh
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
Sound design: budget = min(default, deadline − now) with a 120s fail-fast floor that converts the silent CANCELLED into an explicit "setup consumed the budget" diagnosis, env-unset preserves local behavior, and the unit harness gates the lane from the discover job before any cluster spins up. The flux path reusing EXIT_ARGOCD_SYNC_TIMEOUT matches the pre-existing documented pattern, and anchoring the deadline in the action's first step is the right call. One nit on the job_timeout_minutes default inline; nothing blocking. CI green.
| Keep in sync with the calling job's timeout-minutes. | ||
| required: false | ||
| default: '18' | ||
|
|
There was a problem hiding this comment.
nit: all three call sites pass job_timeout_minutes explicitly, and the "keep in sync with the calling job's timeout-minutes" contract is manual — a future caller that bumps its timeout-minutes and relies on this stale default silently re-creates the CANCELLED-without-diagnostics failure this mechanism exists to prevent. Dropping the default (making the input required) turns that drift into a loud failure at wiring time.
There was a problem hiding this comment.
Made it required and dropped the default in 624a947 — a missing value now fails the derive step's integer validation loudly.
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
🔍 Multi-Persona Review
Method: 4 independent persona reviewers (Correctness · CI-DX/Operability · Domain & Architecture · Docs), each finding routed through an adversarial senior meta-reviewer that re-derived it from the resolved code at 624a947d.
Tier legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick
Overall assessment
This is a well-reasoned, well-tested CI fix. The core mechanism is sound: the deadline is anchored in the action's first step so pre-setup drift stays inside the margin, both gates consume min(default, deadline−now) with a 120s fail-fast floor, the unit harness runs in discover before any cluster spins up, and local runs are unaffected when the env var is unset. The highest-risk theories were independently cleared: the flux gate's reuse of EXIT_ARGOCD_SYNC_TIMEOUT (50) is intended and consumer-correct (run-all-recipes.sh matches argocd-*||flux-* on rc 50), there's no double-charge (one recipe×deployer per job; the absolute deadline shrinks budgets monotonically), the readonly const is sourced exactly once, and the 6 unit tests cover the floor boundary precisely.
Nothing here blocks. What surfaced is latent foot-guns and doc drift, all inline below.
Recommendation: Approve with comments.
✅ Confirmed non-issues (examined and cleared)
- Flux gate returns
EXIT_ARGOCD_SYNC_TIMEOUT(50) — intentional shared code; the header comment scopes 50 to both gates andrun-all-recipes.shmatchesargocd-*||flux-*on it. Not a naming bug. - Double-charging the budget — one recipe×deployer per job; the absolute deadline shrinks budgets monotonically. Correct.
readonly SYNC_BUDGET_FLOOR_SECONDSre-source error — sourced exactly once byvalidate-scheduling.sh;run-all-recipes.shinvokes it as a subprocess. Matches thecleanup.shcontract.set -earithmetic landmines incompute_sync_budget— every(( ))is an if-condition; theif ! budget=$(…)capture correctly propagatesreturn 1.- Cold-cache exit-50 misattribution — already handled: the fail-fast branch logs
"…<120s left before the job deadline… — setup consumed the budget", which is the diagnosis. .github/actions/README.mdnot updated — this PR doesn't touch it, and its kwok-test example already omits otherrequiredinputs; illustrative, not copy-paste-complete. Not a regression this PR introduced (though a broader README refresh wouldn't hurt).required: trueis advisory for composite inputs — true, but the regex guard rejects the empty/missing case loudly, so the "fails loudly" claim holds.
Summary
🔴 Blocker 0 | 🟠 Major 0 | 🟡 Minor 3 | 🔵 Nitpick 2 Recommendation: Approve with comments
The three 🟡s are all cheap: a sync-assertion for the twin 18s, one doc line, and a two-char regex tightening. None gate merge.
Review method: multi-persona fan-out + adversarial meta-review.
| chainsaw_version: ${{ steps.versions.outputs.chainsaw }} | ||
| chainsaw_sha256: ${{ steps.versions.outputs.chainsaw_sha256_linux_amd64 }} | ||
| kind_node_image: ${{ steps.versions.outputs.kind_node_image }} | ||
| job_timeout_minutes: '18' |
There was a problem hiding this comment.
🟡 Minor — The two 18s are unguarded hand-synced literals — the dangerous drift direction re-creates the exact bug this PR fixes
The deadline derives from the passed job_timeout_minutes, but GitHub kills the job at the separate timeout-minutes (L294). These are two literals per job, hand-synced across three call sites (tier1 L294↔L326, tier2 L337↔L367, kwok-tier3-shard L38↔L69). If a future edit ever makes job_timeout_minutes > timeout-minutes (e.g. bump the input to 25, forget the job cap), deadline = derive_time + 1500 − 240 lands past the job_start + 1080 kill → min() never shrinks → the gate runs until GitHub kills it → silent CANCELLED-without-diagnostics, the precise failure this PR exists to prevent.
Blast radius: All KWOK tier1/2/3 lanes. Nothing is broken today (all six literals = 18), hence Minor — but a future timeout bump that touches one literal and not the other silently reintroduces the diagnostics-loss regression. GitHub exposes no context for a job's own timeout-minutes, so it genuinely can't be auto-derived.
Fix: Add a grep/unit assertion (alongside sync-budget_test.sh) that each caller's job_timeout_minutes equals its timeout-minutes, plus a paired # keep == timeout-minutes above comment at each of the 3 sites.
There was a problem hiding this comment.
Added three cases to sync-budget_test.sh (tier1/tier2/tier3-shard) asserting each caller's job_timeout_minutes equals that job's timeout-minutes, plus paired comments at all three sites — mutation-checked: drifting one literal to 19 fails the matching case. cfb4961.
| In CI, the `kwok-test` action derives `KWOK_SYNC_DEADLINE_EPOCH` in its | ||
| first step — before toolchain setup and the `aicr` build, so the anchor | ||
| sits within ~60 s of job start — from its `job_timeout_minutes` input | ||
| (default `18`, kept in sync with the calling job's `timeout-minutes`) |
There was a problem hiding this comment.
🟡 Minor — Docs claim job_timeout_minutes has "default 18" — but the action declares it required: true with no default
The prose says the deadline is derived "from its job_timeout_minutes input (default 18…)". But the second commit flipped action.yml to required: true with no default and a description that explicitly forbids a stale default — the docs still describe the exact behavior the action was redesigned to forbid. A contributor reading this will believe omitting the input is safe; it isn't (the empty string fails the ^[0-9]+$ regex). It fails loudly, which is why this is Minor and not higher.
Blast radius: Doc-only inaccuracy; also misrepresents the deliberate no-default design rationale. The same wording appears in the PR body — worth correcting there too for the permanent record.
Fix: Drop "default 18"; state the input is required, integer, and must match the caller's timeout-minutes (currently 18 for the KWOK jobs).
There was a problem hiding this comment.
Reworded in cfb4961: required, integer, must match the caller's timeout-minutes (currently 18). Corrected the same claim in the PR description.
| echo "::error::job_timeout_minutes must be an integer, got '${JOB_TIMEOUT_MINUTES}'" | ||
| exit 1 | ||
| fi | ||
| KWOK_SYNC_DEADLINE_EPOCH=$(( $(date +%s) + JOB_TIMEOUT_MINUTES * 60 - 240 )) |
There was a problem hiding this comment.
🟡 Minor — Integer guard ^[0-9]+$ admits zero-padded values → octal misparse
The regex accepts leading zeros, but the arithmetic $(( … + JOB_TIMEOUT_MINUTES*60 − 240 )) reads them in base-8. 010 → 8 (deadline silently too short, computed with no error — the exact silent-miscompute class this PR fights); 08/09 → value too great for base, which under the composite step's set -eo pipefail aborts the step before the friendly ::error:: line fires.
Blast radius: Latent — all three callers pass 18 — but the validation block's whole job is to reject bad input loudly, and for zero-padded inputs it doesn't.
Fix: Tighten to ^[1-9][0-9]*$, or force base-10: $(( … + 10#${JOB_TIMEOUT_MINUTES}*60 − 240 )).
There was a problem hiding this comment.
Tightened to ^[1-9][0-9]*$ with a clearer error, and forced base-10 (10#) at both arithmetic sites. cfb4961.
| local sync_timeout="${KWOK_FLUX_SYNC_TIMEOUT:-500}s" | ||
| log_info "Flux sync timeout: ${sync_timeout}" | ||
| local budget_seconds | ||
| if ! budget_seconds=$(compute_sync_budget "${KWOK_FLUX_SYNC_TIMEOUT:-500}"); then |
There was a problem hiding this comment.
🔵 Nitpick — "Always finishes before the kill" is mildly overstated — the budget is per-chainsaw-operation, not a whole-test wall bound
The single derived budget is passed as both --assert-timeout and --error-timeout, which chainsaw applies per operation. The flux gate has ~4 sequential asserts; a chain of slow-but-eventually-passing steps can accumulate N×budget and overrun the deadline.
Blast radius: Pre-existing (identical per-op mechanics with the old fixed 480/500s) and low-probability (first timed-out step aborts the test; KWOK resources go ready-fast-or-stuck). A wording caveat, not a defect.
Fix: Note in the sync-budget.sh header that the budget is per-operation — the guarantee holds for a single stuck step, not N slow-passing ones.
There was a problem hiding this comment.
Softened in cfb4961 — the overstated claim actually lived in sync-budget.sh's header comment and the action description (validate-scheduling.sh sources the lib); both now state the per-operation bound rather than a whole-test wall guarantee.
| fi | ||
| KWOK_SYNC_DEADLINE_EPOCH=$(( $(date +%s) + JOB_TIMEOUT_MINUTES * 60 - 240 )) | ||
| echo "KWOK_SYNC_DEADLINE_EPOCH=${KWOK_SYNC_DEADLINE_EPOCH}" >> "$GITHUB_ENV" | ||
| echo "KWOK_SYNC_DEADLINE_EPOCH=${KWOK_SYNC_DEADLINE_EPOCH} (job_timeout_minutes=${JOB_TIMEOUT_MINUTES}, margin=240s)" |
There was a problem hiding this comment.
🔵 Nitpick — Derive step logs the deadline epoch but not the expected-kill offset
If pre-derive drift ever exceeds 240s (unlikely — shallow checkout), the deadline lands past the kill and the mechanism silently self-disables. The step already logs the epoch/minutes/margin, so it's mostly observable; what's missing is a comparison against the expected kill time to make the degenerate case greppable.
Blast radius: Observability nit only; low probability given shallow checkout and the 60s drift allowance.
Fix: Also log the expected kill epoch and warn if deadline ≥ kill.
There was a problem hiding this comment.
The derive step's log line now includes the implied kill offset (job kill in ~Ns) next to the deadline, so a drifted deadline is visible at a glance. cfb4961.
cfb4961 to
cae22b6
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/actions/kwok-test/action.yml (1)
106-112: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMargin-exceeds-budget case still isn't validated — this was flagged in a prior review round and appears unaddressed.
The step validates
job_timeout_minutesis a positive integer, but never checks thatjob_timeout_minutes*60 - 240 >= SYNC_BUDGET_FLOOR_SECONDS (120). If a future caller wires a smalljob_timeout_minutes(or the 240s margin is later raised) without correspondingly raising this floor,KWOK_SYNC_DEADLINE_EPOCHcomputes to an already-past or barely-positive value here, but the failure only surfaces later — after Go setup, kind prep, andmake buildhave already burned CI minutes — when the first sync gate hitscompute_sync_budget's floor check.All three current callers pass
18, so nothing breaks today, but the gap that was previously called out (fail fast on an unworkable margin, right after the input format check) is still open in this version of the file.🛡️ Proposed fix to fail fast on an unworkable margin
if ! [[ "${JOB_TIMEOUT_MINUTES}" =~ ^[1-9][0-9]*$ ]]; then echo "::error::job_timeout_minutes must be a positive integer, no leading zeros, got '${JOB_TIMEOUT_MINUTES}'" exit 1 fi + if (( 10#${JOB_TIMEOUT_MINUTES} * 60 - 240 < 120 )); then + echo "::error::job_timeout_minutes=${JOB_TIMEOUT_MINUTES} leaves no usable budget after the 240s diagnostics margin" + exit 1 + fi KWOK_SYNC_DEADLINE_EPOCH=$(( $(date +%s) + 10#${JOB_TIMEOUT_MINUTES} * 60 - 240 ))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/kwok-test/action.yml around lines 106 - 112, After the positive-integer validation for JOB_TIMEOUT_MINUTES, add a fail-fast check that verifies the computed timeout budget (JOB_TIMEOUT_MINUTES*60 minus the 240-second margin) is at least SYNC_BUDGET_FLOOR_SECONDS (120). Emit a clear GitHub Actions error and exit nonzero when this constraint fails, before calculating or exporting KWOK_SYNC_DEADLINE_EPOCH.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.github/actions/kwok-test/action.yml:
- Around line 106-112: After the positive-integer validation for
JOB_TIMEOUT_MINUTES, add a fail-fast check that verifies the computed timeout
budget (JOB_TIMEOUT_MINUTES*60 minus the 240-second margin) is at least
SYNC_BUDGET_FLOOR_SECONDS (120). Emit a clear GitHub Actions error and exit
nonzero when this constraint fails, before calculating or exporting
KWOK_SYNC_DEADLINE_EPOCH.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b253e3f2-4731-4462-85e0-9dc86edd43ce
📒 Files selected for processing (7)
.github/actions/kwok-test/action.yml.github/workflows/kwok-recipes.yaml.github/workflows/kwok-tier3-shard.yamldocs/contributor/tests.mdkwok/scripts/lib/sync-budget.shkwok/scripts/lib/sync-budget_test.shkwok/scripts/validate-scheduling.sh
cae22b6 to
a30bb2e
Compare
KWOK tier jobs (timeout-minutes: 15) raced the chainsaw sync gates' fixed budgets (flux 500s, argocd 480s): GitHub killed the job ~1min before chainsaw would time out and print its catch-block diagnostics, so failures surfaced as CANCELLED with zero error report (e.g. job 86181256207). The budget was over-committed even on the good path: ~300s setup + 500s gate + ~180s post-gate diagnostics/artifacts > 900s. Fix, in four parts: - kwok/scripts/lib/sync-budget.sh: compute_sync_budget derives each gate budget as min(default, KWOK_SYNC_DEADLINE_EPOCH - now) with a 120s floor below which the gate fails fast (existing exit code 50) with an explicit 'setup consumed the budget' error. Unit harness sibling is wired into the discover job. Local runs (env unset) keep today's fixed budgets. - kwok/scripts/validate-scheduling.sh: both the argocd and flux sync gates consume the helper; existing KWOK_*_SYNC_TIMEOUT overrides still feed the default side; the derived budget is logged every run. - .github/actions/kwok-test: new job_timeout_minutes input (default 18); the deadline is derived in the action's FIRST step (before toolchain setup and the aicr build, so pre-action drift stays within the 240s margin's 60s allowance) and exported via GITHUB_ENV; the input is integer-guarded. - Workflows: tier timeout-minutes 15 -> 18 (tier1/tier2/tier3-shard), since 18m fits setup + full gate + diagnostics margin with slack; each kwok-test call site passes job_timeout_minutes explicitly. Verified live: full flux-oci lane passes with an ample deadline (gate logs its full 500s budget); a nearly-consumed deadline produces the explicit fail-fast error instead of a silent CANCELLED. docs/contributor/tests.md documents the KWOK_SYNC_DEADLINE_EPOCH contract. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
All three call sites already pass it explicitly; a default that drifts from a caller's timeout-minutes would silently re-create the CANCELLED-without-diagnostics failure this mechanism prevents. A missing value now fails the derive step's integer validation loudly. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Adds a sync-budget_test.sh case asserting each caller's job_timeout_minutes matches its timeout-minutes (drift silently re-creates the CANCELLED-without-diagnostics failure); tightens the integer guard against zero-padded octal misparse and forces base-10 arithmetic; corrects the stale 'default 18' doc claim; per-operation budget wording; derive step logs the implied kill offset. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
The derive step validated job_timeout_minutes as a positive integer but never checked that the resulting sync-gate budget (job_timeout_minutes*60 - 240s margin) clears the SYNC_BUDGET_FLOOR_SECONDS floor consumed by compute_sync_budget. A misconfigured caller (small job_timeout_minutes, or a future bump to the 240s margin) would compute an already-past or barely-positive deadline that only surfaced at the first sync gate — after Go setup, kind prep, and make build had already burned CI minutes. Add the same floor check at derive time so misconfiguration fails before any of that work starts. The 120 literal is hand-synced with SYNC_BUDGET_FLOOR_SECONDS in kwok/scripts/lib/sync-budget.sh and is guarded by sync-budget_test.sh, same contract as the existing 240s margin sync check. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
a30bb2e to
62f2268
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@kwok/scripts/lib/sync-budget_test.sh`:
- Around line 73-99: Replace the indent-sensitive regex parsing in
job_timeout_sync with yq-based YAML introspection, using job names and their
timeout-minutes and job_timeout_minutes values directly from the workflow
structure. Preserve the existing output format of job:jtm=...,tm=... and the
current synchronization checks, while allowing normal YAML reformatting and
inline comments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 1e7c686c-55e4-4d85-b6ee-95baf5aa5dd3
📒 Files selected for processing (7)
.github/actions/kwok-test/action.yml.github/workflows/kwok-recipes.yaml.github/workflows/kwok-tier3-shard.yamldocs/contributor/tests.mdkwok/scripts/lib/sync-budget.shkwok/scripts/lib/sync-budget_test.shkwok/scripts/validate-scheduling.sh
| job_timeout_sync() { | ||
| local file="$1" job="" job_timeout="" | ||
| while IFS= read -r line; do | ||
| if [[ "${line}" =~ ^\ \ ([A-Za-z0-9_-]+):[[:space:]]*$ ]]; then | ||
| job="${BASH_REMATCH[1]}" | ||
| job_timeout="" | ||
| continue | ||
| fi | ||
| if [[ "${line}" =~ ^\ \ \ \ timeout-minutes:\ *([0-9]+)[[:space:]]*$ ]]; then | ||
| job_timeout="${BASH_REMATCH[1]}" | ||
| continue | ||
| fi | ||
| if [[ "${line}" =~ job_timeout_minutes:\ *\'?([0-9]+)\'? ]]; then | ||
| echo "${job}:jtm=${BASH_REMATCH[1]},tm=${job_timeout:-unset}" | ||
| fi | ||
| done < "${file}" | ||
| } | ||
|
|
||
| kwok_recipes_out=$(job_timeout_sync "${REPO_ROOT}/.github/workflows/kwok-recipes.yaml") | ||
| check "kwok-recipes-tier1-job-timeout-in-sync" 0 "test-tier1:jtm=18,tm=18" \ | ||
| 0 "$(echo "${kwok_recipes_out}" | grep '^test-tier1:')" | ||
| check "kwok-recipes-tier2-job-timeout-in-sync" 0 "test-tier2:jtm=18,tm=18" \ | ||
| 0 "$(echo "${kwok_recipes_out}" | grep '^test-tier2:')" | ||
|
|
||
| tier3_shard_out=$(job_timeout_sync "${REPO_ROOT}/.github/workflows/kwok-tier3-shard.yaml") | ||
| check "kwok-tier3-shard-job-timeout-in-sync" 0 "test:jtm=18,tm=18" \ | ||
| 0 "$(echo "${tier3_shard_out}" | grep '^test:')" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Regex-based YAML parsing in job_timeout_sync is indent-sensitive and will silently stop matching on reformatting.
The parser depends on exact 2-space job-key indent and exact 4-space timeout-minutes indent (Lines 76, 81). A future reformat (e.g. a trailing inline comment on timeout-minutes: 18 # ..., or re-indentation) would make job_timeout fall back to "unset" without any parse error — silently defeating the exact literal-sync guard this test exists to enforce. yq is already a required repo tool (per DEVELOPMENT.md's tools table) and is used elsewhere in this same repo's workflows for YAML introspection; using it here would remove the indent-coupling.
Not blocking — current behavior is correct and mutation-tested — but worth hardening given this guard's whole purpose is catching silent drift.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@kwok/scripts/lib/sync-budget_test.sh` around lines 73 - 99, Replace the
indent-sensitive regex parsing in job_timeout_sync with yq-based YAML
introspection, using job names and their timeout-minutes and job_timeout_minutes
values directly from the workflow structure. Preserve the existing output format
of job:jtm=...,tm=... and the current synchronization checks, while allowing
normal YAML reformatting and inline comments.
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
Solid CI reliability fix — deadline-derived sync budgets with a fail-fast floor turn silent CANCELLED-with-no-diagnostics into an explicit exit-50 report. Logic is correct, boundaries between action.yml and sync-budget.sh are consistent (jtm=6 ↔ 120s floor), the deadline is anchored early so setup drift is accounted for, and the test harness extracts CI logic from source rather than duplicating it. One low nit inline: the new required job_timeout_minutes input isn't reflected in .github/actions/README.md. Nothing blocking.
| diagnostics - before GitHub kills the job in the expected | ||
| single-gate-dominates case (this bounds each gate operation, not the | ||
| job's overall wall time). Required with no default: every caller | ||
| must wire its own timeout-minutes here, so a job that changes its |
There was a problem hiding this comment.
nit: job_timeout_minutes is now a required input, but .github/actions/README.md (the kwok-test input catalog + usage example) wasn't updated — its Inputs list omits this input and the example would fail the integer guard if copy-pasted. Worth adding in this PR.
3 participants
Summary
Makes chainsaw sync-gate diagnostics structurally guaranteed in the KWOK CI lane: gate budgets derive from the remaining job time (new
KWOK_SYNC_DEADLINE_EPOCHcontract) with a fail-fast floor, and tier job timeouts move 15m → 18m so the full budget actually fits.Motivation / Context
KWOK tier jobs (
timeout-minutes: 15) raced the chainsaw sync gates' fixed budgets (flux 500s, argocd 480s): GitHub killed the job ~1 minute before chainsaw would time out and print itscatch:diagnostics, so failures surfaced as CANCELLED with zero error report (job 86181256207 on PR #1686 shows the shape). The budget was over-committed even on the good path: ~300s setup + 500s gate + ~180s post-gate diagnostics/verify/artifact-upload ≈ 980s > 900s. Sibling PR #1707 fixes the Docker Hub throttling that made setup slow enough to hit this consistently; this PR guarantees that whenever anything slow happens again, the lane reports instead of silently dying.Fixes: N/A
Related: #1707, #1686 (diagnosis context)
Type of Change
Component(s) Affected
docs/)kwok/scripts/,.github/actions/kwok-test,.github/workflows/kwok-recipes.yaml,.github/workflows/kwok-tier3-shard.yaml)Implementation Notes
kwok/scripts/lib/sync-budget.sh:compute_sync_budgetreturnsmin(gate default, deadline − now); below a 120s floor the gate fails fast with the existing GitOps sync-deadline exit code (50) and an explicit "setup consumed the budget" error — itself the diagnosis a silent CANCELLED used to destroy. Env unset (local runs) ⇒ behavior unchanged.KWOK_ARGOCD_SYNC_TIMEOUT/KWOK_FLUX_SYNC_TIMEOUToverrides still work; the derived budget is logged on every run so the margin is tunable from evidence.job_timeout_minutesinput is required (no default), integer-guarded (positive, no leading zeros), and passed explicitly by all three call sites — a sync-budget_test.sh case asserts each caller's value matches its job'stimeout-minutes.discoverjob, so a regression fails the lane before any cluster spins up.Testing
Live behavioral verification on a local kind cluster:
Flux sync timeout: 500s (default 500s; deadline-derived when KWOK_SYNC_DEADLINE_EPOCH is set).[ERROR] Flux sync gate: <120s left before the job deadline (…) — setup consumed the budgetand exits 50 — the failure now reports instead of CANCELLED-with-nothing.Risk Assessment
Rollout notes: CI-only behavior; local runs are unaffected (env unset ⇒ fixed defaults). Revert restores the fixed budgets and 15m timeouts. If the 240s margin proves mistuned, the derived budget is logged on every run for evidence-based adjustment.
Checklist
make testwith-race)make lint)git commit -S) — GPG signing info