Add PostgreSQL node failover leases - #1000
joostjager wants to merge 1 commit into
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
|
I’m primarily looking for a concept ACK on the overall direction at this stage. The code has already gone through several iterations, and it will probably need another pass once we agree that the lease, fencing, and process-exit approach is the right path.
|
629386d to
056be7a
Compare
|
Would be nice if this is abstracted out to also work with VSS. That way multi-device with a VSS backend can work |
|
The safety-critical part seems mostly backend-specific to me, since lease validation and the KV mutation must be atomic within the backend. What layer of this do you envision abstracting? If generalizing this later would not require much rewriting, I would prefer to leave any necessary code movement to a future VSS lease PR. |
|
Friendly ping @tnull. I would like to agree on the overall approach before doing further implementation cleanup. If you do not currently have the bandwidth to take a look, please let me know too. |
I'll try to get back to a first look some time this week, but review on PRs for v0.8 has priority right now. I think to begin with my first question is whether we don't need a more general version of this anyways, i.e., something a |
For an initial version, I think rebuilding can remain the responsibility of the infrastructure, for example Kubernetes. ldk-node reports the lease lost, ldk-server exits, and the infrastructure restarts the process. The wrapper you propose seems quite a bit broader, and I do not think it is needed to meet the initial objective of failover? As mentioned in #1000 (comment), making lease+mutation atomic is backend-specific, so a general wrapper would not replace that work. |
9d0c61a to
0ff95ef
Compare
|
Pushed a few AI fixes in the mean time. Once we agree on general direction and scope, I can go in and make this PR merge ready. |
|
Moved this to the v0.9 milestone. It seems we need more time here for general architectural discussions, but even out side of these "big picture" questions (whether to find a solution that also incorporates VSS or not etc) it seems that this PR still has a lot of edge cases uncovered. E.g:
|
My review request was explicitly for feedback on the general direction and proposed scope. Many of the items in this list restate scope boundaries and threat-model assumptions that were already called out, rather than identifying newly uncovered implementation edge cases. Within the supported configuration, I do not currently see a concrete safety violation in the list. Mutations are atomically fenced by the lease. Some other scenarios may cause stale reads, duplicate network activity, reduced availability, or unsupported behavior, but it is not clear which one results in state corruption or funds risk. Upgrades were not previously a compatibility concern because PostgreSQL support had not shipped. If #1012 ships first, that changes: its advisory lock and this PR's lease row do not coordinate. |
The migration step indeed. How to migrate from one type of lock to another. |
Make PostgresStore acquire a table-scoped lease during construction and fence schema setup, migrations, and every mutation with it. Track renewals conservatively and fail closed when the local deadline elapses, including while database I/O is stalled. Coordinate the one-way upgrade from schema v1 by taking the legacy advisory-lock key transactionally and committing the schema-v2 marker with the first lease. Older releases then reject the upgraded store. Treat runtime lease loss as process-fatal, reject startup after lease loss, and notify servers so they can exit without final persistence and restart from durable state.
0ff95ef to
ca9246f
Compare
|
I’ve rebased this on the latest main, which includes the temporary advisory lock from #1012 and a migration path to safer locking. This PR is still blocked on a concept ack. I outlined the intended scope in #1000 (comment). @tnull, could you elaborate on how you would like to deviate from or extend that scope (for this PR only). |
| /// from persistence. Store fencing cannot revoke an external effect from a process that pauses | ||
| /// after a fenced mutation, so every critical external effect must immediately follow one and | ||
| /// finish within that mutation's renewed lease interval. | ||
| pub fn wait_for_lease_loss(&self) -> Option<impl Future<Output = ()> + Send + 'static> { |
There was a problem hiding this comment.
I still don't think we'd want something like this in the public LDK Node API. To properly deal with lease losses, we'll need to refactor Node and handle the loss recovery/restarts internally. FWIW, this would also help with cascading persistence failures.
Currently we can end up in a zombie state where we hit a PersistenceFailed but then continue the node operation, which leads to us having to add recovery/replay/retry logic all over the place. I think architecturally we should rather consider towards an STM pattern: some operation in Node hits PersistenceFailed or we lose the lease -> clean shutdown/stop -> try restart.
There was a problem hiding this comment.
How about if we first replace #1012's temporary mechanism with the leases and atomic fencing in this PR, but retain the panic when we lose the lease (no wait_for_lease_loss)? And then follow up with internal recovery, which is a bigger refactor.
There was a problem hiding this comment.
Yes, converging now on a lease design that we know we want to have going forward seems like a good middle ground, to avoid running into any migration issues. Mind updating the PR to only switch the approach (also needs a rebase)?
There was a problem hiding this comment.
I'll work it out in a new one so that we can keep the relevant discussion about how to proceed with the lifecycle here.
4 participants
Addresses #932 by adding minimal active/passive failover support for nodes using the built-in PostgreSQL store.
PostgresStoreacquires an exclusive lease for its KV table, renews it in the background, and fences every mutation by validating and renewing the lease in the same transaction. Lease loss is terminal for the node and is exposed to the application (ldk-server) so the process can exit and restart from persisted state.This initial implementation is intentionally limited to PostgreSQL. Generic stores and VSS are out of scope. The API, timing configuration, and backend support can be extended later if operational experience requires it.