fix(ci): break orphaned TF state lock before UAT teardown - #2049
Conversation
njhensley
added
the
theme/ci-dx
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info���️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe AWS UAT workflow documents that its S3 backend does not use state locking. The Azure UAT workflow retrieves the storage account key and breaks stale state-blob leases before cluster destruction. The GCP UAT workflow removes the deployment’s Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/workflows/uat-azure.yaml:
- Around line 903-912: Update the stale-lease cleanup block around STATE_KEY and
the az storage blob lease break command to stop retrieving or passing the
storage account key on the command line. Authenticate the blob operation with
--auth-mode login instead, ensuring the workflow identity has only the required
Blob lease permissions.
In @.github/workflows/uat-gcp.yaml:
- Around line 703-707: Update the cleanup logic in
.github/workflows/uat-gcp.yaml lines 703-707 to treat only a genuinely missing
default.tflock object as a successful no-op; preserve and retry or fail on
authentication, permission, network, and wrong-URI errors instead of reporting
them as “no lock.” Update the corresponding cleanup flow in
.github/workflows/uat-azure.yaml lines 906-920 to distinguish missing
account/blob/lease results from account lookup or key retrieval failures, and
remove any || true behavior that suppresses operational errors before the
destroy loop.
🪄 Autofix
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: 4b17f744-ee37-417a-a3f7-5b7a4fbde0d5
📒 Files selected for processing (3)
.github/workflows/uat-aws.yaml.github/workflows/uat-azure.yaml.github/workflows/uat-gcp.yaml
9f08563 to
e42b4eb
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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/workflows/uat-azure.yaml:
- Around line 945-947: The teardown command using az storage blob lease break
must not expose AZURE_STORAGE_KEY through the process environment. Update the
teardown flow around the lease-break invocation to authenticate via --auth-mode
login with narrowly scoped Blob permissions, or move the operation behind a
trusted teardown boundary, while preserving the existing account, container,
blob, error-file, and return-code behavior.
🪄 Autofix
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: 3ed0594c-cf0f-4a54-a064-968d94ead9c8
📒 Files selected for processing (3)
.github/workflows/uat-aws.yaml.github/workflows/uat-azure.yaml.github/workflows/uat-gcp.yaml
A UAT run cancelled mid-bringup SIGKILLs the actuator's terraform apply before it can release the backend state lock, orphaning it. The always()-gated Destroy Cluster step then can never acquire the lock, so all 3 destroy attempts fail identically and the GPU node leaks (observed on GKE run 30948013962: 'destroy failed after 3 attempts'). The Bringup container has already exited by the time the teardown step runs, so any lock present has no live holder and is safe to break. Clear it at the top of each destroy attempt, per backend: - GCP (GCS): delete the default.tflock object. HTTP 412 on acquire. - Azure (azurerm): break the infinite blob lease on the state blob. The account key reaches az via AZURE_STORAGE_KEY rather than --account-key so it stays out of argv (world-readable through /proc/<pid>/cmdline), and xtrace is off across the helper so it never hits the build log. --auth-mode login is not usable: the pipeline identity holds subscription Contributor, which grants listKeys but no data-plane blob role, and the ABAC condition on its RBAC Administrator grant forbids self-assigning one. - AWS (S3): no change — the EKS actuator's -backend-config sets no dynamodb_table / use_lockfile, so the backend does no state locking and cannot orphan a lock. Documented inline for future readers. Only a genuinely-absent lock (GCS) or an unheld lease / never-written state blob (azurerm) counts as the happy path; auth, permission, and network failures are surfaced as ::warning:: instead of being collapsed into "no lock", and retry along with the destroy attempt they precede. Cleanup is deliberately non-fatal — aborting the step would skip the destroy entirely and guarantee the leak it exists to prevent, so the loop's terminal exit 1 stays the gate on a real teardown failure. Backend mechanics confirmed by inspecting the pinned actuator images' terraform backend.tf and -backend-config flags. Signed-off-by: Nathan Hensley <nhensley@nvidia.com>
e42b4eb to
0cd7577
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
APPROVE - 0 blockers at head 0cd7577.
Reviewed the stale-Terraform-lock break added to the GCP and Azure teardowns, plus the AWS "deliberately none" note.
An earlier head (e42b4eb) failed tests/uat two ways, both of which this head fixes:
- Azure: the
az storage account list/az storage account keys listinvocations split a quoted--queryacross a\continuation, whichstripShellComment(run per physical line) read as an unterminated quote. Now one line each. - GCP:
not ?foundin thegrep -qiEpattern trippedhasUnsupportedShellGlob, andshellEnablesXtracefails closed, so the credential-bearing step was reported as enabling xtrace. Nownot found|notfound.
Verified locally against this head: go test ./tests/uat/ passes with these workflows overlaid on main. CI is green on 0cd7577.
Spot-checked the safety argument: both breaks run only under always() after the Bringup container has exited, and every UAT run serializes on the per-reservation lease owned by uat-run.yaml, so there is no live lock holder to race. The Azure account key never reaches argv or the xtrace log (AZURE_STORAGE_KEY plus set +x around the helper), and every non-happy-path outcome is a ::warning:: rather than a swallowed || true.
Merge-ordering note: this touches the same Destroy Cluster steps as #2042, so expect a conflict depending on which lands first.
2 participants
Summary
Break the orphaned Terraform state lock/lease before the UAT
Destroy Clusterretry loop, so a run cancelled mid-bringup can still tear down its cluster instead of leaking the GPU node.Motivation / Context
GKE UAT run 30948013962 was cancelled mid-bringup and then failed teardown with
Cluster aicr-30948013962 destroy failed after 3 attempts; GPU node may be leaking, leaving a 5-node cluster + 9 VPC networks running (cleaned up manually).Root cause: GitHub cancels a job by SIGKILLing the running step's process tree after a ~7.5s grace period. The Bringup Infra step's
terraform apply(a multi-minute GKE/AKS provision inside the actuator container) is killed before Terraform can release its backend state lock, orphaning it. Thealways()-gated Destroy Cluster step then can never acquire the lock — and because a stale lock is persistent, all 3 retry attempts fail identically. The retry loop is designed for transient failures and cannot recover here.Fix: by the time the teardown step runs, the Bringup container has already exited, so any lock present has no live holder and is safe to break. Clear it best-effort before the retry loop.
Fixes: N/A
Related: run 30948013962
Type of Change
Component(s) Affected
.github/workflows/uat-{gcp,azure,aws}.yaml)Implementation Notes
Handling differs per cloud because the backend lock mechanism differs — verified by inspecting the pinned actuator images'
terraform/backend.tfand the-backend-configflags baked into eachclusterbinary:default.tflockobject lock; acquire fails with HTTP 412conditionNotMet. Fix:gcloud storage rm …/default.tflock(teardown SA already owns the state bucket).azurermbackend): holds an infinite blob lease on the state blob (always on — not opt-in). Fix:az storage blob lease breakondeployments/<location>/<id>/terraform.tfstatein theclst<sub-hex>account, using the account key (the actuator's backend authenticates the same way).xtraceis disabled around the key so it never lands in the build log.-backend-configsets nodynamodb_tableand nouse_lockfile, so the backend does no state locking and cannot orphan a lock. No code change — documented inline so the asymmetry is intentional, with a pointer to add an equivalent break if the actuator ever enables S3 locking.All three breaks are best-effort no-ops on the happy path (the lock/lease is absent once a successful apply releases it) and are scoped to this run's own
deployment.id, so they cannot disturb another run's state.Testing
yamllint .github/workflows/uat-{gcp,azure,aws}.yaml # clean actionlint .github/workflows/uat-{gcp,azure,aws}.yaml # no new findings (pre-existing info-level SC2016 only) shellcheck -x <extracted Azure block> # cleanThe GCP path is validated end-to-end: this exact
gcloud storage rm …/default.tflock+ actuator re-destroy sequence is what recovered the leaked cluster from run 30948013962 manually. The Azure lease-break is reasoned from the actuator's verified state layout/auth but has not yet been exercised against a live cancelled-mid-bringup Azure run.Risk Assessment
Rollout notes: N/A — CI-only workflow change.
Checklist
make lint— yamllint/actionlint clean on the edited steps)git commit -S)