fix(nemesis): filter raft-topology errors when starting/stopping nodes #9580

timtimb0t · 2024-12-18T09:47:30Z

raft is generating the following errors:
raft_topology - topology change coordinator fiber got error std::runtime_error
when one of the nodes is stopped, it was decided we can safely ignore those errors

fixes: #9031

PR pre-checks (self review)

I added the relevant backport labels
I didn't leave commented-out/debugging code

Reminders

Add New configuration option and document them (in sdcm/sct_config.py)
Add unit tests to cover my changes (under unit-test/ folder)
Update the Readme/doc folder relevant to this change (if needed)

sdcm/nemesis.py

aleksbykov · 2024-12-18T10:09:31Z

sdcm/nemesis.py

+        with ignore_raft_topology_cmd_failing():
+            self.target_node.stop_scylla_server(verify_up=False, verify_down=True)


Here i think better to use Context manager only for operations stop/start

why ?

@decorate_with_context that already exists on this function is a natural fit for this, and was design for those cases exactly

aleksbykov

LGTM

aleksbykov · 2024-12-18T11:43:57Z

@timtimb0t , fix only precommits

soyacz

Please add description to commit why this change is needed, so when reviewing in git blane one does not have to peek into gh.

Besides, is it required for every Scylla restart or only in places when there's a wait between stop and start? If so, I suppose there could be more places needed to adjust.

soyacz · 2024-12-18T12:42:43Z

sdcm/nemesis.py

