From 5e3fd9fd053a26cafca7b74d310f8f8f9e93fd46 Mon Sep 17 00:00:00 2001 From: Alexey Bashtanov Date: Thu, 7 Nov 2024 08:36:54 +0000 Subject: [PATCH 01/15] rpc: don't wait for the obsolete feature on start That's feature::rpc_transport_unknown_errc. On boostrap, when a node doesn't have all RPC services available yet, it may return non-retriable errors on RPC requests. There is already a mechanism thatmakes RPC return a retriable error on unknown method if some services are still yet to start. Unfortunately, this mechanism requires all nodes to support it, so we check for a feature flag before we use it. As a result, non-retriable errors still can sneak through, as checking the feature flag across all nodes takes some time. This is to remove the feature flag check, as current and previous versions definitely support it. --- src/v/redpanda/application.cc | 14 -------------- src/v/rpc/rpc_server.cc | 2 +- src/v/rpc/rpc_server.h | 3 --- src/v/rpc/test/rpc_gen_cycling_test.cc | 7 ++----- 4 files changed, 3 insertions(+), 23 deletions(-) diff --git a/src/v/redpanda/application.cc b/src/v/redpanda/application.cc index 1948ac767ddc5..f5b3b3e637e35 100644 --- a/src/v/redpanda/application.cc +++ b/src/v/redpanda/application.cc @@ -2915,20 +2915,6 @@ void application::wire_up_and_start(::stop_signal& app_signal, bool test_mode) { void application::start_runtime_services( cluster::cluster_discovery& cd, ::stop_signal& app_signal) { - ssx::background = feature_table.invoke_on_all( - [this](features::feature_table& ft) { - return ft.await_feature_then( - features::feature::rpc_transport_unknown_errc, [this] { - if (ss::this_shard_id() == 0) { - vlog( - _log.debug, "All nodes support unknown RPC error codes"); - } - // Redpanda versions <= v22.3.x don't properly parse error - // codes they don't know about. - _rpc.local().set_use_service_unavailable(); - }); - }); - // single instance node_status_backend.invoke_on_all(&cluster::node_status_backend::start) .get(); diff --git a/src/v/rpc/rpc_server.cc b/src/v/rpc/rpc_server.cc index 43c31a80d54c0..b4221ac4380a4 100644 --- a/src/v/rpc/rpc_server.cc +++ b/src/v/rpc/rpc_server.cc @@ -167,7 +167,7 @@ ss::future<> rpc_server::dispatch_method_once( if (unlikely(it == _services.end())) { ss::sstring msg_suffix; rpc::status s = rpc::status::method_not_found; - if (!_all_services_added && _service_unavailable_allowed) { + if (!_all_services_added) { msg_suffix = " during startup. Ignoring..."; s = rpc::status::service_unavailable; } diff --git a/src/v/rpc/rpc_server.h b/src/v/rpc/rpc_server.h index 80b2db498d533..42897e40a4afc 100644 --- a/src/v/rpc/rpc_server.h +++ b/src/v/rpc/rpc_server.h @@ -37,8 +37,6 @@ class rpc_server : public net::server { void set_all_services_added() { _all_services_added = true; } - void set_use_service_unavailable() { _service_unavailable_allowed = true; } - // Adds the given services to the protocol. // May be called whether or not the server has already been started. void add_services(std::vector> services) { @@ -73,7 +71,6 @@ class rpc_server : public net::server { send_reply_skip_payload(ss::lw_shared_ptr, netbuf); bool _all_services_added{false}; - bool _service_unavailable_allowed{false}; std::vector> _services; }; diff --git a/src/v/rpc/test/rpc_gen_cycling_test.cc b/src/v/rpc/test/rpc_gen_cycling_test.cc index 9e952f533ed92..39718bb439d49 100644 --- a/src/v/rpc/test/rpc_gen_cycling_test.cc +++ b/src/v/rpc/test/rpc_gen_cycling_test.cc @@ -688,12 +688,9 @@ FIXTURE_TEST(missing_method_test, rpc_integration_fixture) { ss::when_all_succeed(requests.begin(), requests.end()).get(); }; - // If the server is configured to allow use of service_unavailable, while - // the server hasn't added all services, we should see a retriable error - // instead of method_not_found. - server().set_use_service_unavailable(); + // While the server hasn't added all services, we should see a retriable + // error instead of method_not_found. verify_bad_method_errors(rpc::errc::service_unavailable); - server().set_all_services_added(); verify_bad_method_errors(rpc::errc::method_not_found); } From 81f47096c45ff06541a25ad0da548c82dc0d00a6 Mon Sep 17 00:00:00 2001 From: Alexey Bashtanov Date: Thu, 7 Nov 2024 09:50:34 +0000 Subject: [PATCH 02/15] feat/tests: use new features to retire older ones --- src/v/features/tests/feature_table_test.cc | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/v/features/tests/feature_table_test.cc b/src/v/features/tests/feature_table_test.cc index 0a61522b6dfdd..0b11eea631c30 100644 --- a/src/v/features/tests/feature_table_test.cc +++ b/src/v/features/tests/feature_table_test.cc @@ -81,10 +81,7 @@ SEASTAR_THREAD_TEST_CASE(feature_table_test_hook_off) { SEASTAR_THREAD_TEST_CASE(feature_table_strings) { BOOST_REQUIRE_EQUAL(to_string_view(feature::test_alpha), mock_feature); BOOST_REQUIRE_EQUAL( - to_string_view(feature::rpc_v2_by_default), "rpc_v2_by_default"); - BOOST_REQUIRE_EQUAL(to_string_view(feature::kafka_gssapi), "kafka_gssapi"); - BOOST_REQUIRE_EQUAL( - to_string_view(feature::node_isolation), "node_isolation"); + to_string_view(feature::audit_logging), "audit_logging"); } /** @@ -315,7 +312,7 @@ FIXTURE_TEST(feature_table_old_snapshot, feature_table_fixture) { snapshot.version = features::feature_table::get_earliest_logical_version(); snapshot.states = { features::feature_state_snapshot{ - .name = "serde_raft_0", + .name = "audit_logging", .state = feature_state::state::available, }, features::feature_state_snapshot{ @@ -328,7 +325,7 @@ FIXTURE_TEST(feature_table_old_snapshot, feature_table_fixture) { // Fast-forwarded feature should still be active. BOOST_CHECK( - ft.get_state(feature::serde_raft_0).get_state() + ft.get_state(feature::audit_logging).get_state() == feature_state::state::active); // A feature with explicit available_policy should be activated by the // snapshot. From 247ce022207646ac651d580231c69c0b59125730 Mon Sep 17 00:00:00 2001 From: Alexey Bashtanov Date: Thu, 7 Nov 2024 10:10:59 +0000 Subject: [PATCH 03/15] redpanda/admin/server: don't check license feature as it is always on in all suppoted versions now --- src/v/cluster/feature_manager.cc | 26 ++++++++++++-------------- src/v/redpanda/admin/server.cc | 18 ------------------ 2 files changed, 12 insertions(+), 32 deletions(-) diff --git a/src/v/cluster/feature_manager.cc b/src/v/cluster/feature_manager.cc index df324241d6ea0..daac9e2c33d6c 100644 --- a/src/v/cluster/feature_manager.cc +++ b/src/v/cluster/feature_manager.cc @@ -299,20 +299,18 @@ ss::future<> feature_manager::maybe_log_license_check_info() { // Shutting down - next iteration will drop out co_return; } - if (_feature_table.local().is_active(features::feature::license)) { - auto enterprise_features = report_enterprise_features(); - if (enterprise_features.any()) { - if (_feature_table.local().should_sanction()) { - vlog( - clusterlog.warn, - "A Redpanda Enterprise Edition license is required to use " - "enterprise features: ([{}]). Enter an active license key " - "(for example, rpk cluster license set ). To request a " - "license, see https://redpanda.com/license-request. For more " - "information, see " - "https://docs.redpanda.com/current/get-started/licenses.", - fmt::join(enterprise_features.enabled(), ", ")); - } + auto enterprise_features = report_enterprise_features(); + if (enterprise_features.any()) { + if (_feature_table.local().should_sanction()) { + vlog( + clusterlog.warn, + "A Redpanda Enterprise Edition license is required to use " + "enterprise features: ([{}]). Enter an active license key " + "(for example, rpk cluster license set ). To request a " + "license, see https://redpanda.com/license-request. For more " + "information, see " + "https://docs.redpanda.com/current/get-started/licenses.", + fmt::join(enterprise_features.enabled(), ", ")); } } } diff --git a/src/v/redpanda/admin/server.cc b/src/v/redpanda/admin/server.cc index a7d48aab4bbd0..9f64c09e718ee 100644 --- a/src/v/redpanda/admin/server.cc +++ b/src/v/redpanda/admin/server.cc @@ -2276,12 +2276,6 @@ admin_server::put_license_handler(std::unique_ptr req) { throw ss::httpd::bad_request_exception( "Missing redpanda license from request body"); } - if (!_controller->get_feature_table().local().is_active( - features::feature::license)) { - throw ss::httpd::bad_request_exception( - "Feature manager reports the cluster is not fully upgraded to " - "accept license put requests"); - } try { boost::trim_if(raw_license, boost::is_any_of(" \n\r")); @@ -2320,12 +2314,6 @@ admin_server::put_license_handler(std::unique_ptr req) { ss::future admin_server::get_enterprise_handler(std::unique_ptr) { - if (!_controller->get_feature_table().local().is_active( - features::feature::license)) { - throw ss::httpd::bad_request_exception( - "Feature manager reports the cluster is not fully upgraded to " - "accept get enterprise requests"); - } using status = ss::httpd::features_json::enterprise_response:: enterprise_response_license_status; @@ -2437,12 +2425,6 @@ void admin_server::register_features_routes() { register_route( ss::httpd::features_json::get_license, [this](std::unique_ptr) { - if (!_controller->get_feature_table().local().is_active( - features::feature::license)) { - throw ss::httpd::bad_request_exception( - "Feature manager reports the cluster is not fully upgraded to " - "accept license get requests"); - } ss::httpd::features_json::license_response res; res.loaded = false; const auto& ft = _controller->get_feature_table().local(); From 1bc4dc78ee1af71b20089b436301d146f45eaf78 Mon Sep 17 00:00:00 2001 From: Alexey Bashtanov Date: Thu, 7 Nov 2024 10:14:13 +0000 Subject: [PATCH 04/15] raft: don't check retired feature feature::raft_improved_configuration is always on in all suppoted versions now --- src/v/raft/consensus.cc | 4 +--- src/v/raft/group_manager.cc | 4 ---- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/src/v/raft/consensus.cc b/src/v/raft/consensus.cc index 9fc3795ecf0d9..99d5d62dbed5e 100644 --- a/src/v/raft/consensus.cc +++ b/src/v/raft/consensus.cc @@ -2659,9 +2659,7 @@ ss::future consensus::replicate_configuration( void consensus::maybe_upgrade_configuration_to_v4(group_configuration& cfg) { if (unlikely(cfg.version() < group_configuration::v_4)) { - if ( - _features.is_active(features::feature::raft_improved_configuration) - && cfg.get_state() == configuration_state::simple) { + if (cfg.get_state() == configuration_state::simple) { vlog(_ctxlog.debug, "Upgrading configuration version"); cfg.set_version(raft::group_configuration::v_4); } diff --git a/src/v/raft/group_manager.cc b/src/v/raft/group_manager.cc index 74f464dd97509..2b40080f14457 100644 --- a/src/v/raft/group_manager.cc +++ b/src/v/raft/group_manager.cc @@ -182,10 +182,6 @@ raft::group_configuration group_manager::create_initial_configuration( // old configuration with brokers auto raft_cfg = raft::group_configuration( std::move(initial_brokers), revision); - if (unlikely(!_feature_table.is_active( - features::feature::raft_improved_configuration))) { - raft_cfg.set_version(group_configuration::v_3); - } return raft_cfg; } From f8f569173c6cc4b699eca3fe1600167d77f72da9 Mon Sep 17 00:00:00 2001 From: Alexey Bashtanov Date: Thu, 7 Nov 2024 10:16:37 +0000 Subject: [PATCH 05/15] c/rm_stm: remove feature::transaction_ga check as it is always on in all suppoted versions now --- src/v/cluster/rm_stm.cc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/v/cluster/rm_stm.cc b/src/v/cluster/rm_stm.cc index 9716a9b97febc..aecb81d61d8f6 100644 --- a/src/v/cluster/rm_stm.cc +++ b/src/v/cluster/rm_stm.cc @@ -91,10 +91,6 @@ rm_stm::rm_stm( , _ctx_log(txlog, ssx::sformat("[{}]", c->ntp())) , _producer_state_manager(producer_state_manager) , _vcluster_id(vcluster_id) { - vassert( - _feature_table.local().is_active(features::feature::transaction_ga), - "unexpected state for transactions support. skipped a few " - "versions during upgrade?"); setup_metrics(); if (!_is_tx_enabled) { _is_autoabort_enabled = false; From 4314c8df8244e8194f2d2252aa15f8ab441fc670 Mon Sep 17 00:00:00 2001 From: Alexey Bashtanov Date: Thu, 7 Nov 2024 10:17:37 +0000 Subject: [PATCH 06/15] c/tx_gateway_frontend: remove dead code --- src/v/cluster/tx_gateway_frontend.h | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/v/cluster/tx_gateway_frontend.h b/src/v/cluster/tx_gateway_frontend.h index ff1ee0931cbf3..f1f8c59022bf3 100644 --- a/src/v/cluster/tx_gateway_frontend.h +++ b/src/v/cluster/tx_gateway_frontend.h @@ -97,14 +97,6 @@ class tx_gateway_frontend final bool _transactions_enabled; config::binding _max_transactions_per_coordinator; - // Transaction GA includes: KIP_447, KIP-360, fix for compaction tx_group* - // records, perf fix#1(Do not writing preparing state on disk in tm_stn), - // perf fix#2(Do not writeing prepared marker in rm_stm) - bool is_transaction_ga() { - return _feature_table.local().is_active( - features::feature::transaction_ga); - } - void start_expire_timer(); void rearm_expire_timer(bool force = false); From 6ab5551d495129cda3e0d2a1852480cfbf8dd270 Mon Sep 17 00:00:00 2001 From: Alexey Bashtanov Date: Thu, 7 Nov 2024 10:18:57 +0000 Subject: [PATCH 07/15] c/node_status_backend: don't check retired feature feature::raftless_node_status is always on in all suppoted versions now --- src/v/cluster/node_status_backend.cc | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/v/cluster/node_status_backend.cc b/src/v/cluster/node_status_backend.cc index 9ade7687e0ea9..2c7663319eb07 100644 --- a/src/v/cluster/node_status_backend.cc +++ b/src/v/cluster/node_status_backend.cc @@ -172,11 +172,6 @@ void node_status_backend::tick() { } ss::future<> node_status_backend::collect_and_store_updates() { - if (!_feature_table.local().is_active( - features::feature::raftless_node_status)) { - co_return; - } - auto updates = co_await collect_updates_from_peers(); co_return co_await _node_status_table.invoke_on_all( [updates = std::move(updates)](auto& table) { From 3f27b93a4e77373b30cb86cb02755d9da2ee590e Mon Sep 17 00:00:00 2001 From: Alexey Bashtanov Date: Thu, 7 Nov 2024 10:50:33 +0000 Subject: [PATCH 08/15] c/members_manager: do not check retired feature feature::node_id_assignment is always on in all suppoted versions now --- src/v/cluster/members_manager.cc | 41 ++------------------------------ 1 file changed, 2 insertions(+), 39 deletions(-) diff --git a/src/v/cluster/members_manager.cc b/src/v/cluster/members_manager.cc index 7a036c841acb0..934e72b7f8d7d 100644 --- a/src/v/cluster/members_manager.cc +++ b/src/v/cluster/members_manager.cc @@ -1224,27 +1224,13 @@ ss::future> members_manager::replicate_new_node_uuid( co_await make_join_node_success_reply(get_node_id(node_uuid))); } -static bool contains_address( - const net::unresolved_address& address, - const members_table::cache_t& brokers) { - return std::find_if( - brokers.begin(), - brokers.end(), - [&address](const auto& p) { - return p.second.broker.rpc_address() == address; - }) - != brokers.end(); -} - ss::future> members_manager::handle_join_request(const join_node_request req) { using ret_t = result; using status_t = join_node_reply::status_code; - bool node_id_assignment_supported = _feature_table.local().is_active( - features::feature::node_id_assignment); bool req_has_node_uuid = !req.node_uuid.empty(); - if (node_id_assignment_supported && !req_has_node_uuid) { + if (!req_has_node_uuid) { vlog( clusterlog.warn, "Invalid join request for node ID {}, node UUID is required", @@ -1255,13 +1241,6 @@ members_manager::handle_join_request(const join_node_request req) { if (req.node.id() >= 0) { req_node_id = req.node.id(); } - if (!node_id_assignment_supported && !req_node_id) { - vlog( - clusterlog.warn, - "Got request to assign node ID, but feature not active", - req.node.id()); - co_return errc::invalid_request; - } if ( req_has_node_uuid && req.node_uuid.size() != model::node_uuid::type::length) { @@ -1324,7 +1303,7 @@ members_manager::handle_join_request(const join_node_request req) { join_node_reply{status_t::not_ready, model::unassigned_node_id}); } - if (likely(node_id_assignment_supported && req_has_node_uuid)) { + if (likely(req_has_node_uuid)) { const auto it = _id_by_uuid.find(node_uuid); if (!req_node_id) { if (it == _id_by_uuid.end()) { @@ -1403,22 +1382,6 @@ members_manager::handle_join_request(const join_node_request req) { }); } - // Older versions of Redpanda don't support having multiple servers pointed - // at the same address. - if ( - !node_id_assignment_supported - && contains_address( - req.node.rpc_address(), _members_table.local().nodes())) { - vlog( - clusterlog.info, - "Broker {} address ({}) conflicts with the address of another " - "node", - req.node.id(), - req.node.rpc_address()); - co_return ret_t( - join_node_reply{status_t::conflict, model::unassigned_node_id}); - } - if (req.node.id() != _self.id()) { co_await update_broker_client( _self.id(), From 77d6b4d2369bc334c4b9aa1b2493aa51d843f712 Mon Sep 17 00:00:00 2001 From: Alexey Bashtanov Date: Thu, 7 Nov 2024 10:52:15 +0000 Subject: [PATCH 09/15] c/topics_frontend: do not check retired feature feature::replication_factor_change is always on in all suppoted versions --- src/v/cluster/topics_frontend.cc | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/v/cluster/topics_frontend.cc b/src/v/cluster/topics_frontend.cc index bfd83501ae6cf..38ef957be347f 100644 --- a/src/v/cluster/topics_frontend.cc +++ b/src/v/cluster/topics_frontend.cc @@ -354,11 +354,6 @@ ss::future topics_frontend::do_update_replication_factor( topic_properties_update& update, model::timeout_clock::time_point timeout) { switch (update.custom_properties.replication_factor.op) { case incremental_update_operation::set: { - if (!_features.local().is_active( - features::feature::replication_factor_change)) { - co_return cluster::errc::feature_disabled; - } - if (_topics.local().is_fully_disabled(update.tp_ns)) { co_return errc::topic_disabled; } From 748de08bd1605e8e006569b7a8f4bca8de9f909b Mon Sep 17 00:00:00 2001 From: Alexey Bashtanov Date: Thu, 7 Nov 2024 10:53:52 +0000 Subject: [PATCH 10/15] c/ephemeral_credential_FE: don't check old feature feature::ephemeral_secrets is always on in all suppoted versions now --- src/v/cluster/ephemeral_credential_frontend.cc | 9 --------- src/v/cluster/tests/ephemeral_credential_test.cc | 5 ----- 2 files changed, 14 deletions(-) diff --git a/src/v/cluster/ephemeral_credential_frontend.cc b/src/v/cluster/ephemeral_credential_frontend.cc index 6109fbb4a0c69..a62c7a7df4417 100644 --- a/src/v/cluster/ephemeral_credential_frontend.cc +++ b/src/v/cluster/ephemeral_credential_frontend.cc @@ -79,15 +79,6 @@ ephemeral_credential_frontend::get(const security::acl_principal& principal) { auto guard = _gate.hold(); get_return res; - if (!_feature_table.local().is_active( - features::feature::ephemeral_secrets)) { - vlog( - clusterlog.info, - "Ephemeral credentials feature is not active (upgrade in progress?)"); - res.err = errc::invalid_node_operation; - co_return res; - } - if (auto it = _e_store.local().find(principal); !_e_store.local().has(it)) { res.credential = make_ephemeral_credential(principal); co_await put(res.credential); diff --git a/src/v/cluster/tests/ephemeral_credential_test.cc b/src/v/cluster/tests/ephemeral_credential_test.cc index 48e092eead954..f1ecbbf967793 100644 --- a/src/v/cluster/tests/ephemeral_credential_test.cc +++ b/src/v/cluster/tests/ephemeral_credential_test.cc @@ -41,11 +41,6 @@ FIXTURE_TEST(test_ephemeral_credential_frontend, cluster_test_fixture) { wait_for_all_members(3s).get(); - tests::cooperative_spin_wait_with_timeout(10s, [&apps] { - return apps.front()->controller->get_feature_table().local().is_active( - features::feature::ephemeral_secrets); - }).get(); - auto& fe_0 = apps[0]->controller->get_ephemeral_credential_frontend().local(); auto& fe_1 From 2ce51dd61d61ad76bbc8389219da43243608c9c8 Mon Sep 17 00:00:00 2001 From: Alexey Bashtanov Date: Thu, 7 Nov 2024 11:00:07 +0000 Subject: [PATCH 11/15] c/controller: do not check old feature feature::seeds_driven_bootstrap_capable is always on in all suppoted versions now --- src/v/cluster/controller.cc | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/v/cluster/controller.cc b/src/v/cluster/controller.cc index 08fc8c1e0dda1..7af6385ba1c85 100644 --- a/src/v/cluster/controller.cc +++ b/src/v/cluster/controller.cc @@ -1037,15 +1037,10 @@ ss::future<> controller::cluster_creation_hook(cluster_discovery& discovery) { // upgraded from a version before 22.3. Replicate just a cluster UUID so we // can advertise that a cluster has already been bootstrapped to nodes // trying to discover existing clusters. + bootstrap_cluster_cmd_data cmd_data; + cmd_data.uuid = model::cluster_uuid(uuid_t::create()); ssx::background - = _feature_table.local() - .await_feature( - features::feature::seeds_driven_bootstrap_capable, _as.local()) - .then([this] { - bootstrap_cluster_cmd_data cmd_data; - cmd_data.uuid = model::cluster_uuid(uuid_t::create()); - return create_cluster(std::move(cmd_data)); - }) + = create_cluster(std::move(cmd_data)) .handle_exception([](const std::exception_ptr e) { vlog(clusterlog.warn, "Error creating cluster UUID. {}", e); }); From 9d0248c89994b1b42bb3cc39e93cafd5674ea678 Mon Sep 17 00:00:00 2001 From: Alexey Bashtanov Date: Thu, 7 Nov 2024 11:02:06 +0000 Subject: [PATCH 12/15] k/server: do not check feature::kafka_gssapi it is always on in all suppoted versions now --- src/v/kafka/server/server.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/v/kafka/server/server.cc b/src/v/kafka/server/server.cc index 48d52947cdda2..522942cebfb4b 100644 --- a/src/v/kafka/server/server.cc +++ b/src/v/kafka/server/server.cc @@ -719,9 +719,7 @@ ss::future sasl_handshake_handler::handle( } } - const bool has_kafka_gssapi = ctx.feature_table().local().is_active( - features::feature::kafka_gssapi); - if (has_kafka_gssapi && supports("GSSAPI")) { + if (supports("GSSAPI")) { supported_sasl_mechanisms.emplace_back( security::gssapi_authenticator::name); From c74937998e615009276dd4bbbad47140743e9075 Mon Sep 17 00:00:00 2001 From: Alexey Bashtanov Date: Thu, 7 Nov 2024 11:11:02 +0000 Subject: [PATCH 13/15] c/controller_backend: do not check old feature feature::partition_move_revert_cancel is always on in all suppoted versions now --- src/v/cluster/controller_backend.cc | 113 +++++----------------------- src/v/cluster/controller_backend.h | 2 - src/v/cluster/feature_manager.cc | 9 +-- 3 files changed, 22 insertions(+), 102 deletions(-) diff --git a/src/v/cluster/controller_backend.cc b/src/v/cluster/controller_backend.cc index 87fafb40ab1ea..640d6c987b900 100644 --- a/src/v/cluster/controller_backend.cc +++ b/src/v/cluster/controller_backend.cc @@ -529,30 +529,6 @@ ss::future do_update_replica_set( co_return co_await p->update_replica_set(std::move(brokers), cmd_revision); } -ss::future revert_configuration_update( - ss::lw_shared_ptr p, - const replicas_t& replicas, - const replicas_revision_map& replica_revisions, - model::revision_id cmd_revision, - members_table& members, - bool command_based_members_update) { - vlog( - clusterlog.debug, - "[{}] reverting already finished reconfiguration. Revision: {}, replica " - "set: {}", - p->ntp(), - cmd_revision, - replicas); - return do_update_replica_set( - std::move(p), - replicas, - replica_revisions, - cmd_revision, - members, - command_based_members_update, - std::nullopt); -} - /** * Retrieve topic property based on the following logic * @@ -1270,7 +1246,6 @@ ss::future> controller_backend::reconcile_ntp_step( replicas_view.last_cmd_revision(), op_type, replicas_view.assignment); co_return co_await reconcile_partition_reconfiguration( - rs, std::move(partition), *replicas_view.update, replicas_view.revisions()); @@ -1281,7 +1256,6 @@ ss::future> controller_backend::reconcile_ntp_step( ss::future> controller_backend::reconcile_partition_reconfiguration( - ntp_reconciliation_state& rs, ss::lw_shared_ptr partition, const topic_table::in_progress_update& update, const replicas_revision_map& replicas_revisions) { @@ -1305,19 +1279,14 @@ controller_backend::reconcile_partition_reconfiguration( _self, partition, update.get_resulting_replicas(), cmd_revision); if (!update_ec) { auto leader = partition->get_leader_id(); - size_t retries = (rs.cur_operation ? rs.cur_operation->retries : 0); vlog( clusterlog.trace, "[{}] update complete, checking if our node can finish it " - "(leader: {}, retry: {})", + "(leader: {})", partition->ntp(), - leader, - retries); + leader); if (can_finish_update( - leader, - retries, - update.get_state(), - update.get_resulting_replicas())) { + leader, update.get_state(), update.get_resulting_replicas())) { auto ec = co_await dispatch_update_finished( partition->ntp(), update.get_resulting_replicas()); if (ec) { @@ -1372,7 +1341,6 @@ controller_backend::reconcile_partition_reconfiguration( bool controller_backend::can_finish_update( std::optional current_leader, - uint64_t current_retry, reconfiguration_state state, const replicas_t& current_replicas) { if ( @@ -1382,23 +1350,9 @@ bool controller_backend::can_finish_update( return current_leader == _self; } /** - * If the revert feature is active we use current leader to dispatch - * partition move - */ - if (_features.local().is_active( - features::feature::partition_move_revert_cancel)) { - return current_leader == _self - && contains_node(current_replicas, _self); - } - /** - * Use retry count to determine which node is eligible to dispatch update - * finished. Using modulo division allow us to round robin between - * candidates + * Use current leader to dispatch partition move */ - const model::broker_shard& candidate - = current_replicas[current_retry % current_replicas.size()]; - - return candidate.node_id == _self; + return current_leader == _self && contains_node(current_replicas, _self); } ss::future controller_backend::create_partition( @@ -1613,28 +1567,14 @@ controller_backend::cancel_replica_set_update( return result{ec}; }); } else if (already_moved) { - if (likely(_features.local().is_active( - features::feature::partition_move_revert_cancel))) { - return dispatch_revert_cancel_move(p->ntp()).then( - [](std::error_code ec) -> result { - if (ec) { - return ec; - } - // revert_cancel is dispatched, nothing else to do, - // but wait for the topic table update. - return ss::stop_iteration::yes; - }); - } - - return revert_configuration_update( - std::move(p), - replicas, - replicas_revisions, - cmd_revision, - _members_table.local(), - command_based_membership_active()) - .then([](std::error_code ec) { - return result{ec}; + return dispatch_revert_cancel_move(p->ntp()).then( + [](std::error_code ec) -> result { + if (ec) { + return ec; + } + // revert_cancel is dispatched, nothing else to do, + // but wait for the topic table update. + return ss::stop_iteration::yes; }); } return ss::make_ready_future>( @@ -1704,29 +1644,12 @@ controller_backend::force_abort_replica_set_update( cmd_revision, reconfiguration_policy::full_local_retention); } else if (already_moved) { - if (likely(_features.local().is_active( - features::feature::partition_move_revert_cancel))) { - std::error_code ec = co_await dispatch_revert_cancel_move( - partition->ntp()); - if (ec) { - co_return ec; - } - co_return ss::stop_iteration::yes; + std::error_code ec = co_await dispatch_revert_cancel_move( + partition->ntp()); + if (ec) { + co_return ec; } - - co_return co_await apply_configuration_change_on_leader( - std::move(partition), - replicas, - cmd_revision, - [&](ss::lw_shared_ptr p) { - return revert_configuration_update( - std::move(p), - replicas, - replicas_revisions, - cmd_revision, - _members_table.local(), - command_based_membership_active()); - }); + co_return ss::stop_iteration::yes; } co_return errc::waiting_for_recovery; } else { diff --git a/src/v/cluster/controller_backend.h b/src/v/cluster/controller_backend.h index 4107757f0db36..c823cab69e9ee 100644 --- a/src/v/cluster/controller_backend.h +++ b/src/v/cluster/controller_backend.h @@ -327,7 +327,6 @@ class controller_backend shard_placement_table&); ss::future> reconcile_partition_reconfiguration( - ntp_reconciliation_state&, ss::lw_shared_ptr, const topic_table::in_progress_update&, const replicas_revision_map& replicas_revisions); @@ -365,7 +364,6 @@ class controller_backend bool can_finish_update( std::optional current_leader, - uint64_t current_retry, reconfiguration_state, const replicas_t& requested_replicas); diff --git a/src/v/cluster/feature_manager.cc b/src/v/cluster/feature_manager.cc index daac9e2c33d6c..3d4df3da4a5e3 100644 --- a/src/v/cluster/feature_manager.cc +++ b/src/v/cluster/feature_manager.cc @@ -305,11 +305,10 @@ ss::future<> feature_manager::maybe_log_license_check_info() { vlog( clusterlog.warn, "A Redpanda Enterprise Edition license is required to use " - "enterprise features: ([{}]). Enter an active license key " - "(for example, rpk cluster license set ). To request a " - "license, see https://redpanda.com/license-request. For more " - "information, see " - "https://docs.redpanda.com/current/get-started/licenses.", + "enterprise features: ([{}]). Enter an active license key (for " + "example, rpk cluster license set ). To request a license, " + "see https://redpanda.com/license-request. For more information, " + "see https://docs.redpanda.com/current/get-started/licenses.", fmt::join(enterprise_features.enabled(), ", ")); } } From 61cd125122b57755b5cc0bc1d0cc46f96c40cbcf Mon Sep 17 00:00:00 2001 From: Alexey Bashtanov Date: Thu, 7 Nov 2024 11:25:32 +0000 Subject: [PATCH 14/15] k/server: do not check feature::node_isolation it is always on in all suppoted versions now --- src/v/kafka/server/BUILD | 1 - .../handlers/details/isolated_node_utils.h | 37 ------------------- src/v/kafka/server/handlers/metadata.cc | 8 +++- 3 files changed, 6 insertions(+), 40 deletions(-) delete mode 100644 src/v/kafka/server/handlers/details/isolated_node_utils.h diff --git a/src/v/kafka/server/BUILD b/src/v/kafka/server/BUILD index e8799e2c5a742..3bc722038251d 100644 --- a/src/v/kafka/server/BUILD +++ b/src/v/kafka/server/BUILD @@ -103,7 +103,6 @@ redpanda_cc_library( "handlers/describe_log_dirs.h", "handlers/describe_producers.h", "handlers/describe_transactions.h", - "handlers/details/isolated_node_utils.h", "handlers/details/leader_epoch.h", "handlers/details/security.h", "handlers/end_txn.h", diff --git a/src/v/kafka/server/handlers/details/isolated_node_utils.h b/src/v/kafka/server/handlers/details/isolated_node_utils.h deleted file mode 100644 index c464025ad682e..0000000000000 --- a/src/v/kafka/server/handlers/details/isolated_node_utils.h +++ /dev/null @@ -1,37 +0,0 @@ -/* - * Copyright 2022 Redpanda Data, Inc. - * - * Use of this software is governed by the Business Source License - * included in the file licenses/BSL.md - * - * As of the Change Date specified in that file, in accordance with - * the Business Source License, use of this software will be governed - * by the Apache License, Version 2.0 - */ - -#pragma once - -#include "base/seastarx.h" -#include "cluster/metadata_cache.h" -#include "features/feature_table.h" -#include "kafka/server/request_context.h" - -namespace kafka { - -using is_node_isolated_or_decommissioned - = ss::bool_class; - -inline is_node_isolated_or_decommissioned -node_isolated_or_decommissioned(request_context& ctx) { - auto isoalted_or_decommissioned = is_node_isolated_or_decommissioned::no; - if (ctx.feature_table().local().is_active( - features::feature::node_isolation)) { - isoalted_or_decommissioned = ctx.metadata_cache().is_node_isolated() - ? is_node_isolated_or_decommissioned::yes - : is_node_isolated_or_decommissioned::no; - } - - return isoalted_or_decommissioned; -} - -} // namespace kafka diff --git a/src/v/kafka/server/handlers/metadata.cc b/src/v/kafka/server/handlers/metadata.cc index 4b6e3cefefa09..c97beaf9968d1 100644 --- a/src/v/kafka/server/handlers/metadata.cc +++ b/src/v/kafka/server/handlers/metadata.cc @@ -19,7 +19,6 @@ #include "kafka/protocol/schemata/metadata_response.h" #include "kafka/server/errors.h" #include "kafka/server/fwd.h" -#include "kafka/server/handlers/details/isolated_node_utils.h" #include "kafka/server/handlers/details/leader_epoch.h" #include "kafka/server/handlers/details/security.h" #include "kafka/server/handlers/topics/topic_utils.h" @@ -39,6 +38,10 @@ #include #include +namespace { +using is_node_isolated_or_decommissioned + = ss::bool_class; +} namespace kafka { static constexpr model::node_id no_leader(-1); @@ -509,7 +512,8 @@ static ss::future fill_info_about_brokers_and_controller_id( template<> ss::future metadata_handler::handle( request_context ctx, [[maybe_unused]] ss::smp_service_group g) { - auto isolated_or_decommissioned = node_isolated_or_decommissioned(ctx); + is_node_isolated_or_decommissioned isolated_or_decommissioned{ + ctx.metadata_cache().is_node_isolated()}; auto reply = co_await fill_info_about_brokers_and_controller_id( ctx, isolated_or_decommissioned); From 40e7af355f769355e24f472a28294575a3a42f8e Mon Sep 17 00:00:00 2001 From: Alexey Bashtanov Date: Thu, 7 Nov 2024 11:39:55 +0000 Subject: [PATCH 15/15] feature_table: remove unused feature flags --- src/v/features/feature_table.cc | 28 -------- src/v/features/feature_table.h | 111 ++++---------------------------- 2 files changed, 13 insertions(+), 126 deletions(-) diff --git a/src/v/features/feature_table.cc b/src/v/features/feature_table.cc index d3cfcde79943a..1d8afb5e701ec 100644 --- a/src/v/features/feature_table.cc +++ b/src/v/features/feature_table.cc @@ -36,40 +36,12 @@ namespace features { std::string_view to_string_view(feature f) { switch (f) { - case feature::serde_raft_0: - return "serde_raft_0"; - case feature::license: - return "license"; - case feature::raft_improved_configuration: - return "raft_improved_configuration"; - case feature::transaction_ga: - return "transaction_ga"; - case feature::raftless_node_status: - return "raftless_node_status"; - case feature::rpc_v2_by_default: - return "rpc_v2_by_default"; case feature::cloud_retention: return "cloud_retention"; - case feature::node_id_assignment: - return "node_id_assignment"; - case feature::replication_factor_change: - return "replication_factor_change"; - case feature::ephemeral_secrets: - return "ephemeral_secrets"; - case feature::seeds_driven_bootstrap_capable: - return "seeds_driven_bootstrap_capable"; - case feature::tm_stm_cache: - return "tm_stm_cache"; - case feature::kafka_gssapi: - return "kafka_gssapi"; - case feature::partition_move_revert_cancel: - return "partition_move_cancel_revert"; case feature::node_isolation: return "node_isolation"; case feature::group_offset_retention: return "group_offset_retention"; - case feature::rpc_transport_unknown_errc: - return "rpc_transport_unknown_errc"; case feature::membership_change_controller_cmds: return "membership_change_controller_cmds"; case feature::controller_snapshots: diff --git a/src/v/features/feature_table.h b/src/v/features/feature_table.h index fd3136e6a05e3..c52f86503f0a4 100644 --- a/src/v/features/feature_table.h +++ b/src/v/features/feature_table.h @@ -38,23 +38,9 @@ struct feature_table_snapshot; /// only used at runtime. Therefore it is safe to re-use an integer that /// has been made available by another feature being retired. enum class feature : std::uint64_t { - serde_raft_0 = 1ULL << 5U, - license = 1ULL << 6U, - raft_improved_configuration = 1ULL << 7U, - transaction_ga = 1ULL << 8U, - raftless_node_status = 1ULL << 9U, - rpc_v2_by_default = 1ULL << 10U, cloud_retention = 1ULL << 11U, - node_id_assignment = 1ULL << 12U, - replication_factor_change = 1ULL << 13U, - ephemeral_secrets = 1ULL << 14U, - seeds_driven_bootstrap_capable = 1ULL << 15U, - tm_stm_cache = 1ULL << 16U, - kafka_gssapi = 1ULL << 17U, - partition_move_revert_cancel = 1ULL << 18U, node_isolation = 1ULL << 19U, group_offset_retention = 1ULL << 20U, - rpc_transport_unknown_errc = 1ULL << 21U, membership_change_controller_cmds = 1ULL << 22U, controller_snapshots = 1ULL << 23U, cloud_storage_manifest_format_v2 = 1ULL << 24U, @@ -109,6 +95,19 @@ inline const std::unordered_set retired_features = { "idempotency_v2", "transaction_partitioning", "lightweight_heartbeats", + "serde_raft_0", + "license", + "raft_improved_configuration", + "raftless_node_status", + "rpc_v2_by_default", + "node_id_assignment", + "replication_factor_change", + "ephemeral_secrets", + "seeds_driven_bootstrap_capable", + "tm_stm_cache", + "kafka_gssapi", + "partition_move_revert_cancel", + "rpc_transport_unknown_errc", }; // The latest_version associated with past releases. Increment this @@ -220,90 +219,12 @@ struct feature_spec { }; inline constexpr std::array feature_schema{ - feature_spec{ - release_version::v22_2_1, - "serde_raft_0", - feature::serde_raft_0, - feature_spec::available_policy::always, - feature_spec::prepare_policy::always}, - feature_spec{ - release_version::v22_2_1, - "license", - feature::license, - feature_spec::available_policy::always, - feature_spec::prepare_policy::always}, - feature_spec{ - release_version::v22_2_1, - "raft_improved_configuration", - feature::raft_improved_configuration, - feature_spec::available_policy::always, - feature_spec::prepare_policy::always}, - feature_spec{ - release_version::v22_2_6, - "transaction_ga", - feature::transaction_ga, - feature_spec::available_policy::always, - feature_spec::prepare_policy::always}, - feature_spec{ - release_version::v22_3_1, - "raftless_node_status", - feature::raftless_node_status, - feature_spec::available_policy::always, - feature_spec::prepare_policy::always}, - feature_spec{ - release_version::v22_3_1, - "rpc_v2_by_default", - feature::rpc_v2_by_default, - feature_spec::available_policy::always, - feature_spec::prepare_policy::always}, feature_spec{ release_version::v22_3_1, "cloud_retention", feature::cloud_retention, feature_spec::available_policy::always, feature_spec::prepare_policy::requires_migration}, - feature_spec{ - release_version::v22_3_1, - "node_id_assignment", - feature::node_id_assignment, - feature_spec::available_policy::always, - feature_spec::prepare_policy::always}, - feature_spec{ - release_version::v22_3_1, - "replication_factor_change", - feature::replication_factor_change, - feature_spec::available_policy::always, - feature_spec::prepare_policy::always}, - feature_spec{ - release_version::v22_3_1, - "ephemeral_secrets", - feature::ephemeral_secrets, - feature_spec::available_policy::always, - feature_spec::prepare_policy::always}, - feature_spec{ - release_version::v22_3_1, - "seeds_driven_bootstrap_capable", - feature::seeds_driven_bootstrap_capable, - feature_spec::available_policy::always, - feature_spec::prepare_policy::always}, - feature_spec{ - release_version::v22_3_6, - "tm_stm_cache", - feature::tm_stm_cache, - feature_spec::available_policy::always, - feature_spec::prepare_policy::always}, - feature_spec{ - release_version::v23_1_1, - "kafka_gssapi", - feature::kafka_gssapi, - feature_spec::available_policy::always, - feature_spec::prepare_policy::always}, - feature_spec{ - release_version::v23_1_1, - "partition_move_revert_cancel", - feature::partition_move_revert_cancel, - feature_spec::available_policy::always, - feature_spec::prepare_policy::always}, feature_spec{ release_version::v23_1_1, "node_isolation", @@ -316,12 +237,6 @@ inline constexpr std::array feature_schema{ feature::group_offset_retention, feature_spec::available_policy::always, feature_spec::prepare_policy::always}, - feature_spec{ - release_version::v23_1_1, - "rpc_transport_unknown_errc", - feature::rpc_transport_unknown_errc, - feature_spec::available_policy::always, - feature_spec::prepare_policy::always}, feature_spec{ release_version::v23_2_1, "membership_change_controller_cmds",