Merge pull request #30130 from vespa-engine/balder/gc-void-config-from-stor-bouncer

GC void config from stor-bouncer.def

4 files changed, 15 insertions, 129 deletions

diff --git a/storage/src/tests/storageserver/bouncertest.cpp b/storage/src/tests/storageserver/bouncertest.cpp
index 225b3c94120..296ed6d23bc 100644
--- a/storage/src/tests/storageserver/bouncertest.cpp
+++ b/storage/src/tests/storageserver/bouncertest.cpp
@@ -4,6 +4,7 @@
 #include <tests/common/testhelper.h>
 #include <tests/common/teststorageapp.h>
 #include <vespa/config/common/exceptions.h>
+#include <memory>
 #include <vespa/config/helper/configgetter.hpp>
 #include <vespa/document/bucket/fixed_bucket_spaces.h>
 #include <vespa/document/fieldset/fieldsets.h>
@@ -41,18 +42,6 @@ struct BouncerTest : public Test {
 
     static constexpr int RejectionDisabledConfigValue = -1;
 
-    // Note: newThreshold is intentionally int (rather than Priority) in order
-    // to be able to test out of bounds values.
-    void configureRejectionThreshold(int newThreshold);
-
-    std::shared_ptr<api::StorageCommand> createDummyFeedMessage(
-            api::Timestamp timestamp,
-            Priority priority = 0);
-
-    std::shared_ptr<api::StorageCommand> createDummyFeedMessage(
-            api::Timestamp timestamp,
-            document::BucketSpace bucketSpace);
-
     void expectMessageBouncedWithRejection() const;
     void expect_message_bounced_with_node_down_abort() const;
     void expect_message_bounced_with_shutdown_abort() const;
@@ -70,11 +59,11 @@ BouncerTest::BouncerTest()
 void BouncerTest::setUpAsNode(const lib::NodeType& type) {
     vdstestlib::DirConfig config(getStandardConfig(type == lib::NodeType::STORAGE));
     if (type == lib::NodeType::STORAGE) {
-        _node.reset(new TestServiceLayerApp(NodeIndex(2), config.getConfigId()));
+        _node = std::make_unique<TestServiceLayerApp>(NodeIndex(2), config.getConfigId());
     } else {
-        _node.reset(new TestDistributorApp(NodeIndex(2), config.getConfigId()));
+        _node = std::make_unique<TestDistributorApp>(NodeIndex(2), config.getConfigId());
     }
-    _upper.reset(new DummyStorageLink());
+    _upper = std::make_unique<DummyStorageLink>();
     using StorBouncerConfig = vespa::config::content::core::StorBouncerConfig;
     auto cfg_uri = config::ConfigUri(config.getConfigId());
     auto cfg = config::ConfigGetter<StorBouncerConfig>::getConfig(cfg_uri.getConfigId(), cfg_uri.getContext());
@@ -104,8 +93,8 @@ BouncerTest::TearDown() {
 }
 
 std::shared_ptr<api::StorageCommand>
-BouncerTest::createDummyFeedMessage(api::Timestamp timestamp,
-                                    api::StorageMessage::Priority priority)
+createDummyFeedMessage(api::Timestamp timestamp,
+                       api::StorageMessage::Priority priority = 0)
 {
     auto cmd = std::make_shared<api::RemoveCommand>(
             makeDocumentBucket(document::BucketId(0)),
@@ -116,14 +105,14 @@ BouncerTest::createDummyFeedMessage(api::Timestamp timestamp,
 }
 
 std::shared_ptr<api::StorageCommand>
-BouncerTest::createDummyFeedMessage(api::Timestamp timestamp,
-                                    document::BucketSpace bucketSpace)
+createDummyFeedMessage(api::Timestamp timestamp,
+                       document::BucketSpace bucketSpace)
 {
     auto cmd = std::make_shared<api::RemoveCommand>(
             document::Bucket(bucketSpace, document::BucketId(0)),
             document::DocumentId("id:ns:foo::bar"),
             timestamp);
-    cmd->setPriority(Priority(0));
+    cmd->setPriority(BouncerTest::Priority(0));
     return cmd;
 }
 
@@ -226,58 +215,21 @@ BouncerTest::expectMessageNotBounced() const
     EXPECT_EQ(size_t(1), _lower->getNumCommands());
 }
 
-void
-BouncerTest::configureRejectionThreshold(int newThreshold)
-{
-    using Builder = vespa::config::content::core::StorBouncerConfigBuilder;
-    Builder config;
-    config.feedRejectionPriorityThreshold = newThreshold;
-    _manager->on_configure(config);
-}
-
-TEST_F(BouncerTest, reject_lower_prioritized_feed_messages_when_configured) {
-    configureRejectionThreshold(Priority(120));
-    _upper->sendDown(createDummyFeedMessage(11 * 1000000, Priority(121)));
-    expectMessageBouncedWithRejection();
-}
-
-TEST_F(BouncerTest, do_not_reject_higher_prioritized_feed_messages_than_configured) {
-    configureRejectionThreshold(Priority(120));
-    _upper->sendDown(createDummyFeedMessage(11 * 1000000, Priority(119)));
-    expectMessageNotBounced();
-}
-
-TEST_F(BouncerTest, priority_rejection_threshold_is_exclusive) {
-    configureRejectionThreshold(Priority(120));
-    _upper->sendDown(createDummyFeedMessage(11 * 1000000, Priority(120)));
-    expectMessageNotBounced();
-}
-
-TEST_F(BouncerTest, only_priority_reject_feed_messages_when_configured) {
-    configureRejectionThreshold(RejectionDisabledConfigValue);
-    // A message with even the lowest priority should not be rejected.
-    _upper->sendDown(createDummyFeedMessage(11 * 1000000, Priority(255)));
-    expectMessageNotBounced();
-}
-
 TEST_F(BouncerTest, priority_rejection_is_disabled_by_default_in_config) {
     _upper->sendDown(createDummyFeedMessage(11 * 1000000, Priority(255)));
     expectMessageNotBounced();
 }
 
-TEST_F(BouncerTest, read_only_operations_are_not_priority_rejected) {
-    configureRejectionThreshold(Priority(1));
+TEST_F(BouncerTest, read_only_operations_are_not_rejected) {
     // StatBucket is an external operation, but it's not a mutating operation
     // and should therefore not be blocked.
-    auto cmd = std::make_shared<api::StatBucketCommand>(
-            makeDocumentBucket(document::BucketId(16, 5)), "");
+    auto cmd = std::make_shared<api::StatBucketCommand>(makeDocumentBucket(document::BucketId(16, 5)), "");
     cmd->setPriority(Priority(2));
     _upper->sendDown(cmd);
     expectMessageNotBounced();
 }
 
 TEST_F(BouncerTest, internal_operations_are_not_rejected) {
-    configureRejectionThreshold(Priority(1));
     document::BucketId bucket(16, 1234);
     api::BucketInfo info(0x1, 0x2, 0x3);
     auto cmd = std::make_shared<api::NotifyBucketChangeCommand>(makeDocumentBucket(bucket), info);
@@ -286,12 +238,6 @@ TEST_F(BouncerTest, internal_operations_are_not_rejected) {
     expectMessageNotBounced();
 }
 
-TEST_F(BouncerTest, out_of_bounds_config_values_throw_exception) {
-    EXPECT_THROW(configureRejectionThreshold(256), config::InvalidConfigException);
-    EXPECT_THROW(configureRejectionThreshold(-2), config::InvalidConfigException);
-}
-
-
 namespace {
 
 std::shared_ptr<const lib::ClusterStateBundle>
diff --git a/storage/src/vespa/storage/config/stor-bouncer.def b/storage/src/vespa/storage/config/stor-bouncer.def
index db4471825c9..aa65c31c186 100644
--- a/storage/src/vespa/storage/config/stor-bouncer.def
+++ b/storage/src/vespa/storage/config/stor-bouncer.def
@@ -1,26 +1,6 @@
 # Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 namespace=vespa.config.content.core
 
-## Whether or not the bouncer should stop external load from
-## entering node when the cluster state is down.
-stop_external_load_when_cluster_down bool default=true
-
-## Sets what node states the node will allow incoming commands
-## in.
-stop_all_load_when_nodestate_not_in string default="uri"
-
 ## The maximum clock skew allowed in the system. Any messages received 
 ## that have a timestamp longer in the future than this will be failed.
 max_clock_skew_seconds int default=5
-
-## If this config value is != -1, the node will reject any external feed
-## operations with a priority lower than that specified here. Note that since
-## we map priorities in such a way that 0 is the _highest_ priority and 255 the
-## _lowest_ priority, for two operations A and B, if B has a lower priority
-## than A it will have a higher priority _integer_ value.
-##
-## Only mutating external feed operations will be blocked. Read-only operations
-## and internal operations are always let through.
-##
-## Default is -1 (i.e. rejection is disabled and load is allowed through)
-feed_rejection_priority_threshold int default=-1
diff --git a/storage/src/vespa/storage/storageserver/bouncer.cpp b/storage/src/vespa/storage/storageserver/bouncer.cpp
index bfc38e0c8ba..0aedee6799d 100644
--- a/storage/src/vespa/storage/storageserver/bouncer.cpp
+++ b/storage/src/vespa/storage/storageserver/bouncer.cpp
@@ -2,16 +2,13 @@
 
 #include "bouncer.h"
 #include "bouncer_metrics.h"
-#include "config_logging.h"
 #include <vespa/storageframework/generic/clock/clock.h>
 #include <vespa/vdslib/state/cluster_state_bundle.h>
 #include <vespa/vdslib/state/clusterstate.h>
 #include <vespa/persistence/spi/bucket_limits.h>
 #include <vespa/storageapi/message/state.h>
 #include <vespa/storageapi/message/persistence.h>
-#include <vespa/config/subscription/configuri.h>
 #include <vespa/config/helper/configfetcher.hpp>
-#include <vespa/config/common/exceptions.h>
 #include <vespa/vespalib/util/stringfmt.h>
 #include <sstream>
 
@@ -62,7 +59,6 @@ Bouncer::onClose()
 void
 Bouncer::on_configure(const vespa::config::content::core::StorBouncerConfig& config)
 {
-    validateConfig(config);
     auto new_config = std::make_unique<StorBouncerConfig>(config);
     std::lock_guard lock(_lock);
     _config = std::move(new_config);
@@ -72,27 +68,6 @@ const BouncerMetrics& Bouncer::metrics() const noexcept {
     return *_metrics;
 }
 
-void
-Bouncer::validateConfig(const vespa::config::content::core::StorBouncerConfig& newConfig) const
-{
-    if (newConfig.feedRejectionPriorityThreshold != -1) {
-        if (newConfig.feedRejectionPriorityThreshold
-            > std::numeric_limits<api::StorageMessage::Priority>::max())
-        {
-            throw config::InvalidConfigException(
-                    "feed_rejection_priority_threshold config value exceeds "
-                    "maximum allowed value");
-        }
-        if (newConfig.feedRejectionPriorityThreshold
-            < std::numeric_limits<api::StorageMessage::Priority>::min())
-        {
-            throw config::InvalidConfigException(
-                    "feed_rejection_priority_threshold config value lower than "
-                    "minimum allowed value");
-        }
-    }
-}
-
 void Bouncer::append_node_identity(std::ostream& target_stream) const {
     target_stream << " (on " << _component.getNodeType() << '.' << _component.getIndex() << ")";
 }
@@ -101,8 +76,7 @@ void
 Bouncer::abortCommandForUnavailableNode(api::StorageMessage& msg, const lib::State& state)
 {
     // If we're not up or retired, fail due to this nodes state.
-    std::shared_ptr<api::StorageReply> reply(
-            static_cast<api::StorageCommand&>(msg).makeReply());
+    std::shared_ptr<api::StorageReply> reply(static_cast<api::StorageCommand&>(msg).makeReply());
     std::ostringstream ost;
     ost << "We don't allow command of type " << msg.getType()
         << " when node is in state " << state.toString(true);
@@ -235,18 +209,14 @@ Bouncer::onDown(const std::shared_ptr<api::StorageMessage>& msg)
     const lib::State* state;
     int maxClockSkewInSeconds;
     bool isInAvailableState;
-    bool abortLoadWhenClusterDown;
     bool closed;
     const lib::State* cluster_state;
-    int feedPriorityLowerBound;
     {
         std::lock_guard lock(_lock);
         state                    = &getDerivedNodeState(msg->getBucket().getBucketSpace()).getState();
         maxClockSkewInSeconds    = _config->maxClockSkewSeconds;
-        abortLoadWhenClusterDown = _config->stopExternalLoadWhenClusterDown;
         cluster_state            = _clusterState;
-        isInAvailableState       = state->oneOf(_config->stopAllLoadWhenNodestateNotIn.c_str());
-        feedPriorityLowerBound   = _config->feedRejectionPriorityThreshold;
+        isInAvailableState       = state->oneOf("uri");
         closed                   = _closed;
     }
     const api::MessageType& type = msg->getType();
@@ -292,13 +262,6 @@ Bouncer::onDown(const std::shared_ptr<api::StorageMessage>& msg)
     if (!externalLoad) {
         return false;
     }
-    if (priorityRejectionIsEnabled(feedPriorityLowerBound)
-        && isExternalWriteOperation(type)
-        && (msg->getPriority() > feedPriorityLowerBound))
-    {
-        rejectDueToInsufficientPriority(*msg, feedPriorityLowerBound);
-        return true;
-    }
 
     uint64_t timestamp = extractMutationTimestampIfAny(*msg);
     if (timestamp != 0) {
@@ -311,7 +274,7 @@ Bouncer::onDown(const std::shared_ptr<api::StorageMessage>& msg)
     }
 
     // If cluster state is not up, fail external load
-    if (abortLoadWhenClusterDown && !clusterIsUp(*cluster_state)) {
+    if (!clusterIsUp(*cluster_state)) {
         abortCommandDueToClusterDown(*msg, *cluster_state);
         return true;
     }
diff --git a/storage/src/vespa/storage/storageserver/bouncer.h b/storage/src/vespa/storage/storageserver/bouncer.h
index 26282625269..51620a49bda 100644
--- a/storage/src/vespa/storage/storageserver/bouncer.h
+++ b/storage/src/vespa/storage/storageserver/bouncer.h
@@ -49,7 +49,6 @@ public:
     const BouncerMetrics& metrics() const noexcept;
 
 private:
-    void validateConfig(const vespa::config::content::core::StorBouncerConfig&) const;
     void onClose() override;
     void abortCommandForUnavailableNode(api::StorageMessage&, const lib::State&);
     void rejectCommandWithTooHighClockSkew(api::StorageMessage& msg, int maxClockSkewInSeconds);
@@ -61,9 +60,7 @@ private:
     bool isDistributor() const;
     static bool isExternalLoad(const api::MessageType&) noexcept;
     static bool isExternalWriteOperation(const api::MessageType&) noexcept;
-    static bool priorityRejectionIsEnabled(int configuredPriority) noexcept {
-        return (configuredPriority != -1);
-    }
+
 
     /**
      * If msg is a command containing a mutating timestamp (put, remove or