Merge pull request #30137 from vespa-engine/balder/always-report-host-info

Always report hostinfo

8 files changed, 7 insertions, 81 deletions

diff --git a/storage/src/tests/distributor/distributor_host_info_reporter_test.cpp b/storage/src/tests/distributor/distributor_host_info_reporter_test.cpp
index ecdb901a06a..e9c1cea38f3 100644
--- a/storage/src/tests/distributor/distributor_host_info_reporter_test.cpp
+++ b/storage/src/tests/distributor/distributor_host_info_reporter_test.cpp
@@ -196,20 +196,6 @@ TEST_F(DistributorHostInfoReporterTest, generate_example_json) {
     EXPECT_EQ(goldenSlime, jsonSlime);
 }
 
-TEST_F(DistributorHostInfoReporterTest, no_report_generated_if_disabled) {
-    Fixture f;
-    f.reporter.enableReporting(false);
-
-    MinReplicaStats minReplica;
-    minReplica[0] = 2;
-    minReplica[5] = 9;
-    f.minReplicaProvider.minReplica = minReplica;
-
-    vespalib::Slime root;
-    util::reporterToSlime(f.reporter, root);
-    EXPECT_EQ(0, root.get().children());
-}
-
 TEST_F(DistributorHostInfoReporterTest, bucket_spaces_stats_are_reported) {
     Fixture f;
     PerNodeBucketSpacesStats stats;
diff --git a/storage/src/tests/distributor/top_level_distributor_test.cpp b/storage/src/tests/distributor/top_level_distributor_test.cpp
index 9b859f59625..4e44abe71fb 100644
--- a/storage/src/tests/distributor/top_level_distributor_test.cpp
+++ b/storage/src/tests/distributor/top_level_distributor_test.cpp
@@ -57,7 +57,7 @@ struct TopLevelDistributorTest : Test, TopLevelDistributorTestUtil {
     using Redundancy = int;
     using ConfigBuilder = vespa::config::content::core::StorDistributormanagerConfigBuilder;
 
-    std::string resolve_stripe_operation_routing(std::shared_ptr<api::StorageMessage> msg) {
+    std::string resolve_stripe_operation_routing(const std::shared_ptr<api::StorageMessage> & msg) {
         handle_top_level_message(msg);
 
         vespalib::asciistream posted_msgs;
@@ -106,7 +106,7 @@ struct TopLevelDistributorTest : Test, TopLevelDistributorTestUtil {
         return _node->getNodeStateUpdater().explicit_node_state_reply_send_invocations();
     }
 
-    std::shared_ptr<api::RemoveCommand> make_dummy_remove_command() {
+    static std::shared_ptr<api::RemoveCommand> make_dummy_remove_command() {
         return std::make_shared<api::RemoveCommand>(
                 makeDocumentBucket(document::BucketId(0)),
                 document::DocumentId("id:foo:testdoctype1:n=1:foo"),
@@ -374,8 +374,8 @@ void TopLevelDistributorTest::reply_to_1_node_bucket_info_fetch_with_n_buckets(s
         if (bucket_req.getBucketSpace() == FixedBucketSpaces::default_space()) {
             auto& bucket_reply = dynamic_cast<api::RequestBucketInfoReply&>(*reply);
             for (size_t j = 1; j <= n; ++j) {
-                bucket_reply.getBucketInfo().push_back(api::RequestBucketInfoReply::Entry(
-                        document::BucketId(16, j), api::BucketInfo(20, 10, 12, 50, 60, true, true)));
+                bucket_reply.getBucketInfo().emplace_back(document::BucketId(16, j),
+                                                          api::BucketInfo(20, 10, 12, 50, 60, true, true));
             }
         }
         handle_top_level_message(std::move(reply));
@@ -474,19 +474,6 @@ TEST_F(TopLevelDistributorTest, non_bootstrap_host_info_send_request_delays_send
     EXPECT_EQ(2, explicit_node_state_reply_send_invocations());
 }
 
-TEST_F(TopLevelDistributorTest, host_info_reporter_config_is_propagated_to_reporter) {
-    setup_distributor(Redundancy(2), NodeCount(2), "storage:2 distributor:1");
-
-    // Default is enabled=true.
-    EXPECT_TRUE(distributor_host_info_reporter().isReportingEnabled());
-
-    auto cfg = current_distributor_config();
-    cfg.enableHostInfoReporting = false;
-    reconfigure(cfg);
-
-    EXPECT_FALSE(distributor_host_info_reporter().isReportingEnabled());
-}
-
 namespace {
 
 void assert_invalid_bucket_stats_for_all_spaces(
diff --git a/storage/src/vespa/storage/config/distributorconfiguration.cpp b/storage/src/vespa/storage/config/distributorconfiguration.cpp
index a855ea578ef..43d1ba404c9 100644
--- a/storage/src/vespa/storage/config/distributorconfiguration.cpp
+++ b/storage/src/vespa/storage/config/distributorconfiguration.cpp
@@ -37,7 +37,6 @@ DistributorConfiguration::DistributorConfiguration(StorageComponent& component)
       _doInlineSplit(true),
       _enableJoinForSiblingLessBuckets(false),
       _enableInconsistentJoin(false),
-      _enableHostInfoReporting(true),
       _disableBucketActivation(false),
       _sequenceMutatingOperations(true),
       _allowStaleReadsDuringClusterStateTransitions(false),
@@ -136,7 +135,6 @@ DistributorConfiguration::configure(const vespa::config::content::core::StorDist
     _enableJoinForSiblingLessBuckets = config.enableJoinForSiblingLessBuckets;
     _enableInconsistentJoin = config.enableInconsistentJoin;
 
-    _enableHostInfoReporting = config.enableHostInfoReporting;
     _disableBucketActivation = config.disableBucketActivation;
     _sequenceMutatingOperations = config.sequenceMutatingOperations;
     _allowStaleReadsDuringClusterStateTransitions = config.allowStaleReadsDuringClusterStateTransitions;
diff --git a/storage/src/vespa/storage/config/distributorconfiguration.h b/storage/src/vespa/storage/config/distributorconfiguration.h
index 6a5ea9e3bbd..fd2c47c8f6f 100644
--- a/storage/src/vespa/storage/config/distributorconfiguration.h
+++ b/storage/src/vespa/storage/config/distributorconfiguration.h
@@ -165,10 +165,6 @@ public:
         return _enableInconsistentJoin;
     }
 
-    bool getEnableHostInfoReporting() const noexcept {
-        return _enableHostInfoReporting;
-    }
-
     using ReplicaCountingMode = DistrConfig::MinimumReplicaCountingMode;
 
     ReplicaCountingMode getMinimumReplicaCountingMode() const noexcept {
@@ -306,7 +302,6 @@ private:
     bool _doInlineSplit;
     bool _enableJoinForSiblingLessBuckets;
     bool _enableInconsistentJoin;
-    bool _enableHostInfoReporting;
     bool _disableBucketActivation;
     bool _sequenceMutatingOperations;
     bool _allowStaleReadsDuringClusterStateTransitions;
diff --git a/storage/src/vespa/storage/config/stor-distributormanager.def b/storage/src/vespa/storage/config/stor-distributormanager.def
index 3136a54d080..d586cca56b1 100644
--- a/storage/src/vespa/storage/config/stor-distributormanager.def
+++ b/storage/src/vespa/storage/config/stor-distributormanager.def
@@ -63,16 +63,6 @@ enable_join_for_sibling_less_buckets bool default=false
 ## being joined into.
 enable_inconsistent_join bool default=false
 
-## The distributor host info reporter may be disabled entirely, in which case
-## no per-node statistics for merges, latencies or bucket replication factors
-## will be reported back to the cluster controller. Disabling this may make
-## sense in large clusters that do not make use of these reports directly or
-## indirectly, as it causes potentially significant processing overhead on the
-## cluster controller.
-## This host reporter must never be disabled on a Hosted Vespa system, or
-## automatic upgrades will stall.
-enable_host_info_reporting bool default=true
-
 ## For each available node, the distributor will report back to the cluster
 ## controller a value which indicates the minimum replication factor for any
 ## bucket contained on said node. This config exposes a way to alter how this
diff --git a/storage/src/vespa/storage/distributor/distributor_host_info_reporter.cpp b/storage/src/vespa/storage/distributor/distributor_host_info_reporter.cpp
index 918156c3048..330107e811c 100644
--- a/storage/src/vespa/storage/distributor/distributor_host_info_reporter.cpp
+++ b/storage/src/vespa/storage/distributor/distributor_host_info_reporter.cpp
@@ -17,8 +17,7 @@ DistributorHostInfoReporter::DistributorHostInfoReporter(
         MinReplicaProvider& minReplicaProvider,
         BucketSpacesStatsProvider& bucketSpacesStatsProvider)
     : _minReplicaProvider(minReplicaProvider),
-      _bucketSpacesStatsProvider(bucketSpacesStatsProvider),
-      _enabled(true)
+      _bucketSpacesStatsProvider(bucketSpacesStatsProvider)
 {
 }
 
@@ -80,19 +79,13 @@ outputStorageNodes(vespalib::JsonStream& output,
 void
 DistributorHostInfoReporter::report(vespalib::JsonStream& output)
 {
-    if (!isReportingEnabled()) {
-        return;
-    }
-
     auto minReplica = _minReplicaProvider.getMinReplica();
     auto bucketSpacesStats = _bucketSpacesStatsProvider.getBucketSpacesStats();
 
     output << "distributor" << Object();
     {
         output << "storage-nodes" << Array();
-
         outputStorageNodes(output, minReplica, bucketSpacesStats);
-
         output << End();
     }
     output << End();
diff --git a/storage/src/vespa/storage/distributor/distributor_host_info_reporter.h b/storage/src/vespa/storage/distributor/distributor_host_info_reporter.h
index ab1fbe5feea..52921deb4b9 100644
--- a/storage/src/vespa/storage/distributor/distributor_host_info_reporter.h
+++ b/storage/src/vespa/storage/distributor/distributor_host_info_reporter.h
@@ -2,13 +2,11 @@
 #pragma once
 
 #include <vespa/storage/common/hostreporter/hostreporter.h>
-#include <atomic>
 
 namespace storage::distributor {
 
 class BucketSpacesStatsProvider;
 class MinReplicaProvider;
-struct OperationStats;
 
 class DistributorHostInfoReporter : public HostReporter
 {
@@ -17,32 +15,13 @@ public:
                                 BucketSpacesStatsProvider& bucketSpacesStatsProvider);
 
     DistributorHostInfoReporter(const DistributorHostInfoReporter&) = delete;
-    DistributorHostInfoReporter& operator=(
-            const DistributorHostInfoReporter&) = delete;
+    DistributorHostInfoReporter& operator=(const DistributorHostInfoReporter&) = delete;
 
     void report(vespalib::JsonStream& output) override;
 
-    /**
-     * Set wether per-node latency, replication factors, merge stats etc are
-     * to be included in the generated JSON report.
-     *
-     * Thread safe.
-     */
-    void enableReporting(bool enabled) noexcept {
-        _enabled.store(enabled, std::memory_order_relaxed);
-    }
-
-    /**
-     * Thread safe.
-     */
-    bool isReportingEnabled() const noexcept {
-        return _enabled.load(std::memory_order_relaxed);
-    }
-
 private:
     MinReplicaProvider& _minReplicaProvider;
     BucketSpacesStatsProvider& _bucketSpacesStatsProvider;
-    std::atomic<bool> _enabled;
 };
 
 } // storage::distributor
diff --git a/storage/src/vespa/storage/distributor/top_level_distributor.cpp b/storage/src/vespa/storage/distributor/top_level_distributor.cpp
index 710f554df4b..aa02ec9b7d3 100644
--- a/storage/src/vespa/storage/distributor/top_level_distributor.cpp
+++ b/storage/src/vespa/storage/distributor/top_level_distributor.cpp
@@ -109,10 +109,9 @@ TopLevelDistributor::TopLevelDistributor(DistributorComponentRegister& compReg,
     _bucket_db_status_delegate = std::make_unique<StatusReporterDelegate>(compReg, *this, *_bucket_db_updater);
     _bucket_db_status_delegate->registerStatusPage();
 
-    _hostInfoReporter.enableReporting(config().getEnableHostInfoReporting());
     hostInfoReporterRegistrar.registerReporter(&_hostInfoReporter);
     propagate_default_distribution_thread_unsafe(_component.getDistribution()); // Stripes not started yet
-};
+}
 
 TopLevelDistributor::~TopLevelDistributor()
 {
@@ -437,7 +436,6 @@ TopLevelDistributor::enable_next_config_if_changed()
             auto guard = _stripe_accessor->rendezvous_and_hold_all();
             guard->update_total_distributor_config(_component.total_distributor_config_sp());
         }
-        _hostInfoReporter.enableReporting(config().getEnableHostInfoReporting());
         _maintenance_safe_time_delay = _total_config->getMaxClusterClockSkew();
         _current_internal_config_generation = _component.internal_config_generation();
     }