Make PMM-T1087's postgres-db drop deterministic - #1293
Conversation
The `postgres` database on postgresnodb was dropped by a fire-and-forget one-shot container: no retry, no verification, and `docker compose up -d` never sees its exit code. When the drop does not take effect, PMM correctly accepts the remote service and PMM-T1087 fails ~12 minutes later with a message that reads like a product regression. Retry the drop until it is confirmed, and assert the state from db_setup.sh so a failure fails the setup step instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PCsB6xKBJ8gxnjJUKmRVjV Signed-off-by: travagliad <davi.travaglia@percona.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe PostgreSQL default database removal logic now runs from a mounted Sequence Diagram(s)sequenceDiagram
participant db_setup.sh
participant verify_no_default_db.sh
participant postgresremovedefaultdb
participant drop_default_db.sh
participant postgresnodb
db_setup.sh->>verify_no_default_db.sh: start verification
verify_no_default_db.sh->>postgresremovedefaultdb: wait for completion
postgresremovedefaultdb->>drop_default_db.sh: execute mounted script
drop_default_db.sh->>postgresnodb: force-drop postgres database
verify_no_default_db.sh->>postgresnodb: query pg_database
postgresnodb-->>verify_no_default_db.sh: return database state
Merge Risk: ⚪ Minimal · up to The change makes the test database setup deterministic and fails early when the required precondition is not met; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review follow-up: inline the literals the compose file already declares, set PGCONNECT_TIMEOUT and bound `docker wait` so a black-holed postgresnodb fails within a known budget instead of hanging the setup step, propagate the assertion's exit status explicitly from db_setup.sh, and trim the comments to the invariant. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PCsB6xKBJ8gxnjJUKmRVjV Signed-off-by: travagliad <davi.travaglia@percona.com>
|
It failed in That is a publish/mirror-sync race in Percona's own Everything relevant to this PR is green on the same commit: the 29 other jobs passed, Generated by Claude Code |
Failures fixed (investigator)
E2E / Instances UI tests / e2e tests: @fb-instancescodeceptjs-e2e/tests/verifyRemoteInstances_test.js:316/@fb-instances— PMM-T1087 Verify adding PostgreSQL remote instance without postgres databaseWhat failed
The only red job of 30 (Launchable gate:
1 actionable, 0 quarantined; 31 passed, 1 failed, 15 skipped), and it failed both attempts ofFeature(...).retry(1):The test points a remote PostgreSQL service at
postgresnodbwith databasepostgresand expects PMM's pre-add connection check to reject it. Instead the service was added.
Root cause — the QA setup, not the product
postgresnodbhad apostgresdatabase in that run. From the run's ownpmm-managed.log:The connection check succeeded,
pg_databaselistedpostgres, and the service'spostgres_exporterlog has no connection errors. PMM did the right thing.That database is supposed to be removed during setup by the one-shot
postgresremovedefaultdbcontainer:One attempt, no verification — and
docker compose up -dnever looks at its exitcode, so when that
psqlfails the run continues with the wrong precondition and thetest fails ~12 minutes later with a message that reads like a product regression.
Reproduced
On a throwaway Linode VM running the FB server image
perconalab/pmm-server-fb:PR-4543-018d0e8,AddServiceagainst the samepostgresnodbcontainer, same payload the UI sends:postgresdatabaseHTTP 400—Connection check failed: pq: database "postgres" does not exist (3D000).200— service created, exactly the toast the test sawSo the outcome is entirely determined by whether the setup dropped that database;
the product behaves correctly in both cases.
The drop itself succeeded 6/6 on an idle box, so the original
psqlfailure wastransient and is not captured in the CI logs. What is reproducible is the failure
mode — the old one-shot dies on the first hiccup and says nothing:
Fix
drop_default_db.sh(run by the one-shot) retries the drop until it is confirmedgone via
pg_database, so a transient resolve/connect error self-heals. Verifiedon the VM: with
postgresnodbdown when the one-shot starts, it recovers on attempt 6and exits 0 — the old command would have exited non-zero at attempt 1 and left the
database in place.
verify_no_default_db.sh, called fromdb_setup.sh, asserts the precondition on thehost (and surfaces the one-shot's exit code and logs). Verified both ways: exit 0 in
the good state, exit 1 with
postgresnodb still has a postgres database …afterre-creating it.
PGCONNECT_TIMEOUT,timeoutarounddocker waitandthe query), so an unreachable
postgresnodbfails within a known budget instead ofhanging the setup step — see the review thread.
The assertion in the test is untouched — the
postgresdatabase really must be absentfor T1087 to mean anything, so the setup is what had to become reliable.
CI on this branch
In
E2E tests Matrixrun 33650775022 oncdc7d9d, thejob
FB E2E tests / Instances UI tests / e2e tests: @fb-instancescompletedsuccessfully (16:27:25 UTC) — 35 tests found, 35 passed, 0 failed at
LAUNCHABLE_CONFIDENCE: 100%, so the whole spec including T1087 ran, withSetup PMM Server(which now runs the assertion) green. That is the end-to-endconfirmation the suite itself can give. The run as a whole reads
cancelledbecause the follow-up commit
c6c8ab7superseded it while three unrelated jobswere still going; the Instances job had already finished green by then.
c6c8ab7(timeouts, inlined literals, explicit exit propagation) is re-running as33655528833.
🤖 Generated with Claude Code
https://claude.ai/code/session_01PCsB6xKBJ8gxnjJUKmRVjV