Merge 'cql3: Introduce RF-rack-valid keyspaces' from Dawid Mędrek

avikivity · avikivity · commit 7646e1448a48 · 2025-03-20T19:10:36.000+02:00
This PR is an introductory step towards enforcing RF-rack-valid keyspaces in Scylla. The scope of changes: * defining RF-rack-valid keyspaces, * introducing a configuration option enforcing RF-rack-valid keyspaces, * restricting the CREATE and ALTER KEYSPACE statements so that they never lead to RF-rack invalid keyspaces, * during the initialization of a node, it verifies that all existing keyspaces are RF-rack-valid. If not, the initialization fails. We provide tests verifying that the changes behave as intended. --- Note that there are a number of things that still need to be implemented. That includes, for instance, restricting topology operations too. --- Implementation strategy (going beyond the scope of this PR): 1. Introduce the new configuration option `rf_rack_valid_keyspaces`. 2. Start enforcing RF-rack-validity in keyspaces if the option is enabled. 3. Adjust the tests: in the tree and out of it. Explicitly enable the option in all tests. 4. Once the tests have been adjusted, change the default value of the option to enabled. 5. Stop explicitly enabling the option in tests. 6. Get rid of the option. --- Fixes scylladb#20356 Fixes scylladb#23276 Fixes scylladb#23300 --- Backport: this is part of the requirements for releasing 2025.1. Closes scylladb#23138 * github.com:scylladb/scylladb: main: Refuse to start node when RF-rack-invalid keyspace exists cql3: Ensure that CREATE and ALTER never lead to RF-rack-invalid keyspaces db/config: Introduce RF-rack-valid keyspaces
diff --git a/conf/scylla.yaml b/conf/scylla.yaml
@@ -837,3 +837,6 @@ maintenance_socket: ignore
 # Note that creating keyspaces with tablets enabled or disabled is irreversible.
 # The `tablets` option cannot be changed using `ALTER KEYSPACE`.
 enable_tablets: true
+
+# Enforce RF-rack-valid keyspaces.
+rf_rack_valid_keyspaces: false
diff --git a/cql3/statements/alter_keyspace_statement.cc b/cql3/statements/alter_keyspace_statement.cc
@@ -13,6 +13,7 @@
 #include <seastar/core/on_internal_error.hh>
 #include <stdexcept>
 #include "alter_keyspace_statement.hh"
+#include "locator/tablets.hh"
 #include "prepared_statement.hh"
 #include "service/migration_manager.hh"
 #include "service/storage_proxy.hh"
@@ -25,6 +26,7 @@
 #include "create_keyspace_statement.hh"
 #include "gms/feature_service.hh"
 #include "replica/database.hh"
+#include "db/config.hh"
 
 using namespace std::string_literals;
 
@@ -194,9 +196,9 @@ cql3::statements::alter_keyspace_statement::prepare_schema_mutations(query_proce
         event::schema_change::target_type target_type = event::schema_change::target_type::KEYSPACE;
         auto ks = qp.db().find_keyspace(_name);
         auto ks_md = ks.metadata();
-        const auto& tm = *qp.proxy().get_token_metadata_ptr();
+        const auto tmptr = qp.proxy().get_token_metadata_ptr();
         const auto& feat = qp.proxy().features();
-        auto ks_md_update = _attrs->as_ks_metadata_update(ks_md, tm, feat);
+        auto ks_md_update = _attrs->as_ks_metadata_update(ks_md, *tmptr, feat);
         std::vector<mutation> muts;
         std::vector<sstring> warnings;
         auto old_ks_options = get_old_options_flattened(ks);
@@ -265,6 +267,36 @@ cql3::statements::alter_keyspace_statement::prepare_schema_mutations(query_proce
             muts.insert(muts.begin(), schema_mutations.begin(), schema_mutations.end());
         }
 
+        // If `rf_rack_valid_keyspaces` is enabled, it's forbidden to perform a schema change that
+        // would lead to an RF-rack-valid keyspace. Verify that this change does not.
+        // For more context, see: scylladb/scylladb#23071.
+        if (qp.db().get_config().rf_rack_valid_keyspaces()) {
+            auto rs = locator::abstract_replication_strategy::create_replication_strategy(
+                    ks_md_update->strategy_name(),
+                    locator::replication_strategy_params(ks_md_update->strategy_options(), ks_md_update->initial_tablets()));
+
+            try {
+                // There are two things to note here:
+                // 1. We hold a group0_guard, so it's correct to check this here.
+                //    The topology or schema cannot change while we're performing this query.
+                // 2. The replication strategy we use here does NOT represent the actual state
+                //    we will arrive at after applying the schema change. For instance, if the user
+                //    did not specify the RF for some of the DCs, it's equal to 0 in the replication
+                //    strategy we pass to this function, while in reality that means that the RF
+                //    will NOT change. That is not a problem:
+                //    - RF=0 is valid for all DCs, so it won't trigger an exception on its own,
+                //    - the keyspace must've been RF-rack-valid before this change. We check that
+                //      condition for all keyspaces at startup.
+                //    The second hyphen is not really true because currently topological changes can
+                //    disturb it (see scylladb/scylladb#23345), but we ignore that.
+                locator::assert_rf_rack_valid_keyspace(_name, tmptr, *rs);
+            } catch (const std::exception& e) {
+                // There's no guarantee what the type of the exception will be, so we need to
+                // wrap it manually here in a type that can be passed to the user.
+                throw exceptions::invalid_request_exception(e.what());
+            }
+        }
+
         auto ret = ::make_shared<event::schema_change>(
                 event::schema_change::change_type::UPDATED,
                 target_type,
diff --git a/cql3/statements/create_keyspace_statement.cc b/cql3/statements/create_keyspace_statement.cc
@@ -11,6 +11,8 @@
 #include <seastar/core/coroutine.hh>
 #include "cql3/statements/create_keyspace_statement.hh"
 #include "cql3/statements/ks_prop_defs.hh"
+#include "exceptions/exceptions.hh"
+#include "locator/tablets.hh"
 #include "prepared_statement.hh"
 #include "data_dictionary/data_dictionary.hh"
 #include "data_dictionary/keyspace_metadata.hh"
@@ -90,14 +92,14 @@ void create_keyspace_statement::validate(query_processor& qp, const service::cli
 
 future<std::tuple<::shared_ptr<cql_transport::event::schema_change>, std::vector<mutation>, cql3::cql_warnings_vec>> create_keyspace_statement::prepare_schema_mutations(query_processor& qp, const query_options&, api::timestamp_type ts) const {
     using namespace cql_transport;
-    const auto& tm = *qp.proxy().get_token_metadata_ptr();
+    const auto tmptr = qp.proxy().get_token_metadata_ptr();
     const auto& feat = qp.proxy().features();
     const auto& cfg = qp.db().get_config();
     std::vector<mutation> m;
     std::vector<sstring> warnings;
 
     try {
-        auto ksm = _attrs->as_ks_metadata(_name, tm, feat, cfg);
+        auto ksm = _attrs->as_ks_metadata(_name, *tmptr, feat, cfg);
         m = service::prepare_new_keyspace_announcement(qp.db().real_database(), ksm, ts);
         // If the new keyspace uses tablets, as long as there are features
         // which aren't supported by tablets we want to warn the user that
@@ -119,6 +121,21 @@ future<std::tuple<::shared_ptr<cql_transport::event::schema_change>, std::vector
                 warnings.push_back("Keyspace `initial` tablets option is deprecated.  Use per-table tablet options instead.");
             }
         }
+
+        // If `rf_rack_valid_keyspaces` is enabled, it's forbidden to create an RF-rack-invalid keyspace.
+        // Verify that it's RF-rack-valid.
+        // For more context, see: scylladb/scylladb#23071.
+        if (cfg.rf_rack_valid_keyspaces()) {
+            try {
+                // We hold a group0_guard, so it's correct to check this here.
+                // The topology or schema cannot change while we're performing this query.
+                locator::assert_rf_rack_valid_keyspace(_name, tmptr, *rs);
+            } catch (const std::exception& e) {
+                // There's no guarantee what the type of the exception will be, so we need to
+                // wrap it manually here in a type that can be passed to the user.
+                throw exceptions::invalid_request_exception(e.what());
+            }
+        }
     } catch (const exceptions::already_exists_exception& e) {
         if (!_if_not_exists) {
           co_return coroutine::exception(std::current_exception());
diff --git a/db/config.cc b/db/config.cc
@@ -1382,6 +1382,9 @@ db::config::config(std::shared_ptr<db::extensions> exts)
     , disk_space_monitor_high_polling_interval_in_seconds(this, "disk_space_monitor_high_polling_interval_in_seconds", value_status::Used, 1, "Disk-space polling interval at or above polling threshold")
     , disk_space_monitor_polling_interval_threshold(this, "disk_space_monitor_polling_interval_threshold", value_status::Used, 0.9, "Disk-space polling threshold. Polling interval is increased when disk utilization is greater than or equal to this threshold")
     , enable_create_table_with_compact_storage(this, "enable_create_table_with_compact_storage", liveness::LiveUpdate, value_status::Used, false, "Enable the deprecated feature of CREATE TABLE WITH COMPACT STORAGE.  This feature will eventually be removed in a future version.")
+    , rf_rack_valid_keyspaces(this, "rf_rack_valid_keyspaces", liveness::MustRestart, value_status::Used, false,
+        "Enforce RF-rack-valid keyspaces. Additionally, if there are existing RF-rack-invalid "
+        "keyspaces, attempting to start a node with this option ON will fail.")
     , default_log_level(this, "default_log_level", value_status::Used, seastar::log_level::info, "Default log level for log messages")
     , logger_log_level(this, "logger_log_level", value_status::Used, {}, "Map of logger name to log level. Valid log levels are 'error', 'warn', 'info', 'debug' and 'trace'")
     , log_to_stdout(this, "log_to_stdout", value_status::Used, true, "Send log output to stdout")
diff --git a/db/config.hh b/db/config.hh
@@ -546,6 +546,8 @@ public:
 
     named_value<bool> enable_create_table_with_compact_storage;
 
+    named_value<bool> rf_rack_valid_keyspaces;
+
     static const sstring default_tls_priority;
 private:
     template<typename T>
diff --git a/docs/architecture/tablets.rst b/docs/architecture/tablets.rst
@@ -140,6 +140,12 @@ You can create a keyspace with tablets enabled with the ``tablets = {'enabled':
 Limitations and Unsupported Features
 --------------------------------------
 
+.. warning::
+
+    If a keyspace has tablets enabled, it must remain :term:`RF-rack-valid <RF-rack-valid keyspace>`
+    throughout its lifetime. Failing to keep that invariant satisfied may result in data inconsistencies,
+    performance problems, or other issues.
+
 The following ScyllaDB features are not supported if a keyspace has tablets
 enabled:
 
diff --git a/docs/architecture/zero-token-nodes.rst b/docs/architecture/zero-token-nodes.rst
@@ -21,3 +21,8 @@ Note that:
 * Zero-token nodes never store replicated data, so running ``nodetool rebuild``,
   ``nodetool repair``, and ``nodetool cleanup`` can be skipped as it does not
   affect zero-token nodes.
+* Racks consisting solely of zero-token nodes are not taken into consideration
+  when deciding whether a keyspace is :term:`RF-rack-valid <RF-rack-valid keyspace>`.
+  However, an RF-rack-valid keyspace must have the replication factor equal to 0
+  in an :doc:`arbiter DC </operating-scylla/procedures/cluster-management/arbiter-dc>`.
+  Otherwise, it is RF-rack-invalid.
diff --git a/docs/cql/ddl.rst b/docs/cql/ddl.rst
@@ -224,7 +224,8 @@ By default, a keyspace is created with tablets enabled. You can use the ``tablet
 to opt out a keyspace from tablets-based distribution.
 
 You may want to opt out if you plan to use features that are not supported for keyspaces
-with tablets enabled. See :ref:`Limitations and Unsupported Features <tablets-limitations>`
+with tablets enabled. Keyspaces using tablets must also remain :term:`RF-rack-valid <RF-rack-valid keyspace>`
+throughout their lifetime. See :ref:`Limitations and Unsupported Features <tablets-limitations>`
 for details.
 
 **The ``initial`` sub-option (deprecated)**
@@ -300,6 +301,7 @@ Modifying a keyspace with tablets enabled is possible and doesn't require any sp
 - If there's any other ongoing global topology operation, executing the ``ALTER`` statement will fail (with an explicit and specific error) and needs to be repeated.
 - The ``ALTER`` statement may take longer than the regular query timeout, and even if it times out, it will continue to execute in the background.
 - The replication strategy cannot be modified, as keyspaces with tablets only support ``NetworkTopologyStrategy``.
+- The ``ALTER`` statement will fail if it would make the keyspace :term:`RF-rack-invalid <RF-rack-valid keyspace>`.
 
 .. _drop-keyspace-statement:
 
diff --git a/docs/reference/glossary.rst b/docs/reference/glossary.rst
@@ -127,6 +127,12 @@ Glossary
       RBNO is enabled by default for a subset node operations. 
       See :doc:`Repair Based Node Operations </operating-scylla/procedures/cluster-management/repair-based-node-operation>` for details.
 
+    RF-rack-valid keyspace
+      A keyspace with :doc:`tablets </architecture/tablets>` enabled is RF-rack-valid if all of its data centers
+      have the :term:`Replication Factor (RF) <Replication Factor (RF)>` of 0, 1, or the number of racks in that data center.
+
+      Keyspaces with tablets disabled are always deemed RF-rack-valid, even if they do not satisfy the aforementioned condition.
+
     Shard
       Each ScyllaDB node is internally split into *shards*, an independent thread bound to a dedicated core.
       Each shard of data is allotted CPU, RAM, persistent storage, and networking resources which it uses as efficiently as possible.
diff --git a/locator/tablets.cc b/locator/tablets.cc
@@ -6,6 +6,7 @@
  * SPDX-License-Identifier: LicenseRef-ScyllaDB-Source-Available-1.0
  */
 
+#include "locator/network_topology_strategy.hh"
 #include "locator/tablet_replication_strategy.hh"
 #include "locator/tablets.hh"
 #include "locator/tablet_metadata_guard.hh"
@@ -1042,6 +1043,64 @@ void tablet_metadata_guard::subscribe() {
     });
 }
 
+void assert_rf_rack_valid_keyspace(std::string_view ks, const token_metadata_ptr tmptr, const abstract_replication_strategy& ars) {
+    tablet_logger.debug("[assert_rf_rack_valid_keyspace]: Starting verifying that keyspace '{}' is RF-rack-valid", ks);
+
+    // Any keyspace that does NOT use tablets is RF-rack-valid.
+    if (!ars.uses_tablets()) {
+        tablet_logger.debug("[assert_rf_rack_valid_keyspace]: Keyspace '{}' has been verified to be RF-rack-valid (no tablets)", ks);
+        return;
+    }
+
+    // Tablets can only be used with NetworkTopologyStrategy.
+    SCYLLA_ASSERT(ars.get_type() == replication_strategy_type::network_topology);
+    const auto& nts = *static_cast<const network_topology_strategy*>(std::addressof(ars));
+
+    const auto& dc_rack_map = tmptr->get_topology().get_datacenter_racks();
+
+    for (const auto& dc : nts.get_datacenters()) {
+        if (!dc_rack_map.contains(dc)) {
+            on_internal_error(tablet_logger, seastar::format(
+                    "Precondition violated: DC '{}' is part of the passed replication strategy, but it is not "
+                    "known by the passed locator::token_metadata_ptr.", dc));
+        }
+    }
+
+    for (const auto& [dc, rack_map] : dc_rack_map) {
+        tablet_logger.debug("[assert_rf_rack_valid_keyspace]: Verifying for '{}' / '{}'", ks, dc);
+
+        size_t normal_rack_count = 0;
+        for (const auto& [_, rack_nodes] : rack_map) {
+            // We must ignore zero-token nodes because they don't take part in replication.
+            // Verify that this rack has at least one normal node.
+            const bool normal_rack = std::ranges::any_of(rack_nodes, [tmptr] (host_id host_id) {
+                return tmptr->is_normal_token_owner(host_id);
+            });
+            if (normal_rack) {
+                ++normal_rack_count;
+            }
+        }
+
+        const size_t rf = nts.get_replication_factor(dc);
+
+        // We must not allow for a keyspace to become RF-rack-invalid. Any attempt at that must be rejected.
+        // For more context, see: scylladb/scylladb#23276.
+        const bool invalid_rf = rf != normal_rack_count && rf != 1 && rf != 0;
+        // Edge case: the DC in question is an arbiter DC and does NOT take part in replication.
+        // Any positive RF for that DC is invalid.
+        const bool invalid_arbiter_dc = normal_rack_count == 0 && rf > 0;
+
+        if (invalid_rf || invalid_arbiter_dc) {
+            throw std::invalid_argument(std::format(
+                    "The option `rf_rack_valid_keyspaces` is enabled. It requires that all keyspaces are RF-rack-valid. "
+                    "That condition is violated: keyspace '{}' doesn't satisfy it for DC '{}': RF={} vs. rack count={}.",
+                    ks, std::string_view(dc), rf, normal_rack_count));
+        }
+    }
+
+    tablet_logger.debug("[assert_rf_rack_valid_keyspace]: Keyspace '{}' has been verified to be RF-rack-valid", ks);
+}
+
 }
 
 auto fmt::formatter<locator::resize_decision_way>::format(const locator::resize_decision_way& way, fmt::format_context& ctx) const
diff --git a/locator/tablets.hh b/locator/tablets.hh
@@ -9,6 +9,7 @@
 #pragma once
 
 #include "dht/token.hh"
+#include "locator/token_metadata_fwd.hh"
 #include "utils/small_vector.hh"
 #include "locator/host_id.hh"
 #include "service/session.hh"
@@ -691,6 +692,30 @@ struct tablet_metadata_change_hint {
     explicit operator bool() const noexcept { return !tables.empty(); }
 };
 
+class abstract_replication_strategy;
+
+/// Verify that the provided keyspace corresponding to the provided replication strategy is RF-rack-valid, i.e.
+/// whether it satisfies the following conditions:
+/// * does NOT use tablets, OR,
+/// * for every DC, the replication factor corresponding to that DC must be an element of the set
+///     {0, 1, the number of racks in that DC with at least one normal node}
+///   Special case: if the DC is an arbiter DC (i.e. only consists of zero-token nodes), the RF MUST be equal
+///   to 0 for that DC.
+///
+/// Result:
+/// * if the keyspace is RF-rack-valid, no side effect,
+/// * if the keyspace is RF-rack-invalid, an exception will be thrown. It will contain information about the reason
+///   why the keyspace is RF-rack-invalid and will be worded in a way that can be returned directly to the user.
+///   There are NO guarantees about the type of the exception.
+///
+/// Preconditions:
+/// * Every DC that takes part in replication according to the passed replication strategy MUST be known
+///   by the passed `token_metadata_ptr`.
+///
+/// Non-requirements:
+/// * The keyspace need not exist. We use its name purely for informational reasons (in error messages).
+void assert_rf_rack_valid_keyspace(std::string_view ks, const token_metadata_ptr, const abstract_replication_strategy&);
+
 }
 
 template <>
diff --git a/main.cc b/main.cc
@@ -12,6 +12,7 @@
 
 #include <seastar/util/closeable.hh>
 #include <seastar/core/abort_source.hh>
+#include "exceptions/exceptions.hh"
 #include "gms/inet_address.hh"
 #include "auth/allow_all_authenticator.hh"
 #include "auth/allow_all_authorizer.hh"
@@ -2122,6 +2123,14 @@ sharded<locator::shared_token_metadata> token_metadata;
                 return ss.local().join_cluster(proxy, service::start_hint_manager::yes, generation_number);
             }).get();
 
+            // At this point, `locator::topology` should be stable, i.e. we should have complete information
+            // about the layout of the cluster (= list of nodes along with the racks/DCs).
+            if (cfg->rf_rack_valid_keyspaces()) {
+                startlog.info("Verifying that all of the keyspaces are RF-rack-valid");
+                db.local().check_rf_rack_validity(token_metadata.local().get());
+                startlog.info("All keyspaces are RF-rack-valid");
+            }
+
             dictionary_service dict_service(
                 dict_sampler,
                 sys_ks.local(),
diff --git a/replica/database.cc b/replica/database.cc
@@ -10,6 +10,9 @@
 
 #include <fmt/ranges.h>
 #include <fmt/std.h>
+#include "locator/network_topology_strategy.hh"
+#include "locator/tablets.hh"
+#include "locator/token_metadata_fwd.hh"
 #include "utils/log.hh"
 #include "replica/database_fwd.hh"
 #include "utils/assert.hh"
@@ -3184,4 +3187,12 @@ database::on_effective_service_levels_cache_reloaded() {
     co_return;
 }
 
+void database::check_rf_rack_validity(const locator::token_metadata_ptr tmptr) const {
+    SCYLLA_ASSERT(get_config().rf_rack_valid_keyspaces());
+
+    for (const auto& [name, info] : get_keyspaces()) {
+        locator::assert_rf_rack_valid_keyspace(name, tmptr, info.get_replication_strategy());
+    }
+}
+
 }
diff --git a/replica/database.hh b/replica/database.hh
@@ -1951,6 +1951,14 @@ public:
     /** This callback is going to be called just before the service level is changed **/
     virtual future<> on_before_service_level_change(qos::service_level_options slo_before, qos::service_level_options slo_after, qos::service_level_info sl_info) override;
     virtual future<> on_effective_service_levels_cache_reloaded() override;
+
+    // Verify that the existing keyspaces are all RF-rack-valid.
+    //
+    // Preconditions:
+    // * the option `rf_rack_valid_keyspaces` in enabled,
+    // * the `locator::topology` instance corresponding to the passed `locator::token_metadata_ptr`
+    //   must contain a complete list of racks and data centers in the cluster.
+    void check_rf_rack_validity(const locator::token_metadata_ptr) const;
 };
 
 // A helper function to parse the directory name back
diff --git a/test/cluster/test_multidc.py b/test/cluster/test_multidc.py
diff --git a/test/lib/cql_test_env.cc b/test/lib/cql_test_env.cc
