test_decommission_after_changing_node_ip: node4 fails to start when stale ip address of node3 appears as DOWN, status=shutdown #14468
Assignees
Labels
Milestone
Comments
bhalevy
changed the title
|
@kbr-scylla, I bisected this to this merge: 50e8ec7 |
bhalevy
added
status/regression
area/repair-based operations
|
The tests passes with the following patch: diff --git a/service/storage_service.cc b/service/storage_service.cc
index 1771800565..9e9fecc436 100644
--- a/service/storage_service.cc
+++ b/service/storage_service.cc
@@ -2459,11 +2459,14 @@ future<> storage_service::handle_state_normal(inet_address endpoint) {
if (tmptr->is_normal_token_owner(endpoint)) {
slogger.info("Node {} state jump to normal", endpoint);
}
- std::unordered_set<inet_address> endpoints_to_remove;
+ std::unordered_map<inet_address, bool> endpoints_to_remove;
- auto do_remove_node = [&] (gms::inet_address node) {
+ auto do_remove_node = [&] (gms::inet_address node, bool force = false) {
tmptr->remove_endpoint(node);
- endpoints_to_remove.insert(node);
+ auto [it, inserted] = endpoints_to_remove.try_emplace(node, force);
+ if (!inserted && force) {
+ it->second = force;
+ }
};
// Order Matters, TM.updateHostID() should be called before TM.updateNormalToken(), (see CASSANDRA-4300).
if (_gossiper.uses_host_id(endpoint)) {
@@ -2475,7 +2478,7 @@ future<> storage_service::handle_state_normal(inet_address endpoint) {
do_remove_node(endpoint);
} else if (_gossiper.compare_endpoint_startup(endpoint, *existing) > 0) {
slogger.warn("Host ID collision for {} between {} and {}; {} is the new owner", host_id, *existing, endpoint, endpoint);
- do_remove_node(*existing);
+ do_remove_node(*existing, true);
slogger.info("Set host_id={} to be owned by node={}, existing={}", host_id, endpoint, *existing);
tmptr->update_host_id(host_id, endpoint);
} else {
@@ -2557,7 +2560,7 @@ future<> storage_service::handle_state_normal(inet_address endpoint) {
for (const auto& ep : candidates_for_removal) {
slogger.info("handle_state_normal: endpoints_to_remove endpoint={}", ep);
- endpoints_to_remove.insert(ep);
+ endpoints_to_remove.try_emplace(ep, false);
}
bool is_normal_token_owner = tmptr->is_normal_token_owner(endpoint);
@@ -2584,8 +2587,8 @@ future<> storage_service::handle_state_normal(inet_address endpoint) {
co_await replicate_to_all_cores(std::move(tmptr));
tmlock.reset();
- for (auto ep : endpoints_to_remove) {
- co_await remove_endpoint(ep);
+ for (const auto& [ep, force] : endpoints_to_remove) {
+ co_await remove_endpoint(ep, force);
}
slogger.debug("handle_state_normal: endpoint={} is_normal_token_owner={} endpoint_to_remove={} owned_tokens={}", endpoint, is_normal_token_owner, endpoints_to_remove.contains(endpoint), owned_tokens);
if (!owned_tokens.empty() && !endpoints_to_remove.count(endpoint)) {
@@ -3208,8 +3211,12 @@ future<> storage_service::check_for_endpoint_collision(std::unordered_set<gms::i
});
}
-future<> storage_service::remove_endpoint(inet_address endpoint) {
- co_await _gossiper.remove_endpoint(endpoint);
+future<> storage_service::remove_endpoint(inet_address endpoint, bool force) {
+ if (force) {
+ co_await _gossiper.force_remove_endpoint(endpoint);
+ } else {
+ co_await _gossiper.remove_endpoint(endpoint);
+ }
try {
co_await _sys_ks.local().remove_endpoint(endpoint);
} catch (...) {
diff --git a/service/storage_service.hh b/service/storage_service.hh
index c0ba08ac67..abf49cb104 100644
--- a/service/storage_service.hh
+++ b/service/storage_service.hh
@@ -547,7 +547,7 @@ class storage_service : public service::migration_listener, public gms::i_endpoi
future<> excise(std::unordered_set<token> tokens, inet_address endpoint, long expire_time);
/** unlike excise we just need this endpoint gone without going through any notifications **/
- future<> remove_endpoint(inet_address endpoint);
+ future<> remove_endpoint(inet_address endpoint, bool force = false);
void add_expire_time_if_found(inet_address endpoint, int64_t expire_time);
|
|
I'm testing this patch with the following dtest suites: update_cluster_layout_tests.py replace_address_test.py repair_based_node_operations_test.py |
bhalevy
added a commit
to bhalevy/scylla
that referenced
this issue
Jul 2, 2023
When a host changes its ip address we should force remove the previous endpoint state since we want only one endpoint to refer to this host_id. If the new node that changed its ip address is decommissioned, the previous node seems as a normal token owner, just in shutdown status, but it is not longer in the cluster. Fixes scylladb#14468 Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
kbr-scylla
added a commit
to kbr-scylla/scylladb
that referenced
this issue
Jul 4, 2023
…rmal handlers Before this commit the `wait_for_normal_state_handled_on_boot` would wait for a static set of nodes (`sync_nodes`), calculated using the `get_nodes_to_sync_with` function and `parse_node_list`; the latter was used to obtain a list of "nodes to ignore" (for replace operation) and translate them, using `token_metadata`, from IP addresses to Host IDs and vice versa. `sync_nodes` was also used in `_gossiper.wait_alive` call which we do after `wait_for_normal_state_handled_on_boot`. Recently we started doing these calculations and this wait very early in the boot procedure - immediately after we start gossiping (50e8ec7). Unfortunately, as always with gossiper, there are complications. In scylladb#14468 and scylladb#14487 two problems were detected: - Gossiper may contain obsolete entries for nodes which were recently replaced or changed their IPs. These entries are still using status `NORMAL` or `shutdown` (which is treated like `NORMAL`, e.g. `handle_state_normal` is also called for it). The `_gossiper.wait_alive` call would wait for those entries too and eventually time out. - Furthermore, by the time we call `parse_node_list`, `token_metadata` may not be populated yet, which is required to do the IP<->Host ID translations -- and populating `token_metadata` happens inside `handle_state_normal`, so we have a chicken-and-egg problem here. The `parse_node_list` problem is solved in this commit. It turns out that we don't need to calculate `sync_nodes` (and hence `ignore_nodes`) in order to wait for NORMAL state handlers. We can wait for handlers to finish for *any* `NORMAL`/`shutdown` entries appearing in gossiper, even those that correspond to dead/ignored nodes and obsolete IPs. `handle_state_normal` is called, and eventually finishes, for all of them. `wait_for_normal_state_handled_on_boot` no longer receives a set of nodes as parameter and is modified appropriately, it's now calculating the necessary set of nodes on each retry (the set may shrink while we're waiting, e.g. because an entry corresponding to a node that was replaced is garbage-collected from gossiper state). Thanks to this, we can now put the `sync_nodes` calculation (which is still necessary for `_gossiper.wait_alive`), and hence the `parse_node_list` call, *after* we wait for NORMAL state handlers, solving the chickend-and-egg problem. This addresses the immediate failure described in scylladb#14487, but the test will still fail. That's because `_gossiper.wait_alive` may still receive a too large set of nodes -- we may still include obsolete IPs or entries corresponding to replaced nodes in the `sync_nodes` set. We fix this in the following commit which will solve both issues.
kbr-scylla
added a commit
to kbr-scylla/scylladb
that referenced
this issue
Jul 4, 2023
…o be UP At bootstrap, after we start gossiping, we calculate a set of nodes (`sync_nodes`) which we need to "synchronize" with, waiting for them to be UP before proceeding; these nodes are required for streaming/repair and CDC generation data write, and generally are supposed to constitute the current set of cluster members. In scylladb#14468 and scylladb#14487 we observed that this set may calculate entries corresponding to nodes that were just replaced or changed their IPs (but the old-IP entry is still there). We pass them to `_gossiper.wait_alive` and the call eventually times out. We need a better way to calculate `sync_nodes` which detects ignores obsolete IPs and nodes that are already gone but just weren't garbage-collected from gossiper state yet. In fact such a method was already introduced in the past: ca61d88 but it wasn't used everywhere. There, we use `token_metadata` in which collisions between Host IDs and tokens are resolved, so it contains only entries that correspond to the "real" current set of NORMAL nodes. We use this method to calculate the set of nodes passed to `_gossiper.wait_alive`. Fixes scylladb#14468 Fixes scylladb#14487
kbr-scylla
added a commit
to kbr-scylla/scylladb
that referenced
this issue
Jul 4, 2023
Regression test for scylladb#14468.
kbr-scylla
added a commit
to kbr-scylla/scylladb
that referenced
this issue
Jul 4, 2023
Regression test for scylladb#14468.
kbr-scylla
added a commit
to kbr-scylla/scylladb
that referenced
this issue
Jul 4, 2023
…rmal handlers Before this commit the `wait_for_normal_state_handled_on_boot` would wait for a static set of nodes (`sync_nodes`), calculated using the `get_nodes_to_sync_with` function and `parse_node_list`; the latter was used to obtain a list of "nodes to ignore" (for replace operation) and translate them, using `token_metadata`, from IP addresses to Host IDs and vice versa. `sync_nodes` was also used in `_gossiper.wait_alive` call which we do after `wait_for_normal_state_handled_on_boot`. Recently we started doing these calculations and this wait very early in the boot procedure - immediately after we start gossiping (50e8ec7). Unfortunately, as always with gossiper, there are complications. In scylladb#14468 and scylladb#14487 two problems were detected: - Gossiper may contain obsolete entries for nodes which were recently replaced or changed their IPs. These entries are still using status `NORMAL` or `shutdown` (which is treated like `NORMAL`, e.g. `handle_state_normal` is also called for it). The `_gossiper.wait_alive` call would wait for those entries too and eventually time out. - Furthermore, by the time we call `parse_node_list`, `token_metadata` may not be populated yet, which is required to do the IP<->Host ID translations -- and populating `token_metadata` happens inside `handle_state_normal`, so we have a chicken-and-egg problem here. The `parse_node_list` problem is solved in this commit. It turns out that we don't need to calculate `sync_nodes` (and hence `ignore_nodes`) in order to wait for NORMAL state handlers. We can wait for handlers to finish for *any* `NORMAL`/`shutdown` entries appearing in gossiper, even those that correspond to dead/ignored nodes and obsolete IPs. `handle_state_normal` is called, and eventually finishes, for all of them. `wait_for_normal_state_handled_on_boot` no longer receives a set of nodes as parameter and is modified appropriately, it's now calculating the necessary set of nodes on each retry (the set may shrink while we're waiting, e.g. because an entry corresponding to a node that was replaced is garbage-collected from gossiper state). Thanks to this, we can now put the `sync_nodes` calculation (which is still necessary for `_gossiper.wait_alive`), and hence the `parse_node_list` call, *after* we wait for NORMAL state handlers, solving the chickend-and-egg problem. This addresses the immediate failure described in scylladb#14487, but the test will still fail. That's because `_gossiper.wait_alive` may still receive a too large set of nodes -- we may still include obsolete IPs or entries corresponding to replaced nodes in the `sync_nodes` set. We fix this in the following commit which will solve both issues.
kbr-scylla
added a commit
to kbr-scylla/scylladb
that referenced
this issue
Jul 4, 2023
…o be UP At bootstrap, after we start gossiping, we calculate a set of nodes (`sync_nodes`) which we need to "synchronize" with, waiting for them to be UP before proceeding; these nodes are required for streaming/repair and CDC generation data write, and generally are supposed to constitute the current set of cluster members. In scylladb#14468 and scylladb#14487 we observed that this set may calculate entries corresponding to nodes that were just replaced or changed their IPs (but the old-IP entry is still there). We pass them to `_gossiper.wait_alive` and the call eventually times out. We need a better way to calculate `sync_nodes` which detects ignores obsolete IPs and nodes that are already gone but just weren't garbage-collected from gossiper state yet. In fact such a method was already introduced in the past: ca61d88 but it wasn't used everywhere. There, we use `token_metadata` in which collisions between Host IDs and tokens are resolved, so it contains only entries that correspond to the "real" current set of NORMAL nodes. We use this method to calculate the set of nodes passed to `_gossiper.wait_alive`. Fixes scylladb#14468 Fixes scylladb#14487
kbr-scylla
added a commit
to kbr-scylla/scylladb
that referenced
this issue
Jul 4, 2023
Regression test for scylladb#14468.
kbr-scylla
added a commit
to kbr-scylla/scylladb
that referenced
this issue
Jul 6, 2023
…rmal handlers Before this commit the `wait_for_normal_state_handled_on_boot` would wait for a static set of nodes (`sync_nodes`), calculated using the `get_nodes_to_sync_with` function and `parse_node_list`; the latter was used to obtain a list of "nodes to ignore" (for replace operation) and translate them, using `token_metadata`, from IP addresses to Host IDs and vice versa. `sync_nodes` was also used in `_gossiper.wait_alive` call which we do after `wait_for_normal_state_handled_on_boot`. Recently we started doing these calculations and this wait very early in the boot procedure - immediately after we start gossiping (50e8ec7). Unfortunately, as always with gossiper, there are complications. In scylladb#14468 and scylladb#14487 two problems were detected: - Gossiper may contain obsolete entries for nodes which were recently replaced or changed their IPs. These entries are still using status `NORMAL` or `shutdown` (which is treated like `NORMAL`, e.g. `handle_state_normal` is also called for it). The `_gossiper.wait_alive` call would wait for those entries too and eventually time out. - Furthermore, by the time we call `parse_node_list`, `token_metadata` may not be populated yet, which is required to do the IP<->Host ID translations -- and populating `token_metadata` happens inside `handle_state_normal`, so we have a chicken-and-egg problem here. The `parse_node_list` problem is solved in this commit. It turns out that we don't need to calculate `sync_nodes` (and hence `ignore_nodes`) in order to wait for NORMAL state handlers. We can wait for handlers to finish for *any* `NORMAL`/`shutdown` entries appearing in gossiper, even those that correspond to dead/ignored nodes and obsolete IPs. `handle_state_normal` is called, and eventually finishes, for all of them. `wait_for_normal_state_handled_on_boot` no longer receives a set of nodes as parameter and is modified appropriately, it's now calculating the necessary set of nodes on each retry (the set may shrink while we're waiting, e.g. because an entry corresponding to a node that was replaced is garbage-collected from gossiper state). Thanks to this, we can now put the `sync_nodes` calculation (which is still necessary for `_gossiper.wait_alive`), and hence the `parse_node_list` call, *after* we wait for NORMAL state handlers, solving the chickend-and-egg problem. This addresses the immediate failure described in scylladb#14487, but the test will still fail. That's because `_gossiper.wait_alive` may still receive a too large set of nodes -- we may still include obsolete IPs or entries corresponding to replaced nodes in the `sync_nodes` set. We fix this in the following commit which will solve both issues.
kbr-scylla
added a commit
to kbr-scylla/scylladb
that referenced
this issue
Jul 6, 2023
Regression test for scylladb#14468.
tgrabiec
added a commit
that referenced
this issue
Jul 9, 2023
…es, recently replaced nodes, and recently changed IPs' from Kamil Braun Before this PR, the `wait_for_normal_state_handled_on_boot` would wait for a static set of nodes (`sync_nodes`), calculated using the `get_nodes_to_sync_with` function and `parse_node_list`; the latter was used to obtain a list of "nodes to ignore" (for replace operation) and translate them, using `token_metadata`, from IP addresses to Host IDs and vice versa. `sync_nodes` was also used in `_gossiper.wait_alive` call which we do after `wait_for_normal_state_handled_on_boot`. Recently we started doing these calculations and this wait very early in the boot procedure - immediately after we start gossiping (50e8ec7). Unfortunately, as always with gossiper, there are complications. In #14468 and #14487 two problems were detected: - Gossiper may contain obsolete entries for nodes which were recently replaced or changed their IPs. These entries are still using status `NORMAL` or `shutdown` (which is treated like `NORMAL`, e.g. `handle_state_normal` is also called for it). The `_gossiper.wait_alive` call would wait for those entries too and eventually time out. - Furthermore, by the time we call `parse_node_list`, `token_metadata` may not be populated yet, which is required to do the IP<->Host ID translations -- and populating `token_metadata` happens inside `handle_state_normal`, so we have a chicken-and-egg problem here. It turns out that we don't need to calculate `sync_nodes` (and hence `ignore_nodes`) in order to wait for NORMAL state handlers. We can wait for handlers to finish for *any* `NORMAL`/`shutdown` entries appearing in gossiper, even those that correspond to dead/ignored nodes and obsolete IPs. `handle_state_normal` is called, and eventually finishes, for all of them. `wait_for_normal_state_handled_on_boot` no longer receives a set of nodes as parameter and is modified appropriately, it's now calculating the necessary set of nodes on each retry (the set may shrink while we're waiting, e.g. because an entry corresponding to a node that was replaced is garbage-collected from gossiper state). Thanks to this, we can now put the `sync_nodes` calculation (which is still necessary for `_gossiper.wait_alive`), and hence the `parse_node_list` call, *after* we wait for NORMAL state handlers, solving the chickend-and-egg problem. This addresses the immediate failure described in #14487, but the test would still fail. That's because `_gossiper.wait_alive` may still receive a too large set of nodes -- we may still include obsolete IPs or entries corresponding to replaced nodes in the `sync_nodes` set. We need a better way to calculate `sync_nodes` which detects ignores obsolete IPs and nodes that are already gone but just weren't garbage-collected from gossiper state yet. In fact such a method was already introduced in the past: ca61d88 but it wasn't used everywhere. There, we use `token_metadata` in which collisions between Host IDs and tokens are resolved, so it contains only entries that correspond to the "real" current set of NORMAL nodes. We use this method to calculate the set of nodes passed to `_gossiper.wait_alive`. We also introduce regression tests with necessary extensions to the test framework. Fixes #14468 Fixes #14487 Closes #14507 * github.com:scylladb/scylladb: test: rename `test_topology_ip.py` to `test_replace.py` test: test bootstrap after IP change test: scylla_cluster: return the new IP from `change_ip` API test: node replace with `ignore_dead_nodes` test test: scylla_cluster: accept `ignore_dead_nodes` in `ReplaceConfig` storage_service: remove `get_nodes_to_sync_with` storage_service: use `token_metadata` to calculate nodes waited for to be UP storage_service: don't calculate `ignore_nodes` before waiting for normal handlers
syuu1228
pushed a commit
to syuu1228/scylla
that referenced
this issue
Jul 10, 2023
…rmal handlers Before this commit the `wait_for_normal_state_handled_on_boot` would wait for a static set of nodes (`sync_nodes`), calculated using the `get_nodes_to_sync_with` function and `parse_node_list`; the latter was used to obtain a list of "nodes to ignore" (for replace operation) and translate them, using `token_metadata`, from IP addresses to Host IDs and vice versa. `sync_nodes` was also used in `_gossiper.wait_alive` call which we do after `wait_for_normal_state_handled_on_boot`. Recently we started doing these calculations and this wait very early in the boot procedure - immediately after we start gossiping (50e8ec7). Unfortunately, as always with gossiper, there are complications. In scylladb#14468 and scylladb#14487 two problems were detected: - Gossiper may contain obsolete entries for nodes which were recently replaced or changed their IPs. These entries are still using status `NORMAL` or `shutdown` (which is treated like `NORMAL`, e.g. `handle_state_normal` is also called for it). The `_gossiper.wait_alive` call would wait for those entries too and eventually time out. - Furthermore, by the time we call `parse_node_list`, `token_metadata` may not be populated yet, which is required to do the IP<->Host ID translations -- and populating `token_metadata` happens inside `handle_state_normal`, so we have a chicken-and-egg problem here. The `parse_node_list` problem is solved in this commit. It turns out that we don't need to calculate `sync_nodes` (and hence `ignore_nodes`) in order to wait for NORMAL state handlers. We can wait for handlers to finish for *any* `NORMAL`/`shutdown` entries appearing in gossiper, even those that correspond to dead/ignored nodes and obsolete IPs. `handle_state_normal` is called, and eventually finishes, for all of them. `wait_for_normal_state_handled_on_boot` no longer receives a set of nodes as parameter and is modified appropriately, it's now calculating the necessary set of nodes on each retry (the set may shrink while we're waiting, e.g. because an entry corresponding to a node that was replaced is garbage-collected from gossiper state). Thanks to this, we can now put the `sync_nodes` calculation (which is still necessary for `_gossiper.wait_alive`), and hence the `parse_node_list` call, *after* we wait for NORMAL state handlers, solving the chickend-and-egg problem. This addresses the immediate failure described in scylladb#14487, but the test will still fail. That's because `_gossiper.wait_alive` may still receive a too large set of nodes -- we may still include obsolete IPs or entries corresponding to replaced nodes in the `sync_nodes` set. We fix this in the following commit which will solve both issues.
syuu1228
pushed a commit
to syuu1228/scylla
that referenced
this issue
Jul 10, 2023
…o be UP At bootstrap, after we start gossiping, we calculate a set of nodes (`sync_nodes`) which we need to "synchronize" with, waiting for them to be UP before proceeding; these nodes are required for streaming/repair and CDC generation data write, and generally are supposed to constitute the current set of cluster members. In scylladb#14468 and scylladb#14487 we observed that this set may calculate entries corresponding to nodes that were just replaced or changed their IPs (but the old-IP entry is still there). We pass them to `_gossiper.wait_alive` and the call eventually times out. We need a better way to calculate `sync_nodes` which detects ignores obsolete IPs and nodes that are already gone but just weren't garbage-collected from gossiper state yet. In fact such a method was already introduced in the past: ca61d88 but it wasn't used everywhere. There, we use `token_metadata` in which collisions between Host IDs and tokens are resolved, so it contains only entries that correspond to the "real" current set of NORMAL nodes. We use this method to calculate the set of nodes passed to `_gossiper.wait_alive`. Fixes scylladb#14468 Fixes scylladb#14487
syuu1228
pushed a commit
to syuu1228/scylla
that referenced
this issue
Jul 10, 2023
Regression test for scylladb#14468.
bhalevy
added a commit
to bhalevy/scylla
that referenced
this issue
Jul 24, 2023
When a host changes its ip address we should force remove the previous endpoint state since we want only one endpoint to refer to this host_id. If the new node that changed its ip address is decommissioned, the previous node seems as a normal token owner, just in shutdown status, but it is not longer in the cluster. Fixes scylladb#14468 Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
bhalevy
added a commit
to bhalevy/scylla
that referenced
this issue
Aug 3, 2023
When a host changes its ip address we should force remove the previous endpoint state since we want only one endpoint to refer to this host_id. If the new node that changed its ip address is decommissioned, the previous node seems as a normal token owner, just in shutdown status, but it is not longer in the cluster. Fixes scylladb#14468 Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
bhalevy
added a commit
to bhalevy/scylla
that referenced
this issue
Aug 6, 2023
When a host changes its ip address we should force remove the previous endpoint state since we want only one endpoint to refer to this host_id. If the new node that changed its ip address is decommissioned, the previous node seems as a normal token owner, just in shutdown status, but it is not longer in the cluster. Refs scylladb#14468 Fixes scylladb#13775 Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
bhalevy
added a commit
to bhalevy/scylla
that referenced
this issue
Aug 6, 2023
When a host changes its ip address we should force remove the previous endpoint state since we want only one endpoint to refer to this host_id. If the new node that changed its ip address is decommissioned, the previous node seems as a normal token owner, just in shutdown status, but it is not longer in the cluster. Refs scylladb#14468 Fixes scylladb#13775 Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
syuu1228
pushed a commit
to syuu1228/scylla
that referenced
this issue
Aug 28, 2023
…rmal handlers Before this commit the `wait_for_normal_state_handled_on_boot` would wait for a static set of nodes (`sync_nodes`), calculated using the `get_nodes_to_sync_with` function and `parse_node_list`; the latter was used to obtain a list of "nodes to ignore" (for replace operation) and translate them, using `token_metadata`, from IP addresses to Host IDs and vice versa. `sync_nodes` was also used in `_gossiper.wait_alive` call which we do after `wait_for_normal_state_handled_on_boot`. Recently we started doing these calculations and this wait very early in the boot procedure - immediately after we start gossiping (50e8ec7). Unfortunately, as always with gossiper, there are complications. In scylladb#14468 and scylladb#14487 two problems were detected: - Gossiper may contain obsolete entries for nodes which were recently replaced or changed their IPs. These entries are still using status `NORMAL` or `shutdown` (which is treated like `NORMAL`, e.g. `handle_state_normal` is also called for it). The `_gossiper.wait_alive` call would wait for those entries too and eventually time out. - Furthermore, by the time we call `parse_node_list`, `token_metadata` may not be populated yet, which is required to do the IP<->Host ID translations -- and populating `token_metadata` happens inside `handle_state_normal`, so we have a chicken-and-egg problem here. The `parse_node_list` problem is solved in this commit. It turns out that we don't need to calculate `sync_nodes` (and hence `ignore_nodes`) in order to wait for NORMAL state handlers. We can wait for handlers to finish for *any* `NORMAL`/`shutdown` entries appearing in gossiper, even those that correspond to dead/ignored nodes and obsolete IPs. `handle_state_normal` is called, and eventually finishes, for all of them. `wait_for_normal_state_handled_on_boot` no longer receives a set of nodes as parameter and is modified appropriately, it's now calculating the necessary set of nodes on each retry (the set may shrink while we're waiting, e.g. because an entry corresponding to a node that was replaced is garbage-collected from gossiper state). Thanks to this, we can now put the `sync_nodes` calculation (which is still necessary for `_gossiper.wait_alive`), and hence the `parse_node_list` call, *after* we wait for NORMAL state handlers, solving the chickend-and-egg problem. This addresses the immediate failure described in scylladb#14487, but the test will still fail. That's because `_gossiper.wait_alive` may still receive a too large set of nodes -- we may still include obsolete IPs or entries corresponding to replaced nodes in the `sync_nodes` set. We fix this in the following commit which will solve both issues.
syuu1228
pushed a commit
to syuu1228/scylla
that referenced
this issue
Aug 28, 2023
…o be UP At bootstrap, after we start gossiping, we calculate a set of nodes (`sync_nodes`) which we need to "synchronize" with, waiting for them to be UP before proceeding; these nodes are required for streaming/repair and CDC generation data write, and generally are supposed to constitute the current set of cluster members. In scylladb#14468 and scylladb#14487 we observed that this set may calculate entries corresponding to nodes that were just replaced or changed their IPs (but the old-IP entry is still there). We pass them to `_gossiper.wait_alive` and the call eventually times out. We need a better way to calculate `sync_nodes` which detects ignores obsolete IPs and nodes that are already gone but just weren't garbage-collected from gossiper state yet. In fact such a method was already introduced in the past: ca61d88 but it wasn't used everywhere. There, we use `token_metadata` in which collisions between Host IDs and tokens are resolved, so it contains only entries that correspond to the "real" current set of NORMAL nodes. We use this method to calculate the set of nodes passed to `_gossiper.wait_alive`. Fixes scylladb#14468 Fixes scylladb#14487
syuu1228
pushed a commit
to syuu1228/scylla
that referenced
this issue
Aug 28, 2023
Regression test for scylladb#14468.
|
@bhalevy @kbr-scylla do we need to backport this to 5.2? If yes, please open a backport PR. |
|
This issue was caused by 50e8ec7 which did not end up in 5.2, so no. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Seen first in https://jenkins.scylladb.com/job/scylla-master/job/dtest-daily-release/290/testReport/update_cluster_layout_tests/TestUpdateClusterLayout/Run_Dtest_Parallel_Cloud_Machines___FullDtest___full_split010___test_decommission_after_changing_node_ip/
https://jenkins.scylladb.com/job/scylla-master/job/dtest-daily-release/290/artifact/logs-full.release.010/1688093180144_update_cluster_layout_tests.py%3A%3ATestUpdateClusterLayout%3A%3Atest_decommission_after_changing_node_ip/node4.log
The text was updated successfully, but these errors were encountered: