tablets: alter keyspace

[ptrsmrn]

This change supports changing replication factor in tablets-enabled keyspaces.
This covers both increasing and decreasing the number of tablets replicas through
first building topology mutations (alter_keyspace_statement.cc) and then
tablets/topology/schema mutations (topology_coordinator.cc).
For the limitations of the current solution, please see the docs changes attached to this PR.

Fixes: #16129

[scylladb-promoter]

🔴 CI State: FAILURE

❌ - Build

Build Details:

• Duration: 15 min
• Builder: spider6.cloudius-systems.com

[ptrsmrn]

Version 3:

• made the change compile'able and I'll try to keep it that way
• had to rebase on top of master, which was painful due to topology builder(s) refactor, but @piodul says he's going to resume his topology refactor PR
• I'll reply to the comments as for the other changes in this version, basically I am checking if topology state machine processes any other global req from alter_keyspace_statement.

[ptrsmrn]

Version 4:

1. Rebased on top of master, which eliminated my commit refactoring topology_builder to a separate fail thanks to https://github.com/scylladb/scylladb/pull/16609/files + I got tablets.transition thanks to https://github.com/scylladb/scylladb/pull/16894/files
2. Added tablet_transition_kind.rf_change, though it's not properly handled everywhere yet
3. Using wait_for_topology_request_completion() when altering tablets ks in storage_service::alter_tablets_keyspace
4. Moved altering tablets ks code from alter_keyspace_statement.cc to storage_service.cc to eliminate bringing too many dependencies into cql3 module (where alter_keyspace_statement.cc is).

CC @tgrabiec

[ptrsmrn]

Version 5:

1. resolve minor comments - mostly cleaned the code a bit
2. save rf change data as a topology mutation
3. add tablets migrations to topology coordinator

[ptrsmrn]

Version 6:

• Trying to compile, but hit qp vs ss init problems, see https://github.com/scylladb/scylladb/pull/16723/files#diff-bf1eacd22947b4daf9f4c2639427b8593d489f093eb1acfbba3e4cc1c9b0288bR1420 - of course simple reordering doesn't solve the issue
• minor fixes
• rebased with master

[ptrsmrn]

CC @tgrabiec

Version 7:

1. (there is some extra debug code that prints additional stuff, I'll remove it later).
2. the code compiles and runs, migrations seem to be constructed and run properly (with the exception that I am not really changing anything atm - the code that is going to add/remove tablets replicas will be included soon). What was fixed was mainly:

• deleting the global topology request once alter rf req is created
• I moved creating request id out of raft guard down in hopes to fix one of the bugs listed below but it didn't help
• removed a deadlock caused by creating a raft guard when a raft guard operation has already been started
• handled loading new topology table columns

4. There's a couple of bugs that still need to be fixed:

• the easy one is that I am not storing dc:rf map correctly in topology table, the value is empty
• another problem is that I get a stacktrace when waiting for topology request completion:

ERROR 2024-02-19 22:21:16,278 [shard 0:stmt] system_keyspace - no entry for request id d0d6d24a-cf6c-11ee-05a6-9c5353029f4f, at: 0xd4f530a /home/piotrs/src/scylla2/build/debug/seastar/libseastar.so+0x55f96ec
...
   seastar::internal::coroutine_traits_base<service::topology_request_state>::promise_type
   --------
   seastar::internal::coroutine_traits_base<seastar::basic_sstring<char, unsigned int, 15u, true> >::promise_type
   --------
   seastar::internal::coroutine_traits_base<void>::promise_type
   --------
   seastar::internal::coroutine_traits_base<seastar::shared_ptr<cql_transport::messages::result_message> >::promise_type
   --------
   seastar::internal::coroutine_traits_base<seastar::shared_ptr<cql_transport::messages::result_message> >::promise_type
   --------
   seastar::internal::coroutine_traits_base<seastar::shared_ptr<cql_transport::messages::result_message> >::promise_type
   --------
   seastar::internal::coroutine_traits_base<seastar::shared_ptr<cql_transport::messages::result_message> >::promise_type
...

• the other issue is that once I enter:

INFO  2024-02-19 22:21:16,283 [shard 0:strm] raft_topology - entered `tablet migration` transition state

I repeatedly (approx. each ~45-60 seconds) get the following (which should happen only once?):

INFO  2024-02-19 22:21:16,283 [shard 0:strm] load_balancer - Node 1dee4885-c35e-4499-aa89-e06c2823f736: rack=rack1 avg_load=0, tablets=0, shards=3, state=normal
INFO  2024-02-19 22:21:16,283 [shard 0:strm] load_balancer - Prepared 0 migrations in DC datacenter1
INFO  2024-02-19 22:21:16,283 [shard 0:strm] load_balancer - Prepared 0 migration plans, out of which there were 0 tablet migration(s) and 0 resize decision(s)
INFO  2024-02-19 22:21:16,283 [shard 0:strm] raft_topology - updating topology state: Finished tablet migration

[ptrsmrn]

My guess is that waiting for topology request completion doesn't work, because this function doesn't handle global requests.

[xemul]

@ptrsmrn , regarding #18772 -- once the latter is merged, test cases checking how ALTER works from this PR would stop passing, won't they?

[ptrsmrn]

@ptrsmrn , regarding #18772 -- once the latter is merged, test cases checking how ALTER works from this PR would stop passing, won't they?

Yes. There are many ways to keep things working, I guess, but the way I imagined it working is that #18772 should be merged to 6.0 only, and this PR should be merged to master (which will become 6.1 eventually) and - maybe - backported to 6.0. Of course backporting this PR to 6.0 means #18772 would need to be reverted from 6.0.

The easiest would be to drop #18772, merge this PR to master and then backport to 6.0, but this is against plans of @mykaul.

[xemul]

If @mykaul 's plan is to ban ALTER for 6.0, then #18772 should be considered for 6.0 branch only

[mykaul]

If @mykaul 's plan is to ban ALTER for 6.0, then #18772 should be considered for 6.0 branch only

Nit: it's not my plan, it's our plan.

[scylladb-promoter]

🟢 CI State: SUCCESS

✅ - Build
✅ - Unit Tests Custom
The following new/updated tests ran 100 times for each mode:
🔹 boost/network_topology_strategy_test
🔹 boost/tablets_test
🔹 boost/transport_test
🔹 cql-pytest/test_keyspace
🔹 cql-pytest/test_tablets
🔹 topology_custom/test_tablets
✅ - Container Test
✅ - dtest with topology changes
✅ - dtest
✅ - Unit Tests

Build Details:

• Duration: 7 hr 43 min
• Builder: i-0d7e7786c918fc829 (m5ad.12xlarge)

[xemul]

If @mykaul 's plan is to ban ALTER for 6.0, then #18772 should be considered for 6.0 branch only

Nit: it's not my plan, it's our plan.

Absolutely. However (quoting)

so do we merge ALTER rejection to 6.0 only, or does it go via master as well, (or is it up to maintainers to decide on their own)?

[xemul]

For the record -- this is to be backported to 6.0 once it hits the master

[mykaul]

If @mykaul 's plan is to ban ALTER for 6.0, then #18772 should be considered for 6.0 branch only

Nit: it's not my plan, it's our plan.

Absolutely. However (quoting)

so do we merge ALTER rejection to 6.0 only, or does it go via master as well, (or is it up to maintainers to decide on their own)?

We've discussed it on the daily call. We'll take the change RF to 6.0. Reject ALTER is not needed at this point.

[xemul]

Backported into 6.0
Had to pick #18644 as well