diff --git a/.github/workflows/cloud-deploy-relay-production-same-cap-job.yml b/.github/workflows/cloud-deploy-relay-production-same-cap-job.yml index 1e924e2c79d..fec048bf4b2 100644 --- a/.github/workflows/cloud-deploy-relay-production-same-cap-job.yml +++ b/.github/workflows/cloud-deploy-relay-production-same-cap-job.yml @@ -626,7 +626,7 @@ jobs: | select(.change.actions | any(. != "no-op" and . != "read")) | .address] | join(","))' echo 'requiring reviewed rollback-image drift' - terraform -chdir=infra/terraform show -json \ + RESUME_REVIEW="$(terraform -chdir=infra/terraform show -json \ "${RUNNER_TEMP}/relay-same-cap-resume.tfplan" \ | node dev/scripts/validate-relay-capacity-plan.mjs \ --mode same-cap-cell --cell-id "${TARGET_CELL_ID}" \ @@ -638,9 +638,23 @@ jobs: --rehome-director-service-account "${DIRECTOR_RUNTIME_SERVICE_ACCOUNT}" \ --rehome-audience https://relay.onorca.dev/v1/admin/host-drain \ --regional-rehome-protocol "${DESIRED_REHOME_PROTOCOL}" \ - "${POOL_ARGUMENTS[@]}" \ - | jq -e '.changes == 2 - or (.changes == 0 and ((.backendUpdate // []) | length) > 0)' >/dev/null + "${POOL_ARGUMENTS[@]}")" + echo "${RESUME_REVIEW}" + jq -e '.changes == 2 + or (.changes == 0 and ((.backendUpdate // []) | length) > 0)' \ + <<< "${RESUME_REVIEW}" >/dev/null + # changes == 0 here means the template and MIG are converged and this cell's + # reviewed backend update is the only thing left, so the resume is not complete: + # apply it, or the cell silently keeps the 300-second drain and no request + # logging and the operator reads that as a finished roll. The plan holds nothing + # else (the validator bounded it to this cell's backend, and the template and MIG + # are no-ops in it), and neither attribute restarts an instance, so the + # incarnation check downstream stays honest. Template-and-MIG drift still applies + # nothing, which is what a resume means. + if test "$(jq -er '.changes' <<< "${RESUME_REVIEW}")" = 0; then + terraform -chdir=infra/terraform apply -auto-approve \ + "${RUNNER_TEMP}/relay-same-cap-resume.tfplan" + fi fi gcloud compute instance-groups managed wait-until "${MIG_NAME}" --stable \ --project "${GCP_PROJECT_ID}" --zone "${TARGET_ZONE}" --timeout 900 @@ -829,6 +843,15 @@ jobs: id: shadow-gate if: ${{ inputs.mode != 'verify' }} continue-on-error: true + # continue-on-error bounds this step's contribution to the job outcome, not its clock, + # and its reads are serialised. A timed-out step is a failed step, which continue-on-error + # absorbs; without this bound a Logging 429 storm or an expired credential makes every + # read cost its full retry budget and can push the job past timeout-minutes, and a + # cancelled job takes the whole wave with it. A healthy gate is already minutes of + # serial reads, so both bounds sit above that: the script settles at seven minutes and + # reaching this eight is the pathological case. Eight on top of a ~14-minute cell still + # leaves the job's 75 minutes intact. + timeout-minutes: 8 env: DRAIN_STARTED_AT: ${{ steps.drain.outputs.drain-started-at }} APPLY_STARTED_AT: ${{ steps.apply.outputs.apply-started-at }} @@ -853,6 +876,8 @@ jobs: - name: Publish the shadow health gate verdict if: ${{ inputs.mode != 'verify' }} continue-on-error: true + # One small JSON file; a retrying upload must not spend the wave's remaining minutes either. + timeout-minutes: 2 uses: actions/upload-artifact@v4 with: name: relay-same-cap-shadow-gate-${{ inputs.target-cell-id }}-${{ github.run_id }}.json diff --git a/cloud/dev/scripts/relay-regional-rehome-workflow.test.mjs b/cloud/dev/scripts/relay-regional-rehome-workflow.test.mjs index 4de2034a12e..2e2be1f7cc5 100644 --- a/cloud/dev/scripts/relay-regional-rehome-workflow.test.mjs +++ b/cloud/dev/scripts/relay-regional-rehome-workflow.test.mjs @@ -57,7 +57,8 @@ test('same-cap wrapper is reusable, canary-bound, and sequential', () => { // The relaxation is only safe if the reviewed validator actually runs on // the NON-converged branch, in same-cap-cell mode, with the trust config // the validator requires, restricted to the template-and-MIG change pair or, - // when only the reviewed backend attributes are left, to those alone. + // when only the reviewed backend attributes are left, to those alone — and + // that last case then has to be applied, not waved through as converged. assert.match( job, /if ! terraform -chdir=infra\/terraform show -json[\s\S]{0,220}\| length == 0' >\/dev\/null\n then\n/ @@ -76,8 +77,20 @@ test('same-cap wrapper is reusable, canary-bound, and sequential', () => { ) assert.match( job, - /host-drain \\\n {16}--regional-rehome-protocol "\$\{DESIRED_REHOME_PROTOCOL\}" \\\n {16}"\$\{POOL_ARGUMENTS\[@\]\}" \\\n {14}\| jq -e '\.changes == 2\n {18}or \(\.changes == 0 and \(\(\.backendUpdate \/\/ \[\]\) \| length\) > 0\)' >\/dev\/null/ + /host-drain \\\n {16}--regional-rehome-protocol "\$\{DESIRED_REHOME_PROTOCOL\}" \\\n {16}"\$\{POOL_ARGUMENTS\[@\]\}"\)"\n {12}echo "\$\{RESUME_REVIEW\}"\n {12}jq -e '\.changes == 2\n {16}or \(\.changes == 0 and \(\(\.backendUpdate \/\/ \[\]\) \| length\) > 0\)' \\\n {14}<<< "\$\{RESUME_REVIEW\}" >\/dev\/null/ ) + // A resume whose only unapplied change is the reviewed backend update must apply it. Leaving + // it is how a cell keeps the 300-second drain and no request logging behind a green resume. + assert.match( + job, + /if test "\$\(jq -er '\.changes' <<< "\$\{RESUME_REVIEW\}"\)" = 0; then\n {14}terraform -chdir=infra\/terraform apply -auto-approve \\\n {16}"\$\{RUNNER_TEMP\}\/relay-same-cap-resume\.tfplan"\n {12}fi\n/ + ) + // Template-and-MIG drift still applies nothing on resume, which is what a resume means. + const resumeStep = job.slice( + job.indexOf('- name: Require converged Terraform state and a stable MIG on resume'), + job.indexOf('- name: Apply only the selected same-cap template and MIG') + ) + assert.equal(resumeStep.split('terraform -chdir=infra/terraform apply').length, 2) assert.match(job, /resume requires the isolated migration-only cell/) assert.match(job, /test "\$\{TARGET_INCARNATION\}" = "\$\{SOURCE_INCARNATION\}"/) assert.match(job, /\(.regionalRehomeProtocol \/\/ 0\) == \$protocol/) diff --git a/cloud/dev/scripts/relay-same-cap-script-census.test.mjs b/cloud/dev/scripts/relay-same-cap-script-census.test.mjs index 9562faab563..b4f8780b291 100644 --- a/cloud/dev/scripts/relay-same-cap-script-census.test.mjs +++ b/cloud/dev/scripts/relay-same-cap-script-census.test.mjs @@ -718,6 +718,77 @@ describe('same-cap roll scripts accept every same-cap cell', () => { assert.equal(apply.split('wait-until "${MIG_NAME}" --stable').length, 3) }) + // One validator verdict decides three different outcomes. Run the predicates the job ships + // rather than restating them, because restating them is how the two drift apart. + it('decides refuse, apply, or skip on resume from the shipped predicate', () => { + const step = workflow.slice( + workflow.indexOf('- name: Require converged Terraform state and a stable MIG on resume'), + workflow.indexOf('- name: Apply only the selected same-cap template and MIG') + ) + const accept = /jq -e '(\.changes == 2\n[\s\S]*?)' \\\n\s+<<< "\$\{RESUME_REVIEW\}"/.exec(step) + assert.notEqual(accept, null, 'the resume step no longer gates on a validator verdict') + assert.match(step, /if test "\$\(jq -er '\.changes' <<< "\$\{RESUME_REVIEW\}"\)" = 0; then/) + const outcome = (review) => { + const resolved = spawnSync('bash', ['-euo', 'pipefail', '-c', [ + `RESUME_REVIEW=${JSON.stringify(JSON.stringify(review))}`, + `jq -e '${accept[1]}' <<< "\${RESUME_REVIEW}" >/dev/null || { echo refuse; exit 0; }`, + 'if test "$(jq -er \'.changes\' <<< "${RESUME_REVIEW}")" = 0', + 'then echo apply; else echo skip; fi' + ].join('\n')], { encoding: 'utf8' }) + assert.equal(resolved.status, 0, resolved.stderr) + return resolved.stdout.trim() + } + // Template and MIG converged, this cell's reviewed backend not: apply it here, or the cell + // keeps the 300-second drain and no request logging behind a green resume. + assert.equal( + outcome({ changes: 0, backendUpdate: ['connection_draining_timeout_sec', 'log_config.0'] }), + 'apply' + ) + assert.equal(outcome({ changes: 0, backendUpdate: ['log_config.0'] }), 'apply') + // Template-and-MIG drift still applies nothing, which is what a resume means. + assert.equal(outcome({ changes: 2 }), 'skip') + assert.equal( + outcome({ changes: 2, backendUpdate: ['connection_draining_timeout_sec'] }), + 'skip' + ) + // Anything the validator did not bound to this cell's reviewed change set fails the step. + assert.equal(outcome({ changes: 0 }), 'refuse') + assert.equal(outcome({ changes: 0, backendUpdate: [] }), 'refuse') + assert.equal(outcome({ changes: 1, backendUpdate: ['log_config.0'] }), 'refuse') + assert.equal(outcome({ changes: 3 }), 'refuse') + }) + + // The stranded path is the other reader of `changes`, and a pending backend update must not + // suppress the explicit MIG roll that is the only thing clearing a stranded cell's drain flag. + it('rolls a stranded MIG on the shipped predicate, backend update or not', () => { + const apply = workflow + .split('name: Apply only the selected same-cap template and MIG')[1] + .split('\n - id:')[0] + const condition = + /if test "\$\{ROLLBACK_STAGE\}" = stranded \\\n\s+(&& test "\$\(jq -er '\.changes' <<< "\$\{PLAN_REVIEW\}"\)" = 0); then/ + .exec(apply) + assert.notEqual(condition, null, 'the stranded roll no longer gates on the plan review') + const rolls = (stage, review) => { + const resolved = spawnSync('bash', ['-euo', 'pipefail', '-c', [ + `ROLLBACK_STAGE=${stage}`, + `PLAN_REVIEW=${JSON.stringify(JSON.stringify(review))}`, + `if test "\${ROLLBACK_STAGE}" = stranded \\\n ${condition[1]}; then`, + 'echo replace; else echo no-replace; fi' + ].join('\n')], { encoding: 'utf8' }) + assert.equal(resolved.status, 0, resolved.stderr) + return resolved.stdout.trim() + } + assert.equal(rolls('stranded', { changes: 0 }), 'replace') + assert.equal( + rolls('stranded', { changes: 0, backendUpdate: ['connection_draining_timeout_sec'] }), + 'replace' + ) + // A real template replacement already restarts the instance; rolling again would be a second. + assert.equal(rolls('stranded', { changes: 2 }), 'no-replace') + assert.equal(rolls('resume', { changes: 0 }), 'no-replace') + assert.equal(rolls('none', { changes: 0 }), 'no-replace') + }) + it('waits on the image a stranded cell actually serves', () => { const isolate = workflow .split('name: Reversibly isolate and drain only the selected cell')[1] diff --git a/cloud/dev/scripts/relay-same-cap-shadow-gate-verdict.mjs b/cloud/dev/scripts/relay-same-cap-shadow-gate-verdict.mjs index 859f423bdef..cf25b4ff9ff 100644 --- a/cloud/dev/scripts/relay-same-cap-shadow-gate-verdict.mjs +++ b/cloud/dev/scripts/relay-same-cap-shadow-gate-verdict.mjs @@ -37,7 +37,14 @@ export const SHADOW_GATE_THRESHOLDS = { // somewhere; this is how far back of the verify end it reaches instead. fallbackWindowMinutes: 30, // A read that stalls must not be allowed to spend the job's remaining minutes. - readTimeoutMs: 60_000 + readTimeoutMs: 60_000, + // Reads are serialised, so a failure mode that makes every read cost its full retry budget + // (an expired credential, a Logging 429 storm) scales with the window, not with one read. + // Past this the gate stops reading and reports the rest unverified, which is a verdict; the + // step's own timeout-minutes sits above it and exists only for a hung process. Set well clear + // of a healthy gate's own serial read time, or ordinary days report unverified tails and the + // shadow roll stops measuring the thing it exists to measure. Raise both bounds together. + overallDeadlineMs: 420_000 } const MINUTE_MS = 60_000 diff --git a/cloud/dev/scripts/relay-same-cap-shadow-gate.mjs b/cloud/dev/scripts/relay-same-cap-shadow-gate.mjs index e04c7ddc401..983e9761a26 100644 --- a/cloud/dev/scripts/relay-same-cap-shadow-gate.mjs +++ b/cloud/dev/scripts/relay-same-cap-shadow-gate.mjs @@ -36,9 +36,10 @@ const CELL_HOST = /^c[1-9][0-9]*\.relay\.onorca\.dev$/ const PROJECT_ID = /^[a-z][a-z0-9-]{4,28}[a-z0-9]$/ const SERVICE_NAME = /^[a-z][a-z0-9-]{0,62}$/ -const READ_ATTEMPTS = 3 +export const READ_ATTEMPTS = 3 const READ_RETRY_DELAY_MS = 5000 const READ_TIMEOUT_MS = SHADOW_GATE_THRESHOLDS.readTimeoutMs +const OVERALL_DEADLINE_MS = SHADOW_GATE_THRESHOLDS.overallDeadlineMs // json(timestamp) over a busy minute is a few hundred KB; leave room for the widest sub-window. const READ_MAX_BUFFER_BYTES = 256 * 1024 * 1024 @@ -92,8 +93,15 @@ async function readLogEntries(reader, { filter, projection, limit = ENTRY_LIMIT ] let lastError for (let attempt = 1; attempt <= READ_ATTEMPTS; attempt += 1) { + // Every remaining read short-circuits once the budget is gone, so the gate always reaches a + // verdict instead of being killed part-way through with nothing written. + const remainingMs = reader.deadlineAt - reader.now() + if (remainingMs <= 0) { + return { entries: [], failed: true, error: 'shadow gate read deadline exceeded' } + } try { - const { stdout } = await reader.runGcloud(args, { timeoutMs: reader.readTimeoutMs }) + const timeoutMs = Math.min(reader.readTimeoutMs, remainingMs) + const { stdout } = await reader.runGcloud(args, { timeoutMs }) return { entries: JSON.parse(stdout || '[]'), failed: false } } catch (error) { lastError = error @@ -232,9 +240,18 @@ async function readCloudSqlFatal(reader, { window }) { export async function evaluateShadowGate(config, { runGcloud, retryDelayMs = READ_RETRY_DELAY_MS, - readTimeoutMs = READ_TIMEOUT_MS + readTimeoutMs = READ_TIMEOUT_MS, + overallDeadlineMs = OVERALL_DEADLINE_MS, + now = Date.now }) { - const reader = { runGcloud, retryDelayMs, readTimeoutMs, projectId: config.projectId } + const reader = { + runGcloud, + retryDelayMs, + readTimeoutMs, + now, + deadlineAt: now() + overallDeadlineMs, + projectId: config.projectId + } const window = resolveWindow(config) // Everything this roll's instance logged, from the moment the apply could first restart it. const searchFrom = config.applyStartedAt diff --git a/cloud/dev/scripts/relay-same-cap-shadow-gate.test.mjs b/cloud/dev/scripts/relay-same-cap-shadow-gate.test.mjs index ab2eba57816..0d5ad239446 100644 --- a/cloud/dev/scripts/relay-same-cap-shadow-gate.test.mjs +++ b/cloud/dev/scripts/relay-same-cap-shadow-gate.test.mjs @@ -2,6 +2,7 @@ import assert from 'node:assert/strict' import { test } from 'node:test' import { readRelayWorkflow } from './relay-repository.mjs' import { + READ_ATTEMPTS, evaluateShadowGate, parseShadowGateArguments } from './relay-same-cap-shadow-gate.mjs' @@ -438,6 +439,39 @@ test('every read is given a bounded timeout, and a timed-out read is just a fail assert.equal(timedOut.verdict, 'WARN') }) +// The reads are serialised, so the cost of a failure that makes every one of them spend its full +// retry budget scales with the window. The deadline is what turns that into a verdict rather than +// a cancelled job, which would take every later cell in the wave with it. +test('the gate stops reading at its own deadline and still reports a verdict', async () => { + const seam = gcloudSeam() + // A clock where every read costs its whole retry budget, which is the case the deadline exists + // for: an expired credential or a Logging 429 storm answers nothing, slowly, every time. + let elapsedMs = 0 + const report = await evaluateShadowGate(parseShadowGateArguments(ARGV), { + ...seam, + now: () => { + elapsedMs += SHADOW_GATE_THRESHOLDS.readTimeoutMs * READ_ATTEMPTS + return elapsedMs + } + }) + // Everything past the deadline is skipped rather than attempted, so the gate cannot outlive it. + assert.ok(seam.calls.length > 0, 'the gate must still attempt reads inside its budget') + assert.ok( + seam.calls.length * SHADOW_GATE_THRESHOLDS.readTimeoutMs * READ_ATTEMPTS <= + SHADOW_GATE_THRESHOLDS.overallDeadlineMs, + 'the gate read past its own deadline' + ) + // A verdict, not a crash: a skipped read is an unverified check, which can never read as PASS. + assert.equal(report.reportOnly, true) + assert.equal(report.verdict, 'WARN') + assert.equal(report.checks.cellServing.status, 'unverified') + // No read is ever given more time than the budget still has left. + for (const { options } of seam.calls) { + assert.ok(options.timeoutMs > 0) + assert.ok(options.timeoutMs <= SHADOW_GATE_THRESHOLDS.readTimeoutMs) + } +}) + test('the job runs the gate report-only, after verification, and uploads its artifact', () => { const workflow = readRelayWorkflow('deploy-relay-production-same-cap-job.yml') const gate = workflow.slice(workflow.indexOf('- name: Shadow health gate (report only)')) @@ -445,6 +479,20 @@ test('the job runs the gate report-only, after verification, and uploads its art // Two independent guarantees that no verdict can fail a cell: the step's own exit code and this. assert.match(gate.slice(0, gate.indexOf('run:')), /continue-on-error: true/) assert.match(gate, /relay-same-cap-shadow-gate\.mjs/) + // The gate and its upload must be bounded in time as well as in outcome: a step that runs past + // the job's timeout-minutes gets the job cancelled, and cancellation stops the whole wave. + const gateHeader = gate.slice(0, gate.indexOf('run:')) + assert.match(gateHeader, /timeout-minutes: (\d+)/) + const stepTimeoutMinutes = Number(/timeout-minutes: (\d+)/.exec(gateHeader)[1]) + assert.equal(stepTimeoutMinutes, 8) + // The script has to settle on its own before the runner kills it, or the artifact is never + // written and the step reports nothing at all. + assert.ok( + SHADOW_GATE_THRESHOLDS.overallDeadlineMs < stepTimeoutMinutes * 60_000, + 'the gate deadline must leave the step time to write its verdict' + ) + const upload = workflow.slice(workflow.indexOf('- name: Publish the shadow health gate verdict')) + assert.match(upload.slice(0, upload.indexOf('uses:')), /timeout-minutes: 2/) assert.match( workflow, /name: relay-same-cap-shadow-gate-\$\{\{ inputs\.target-cell-id \}\}-\$\{\{ github\.run_id \}\}\.json/