fix(FullScan): choose non-running_nemesis node

[temichus]

this commit has the following changes

1 introduce common targed_node_lock mechanism
that can be used in nemesis and Scan operations

2 FullScan operation now run only on free of nemeses node

3 change all node.running_nemesis settings to use common methods
set/unset_running_nemesis from common targed_node_lock file (except unit tests)

fixes: #9284

Testing

• https://argus.scylladb.com/tests/scylla-cluster-tests/d1a01caa-4f7e-40c1-8c17-69c9a6882de4

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)

[temichus]

@fruch, @soyacz, @aleksbykov Coul you please take a look at this idea before I start testing it

[soyacz]

Generally LGTM, still needs testing as setting target node is messy (e.g. we set target node in disrupt_method_wrapper and unset it in run method.)

[soyacz]

_get_target_nodes anyway works the old way - never selecting any node that runs any nemesis, regardless of descriptiveness. So selecting node during nemesis startup will work the old way.
I'm not saying it's wrong, just noting it.

[fruch]

just remove the whole if

[fruch]

how does self.disruptive would work ?

all those are set on only on nemesis classes, not the Nemesis class.

[aleksbykov]

Short remarks.
Also, because running_nemesis is list now, need to be carefully with passing to (un)set_running_nemesis function name of nemesis.

[aleksbykov]

i think better have is_disruptive=False as in set_running_nemesis

[aleksbykov]

may be it is not needed at all

[temichus]

@soyacz @fruch @aleksbykov, Thank you for your comments. I've changed the logic to choose only a non running_nemesis node for run_nemesis, and it will help to do FullScan safe when several nemesis are running. But I don't think it will help for RollingRestartCluster because it 'disrupts' all nodes in the cluster. What do you think regarding updating disrupt_rolling_restart_cluster to restart only non running_nemesis nodes?

[fruch]

@temichus

have you tried just raising the timout and connection_timeout for the client used for the fullscans ?
so it can tolerate for nodes being down ?

in cassandra-stress and all other tools we have retries for this prouposes, do fullscan have them ?

(FYI, you took this yourself, I didn't seen this one before you raised this PR)

[temichus]

@temichus

have you tried just raising the timout and connection_timeout for the client used for the fullscans ? so it can tolerate for nodes being down ?

in cassandra-stress and all other tools we have retries for this prouposes, do fullscan have them ?

(FYI, you took this yourself, I didn't seen this one before you raised this PR)

@fruch

I tried.

2024-12-09 14:14:30.612: (FullScanAggregateEvent Severity.ERROR) period_type=end event_id=fb92eb18-41e3-4bf0-8113-38bb4165e433 during_nemesis=RollingRestartCluster duration=40m0s node=longevity-10gb-3h-node-ope-db-node-e76e436f-1 select_from=keyspace1.standard1 message=FullScanAggregatesOperation operation failed: OperationTimedOut('errors={<Host: 10.4.2.76:9042 eu-west>: <Error from server: code=0000 [Server error] message="mapreduce_service is shutting down">}, last_host=10.4.2.234:9042'),session execution timeout 2400 is exceeded

2024-12-09 12:54:30.158: (FullScanAggregateEvent Severity.ERROR) period_type=end event_id=b1c5ce17-c805-43cd-9dda-7b0cd1e3952a during_nemesis=RollingRestartCluster duration=5m2s node=longevity-10gb-3h-node-ope-db-node-e76e436f-3 select_from=keyspace1.standard1 message=NoHostAvailable('Unable to complete the operation against any hosts', {<Host: 10.4.2.76:9042 eu-west>: ConnectionShutdown('Connection to 10.4.2.76:9042 was closed'), <Host: 10.4.2.234:9042 eu-west>: ConnectionShutdown('Connection to 10.4.2.234:19042 was closed'), <Host: 10.4.1.48:9042 eu-west>: ConnectionShutdown('Connection to 10.4.1.48:19042 was closed')})

2024-12-09 15:43:42.270 <2024-12-09 15:43:42.246>: (DatabaseLogEvent Severity.ERROR) period_type=one-time event_id=b001cd25-cf7c-4069-a1eb-56bb6a741610: type=RUNTIME_ERROR regex=std::runtime_error line_number=12014 node=longevity-10gb-3h-node-ope-db-node-e76e436f-3
2024-12-09T15:43:42.246+00:00 longevity-10gb-3h-node-ope-db-node-e76e436f-3      !ERR | scylla[9204]:  [shard 0: gms] raft_topology - drain rpc failed, proceed to fence old writes: std::runtime_error (raft topology: exec_global_command(barrier_and_drain) failed with seastar::rpc::closed_error (connection is closed))

But I agree with you that the statement that "fullscan broken by rolling restart" sounds strange(maybe eleven like a bug), so will continue to investigate

[temichus]

@fruch @soyacz @aleksbykov

according to @fruch, I've back to solution with locking nodes 1 by one during RollingRestartCluster and increasing timouts

The last test run sows only 2 error messages but all nemesis pass

2024-12-10 13:54:40.470: (FullScanAggregateEvent Severity.ERROR) period_type=end event_id=5ff46034-9a64-4978-a370-a35effc5c1f4 during_nemesis=RollingRestartCluster duration=40m0s node=longevity-10gb-3h-node-ope-db-node-d1a01caa-3 select_from=keyspace1.standard1 message=FullScanAggregatesOperation operation failed: OperationTimedOut("errors={'Connection defunct by heartbeat': 'Client request timeout. See Session.execute[_async](timeout)'}, last_host=10.4.0.234:9042"),session execution timeout 2400 is exceeded

can be #8705

and

2024-12-10 11:22:38.945: (FullScanAggregateEvent Severity.ERROR) period_type=end event_id=eadc36fd-dd71-4b8c-adc5-46e1f6b664ff duration=1m56s node=longevity-10gb-3h-node-ope-db-node-d1a01caa-2 select_from=keyspace1.standard1 message=FullScanAggregatesOperation operation failed: Fullscan failed - 'mapreduce_service_requests_dispatched_to_other_nodes' was not triggered

is scylladb/scylladb#21578 - will change it to warning in the next PR

[fruch]

@temichus
https://github.com/scylladb/scylla-enterprise/issues/4740 is something happens in upgrade, I don't understand how it's related here...

[temichus]

@temichus scylladb/scylla-enterprise#4740 is something happens in upgrade, I don't understand how it's related here...

wrolg link scylladb/scylladb#21578

[fruch]

it's not the only place in SCT we have a rolling restart

[fruch]

• _enable_disable_table_encryption
• disrupt_mgmt_restore

[fruch]

maybe we should consider moving this logic into restart_scylla

[temichus]

node.restart_scylla and cluster.restart_scylla is used in many places(test_functional.py, mng_cli_test.py, audit.py, artifacts_test.py, stop_compaction_test.py). But the lock mechanism is needed only when we have nemeses(in longevity tests). So let's keep it for nemeses only(at least for now)

will update _enable_disable_table_encryption and disrupt_mgmt_restore

[roydahan]

First, does it solve the issue?

[temichus]

First, does it solve the issue?

yes, there is no conflict anymore between FullScan and RollingRestart. Each chooses only the "free" node and locks it while doing an operation on ti

[temichus]

after a discussion with @fruch lock mechanism is not the best because Nemeses just will not run and wait for the lock. we heed a solution to let full-scan know (somehow) that the node was rebooted during full-scan

cc @aleksbykov @roydahan

[roydahan]

I'm suggesting again, to have a set of errors we can ignore approved by relevant developer and apply it in all relevant nemesis.

[fruch]

I'm suggesting again, to have a set of errors we can ignore approved by relevant developer and apply it in all relevant nemesis.

1. This is about a failure in fullscan, not nemesis

2. We already have code there that ignore fullscan results nemesis targeted the node that was the coordinator.

3. The problem we are facing is, that in rolling restart you target all nodes, without letting other parts of the code know about it, so the full scan thread doesn't know it should ignore the failure.

Lock suggested in this PR might cause high changes the nemesis doing rolling restart to be skipped in parallel nemesis cases.

I suggested that we need some way to identify that nodes restarted during the fullscan operation, but this not trivial with current event system.

One other option, since fullscan is just a stress tool, we implemented in python.
Maybe we should take the same approach, and apply retries with exponential backkoff

[roydahan]

One other option, since fullscan is just a stress tool, we implemented in python.
Maybe we should take the same approach, and apply retries with exponential backkoff

This is an option but it may hide problems.

[roydahan]

We already have code there that ignore fullscan results nemesis targeted the node that was the coordinator.

So what's the problem to ignore also the "map reduce" specific error we are getting now?

[fruch]

We already have code there that ignore fullscan results nemesis targeted the node that was the coordinator.

So what's the problem to ignore also the "map reduce" specific error we are getting now?

The problem is that ignore logic kicked in only if fullscan was using the same node as the current nemesis target node. which isn't the case in rolling restarts, cause we target all nodes.

If we're gonna just ignore the validation of the map reduce metric, we might as well remove it completely.
But it's just one of the symptoms we are seeing, we have there also failures of the fullscan queries themselves.

[fruch]

One other option, since fullscan is just a stress tool, we implemented in python.
Maybe we should take the same approach, and apply retries with exponential backkoff

This is an option but it may hide problems.

How it's different from the problems it might hide in c-s when we are retrying ?

Our from current implemention that make anything into a warning if it's during a nemesis targeting the same node ?

We use retry when there's an inerit cause of failure which is expected, and we can specifically cross match.
Consider if we had implement fullscan as stress tools in multiple languages, we would have implement retries, wouldn't we ?

[roydahan]

The problem is that ignore logic kicked in only if fullscan was using the same node as the current nemesis target node. which isn't the case in rolling restarts, cause we target all nodes.

If we're gonna just ignore the validation of the map reduce metric, we might as well remove it completely. But it's just one of the symptoms we are seeing, we have there also failures of the fullscan queries themselves.

We don't need to ignore the validation, we just need to ignore that specific error we're getting now.
If there are more errors, we need to report issue about them and ignore them only if this is what's decided.
In that case, what's the problem to run full scans inside a context manager that ignores this error?

[roydahan]

How it's different from the problems it might hide in c-s when we are retrying ?

It's a background thread that one hardly notice if it runs or not.
E.g. when we had aggregation issue at field and we didn't notice.

Our from current implemention that make anything into a warning if it's during a nemesis targeting the same node ?

We use retry when there's an inerit cause of failure which is expected, and we can specifically cross match. Consider if we had implement fullscan as stress tools in multiple languages, we would have implement retries, wouldn't we ?

You may use retry which will hide 99% of the problems and you wouldn't know, can't tell from monitor or anywhere else.