box: apply deletion of replica from `_cluster` on the deleted replica #10114

CuriousGeorgiy · 2024-06-07T06:36:16Z

This patch series applies deletion of replica from _cluster on the deleted replica.

coveralls · 2024-06-07T06:49:19Z

coverage: 87.116% (+0.02%) from 87.101%
when pulling c682fc6 on CuriousGeorgiy:gh-10088-v2
into 822aedf
on tarantool:master.

lenkis · 2024-06-07T08:40:48Z

...lication-luatest/gh_10088_apply_deletion_of_replica_from_cluster_on_deleted_replica_test.lua

+    }}
+    cg.replica:start()
+    -- Add the replica to the master's replicaset table: needed for testing
+    -- behaviour or deleted replica after recovery.


Suggested change

-- behaviour or deleted replica after recovery.

-- the behaviour of the deleted replica after recovery.

lenkis · 2024-06-07T08:41:33Z

...lication-luatest/gh_10088_apply_deletion_of_replica_from_cluster_on_deleted_replica_test.lua

+                    "replica as an anonymous replica"
+    -- If the deleted replica is configured as anonymous during recovery, the
+    -- master sees it as a non-anonymous replica in his replicaset table, and
+    -- reject the subscription request.


Suggested change

-- reject the subscription request.

-- rejects the subscription request.

lenkis · 2024-06-07T08:42:12Z

...lication-luatest/gh_10088_apply_deletion_of_replica_from_cluster_on_deleted_replica_test.lua

+end)
+
+-- Test that synchronous deletion from 3-member cluster with 1 disabled node and
+-- (the deleted node alive) works properly.


Suggested change

-- (the deleted node alive) works properly.

-- the deleted node alive works properly.

lenkis · 2024-06-07T08:57:22Z

changelogs/unreleased/gh-10088-apply-deletion-of-replica-from_cluster-on-deleted-replica.md

@@ -0,0 +1,4 @@
+## feature/replication
+
+* The deletion of a replica from the `_cluster` space is now applied by the


Suggested change

* The deletion of a replica from the `_cluster` space is now applied by the

* A replica deleted from the `_cluster` space now applies its own deletion and does not try to rejoin

lenkis · 2024-06-07T08:57:34Z

changelogs/unreleased/gh-10088-apply-deletion-of-replica-from_cluster-on-deleted-replica.md

+## feature/replication
+
+* The deletion of a replica from the `_cluster` space is now applied by the
+  deleted replica (gh-10088).


Suggested change

deleted replica (gh-10088).

(gh-10088).

coveralls · 2024-06-07T09:53:22Z

coverage: 87.096% (-0.02%) from 87.119%
when pulling 01e87f1 on CuriousGeorgiy:gh-10088-v2
into bc0daf9
on tarantool:master.

Gerold103

Thanks for the patch! Good planing, I like how you managed to split the feature into independent small PRs.

src/box/alter.cc

src/box/replication.cc

test/replication/prune.result

coveralls · 2024-06-10T08:22:25Z

coverage: 87.099% (+0.001%) from 87.098%
when pulling b46fab4 on CuriousGeorgiy:gh-10088-v2
into 32bcea7
on tarantool:master.

Gerold103

Thanks for the patch! Cool, the synchro system spaces are moving quick.

...lication-luatest/gh_10088_apply_deletion_of_replica_from_cluster_on_deleted_replica_test.lua

coveralls · 2024-06-11T09:46:13Z

coverage: 87.074% (-0.04%) from 87.11%
when pulling c646d20 on CuriousGeorgiy:gh-10088-v2
into 9b63ced
on tarantool:master.

Gerold103

Thanks for the fixes! All good now.

Serpentian

Thank you for the patch. No objections)

src/box/applier.cc

src/box/alter.cc

coveralls · 2024-06-13T09:31:15Z

coverage: 87.095% (-0.03%) from 87.127%
when pulling 9ff6e08 on CuriousGeorgiy:gh-10088-v2
into 19d1f1c
on tarantool:master.

