From f888f2dced987b1d767b53c94cd74d94f9f29805 Mon Sep 17 00:00:00 2001 From: Gleb Natapov Date: Wed, 11 Mar 2026 16:17:01 +0200 Subject: [PATCH] service level: remove remnants of version 1 service level can_use_effective_service_level_cache() always returns true now, so the function can be dropped entirely and all the code that assumes it may return false can be dropped as well. --- ...service_level_distributed_data_accessor.cc | 4 -- ...service_level_distributed_data_accessor.hh | 1 - service/qos/service_level_controller.cc | 60 ++----------------- service/qos/service_level_controller.hh | 10 ---- test/boost/service_level_controller_test.cc | 3 - transport/server.cc | 14 +---- transport/server.hh | 3 +- 7 files changed, 7 insertions(+), 88 deletions(-) diff --git a/service/qos/raft_service_level_distributed_data_accessor.cc b/service/qos/raft_service_level_distributed_data_accessor.cc index 6792351875..1fa08ae83d 100644 --- a/service/qos/raft_service_level_distributed_data_accessor.cc +++ b/service/qos/raft_service_level_distributed_data_accessor.cc @@ -97,9 +97,5 @@ future<> raft_service_level_distributed_data_accessor::commit_mutations(service: co_await _group0_client.send_group0_read_barrier_to_live_members(); } -bool raft_service_level_distributed_data_accessor::can_use_effective_service_level_cache() const { - return true; -} - } diff --git a/service/qos/raft_service_level_distributed_data_accessor.hh b/service/qos/raft_service_level_distributed_data_accessor.hh index d7db58b3ff..feab47e119 100644 --- a/service/qos/raft_service_level_distributed_data_accessor.hh +++ b/service/qos/raft_service_level_distributed_data_accessor.hh @@ -38,7 +38,6 @@ public: virtual future<> drop_service_level(sstring service_level_name, service::group0_batch& mc) const override; virtual future<> commit_mutations(service::group0_batch&& mc, abort_source& as) const override; - virtual bool can_use_effective_service_level_cache() const override; static future> set_service_level_mutations(cql3::query_processor& qp, sstring service_level_name, qos::service_level_options slo, api::timestamp_type timestamp); }; diff --git a/service/qos/service_level_controller.cc b/service/qos/service_level_controller.cc index dc5d00a202..7e36f15804 100644 --- a/service/qos/service_level_controller.cc +++ b/service/qos/service_level_controller.cc @@ -321,17 +321,6 @@ future<> service_level_controller::auth_integration::reload_cache(qos::query_con SCYLLA_ASSERT(this_shard_id() == global_controller); const auto _ = _stop_gate.hold(); - if (!_sl_controller._sl_data_accessor || !_sl_controller._sl_data_accessor->can_use_effective_service_level_cache()) { - // Don't populate the effective service level cache until auth is migrated to raft. - // Otherwise, executing the code that follows would read roles data - // from system_auth tables; that would be bad because reading from - // those tables is prone to timeouts, and `reload_cache` - // is called from the group0 context - a timeout like that would render - // group0 non-functional on the node until restart. - // - // See scylladb/scylladb#24963 for more details. - co_return; - } auto units = co_await get_units(_sl_controller._global_controller_db->notifications_serializer, 1); auto& qs = qos_query_state(ctx); @@ -457,47 +446,10 @@ future> service_level_controller::migrate_t future> service_level_controller::auth_integration::find_effective_service_level(const sstring& role_name) { const auto _ = _stop_gate.hold(); - if (_sl_controller._sl_data_accessor->can_use_effective_service_level_cache()) { - auto effective_sl_it = _cache.find(role_name); - co_return effective_sl_it != _cache.end() - ? std::optional(effective_sl_it->second) - : std::nullopt; - } else { - auto& role_manager = _auth_service.underlying_role_manager(); - auto roles = co_await role_manager.query_granted(role_name, auth::recursive_role_query::yes); - - // converts a list of roles into the chosen service level. - co_return co_await ::map_reduce(roles.begin(), roles.end(), [&role_manager, this] (const sstring& role) { - return role_manager.get_attribute(role, "service_level").then_wrapped([this, role] (future> sl_name_fut) -> std::optional { - try { - std::optional sl_name = sl_name_fut.get(); - if (!sl_name) { - return std::nullopt; - } - auto sl_it = _sl_controller._service_levels_db.find(*sl_name); - if ( sl_it == _sl_controller._service_levels_db.end()) { - return std::nullopt; - } - - sl_it->second.slo.init_effective_names(*sl_name); - auto slo = sl_it->second.slo; - slo.shares_name = sl_name; - return slo; - } catch (...) { // when we fail, we act as if the attribute does not exist so the node - // will not be brought down. - return std::nullopt; - } - }); - }, std::optional{}, [] (std::optional first, std::optional second) -> std::optional { - if (!second) { - return first; - } else if (!first) { - return second; - } else { - return first->merge_with(*second); - } - }); - } + auto effective_sl_it = _cache.find(role_name); + co_return effective_sl_it != _cache.end() + ? std::optional(effective_sl_it->second) + : std::nullopt; } future> service_level_controller::find_effective_service_level(const sstring& role_name) { @@ -715,10 +667,6 @@ future service_level_controller::get_distributed_service_le return _sl_data_accessor ? _sl_data_accessor->get_service_level(service_level_name) : make_ready_future(); } -bool service_level_controller::can_use_effective_service_level_cache() const{ - return _sl_data_accessor && _sl_data_accessor->can_use_effective_service_level_cache(); -} - future service_level_controller::validate_before_service_level_add() { assert(this_shard_id() == global_controller); if (_global_controller_db->deleted_scheduling_groups.size() > 0) { diff --git a/service/qos/service_level_controller.hh b/service/qos/service_level_controller.hh index 2606e25026..286207ac87 100644 --- a/service/qos/service_level_controller.hh +++ b/service/qos/service_level_controller.hh @@ -117,9 +117,6 @@ public: virtual future<> drop_service_level(sstring service_level_name, service::group0_batch& mc) const = 0; virtual future<> commit_mutations(service::group0_batch&& mc, abort_source& as) const = 0; - // Returns whether effective service level cache can be populated and used. - // This is equivalent to checking whether auth + raft have been migrated to raft. - virtual bool can_use_effective_service_level_cache() const = 0; }; using service_level_distributed_data_accessor_ptr = ::shared_ptr; @@ -380,13 +377,6 @@ public: future get_distributed_service_levels(qos::query_context ctx); future get_distributed_service_level(sstring service_level_name); - /* - * Returns whether effective service level cache can be populated and used. - * This is equivalent to checking whether auth + raft have been migrated to raft. - */ - bool can_use_effective_service_level_cache() const; - - /** * Returns the service level options **in effect** for a user having the given * collection of roles. diff --git a/test/boost/service_level_controller_test.cc b/test/boost/service_level_controller_test.cc index 3ff96b81a6..c0ea929a3b 100644 --- a/test/boost/service_level_controller_test.cc +++ b/test/boost/service_level_controller_test.cc @@ -163,9 +163,6 @@ SEASTAR_THREAD_TEST_CASE(too_many_service_levels) { } return make_ready_future<>(); } - virtual bool can_use_effective_service_level_cache() const override { - return true; - } virtual future<> commit_mutations(service::group0_batch&& mc, abort_source& as) const override { return make_ready_future<>(); } diff --git a/transport/server.cc b/transport/server.cc index 11fd47cc67..60b88c9660 100644 --- a/transport/server.cc +++ b/transport/server.cc @@ -1023,15 +1023,7 @@ future> cql_server::connection::process_st co_return res; } -void cql_server::connection::update_user_scheduling_group_v1(const std::optional& usr) { - switch_tenant([this, usr] (this auto self, noncopyable_function ()> process_loop) -> future<> { - auto shg = co_await _server._sl_controller.get_user_scheduling_group(usr); - _current_scheduling_group = shg; - co_return co_await _server._sl_controller.with_user_service_level(usr, std::move(process_loop)); - }); -} - -void cql_server::connection::update_user_scheduling_group_v2(const std::optional& usr) { +void cql_server::connection::update_user_scheduling_group(const std::optional& usr) { auto shg_grp = _server._sl_controller.get_cached_user_scheduling_group(usr); _current_scheduling_group = shg_grp; switch_tenant([this, usr] (this auto self, noncopyable_function ()> process_loop) -> future<> { @@ -1050,10 +1042,8 @@ void cql_server::connection::update_control_connection_scheduling_group() { void cql_server::connection::update_scheduling_group() { if (_client_state.is_control_connection()) { update_control_connection_scheduling_group(); - } else if (_server._sl_controller.can_use_effective_service_level_cache()) { - update_user_scheduling_group_v2(_client_state.user()); } else { - update_user_scheduling_group_v1(_client_state.user()); + update_user_scheduling_group(_client_state.user()); } } diff --git a/transport/server.hh b/transport/server.hh index 4082beaea9..ca711888aa 100644 --- a/transport/server.hh +++ b/transport/server.hh @@ -364,8 +364,7 @@ private: void write_response(foreign_ptr>&& response, service_permit permit = empty_service_permit(), cql_compression compression = cql_compression::none); - void update_user_scheduling_group_v1(const std::optional& usr); - void update_user_scheduling_group_v2(const std::optional& usr); + void update_user_scheduling_group(const std::optional& usr); void update_control_connection_scheduling_group(); friend event_notifier;