Ensure node is fully removed instead of partially. by crypticC0der · Pull Request #785 · canonical/microcluster

crypticC0der · 2026-07-09T10:49:51Z

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

roosterfish

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?

roosterfish · 2026-07-09T11:52:39Z

 		}
+
+		if err != nil {
+			logger.Error("Failed to remove cluster member from dqlite, continuing with other cleanup", slog.String("error", err.Error()))


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?

crypticC0der · 2026-07-09T13:52:08Z

DeleteClusterMember is called with force on

microcluster/internal/rest/resources/control.go

Line 133 in a2735b4

    
           err = internalClient.DeleteClusterMember(context.Background(), client, req.Name, "", true)

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

Copilot

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 clusterMemberDelete to proceed with subsequent cleanup when leader.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()))
+		}


roosterfish · 2026-07-10T07:20:54Z

DeleteClusterMember is called with force on

microcluster/internal/rest/resources/control.go

Line 133 in a2735b4

err = internalClient.DeleteClusterMember(context.Background(), client, req.Name, "", true)

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

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 example/test to simulate and test this? We could maybe modify the PreJoin hook in there to have a different outcome based on something we can set external to Microcluster like the existence of a file or an env variable being set like FAIL_PREJOIN=1.

roosterfish · 2026-07-21T08:07:26Z

@crypticC0der tests are failing.

roosterfish · 2026-09-04T07:59:35Z

I'll move this to draft until the failing test/comments are addressed.

markylaing · 2026-09-30T12:41:26Z

@crypticC0der do you plan to continue working on this?

When a join fails (for example because the PreJoin hook returns an error), the revert handler removes the joining member from the cluster. Previously this cleanup was unreliable and could strand the cluster in an inconsistent state, blocking the node from rejoining: - The cluster member delete request was issued in a goroutine that only ran after the request finished, so a process exit or failed network call after the error could leave stale core_cluster_members and truststore entries. - A failure removing the node from dqlite returned early even when `force` was set, skipping the rest of the cleanup. - Propagating the truststore removal to the other members returned on the first error, so a member that never received the entry (a join that failed before propagation) aborted the local removal and left a stale entry. Run the delete synchronously, ignore dqlite removal errors when forcing, and make the truststore propagation best-effort so the local entry is always removed. This keeps core_cluster_members, the truststore and dqlite consistent after a failed join. Fixes: canonical#784 Signed-off-by: MJ Ponsonby <mj.ponsonby@canonical.com>

Extend the example daemon's PreJoin hook to bail out when the init config carries `fail=1`, and add a system test that verifies a node whose PreJoin hook fails can still rejoin the cluster afterwards. The config-driven failure replaces the previous environment-variable approach, which survived the daemon reset (re-exec) after the failed join and racy restart. Signed-off-by: MJ Ponsonby <mj.ponsonby@canonical.com>

crypticC0der · 2026-10-30T19:27:03Z

@roosterfish this now fixes #784 at the root, not just recover after the fact.

The leftover was the truststore entry. On a failed join the revert calls DeleteClusterMember on the leader; clusterMemberDelete removes the member from core_cluster_members and dqlite fine, then removes the truststore entry via trustDelete. That handler propagates the delete to every other member first and returned on the first error — but on a failed join only the leader ever has the entry (the AddTrustStoreEntry broadcast in StartAPI runs after PreJoin, which never completed), so the peers return No truststore entry found and the local removal was skipped. That's exactly the inconsistency #784 reports.

Fixed by making the truststore propagation best-effort (log + continue), so the local entry is always removed. Together with the synchronous DeleteClusterMember revert (already in this PR) that leaves core_cluster_members / truststore / dqlite consistent, and a node whose PreJoin hook fails can rejoin cleanly.

Also reworked the test into a real regression test — the PreJoin failure is now driven via --config fail=1 (a per-command init config) rather than an env var, which previously survived the daemon re-exec and raced the restart.

CI is green now. Mind taking another look?

roosterfish requested changes Jul 9, 2026

View reviewed changes

roosterfish requested a review from Copilot July 10, 2026 07:13

Copilot started reviewing on behalf of roosterfish July 10, 2026 07:14

View session

Copilot AI reviewed Jul 10, 2026

View reviewed changes

Comment thread internal/rest/resources/cluster.go

Comment on lines +716 to +718

if err != nil {

logger.Error("Failed to remove cluster member from dqlite, continuing with other cleanup", slog.String("error", err.Error()))

}

crypticC0der force-pushed the v3 branch 3 times, most recently from 66a9be1 to 35005d0 Compare July 13, 2026 14:55

roosterfish marked this pull request as draft September 4, 2026 07:59

crypticC0der added 2 commits October 30, 2026 20:05

crypticC0der force-pushed the v3 branch from 35005d0 to 3eec525 Compare October 30, 2026 19:13

Navigation Menu

Repository navigation

Ensure node is fully removed instead of partially. - #785

Ensure node is fully removed instead of partially.#785
crypticC0der wants to merge 2 commits into
canonical:v3from
crypticC0der:v3

crypticC0der commented Jul 9, 2026

Uh oh!

roosterfish left a comment

Uh oh!

roosterfish Jul 9, 2026

Uh oh!

crypticC0der commented Jul 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

roosterfish commented Jul 10, 2026

Uh oh!

roosterfish commented Jul 21, 2026

Uh oh!

roosterfish commented Sep 4, 2026

Uh oh!

markylaing commented Sep 30, 2026

Uh oh!

crypticC0der commented Oct 30, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

crypticC0der commented Jul 9, 2026

Uh oh!

roosterfish left a comment

Choose a reason for hiding this comment

Uh oh!

roosterfish Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

crypticC0der commented Jul 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

roosterfish commented Jul 10, 2026

Uh oh!

roosterfish commented Jul 21, 2026

Uh oh!

roosterfish commented Sep 4, 2026

Uh oh!

markylaing commented Sep 30, 2026

Uh oh!

crypticC0der commented Oct 30, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants