[QUEUED scylladb next] service/raft: raft_group0_client: read on-disk an in-memory group0 upgrade atomically

[Commit Bot]

From: Kamil Braun <kbr...@scylladb.com>
Committer: Tomasz Grabiec <tgra...@scylladb.com>
Branch: next

service/raft: raft_group0_client: read on-disk an in-memory group0 upgrade atomically

`set_group0_upgrade_state` writes the on-disk state first, then
in-memory state second, both under a write lock.
`get_group0_upgrade_state` would only take the lock if the in-memory
state was `use_pre_raft_procedures`.

If there's an external observer who watches the on-disk state to decide
whether Raft upgrade finished yet, the following could happen:
1. The node wrote `use_post_raft_procedures` to disk but didn't update
the in-memory state yet, which is still `synchronize`.
2. The external client reads the table and sees that the state is
`use_post_raft_procedures`, and deduces that upgrade has finished.
3. The external client immediately tries to perform a schema change. The
schema change code calls `get_group0_upgrade_state` which does not
take the read lock and returns `synchronize`. The schema change gets
denied because schema changes are not allowed in `synchronize`.

Make sure that `get_group0_upgrade_state` cannot execute in-between
writing to disk and updating the in-memory state by always taking the
read lock before reading the in-memory state. As it was before, it will
immediately drop the lock if the state is not `use_pre_raft_procedures`.

This is useful for upgrade tests, which read the on-disk state to decide
whether upgrade has finished and often try to perform a schema change
immediately afterwards.

Closes #11672

---
diff --git a/service/raft/raft_group0_client.cc b/service/raft/raft_group0_client.cc
--- a/service/raft/raft_group0_client.cc
+++ b/service/raft/raft_group0_client.cc
@@ -331,12 +331,10 @@ future<> raft_group0_client::init() {
}

future<std::pair<rwlock::holder, group0_upgrade_state>> raft_group0_client::get_group0_upgrade_state() {
- if (_upgrade_state == group0_upgrade_state::use_pre_raft_procedures) {
- auto holder = co_await _upgrade_lock.hold_read_lock();
+ auto holder = co_await _upgrade_lock.hold_read_lock();

- if (_upgrade_state == group0_upgrade_state::use_pre_raft_procedures) {
- co_return std::pair{std::move(holder), _upgrade_state};
- }
+ if (_upgrade_state == group0_upgrade_state::use_pre_raft_procedures) {
+ co_return std::pair{std::move(holder), _upgrade_state};
}

co_return std::pair{rwlock::holder{}, _upgrade_state};

[Commit Bot]

From: Kamil Braun <kbr...@scylladb.com>
Committer: Tomasz Grabiec <tgra...@scylladb.com>

Branch: master