Add replace-node-first-boot option #12316

bhalevy · 2022-12-14T14:58:16Z

Allow replacing a node given its Host ID rather than its ip address.

This series adds a replace_node_first_boot option to db/config
and makes use of it in storage_service.

The new option takes priority over the legacy replace_address* options.
When the latter are used, a deprecation warning is printed.

Documentation updated respectively.

And a cql unit_test is added.

Ref #12277

service/storage_service.cc

kbr-scylla · 2022-12-14T16:09:31Z

test/topology/test_topology.py

+async def test_replace_using_host_id_reuse_ip(manager: ManagerClient, random_tables) -> None:
+    servers = await manager.running_servers()
+    await manager.server_stop(servers[0].server_id)
+    replace_cfg = ReplaceConfig(replaced_id = servers[0].server_id, reuse_ip_addr = True, use_host_id = True)


Maybe we could do the two replaces (using host ID and not using host ID) in a single test, so we don't grow the number of tests cases? Each test case requires starting a separate cluster - which takes some time.

(Maybe we should do the same with reuse_ip_addr - but out of this PR's scope)

okay.

Maybe we should take the same approach in
https://github.com/scylladb/scylla-dtest/pull/2946

@fruch what do you think?

@kbr-scylla this has proven to be more difficult than I thought, since it the second replace in a row fails (ta least some of the times) because it sees one of the nodes as down. I think it's the first replacing node that is not recognized as UN by all other nodes when add_server resolves.

It's out of scope for this PR to deal with that, so I'll keep the tests separated and we can merge them as a follow up.

Ok. Maybe I'll work on merging them later.

kbr-scylla · 2022-12-14T16:23:48Z

docs/operating-scylla/procedures/cluster-management/replace-dead-node.rst

@@ -189,11 +189,11 @@ In this case, the node's data will be cleaned after restart. To remedy this, you

      sudo sed -e '/auto_bootstrap:.*/s/False/True/g' -i /etc/scylla/scylla.yaml

-#. Run the following command, replacing 172.30.0.186 with the listen_address / rpc_address of the node that you are restarting:
+#. Run the following command to replace the Host ID of the node that you are restarting:


The previous statement instructed the reader to take the command below, modify the command by replacing the IP provided here with the IP of the node that they want to restart, and then run the command.

This statement tells the reader that the command replaces the Host ID of the node that the reader is restarting... replaces it with what? This doesn't make much sense. And running the command verbatim is obviously an error - we don't want the reader to run the following command, actually (we want them to run a modified command).

I think we should keep the format of the statement as it was and write something like:
"Run the following command, replacing 87a2b646-c968-4108-a38b-f8468b8b7360 with the Host ID of the node that you are restarting"

yeah, that makes sense.

I'm actually not happy with the phrase you suggested since the action is not about replacing the Host ID, it's about replacing the node previously known by that Host ID.

How about:

#. Run the following command to replace the node known by the previous Host ID of the node you are restarting, 87a2b646-c968-4108-a38b-f8468b8b7360, by the restarted instance, that will be assigned a new, random, Host ID:

action is not about replacing the Host ID

The command itself is not about replacing the Host ID, but the reader of the documentation must first replace the Host ID in the command with the Host ID of their node. That's what the "replacing 87a2b646-c968-4108-a38b-f8468b8b7360 with..." is about.

So the statement is a succint wait of saying:

In the command below, replace "87a2b646-c968-4108-a38b-f8468b8b7360" with the Host ID of the node that you are restarting

Then execute the command below.

init.cc

kbr-scylla · 2022-12-14T16:27:05Z

service/storage_service.hh

            const std::unordered_map<gms::inet_address, sstring>& loaded_peer_features);

+    std::optional<replacement_info> _replacement_info;