@@ -1152,7 +1153,8 @@ def _destroy_data_and_restart_scylla(self, keyspaces_for_destroy: list = None, s
        self.log.debug("Chosen tables: %s", tables)

        # Stop scylla service before deleting sstables to avoid partial deletion of files that are under compaction
-        self.target_node.stop_scylla_server(verify_up=False, verify_down=True)
+        with ignore_raft_topology_cmd_failing():
+            self.target_node.stop_scylla_server(verify_up=False, verify_down=True)

        try:


shouldn't this block be inside context?

shouldn't this block be inside context?

I thought the same, but after a short conversation with @aleksbykov, I decided to do everything as is, because once the node is stopped, the cluster will recognize this and won’t send any more requests to it.

timtimb0t · 2024-12-18T12:55:53Z

Please add description to commit why this change is needed, so when reviewing in git blane one does not have to peek into gh.

Besides, is it required for every Scylla restart or only in places when there's a wait between stop and start? If so, I suppose there could be more places needed to adjust.

You are right; there may be other places that need a similar change, but this fix is a response to the current issue. If such an error occurs again, we will likely need a new PR.

fruch · 2024-12-18T21:05:12Z

sdcm/nemesis.py

@@ -675,14 +675,15 @@ def _kill_scylla_daemon(self):

    @target_all_nodes
    def disrupt_stop_wait_start_scylla_server(self, sleep_time=300):  # pylint: disable=invalid-name


I would recommend using @decorate_with_context, it would be less of interruption to the code

fruch · 2024-12-18T21:16:09Z

Please add description to commit why this change is needed, so when reviewing in git blane one does not have to peek into gh.

Besides, is it required for every Scylla restart or only in places when there's a wait between stop and start? If so, I suppose there could be more places needed to adjust.

Yes commit should always have description, also as for the title
sct lib doesn't mean much, in this case fix(nemesis): ...

as for the description should be something more like

fix(nemesis): filter raft-topology errors when starting/stopping nodes

raft is generating the following errors:

[ one example of such print ]

when one of the nodes is stopped, it was decided we can safely ignore those errors

Ref: [scylla issue about how core would address that]
Fixes: #9031

in such manner people can understand the reasoning, and are don't need to go for walk across multiple other issue to understand it.

soyacz · 2024-12-19T09:00:18Z

@timtimb0t please add proper backport labels

raft is generating the following errors: raft_topology - topology change coordinator fiber got error std::runtime_error when one of the nodes is stopped, it was decided we can safely ignore those errors fixes:9031

fruch

LGTM

timtimb0t requested a review from aleksbykov December 18, 2024 09:47

github-actions bot assigned timtimb0t Dec 18, 2024

aleksbykov reviewed Dec 18, 2024

View reviewed changes

timtimb0t force-pushed the sct_raft_topology_errors_ignoring branch from 9cd2665 to 287dfd7 Compare December 18, 2024 11:30

aleksbykov previously approved these changes Dec 18, 2024

View reviewed changes

timtimb0t force-pushed the sct_raft_topology_errors_ignoring branch from 287dfd7 to 8ff4f7d Compare December 18, 2024 12:00

timtimb0t marked this pull request as ready for review December 18, 2024 12:02

timtimb0t requested a review from soyacz December 18, 2024 12:02

soyacz reviewed Dec 18, 2024

View reviewed changes

fruch reviewed Dec 18, 2024

View reviewed changes

timtimb0t dismissed aleksbykov’s stale review via 7ea69ad December 19, 2024 08:53

timtimb0t force-pushed the sct_raft_topology_errors_ignoring branch from 8ff4f7d to 7ea69ad Compare December 19, 2024 08:53

timtimb0t added backport/2024.2 Need backport to 2024.2 backport/6.1 Need backport to 6.1 backport/6.2 labels Dec 19, 2024

timtimb0t force-pushed the sct_raft_topology_errors_ignoring branch from 7ea69ad to 3bc8153 Compare December 19, 2024 09:41

timtimb0t changed the title ~~fix(sct lib):decorators added to ignore raft-topology errors~~ fix(nemesis): decorators added to ignore raft-topology errors Dec 19, 2024

timtimb0t force-pushed the sct_raft_topology_errors_ignoring branch from 3bc8153 to fa1a335 Compare December 19, 2024 16:17

fix(nemesis): filter raft-topology errors when starting/stopping nodes

1ff7b0a

raft is generating the following errors: raft_topology - topology change coordinator fiber got error std::runtime_error when one of the nodes is stopped, it was decided we can safely ignore those errors fixes:9031

timtimb0t force-pushed the sct_raft_topology_errors_ignoring branch from fa1a335 to 1ff7b0a Compare December 19, 2024 16:27

fruch changed the title ~~fix(nemesis): decorators added to ignore raft-topology errors~~ fix(nemesis): filter raft-topology errors when starting/stopping nodes Dec 19, 2024

fruch approved these changes Dec 19, 2024

View reviewed changes

fruch merged commit 2093714 into scylladb:master Dec 19, 2024
5 of 7 checks passed

scylladbbot added the promoted-to-master label Dec 19, 2024

This was referenced Dec 19, 2024

[Backport 6.1] fix(nemesis): filter raft-topology errors when starting/stopping nodes #9591

Merged

[Backport 6.2] fix(nemesis): filter raft-topology errors when starting/stopping nodes #9592

Merged

mergify bot mentioned this pull request Dec 19, 2024

[Backport 2024.2] fix(nemesis): filter raft-topology errors when starting/stopping nodes #9593

Merged

2 tasks

scylladbbot added backport/6.2-done backport/2024.2-done Commit backported to 2024.2 backport/6.1-done Commit backported to 6.1 and removed backport/6.2 backport/2024.2 Need backport to 2024.2 backport/6.1 Need backport to 6.1 labels Dec 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(nemesis): filter raft-topology errors when starting/stopping nodes #9580

fix(nemesis): filter raft-topology errors when starting/stopping nodes #9580

timtimb0t commented Dec 18, 2024 •

edited by fruch

Loading

aleksbykov Dec 18, 2024

fruch Dec 18, 2024

aleksbykov left a comment

aleksbykov commented Dec 18, 2024

soyacz left a comment

soyacz Dec 18, 2024

timtimb0t Dec 18, 2024

timtimb0t commented Dec 18, 2024

fruch Dec 18, 2024

fruch commented Dec 18, 2024

soyacz commented Dec 19, 2024

fruch left a comment

		with ignore_raft_topology_cmd_failing():
		self.target_node.stop_scylla_server(verify_up=False, verify_down=True)

		@@ -675,14 +675,15 @@ def _kill_scylla_daemon(self):

		@target_all_nodes
		def disrupt_stop_wait_start_scylla_server(self, sleep_time=300): # pylint: disable=invalid-name

fix(nemesis): filter raft-topology errors when starting/stopping nodes #9580

fix(nemesis): filter raft-topology errors when starting/stopping nodes #9580

Conversation

timtimb0t commented Dec 18, 2024 • edited by fruch Loading

PR pre-checks (self review)

Reminders

aleksbykov Dec 18, 2024

Choose a reason for hiding this comment

fruch Dec 18, 2024

Choose a reason for hiding this comment

aleksbykov left a comment

Choose a reason for hiding this comment

aleksbykov commented Dec 18, 2024

soyacz left a comment

Choose a reason for hiding this comment

soyacz Dec 18, 2024

Choose a reason for hiding this comment

timtimb0t Dec 18, 2024

Choose a reason for hiding this comment

timtimb0t commented Dec 18, 2024

fruch Dec 18, 2024

Choose a reason for hiding this comment

fruch commented Dec 18, 2024

soyacz commented Dec 19, 2024

fruch left a comment

Choose a reason for hiding this comment

timtimb0t commented Dec 18, 2024 •

edited by fruch

Loading