sergepetrenko · 2024-06-13T15:03:51Z

Sorry, didn't have time today. Will take a closer look and merge tomorrow.

coveralls · 2024-06-28T07:38:44Z

coverage: 87.114% (+0.05%) from 87.066%
when pulling 7be2395 on CuriousGeorgiy:gh-10088-v2
into 319357d
on tarantool:master.

Serpentian

Thank you for the patch. No objections

sergepetrenko · 2024-07-12T16:29:57Z

src/box/box.cc

+		if (!replica->anon)
+			tnt_raise(ClientError, ER_PROTOCOL, "Can't subscribe "
+				  "a previously deleted non-anonymous replica "
+				  "as an anonymous replica");


Are you sure this check is needed?

Why can't we allow to subscribe the replica as anonymous?

Besides, you need another check for !req.is_anon: I just managed to:

delete replica from _cluster

issue box.cfg{replication=""} and box.cfg{replication="old_replication"} on it

and insert it into the just-deleted slot.

Is this expected?

I think that the replica should be able to subscribe anonymously after the _cluster deletion.

cc @Gerold103, @Serpentian

We cannot subscribe the deleted replica as anonymous, since the master node sees it as non-anonymous in his replicaset table, and AFAIC, transitioning it to anonymous (instead of deleting it from the replicaset table and subscribing it as a new anonymous replica) is not easy, since it will break a lot of invariants.

Why can't we allow to subscribe the replica as anonymous?

Besides, you need another check for !req.is_anon: I just managed to:

delete replica from _cluster

issue box.cfg{replication=""} and box.cfg{replication="old_replication"} on > it

and insert it into the just-deleted slot.

Is this expected?

Yes, this is exactly the expected behavior.

Is this expected?

Also seems valid to me.

since it will break a lot of invariants.

The question here why it is forbidden to turn a normal replica into an anonymous one. Maybe @sergepetrenko knows the motivation. Moreover, you won't catch this situation of resubscription if replica object is deleted.

Sorry, I didn't explain my point good enough.

Basically what happened on steps 2 and 3, the deleted replica registered in the cluster once again.

Anyway, I thought about this some more and I don't see how we can forbid this.

This only happens if the user restarts replication manually, so this seems fine, indeed.

The question here why it is forbidden to turn a normal replica into an anonymous one. Maybe @sergepetrenko knows the motivation. Moreover, you won't catch this situation of resubscription if replica object is deleted.

This simply was never needed.

After oral discussion: this check is legit.

sergepetrenko · 2024-07-12T16:42:08Z

src/box/alter.cc

+			diag_set(ClientError, ER_LOCAL_INSTANCE_ID_IS_READ_ONLY,
+				 (unsigned)instance_id);
+			struct diag *diag = diag_get();
+			applier_kill(origin->applier, diag_last_error(diag));


Thanks for digging into this!
You're right, it's hard to fix this.

I think the correct behavior should be:

ack the transaction which contains replica deletion from _cluster.

Do not ack any further transactions.

Otherwise we might end up in a situation where master commits replica deletion (2 out of 3 nodes agree, but the replica to be deleted hasn't seen the transaction yet), then master is free to add another replica in place of the old one and perform more transactions. Our old replica might receive its own deletion from cluster together with a bunch of new transactions and ack them wrongly. This can lead to a split-brain potentially.

Maybe you can stop acking everything at the moment cluster deletion is received by the replica and send a single special ack just for the transaction containing the deletion from an on_commit trigger?

Another option is to track lsns up to which a particular node has the right to ACK.

I mean, you perform _cluster deletion of a particular replica in a transaction which ends at LSN 8. Then txn_limbo may only count acks for transactions with lsn <= 8.

The second option has a problem that you have to distinguish acks not by replica ids of their senders, but by their UUIDs.

I'm summoning @Gerold103 and @Serpentian here to discuss

Serpentian · 2024-07-15T15:23:09Z

src/box/alter.cc

+			}
+			diag_clear(diag);
+		}
+	}


