scylla_node: watch_rest_for_alive: wait for others to be considered normal token owners #523

bhalevy · 2023-11-08T16:11:10Z

It is not enough for node to know about other nodes' tokens, as they might not be reflected in the token_metadata map.

Instead, check the /storage_service/host_id api that provides a list of nodes that are normal token owners and ready to be used by queries.

Refs scylladb/scylladb#15146

ccmlib/scylla_node.py

bhalevy · 2023-11-09T06:05:37Z

@nyh, @fruch please review

ccmlib/scylla_node.py

fruch · 2023-11-09T06:56:43Z

Seems o.k.

But I'll want to take a ride in at least in gating

fruch · 2023-11-09T08:38:43Z

@bhalevy

giving it a go:

https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/134/ (OSS)
https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/135/ (OSS + raft)
https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/136/ (Enterprise)
https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/137/ (Enterprise + raft)

fruch · 2023-11-09T11:25:50Z

@bhalevy

giving it a go:

https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/134/ (OSS)

https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/135/ (OSS + raft)

https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/136/ (Enterprise)

https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/137/ (Enterprise + raft)

@bhalevy seems like we are getting None as host_id all over the place

bhalevy · 2023-11-09T13:54:53Z

@bhalevy seems like we are getting None as host_id all over the place

should be fixed now (at least it passes for me locally, for example update_cluster_layout_tests.py::TestUpdateClusterLayout::test_simple_removenode_3[api-True])

fruch · 2023-11-09T14:25:36Z

@bhalevy seems like we are getting None as host_id all over the place

should be fixed now (at least it passes for me locally, for example update_cluster_layout_tests.py::TestUpdateClusterLayout::test_simple_removenode_3[api-True])

running gating again:

https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/138/ (OSS)
https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/139/ (OSS + raft)
https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/140/ (Enterprise)
https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/141/ (Enterprise + raft)

bhalevy · 2023-11-09T19:43:27Z

scylla_node: start_scylla: reset node_hostid

so to force reload of the host_id after restart as the node may wake up with a different host_id (after being wiped)

bhalevy · 2023-11-09T19:44:21Z

https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/142/

bhalevy · 2023-11-10T19:29:25Z

Depends on https://github.com/scylladb/scylla-dtest/pull/3716

bhalevy · 2023-11-10T19:32:38Z

In 54332a9:

scylla_node: hostid: handle errors
scylla_node: watch_rest_for_alive: check response.status_code
scylla_node: watch_rest_for_alive: wait for others to be considered normal token owners:
- extends current function by waiting on tokens first, then on nrmal token ownership
scylla_node: watch_rest_for_alive: add wait_normal_token_owner param
- for backward compatibility
- applied also from ScyllaCluster::start_nodes

fruch · 2023-11-12T06:51:36Z

giving one last run, this is some that gonna happen on every single test:

https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/143/ (OSS)
https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/144/ (OSS + raft)
https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/145/ (Enterprise)
https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/146/ (Enterprise + raft)

@bhalevy also keep in mind it might break non gating tests...
we'll need to keep that in mind following this merge

bhalevy · 2023-11-12T07:09:03Z

@bhalevy also keep in mind it might break non gating tests...
we'll need to keep that in mind following this merge

Right. That's why I added the wait_normal_token_owner flag.

fruch · 2023-11-12T09:14:29Z

@bhalevy also keep in mind it might break non gating tests...
we'll need to keep that in mind following this merge

Right. That's why I added the wait_normal_token_owner flag.

And which tests are gonna use that ? Only one we will be unstable?

bhalevy · 2023-11-12T09:25:55Z

@bhalevy also keep in mind it might break non gating tests...
we'll need to keep that in mind following this merge

Right. That's why I added the wait_normal_token_owner flag.

And which tests are gonna use that ? Only one we will be unstable?

