Revert "Continue serving search queries when in Maintenance node state [run-systemtest]"

11 files changed, 21 insertions, 164 deletions

diff --git a/searchcore/src/tests/proton/documentdb/buckethandler/buckethandler_test.cpp b/searchcore/src/tests/proton/documentdb/buckethandler/buckethandler_test.cpp
index cadfa8cd72f..29748a2010c 100644
--- a/searchcore/src/tests/proton/documentdb/buckethandler/buckethandler_test.cpp
+++ b/searchcore/src/tests/proton/documentdb/buckethandler/buckethandler_test.cpp
@@ -140,11 +140,6 @@ struct Fixture
     setNodeUp(bool value)
     {
         _calc->setNodeUp(value);
-        _calc->setNodeMaintenance(false);
-        _handler.notifyClusterStateChanged(_calc);
-    }
-    void setNodeMaintenance(bool value) {
-        _calc->setNodeMaintenance(value);
         _handler.notifyClusterStateChanged(_calc);
     }
 };
@@ -228,7 +223,7 @@ TEST_F("require that unready bucket can be reported as active", Fixture)
 }
 
 
-TEST_F("node going down (but not into maintenance state) deactivates all buckets", Fixture)
+TEST_F("require that node being down deactivates buckets", Fixture)
 {
     f._handler.handleSetCurrentState(f._ready.bucket(2),
                                      BucketInfo::ACTIVE, f._genResult);
@@ -257,42 +252,6 @@ TEST_F("node going down (but not into maintenance state) deactivates all buckets
     EXPECT_EQUAL(true, f._bucketInfo.getInfo().isActive());
 }
 
-TEST_F("node going into maintenance state does _not_ deactivate any buckets", Fixture)
-{
-    f._handler.handleSetCurrentState(f._ready.bucket(2),
-                                     BucketInfo::ACTIVE, f._genResult);
-    f.sync();
-    f.setNodeMaintenance(true);
-    f.sync();
-    f.handleGetBucketInfo(f._ready.bucket(2));
-    EXPECT_TRUE(f._bucketInfo.getInfo().isActive());
-}
-
-TEST_F("node going from maintenance to up state deactivates all buckets", Fixture)
-{
-    f._handler.handleSetCurrentState(f._ready.bucket(2),
-                                     BucketInfo::ACTIVE, f._genResult);
-    f.sync();
-    f.setNodeMaintenance(true);
-    f.sync();
-    f.setNodeUp(true);
-    f.sync();
-    f.handleGetBucketInfo(f._ready.bucket(2));
-    EXPECT_FALSE(f._bucketInfo.getInfo().isActive());
-}
-
-TEST_F("node going from maintenance to down state deactivates all buckets", Fixture)
-{
-    f._handler.handleSetCurrentState(f._ready.bucket(2),
-                                     BucketInfo::ACTIVE, f._genResult);
-    f.sync();
-    f.setNodeMaintenance(true);
-    f.sync();
-    f.setNodeUp(false);
-    f.sync();
-    f.handleGetBucketInfo(f._ready.bucket(2));
-    EXPECT_FALSE(f._bucketInfo.getInfo().isActive());
-}
 
 TEST_MAIN()
 {
diff --git a/searchcore/src/tests/proton/matchengine/matchengine.cpp b/searchcore/src/tests/proton/matchengine/matchengine.cpp
index 34c36fd9a72..481a9f061be 100644
--- a/searchcore/src/tests/proton/matchengine/matchengine.cpp
+++ b/searchcore/src/tests/proton/matchengine/matchengine.cpp
@@ -17,7 +17,7 @@ class MySearchHandler : public ISearchHandler {
     std::string _name;
     std::string _reply;
 public:
-    explicit MySearchHandler(size_t numHits = 0) :
+    MySearchHandler(size_t numHits = 0) :
         _numHits(numHits), _name("my"), _reply("myreply")
     {}
     DocsumReply::UP getDocsums(const DocsumRequest &) override {
@@ -91,7 +91,6 @@ assertSearchReply(MatchEngine & engine, const std::string & searchDocType, size_
     LocalSearchClient client;
     engine.search(SearchRequest::Source(request), client);
     SearchReply::UP reply = client.getReply(10000);
-    ASSERT_TRUE(reply);
     return EXPECT_EQUAL(expHits, reply->hits.size());
 }
 
@@ -174,24 +173,11 @@ TEST("requireThatEmptySearchReplyIsReturnedWhenEngineIsClosed")
     LocalSearchClient client;
     SearchRequest::Source request(new SearchRequest());
     SearchReply::UP reply = engine.search(std::move(request), client);
-    ASSERT_TRUE(reply);
+    EXPECT_TRUE(reply );
     EXPECT_EQUAL(0u, reply->hits.size());
     EXPECT_EQUAL(7u, reply->getDistributionKey());
 }
 
-namespace {
-
-constexpr const char* search_interface_offline_slime_str() noexcept {
-    return "{\n"
-           "    \"status\": {\n"
-           "        \"state\": \"OFFLINE\",\n"
-           "        \"message\": \"Search interface is offline\"\n"
-           "    }\n"
-           "}\n";
-}
-
-}
-
 TEST("requireThatStateIsReported")
 {
     MatchEngine engine(1, 1, 7);
@@ -199,44 +185,14 @@ TEST("requireThatStateIsReported")
     Slime slime;
     SlimeInserter inserter(slime);
     engine.get_state(inserter, false);
-    EXPECT_EQUAL(search_interface_offline_slime_str(),
-                 slime.toString());
-}
-
-TEST("searches are executed when node is in maintenance mode")
-{
-    MatchEngine engine(1, 1, 7);
-    engine.setNodeMaintenance(true);
-    engine.putSearchHandler(DocTypeName("foo"), std::make_shared<MySearchHandler>(3));
-    EXPECT_TRUE(assertSearchReply(engine, "foo", 3));
-}
-
-TEST("setNodeMaintenance(true) implies setNodeUp(false)")
-{
-    MatchEngine engine(1, 1, 7);
-    engine.setNodeUp(true);
-    engine.setNodeMaintenance(true);
-    EXPECT_FALSE(engine.isOnline());
-}
-
-TEST("setNodeMaintenance(false) does not imply setNodeUp(false)")
-{
-    MatchEngine engine(1, 1, 7);
-    engine.setNodeUp(true);
-    engine.setNodeMaintenance(false);
-    EXPECT_TRUE(engine.isOnline());
-}
-
-TEST("search interface is reported as offline when node is in maintenance mode")
-{
-    MatchEngine engine(1, 1, 7);
-    engine.setNodeMaintenance(true);
-
-    Slime slime;
-    SlimeInserter inserter(slime);
-    engine.get_state(inserter, false);
-    EXPECT_EQUAL(search_interface_offline_slime_str(),
-                 slime.toString());
+    EXPECT_EQUAL(
+            "{\n"
+            "    \"status\": {\n"
+            "        \"state\": \"OFFLINE\",\n"
+            "        \"message\": \"Search interface is offline\"\n"
+            "    }\n"
+            "}\n",
+            slime.toString());
 }
 
 TEST_MAIN() { TEST_RUN_ALL(); }
diff --git a/searchcore/src/vespa/searchcore/proton/matchengine/matchengine.cpp b/searchcore/src/vespa/searchcore/proton/matchengine/matchengine.cpp
index 58dc473b85e..5ad4a7ed52b 100644
--- a/searchcore/src/vespa/searchcore/proton/matchengine/matchengine.cpp
+++ b/searchcore/src/vespa/searchcore/proton/matchengine/matchengine.cpp
@@ -50,8 +50,7 @@ MatchEngine::MatchEngine(size_t numThreads, size_t threadsPerSearch, uint32_t di
       _handlers(),
       _executor(std::max(size_t(1), numThreads / threadsPerSearch), 256_Ki, match_engine_executor),
       _threadBundlePool(std::max(size_t(1), threadsPerSearch)),
-      _nodeUp(false),
-      _nodeMaintenance(false)
+      _nodeUp(false)
 {
 }
 
@@ -99,8 +98,7 @@ search::engine::SearchReply::UP
 MatchEngine::search(search::engine::SearchRequest::Source request,
                     search::engine::SearchClient &client)
 {
-    // We continue to allow searches if the node is in Maintenance mode
-    if (_closed || (!_nodeUp && !_nodeMaintenance)) {
+    if (_closed || !_nodeUp) {
         auto ret = std::make_unique<search::engine::SearchReply>();
         ret->setDistributionKey(_distributionKey);
 
@@ -179,14 +177,6 @@ MatchEngine::setNodeUp(bool nodeUp)
     _nodeUp = nodeUp;
 }
 
-void
-MatchEngine::setNodeMaintenance(bool nodeMaintenance)
-{
-    _nodeMaintenance = nodeMaintenance;
-    if (nodeMaintenance) {
-        _nodeUp = false;
-    }
-}
 
 StatusReport::UP
 MatchEngine::reportStatus() const
diff --git a/searchcore/src/vespa/searchcore/proton/matchengine/matchengine.h b/searchcore/src/vespa/searchcore/proton/matchengine/matchengine.h
index 7cc0c97048b..b4e32c45003 100644
--- a/searchcore/src/vespa/searchcore/proton/matchengine/matchengine.h
+++ b/searchcore/src/vespa/searchcore/proton/matchengine/matchengine.h
@@ -26,7 +26,6 @@ private:
     vespalib::ThreadStackExecutor      _executor;
     vespalib::SimpleThreadBundle::Pool _threadBundlePool;
     bool                               _nodeUp;
-    bool                               _nodeMaintenance;
 
 public:
     /**
@@ -132,13 +131,6 @@ public:
      */
     void setNodeUp(bool nodeUp);
 
-    /**
-     * Set node into maintenance, based on info from cluster controller. Note that
-     * nodeMaintenance == true also implies setNodeUp(false), as the node is technically
-     * not in a Up state.
-     */
-    void setNodeMaintenance(bool nodeMaintenance);
-
     StatusReport::UP reportStatus() const;
 
     search::engine::SearchReply::UP search(
diff --git a/searchcore/src/vespa/searchcore/proton/server/buckethandler.cpp b/searchcore/src/vespa/searchcore/proton/server/buckethandler.cpp
index 0d9b2f6aaf6..c15be9336fe 100644
--- a/searchcore/src/vespa/searchcore/proton/server/buckethandler.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/buckethandler.cpp
@@ -68,8 +68,7 @@ BucketHandler::BucketHandler(vespalib::Executor &executor)
       _executor(executor),
       _ready(nullptr),
       _changedHandlers(),
-      _nodeUp(false),
-      _nodeMaintenance(false)
+      _nodeUp(false)
 {
     LOG(spam, "BucketHandler::BucketHandler");
 }
@@ -144,32 +143,13 @@ BucketHandler::handlePopulateActiveBuckets(document::BucketId::List &buckets,
     }));
 }
 
-namespace {
-constexpr const char* bool_str(bool v) noexcept {
-    return v ? "true" : "false";
-}
-}
-
 void
 BucketHandler::notifyClusterStateChanged(const std::shared_ptr<IBucketStateCalculator> & newCalc)
 {
     bool oldNodeUp = _nodeUp;
-    bool oldNodeMaintenance = _nodeMaintenance;
-    _nodeUp = newCalc->nodeUp(); // Up, Retired or Initializing
-    _nodeMaintenance = newCalc->nodeMaintenance();
-    LOG(spam, "notifyClusterStateChanged; up: %s -> %s, maintenance: %s -> %s",
-        bool_str(oldNodeUp), bool_str(_nodeUp),
-        bool_str(oldNodeMaintenance), bool_str(_nodeMaintenance));
-    if (_nodeMaintenance) {
-        return; // Don't deactivate buckets in maintenance mode; let query traffic drain away naturally.
-    }
-    // We implicitly deactivate buckets in two edge cases:
-    //  - Up -> Down (not maintenance; handled above), since the node can not be expected to offer
-    //    any graceful query draining when set Down.
-    //  - Maintenance -> !Maintenance, since we'd otherwise introduce a bunch of transient duplicate
-    //    results into queries if we transition to an available state.
-    //    The assumption is that the system has already activated buckets on other nodes in such a scenario.
-    if ((oldNodeUp && !_nodeUp) || oldNodeMaintenance) {
+    _nodeUp = newCalc->nodeUp();
+    LOG(spam, "notifyClusterStateChanged: %s -> %s", oldNodeUp ? "up" : "down", _nodeUp ? "up" : "down");
+    if (oldNodeUp && !_nodeUp) {
         deactivateAllActiveBuckets();
     }
 }
diff --git a/searchcore/src/vespa/searchcore/proton/server/buckethandler.h b/searchcore/src/vespa/searchcore/proton/server/buckethandler.h
index 2344e080450..7f44d2ebd71 100644
--- a/searchcore/src/vespa/searchcore/proton/server/buckethandler.h
+++ b/searchcore/src/vespa/searchcore/proton/server/buckethandler.h
@@ -25,7 +25,6 @@ private:
     documentmetastore::IBucketHandler        *_ready;
     std::vector<IBucketStateChangedHandler *> _changedHandlers;
     bool                                      _nodeUp;
-    bool                                      _nodeMaintenance;
 
     void performSetCurrentState(document::BucketId bucketId,
                                 storage::spi::BucketInfo::ActiveState newState,
diff --git a/searchcore/src/vespa/searchcore/proton/server/clusterstatehandler.cpp b/searchcore/src/vespa/searchcore/proton/server/clusterstatehandler.cpp
index 20cd4ced6b2..3d709bd19d1 100644
--- a/searchcore/src/vespa/searchcore/proton/server/clusterstatehandler.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/clusterstatehandler.cpp
@@ -27,7 +27,6 @@ private:
     bool _nodeUp;
     bool _nodeInitializing;
     bool _nodeRetired;
-    bool _nodeMaintenance;
 
 public:
     ClusterStateAdapter(const ClusterState &calc)
@@ -35,8 +34,7 @@ public:
           _clusterUp(_calc.clusterUp()),
           _nodeUp(_calc.nodeUp()),
           _nodeInitializing(_calc.nodeInitializing()),
-          _nodeRetired(_calc.nodeRetired()),
-          _nodeMaintenance(_calc.nodeMaintenance())
+          _nodeRetired(_calc.nodeRetired())
     {
     }
     vespalib::Trinary shouldBeReady(const document::Bucket &bucket) const override {
@@ -46,7 +44,6 @@ public:
     bool nodeUp() const override { return _nodeUp; }
     bool nodeInitializing() const override { return _nodeInitializing; }
     bool nodeRetired() const override { return _nodeRetired; }
-    bool nodeMaintenance() const noexcept override { return _nodeMaintenance; }
 };
 
 }
@@ -56,12 +53,11 @@ ClusterStateHandler::performSetClusterState(const ClusterState *calc, IGenericRe
 {
     LOG(debug,
         "performSetClusterState(): "
-        "clusterUp(%s), nodeUp(%s), nodeInitializing(%s), nodeMaintenance(%s)"
+        "clusterUp(%s), nodeUp(%s), nodeInitializing(%s)"
         "changedHandlers.size() = %zu",
         (calc->clusterUp() ? "true" : "false"),
         (calc->nodeUp() ? "true" : "false"),
         (calc->nodeInitializing() ? "true" : "false"),
-        (calc->nodeMaintenance() ? "true" : "false"),
         _changedHandlers.size());
     if (!_changedHandlers.empty()) {
         auto newCalc = std::make_shared<ClusterStateAdapter>(*calc);
diff --git a/searchcore/src/vespa/searchcore/proton/server/ibucketstatecalculator.h b/searchcore/src/vespa/searchcore/proton/server/ibucketstatecalculator.h
index 9534f346d1f..b60a5ad8175 100644
--- a/searchcore/src/vespa/searchcore/proton/server/ibucketstatecalculator.h
+++ b/searchcore/src/vespa/searchcore/proton/server/ibucketstatecalculator.h
@@ -15,7 +15,6 @@ struct IBucketStateCalculator
     virtual bool nodeUp() const = 0;
     virtual bool nodeInitializing() const = 0;
     virtual bool nodeRetired() const = 0;
-    virtual bool nodeMaintenance() const noexcept = 0;
     virtual ~IBucketStateCalculator() = default;
 };
 
diff --git a/searchcore/src/vespa/searchcore/proton/server/proton.cpp b/searchcore/src/vespa/searchcore/proton/server/proton.cpp
index 5b21242e397..275f9029107 100644
--- a/searchcore/src/vespa/searchcore/proton/server/proton.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/proton.cpp
@@ -865,11 +865,10 @@ Proton::setClusterState(BucketSpace bucketSpace, const storage::spi::ClusterStat
     // forward info sent by cluster controller to persistence engine
     // about whether node is supposed to be up or not.  Match engine
     // needs to know this in order to stop serving queries.
-    bool nodeUpInBucketSpace(calc.nodeUp()); // TODO rename calculator function to imply bucket space affinity
+    bool nodeUpInBucketSpace(calc.nodeUp());
     bool nodeRetired(calc.nodeRetired());
     bool nodeUp = updateNodeUp(bucketSpace, nodeUpInBucketSpace);
     _matchEngine->setNodeUp(nodeUp);
-    _matchEngine->setNodeMaintenance(calc.nodeMaintenance()); // Note: _all_ bucket spaces in maintenance
     if (_memoryFlushConfigUpdater) {
         _memoryFlushConfigUpdater->setNodeRetired(nodeRetired);
     }
diff --git a/searchcore/src/vespa/searchcore/proton/server/proton.h b/searchcore/src/vespa/searchcore/proton/server/proton.h
index 6fd31352051..6b0bef50cae 100644
--- a/searchcore/src/vespa/searchcore/proton/server/proton.h
+++ b/searchcore/src/vespa/searchcore/proton/server/proton.h
@@ -133,7 +133,6 @@ private:
     uint32_t getDistributionKey() const override { return _distributionKey; }
     BootstrapConfig::SP getActiveConfigSnapshot() const;
     std::shared_ptr<IDocumentDBReferenceRegistry> getDocumentDBReferenceRegistry() const override;
-    // Returns true if the node is up in _any_ bucket space
     bool updateNodeUp(BucketSpace bucketSpace, bool nodeUpInBucketSpace);
     void closeDocumentDBs(vespalib::ThreadStackExecutorBase & executor);
 public:
diff --git a/searchcore/src/vespa/searchcore/proton/test/bucketstatecalculator.h b/searchcore/src/vespa/searchcore/proton/test/bucketstatecalculator.h
index e218058f01e..a5a0185d787 100644
--- a/searchcore/src/vespa/searchcore/proton/test/bucketstatecalculator.h
+++ b/searchcore/src/vespa/searchcore/proton/test/bucketstatecalculator.h
@@ -20,7 +20,6 @@ private:
     bool                   _clusterUp;
     bool                   _nodeUp;
     bool                   _nodeRetired;
-    bool                   _nodeMaintenance;
 
 public:
     typedef std::shared_ptr<BucketStateCalculator> SP;
@@ -29,8 +28,7 @@ public:
         _asked(),
         _clusterUp(true),
         _nodeUp(true),
-        _nodeRetired(false),
-        _nodeMaintenance(false)
+        _nodeRetired(false)
     {
     }
     BucketStateCalculator &addReady(const document::BucketId &bucket) {
@@ -56,15 +54,6 @@ public:
         return *this;
     }
 
-    BucketStateCalculator& setNodeMaintenance(bool maintenance) noexcept {
-        _nodeMaintenance = maintenance;
-        if (maintenance) {
-            _nodeUp = false;
-            _nodeRetired = false;
-        }
-        return *this;
-    }
-
     const BucketIdVector &asked() const noexcept { return _asked; }
     void resetAsked() { _asked.clear(); }
 
@@ -78,7 +67,6 @@ public:
     bool nodeUp() const override { return _nodeUp; }
     bool nodeRetired() const override { return _nodeRetired; }
     bool nodeInitializing() const override { return false; }
-    bool nodeMaintenance() const noexcept override { return _nodeMaintenance; }
 };
 
 }