Merge pull request #19594 from vespa-engine/vekterli/only-block-bucket-removes-if-pending-node-overlaps-with-send-set

Only block bucket remove operation if overlapping with pending message target node [run-systemtest]

13 files changed, 41 insertions, 22 deletions

diff --git a/storage/src/tests/distributor/idealstatemanagertest.cpp b/storage/src/tests/distributor/idealstatemanagertest.cpp
index d521975e0cb..a1263c9433b 100644
--- a/storage/src/tests/distributor/idealstatemanagertest.cpp
+++ b/storage/src/tests/distributor/idealstatemanagertest.cpp
@@ -220,8 +220,8 @@ TEST_F(IdealStateManagerTest, block_check_for_all_operations_to_specific_bucket)
         pending_message_tracker().insert(msg);
     }
     {
-        RemoveBucketOperation op(dummy_cluster_context,
-                                 BucketAndNodes(makeDocumentBucket(bid), toVector<uint16_t>(7)));
+        // TODO we might not want this particular behavior for merge operations either
+        MergeOperation op(BucketAndNodes(makeDocumentBucket(bid), toVector<uint16_t>(2, 3)));
         // Not blocked for exact node match.
         EXPECT_FALSE(checkBlock(op, makeDocumentBucket(bid), operation_context(), op_seq));
         // But blocked for bucket match!
diff --git a/storage/src/tests/distributor/removebucketoperationtest.cpp b/storage/src/tests/distributor/removebucketoperationtest.cpp
index e877f4601b7..971ff36c833 100644
--- a/storage/src/tests/distributor/removebucketoperationtest.cpp
+++ b/storage/src/tests/distributor/removebucketoperationtest.cpp
@@ -119,4 +119,16 @@ TEST_F(RemoveBucketOperationTest, fail_with_invalid_bucket_info) {
     EXPECT_EQ("NONEXISTING", dumpBucket(document::BucketId(16, 1)));
 }
 
+TEST_F(RemoveBucketOperationTest, operation_blocked_when_pending_message_to_target_node) {
+    RemoveBucketOperation op(dummy_cluster_context,
+                             BucketAndNodes(makeDocumentBucket(document::BucketId(16, 1)),
+                                            toVector<uint16_t>(1, 3)));
+    // In node target set
+    EXPECT_TRUE(op.shouldBlockThisOperation(api::MessageType::PUT_ID, 1, 120));
+    EXPECT_TRUE(op.shouldBlockThisOperation(api::MessageType::PUT_ID, 3, 120));
+    // Not in node target set
+    EXPECT_FALSE(op.shouldBlockThisOperation(api::MessageType::PUT_ID, 0, 120));
+    EXPECT_FALSE(op.shouldBlockThisOperation(api::MessageType::PUT_ID, 2, 120));
+}
+
 } // storage::distributor
