From 4008ebb1b02b59bf1093c025a312ef1489cca55f Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Tue, 26 Sep 2023 11:25:43 +0300 Subject: [PATCH 1/5] api/storage_service: Get token_metadata from storage service The API handlers that live in set_storage_service() should be self-contained and operate on storage-service only. Said that, they should get the token metadata, when needed, from storage service, not from somewhere else. Signed-off-by: Pavel Emelyanov --- api/storage_service.cc | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/api/storage_service.cc b/api/storage_service.cc index 3bfe210404..0defa3a100 100644 --- a/api/storage_service.cc +++ b/api/storage_service.cc @@ -54,10 +54,6 @@ extern logging::logger apilog; namespace api { -const locator::token_metadata& http_context::get_token_metadata() { - return *shared_token_metadata.local().get(); -} - namespace ss = httpd::storage_service_json; using namespace json; @@ -466,20 +462,20 @@ static future describe_ring_as_json(sharded& ss, gms::gossiper& g, sharded& sys_ks) { - ss::local_hostid.set(r, [&ctx](std::unique_ptr req) { - auto id = ctx.db.local().get_token_metadata().get_my_id(); + ss::local_hostid.set(r, [&ss](std::unique_ptr req) { + auto id = ss.local().get_token_metadata().get_my_id(); return make_ready_future(id.to_sstring()); }); - ss::get_tokens.set(r, [&ctx] (std::unique_ptr req) { - return make_ready_future(stream_range_as_array(ctx.get_token_metadata().sorted_tokens(), [](const dht::token& i) { + ss::get_tokens.set(r, [&ss] (std::unique_ptr req) { + return make_ready_future(stream_range_as_array(ss.local().get_token_metadata().sorted_tokens(), [](const dht::token& i) { return fmt::to_string(i); })); }); - ss::get_node_tokens.set(r, [&ctx] (std::unique_ptr req) { + ss::get_node_tokens.set(r, [&ss] (std::unique_ptr req) { gms::inet_address addr(req->param["endpoint"]); - return make_ready_future(stream_range_as_array(ctx.get_token_metadata().get_tokens(addr), [](const dht::token& i) { + return make_ready_future(stream_range_as_array(ss.local().get_token_metadata().get_tokens(addr), [](const dht::token& i) { return fmt::to_string(i); })); }); @@ -547,8 +543,8 @@ void set_storage_service(http_context& ctx, routes& r, sharded addr; for (auto i: points) { addr.insert(fmt::to_string(i.second)); @@ -629,9 +625,9 @@ void set_storage_service(http_context& ctx, routes& r, shardedparam)); }); - ss::get_host_id_map.set(r, [&ctx](const_req req) { + ss::get_host_id_map.set(r, [&ss](const_req req) { std::vector res; - return map_to_key_value(ctx.get_token_metadata().get_endpoint_to_host_id_map_for_reading(), res); + return map_to_key_value(ss.local().get_token_metadata().get_endpoint_to_host_id_map_for_reading(), res); }); ss::get_load.set(r, [&ctx](std::unique_ptr req) { From 27eaff9d4444b0ee07a2bb368d05d3054fd0c370 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Tue, 26 Sep 2023 12:08:46 +0300 Subject: [PATCH 2/5] api/storage_service: Get gossiper from storage service Some handlers in set_storage_service() have implicit dependency on gossiper. It's not API that should track it, but storage service itself, so get the gossiper from service, not from the external argument (it will be removed soon) Signed-off-by: Pavel Emelyanov --- api/storage_service.cc | 18 +++++++++--------- service/storage_service.hh | 6 +++--- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/api/storage_service.cc b/api/storage_service.cc index 0defa3a100..7d0f53ec7e 100644 --- a/api/storage_service.cc +++ b/api/storage_service.cc @@ -647,9 +647,9 @@ void set_storage_service(http_context& ctx, routes& r, sharded req) { + ss::get_current_generation_number.set(r, [&ss](std::unique_ptr req) { gms::inet_address ep(utils::fb_utilities::get_broadcast_address()); - return g.get_current_generation_number(ep).then([](gms::generation_type res) { + return ss.local().gossiper().get_current_generation_number(ep).then([](gms::generation_type res) { return make_ready_future(res.value()); }); }); @@ -894,11 +894,11 @@ void set_storage_service(http_context& ctx, routes& r, sharded(json_void()); }); - ss::is_initialized.set(r, [&ss, &g](std::unique_ptr req) { - return ss.local().get_operation_mode().then([&g] (auto mode) { + ss::is_initialized.set(r, [&ss](std::unique_ptr req) { + return ss.local().get_operation_mode().then([&ss] (auto mode) { bool is_initialized = mode >= service::storage_service::mode::STARTING; if (mode == service::storage_service::mode::NORMAL) { - is_initialized = g.is_enabled(); + is_initialized = ss.local().gossiper().is_enabled(); } return make_ready_future(is_initialized); }); @@ -1119,12 +1119,12 @@ void set_storage_service(http_context& ctx, routes& r, sharded(json_void()); }); - ss::get_cluster_name.set(r, [&g](const_req req) { - return g.get_cluster_name(); + ss::get_cluster_name.set(r, [&ss](const_req req) { + return ss.local().gossiper().get_cluster_name(); }); - ss::get_partitioner_name.set(r, [&g](const_req req) { - return g.get_partitioner_name(); + ss::get_partitioner_name.set(r, [&ss](const_req req) { + return ss.local().gossiper().get_partitioner_name(); }); ss::get_tombstone_warn_threshold.set(r, [](std::unique_ptr req) { diff --git a/service/storage_service.hh b/service/storage_service.hh index ec17b41b6c..b6bedd7051 100644 --- a/service/storage_service.hh +++ b/service/storage_service.hh @@ -228,6 +228,9 @@ private: return _batchlog_manager; } + friend struct ::node_ops_ctl; +public: + const gms::gossiper& gossiper() const noexcept { return _gossiper; }; @@ -236,9 +239,6 @@ private: return _gossiper; }; - friend struct ::node_ops_ctl; -public: - locator::effective_replication_map_factory& get_erm_factory() noexcept { return _erm_factory; } From 8dc6e74138d3016dc67ef5cdd8f07e73686e5db2 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Tue, 26 Sep 2023 12:00:06 +0300 Subject: [PATCH 3/5] api/storage_service: Remove system keyspace arg from API It's not used nowadays Signed-off-by: Pavel Emelyanov --- api/api.cc | 6 +++--- api/api_init.hh | 2 +- api/storage_service.cc | 2 +- api/storage_service.hh | 2 +- main.cc | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/api/api.cc b/api/api.cc index 3e05f78798..0719242964 100644 --- a/api/api.cc +++ b/api/api.cc @@ -102,9 +102,9 @@ future<> unset_rpc_controller(http_context& ctx) { return ctx.http_server.set_routes([&ctx] (routes& r) { unset_rpc_controller(ctx, r); }); } -future<> set_server_storage_service(http_context& ctx, sharded& ss, sharded& g, sharded& sys_ks) { - return register_api(ctx, "storage_service", "The storage service API", [&ss, &g, &sys_ks] (http_context& ctx, routes& r) { - set_storage_service(ctx, r, ss, g.local(), sys_ks); +future<> set_server_storage_service(http_context& ctx, sharded& ss, sharded& g) { + return register_api(ctx, "storage_service", "The storage service API", [&ss, &g] (http_context& ctx, routes& r) { + set_storage_service(ctx, r, ss, g.local()); }); } diff --git a/api/api_init.hh b/api/api_init.hh index d3be39e425..1d5e186d15 100644 --- a/api/api_init.hh +++ b/api/api_init.hh @@ -85,7 +85,7 @@ future<> set_server_init(http_context& ctx); future<> set_server_config(http_context& ctx, const db::config& cfg); future<> set_server_snitch(http_context& ctx, sharded& snitch); future<> unset_server_snitch(http_context& ctx); -future<> set_server_storage_service(http_context& ctx, sharded& ss, sharded& g, sharded& sys_ks); +future<> set_server_storage_service(http_context& ctx, sharded& ss, sharded& g); future<> set_server_sstables_loader(http_context& ctx, sharded& sst_loader); future<> unset_server_sstables_loader(http_context& ctx); future<> set_server_view_builder(http_context& ctx, sharded& vb); diff --git a/api/storage_service.cc b/api/storage_service.cc index 7d0f53ec7e..75d23a8b1b 100644 --- a/api/storage_service.cc +++ b/api/storage_service.cc @@ -461,7 +461,7 @@ static future describe_ring_as_json(sharded& ss, gms::gossiper& g, sharded& sys_ks) { +void set_storage_service(http_context& ctx, routes& r, sharded& ss, gms::gossiper& g) { ss::local_hostid.set(r, [&ss](std::unique_ptr req) { auto id = ss.local().get_token_metadata().get_my_id(); return make_ready_future(id.to_sstring()); diff --git a/api/storage_service.hh b/api/storage_service.hh index 9c4de638b2..1727921aa0 100644 --- a/api/storage_service.hh +++ b/api/storage_service.hh @@ -57,7 +57,7 @@ std::vector parse_tables(const sstring& ks_name, http_context& ctx, con // if the parameter is not found or is empty, returns a list of all table infos in the keyspace. std::vector parse_table_infos(const sstring& ks_name, http_context& ctx, const std::unordered_map& query_params, sstring param_name); -void set_storage_service(http_context& ctx, httpd::routes& r, sharded& ss, gms::gossiper& g, sharded& sys_ls); +void set_storage_service(http_context& ctx, httpd::routes& r, sharded& ss, gms::gossiper& g); void set_sstables_loader(http_context& ctx, httpd::routes& r, sharded& sst_loader); void unset_sstables_loader(http_context& ctx, httpd::routes& r); void set_view_builder(http_context& ctx, httpd::routes& r, sharded& vb); diff --git a/main.cc b/main.cc index cf983aa586..27d5eedd36 100644 --- a/main.cc +++ b/main.cc @@ -1590,7 +1590,7 @@ To start the scylla server proper, simply invoke as: scylla server (or just scyl auto stop_messaging_api = defer_verbose_shutdown("messaging service API", [&ctx] { api::unset_server_messaging_service(ctx).get(); }); - api::set_server_storage_service(ctx, ss, gossiper, sys_ks).get(); + api::set_server_storage_service(ctx, ss, gossiper).get(); api::set_server_repair(ctx, repair).get(); auto stop_repair_api = defer_verbose_shutdown("repair API", [&ctx] { api::unset_server_repair(ctx).get(); From 78a22c5ae33ce6acc2bef813e3330e9e9c7b623b Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Tue, 26 Sep 2023 12:11:05 +0300 Subject: [PATCH 4/5] api/storage_service: Remove gossiper arg from API Now all handlers work purely on storage_service and gossiper argument is no longer needed Signed-off-by: Pavel Emelyanov --- api/api.cc | 6 +++--- api/api_init.hh | 2 +- api/storage_service.cc | 2 +- api/storage_service.hh | 2 +- main.cc | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/api/api.cc b/api/api.cc index 0719242964..4371748759 100644 --- a/api/api.cc +++ b/api/api.cc @@ -102,9 +102,9 @@ future<> unset_rpc_controller(http_context& ctx) { return ctx.http_server.set_routes([&ctx] (routes& r) { unset_rpc_controller(ctx, r); }); } -future<> set_server_storage_service(http_context& ctx, sharded& ss, sharded& g) { - return register_api(ctx, "storage_service", "The storage service API", [&ss, &g] (http_context& ctx, routes& r) { - set_storage_service(ctx, r, ss, g.local()); +future<> set_server_storage_service(http_context& ctx, sharded& ss) { + return register_api(ctx, "storage_service", "The storage service API", [&ss] (http_context& ctx, routes& r) { + set_storage_service(ctx, r, ss); }); } diff --git a/api/api_init.hh b/api/api_init.hh index 1d5e186d15..a6adcae1b7 100644 --- a/api/api_init.hh +++ b/api/api_init.hh @@ -85,7 +85,7 @@ future<> set_server_init(http_context& ctx); future<> set_server_config(http_context& ctx, const db::config& cfg); future<> set_server_snitch(http_context& ctx, sharded& snitch); future<> unset_server_snitch(http_context& ctx); -future<> set_server_storage_service(http_context& ctx, sharded& ss, sharded& g); +future<> set_server_storage_service(http_context& ctx, sharded& ss); future<> set_server_sstables_loader(http_context& ctx, sharded& sst_loader); future<> unset_server_sstables_loader(http_context& ctx); future<> set_server_view_builder(http_context& ctx, sharded& vb); diff --git a/api/storage_service.cc b/api/storage_service.cc index 75d23a8b1b..599ebc5860 100644 --- a/api/storage_service.cc +++ b/api/storage_service.cc @@ -461,7 +461,7 @@ static future describe_ring_as_json(sharded& ss, gms::gossiper& g) { +void set_storage_service(http_context& ctx, routes& r, sharded& ss) { ss::local_hostid.set(r, [&ss](std::unique_ptr req) { auto id = ss.local().get_token_metadata().get_my_id(); return make_ready_future(id.to_sstring()); diff --git a/api/storage_service.hh b/api/storage_service.hh index 1727921aa0..82bb71d213 100644 --- a/api/storage_service.hh +++ b/api/storage_service.hh @@ -57,7 +57,7 @@ std::vector parse_tables(const sstring& ks_name, http_context& ctx, con // if the parameter is not found or is empty, returns a list of all table infos in the keyspace. std::vector parse_table_infos(const sstring& ks_name, http_context& ctx, const std::unordered_map& query_params, sstring param_name); -void set_storage_service(http_context& ctx, httpd::routes& r, sharded& ss, gms::gossiper& g); +void set_storage_service(http_context& ctx, httpd::routes& r, sharded& ss); void set_sstables_loader(http_context& ctx, httpd::routes& r, sharded& sst_loader); void unset_sstables_loader(http_context& ctx, httpd::routes& r); void set_view_builder(http_context& ctx, httpd::routes& r, sharded& vb); diff --git a/main.cc b/main.cc index 27d5eedd36..e131ca8bbc 100644 --- a/main.cc +++ b/main.cc @@ -1590,7 +1590,7 @@ To start the scylla server proper, simply invoke as: scylla server (or just scyl auto stop_messaging_api = defer_verbose_shutdown("messaging service API", [&ctx] { api::unset_server_messaging_service(ctx).get(); }); - api::set_server_storage_service(ctx, ss, gossiper).get(); + api::set_server_storage_service(ctx, ss).get(); api::set_server_repair(ctx, repair).get(); auto stop_repair_api = defer_verbose_shutdown("repair API", [&ctx] { api::unset_server_repair(ctx).get(); From e022a763505404bf82ffa253f053c7f7c6ac0d27 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Tue, 26 Sep 2023 12:19:18 +0300 Subject: [PATCH 5/5] main, api: Set/Unset storage_service API in proper place Currently the storage-service API handlers are set up in "random" place. It can happen earlier -- as soon as the storage service itself is ready. Also, despite storage service is stopped on shutdown, API handlers continue reference it leading to potential use-after-frees or "local is not initialized" assertions. Fix both. Unsetting is pretty bulky, scylladb/seastar#1620 is to help. Signed-off-by: Pavel Emelyanov --- api/api.cc | 4 ++ api/api_init.hh | 1 + api/storage_service.cc | 89 ++++++++++++++++++++++++++++++++++++++++++ api/storage_service.hh | 1 + main.cc | 7 +++- 5 files changed, 101 insertions(+), 1 deletion(-) diff --git a/api/api.cc b/api/api.cc index 4371748759..32e6e9663a 100644 --- a/api/api.cc +++ b/api/api.cc @@ -108,6 +108,10 @@ future<> set_server_storage_service(http_context& ctx, sharded unset_server_storage_service(http_context& ctx) { + return ctx.http_server.set_routes([&ctx] (routes& r) { unset_storage_service(ctx, r); }); +} + future<> set_server_sstables_loader(http_context& ctx, sharded& sst_loader) { return ctx.http_server.set_routes([&ctx, &sst_loader] (routes& r) { set_sstables_loader(ctx, r, sst_loader); }); } diff --git a/api/api_init.hh b/api/api_init.hh index a6adcae1b7..8a38989e51 100644 --- a/api/api_init.hh +++ b/api/api_init.hh @@ -86,6 +86,7 @@ future<> set_server_config(http_context& ctx, const db::config& cfg); future<> set_server_snitch(http_context& ctx, sharded& snitch); future<> unset_server_snitch(http_context& ctx); future<> set_server_storage_service(http_context& ctx, sharded& ss); +future<> unset_server_storage_service(http_context& ctx); future<> set_server_sstables_loader(http_context& ctx, sharded& sst_loader); future<> unset_server_sstables_loader(http_context& ctx); future<> set_server_view_builder(http_context& ctx, sharded& vb); diff --git a/api/storage_service.cc b/api/storage_service.cc index 599ebc5860..731b80747e 100644 --- a/api/storage_service.cc +++ b/api/storage_service.cc @@ -1335,6 +1335,95 @@ void set_storage_service(http_context& ctx, routes& r, sharded parse_tables(const sstring& ks_name, http_context& ctx, con std::vector parse_table_infos(const sstring& ks_name, http_context& ctx, const std::unordered_map& query_params, sstring param_name); void set_storage_service(http_context& ctx, httpd::routes& r, sharded& ss); +void unset_storage_service(http_context& ctx, httpd::routes& r); void set_sstables_loader(http_context& ctx, httpd::routes& r, sharded& sst_loader); void unset_sstables_loader(http_context& ctx, httpd::routes& r); void set_view_builder(http_context& ctx, httpd::routes& r, sharded& vb); diff --git a/main.cc b/main.cc index e131ca8bbc..68bd4780f7 100644 --- a/main.cc +++ b/main.cc @@ -1357,6 +1357,12 @@ To start the scylla server proper, simply invoke as: scylla server (or just scyl auto stop_storage_service = defer_verbose_shutdown("storage_service", [&] { ss.stop().get(); }); + + api::set_server_storage_service(ctx, ss).get(); + auto stop_ss_api = defer_verbose_shutdown("storage service API", [&ctx] { + api::unset_server_storage_service(ctx).get(); + }); + supervisor::notify("initializing virtual tables"); smp::invoke_on_all([&] { return db::initialize_virtual_tables(db, ss, gossiper, raft_gr, sys_ks, *cfg); @@ -1590,7 +1596,6 @@ To start the scylla server proper, simply invoke as: scylla server (or just scyl auto stop_messaging_api = defer_verbose_shutdown("messaging service API", [&ctx] { api::unset_server_messaging_service(ctx).get(); }); - api::set_server_storage_service(ctx, ss).get(); api::set_server_repair(ctx, repair).get(); auto stop_repair_api = defer_verbose_shutdown("repair API", [&ctx] { api::unset_server_repair(ctx).get();