Yes. This category of tests monitor starting nodes' log looking for particular events.
In most cases we already call start with wait_other_notice=False, wait_for_binary_protocol=False, but in cases we didn't do that we'll have to figure out what's the right thing to do.

bhalevy · 2023-11-12T09:27:23Z

giving one last run, this is some that gonna happen on every single test:

https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/143/ (OSS)

https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/144/ (OSS + raft)

https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/145/ (Enterprise)

https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/146/ (Enterprise + raft)

There's only https://jenkins.scylladb.com/job/scylla-staging/job/dtest-pytest-gating/146/testReport/junit/cql_tests/TestTruncate/FullDtest___full_split000___test_cql_query_filtering_without_indexes/
which seems unrelated. Need to analyse it further

bhalevy · 2023-11-12T09:38:34Z

This is a test issue: https://github.com/scylladb/scylla-dtest/issues/3717

bhalevy · 2023-11-13T09:56:51Z

@nyh since you're the last to change watch_rest_for_alive, any comments before we merge this change?

fruch

LGTM

bhalevy · 2023-11-20T07:17:14Z

@nyh please review (since you were the last one to change this area)

ccmlib/scylla_node.py

nyh · 2023-11-27T12:33:13Z

ccmlib/scylla_node.py

+                return self.node_hostid
+        except Exception as e:
+            self.error(f"Failed to get hostid using {url}: {e}")
+            pass


‎This "pass" isn't needed.

True, but I find it clarifying, as it documents the fact that the error is ignored on purpose.
Otherwise, a naive reader might suspect there is a missing raise.

dropped pass in next version

nyh · 2023-11-27T12:42:09Z

ccmlib/scylla_node.py

                if tofind.issubset(live):
                    # This node thinks that all given nodes are alive and not
                    # "joining", we're almost done, but still need to verify
                    # that the node knows the others' tokens.
                    check = tofind
-                    tofind = set()
+                    have_no_tokens = set()


Nitpick: The original code, after checking that node X does have tokens, removed it from the "tofind" list, so no need to check it again in the next iteration. Your rewrite lost this optimization. But I agree it's not a very important optimization.

I'll see what it takes to preserve that.

I restored the trimming of tofind in the next version (in a different way)

ccmlib/scylla_cluster.py

bhalevy · 2023-11-30T09:37:51Z

In 5fb352a:

scylla_node: override hostid() using the REST api
- used timeout arg for requests.get
scylla_node: start_scylla: reset node_hostid
- added code comment explaining why we do that
scylla_node: hostid: handle errors
- dropped pass statement
scylla_node: watch_rest_for_alive: wait for others to be considered normal token owners
- trim tofind as nodes are found
- but maintain cummulative found set and found_host_id_map dict for debugging purposes
scylla_node: watch_rest_for_alive: add wait_normal_token_owner param
- Document wait_normal_token_owner param also in watch_rest_for_alive

bhalevy · 2023-12-06T12:45:06Z

@nyh please re-review.
I addressed your review comments.

bhalevy · 2023-12-12T12:29:16Z

@nyh please re-review. I addressed your review comments.

@nyh ping, when do you think you can re-review?

nyh · 2023-12-19T07:38:56Z

ccmlib/scylla_node.py

+                        tofind = tofind.difference(normal)
+                        if not tofind:
+                            return
+                        # Update cummulative maps for debugging


spelling error: cummulative

ccmlib/scylla_cluster.py

For scylla, it is better to retrieve the node host_id using the REST api as it doesn't require scylla-jmx to serve `nodetool`. This allows us to get the node's host_id much earlier in its start process, as soone as it starts to server the REST API. Signed-off-by: Benny Halevy <[email protected]>

So it will be retrieved again once the node restarts since it might restart with a different host_id than it previously had if it was wiped and reused for bootstrap / replace. Signed-off-by: Benny Halevy <[email protected]>