diff --git a/storage/src/tests/distributor/statecheckerstest.cpp b/storage/src/tests/distributor/statecheckerstest.cpp
index 25009156f18..f5531a134d0 100644
--- a/storage/src/tests/distributor/statecheckerstest.cpp
+++ b/storage/src/tests/distributor/statecheckerstest.cpp
@@ -38,14 +38,15 @@ struct StateCheckersTest : Test, DistributorStripeTestUtil {
     struct PendingMessage
     {
         uint32_t _msgType;
+        uint16_t _node;
         uint8_t _pri;
 
-        PendingMessage() : _msgType(UINT32_MAX), _pri(0) {}
+        constexpr PendingMessage() noexcept : _msgType(UINT32_MAX), _node(0), _pri(0) {}
 
-        PendingMessage(uint32_t msgType, uint8_t pri)
-            : _msgType(msgType), _pri(pri) {}
+        constexpr PendingMessage(uint32_t msgType, uint8_t pri) noexcept
+            : _msgType(msgType), _node(0), _pri(pri) {}
 
-        bool shouldCheck() const { return _msgType != UINT32_MAX; }
+        bool shouldCheck() const noexcept { return _msgType != UINT32_MAX; }
     };
 
     void enableClusterState(const lib::ClusterState& systemState) {
@@ -97,8 +98,7 @@ struct StateCheckersTest : Test, DistributorStripeTestUtil {
                 IdealStateOperation::UP op(result.createOperation());
                 if (op.get()) {
                     if (blocker.shouldCheck()
-                        && op->shouldBlockThisOperation(blocker._msgType,
-                                                        blocker._pri))
+                        && op->shouldBlockThisOperation(blocker._msgType, blocker._node, blocker._pri))
                     {
                         return "BLOCKED";
                     }
diff --git a/storage/src/vespa/storage/distributor/operations/idealstate/garbagecollectionoperation.cpp b/storage/src/vespa/storage/distributor/operations/idealstate/garbagecollectionoperation.cpp
index fbe1c142b09..7f66d1effd5 100644
--- a/storage/src/vespa/storage/distributor/operations/idealstate/garbagecollectionoperation.cpp
+++ b/storage/src/vespa/storage/distributor/operations/idealstate/garbagecollectionoperation.cpp
@@ -86,7 +86,7 @@ void GarbageCollectionOperation::update_gc_metrics() {
 }
 
 bool
-GarbageCollectionOperation::shouldBlockThisOperation(uint32_t, uint8_t) const {
+GarbageCollectionOperation::shouldBlockThisOperation(uint32_t, uint16_t, uint8_t) const {
     return true;
 }
 
diff --git a/storage/src/vespa/storage/distributor/operations/idealstate/garbagecollectionoperation.h b/storage/src/vespa/storage/distributor/operations/idealstate/garbagecollectionoperation.h
index 2e010a61bde..f51739242b7 100644
--- a/storage/src/vespa/storage/distributor/operations/idealstate/garbagecollectionoperation.h
+++ b/storage/src/vespa/storage/distributor/operations/idealstate/garbagecollectionoperation.h
@@ -21,7 +21,7 @@ public:
     void onReceive(DistributorStripeMessageSender& sender, const std::shared_ptr<api::StorageReply> &) override;
     const char* getName() const override { return "garbagecollection"; };
     Type getType() const override { return GARBAGE_COLLECTION; }
-    bool shouldBlockThisOperation(uint32_t, uint8_t) const override;
+    bool shouldBlockThisOperation(uint32_t, uint16_t, uint8_t) const override;
 
 protected:
     MessageTracker _tracker;
diff --git a/storage/src/vespa/storage/distributor/operations/idealstate/idealstateoperation.cpp b/storage/src/vespa/storage/distributor/operations/idealstate/idealstateoperation.cpp
index b1231fafcd9..744b24b593e 100644
--- a/storage/src/vespa/storage/distributor/operations/idealstate/idealstateoperation.cpp
+++ b/storage/src/vespa/storage/distributor/operations/idealstate/idealstateoperation.cpp
@@ -138,8 +138,7 @@ public:
 
     bool check(uint32_t messageType, uint16_t node, uint8_t priority) override
     {
-        (void) node;
-        if (op.shouldBlockThisOperation(messageType, priority)) {
+        if (op.shouldBlockThisOperation(messageType, node, priority)) {
             blocked = true;
             return false;
         }
@@ -232,6 +231,7 @@ IdealStateOperation::toString() const
 
 bool
 IdealStateOperation::shouldBlockThisOperation(uint32_t messageType,
+                                              [[maybe_unused]] uint16_t node,
                                               uint8_t) const
 {
     for (uint32_t i = 0; MAINTENANCE_MESSAGE_TYPES[i] != 0; ++i) {
diff --git a/storage/src/vespa/storage/distributor/operations/idealstate/idealstateoperation.h b/storage/src/vespa/storage/distributor/operations/idealstate/idealstateoperation.h
index d4dc4e405df..f8f35afe821 100644
--- a/storage/src/vespa/storage/distributor/operations/idealstate/idealstateoperation.h
+++ b/storage/src/vespa/storage/distributor/operations/idealstate/idealstateoperation.h
@@ -217,7 +217,7 @@ public:
     /**
      * Should return true if the given message type should block this operation.
      */
-    virtual bool shouldBlockThisOperation(uint32_t messageType, uint8_t priority) const;
+    virtual bool shouldBlockThisOperation(uint32_t messageType, uint16_t node, uint8_t priority) const;
 
 protected:
     friend struct IdealStateManagerTest;
diff --git a/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.cpp b/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.cpp
index 7cfe4172b2c..f951a880e5d 100644
--- a/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.cpp
+++ b/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.cpp
@@ -329,14 +329,14 @@ constexpr std::array<uint32_t, 7> WRITE_FEED_MESSAGE_TYPES {{
 
 }
 
-bool MergeOperation::shouldBlockThisOperation(uint32_t messageType, uint8_t pri) const {
+bool MergeOperation::shouldBlockThisOperation(uint32_t messageType, uint16_t node, uint8_t pri) const {
     for (auto blocking_type : WRITE_FEED_MESSAGE_TYPES) {
         if (messageType == blocking_type) {
             return true;
         }
     }
 
-    return IdealStateOperation::shouldBlockThisOperation(messageType, pri);
+    return IdealStateOperation::shouldBlockThisOperation(messageType, node, pri);
 }
 
 bool MergeOperation::isBlocked(const DistributorStripeOperationContext& ctx,
diff --git a/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.h b/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.h
index 1bca1f7389f..832c0f99681 100644
--- a/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.h
+++ b/storage/src/vespa/storage/distributor/operations/idealstate/mergeoperation.h
@@ -48,7 +48,7 @@ public:
             const document::BucketId&, MergeLimiter&,
             std::vector<MergeMetaData>&);
 
-    bool shouldBlockThisOperation(uint32_t messageType, uint8_t pri) const override;
+    bool shouldBlockThisOperation(uint32_t messageType, uint16_t node, uint8_t pri) const override;
     bool isBlocked(const DistributorStripeOperationContext& ctx, const OperationSequencer&) const override;
 private:
     static void addIdealNodes(
diff --git a/storage/src/vespa/storage/distributor/operations/idealstate/removebucketoperation.cpp b/storage/src/vespa/storage/distributor/operations/idealstate/removebucketoperation.cpp
index 9a57722dc7e..25cae5b9979 100644
--- a/storage/src/vespa/storage/distributor/operations/idealstate/removebucketoperation.cpp
+++ b/storage/src/vespa/storage/distributor/operations/idealstate/removebucketoperation.cpp
@@ -61,8 +61,7 @@ RemoveBucketOperation::onStart(DistributorStripeMessageSender& sender)
 bool
 RemoveBucketOperation::onReceiveInternal(const std::shared_ptr<api::StorageReply> &msg)
 {
-    api::DeleteBucketReply* rep =
-        dynamic_cast<api::DeleteBucketReply*>(msg.get());
+    auto* rep = dynamic_cast<api::DeleteBucketReply*>(msg.get());
 
     uint16_t node = _tracker.handleReply(*rep);
 
@@ -112,8 +111,15 @@ RemoveBucketOperation::onReceive(DistributorStripeMessageSender&, const std::sha
 }
 
 bool
-RemoveBucketOperation::shouldBlockThisOperation(uint32_t, uint8_t) const
+RemoveBucketOperation::shouldBlockThisOperation(uint32_t, uint16_t target_node, uint8_t) const
 {
-    return true;
+    // Number of nodes is expected to be 1 in the vastly common case (and a highly bounded
+    // number in the worst case), so a simple linear scan suffices.
+    for (uint16_t node : getNodes()) {
+        if (target_node == node) {
+            return true;
+        }
+    }
+    return false;
 }
 
diff --git a/storage/src/vespa/storage/distributor/operations/idealstate/removebucketoperation.h b/storage/src/vespa/storage/distributor/operations/idealstate/removebucketoperation.h
index a0d496f948a..5e0922d5685 100644
--- a/storage/src/vespa/storage/distributor/operations/idealstate/removebucketoperation.h
+++ b/storage/src/vespa/storage/distributor/operations/idealstate/removebucketoperation.h
@@ -30,7 +30,7 @@ public:
     void onReceive(DistributorStripeMessageSender& sender, const std::shared_ptr<api::StorageReply> &) override;
     const char* getName() const override { return "remove"; };
     Type getType() const override { return DELETE_BUCKET; }
-    bool shouldBlockThisOperation(uint32_t, uint8_t) const  override;
+    bool shouldBlockThisOperation(uint32_t, uint16_t, uint8_t) const  override;
 protected:
     MessageTracker _tracker;
 };
diff --git a/storage/src/vespa/storage/distributor/operations/idealstate/splitoperation.cpp b/storage/src/vespa/storage/distributor/operations/idealstate/splitoperation.cpp
index 649503cf0f5..6f3924535ef 100644
--- a/storage/src/vespa/storage/distributor/operations/idealstate/splitoperation.cpp
+++ b/storage/src/vespa/storage/distributor/operations/idealstate/splitoperation.cpp
@@ -150,6 +150,7 @@ SplitOperation::isBlocked(const DistributorStripeOperationContext& ctx, const Op
 
 bool
 SplitOperation::shouldBlockThisOperation(uint32_t msgType,
+                                         [[maybe_unused]] uint16_t node,
                                          uint8_t pri) const
 {
     if (msgType == api::MessageType::SPLITBUCKET_ID && _priority >= pri) {
diff --git a/storage/src/vespa/storage/distributor/operations/idealstate/splitoperation.h b/storage/src/vespa/storage/distributor/operations/idealstate/splitoperation.h
index ee957309088..6a268155fc8 100644
--- a/storage/src/vespa/storage/distributor/operations/idealstate/splitoperation.h
+++ b/storage/src/vespa/storage/distributor/operations/idealstate/splitoperation.h
@@ -21,7 +21,7 @@ public:
     const char* getName() const override { return "split"; };
     Type getType() const override { return SPLIT_BUCKET; }
     bool isBlocked(const DistributorStripeOperationContext&, const OperationSequencer&) const override;
-    bool shouldBlockThisOperation(uint32_t, uint8_t) const override;
+    bool shouldBlockThisOperation(uint32_t, uint16_t, uint8_t) const override;
 protected:
     MessageTracker _tracker;