-
Notifications
You must be signed in to change notification settings - Fork 90
fix(kwok): deadline-derived sync-gate budgets + 18m tier timeout #1708
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6b34de1
b00976d
2135446
62f2268
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,10 +71,54 @@ inputs: | |
| description: 'Number of days to retain debug artifacts' | ||
| required: false | ||
| default: '7' | ||
| job_timeout_minutes: | ||
| description: >- | ||
| The calling job's timeout-minutes. Used to derive | ||
| KWOK_SYNC_DEADLINE_EPOCH (deadline = step start + this budget - 240s | ||
| diagnostics margin) so each chainsaw sync gate's budget stays within | ||
| that margin, letting the gate finish - and print its catch-block | ||
| 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 | ||
| timeout cannot silently drift from a stale default (a missing value | ||
| fails the integer validation below loudly). | ||
| required: true | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: all three call sites pass
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Made it required and dropped the default in 624a947 — a missing value now fails the derive step's integer validation loudly. |
||
| runs: | ||
| using: 'composite' | ||
| steps: | ||
| - name: Derive sync-gate deadline | ||
| shell: bash | ||
| env: | ||
| JOB_TIMEOUT_MINUTES: ${{ inputs.job_timeout_minutes }} | ||
| run: | | ||
| # Anchor the sync-gate deadline as early as possible in the job: | ||
| # only checkout + load-versions run before this step, so the | ||
| # unaccounted job-start drift stays inside the 240s margin's 60s | ||
| # allowance (the other 180s is reserved for post-gate work: | ||
| # chainsaw catch diagnostics, verify_pods, debug-artifact upload). | ||
| # Anchoring later (at the test step, after setup-go + tool installs | ||
| # + make build) lets the deadline land past GitHub's job kill and | ||
| # silently re-creates the CANCELLED-without-diagnostics failure | ||
| # this mechanism exists to prevent. | ||
| # See kwok/scripts/lib/sync-budget.sh for the consuming side. | ||
| 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 | ||
| # 120 must equal SYNC_BUDGET_FLOOR_SECONDS in kwok/scripts/lib/sync-budget.sh | ||
| # (hand-synced literal, guarded by sync-budget_test.sh — same contract as the | ||
| # 240s margin). A budget under the floor would compute an already-unusable | ||
| # deadline that only fails later, inside the job, after setup burned CI minutes. | ||
| if (( 10#${JOB_TIMEOUT_MINUTES} * 60 - 240 < 120 )); then | ||
| echo "::error::job_timeout_minutes=${JOB_TIMEOUT_MINUTES} leaves no usable sync budget after the 240s diagnostics margin (need >= 6)" | ||
| exit 1 | ||
| fi | ||
| KWOK_SYNC_DEADLINE_EPOCH=$(( $(date +%s) + 10#${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, job kill in ~$(( 10#${JOB_TIMEOUT_MINUTES} * 60 ))s)" | ||
|
|
||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| - name: Set up Go | ||
| uses: actions/setup-go@7a3fe6cf4cb3a834922a1244abfce67bcef6a0c5 # v6.2.0 | ||
| with: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,6 +92,9 @@ jobs: | |
| # Full checkout needed for diff-aware Tier 2 discovery | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Script unit tests (kwok/scripts/lib) | ||
| run: bash kwok/scripts/lib/sync-budget_test.sh | ||
|
|
||
| - name: Classify recipes into tiers | ||
| id: classify | ||
| shell: bash | ||
|
|
@@ -288,7 +291,7 @@ jobs: | |
| needs.discover.outputs.tier1 != '[]' && | ||
| needs.discover.outputs.tier1 != '' | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 15 | ||
| timeout-minutes: 18 | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
|
|
@@ -320,6 +323,8 @@ jobs: | |
| chainsaw_version: ${{ steps.versions.outputs.chainsaw }} | ||
| chainsaw_sha256: ${{ steps.versions.outputs.chainsaw_sha256_linux_amd64 }} | ||
| kind_node_image: ${{ steps.versions.outputs.kind_node_image }} | ||
| # keep == this job's timeout-minutes (asserted by sync-budget_test.sh) | ||
| job_timeout_minutes: '18' | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Minor — The two The deadline derives from the passed 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 Fix: Add a grep/unit assertion (alongside
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| # ── Tier 2: diff-aware accelerator tests (PR only, conditional) ── | ||
| test-tier2: | ||
|
|
@@ -330,7 +335,7 @@ jobs: | |
| needs.discover.outputs.tier2 != '[]' && | ||
| needs.discover.outputs.tier2 != '' | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 15 | ||
| timeout-minutes: 18 | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
|
|
@@ -360,6 +365,8 @@ jobs: | |
| chainsaw_version: ${{ steps.versions.outputs.chainsaw }} | ||
| chainsaw_sha256: ${{ steps.versions.outputs.chainsaw_sha256_linux_amd64 }} | ||
| kind_node_image: ${{ steps.versions.outputs.kind_node_image }} | ||
| # keep == this job's timeout-minutes (asserted by sync-budget_test.sh) | ||
| job_timeout_minutes: '18' | ||
|
|
||
| # ── Tier 3: full matrix (push to main + nightly schedule) ── | ||
| # The recipe × deployer cross-product exceeds GitHub's 256-config matrix cap, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| #!/usr/bin/env bash | ||
| # shellcheck shell=bash | ||
| # Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| # Deadline-derived sync-gate budgets for the KWOK deployer-matrix CI lane. | ||
| # | ||
| # Sourced by validate-scheduling.sh. CI (.github/actions/kwok-test) exports | ||
| # KWOK_SYNC_DEADLINE_EPOCH — the absolute epoch after which the job has no | ||
| # time left for sync-gate work (job timeout minus a diagnostics margin). | ||
| # Each chainsaw sync gate derives its budget as min(<gate default>, | ||
| # deadline − now), bounding that gate's assert/error operation so it | ||
| # finishes — and prints its catch-block diagnostics — before GitHub's | ||
| # job timeout kills the runner in the expected single-gate-dominates | ||
| # case (this bounds each gate operation, not the job's whole wall time). | ||
| # | ||
| # Source guard: constants and functions only, no side effects at source | ||
| # time (same contract as lib/cleanup.sh). | ||
|
|
||
| # Below this floor a shrunken budget cannot produce a meaningful gate run; | ||
| # callers fail fast with an explicit error instead — which is itself the | ||
| # diagnosis a silent CANCELLED would have destroyed. | ||
| readonly SYNC_BUDGET_FLOOR_SECONDS=120 | ||
|
|
||
| # compute_sync_budget <default_seconds> [<now_epoch>] | ||
| # | ||
| # Prints the effective sync-gate budget (seconds) to stdout: | ||
| # - KWOK_SYNC_DEADLINE_EPOCH unset/empty: <default_seconds> unchanged | ||
| # (local runs keep today's fixed budgets). | ||
| # - Set: min(<default_seconds>, KWOK_SYNC_DEADLINE_EPOCH − now). | ||
| # Returns 1 (printing nothing) when the derived budget is below | ||
| # SYNC_BUDGET_FLOOR_SECONDS — callers must fail fast (exit code 50). | ||
| # <now_epoch> is injectable for tests; defaults to $(date +%s). | ||
| compute_sync_budget() { | ||
| local default_seconds="$1" | ||
| local now="${2:-$(date +%s)}" | ||
| if [[ -z "${KWOK_SYNC_DEADLINE_EPOCH:-}" ]]; then | ||
| echo "${default_seconds}" | ||
| return 0 | ||
| fi | ||
| local remaining=$(( KWOK_SYNC_DEADLINE_EPOCH - now )) | ||
| local budget="${default_seconds}" | ||
| if (( remaining < budget )); then | ||
| budget="${remaining}" | ||
| fi | ||
| if (( budget < SYNC_BUDGET_FLOOR_SECONDS )); then | ||
| return 1 | ||
| fi | ||
| echo "${budget}" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,147 @@ | ||
| #!/usr/bin/env bash | ||
| # Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| # Unit harness for lib/sync-budget.sh (deadline-derived sync-gate budgets). | ||
| # Run directly: bash kwok/scripts/lib/sync-budget_test.sh | ||
| # Wired into CI by the kwok-recipes discover job. | ||
| set -uo pipefail | ||
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| # Resolve the subject SCRIPT_DIR-relative — never a deployed copy. | ||
| # shellcheck source=sync-budget.sh | ||
| source "${SCRIPT_DIR}/sync-budget.sh" | ||
|
|
||
| fails=0 | ||
| check() { # <name> <want_rc> <want_stdout> <got_rc> <got_stdout> | ||
| local name="$1" want_rc="$2" want_out="$3" got_rc="$4" got_out="$5" | ||
| if [[ "${got_rc}" == "${want_rc}" && "${got_out}" == "${want_out}" ]]; then | ||
| echo "PASS: ${name}" | ||
| else | ||
| echo "FAIL: ${name} (want rc=${want_rc} out='${want_out}'; got rc=${got_rc} out='${got_out}')" | ||
| fails=$((fails + 1)) | ||
| fi | ||
| } | ||
|
|
||
| # 1. Env unset -> default passthrough (local runs keep fixed budgets). | ||
| unset KWOK_SYNC_DEADLINE_EPOCH | ||
| out=$(compute_sync_budget 500 1000000); rc=$? | ||
| check "env-unset-returns-default" 0 "500" "${rc}" "${out}" | ||
|
|
||
| # 2. Ample remaining (10000s) -> default wins the min(). | ||
| export KWOK_SYNC_DEADLINE_EPOCH=1010000 | ||
| out=$(compute_sync_budget 500 1000000); rc=$? | ||
| check "ample-remaining-returns-default" 0 "500" "${rc}" "${out}" | ||
|
|
||
| # 3. Tight remaining (300s < 500s default) -> budget shrinks to remaining. | ||
| export KWOK_SYNC_DEADLINE_EPOCH=1000300 | ||
| out=$(compute_sync_budget 500 1000000); rc=$? | ||
| check "tight-remaining-shrinks-budget" 0 "300" "${rc}" "${out}" | ||
|
|
||
| # 4. Remaining exactly at the 120s floor -> still runs (boundary). | ||
| export KWOK_SYNC_DEADLINE_EPOCH=1000120 | ||
| out=$(compute_sync_budget 500 1000000); rc=$? | ||
| check "floor-boundary-runs" 0 "120" "${rc}" "${out}" | ||
|
|
||
| # 5. Remaining below floor (119s) -> rc 1, no output (caller fails fast). | ||
| export KWOK_SYNC_DEADLINE_EPOCH=1000119 | ||
| out=$(compute_sync_budget 500 1000000); rc=$? | ||
| check "below-floor-fails" 1 "" "${rc}" "${out}" | ||
|
|
||
| # 6. Deadline already in the past -> rc 1, no output. | ||
| export KWOK_SYNC_DEADLINE_EPOCH=999000 | ||
| out=$(compute_sync_budget 500 1000000); rc=$? | ||
| check "deadline-past-fails" 1 "" "${rc}" "${out}" | ||
|
|
||
| # 7. Literal-sync guard: job_timeout_minutes must equal the SAME job's | ||
| # timeout-minutes in every caller workflow. This is a hand-synced literal | ||
| # (the composite action cannot read the calling job's own timeout-minutes); | ||
| # drift silently re-creates the CANCELLED-without-diagnostics failure this | ||
| # whole mechanism exists to prevent. Resolve workflows SCRIPT_DIR-relative | ||
| # so this always tests THIS checkout, never a deployed copy. | ||
| REPO_ROOT="$(cd "${SCRIPT_DIR}/../../.." && pwd)" | ||
|
|
||
| # job_timeout_sync <workflow_file> -> one "<job>:jtm=<X>,tm=<Y>" line per | ||
| # job that sets job_timeout_minutes, where <X> is that value and <Y> is | ||
| # the nearest preceding job-level (4-space-indented) timeout-minutes | ||
| # ("unset" if none was seen for the current job). | ||
| 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:')" | ||
|
Comment on lines
+73
to
+99
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win Regex-based YAML parsing in The parser depends on exact 2-space job-key indent and exact 4-space 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 |
||
|
|
||
| # 10-11. Margin-floor guard in the "Derive sync-gate deadline" step | ||
| # (action.yml): job_timeout_minutes must leave >= SYNC_BUDGET_FLOOR_SECONDS | ||
| # of usable budget after the 240s diagnostics margin, or the step must fail | ||
| # fast before toolchain setup + make build burn CI minutes. Extracted | ||
| # straight from action.yml (not reimplemented here) so this always exercises | ||
| # THIS checkout's actual CI logic, never a stale copy. | ||
| ACTION_YML="${REPO_ROOT}/.github/actions/kwok-test/action.yml" | ||
|
|
||
| # extract_derive_step <file> -> the dedented run: block of the "Derive | ||
| # sync-gate deadline" step. | ||
| extract_derive_step() { | ||
| local file="$1" | ||
| awk ' | ||
| /^ - name: Derive sync-gate deadline$/ { in_step=1; next } | ||
| in_step && /^ run: \|$/ { in_run=1; next } | ||
| in_run && /^ - name:/ { exit } | ||
| in_run { sub(/^ /, ""); print } | ||
| ' "${file}" | ||
| } | ||
|
|
||
| # run_derive_step <job_timeout_minutes> -> sets got_rc, got_env_file | ||
| # (caller-owned temp file, removed by caller). -eo pipefail mirrors the | ||
| # composite step's actual shell (`shell: bash` -> bash -eo pipefail) so a | ||
| # future set -e interaction cannot diverge between CI and this harness. | ||
| run_derive_step() { | ||
| local jtm="$1" | ||
| got_env_file=$(mktemp) | ||
| JOB_TIMEOUT_MINUTES="${jtm}" GITHUB_ENV="${got_env_file}" \ | ||
| bash -eo pipefail -c "$(extract_derive_step "${ACTION_YML}")" > /dev/null 2>&1 | ||
| got_rc=$? | ||
| } | ||
|
|
||
| run_derive_step 5 | ||
| check "derive-step-below-floor-fails" 1 "" "${got_rc}" "" | ||
| rm -f "${got_env_file}" | ||
|
|
||
| run_derive_step 6 | ||
| env_has_deadline="no" | ||
| grep -q '^KWOK_SYNC_DEADLINE_EPOCH=' "${got_env_file}" && env_has_deadline="yes" | ||
| check "derive-step-at-floor-boundary-succeeds" 0 "yes" "${got_rc}" "${env_has_deadline}" | ||
| rm -f "${got_env_file}" | ||
|
|
||
| if (( fails > 0 )); then | ||
| echo "${fails} test(s) failed" | ||
| exit 1 | ||
| fi | ||
| echo "All 11 tests passed" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
job_timeout_minutesis now a required input, but.github/actions/README.md(thekwok-testinput 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.