feat(cluster): Add `--cluster_id` flag #2695

chakaz · 2024-03-06T11:47:53Z

This flag sets the unique ID of a node in a cluster.

It is UB (and bad) to set the same ID to multiple nodes in a cluster.

If unset (default), the master_replid (previously known as master_id) is used.

Fixes #2643
Related to #2636

This flag sets the unique ID of a node in a cluster. It is UB (and bad) to set the same IDs to multiple nodes in the same cluster. If unset (default), the `master_replid` (previously known as `master_id`) is used. Fixes #2643 Related to #2636

romange · 2024-03-06T12:49:47Z

I think cluster id could be perceivded as an id of the cluster and that's not it. I suggest changing to "node_id" because nodes are usually the correct terms when talking about cluster architecture.

chakaz · 2024-03-06T12:54:49Z

How about --cluster_node_id?

adiholden · 2024-03-06T14:50:38Z

src/server/cluster/cluster_family.cc

@@ -25,6 +25,9 @@
 #include "server/server_state.h"

 ABSL_FLAG(std::string, cluster_announce_ip, "", "ip that cluster commands announce to the client");
+ABSL_FLAG(std::string, cluster_id, "",


Did you check if in redis is the node id the same as the cluster node id?
Lets add it to feature tests of cluster i.e
we should have a test for cluster running with replicas and sending data via seeder (if not we need to add), in this test one of the configurations to run is with this flag

and also is there a reason not to set this id for the master_replid_ but have different ids

suppose we always master_replid_ to constant cluster_node_id, then in case a master restarts, it will still have the same id and the replica will happily replicate empty data from it. that's what we are trying to prevent, right?

Did you check if in redis is the node id the same as the cluster node id?

No, it's not the same ID. Here's a log from a local redis cluster I set:

177592:M 07 Mar 2024 13:01:29.548 * No cluster configuration found, I'm 09a6bad961e78159056f8ad3865a3e2d47d74045 177592:M 07 Mar 2024 13:01:29.560 * Server initialized [...] 177592:M 07 Mar 2024 13:02:15.440 * Replica 127.0.0.1:6379 asks for synchronization 177592:M 07 Mar 2024 13:02:15.440 * Full resync requested by replica 127.0.0.1:6379 177592:M 07 Mar 2024 13:02:15.440 * Replication backlog created, my new replication IDs are '67e4e538e2e0a76da7a1454615c5ce64954c4800' and '0000000000000000000000000000000000000000'

So it's not using the same ID.

Lets add it to feature tests of cluster i.e we should have a test for cluster running with replicas and sending data via seeder (if not we need to add), in this test one of the configurations to run is with this flag

Done.

adiholden · 2024-03-07T13:48:49Z

src/server/server_family.cc

@@ -2385,7 +2385,7 @@ void ServerFamily::ReplConf(CmdArgList args, ConnectionContext* cntx) {
        // The response for 'capa dragonfly' is: <masterid> <syncid> <numthreads> <version>
        auto* rb = static_cast<RedisReplyBuilder*>(cntx->reply_builder());
        rb->StartArray(4);
-        rb->SendSimpleString(master_id_);
+        rb->SendSimpleString(master_replid_);


I dont understand how this is the same as cluster node id ..
master_replid_ is set in this line
master_replid_ = GetRandomHex(eng, CONFIG_RUN_ID_SIZE);
I dont see where you that you overide it with cluster node id

This PR splits the IDs into 2 separate ones, if --cluster_node_id is set.
I.e. the ID used for master_replid_ is always random and can't be overridden, while the ID for cluster has a default value of master_replid_ but can be overridden if set as an argument.

Ok sorry I miss understood your response on the other thread. So if this is not the same id it as the master_replid_ we should say that we do not support this flag for emulated cluster because in emulated cluster we reply the replica / master etl.remote_client_id_ which is the master_replid_ generated

adiholden · 2024-03-07T13:50:52Z

tests/dragonfly/cluster_test.py

 @dfly_args({"proactor_threads": 4, "cluster_mode": "yes"})
-async def test_cluster_replica_sets_non_owned_keys(df_local_factory):
+async def test_cluster_replica_sets_non_owned_keys(
+    df_local_factory: DflyInstanceFactory, set_cluster_node_id: bool


you dont use this param in the test

Sorry, leftovers. I decided to use the test below instead (test_cluster_native_client)

adiholden · 2024-03-07T13:57:39Z

tests/dragonfly/cluster_test.py

@@ -570,14 +584,20 @@ async def test_cluster_blocking_command(df_server):
    await close_clients(c_master, c_master_admin)


+@pytest.mark.parametrize("set_cluster_node_id", [True, False])
 @dfly_args({"proactor_threads": 4, "cluster_mode": "yes"})
 async def test_cluster_native_client(


where do we run replicaof in this test? I cant find it..

good catch! :)
I added it, even though it doesn't really matter too much, because we never reach out to the replicas.. should we?

We have this in the test.. I guess it is not connecting to the replica as we thought, as the test would have failed before because we did not really replicated
# Make sure that getting a value from a replica works as well.
replica_response = await client.execute_command(
"get", "key0", target_nodes=aioredis.RedisCluster.REPLICAS
)
assert "value" in replica_response.values()

again, very nice catch!
The reason why it worked before is that it followed MOVED error replies to the relevant master, and got the response from it.
Now I use the connections directly, which does not follow MOVED (I checked and it fails if I comment out the replicaof flag), and now the test works.

adiholden · 2024-03-10T09:48:04Z

tests/dragonfly/utility.py

@@ -46,12 +46,8 @@ async def wait_available_async(client: aioredis.Redis, timeout=10):
    start = time.time()
    while (time.time() - start) < timeout:
        try:
-            await client.get("key")
+            await client.ping()


why did you change this?
we want here to make sure that server exits loading stage

First, PING does make sure that the server exits loading stage. We reply with an error if we don't:

$ redis-cli ping (error) LOADING Dragonfly is loading the dataset in memory

This is by design: https://github.com/dragonflydb/dragonfly/blob/cluster-id/src/server/generic_family.cc#L1602

As for why: It's somewhat subtle.
We can't use GET before we push a cluster config, as that would return an error. On the other hand, we can't push a config before replica finishes loading. So we simply can't use GET for this scenario.
On the positive side, now we don't have the special handling of MOVED error in this area :)

* feat(cluster): Add `--cluster_id` flag This flag sets the unique ID of a node in a cluster. It is UB (and bad) to set the same IDs to multiple nodes in the same cluster. If unset (default), the `master_replid` (previously known as `master_id`) is used. Fixes #2643 Related to #2636 * gh comments * oops - revert line removal * fix * replica * disallow cluster_node_id in emulated mode * fix replica test

feat(cluster): Add --cluster_id flag

5f8d60e

This flag sets the unique ID of a node in a cluster. It is UB (and bad) to set the same IDs to multiple nodes in the same cluster. If unset (default), the `master_replid` (previously known as `master_id`) is used. Fixes #2643 Related to #2636

kostasrim previously approved these changes Mar 6, 2024

View reviewed changes

chakaz requested a review from romange March 6, 2024 12:35

adiholden reviewed Mar 6, 2024

View reviewed changes

gh comments

81394e5

chakaz dismissed kostasrim’s stale review via 81394e5 March 7, 2024 10:49

chakaz requested a review from adiholden March 7, 2024 11:05

oops - revert line removal

c55f823

adiholden reviewed Mar 7, 2024

View reviewed changes

chakaz added 2 commits March 10, 2024 11:12

fix

af42eb7

replica

76056fd

chakaz requested a review from adiholden March 10, 2024 09:29

adiholden reviewed Mar 10, 2024

View reviewed changes

chakaz added 2 commits March 10, 2024 12:09

disallow cluster_node_id in emulated mode

ef9c4f6

fix replica test

ef99f58

adiholden approved these changes Mar 10, 2024

View reviewed changes

chakaz merged commit 8b31195 into main Mar 10, 2024
10 checks passed

chakaz deleted the cluster-id branch March 10, 2024 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cluster): Add `--cluster_id` flag #2695

feat(cluster): Add `--cluster_id` flag #2695

chakaz commented Mar 6, 2024 •

edited

Loading

romange commented Mar 6, 2024 •

edited

Loading

chakaz commented Mar 6, 2024

adiholden Mar 6, 2024

adiholden Mar 6, 2024

romange Mar 7, 2024

chakaz Mar 7, 2024

adiholden Mar 7, 2024

chakaz Mar 10, 2024

adiholden Mar 10, 2024

adiholden Mar 7, 2024

chakaz Mar 10, 2024

adiholden Mar 7, 2024

chakaz Mar 10, 2024

adiholden Mar 10, 2024

chakaz Mar 10, 2024

adiholden Mar 10, 2024

chakaz Mar 10, 2024

feat(cluster): Add --cluster_id flag #2695

feat(cluster): Add --cluster_id flag #2695

Conversation

chakaz commented Mar 6, 2024 • edited Loading

romange commented Mar 6, 2024 • edited Loading

chakaz commented Mar 6, 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

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

feat(cluster): Add `--cluster_id` flag #2695

feat(cluster): Add `--cluster_id` flag #2695

chakaz commented Mar 6, 2024 •

edited

Loading

romange commented Mar 6, 2024 •

edited

Loading