Don't we also wanna stop the relay on the over nodes to the deleted replica? So that it becomes orphan and we can delete it in replica_clear_id?

I'd say we want to stop relays on the deleted node to the others.
Because basically it has become "anonymous", and we don't allow to replicate from anonymous nodes.

The relays on the other nodes to this replica will be stopped just because the replica stops its appliers.

CuriousGeorgiy requested a review from a team as a code owner June 7, 2024 06:36

CuriousGeorgiy requested review from Gerold103 and Serpentian June 7, 2024 07:21

CuriousGeorgiy assigned Gerold103 and Serpentian Jun 7, 2024

CuriousGeorgiy force-pushed the gh-10088-v2 branch from c682fc6 to fd00a73 Compare June 7, 2024 08:53

lenkis approved these changes Jun 7, 2024

View reviewed changes

CuriousGeorgiy force-pushed the gh-10088-v2 branch 3 times, most recently from 684618b to 01e87f1 Compare June 7, 2024 09:16

Gerold103 requested changes Jun 7, 2024

View reviewed changes

src/box/alter.cc Outdated Show resolved Hide resolved

src/box/replication.cc Show resolved Hide resolved

test/replication/prune.result Show resolved Hide resolved

CuriousGeorgiy force-pushed the gh-10088-v2 branch from 01e87f1 to b46fab4 Compare June 10, 2024 07:37

CuriousGeorgiy requested a review from Gerold103 June 10, 2024 07:38

Gerold103 reviewed Jun 10, 2024

View reviewed changes

...lication-luatest/gh_10088_apply_deletion_of_replica_from_cluster_on_deleted_replica_test.lua Outdated Show resolved Hide resolved

...lication-luatest/gh_10088_apply_deletion_of_replica_from_cluster_on_deleted_replica_test.lua Show resolved Hide resolved

CuriousGeorgiy force-pushed the gh-10088-v2 branch from b46fab4 to c646d20 Compare June 11, 2024 09:27

CuriousGeorgiy requested a review from Gerold103 June 11, 2024 09:28

Gerold103 approved these changes Jun 11, 2024

View reviewed changes

Serpentian approved these changes Jun 13, 2024

View reviewed changes

src/box/applier.cc Show resolved Hide resolved

src/box/alter.cc Outdated Show resolved Hide resolved

CuriousGeorgiy force-pushed the gh-10088-v2 branch from c646d20 to 9ff6e08 Compare June 13, 2024 09:18

CuriousGeorgiy added the full-ci Enables all tests for a pull request label Jun 13, 2024

CuriousGeorgiy assigned sergepetrenko and unassigned Serpentian Jun 13, 2024

sergepetrenko unassigned Gerold103 Jun 13, 2024

sergepetrenko self-requested a review June 13, 2024 15:03

CuriousGeorgiy force-pushed the gh-10088-v2 branch from 78a1925 to 7be2395 Compare June 28, 2024 07:20

CuriousGeorgiy assigned sergepetrenko and unassigned CuriousGeorgiy Jun 28, 2024

Serpentian self-requested a review June 28, 2024 10:29

Serpentian self-assigned this Jun 28, 2024

Serpentian approved these changes Jun 28, 2024

View reviewed changes

Serpentian removed their assignment Jun 28, 2024

This was referenced Jul 11, 2024

[backport 2.11] box: update synchro quorum in on_commit trigger instead of on_replace #10229

Merged

[backport 3.1] box: update synchro quorum in on_commit trigger instead of on_replace #10230

Merged

sergepetrenko reviewed Jul 12, 2024

View reviewed changes

sergepetrenko assigned Gerold103, CuriousGeorgiy and Serpentian Jul 12, 2024

CuriousGeorgiy requested a review from sergepetrenko July 15, 2024 06:49