I won't object to this, but I don't like putting more state inside the class :( It's not clear if this state is mutable, where it's needed etc. Would be nice if we could avoid it by just passing the replacement_info where we need it.

I agree in principle, but unfortunately, the join_token_ring, handle_state_replacing song and dance requires this state :-(

not present in v5, instead
the local replacement_info prepared in join_token_ring is used to pass the replace address down to functions that need it and are called from join_token_ring

scylladb-promoter · 2022-12-14T18:57:55Z

CI state SUCCESS - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/3501/

service/storage_service.hh

asias · 2022-12-15T07:14:42Z

I suggest adding a warning if the old replace options are used and point the user to use the new option instead.

bhalevy · 2022-12-15T07:44:41Z

I suggest adding a warning if the old replace options are used and point the user to use the new option instead.

I added these warnings in prepare_replacement_info:

    } else if (!cfg.replace_address_first_boot().empty()) {
        replace_address = gms::inet_address(cfg.replace_address_first_boot());
        slogger.warn("The replace_address_first_boot={} option is deprecated. Please use the replace_node_first_boot option", replace_address);
    } else if (!cfg.replace_address().empty()) {
        replace_address = gms::inet_address(cfg.replace_address());
        slogger.warn("The replace_address={} option is deprecated. Please use the replace_node_first_boot option", replace_address);

bhalevy · 2022-12-15T10:04:26Z

In v2 (83e64db):

rebased
storage_service: prepare_replacement_info: keep the replaced node host_id and address in replacement_info
- add only .address member
- reuse .host_id
storage_service: is_replacing: rely directly on config options
- doesn't change logic for replace_address config option handling
  - will cause exception to be thrown if node is already bootstrapped
  - rather than returning false for is_replacing
storage_service: get_replace_address: use _replacement_info
- Improved git log
test: pylib: ServerInfo: add host_id
- new
- isn't used at this point in test_topology
- but can be used later for e.g. providing ignore_dead_nodes_for_replace
db: config: describe replace_address* options as deprecated
- fix typo in ignore_dead_nodes_for_replace description
docs: document the new replace_node_first_boot option
- Rephrased "Run the following command" as per Add replace-node-first-boot option #12316 (comment)

docs/operating-scylla/procedures/cluster-management/replace-dead-node.rst

bhalevy · 2022-12-15T11:58:07Z

In v3 (ea57b93):

addressed docs review comments

scylladb-promoter · 2022-12-15T16:23:55Z

CI state SUCCESS - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/3520/

annastuchlik

The docs look good.

kostja · 2022-12-16T11:59:40Z

docs/operating-scylla/admin.rst

-   * - replace_address_first_boot
-     - Address of the node this Scylla instance is meant to replace. Refer to :doc:`Replace a Dead Node in a Scylla Cluster </operating-scylla/procedures/cluster-management/replace-dead-node>` for more details.
+   * - replace_node_first_boot
+     - Host ID of the node this Scylla instance is meant to replace. Refer to :doc:`Replace a Dead Node in a Scylla Cluster </operating-scylla/procedures/cluster-management/replace-dead-node>` for more details.


Just in this sentence we use node, instance and host for the same thing. Could we settle on fewer names for the same thing perhaps or provide a glossary of what the differences are? At the very least we perhaps should consistently use "Scylla node" rather than "instance" in all places.

Makes sense. How about:

* - replace_node_first_boot - Host ID of a dead node this Scylla node is replacing. Refer to ...

Rephrased in 6808aa1

It still says "Host ID of the node this Scylla instance is meant to replace"

I wonder if I had a bad rebase that lost the above change that I mentioned...
Will (re-)fix

docs/operating-scylla/procedures/cluster-management/replace-dead-node.rst

kostja · 2022-12-16T12:05:43Z

db/config.cc

@@ -788,6 +788,7 @@ db::config::config(std::shared_ptr<db::extensions> exts)
    , consistent_rangemovement(this, "consistent_rangemovement", value_status::Used, true, "When set to true, range movements will be consistent. It means: 1) it will refuse to bootstrap a new node if other bootstrapping/leaving/moving nodes detected. 2) data will be streamed to a new node only from the node which is no longer responsible for the token range. Same as -Dcassandra.consistent.rangemovement in cassandra")
    , join_ring(this, "join_ring", value_status::Unused, true, "When set to true, a node will join the token ring. When set to false, a node will not join the token ring. User can use nodetool join to initiate ring joinging later. Same as -Dcassandra.join_ring in cassandra.")
    , load_ring_state(this, "load_ring_state", value_status::Used, true, "When set to true, load tokens and host_ids previously saved. Same as -Dcassandra.load_ring_state in cassandra.")
+    , replace_node_first_boot(this, "replace_node_first_boot", value_status::Used, "", "The Host ID of the dead node to replace. If this node has been bootstrapped successfully, this option will be ignored.")


What is "this node"? Is "this node" the node to replace or the node that has been started with this option? Would be really nice to disambiguate.

It's the replacing node. How about the following:

The Host ID of a dead node to replace. If the replacing node has already been bootstrapped successfully, this option will be ignored.

Rephrased in 6808aa1

It still says "this node".

oops, will change.

bhalevy · 2022-12-19T08:52:09Z

Converted to draft until #12330 is merged.

scylladb-promoter · 2023-01-03T12:03:19Z

CI state SUCCESS - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/3678/

bhalevy · 2023-01-03T13:52:11Z

In v6 (91e696f):

db: config: add replace_node_first_boot option
- fixed description
storage_service: join_token_ring: reuse replacement_info.address
- check for bool(replace_address) rather than is_replacing()
storage_service: pass replacement_info to booststrap
- check for bool(replacement_info) rather than is_replacing()
storage_service: is_replacing: rely directly on config options
- fixed commit message

scylladb-promoter · 2023-01-03T16:20:59Z

CI state FAILURE - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/3687/

kbr-scylla · 2023-01-03T17:09:35Z

boost.mutation_reader_test.combined_reader_galloping_within_partition_test failed, apparently there was a segfault? Nothing interesting in the logs except exit code:
https://jenkins.scylladb.com/job/releng/job/Scylla-CI/3687/artifact/testlog/x86_64/dev/boost.mutation_reader_test.combined_reader_galloping_within_partition_test.1.log

cc @denesb

Restarting the CI

scylladb-promoter · 2023-01-03T20:25:40Z

CI state SUCCESS - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/3696/

denesb · 2023-01-04T06:02:00Z

boost.mutation_reader_test.combined_reader_galloping_within_partition_test failed, apparently there was a segfault? Nothing interesting in the logs except exit code: https://jenkins.scylladb.com/job/releng/job/Scylla-CI/3687/artifact/testlog/x86_64/dev/boost.mutation_reader_test.combined_reader_galloping_within_partition_test.1.log

cc @denesb

Restarting the CI

Looks like #11842.

Allow replacing a node given its Host ID rather than its ip address. This series adds a replace_node_first_boot option to db/config and makes use of it in storage_service. The new option takes priority over the legacy replace_address* options. When the latter are used, a deprecation warning is printed. Documentation updated respectively. And a cql unit_test is added. Ref #12277 Closes #12316 * github.com:scylladb/scylladb: docs: document the new replace_node_first_boot option dist/docker: support --replace-node-first-boot db: config: describe replace_address* options as deprecated test: test_topology: test replace using host_id test: pylib: ServerInfo: add host_id storage_service: get rid of get_replace_address storage_service: is_replacing: rely directly on config options storage_service: pass replace_address to run_replace_ops storage_service: pass replacement_info to booststrap storage_service: join_token_ring: reuse replacement_info.address storage_service: replacement_info: add replace address init: do not allow cfg.replace_node_first_boot of seed node db: config: add replace_node_first_boot option

bhalevy · 2023-01-13T15:25:00Z

@kbr-scylla this series got dequeued due to topology tests flakiness.
Can we try again?
It blocks #12506

kbr-scylla · 2023-01-13T15:52:08Z

Yes, the problem was too many test causing the test framework to run out of IPs, but that was fixed already.
Could you please rebase?

For replacing a node given its (now unique) Host ID. The existing options for replace_address* will be deprecated in the following patches and eventually we will stop supporting them. Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

Populate replacement_info.address in prepare_replacement_info as a first step towards getting rid of get_replace_address(). Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

So it won't need to call get_replace_address. Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

Rather than on get_replace_address, before we remove the latter. Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

It is unused now. Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

Add test cases exercising the --replace-node-first-boot option by replacing nodes using their host_id rather than ip address. Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

The replace_address options are still supported But mention in their description that they are now deprecated and the user should use replace_node_first_boot instead. While at it fix a typo in ignore_dead_nodes_for_replace Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

And mention that replace_address_first_boot is deprecated Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

And mention that replacing a node using the legacy replace_addr* options is deprecated. Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

bhalevy · 2023-01-13T17:02:09Z

v7 (de3142e):

rebased
pass replacement_info to run_replace_ops rather than just the endpoint
- anticipating Use topology nodes for repair and node operations #12506

scylladb-promoter · 2023-01-13T20:23:43Z

CI state SUCCESS - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/3838/

kbr-scylla

Queued

bhalevy requested a review from tgrabiec as a code owner December 14, 2022 14:58

bhalevy requested review from tzach, kbr-scylla, annastuchlik and asias December 14, 2022 14:58

bhalevy mentioned this pull request Dec 14, 2022

scylla_node: start: add replace_node parameter scylladb/scylla-ccm#421

Merged

bhalevy requested a review from gleb-cloudius December 14, 2022 15:03

kbr-scylla requested changes Dec 14, 2022

View reviewed changes

asias reviewed Dec 15, 2022

View reviewed changes

service/storage_service.hh Outdated Show resolved Hide resolved

asias reviewed Dec 15, 2022

View reviewed changes

service/storage_service.hh Outdated Show resolved Hide resolved

bhalevy force-pushed the replace-node-first-boot branch 2 times, most recently from 06174c9 to 83e64db Compare December 15, 2022 10:02

annastuchlik reviewed Dec 15, 2022

View reviewed changes

docs/operating-scylla/procedures/cluster-management/replace-dead-node.rst Outdated Show resolved Hide resolved

annastuchlik reviewed Dec 15, 2022

View reviewed changes

docs/operating-scylla/procedures/cluster-management/replace-dead-node.rst Outdated Show resolved Hide resolved

bhalevy force-pushed the replace-node-first-boot branch from 83e64db to ea57b93 Compare December 15, 2022 11:57

annastuchlik approved these changes Dec 16, 2022

View reviewed changes

kostja reviewed Dec 16, 2022

View reviewed changes

docs/operating-scylla/procedures/cluster-management/replace-dead-node.rst Show resolved Hide resolved

kostja reviewed Dec 16, 2022

View reviewed changes

bhalevy marked this pull request as draft December 19, 2022 08:51

bhalevy force-pushed the replace-node-first-boot branch 2 times, most recently from 07d8629 to 6808aa1 Compare December 19, 2022 09:51

bhalevy force-pushed the replace-node-first-boot branch from 6808aa1 to dbfc33d Compare January 3, 2023 08:08

bhalevy force-pushed the replace-node-first-boot branch from dbfc33d to 91e696f Compare January 3, 2023 13:27

kbr-scylla approved these changes Jan 3, 2023

View reviewed changes

bhalevy added 13 commits January 13, 2023 18:30

db: config: add replace_node_first_boot option

32e7918

For replacing a node given its (now unique) Host ID. The existing options for replace_address* will be deprecated in the following patches and eventually we will stop supporting them. Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

init: do not allow cfg.replace_node_first_boot of seed node

75c8a5a

Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

storage_service: replacement_info: add replace address

add2f20

Populate replacement_info.address in prepare_replacement_info as a first step towards getting rid of get_replace_address(). Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

storage_service: join_token_ring: reuse replacement_info.address

b863f7a

Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

storage_service: pass replacement_info to booststrap

08598e4

So it won't need to call get_replace_address. Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

storage_service: pass replacement_info to run_replace_ops

7282d58

So it won't need to call get_replace_address. Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

storage_service: is_replacing: rely directly on config options

17f70e4

Rather than on get_replace_address, before we remove the latter. Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

storage_service: get rid of get_replace_address

db2b76b

It is unused now. Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

test: pylib: ServerInfo: add host_id

7d0d9e2

Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

test: test_topology: test replace using host_id

90faeed

Add test cases exercising the --replace-node-first-boot option by replacing nodes using their host_id rather than ip address. Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

dist/docker: support --replace-node-first-boot

d4f1563

And mention that replace_address_first_boot is deprecated Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

docs: document the new replace_node_first_boot option

de3142e

And mention that replacing a node using the legacy replace_addr* options is deprecated. Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

bhalevy force-pushed the replace-node-first-boot branch from 91e696f to de3142e Compare January 13, 2023 16:43

bhalevy mentioned this pull request Jan 13, 2023

Use topology nodes for repair and node operations #12506

Draft

kbr-scylla approved these changes Jan 16, 2023

View reviewed changes

scylladb-promoter merged commit 7510144 into scylladb:master Jan 18, 2023

		const std::unordered_map<gms::inet_address, sstring>& loaded_peer_features);

		std::optional<replacement_info> _replacement_info;

Add replace-node-first-boot option #12316

Add replace-node-first-boot option #12316

Conversation

bhalevy commented Dec 14, 2022

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

scylladb-promoter commented Dec 14, 2022

asias commented Dec 15, 2022

bhalevy commented Dec 15, 2022

bhalevy commented Dec 15, 2022

bhalevy commented Dec 15, 2022

scylladb-promoter commented Dec 15, 2022

annastuchlik left a 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

kostja Dec 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhalevy Dec 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhalevy commented Dec 19, 2022

scylladb-promoter commented Jan 3, 2023

bhalevy commented Jan 3, 2023

scylladb-promoter commented Jan 3, 2023

kbr-scylla commented Jan 3, 2023

scylladb-promoter commented Jan 3, 2023

denesb commented Jan 4, 2023

bhalevy commented Jan 13, 2023

kbr-scylla commented Jan 13, 2023

bhalevy commented Jan 13, 2023

scylladb-promoter commented Jan 13, 2023

kbr-scylla left a comment

Choose a reason for hiding this comment

kostja Dec 16, 2022 •

edited

bhalevy Dec 19, 2022 •

edited