mirror of
https://github.com/stablyai/orca.git
synced 2026-09-21 08:02:32 +00:00
chore(relay): bound the shadow health gate and apply a pending backend update on resume (#21865)
* fix(relay): bound the same-cap shadow gate and apply a resumed backend update Two findings both adversarial reviews of tonight's merged set agree on. The report-only shadow health gate (#21849) had `continue-on-error: true` but no step timeout. That bounds the step's contribution to the job outcome, not its clock. Its reads are serialised, and a failure that answers nothing slowly — an expired credential, a project-wide Logging 429 storm — makes every read cost its full 3 x 60 s retry budget, so the cost scales with the roll window: roughly 8S + 2 reads for S ten-minute sub-windows. A 40-minute window is about 34 reads, or 108 minutes, against the job's `timeout-minutes: 75`. A cancelled job cannot be absorbed by continue-on-error, fires the failure-gated cleanup isolation on an already-restored cell, and stops the strict next-cell chain. Give the step `timeout-minutes: 5` and the artifact upload `timeout-minutes: 2`. A timed-out step is a failed step, which continue-on-error covers, so the job stays green. Inside the script, stop reading after an overall four-minute deadline and report the remaining checks unverified, so the normal outcome is a written verdict rather than a killed process; the step timeout is then only for a hung process. The census test pins both timeouts and that the deadline leaves the step time to write its verdict. The resume branch (#21860) accepted `changes == 0` with a non-empty `backendUpdate` as complete and applied nothing, so a resumed cell silently kept the 300-second drain and no request logging behind a green resume. That shape means the template and MIG are converged and only this cell's reviewed backend update is left, so apply the saved resume plan — the validator has already bounded it to this cell's backend and neither attribute restarts an instance — then continue as converged. Template-and-MIG drift still applies nothing, which is what a resume means, and a stranded cell's explicit MIG replace is unchanged. Claude-Session: relay-same-cap-gate-timeout-and-resume * fix(relay): raise the shadow gate bounds clear of a healthy gate's read time A healthy gate is already minutes of serial reads on the 2-vcpu runner, so a four-minute deadline would report unverified tails on ordinary days and stop the shadow roll measuring the comparison it exists for. Raise both together: the step to eight minutes and the script's own deadline to seven, keeping the census pin that the deadline leaves the step room to write its verdict. The job budget is unaffected: a ~14-minute cell plus eight is well inside 75. Claude-Session: relay-same-cap-gate-timeout-and-resume
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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/)
|
||||
|
||||
@@ -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]
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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/
|
||||
|
||||
Reference in New Issue
Block a user