CuriousGeorgiy assigned CuriousGeorgiy, Gerold103, sergepetrenko and Serpentian and unassigned Gerold103, CuriousGeorgiy, Serpentian and sergepetrenko Jul 15, 2024

Serpentian reviewed Jul 15, 2024

View reviewed changes

Gerold103 mentioned this pull request Jul 15, 2024

relay: ack to TX thread only the txns whose on_commit is finished #10252

Open

sergepetrenko assigned CuriousGeorgiy and unassigned sergepetrenko Jul 16, 2024

Serpentian removed their assignment Jul 17, 2024

Serpentian mentioned this pull request Jul 17, 2024

Enable synchronous replication for systems spaces when the synchronous queue is claimed #10133

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

box: apply deletion of replica from `_cluster` on the deleted replica #10114

box: apply deletion of replica from `_cluster` on the deleted replica #10114

CuriousGeorgiy commented Jun 7, 2024 •

edited

Loading

coveralls commented Jun 7, 2024

lenkis Jun 7, 2024

CuriousGeorgiy Jun 7, 2024

lenkis Jun 7, 2024

CuriousGeorgiy Jun 7, 2024

lenkis Jun 7, 2024

CuriousGeorgiy Jun 7, 2024

lenkis Jun 7, 2024

CuriousGeorgiy Jun 7, 2024

lenkis Jun 7, 2024

CuriousGeorgiy Jun 7, 2024

coveralls commented Jun 7, 2024

Gerold103 left a comment

coveralls commented Jun 10, 2024

Gerold103 left a comment

coveralls commented Jun 11, 2024

Gerold103 left a comment

Serpentian left a comment

coveralls commented Jun 13, 2024

sergepetrenko commented Jun 13, 2024

coveralls commented Jun 28, 2024

Serpentian left a comment

sergepetrenko Jul 12, 2024

CuriousGeorgiy Jul 15, 2024 •

edited

Loading

Serpentian Jul 15, 2024 •

edited

Loading

sergepetrenko Jul 16, 2024

sergepetrenko Jul 16, 2024

CuriousGeorgiy Jul 17, 2024

sergepetrenko Jul 12, 2024

Serpentian Jul 15, 2024

sergepetrenko Jul 16, 2024

	-- behaviour or deleted replica after recovery.
	-- the behaviour of the deleted replica after recovery.

	-- reject the subscription request.
	-- rejects the subscription request.

	-- (the deleted node alive) works properly.
	-- the deleted node alive works properly.

		@@ -0,0 +1,4 @@
		## feature/replication

		* The deletion of a replica from the `_cluster` space is now applied by the

	* The deletion of a replica from the `_cluster` space is now applied by the
	* A replica deleted from the `_cluster` space now applies its own deletion and does not try to rejoin

box: apply deletion of replica from _cluster on the deleted replica #10114

Are you sure you want to change the base?

box: apply deletion of replica from _cluster on the deleted replica #10114

Conversation

CuriousGeorgiy commented Jun 7, 2024 • edited Loading

coveralls commented Jun 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 7, 2024

Gerold103 left a comment

Choose a reason for hiding this comment

coveralls commented Jun 10, 2024

Gerold103 left a comment

Choose a reason for hiding this comment

coveralls commented Jun 11, 2024

Gerold103 left a comment

Choose a reason for hiding this comment

Serpentian left a comment

Choose a reason for hiding this comment

coveralls commented Jun 13, 2024

sergepetrenko commented Jun 13, 2024

coveralls commented Jun 28, 2024

Serpentian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CuriousGeorgiy Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Serpentian Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

box: apply deletion of replica from `_cluster` on the deleted replica #10114

box: apply deletion of replica from `_cluster` on the deleted replica #10114

CuriousGeorgiy commented Jun 7, 2024 •

edited

Loading

CuriousGeorgiy Jul 15, 2024 •

edited

Loading

Serpentian Jul 15, 2024 •

edited

Loading