Merge pull request #2298 from yahoo/vekterli/inhibit-bucket-background-move-when-node-is-retired

Inhibit bucket background move when node is retired

8 files changed, 130 insertions, 63 deletions

diff --git a/persistence/src/tests/spi/clusterstatetest.cpp b/persistence/src/tests/spi/clusterstatetest.cpp
index 7770b9045e0..ba9b0e28d50 100644
--- a/persistence/src/tests/spi/clusterstatetest.cpp
+++ b/persistence/src/tests/spi/clusterstatetest.cpp
@@ -18,12 +18,14 @@ struct ClusterStateTest : public CppUnit::TestFixture {
     CPPUNIT_TEST(testNodeUp);
     CPPUNIT_TEST(testNodeInitializing);
     CPPUNIT_TEST(testReady);
+    CPPUNIT_TEST(can_infer_own_node_retired_state);
     CPPUNIT_TEST_SUITE_END();
 
     void testClusterUp();
     void testNodeUp();
     void testNodeInitializing();
     void testReady();
+    void can_infer_own_node_retired_state();
 };
 
 CPPUNIT_TEST_SUITE_REGISTRATION(ClusterStateTest);
@@ -222,5 +224,31 @@ ClusterStateTest::testReady()
     }
 }
 
+namespace {
+
+bool
+node_marked_as_retired_in_state(const std::string& stateStr,
+                                const lib::Distribution& d,
+                                uint16_t node)
+{
+    lib::ClusterState s(stateStr);
+    ClusterState state(s, node, d);
+    return state.nodeRetired();
+}
+
+}
+
+void ClusterStateTest::can_infer_own_node_retired_state() {
+    lib::Distribution d(lib::Distribution::getDefaultDistributionConfig(3, 3));
+
+    CPPUNIT_ASSERT(!node_marked_as_retired_in_state("distributor:3 storage:3", d, 0));
+    CPPUNIT_ASSERT(!node_marked_as_retired_in_state("distributor:3 storage:3 .0.s:i", d, 0));
+    CPPUNIT_ASSERT(!node_marked_as_retired_in_state("distributor:3 storage:3 .0.s:d", d, 0));
+    CPPUNIT_ASSERT(!node_marked_as_retired_in_state("distributor:3 storage:3 .0.s:m", d, 0));
+    CPPUNIT_ASSERT(node_marked_as_retired_in_state("distributor:3 storage:3 .0.s:r", d, 0));
+    CPPUNIT_ASSERT(!node_marked_as_retired_in_state("distributor:3 storage:3 .0.s:r", d, 1));
+    CPPUNIT_ASSERT(!node_marked_as_retired_in_state("distributor:3 storage:3 .1.s:r", d, 0));
+}
+
 } // spi
 } // storage
