fix: prevent subscription breakage on rolling updates#429
Conversation
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
🟢 Metrics 14 complexity · 0 duplication
Metric Results Complexity 14 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR adds replication-slot readiness checks before replica instances become available, removes switchover-retention behavior, updates operation expectations, and expands end-to-end coverage for replicated two-node rolling PostgreSQL upgrades. ChangesReplication-aware rolling updates
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/internal/database/operations/update_nodes.go (1)
45-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCommented-out block will not compile if re-enabled as-is.
The disabled block still references
database.SwitchoverResource,patroni.InstanceRolePrimary, and the now-removedprimaryHostID. Since thedatabase/patroniimports andprimaryHostIDwere dropped in this change, anyone re-enabling this per the TODO must also re-add those imports and re-introduceprimaryHostIDtracking during instance iteration. Consider adding a short note in the TODO to that effect (or tracking the removed logic via git history / a feature flag) so the re-enable path isn't a silent compile break.🤖 Prompt for 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. In `@server/internal/database/operations/update_nodes.go` around lines 45 - 61, The commented-out switchover block in updateNodes will fail to compile if re-enabled because it still depends on database.SwitchoverResource, patroni.InstanceRolePrimary, and primaryHostID that were removed from this change. Update the TODO near updateNodes so it explicitly notes that re-enabling this logic also requires restoring the database and patroni imports plus reintroducing primaryHostID tracking in the instance loop, or otherwise keep the removed logic behind a guarded feature path instead of a bare comment.server/internal/database/instance_resource.go (1)
255-301: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
selectinstead oftime.Sleepfor cancellable polling.
time.Sleep(pollInterval)blocks for 2 seconds without responding to context cancellation. If the context is canceled mid-sleep, the function waits the full interval before checkingctx.Err()at the top of the next iteration. Replace with aselectonctx.Done()andtime.Afterfor prompt cancellation responsiveness.♻️ Proposed refactor
if exists { break } - time.Sleep(pollInterval) + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(pollInterval): + } }🤖 Prompt for 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. In `@server/internal/database/instance_resource.go` around lines 255 - 301, `waitForReplicationSlots` uses `time.Sleep(pollInterval)` inside its polling loop, which delays cancellation handling; replace that sleep with a `select` on `ctx.Done()` and `time.After(pollInterval)` so the loop exits promptly when the context is canceled. Keep the existing replication-slot check flow in `waitForReplicationSlots` and preserve the `ctx.Err()`/error returns, but make the wait cancellable between `postgres.ReplicationSlotExists(...).Scalar(ctx, conn)` retries.
🤖 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.
Nitpick comments:
In `@server/internal/database/instance_resource.go`:
- Around line 255-301: `waitForReplicationSlots` uses `time.Sleep(pollInterval)`
inside its polling loop, which delays cancellation handling; replace that sleep
with a `select` on `ctx.Done()` and `time.After(pollInterval)` so the loop exits
promptly when the context is canceled. Keep the existing replication-slot check
flow in `waitForReplicationSlots` and preserve the `ctx.Err()`/error returns,
but make the wait cancellable between
`postgres.ReplicationSlotExists(...).Scalar(ctx, conn)` retries.
In `@server/internal/database/operations/update_nodes.go`:
- Around line 45-61: The commented-out switchover block in updateNodes will fail
to compile if re-enabled because it still depends on
database.SwitchoverResource, patroni.InstanceRolePrimary, and primaryHostID that
were removed from this change. Update the TODO near updateNodes so it explicitly
notes that re-enabling this logic also requires restoring the database and
patroni imports plus reintroducing primaryHostID tracking in the instance loop,
or otherwise keep the removed logic behind a guarded feature path instead of a
bare comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 25080471-7475-4bb1-aeed-96fe8761c3cd
📒 Files selected for processing (5)
e2e/minor_version_upgrade_test.goserver/internal/database/instance_resource.goserver/internal/database/operations/update_nodes.goserver/internal/database/operations/update_nodes_test.goserver/internal/postgres/create_db.go
💤 Files with no reviewable changes (1)
- server/internal/database/operations/update_nodes_test.go
9eb283b to
d626c7c
Compare
2 participants
Summary
This PR fixes rolling database updates failing on multi-node databases with replica instances.
Changes
instance_resource.go— AddwaitForReplicationSlotshelper. WheninitializeInstanceruns on a replica instance, it now pollspg_replication_slotsfor every subscription where this node is theprovider, blocking until each slot exists before marking the instance
available. Addresses the Spock 5.x race where the worker takes up to
60 s to create failover slots after a switchover.
postgres/create_db.go— AddReplicationSlotExistsquery used bythe helper above.
operations/update_nodes.go— Comment out theSwitchoverResourceaddition that forced a second Patroni switchover to restore the original
primary after a rolling update. This second switchover was the trigger for
the subscription breakage. The
SwitchoverResourcetype is kept so"retain primary" behaviour can be re-enabled later.
update_nodes_test.go— RemoveSwitchoverResourcefrom allexpected states to match the new behaviour.
Testing
Verification:
Checklist
PLAT-665