Refuse to persist an empty control-plane snapshot in rad shutdown - #12849
Refuse to persist an empty control-plane snapshot in rad shutdown#12849pujitha24 wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens rad shutdown so it will not overwrite the durable state archive with an obviously-degenerate control-plane snapshot (specifically: a ucp.sql dump that has zero rows in the resources table). This adds defense-in-depth beyond workflow-level guards by enforcing the invariant inside the command itself.
Changes:
- Add
pgbackup.IsControlPlaneEmpty(stateDir)to detect aucp.sqldump with noresourcesrows (or noresourcesCOPY block). - Update
rad shutdownto bail out before Terraform backup / archive commit when the control-plane snapshot is empty. - Add unit tests for the dump-inspection helper and a shutdown-flow test asserting no commit occurs on empty snapshots.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/cli/pgbackup/pgbackup.go | Adds IsControlPlaneEmpty with a pg_dump COPY-header match for resources and an empty-dump check. |
| pkg/cli/pgbackup/pgbackup_test.go | Adds unit tests covering empty/no-table/non-empty/missing-file dump scenarios. |
| pkg/cli/cmd/shutdown/shutdown.go | Calls IsControlPlaneEmpty after DB backup and returns a user-facing error before committing when empty. |
| pkg/cli/cmd/shutdown/shutdown_test.go | Updates the DB-backup fake to write a realistic ucp.sql fixture and adds a regression test ensuring empty backups stop before commit. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| func IsControlPlaneEmpty(stateDir string) (bool, error) { | ||
| path := filepath.Join(stateDir, "ucp.sql") | ||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| return false, fmt.Errorf("failed to read backup file %q: %w", path, err) | ||
| } |
2004264 to
f27b564
Compare
Motivation: The repo-based deploy pipeline restores durable state with `rad startup` and persists it with `rad shutdown`. A prior fix gated `rad shutdown` on a `state-restored` signal so it only runs after `rad startup` succeeded earlier in the same run -- but that only proves startup ran, not that the control plane is still healthy by the time shutdown runs. If it degrades afterward (a PostgreSQL pod crash-loop, or `rad install` re-run mid-run), `rad shutdown` would still dump and commit an empty database, corrupting the shared archive for every future run. Approach: Add pgbackup.IsControlPlaneEmpty, which inspects the backed-up ucp.sql dump for the pg_dump COPY block of the "resources" table (the table that stores every UCP resource, including resource groups) and reports whether it has zero data rows, or the table is missing entirely. rad shutdown calls this after backing up the databases and before backing up Terraform state or committing to the archive; when the backup is empty it returns a clear clierrors.Message and leaves the existing archive untouched. This is defense-in-depth inside the command itself, independent of the workflow-level guard. Validation: go test ./pkg/cli/pgbackup/... ./pkg/cli/cmd/shutdown/... -v passes, including new tests for IsControlPlaneEmpty (no data rows, missing table, populated, missing file) and Test_Run_EmptyControlPlaneStopsBeforeCommit (asserts Run stops before BackupTerraform/Commit on an empty backup). Reverting only the two source files while keeping the test files causes a build failure and an unexpected-Commit-call test failure, confirming the new check is what prevents the commit. go vet and gofmt are clean, and golangci-lint (pinned repo version v2.13.1) reports 0 issues on both changed packages. The COPY-block parsing was additionally verified against a real, locally-run PostgreSQL 16 instance with the exact "resources" schema this codebase writes, using genuine pg_dump output for both an empty and a populated table; this environment has no live Radius cluster, so the change was not exercised end-to-end through a real control plane. Report: radius-project#12847 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5 (via Claude Code)
722db0f to
48c01de
Compare
|
The validation is thorough and the approach is sound. A couple of suggestions to strengthen the PR further:
This is an optional enhancement; the PR is solid as-is. The test fixtures are realistic and accurately represent real |
Summary
rad shutdownnow refuses to commit a control-plane backup to the durablestate archive when the backed-up
ucpdatabase contains zero rows in theresourcestable (the table that stores every UCP resource, includingresource groups). It bails out with a clear error and leaves the existing
archive untouched, instead of silently persisting the degenerate snapshot.
Reason for change
The repo-based deploy pipeline restores durable state with
rad startupandpersists it with
rad shutdown. A prior fix (#12840) added a workflow-levelguard so
rad shutdownonly runs whenrad startupactually restored stateearlier in the same run — but that guard only proves
rad startupran, notthat the control plane is still healthy by the time
rad shutdownruns. Ifthe control plane degrades after a successful startup (e.g. the PostgreSQL
pod crash-loops, or
rad installis re-run mid-pipeline),rad shutdownwould still dump and commit an empty database, corrupting the shared archive
for every future run. This is defense-in-depth inside the command itself, so
the archive is protected regardless of how
rad shutdownis invoked.Fixes #
How to test
All tests pass, including:
pkg/cli/pgbackup/pgbackup_test.go:Test_IsControlPlaneEmpty_*— unittests for the new
IsControlPlaneEmptyhelper against hand-builtCOPY ... FROM stdin;fixtures (no data rows, noresourcestable at all,data rows present, missing file).
pkg/cli/cmd/shutdown/shutdown_test.go:Test_Run_EmptyControlPlaneStopsBeforeCommit— asserts
Runreturns an error and never reachesBackupTerraformorsession.Commitwhen the database backup is empty.Validation
go build ./pkg/cli/pgbackup/... ./pkg/cli/cmd/shutdown/...passes.go test ./pkg/cli/pgbackup/... ./pkg/cli/cmd/shutdown/... -vpasses (allcases, old and new).
pgbackup.go,shutdown.go) reverted and the test files left in place, the package failsto build (
undefined: IsControlPlaneEmpty) and the new shutdown test failsbecause
Commitis called unexpectedly — confirming the added check iswhat prevents the commit.
go vet ./pkg/cli/pgbackup/... ./pkg/cli/cmd/shutdown/...is clean;gofmt -lreports no formatting issues on the changed files.golangci-lint run(pinned repo version v2.13.1, frombuild/tools.yaml)on both changed packages reports 0 issues.
IsControlPlaneEmptyparsing logic (matching thepg_dumpplain-formatCOPY <table> (<cols>) FROM stdin;... rows ...\.block) was verifiedagainst a real, locally-run PostgreSQL 16 instance: a
resourcestable wascreated with the exact schema this codebase writes
(
pkg/components/database/postgres/postgresclient.go), and realpg_dump --data-onlyoutput was captured for both an empty table and apopulated one (including a row with escaped backslashes/tabs), then fed
through the function directly — both cases classified correctly. This
environment has no live Radius cluster or
kubectl, so the change was notexercised end-to-end through
rad shutdownagainst a real control plane;the above is the closest available proxy for that.
upstream/main(commit745ce9cc0), andupstream/main's own CI (Unit Tests, CodeQL) is green as of this writing.File change summary
pkg/cli/pgbackup/pgbackup.goIsControlPlaneEmpty(stateDir), which detects aucp.sqldump with zero rows in theresourcestable (or no such table at all).pkg/cli/pgbackup/pgbackup_test.goIsControlPlaneEmptycovering empty, missing-table, populated, and missing-file cases.pkg/cli/cmd/shutdown/shutdown.goIsControlPlaneEmptyafter the database backup and before committing; refuse to persist and return a clear error when the backup is empty.pkg/cli/cmd/shutdown/shutdown_test.gofakeStateBackupClient.BackupDatabaseswrite a realisticucp.sqlfixture; addTest_Run_EmptyControlPlaneStopsBeforeCommit.Fixes #12847