diff --git a/persistence/src/vespa/persistence/spi/clusterstate.cpp b/persistence/src/vespa/persistence/spi/clusterstate.cpp
index c505c8c8d3b..70bc8724c96 100644
--- a/persistence/src/vespa/persistence/spi/clusterstate.cpp
+++ b/persistence/src/vespa/persistence/spi/clusterstate.cpp
@@ -16,12 +16,9 @@ ClusterState::ClusterState(const lib::ClusterState& state,
       _nodeIndex(nodeIndex),
       _distribution(new lib::Distribution(distribution.serialize()))
 {
-
 }
 
-void
-ClusterState::deserialize(vespalib::nbostream& i)
-{
+void ClusterState::deserialize(vespalib::nbostream& i) {
     vespalib::string clusterState;
     vespalib::string distribution;
 
@@ -33,13 +30,11 @@ ClusterState::deserialize(vespalib::nbostream& i)
     _distribution.reset(new lib::Distribution(distribution));
 }
 
-ClusterState::ClusterState(vespalib::nbostream& i)
-{
+ClusterState::ClusterState(vespalib::nbostream& i) {
     deserialize(i);
 }
 
-ClusterState::ClusterState(const ClusterState& other)
-{
+ClusterState::ClusterState(const ClusterState& other) {
     vespalib::nbostream o;
     other.serialize(o);
     deserialize(o);
@@ -47,9 +42,7 @@ ClusterState::ClusterState(const ClusterState& other)
 
 ClusterState::~ClusterState() { }
 
-ClusterState&
-ClusterState::operator=(const ClusterState& other)
-{
+ClusterState& ClusterState::operator=(const ClusterState& other) {
     ClusterState copy(other);
     _state = std::move(copy._state);
     _nodeIndex = copy._nodeIndex;
@@ -57,9 +50,7 @@ ClusterState::operator=(const ClusterState& other)
     return *this;
 }
 
-bool
-ClusterState::shouldBeReady(const Bucket& b) const
-{
+bool ClusterState::shouldBeReady(const Bucket& b) const {
     assert(_distribution.get());
     assert(_state.get());
 
@@ -77,31 +68,29 @@ ClusterState::shouldBeReady(const Bucket& b) const
     return false;
 }
 
-bool
-ClusterState::clusterUp() const
-{
+bool ClusterState::clusterUp() const {
     return _state.get() && _state->getClusterState() == lib::State::UP;
 }
 
-bool
-ClusterState::nodeUp() const
-{
+bool ClusterState::nodeHasStateOneOf(const char* states) const {
     return _state.get() &&
-        _state->getNodeState(lib::Node(lib::NodeType::STORAGE, _nodeIndex)).
-        getState().oneOf("uir");
+           _state->getNodeState(lib::Node(lib::NodeType::STORAGE, _nodeIndex)).
+                   getState().oneOf(states);
 }
 
-bool
-ClusterState::nodeInitializing() const
-{
-    return _state.get() &&
-        _state->getNodeState(lib::Node(lib::NodeType::STORAGE, _nodeIndex)).
-        getState().oneOf("i");
+bool ClusterState::nodeUp() const {
+    return nodeHasStateOneOf("uir");
 }
 
-void
-ClusterState::serialize(vespalib::nbostream& o) const
-{
+bool ClusterState::nodeInitializing() const {
+    return nodeHasStateOneOf("i");
+}
+
+bool ClusterState::nodeRetired() const {
+    return nodeHasStateOneOf("r");
+}
+
+void ClusterState::serialize(vespalib::nbostream& o) const {
     assert(_distribution.get());
     assert(_state.get());
     vespalib::asciistream tmp;
diff --git a/persistence/src/vespa/persistence/spi/clusterstate.h b/persistence/src/vespa/persistence/spi/clusterstate.h
index 52a71c561dd..f0fe3cfafa8 100644
--- a/persistence/src/vespa/persistence/spi/clusterstate.h
+++ b/persistence/src/vespa/persistence/spi/clusterstate.h
@@ -53,11 +53,16 @@ public:
     bool nodeUp() const;
 
     /**
-     * Returns true if this node is marked as Initializing in the cluster state.
+     * Returns true iff this node is marked as Initializing in the cluster state.
      */
     bool nodeInitializing() const;
 
     /**
+     * Returns true iff this node is marked as Retired in the cluster state.
+     */
+    bool nodeRetired() const;
+
+    /**
      * Returns a serialized form of this object.
      */
     void serialize(vespalib::nbostream& o) const;
@@ -68,6 +73,7 @@ private:
     std::unique_ptr<lib::Distribution> _distribution;
 
     void deserialize(vespalib::nbostream&);
+    bool nodeHasStateOneOf(const char* states) const;
 };
 
 }
diff --git a/searchcore/src/tests/proton/documentdb/documentbucketmover/documentbucketmover_test.cpp b/searchcore/src/tests/proton/documentdb/documentbucketmover/documentbucketmover_test.cpp
index c70c2c28001..59032f8da9b 100644
--- a/searchcore/src/tests/proton/documentdb/documentbucketmover/documentbucketmover_test.cpp
+++ b/searchcore/src/tests/proton/documentdb/documentbucketmover/documentbucketmover_test.cpp
@@ -1179,6 +1179,42 @@ TEST_F("require that thawed bucket is not moved if active as well", ControllerFi
     EXPECT_EQUAL(f._ready.bucket(1), f.bucketsModified()[0]);
 }
 
+TEST_F("ready bucket not moved to not ready if node is marked as retired", ControllerFixture) {
+    f._calc->setNodeRetired(true);
+    // Bucket 2 would be moved from ready to not ready in a non-retired case, but not when retired.
+    f.addReady(f._ready.bucket(1));
+    f._bmj.scanAndMove(4, 3);
+    EXPECT_TRUE(f._bmj.done());
+    EXPECT_EQUAL(0u, f.docsMoved().size());
+}
+
+// Technically this should never happen since a retired node is never in the ideal state,
+// but test this case for the sake of completion.
+TEST_F("inactive not ready bucket not moved to ready if node is marked as retired", ControllerFixture) {
+    f._calc->setNodeRetired(true);
+    f.addReady(f._ready.bucket(1));
+    f.addReady(f._ready.bucket(2));
+    f.addReady(f._notReady.bucket(3));
+    f._bmj.scanAndMove(4, 3);
+    EXPECT_TRUE(f._bmj.done());
+    EXPECT_EQUAL(0u, f.docsMoved().size());
+}
+
+TEST_F("explicitly active not ready bucket can be moved to ready even if node is marked as retired", ControllerFixture) {
+    f._calc->setNodeRetired(true);
+    f.addReady(f._ready.bucket(1));
+    f.addReady(f._ready.bucket(2));
+    f.addReady(f._notReady.bucket(3));
+    f.activateBucket(f._notReady.bucket(3));
+    f._bmj.scanAndMove(4, 3);
+    EXPECT_FALSE(f._bmj.done());
+    ASSERT_EQUAL(2u, f.docsMoved().size());
+    assertEqual(f._notReady.bucket(3), f._notReady.docs(3)[0], 2, 1, f.docsMoved()[0]);
+    assertEqual(f._notReady.bucket(3), f._notReady.docs(3)[1], 2, 1, f.docsMoved()[1]);
+    ASSERT_EQUAL(1u, f.bucketsModified().size());
+    EXPECT_EQUAL(f._notReady.bucket(3), f.bucketsModified()[0]);
+}
+
 struct ResourceLimitControllerFixture : public ControllerFixture
 {
     using ControllerFixture::ControllerFixture;
diff --git a/searchcore/src/vespa/searchcore/proton/server/bucketmovejob.cpp b/searchcore/src/vespa/searchcore/proton/server/bucketmovejob.cpp
index 94a80a984c8..d5da1cd1868 100644
--- a/searchcore/src/vespa/searchcore/proton/server/bucketmovejob.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/bucketmovejob.cpp
@@ -58,14 +58,19 @@ BucketMoveJob::checkBucket(const BucketId &bucket,
                            DocumentBucketMover &mover,
                            IFrozenBucketHandler::ExclusiveBucketGuard::UP & bucketGuard)
 {
-    bool hasReadyDocs = itr.hasReadyBucketDocs();
-    bool hasNotReadyDocs = itr.hasNotReadyBucketDocs();
+    const bool hasReadyDocs = itr.hasReadyBucketDocs();
+    const bool hasNotReadyDocs = itr.hasNotReadyBucketDocs();
     if (!hasReadyDocs && !hasNotReadyDocs) {
         return; // No documents for bucket in ready or notready subdbs
     }
-    bool shouldBeReady = _calc->shouldBeReady(bucket);
-    bool isActive = itr.isActive();
-    bool wantReady = shouldBeReady || isActive;
+    const bool isActive = itr.isActive();
+    // No point in moving buckets when node is retired and everything will be deleted soon.
+    // However, allow moving of explicitly activated buckets, as this implies a lack of other good replicas.
+    if (_calc->nodeRetired() && !isActive) {
+        return;
+    }
+    const bool shouldBeReady = _calc->shouldBeReady(bucket);
+    const bool wantReady = shouldBeReady || isActive;
     LOG(spam, "checkBucket(): bucket(%s), shouldBeReady(%s), active(%s)",
               bucket.toString().c_str(), bool2str(shouldBeReady), bool2str(isActive));
     if (wantReady) {
diff --git a/searchcore/src/vespa/searchcore/proton/server/clusterstatehandler.cpp b/searchcore/src/vespa/searchcore/proton/server/clusterstatehandler.cpp
index 8eb229a5e6b..bca74b10058 100644
--- a/searchcore/src/vespa/searchcore/proton/server/clusterstatehandler.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/clusterstatehandler.cpp
@@ -32,29 +32,25 @@ public:
     {
     }
 
-    virtual bool
-    shouldBeReady(const document::BucketId &bucket) const override
-    {
+    bool shouldBeReady(const document::BucketId &bucket) const override {
         return _calc.shouldBeReady(Bucket(bucket, PartitionId(0)));
     }
 
-    virtual bool
-    clusterUp(void) const override
-    {
+    bool clusterUp() const override {
         return _calc.clusterUp();
     }
 
-    virtual bool
-    nodeUp(void) const override
-    {
+    bool nodeUp() const override {
         return _calc.nodeUp();
     }
 
-    virtual bool
-    nodeInitializing() const override
-    {
+    bool nodeInitializing() const override {
         return _calc.nodeInitializing();
     }
+
+    bool nodeRetired() const override {
+        return _calc.nodeRetired();
+    }
 };
 
 }
diff --git a/searchcore/src/vespa/searchcore/proton/server/ibucketstatecalculator.h b/searchcore/src/vespa/searchcore/proton/server/ibucketstatecalculator.h
index 3c35ff65c89..bfe7c127a8e 100644
--- a/searchcore/src/vespa/searchcore/proton/server/ibucketstatecalculator.h
+++ b/searchcore/src/vespa/searchcore/proton/server/ibucketstatecalculator.h
@@ -15,6 +15,7 @@ struct IBucketStateCalculator
     virtual bool clusterUp() const = 0;
     virtual bool nodeUp() const = 0;
     virtual bool nodeInitializing() const = 0;
+    virtual bool nodeRetired() const = 0;
     virtual ~IBucketStateCalculator() {}
 };
 
diff --git a/searchcore/src/vespa/searchcore/proton/test/bucketstatecalculator.h b/searchcore/src/vespa/searchcore/proton/test/bucketstatecalculator.h
index b3fc146d9c2..90b290b06cd 100644
--- a/searchcore/src/vespa/searchcore/proton/test/bucketstatecalculator.h
+++ b/searchcore/src/vespa/searchcore/proton/test/bucketstatecalculator.h
@@ -18,6 +18,7 @@ private:
     mutable BucketIdVector _asked;
     bool                   _clusterUp;
     bool                   _nodeUp;
+    bool                   _nodeRetired;
 
 public:
     typedef std::shared_ptr<BucketStateCalculator> SP;
@@ -25,7 +26,8 @@ public:
         _ready(),
         _asked(),
         _clusterUp(true),
-        _nodeUp(true)
+        _nodeUp(true),
+        _nodeRetired(false)
     {
     }
     BucketStateCalculator &addReady(const document::BucketId &bucket) {
@@ -36,39 +38,43 @@ public:
         _ready.erase(bucket);
         return *this;
     }
-    BucketStateCalculator &setClusterUp(bool value) {
+    BucketStateCalculator &setClusterUp(bool value) noexcept {
         _clusterUp = value;
         return *this;
     }
 
-    BucketStateCalculator &
-    setNodeUp(bool value)
-    {
+    BucketStateCalculator & setNodeUp(bool value) noexcept {
         _nodeUp = value;
         return *this;
     }
 
-    const BucketIdVector &asked() const { return _asked; }
+    BucketStateCalculator& setNodeRetired(bool retired) noexcept {
+        _nodeRetired = retired;
+        return *this;
+    }
+
+    const BucketIdVector &asked() const noexcept { return _asked; }
     void resetAsked() { _asked.clear(); }
 
     // Implements IBucketStateCalculator
-    virtual bool shouldBeReady(const document::BucketId &bucket) const {
+    bool shouldBeReady(const document::BucketId &bucket) const override {
         _asked.push_back(bucket);
         return _ready.count(bucket) == 1;
     }
-    virtual bool clusterUp() const {
+
+    bool clusterUp() const override {
         return _clusterUp;
     }
     
-    virtual bool
-    nodeUp(void) const
-    {
+    bool nodeUp() const override {
         return _nodeUp;
     }
 
-    virtual bool
-    nodeInitializing(void) const
-    {
+    bool nodeRetired() const override {
+        return _nodeRetired;
+    }
+
+    bool nodeInitializing() const override {
         return false;
     }
 };