If `hostid()` is called too early, it may fail as follows: ``` requests.exceptions.ConnectionError: HTTPConnectionPool(host='127.0.89.5', port=10000): Max retries exceeded with url: /storage_service/hostid/local (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7fa20680aa50>: Failed to establish a new connection: [Errno 111] Connection refused')) ``` Just print the error and return None to indicate that the hostid is unknown. Signed-off-by: Benny Halevy <[email protected]>

The correct way to determine if the http request succeeded is by checking response.status_code == requests.codes.ok. Signed-off-by: Benny Halevy <[email protected]>

…ormal token owners It is not enough for node to know about other nodes' tokens, as they might not be reflected in the token_metadata map. After checking tokens, check also the `/storage_service/host_id` api that provides a list of nodes that are normal token owners and ready to be used by queries. Refs scylladb/scylladb#15146 Signed-off-by: Benny Halevy <[email protected]>

For backward compatibility. Some tests may want to pass `node.start(wait_other_notice=True)` and not wait for nodes to become normal token owners if they need to examine the node earlier than that. Signed-off-by: Benny Halevy <[email protected]>

bhalevy · 2023-12-19T15:03:47Z

fixed spelling mistake and rebased
@fruch please merge when CI passed

bhalevy · 2023-12-19T21:59:48Z

All checks have passed, please merge

bhalevy · 2024-01-01T13:05:23Z

@fruch can you please backport to 5.4 and 2024.1?

mykaul · 2024-01-01T13:12:12Z

This kinda reminds a (very) long discussion with the operator team, where they did not know when can they safely do a rolling restart of Scylla pods - and the request was a simple API to let them know 'yes, it's real, this node is really up and serving traffic and is part of the cluster and you can safely move to the next node and restart it'.
See scylladb/scylladb#8275

bhalevy requested review from fruch and nyh November 8, 2023 16:11

bhalevy commented Nov 8, 2023

View reviewed changes

ccmlib/scylla_node.py Outdated Show resolved Hide resolved

bhalevy force-pushed the scylla_node-watch_rest_for_alive-normal-token-owners branch from e6f05bc to 9f97400 Compare November 9, 2023 06:05

fruch reviewed Nov 9, 2023

View reviewed changes

ccmlib/scylla_node.py Outdated Show resolved Hide resolved

bhalevy force-pushed the scylla_node-watch_rest_for_alive-normal-token-owners branch from 9f97400 to ca9352d Compare November 9, 2023 13:50

bhalevy force-pushed the scylla_node-watch_rest_for_alive-normal-token-owners branch from ca9352d to 34712cd Compare November 9, 2023 19:42

bhalevy force-pushed the scylla_node-watch_rest_for_alive-normal-token-owners branch from 34712cd to 54332a9 Compare November 10, 2023 19:29

fruch approved these changes Nov 13, 2023

View reviewed changes

nyh requested changes Nov 27, 2023

View reviewed changes

bhalevy force-pushed the scylla_node-watch_rest_for_alive-normal-token-owners branch from 54332a9 to 5fb352a Compare November 30, 2023 09:34

bhalevy requested a review from nyh December 3, 2023 10:57

nyh approved these changes Dec 19, 2023

View reviewed changes

bhalevy added 6 commits December 19, 2023 17:02

scylla_node: start_scylla: reset node_hostid

21db59b

So it will be retrieved again once the node restarts since it might restart with a different host_id than it previously had if it was wiped and reused for bootstrap / replace. Signed-off-by: Benny Halevy <[email protected]>

scylla_node: watch_rest_for_alive: check response.status_code

ef760f7

The correct way to determine if the http request succeeded is by checking response.status_code == requests.codes.ok. Signed-off-by: Benny Halevy <[email protected]>

bhalevy force-pushed the scylla_node-watch_rest_for_alive-normal-token-owners branch from 5fb352a to 1d226d2 Compare December 19, 2023 15:03

fruch merged commit 1411230 into scylladb:next Dec 19, 2023
3 checks passed

fruch added the 5.4-backported label Mar 6, 2024

