fix(tests): detect PG graceful switchover at PostgreSQL level - #100
Conversation
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change updates PostgreSQL functional switchover coverage, persistent test storage, and PostgreSQL initialization. The test now uses direct database-state assertions, reclones and validates streaming replicas, dynamically resolves cluster identity, and restores standby state after round-trip takeover. ChangesPostgreSQL graceful switchover
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TestScript
participant PostgreSQL
participant Orchestrator
TestScript->>PostgreSQL: Verify promotion and read-only state
TestScript->>PostgreSQL: Recl on pgprimary from pgstandby1
PostgreSQL-->>TestScript: Report streaming readiness
TestScript->>Orchestrator: Discover NEW_CLUSTER
TestScript->>Orchestrator: Run graceful-master-takeover-auto
TestScript->>PostgreSQL: Verify final promotion
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the PostgreSQL functional tests to verify switchover success through direct database queries instead of relying on Orchestrator's cluster view, which can be inconsistent during graceful takeovers. It also updates the topology re-unification logic to dynamically track cluster names. The review feedback suggests initializing variables to ensure compatibility with shell safety flags and optimizing performance by reducing redundant API calls within polling loops.
|
|
||
| # Verify orchestrator sees pgstandby1 as primary and pgprimary as standby | ||
| TOPOLOGY_OK=false | ||
| NEW_CLUSTER="" |
There was a problem hiding this comment.
It is good practice to initialize all variables used in comparisons, especially since set -u is enabled at the beginning of the script. While PRIMARY_CLUSTER is assigned inside the loop, initializing it here alongside NEW_CLUSTER improves clarity and safety.
| NEW_CLUSTER="" | |
| NEW_CLUSTER="" | |
| PRIMARY_CLUSTER="" |
| NEW_CLUSTER=$(curl -s --max-time 10 "$ORC_URL/api/all-instances" 2>/dev/null | python3 -c " | ||
| import json, sys | ||
| for inst in json.load(sys.stdin): | ||
| if not inst.get('ReadOnly', True): | ||
| print(inst['Key']['Hostname']) | ||
| if inst['Key']['Hostname'] == '172.30.0.21': | ||
| print(inst.get('ClusterName', '')) | ||
| sys.exit(0) | ||
| " 2>/dev/null || echo "") | ||
| if [ "$PRIMARY_HOST" = "172.30.0.21" ] || [ "$PRIMARY_HOST" = "pgstandby1" ]; then | ||
| TOPOLOGY_OK=true | ||
| # Verify pgprimary (172.30.0.20) joined the same cluster as pgstandby1 | ||
| PRIMARY_CLUSTER=$(curl -s --max-time 10 "$ORC_URL/api/all-instances" 2>/dev/null | python3 -c " | ||
| import json, sys | ||
| for inst in json.load(sys.stdin): | ||
| if inst['Key']['Hostname'] == '172.30.0.20': | ||
| print(inst.get('ClusterName', '')) | ||
| sys.exit(0) | ||
| " 2>/dev/null || echo "") |
There was a problem hiding this comment.
The script calls the /api/all-instances endpoint twice within each iteration of the loop. Since this endpoint returns data for all instances, it is more efficient to fetch the JSON once and then parse it for both hostnames. This reduces the number of API calls and potential network overhead during the test.
| NEW_CLUSTER=$(curl -s --max-time 10 "$ORC_URL/api/all-instances" 2>/dev/null | python3 -c " | |
| import json, sys | |
| for inst in json.load(sys.stdin): | |
| if not inst.get('ReadOnly', True): | |
| print(inst['Key']['Hostname']) | |
| if inst['Key']['Hostname'] == '172.30.0.21': | |
| print(inst.get('ClusterName', '')) | |
| sys.exit(0) | |
| " 2>/dev/null || echo "") | |
| if [ "$PRIMARY_HOST" = "172.30.0.21" ] || [ "$PRIMARY_HOST" = "pgstandby1" ]; then | |
| TOPOLOGY_OK=true | |
| # Verify pgprimary (172.30.0.20) joined the same cluster as pgstandby1 | |
| PRIMARY_CLUSTER=$(curl -s --max-time 10 "$ORC_URL/api/all-instances" 2>/dev/null | python3 -c " | |
| import json, sys | |
| for inst in json.load(sys.stdin): | |
| if inst['Key']['Hostname'] == '172.30.0.20': | |
| print(inst.get('ClusterName', '')) | |
| sys.exit(0) | |
| " 2>/dev/null || echo "") | |
| ALL_INSTANCES=$(curl -s --max-time 10 "$ORC_URL/api/all-instances" 2>/dev/null) | |
| NEW_CLUSTER=$(echo "$ALL_INSTANCES" | python3 -c " | |
| import json, sys | |
| for inst in json.load(sys.stdin): | |
| if inst['Key']['Hostname'] == '172.30.0.21': | |
| print(inst.get('ClusterName', '')) | |
| sys.exit(0) | |
| " 2>/dev/null || echo "") | |
| # Verify pgprimary (172.30.0.20) joined the same cluster as pgstandby1 | |
| PRIMARY_CLUSTER=$(echo "$ALL_INSTANCES" | python3 -c " | |
| import json, sys | |
| for inst in json.load(sys.stdin): | |
| if inst['Key']['Hostname'] == '172.30.0.20': | |
| print(inst.get('ClusterName', '')) | |
| sys.exit(0) | |
| " 2>/dev/null || echo "") |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/functional/test-postgresql.sh (1)
289-316:⚠️ Potential issue | 🟠 MajorAvoid mutating
pgstandby1after a failed round-trip.Because this script intentionally continues after
failcalls, a failedBACK_CODEor failedBACK_PROMOTEDcheck still falls through to reactivatingpgstandby1. That can corrupt the setup for the downstream dead-primary failover test and obscure the original failure.Proposed fix
if [ "$BACK_CODE" = "OK" ]; then pass "Round-trip graceful takeover API returned OK" else fail "Round-trip graceful takeover returned: $BACK_CODE — $BACK_RESULT" fi - sleep 10 + BACK_OK=false + if [ "$BACK_CODE" = "OK" ]; then + sleep 10 - # Verify pgprimary is now promoted (not in recovery) - BACK_PROMOTED=$($COMPOSE exec -T pgprimary psql -U postgres -tAc "SELECT pg_is_in_recovery();" 2>/dev/null | tr -d '[:space:]') - if [ "$BACK_PROMOTED" = "f" ]; then - pass "Round-trip complete: pgprimary is primary again" - else - fail "Round-trip incomplete: pgprimary pg_is_in_recovery='$BACK_PROMOTED' (expected f)" + # Verify pgprimary is now promoted (not in recovery) + BACK_PROMOTED=$($COMPOSE exec -T pgprimary psql -U postgres -tAc "SELECT pg_is_in_recovery();" 2>/dev/null | tr -d '[:space:]') + if [ "$BACK_PROMOTED" = "f" ]; then + pass "Round-trip complete: pgprimary is primary again" + BACK_OK=true + else + fail "Round-trip incomplete: pgprimary pg_is_in_recovery='$BACK_PROMOTED' (expected f)" + fi fi # After round-trip, pgstandby1 is the demoted primary — reactivate # it as a live standby so the downstream failover-kill test has a # replica to promote. - echo "Reactivating pgstandby1 as a live standby of pgprimary..." - $COMPOSE exec -T pgstandby1 bash -c 'touch /var/lib/postgresql/data/standby.signal && chown postgres:postgres /var/lib/postgresql/data/standby.signal' || true - $COMPOSE restart pgstandby1 + if [ "$BACK_OK" = "true" ]; then + echo "Reactivating pgstandby1 as a live standby of pgprimary..." + $COMPOSE exec -T pgstandby1 bash -c 'touch /var/lib/postgresql/data/standby.signal && chown postgres:postgres /var/lib/postgresql/data/standby.signal' || true + $COMPOSE restart pgstandby1 + else + skip "Skipping pgstandby1 reactivation — round-trip did not complete" + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/functional/test-postgresql.sh` around lines 289 - 316, The script currently always reactivates pgstandby1 even if the round-trip checks failed; wrap the reactivation block (the lines touching pgstandby1: the touch/chown and $COMPOSE restart pgstandby1) in a guard that only runs when both BACK_CODE = "OK" and BACK_PROMOTED = "f" (i.e. after the pass calls), or alternatively add an immediate exit/return after the fail calls; reference BACK_CODE, BACK_PROMOTED, the pass/fail calls and the pgstandby1 reactivation commands to locate and change the logic so no mutation happens when the round-trip failed.
🧹 Nitpick comments (1)
tests/functional/test-postgresql.sh (1)
255-287: Use one topology snapshot per polling iteration.
NEW_CLUSTERandPRIMARY_CLUSTERare read from separate/api/all-instancescalls while rediscovery is actively changing state. Comparing values from different snapshots can make this poll flaky; fetch once and derive both values from the same JSON payload.Proposed refactor
NEW_CLUSTER="" for i in $(seq 1 30); do - NEW_CLUSTER=$(curl -s --max-time 10 "$ORC_URL/api/all-instances" 2>/dev/null | python3 -c " + ALL_INSTANCES=$(curl -s --max-time 10 "$ORC_URL/api/all-instances" 2>/dev/null || echo "[]") + NEW_CLUSTER=$(printf '%s' "$ALL_INSTANCES" | python3 -c " import json, sys for inst in json.load(sys.stdin): if inst['Key']['Hostname'] == '172.30.0.21': print(inst.get('ClusterName', '')) sys.exit(0) " 2>/dev/null || echo "") # Verify pgprimary (172.30.0.20) joined the same cluster as pgstandby1 - PRIMARY_CLUSTER=$(curl -s --max-time 10 "$ORC_URL/api/all-instances" 2>/dev/null | python3 -c " + PRIMARY_CLUSTER=$(printf '%s' "$ALL_INSTANCES" | python3 -c " import json, sys for inst in json.load(sys.stdin): if inst['Key']['Hostname'] == '172.30.0.20': print(inst.get('ClusterName', '')) sys.exit(0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/functional/test-postgresql.sh` around lines 255 - 287, The loop currently calls /api/all-instances twice and assigns to NEW_CLUSTER and PRIMARY_CLUSTER from separate snapshots, causing flakiness; change to fetch the topology once per iteration (e.g., store the curl output into a local variable like SNAPSHOT_JSON) and then extract both NEW_CLUSTER and PRIMARY_CLUSTER from that same JSON payload (use the existing python3 parsing blocks or jq) so both values come from the identical snapshot before comparing them and before any re-seed calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/functional/test-postgresql.sh`:
- Around line 155-177: The gate SWITCHOVER_OK is set true after checking
PROMOTED only, allowing later steps to proceed even if DEMOTED_RO check fails;
change the logic so SWITCHOVER_OK is only set to true when both conditions
succeed: check PROMOTED (pg_is_in_recovery=false) and then check DEMOTED_RO
(SHOW default_transaction_read_only = "on"), and only set SWITCHOVER_OK=true
after the DEMOTED_RO branch confirms success (or compute SWITCHOVER_OK as the
conjunction of the two checks using PROMOTED and DEMOTED_RO variables); update
references to SWITCHOVER_OK accordingly so downstream round-trip runs only when
both promotion and demotion succeeded.
---
Outside diff comments:
In `@tests/functional/test-postgresql.sh`:
- Around line 289-316: The script currently always reactivates pgstandby1 even
if the round-trip checks failed; wrap the reactivation block (the lines touching
pgstandby1: the touch/chown and $COMPOSE restart pgstandby1) in a guard that
only runs when both BACK_CODE = "OK" and BACK_PROMOTED = "f" (i.e. after the
pass calls), or alternatively add an immediate exit/return after the fail calls;
reference BACK_CODE, BACK_PROMOTED, the pass/fail calls and the pgstandby1
reactivation commands to locate and change the logic so no mutation happens when
the round-trip failed.
---
Nitpick comments:
In `@tests/functional/test-postgresql.sh`:
- Around line 255-287: The loop currently calls /api/all-instances twice and
assigns to NEW_CLUSTER and PRIMARY_CLUSTER from separate snapshots, causing
flakiness; change to fetch the topology once per iteration (e.g., store the curl
output into a local variable like SNAPSHOT_JSON) and then extract both
NEW_CLUSTER and PRIMARY_CLUSTER from that same JSON payload (use the existing
python3 parsing blocks or jq) so both values come from the identical snapshot
before comparing them and before any re-seed calls.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5bd71028-cc57-4822-bf9e-15dc1a716448
📒 Files selected for processing (1)
tests/functional/test-postgresql.sh
|
Follow-up commit |
|
Re-running functional-postgresql jobs — previous CI logs expired (HTTP 410). Investigating remaining failures. |
…a cluster view The post-merge CI run surfaced that the inherited "primary did not change" check is unreliable for PG graceful takeover. Because the demoted primary keeps running until the operator restarts it with standby.signal, orchestrator sees two cluster roots afterward (one per host, each RO=false), and "find RO=false in original cluster" returns the same host both before and after — producing a spurious failure. Replace with direct PostgreSQL-level verification: - pgstandby1 has pg_is_in_recovery()=false (promoted) - pgprimary has default_transaction_read_only=on (demoted) Existing writability and primary_conninfo checks remain. The round-trip section now gates on SWITCHOVER_OK from the direct check and discovers the new cluster name dynamically (pgstandby1 becomes a new cluster root after the first takeover, so the round-trip API call must target its cluster, not the original PG_CLUSTER). The round-trip completion check also uses pg_is_in_recovery() on pgprimary instead of the split-brain cluster view.
The round-trip round of the PG functional test hung for 60s on the second graceful-master-takeover-auto: the wait-for-LSN step never completed because the demoted primary's WAL receiver could not authenticate to the new primary. Root cause: PostgreSQLReconfigureStandby / PostgreSQLRepositionAsStandby set primary_conninfo using orchestrator's credentials (PostgreSQLTopologyUser / PostgreSQLTopologyPassword), but the test cluster's pg_hba.conf only had "host replication repl" — no entry for the orchestrator user. Streaming replication was silently rejected. Add "host replication orchestrator all md5" so that once the operator restarts the demoted primary with standby.signal, the primary_conninfo orchestrator set actually authenticates. This matches the operational prerequisite a real deployment would need to satisfy for the PostGracefulTakeoverProcesses hook to work with only a restart.
First graceful switchover already succeeded under #100's PG-level checks, but the round-trip failed: restarting the demoted primary with only standby.signal left it past the new timeline fork, so it could not stream and catch-up timed out (pg_is_in_recovery stayed true). - Named volumes for PG data so datadir can be rewritten offline - Reclone demoted primary via pg_basebackup onto the new primary - Wait for walreceiver streaming before asserting topology - Enable wal_log_hints for operators who prefer pg_rewind
d5e0226 to
0b1860c
Compare
The reclone wait loop compared against t,streaming, but casting the bool to text yields true/false, so a healthy streaming standby was treated as a failure (last=true,streaming).
1 participant
Summary
PR #99's functional tests fail on master across PG 15/16/17. The failing assertion (
Primary did not change: was 172.30.0.20:5432, now 172.30.0.20:5432) is an inherited check that does not work for PG graceful takeover:primary_conninfo. Per the design, the operator completes the switchover via aPostGracefulTakeoverProcesseshook that createsstandby.signaland restarts.Run: https://github.com/ProxySQL/orchestrator/actions/runs/24606553086
Fix
Replace the cluster-view check with direct PostgreSQL-level signals:
pg_is_in_recovery()=fon pgstandby1 (the standby was promoted)default_transaction_read_only=onon pgprimary (the primary was demoted)The existing writability check against pgstandby1 and the
primary_conninfocheck on pgprimary stay.The round-trip section now:
SWITCHOVER_OKderived from the direct PG check.PG_CLUSTER).pg_is_in_recovery()on pgprimary to verify the round-trip instead of the split-brain cluster view.Test plan
Summary by CodeRabbit
wal_log_hintsand expanded replication access to support dynamicprimary_conninfoconfiguration.