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.
|
Warning Review limit reached
Next review available in: 48 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 Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR adds replication-slot readiness checks before marking replica instances available, removes switchover-retention logic from node update and end-state paths, updates related tests and fixtures, and expands e2e coverage for rolling Postgres upgrades on a replicated two-node topology. ChangesReplication-aware rolling updates
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 |
![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
tsivaprasad
force-pushed
the
PLAT-665-rolling-database-update-fails-on-multi-replica-nodes
branch
from
9eb283b to
d626c7c
Compare
1 participant
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