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 bf31b0b2f55..46ffba9c027 100644 --- a/.github/workflows/cloud-deploy-relay-production-same-cap-job.yml +++ b/.github/workflows/cloud-deploy-relay-production-same-cap-job.yml @@ -598,16 +598,15 @@ jobs: # Zero resource changes prove the prior run's apply completed and no # restart will follow, keeping the incarnation check honest. Root # outputs may lag a targeted apply, so judge resource_changes only. - # The backend service is targeted too, so its reviewed drain timeout - # and request logging can be the only thing left here; neither - # restarts an instance, so the validator below clears that on its - # own, without the template-and-MIG pair. + # The cell backend service is deliberately not targeted: the capacity role has no + # compute.backendServices.update, so naming it fails the apply after the MIG has + # rolled. Its drain and log_config settings ship in a root apply by a principal + # that holds the permission. terraform -chdir=infra/terraform plan \ -var-file=environments/production.tfvars \ -var-file="${RUNNER_TEMP}/relay-same-cap.tfvars.json" \ "-target=google_compute_instance_template.relay_gce_cell[\"${TARGET_CELL_ID}\"]" \ "-target=google_compute_instance_group_manager.relay_gce_cell[\"${TARGET_CELL_ID}\"]" \ - "-target=google_compute_backend_service.relay_gce_cell[\"${TARGET_CELL_ID}\"]" \ -out="${RUNNER_TEMP}/relay-same-cap-resume.tfplan" if ! terraform -chdir=infra/terraform show -json \ "${RUNNER_TEMP}/relay-same-cap-resume.tfplan" \ @@ -644,21 +643,7 @@ jobs: --regional-rehome-protocol "${DESIRED_REHOME_PROTOCOL}" \ "${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 + jq -e '.changes == 2' <<< "${RESUME_REVIEW}" >/dev/null fi gcloud compute instance-groups managed wait-until "${MIG_NAME}" --stable \ --project "${GCP_PROJECT_ID}" --zone "${TARGET_ZONE}" --timeout 900 @@ -681,7 +666,6 @@ jobs: -var-file="${RUNNER_TEMP}/relay-same-cap.tfvars.json" \ "-target=google_compute_instance_template.relay_gce_cell[\"${TARGET_CELL_ID}\"]" \ "-target=google_compute_instance_group_manager.relay_gce_cell[\"${TARGET_CELL_ID}\"]" \ - "-target=google_compute_backend_service.relay_gce_cell[\"${TARGET_CELL_ID}\"]" \ -out="${RUNNER_TEMP}/relay-same-cap.tfplan" PLAN_REVIEW="$(terraform -chdir=infra/terraform show -json \ "${RUNNER_TEMP}/relay-same-cap.tfplan" \ diff --git a/cloud/dev/scripts/relay-regional-rehome-workflow.test.mjs b/cloud/dev/scripts/relay-regional-rehome-workflow.test.mjs index 2e2be1f7cc5..6704a67f5ae 100644 --- a/cloud/dev/scripts/relay-regional-rehome-workflow.test.mjs +++ b/cloud/dev/scripts/relay-regional-rehome-workflow.test.mjs @@ -56,9 +56,7 @@ 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 — and - // that last case then has to be applied, not waved through as converged. + // the validator requires, restricted to the template-and-MIG change pair. assert.match( job, /if ! terraform -chdir=infra\/terraform show -json[\s\S]{0,220}\| length == 0' >\/dev\/null\n then\n/ @@ -77,20 +75,15 @@ 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 {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/ + /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' <<< "\$\{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. + // A resume applies nothing at all, which is what a resume means: the only accepted + // unconverged plan is the template-and-MIG rollback-image drift, and it is left pending. 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.equal(resumeStep.split('terraform -chdir=infra/terraform apply').length, 1) 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 b4f8780b291..2ef86ed80b8 100644 --- a/cloud/dev/scripts/relay-same-cap-script-census.test.mjs +++ b/cloud/dev/scripts/relay-same-cap-script-census.test.mjs @@ -336,9 +336,10 @@ describe('same-cap roll scripts accept every same-cap cell', () => { assert.equal(workflow.split('POOL_ARGUMENTS=()').length, 3) }) - // One cell's whole serving path and nothing else: the template, the MIG bound to it, and the - // backend service, whose reviewed drain timeout would otherwise need a fleet-wide root apply. - it('targets exactly this cell template, MIG, and backend on every plan the job runs', () => { + // One cell's compute path and nothing else: the template and the MIG bound to it. The cell + // backend service stays out because the capacity role has no compute.backendServices.update, + // so naming it fails the apply after the MIG has already rolled. + it('targets exactly this cell template and MIG on every plan the job runs', () => { const plans = workflow.split('terraform -chdir=infra/terraform plan').slice(1) assert.equal(plans.length, 2) for (const plan of plans) { @@ -350,15 +351,18 @@ describe('same-cap roll scripts accept every same-cap cell', () => { .map(([, resource]) => resource), [ 'google_compute_instance_template.relay_gce_cell', - 'google_compute_instance_group_manager.relay_gce_cell', - 'google_compute_backend_service.relay_gce_cell' + 'google_compute_instance_group_manager.relay_gce_cell' ] ) - // Any target that is not one of those three, or not scoped to this cell, fails here. - assert.equal(call.split('-target=').length, 4) + // Any target that is not one of those two, or not scoped to this cell, fails here. + assert.equal(call.split('-target=').length, 3) } }) + it('never names a backend service on any plan or apply in the job', () => { + assert.equal(workflow.includes('google_compute_backend_service'), false) + }) + it('validates a correct plan for every wave cell at that cell\'s rehome protocol', () => { const trusted = SAME_CAP_CELLS.filter((cell) => REHOME_SOURCE_CELLS.has(cell)) // Only a declared rehome source may roll at a trusted protocol at all; the job refuses @@ -718,49 +722,37 @@ 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', () => { + // Run the predicate the job ships rather than restating it, because restating it is how the + // two drift apart. An unconverged resume accepts the template-and-MIG pair and nothing else, + // and it applies nothing: a backend change cannot reach this plan, which no longer targets one. + it('accepts only the template-and-MIG pair on an unconverged resume', () => { 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) + const accept = /jq -e '(\.changes == 2)' <<< "\$\{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/) + assert.equal(step.includes('terraform -chdir=infra/terraform apply'), false) 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' + 'echo accept' ].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: 2 }), 'accept') + // Anything the validator did not bound to the reviewed rollback-image drift 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: 0, backendUpdate: ['log_config.0'] }), 'refuse') + assert.equal(outcome({ changes: 1 }), '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', () => { + // The stranded cell's explicit MIG roll is the only thing that clears its drain flag, and + // `changes` is what decides it, so run the shipped predicate rather than restating it. + it('rolls a stranded MIG on the shipped predicate', () => { const apply = workflow .split('name: Apply only the selected same-cap template and MIG')[1] .split('\n - id:')[0] @@ -779,10 +771,6 @@ describe('same-cap roll scripts accept every same-cap cell', () => { 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') diff --git a/cloud/dev/scripts/validate-relay-capacity-plan.mjs b/cloud/dev/scripts/validate-relay-capacity-plan.mjs index 261a906b1fe..fc8660144c5 100644 --- a/cloud/dev/scripts/validate-relay-capacity-plan.mjs +++ b/cloud/dev/scripts/validate-relay-capacity-plan.mjs @@ -301,11 +301,12 @@ function requireDesiredStartupScript(script, config) { } } -// The same-cap job targets this cell's backend service so the two declared-but-unapplied -// settings land one cell at a time: an unindexed root plan pulls the whole MIG and template -// resources in as dependencies, which standing image drift turns into a 29-cell roll. Each -// attribute is optional because a cell that already has it plans no change for it. -// Splitting the backend out here keeps `changes` the template-and-MIG count both callers read. +// The same-cap job no longer targets this cell's backend service (the capacity role has no +// compute.backendServices.update), so a wave plan carries no backend change and this reports an +// empty list. It stays as the bound on any caller that does target one: exactly this cell's +// backend, exactly the reviewed drain and request-logging attributes, each optional because a +// cell that already has one plans no change for it. Splitting the backend out keeps `changes` +// the template-and-MIG count both callers read. function takeCellBackendUpdate(changes, config) { const backends = changes.filter( ({ address }) => typeof address === 'string' && address.startsWith(`${CELL_BACKEND_RESOURCE}[`)