Remove legacy distribution hash fallback

Was used to handle rolling upgrades between versions with different
semantics a long time ago on the 7 branch.

7 files changed, 29 insertions, 219 deletions

diff --git a/storage/src/tests/bucketdb/bucketmanagertest.cpp b/storage/src/tests/bucketdb/bucketmanagertest.cpp
index 7491a3aae1d..05c5e9e8850 100644
--- a/storage/src/tests/bucketdb/bucketmanagertest.cpp
+++ b/storage/src/tests/bucketdb/bucketmanagertest.cpp
@@ -693,8 +693,7 @@ public:
         _self._top->getRepliesOnce();
     }
 
-    // TODO remove on Vespa 8 - this is a workaround for https://github.com/vespa-engine/vespa/issues/8475
-    std::unique_ptr<lib::Distribution> default_grouped_distribution() {
+    static std::unique_ptr<lib::Distribution> default_grouped_distribution() {
         return std::make_unique<lib::Distribution>(
                 GlobalBucketSpaceDistributionConverter::string_to_config(vespalib::string(
 R"(redundancy 2
@@ -718,16 +717,16 @@ group[2].nodes[2].index 5
 )")));
     }
 
-    std::shared_ptr<lib::Distribution> derived_global_grouped_distribution(bool use_legacy) {
+    static std::shared_ptr<lib::Distribution> derived_global_grouped_distribution() {
         auto default_distr = default_grouped_distribution();
-        return  GlobalBucketSpaceDistributionConverter::convert_to_global(*default_distr, use_legacy);
+        return  GlobalBucketSpaceDistributionConverter::convert_to_global(*default_distr);
     }
 
     void set_grouped_distribution_configs() {
         auto default_distr = default_grouped_distribution();
         _self._node->getComponentRegister().getBucketSpaceRepo()
                 .get(document::FixedBucketSpaces::default_space()).setDistribution(std::move(default_distr));
-        auto global_distr = derived_global_grouped_distribution(false);
+        auto global_distr = derived_global_grouped_distribution();
         _self._node->getComponentRegister().getBucketSpaceRepo()
                 .get(document::FixedBucketSpaces::global_space()).setDistribution(std::move(global_distr));
     }
@@ -1207,21 +1206,6 @@ TEST_F(BucketManagerTest, db_not_iterated_when_all_requests_rejected) {
     auto replies = fixture.awaitAndGetReplies(1);
 }
 
-// TODO remove on Vespa 8 - this is a workaround for https://github.com/vespa-engine/vespa/issues/8475
-TEST_F(BucketManagerTest, fall_back_to_legacy_global_distribution_hash_on_mismatch) {
-    ConcurrentOperationFixture f(*this);
-
-    f.set_grouped_distribution_configs();
-
-    auto legacy_hash = f.derived_global_grouped_distribution(true)->getNodeGraph().getDistributionConfigHash();
-
-    auto infoCmd = f.createFullFetchCommandWithHash(document::FixedBucketSpaces::global_space(), legacy_hash);
-    _top->sendDown(infoCmd);
-    auto replies = f.awaitAndGetReplies(1);
-    auto& reply = dynamic_cast<api::RequestBucketInfoReply&>(*replies[0]);
-    EXPECT_EQ(api::ReturnCode::OK, reply.getResult().getResult()); // _not_ REJECTED
-}
-
 // It's possible for the request processing thread and onSetSystemState (which use
 // the same mutex) to race with the actual internal component cluster state switch-over.
 // Ensure we detect and handle this by bouncing the request back to the distributor.
diff --git a/storage/src/tests/common/global_bucket_space_distribution_converter_test.cpp b/storage/src/tests/common/global_bucket_space_distribution_converter_test.cpp
index cd70aecd1bb..bd92e6f0c00 100644
--- a/storage/src/tests/common/global_bucket_space_distribution_converter_test.cpp
+++ b/storage/src/tests/common/global_bucket_space_distribution_converter_test.cpp
@@ -13,9 +13,9 @@ using DistributionConfig = vespa::config::content::StorDistributionConfig;
 
 namespace {
 
-vespalib::string default_to_global_config(const vespalib::string& default_config, bool legacy_mode = false) {
+vespalib::string default_to_global_config(const vespalib::string& default_config) {
     auto default_cfg = GlobalBucketSpaceDistributionConverter::string_to_config(default_config);
-    auto as_global = GlobalBucketSpaceDistributionConverter::convert_to_global(*default_cfg, legacy_mode);
+    auto as_global = GlobalBucketSpaceDistributionConverter::convert_to_global(*default_cfg);
     return GlobalBucketSpaceDistributionConverter::config_to_string(*as_global);
 }
 
@@ -355,64 +355,4 @@ group[2].nodes[1].index 2
     }
 }
 
-// By "legacy" read "broken", but we need to be able to generate it to support rolling upgrades properly.
-// TODO remove on Vespa 8 - this is a workaround for https://github.com/vespa-engine/vespa/issues/8475
-TEST(GlobalBucketSpaceDistributionConverterTest, can_generate_config_with_legacy_partition_spec) {
-    vespalib::string default_config(
-R"(redundancy 2
-group[3]
-group[0].name "invalid"
-group[0].index "invalid"
-group[0].partitions 1|*
-group[0].nodes[0]
-group[1].name rack0
-group[1].index 0
-group[1].nodes[3]
-group[1].nodes[0].index 0
-group[1].nodes[1].index 1
-group[1].nodes[2].index 2
-group[2].name rack1
-group[2].index 1
-group[2].nodes[3]
-group[2].nodes[0].index 3
-group[2].nodes[1].index 4
-group[2].nodes[2].index 5
-)");
-
-    vespalib::string expected_global_config(
-R"(redundancy 6
-initial_redundancy 0
-ensure_primary_persisted true
-ready_copies 6
-active_per_leaf_group true
-distributor_auto_ownership_transfer_on_whole_group_down true
-group[0].index "invalid"
-group[0].name "invalid"
-group[0].capacity 1
-group[0].partitions "3|3|*"
-group[1].index "0"
-group[1].name "rack0"
-group[1].capacity 1
-group[1].partitions ""
-group[1].nodes[0].index 0
-group[1].nodes[0].retired false
-group[1].nodes[1].index 1
-group[1].nodes[1].retired false
-group[1].nodes[2].index 2
-group[1].nodes[2].retired false
-group[2].index "1"
-group[2].name "rack1"
-group[2].capacity 1
-group[2].partitions ""
-group[2].nodes[0].index 3
-group[2].nodes[0].retired false
-group[2].nodes[1].index 4
-group[2].nodes[1].retired false
-group[2].nodes[2].index 5
-group[2].nodes[2].retired false
-disk_distribution MODULO_BID
-)");
-    EXPECT_EQ(expected_global_config, default_to_global_config(default_config, true));
 }
-
-}
\ No newline at end of file
diff --git a/storage/src/tests/distributor/top_level_bucket_db_updater_test.cpp b/storage/src/tests/distributor/top_level_bucket_db_updater_test.cpp
index 1632867b627..014827cb03c 100644
--- a/storage/src/tests/distributor/top_level_bucket_db_updater_test.cpp
+++ b/storage/src/tests/distributor/top_level_bucket_db_updater_test.cpp
@@ -2298,53 +2298,6 @@ TEST_F(TopLevelBucketDBUpdaterTest, batch_update_from_distributor_change_does_no
                       "0:5/1/2/3|1:5/7/8/9", true));
 }
 
-// TODO remove on Vespa 8 - this is a workaround for https://github.com/vespa-engine/vespa/issues/8475
-TEST_F(TopLevelBucketDBUpdaterTest, global_distribution_hash_falls_back_to_legacy_format_upon_request_rejection) {
-    set_distribution(dist_config_6_nodes_across_2_groups());
-
-    const vespalib::string current_hash = "(0d*|*(0;0;1;2)(1;3;4;5))";
-    const vespalib::string legacy_hash = "(0d3|3|*(0;0;1;2)(1;3;4;5))";
-
-    set_cluster_state("distributor:6 storage:6");
-    ASSERT_EQ(message_count(6), _sender.commands().size());
-
-    api::RequestBucketInfoCommand* global_req = nullptr;
-    for (auto& cmd : _sender.commands()) {
-        auto& req_cmd = dynamic_cast<api::RequestBucketInfoCommand&>(*cmd);
-        if (req_cmd.getBucketSpace() == document::FixedBucketSpaces::global_space()) {
-            global_req = &req_cmd;
-            break;
-        }
-    }
-    ASSERT_TRUE(global_req != nullptr);
-    ASSERT_EQ(current_hash, global_req->getDistributionHash());
-
-    auto reply = std::make_shared<api::RequestBucketInfoReply>(*global_req);
-    reply->setResult(api::ReturnCode::REJECTED);
-    bucket_db_updater().onRequestBucketInfoReply(reply);
-
-    fake_clock().addSecondsToTime(10);
-    bucket_db_updater().resend_delayed_messages();
-
-    // Should now be a resent request with legacy distribution hash
-    ASSERT_EQ(message_count(6) + 1, _sender.commands().size());
-    auto& legacy_req = dynamic_cast<api::RequestBucketInfoCommand&>(*_sender.commands().back());
-    ASSERT_EQ(legacy_hash, legacy_req.getDistributionHash());
-
-    // Now if we reject it _again_ we should cycle back to the current hash
-    // in case it wasn't a hash-based rejection after all. And the circle of life continues.
-    reply = std::make_shared<api::RequestBucketInfoReply>(legacy_req);
-    reply->setResult(api::ReturnCode::REJECTED);
-    bucket_db_updater().onRequestBucketInfoReply(reply);
-
-    fake_clock().addSecondsToTime(10);
-    bucket_db_updater().resend_delayed_messages();
-
-    ASSERT_EQ(message_count(6) + 2, _sender.commands().size());
-    auto& new_current_req = dynamic_cast<api::RequestBucketInfoCommand&>(*_sender.commands().back());
-    ASSERT_EQ(current_hash, new_current_req.getDistributionHash());
-}
-
 namespace {
 
 template <typename Func>
diff --git a/storage/src/vespa/storage/bucketdb/bucketmanager.cpp b/storage/src/vespa/storage/bucketdb/bucketmanager.cpp
index 73ddeeb888b..166bda1adbb 100644
--- a/storage/src/vespa/storage/bucketdb/bucketmanager.cpp
+++ b/storage/src/vespa/storage/bucketdb/bucketmanager.cpp
@@ -575,26 +575,12 @@ BucketManager::processRequestBucketInfoCommands(document::BucketSpace bucketSpac
                   << "from version " << _firstEqualClusterStateVersion
                   << " differs from this state.";
         } else if (!their_hash.empty() && their_hash != our_hash) {
-            // Empty hash indicates request from 4.2 protocol or earlier
-            // TODO remove on Vespa 8 - this is a workaround for https://github.com/vespa-engine/vespa/issues/8475
-            bool matches_legacy_hash = false;
-            if (bucketSpace == document::FixedBucketSpaces::global_space()) {
-                const auto default_distr =_component.getBucketSpaceRepo()
-                        .get(document::FixedBucketSpaces::default_space()).getDistribution();
-                // Convert in legacy distribution mode, which will accept old 'hash' structure.
-                const auto legacy_global_distr = GlobalBucketSpaceDistributionConverter::convert_to_global(
-                        *default_distr, true/*use legacy mode*/);
-                const auto legacy_hash = legacy_global_distr->getNodeGraph().getDistributionConfigHash();
-                LOG(debug, "Falling back to comparing against legacy distribution hash: %s", legacy_hash.c_str());
-                matches_legacy_hash = (their_hash == legacy_hash);
-            }
-            if (!matches_legacy_hash) {
-                error << "Distribution config has changed since request.";
-            }
+            // Mismatching config hash indicates nodes are out of sync with their config generations
+            error << "Distributor config hash is not equal to our own; must reject request (our hash: "
+                  << our_hash.c_str() << ", their hash: " << their_hash.c_str() << ")";
         }
         if (error.str().empty()) {
-            std::pair<std::set<uint16_t>::iterator, bool> result(
-                    seenDistributors.insert((*it)->getDistributor()));
+            auto result = seenDistributors.insert((*it)->getDistributor());
             if (result.second) {
                 requests[(*it)->getDistributor()] = *it;
                 continue;
@@ -619,10 +605,7 @@ BucketManager::processRequestBucketInfoCommands(document::BucketSpace bucketSpac
     }
 
     std::ostringstream distrList;
-    std::unordered_map<
-        uint16_t,
-        api::RequestBucketInfoReply::EntryVector
-    > result;
+    std::unordered_map<uint16_t, api::RequestBucketInfoReply::EntryVector> result;
     for (auto& nodeAndCmd : requests) {
         result[nodeAndCmd.first];
         if (LOG_WOULD_LOG(debug)) {
@@ -641,8 +624,8 @@ BucketManager::processRequestBucketInfoCommands(document::BucketSpace bucketSpac
         requests.size(), distrList.str().c_str(),
         clusterState->toString().c_str());
    framework::MilliSecTimer runStartTime(_component.getClock());
-        // Don't allow logging to lower performance of inner loop.
-        // Call other type of instance if logging
+    // Don't allow logging to lower performance of inner loop.
+    // Call other type of instance if logging
     const document::BucketIdFactory& idFac(_component.getBucketIdFactory());
     if (LOG_WOULD_LOG(spam)) {
         DistributorInfoGatherer<true> builder(
@@ -655,8 +638,7 @@ BucketManager::processRequestBucketInfoCommands(document::BucketSpace bucketSpac
         _component.getBucketDatabase(bucketSpace).for_each_chunked(std::ref(builder),
                         "BucketManager::processRequestBucketInfoCommands-2");
     }
-    _metrics->fullBucketInfoLatency.addValue(
-            runStartTime.getElapsedTimeAsDouble());
+    _metrics->fullBucketInfoLatency.addValue(runStartTime.getElapsedTimeAsDouble());
     for (auto& nodeAndCmd : requests) {
         auto reply(std::make_shared<api::RequestBucketInfoReply>(
                 *nodeAndCmd.second));
diff --git a/storage/src/vespa/storage/common/global_bucket_space_distribution_converter.cpp b/storage/src/vespa/storage/common/global_bucket_space_distribution_converter.cpp
index a8c6bf6529c..832bd33de42 100644
--- a/storage/src/vespa/storage/common/global_bucket_space_distribution_converter.cpp
+++ b/storage/src/vespa/storage/common/global_bucket_space_distribution_converter.cpp
@@ -58,21 +58,6 @@ vespalib::string sub_groups_to_partition_spec(const Group& parent) {
     return spec.str();
 }
 
-// Allow generating legacy (broken) partition specs that may be used transiently
-// during rolling upgrades from a pre-fix version to a post-fix version.
-// TODO remove on Vespa 8 - this is a workaround for https://github.com/vespa-engine/vespa/issues/8475
-vespalib::string sub_groups_to_legacy_partition_spec(const Group& parent) {
-    vespalib::asciistream partitions;
-    // In case of a flat cluster config, this ends up with a partition spec of '*',
-    // which is fine. It basically means "put all replicas in this group", which
-    // happens to be exactly what we want.
-    for (auto& child : parent.sub_groups) {
-        partitions << child.second->nested_leaf_count << '|';
-    }
-    partitions << '*';
-    return partitions.str();
-}
-
 bool is_leaf_group(const DistributionConfigBuilder::Group& g) noexcept {
     return !g.nodes.empty();
 }
@@ -101,31 +86,19 @@ void insert_new_group_into_tree(
 
 void build_transformed_root_group(DistributionConfigBuilder& builder,
                                   const DistributionConfigBuilder::Group& config_source_root,
-                                  const Group& parsed_root,
-                                  bool legacy_mode) {
+                                  const Group& parsed_root) {
     DistributionConfigBuilder::Group new_root(config_source_root);
-    if (!legacy_mode) {
-        new_root.partitions = sub_groups_to_partition_spec(parsed_root);
-    } else {
-        // TODO remove on Vespa 8 - this is a workaround for https://github.com/vespa-engine/vespa/issues/8475
-        new_root.partitions = sub_groups_to_legacy_partition_spec(parsed_root);
-    }
+    new_root.partitions = sub_groups_to_partition_spec(parsed_root);
     builder.group.emplace_back(std::move(new_root));
 }
 
 void build_transformed_non_root_group(DistributionConfigBuilder& builder,
                                       const DistributionConfigBuilder::Group& config_source_group,
-                                      const Group& parsed_root,
-                                      bool legacy_mode) {
+                                      const Group& parsed_root) {
     DistributionConfigBuilder::Group new_group(config_source_group);
     if (!is_leaf_group(config_source_group)) { // Partition specs only apply to inner nodes
         const auto& g = find_non_root_group_by_index(config_source_group.index, parsed_root);
-        if (!legacy_mode) {
-            new_group.partitions = sub_groups_to_partition_spec(g);
-        } else {
-            // TODO remove on Vespa 8 - this is a workaround for https://github.com/vespa-engine/vespa/issues/8475
-            new_group.partitions = sub_groups_to_legacy_partition_spec(g);
-        }
+        new_group.partitions = sub_groups_to_partition_spec(g);
     }
     builder.group.emplace_back(std::move(new_group));
 }
@@ -161,16 +134,16 @@ std::unique_ptr<Group> create_group_tree_from_config(const DistributionConfig& s
  * transitively, its parents again etc) have already been processed. This directly
  * implies that the root group is always the first group present in the config.
  */
-void build_global_groups(DistributionConfigBuilder& builder, const DistributionConfig& source, bool legacy_mode) {
+void build_global_groups(DistributionConfigBuilder& builder, const DistributionConfig& source) {
     assert(!source.group.empty()); // TODO gracefully handle empty config?
     auto root = create_group_tree_from_config(source);
 
     auto g_iter = source.group.begin();
     const auto g_end = source.group.end();
-    build_transformed_root_group(builder, *g_iter, *root, legacy_mode);
+    build_transformed_root_group(builder, *g_iter, *root);
     ++g_iter;
     for (; g_iter != g_end; ++g_iter) {
-        build_transformed_non_root_group(builder, *g_iter, *root, legacy_mode);
+        build_transformed_non_root_group(builder, *g_iter, *root);
     }
 
     builder.redundancy = root->nested_leaf_count;
@@ -180,17 +153,17 @@ void build_global_groups(DistributionConfigBuilder& builder, const DistributionC
 } // anon ns
 
 std::shared_ptr<DistributionConfig>
-GlobalBucketSpaceDistributionConverter::convert_to_global(const DistributionConfig& source, bool legacy_mode) {
+GlobalBucketSpaceDistributionConverter::convert_to_global(const DistributionConfig& source) {
     DistributionConfigBuilder builder;
     set_distribution_invariant_config_fields(builder, source);
-    build_global_groups(builder, source, legacy_mode);
+    build_global_groups(builder, source);
     return std::make_shared<DistributionConfig>(builder);
 }
 
 std::shared_ptr<lib::Distribution>
-GlobalBucketSpaceDistributionConverter::convert_to_global(const lib::Distribution& distr, bool legacy_mode) {
+GlobalBucketSpaceDistributionConverter::convert_to_global(const lib::Distribution& distr) {
     const auto src_config = distr.serialize();
-    auto global_config = convert_to_global(*string_to_config(src_config), legacy_mode);
+    auto global_config = convert_to_global(*string_to_config(src_config));
     return std::make_shared<lib::Distribution>(*global_config);
 }
 
diff --git a/storage/src/vespa/storage/common/global_bucket_space_distribution_converter.h b/storage/src/vespa/storage/common/global_bucket_space_distribution_converter.h
index 9efca576469..ef508238907 100644
--- a/storage/src/vespa/storage/common/global_bucket_space_distribution_converter.h
+++ b/storage/src/vespa/storage/common/global_bucket_space_distribution_converter.h
@@ -10,9 +10,9 @@ namespace storage {
 
 struct GlobalBucketSpaceDistributionConverter {
     using DistributionConfig = vespa::config::content::StorDistributionConfig;
-    // TODO remove legacy_mode flags on Vespa 8 - this is a workaround for https://github.com/vespa-engine/vespa/issues/8475
-    static std::shared_ptr<DistributionConfig> convert_to_global(const DistributionConfig&, bool legacy_mode = false);
-    static std::shared_ptr<lib::Distribution>  convert_to_global(const lib::Distribution&, bool legacy_mode = false);
+
+    static std::shared_ptr<DistributionConfig> convert_to_global(const DistributionConfig&);
+    static std::shared_ptr<lib::Distribution>  convert_to_global(const lib::Distribution&);
 
     // Helper functions which may be of use outside this class
     static std::unique_ptr<DistributionConfig> string_to_config(const vespalib::string&);
diff --git a/storage/src/vespa/storage/distributor/pendingclusterstate.cpp b/storage/src/vespa/storage/distributor/pendingclusterstate.cpp
index 8183b013668..30071d4d962 100644
--- a/storage/src/vespa/storage/distributor/pendingclusterstate.cpp
+++ b/storage/src/vespa/storage/distributor/pendingclusterstate.cpp
@@ -192,29 +192,7 @@ void
 PendingClusterState::requestNode(BucketSpaceAndNode bucketSpaceAndNode)
 {
     const auto &distribution = _bucket_space_states.get(bucketSpaceAndNode.bucketSpace).get_distribution();
-    vespalib::string distributionHash;
-    // TODO remove on Vespa 8 - this is a workaround for https://github.com/vespa-engine/vespa/issues/8475
-    bool sendLegacyHash = false;
-    if (bucketSpaceAndNode.bucketSpace == document::FixedBucketSpaces::global_space()) {
-        auto transitionIter = _pendingTransitions.find(bucketSpaceAndNode.bucketSpace);
-        assert(transitionIter != _pendingTransitions.end());
-        // First request cannot have been rejected yet and will thus be sent with non-legacy hash.
-        // Subsequent requests will be sent 50-50. This is because a request may be rejected due to
-        // other reasons than just the hash mismatching, so if we don't cycle back to the non-legacy
-        // hash we risk never converging.
-        sendLegacyHash = ((transitionIter->second->rejectedRequests(bucketSpaceAndNode.node) % 2) == 1);
-    }
-    if (!sendLegacyHash) {
-        distributionHash = distribution.getNodeGraph().getDistributionConfigHash();
-    } else {
-        const auto& defaultSpace = _bucket_space_states.get(document::FixedBucketSpaces::default_space());
-        // Generate legacy distribution hash explicitly.
-        auto legacyGlobalDistr = GlobalBucketSpaceDistributionConverter::convert_to_global(
-                defaultSpace.get_distribution(), true/*use legacy mode*/);
-        distributionHash = legacyGlobalDistr->getNodeGraph().getDistributionConfigHash();
-        LOG(debug, "Falling back to sending legacy hash to node %u: %s",
-            bucketSpaceAndNode.node, distributionHash.c_str());
-    }
+    vespalib::string distributionHash = distribution.getNodeGraph().getDistributionConfigHash();
 
     LOG(debug,
         "Requesting bucket info for bucket space %" PRIu64 " node %d with cluster state '%s' "