cleanoutServer Bug Fix (#6537)

* Fixing bug: cleanoutServer will no longer add old leader as follower.

* Fixed rollback.

arangod/Agency/CleanOutServer.cpp

@@ -389,6 +389,8 @@ bool CleanOutServer::scheduleMoveShards(std::shared_ptr<Builder>& trx) {
             serversCopy.end());
         }
 
+        bool isLeader = (found == 0);
+
         // Among those a random destination:
         std::string toServer;
         if (serversCopy.empty()) {
@@ -403,7 +405,7 @@ bool CleanOutServer::scheduleMoveShards(std::shared_ptr<Builder>& trx) {
         // Schedule move into trx:
         MoveShard(_snapshot, _agent, _jobId + "-" + std::to_string(sub++),
                   _jobId, database.first, collptr.first,
-                  shard.first, _server, toServer, found == 0)
+                  shard.first, _server, toServer, isLeader, false)
           .create(trx);
       }
     }

arangod/Agency/MoveShard.cpp

@@ -30,6 +30,22 @@
 using namespace arangodb;
 using namespace arangodb::consensus;
 
+MoveShard::MoveShard(Node const& snapshot, AgentInterface* agent,
+                     std::string const& jobId, std::string const& creator,
+                     std::string const& database,
+                     std::string const& collection, std::string const& shard,
+                     std::string const& from, std::string const& to,
+                     bool isLeader, bool remainsFollower)
+    : Job(NOTFOUND, snapshot, agent, jobId, creator),
+      _database(database),
+      _collection(collection),
+      _shard(shard),
+      _from(id(from)),
+      _to(id(to)),
+      _isLeader(isLeader), // will be initialized properly when information known
+      _remainsFollower(remainsFollower)
+{ }
+
 MoveShard::MoveShard(Node const& snapshot, AgentInterface* agent,
                      std::string const& jobId, std::string const& creator,
                      std::string const& database,
@@ -42,7 +58,8 @@ MoveShard::MoveShard(Node const& snapshot, AgentInterface* agent,
       _shard(shard),
       _from(id(from)),
       _to(id(to)),
-      _isLeader(isLeader) // will be initialized properly when information known
+      _isLeader(isLeader), // will be initialized properly when information known
+      _remainsFollower(isLeader)
 { }
 
 MoveShard::MoveShard(Node const& snapshot, AgentInterface* agent,
@@ -57,6 +74,7 @@ MoveShard::MoveShard(Node const& snapshot, AgentInterface* agent,
   auto tmp_to = _snapshot.hasAsString(path + "toServer");
   auto tmp_shard = _snapshot.hasAsString(path + "shard");
   auto tmp_isLeader = _snapshot.hasAsSlice(path + "isLeader");
+  auto tmp_remainsFollower = _snapshot.hasAsSlice(path + "remainsFollower");
   auto tmp_creator = _snapshot.hasAsString(path + "creator");
 
   if (tmp_database.second && tmp_collection.second && tmp_from.second && tmp_to.second
@@ -67,6 +85,7 @@ MoveShard::MoveShard(Node const& snapshot, AgentInterface* agent,
     _to = tmp_to.first;
     _shard = tmp_shard.first;
     _isLeader = tmp_isLeader.first.isTrue();
+    _remainsFollower = tmp_remainsFollower.second ? tmp_remainsFollower.first.isTrue() : _isLeader;
     _creator = tmp_creator.first;
   } else {
     std::stringstream err;
@@ -130,6 +149,7 @@ bool MoveShard::create(std::shared_ptr<VPackBuilder> envelope) {
     _jb->add("fromServer", VPackValue(_from));
     _jb->add("toServer", VPackValue(_to));
     _jb->add("isLeader", VPackValue(_isLeader));
+    _jb->add("remainsFollower", VPackValue(_remainsFollower));
     _jb->add("jobId", VPackValue(_jobId));
     _jb->add("timeCreated", VPackValue(now));
   }
@@ -276,6 +296,11 @@ bool MoveShard::start() {
     return false;
   }
 
+  if (!_isLeader && _remainsFollower) {
+    finish("", "", false, "remainsFollower is invalid without isLeader");
+    return false;
+  }
+
   // Compute group to move shards together:
   std::vector<Job::shard_t> shardsLikeMe
       = clones(_snapshot, _database, _collection, _shard);
@@ -507,6 +532,7 @@ JOB_STATUS MoveShard::pendingLeader() {
                   trx.add(srv);
                 }
               }
+              // add the old leader as follower in case of a rollback
               trx.add(VPackValue(_from));
             }
             // Precondition: Plan still as it was
@@ -545,7 +571,19 @@ JOB_STATUS MoveShard::pendingLeader() {
       { VPackObjectBuilder trxObject(&trx);
         VPackObjectBuilder preObject(&pre);
         doForAllShards(_snapshot, _database, shardsLikeMe,
-          [&pre](Slice plan, Slice current, std::string& planPath) {
+          [&trx, &pre, this](Slice plan, Slice current, std::string& planPath) {
+            if (!_remainsFollower) {
+              // Remove _from from the list of follower
+              trx.add(VPackValue(planPath));
+              { VPackArrayBuilder guard(&trx);
+                for (auto const& srv : VPackArrayIterator(plan)) {
+                  if (!srv.isEqualString(_from)) {
+                    trx.add(srv);
+                  }
+                }
+              }
+            }
+
             // Precondition: Plan still as it was
             pre.add(VPackValue(planPath));
             { VPackObjectBuilder guard(&pre);

arangod/Agency/MoveShard.h

@@ -32,6 +32,16 @@ namespace consensus {
 
 struct MoveShard : public Job {
 
+  MoveShard(Node const& snapshot, AgentInterface* agent, std::string const& jobId,
+            std::string const& creator,
+            std::string const& database,
+            std::string const& collection,
+            std::string const& shard,
+            std::string const& from,
+            std::string const& to,
+            bool isLeader,
+            bool remainsFollower);
+
   MoveShard(Node const& snapshot, AgentInterface* agent, std::string const& jobId,
             std::string const& creator,
             std::string const& database,
@@ -61,6 +71,7 @@ struct MoveShard : public Job {
   std::string _from;
   std::string _to;
   bool        _isLeader;
+  bool        _remainsFollower;
 };
 }
 }

tests/Agency/CleanOutServerTest.cpp

@@ -66,6 +66,7 @@ VPackBuilder createMoveShardJob() {
     builder.add("fromServer", VPackValue("test"));
     builder.add("toServer", VPackValue("test2"));
     builder.add("isLeader", VPackValue(true));
+    builder.add("remainsFollower", VPackValue(false));
     builder.add("collection", VPackValue("test"));
     builder.add("shard", VPackValue("s99"));
     builder.add("creator", VPackValue("unittest"));

tests/Agency/FailedFollowerTest.cpp

@@ -429,7 +429,7 @@ SECTION("abort any moveShard job blocking the shard and start") {
   AgentInterface &moveShardAgent = moveShardMockAgent.get();
   auto moveShard = MoveShard(
     baseStructure(PREFIX), &moveShardAgent, "2", "strunz", DATABASE,
-    COLLECTION, SHARD, SHARD_LEADER, FREE_SERVER, true);
+    COLLECTION, SHARD, SHARD_LEADER, FREE_SERVER, true, true);
   moveShard.create();
 
   std::string jobId = "1";

tests/Agency/FailedLeaderTest.cpp

@@ -466,7 +466,7 @@ SECTION("abort any moveShard job blocking the shard and start") {
   AgentInterface &moveShardAgent = moveShardMockAgent.get();
   auto moveShard = MoveShard(
     baseStructure("arango"), &moveShardAgent, "2", "strunz", DATABASE,
-    COLLECTION, SHARD, SHARD_LEADER, FREE_SERVER, true);
+    COLLECTION, SHARD, SHARD_LEADER, FREE_SERVER, true, true);
   moveShard.create();
 
   std::string jobId = "1";