Ensure node is fully removed instead of partially. - #785
Conversation
roosterfish
left a comment
roosterfish
left a comment
There was a problem hiding this comment.
Thanks for this! One remark from my side.
Also does this actually fix #784 or does it rather allow cleanly removing the member from the cluster after you hit the consistency error?
| } | ||
|
|
||
| if err != nil { | ||
| logger.Error("Failed to remove cluster member from dqlite, continuing with other cleanup", slog.String("error", err.Error())) |
There was a problem hiding this comment.
Can you please omit the part after the ,? Just logging Failed to remove cluster member from dqlite should be enough.
Also can you please add slog.Bool("force", force) and maybe also a slog.String("name", name) to give a bit more context which member couldn't be deleted?
|
DeleteClusterMember is called with force on so this does fix it when it fails with prejoin, it also means deleting a cluster member will 100% fix the trust store if its somehow still broken |
There was a problem hiding this comment.
Pull request overview
Fixes forced cluster-member removal so that cleanup continues even if the dqlite removal step fails, preventing leftover truststore state after an incomplete join/remove flow (per issue #784).
Changes:
- Allow
clusterMemberDeleteto proceed with subsequent cleanup whenleader.Remove()fails during a forced removal. - Add an error log when
leader.Remove()fails but removal continues.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil { | ||
| logger.Error("Failed to remove cluster member from dqlite, continuing with other cleanup", slog.String("error", err.Error())) | ||
| } |
I see thanks. Please also append the package name as prefix to the commit message. For example see 065316a. Also would it be feasible to extend any of the tests in |
…ially. Previously on removing the node failing, which happens if the node is not in the cluster, the function exits. Which means the trust store isn't cleaned. This stops an issue wherin a prejoin hook could fail and cause the trust store to remain in a broken state which is not good. Fixes: canonical#784 Signed-off-by: MJ Ponsonby <mj.ponsonby@canonical.com> Assisted-by: Deepseek V4 Pro
When PreJoin or StartAPI fails during a join, the reverts that clean up the cluster member state on the leader ran asynchronously in a goroutine. If the process exited or the network call failed, the leader could be left with stale core_cluster_members and truststore entries, causing a consistency error on the next join attempt (issue canonical#784). Make the DeleteClusterMember call in the revert handler synchronous so the leader state is fully cleaned before the error is returned to the client. Signed-off-by: MJ Ponsonby <mj.ponsonby@canonical.com> Assisted-by: Deepseek V4 Pro
b828f3e to
66a9be1
Compare
Add support for FAIL_PREJOIN=1 environment variable in the example daemon's PreJoin hook, which causes it to return an error. Add a system test that verifies a node with a failing PreJoin hook can still rejoin the cluster successfully, proving that the synchronous cleanup after join failure properly cleans the cluster state. Signed-off-by: MJ Ponsonby <mj.ponsonby@canonical.com> Assisted-by: Deepseek V4 Pro
|
@crypticC0der tests are failing. |
Previously on removing the node failing, which happens if the node is not in the cluster, the function exits. Which means the trust store isn't cleaned. This stops an issue wherin a prejoin hook could fail and cause the trust store to remain in a broken state which is not good.
Fixes: #784