lkshminarayanan mentioned this pull request May 14, 2024

[Backport 5.4] : Reload reclaimed bloom filters when memory is available scylladb/scylladb#18660

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scylla_node: watch_rest_for_alive: wait for others to be considered normal token owners #523

scylla_node: watch_rest_for_alive: wait for others to be considered normal token owners #523

bhalevy commented Nov 8, 2023

bhalevy commented Nov 9, 2023

fruch commented Nov 9, 2023

fruch commented Nov 9, 2023

fruch commented Nov 9, 2023

bhalevy commented Nov 9, 2023 •

edited

Loading

fruch commented Nov 9, 2023

bhalevy commented Nov 9, 2023 •

edited

Loading

bhalevy commented Nov 9, 2023

bhalevy commented Nov 10, 2023

bhalevy commented Nov 10, 2023

fruch commented Nov 12, 2023

bhalevy commented Nov 12, 2023 •

edited

Loading

fruch commented Nov 12, 2023

bhalevy commented Nov 12, 2023

bhalevy commented Nov 12, 2023

bhalevy commented Nov 12, 2023

bhalevy commented Nov 13, 2023

fruch left a comment

bhalevy commented Nov 20, 2023

nyh Nov 27, 2023

bhalevy Nov 29, 2023

bhalevy Nov 30, 2023

nyh Nov 27, 2023

bhalevy Nov 29, 2023

bhalevy Nov 30, 2023

bhalevy commented Nov 30, 2023

bhalevy commented Dec 6, 2023 •

edited

Loading

bhalevy commented Dec 12, 2023

nyh Dec 19, 2023

bhalevy commented Dec 19, 2023

bhalevy commented Dec 19, 2023

bhalevy commented Jan 1, 2024

mykaul commented Jan 1, 2024

scylla_node: watch_rest_for_alive: wait for others to be considered normal token owners #523

scylla_node: watch_rest_for_alive: wait for others to be considered normal token owners #523

Conversation

bhalevy commented Nov 8, 2023

bhalevy commented Nov 9, 2023

fruch commented Nov 9, 2023

fruch commented Nov 9, 2023

fruch commented Nov 9, 2023

bhalevy commented Nov 9, 2023 • edited Loading

fruch commented Nov 9, 2023

bhalevy commented Nov 9, 2023 • edited Loading

bhalevy commented Nov 9, 2023

bhalevy commented Nov 10, 2023

bhalevy commented Nov 10, 2023

fruch commented Nov 12, 2023

bhalevy commented Nov 12, 2023 • edited Loading

fruch commented Nov 12, 2023

bhalevy commented Nov 12, 2023

bhalevy commented Nov 12, 2023

bhalevy commented Nov 12, 2023

bhalevy commented Nov 13, 2023

fruch left a comment

Choose a reason for hiding this comment

bhalevy commented Nov 20, 2023

nyh Nov 27, 2023

Choose a reason for hiding this comment

bhalevy Nov 29, 2023

Choose a reason for hiding this comment

bhalevy Nov 30, 2023

Choose a reason for hiding this comment

nyh Nov 27, 2023

Choose a reason for hiding this comment

bhalevy Nov 29, 2023

Choose a reason for hiding this comment

bhalevy Nov 30, 2023

Choose a reason for hiding this comment

bhalevy commented Nov 30, 2023

bhalevy commented Dec 6, 2023 • edited Loading

bhalevy commented Dec 12, 2023

nyh Dec 19, 2023

Choose a reason for hiding this comment

bhalevy commented Dec 19, 2023

bhalevy commented Dec 19, 2023

bhalevy commented Jan 1, 2024

mykaul commented Jan 1, 2024

bhalevy commented Nov 9, 2023 •

edited

Loading

bhalevy commented Nov 9, 2023 •

edited

Loading

bhalevy commented Nov 12, 2023 •

edited

Loading

bhalevy commented Dec 6, 2023 •

edited

Loading