[raft] RemoveReplica (#8054)

Allow RemoveRequest to take a request without range. If we are removing
zombies,
that means the replica is not in the descriptor, so we don't need the
range
descriptor in that case. But we still want to check that the server is
the lease
holder of that range. 

Known issue: the Cleanup Zombie test can become flaky if we are removing
a leader. I will address it in a follow up PR when I refactor the zombie
code.

enterprise/server/raft/store/store.go

@@ -2283,24 +2283,40 @@ func (s *Store) AddReplica(ctx context.Context, req *rfpb.AddReplicaRequest) (*r
 //   - This node must be a member of the cluster that is being removed from
 //   - The provided range descriptor must be up to date
 func (s *Store) RemoveReplica(ctx context.Context, req *rfpb.RemoveReplicaRequest) (*rfpb.RemoveReplicaResponse, error) {
-	// Check this is a range we have and the range descriptor provided is up to date
+	rangeID := uint64(0)
+	if r := req.GetRange(); r != nil {
+		rangeID = r.GetRangeId()
+	} else {
+		rangeID = req.GetRangeId()
+	}
+
+	if rangeID == 0 {
+		return nil, status.InvalidArgumentError("no range id is specified")
+	}
+	if !s.HaveLease(ctx, rangeID) {
+		return nil, status.OutOfRangeErrorf("%s: no lease found for range: %d", constants.RangeLeaseInvalidMsg, rangeID)
+	}
+
+	// Check this is a range we have.
 	s.rangeMu.RLock()
-	rd, rangeOK := s.openRanges[req.GetRange().GetRangeId()]
+	rd, rangeOK := s.openRanges[rangeID]
 	s.rangeMu.RUnlock()
 
 	if !rangeOK {
 		return nil, status.OutOfRangeErrorf("%s: range %d", constants.RangeNotFoundMsg, req.GetRange().GetRangeId())
 	}
-	if rd.GetGeneration() != req.GetRange().GetGeneration() {
-		return nil, status.OutOfRangeErrorf("%s: generation: %d requested: %d", constants.RangeNotCurrentMsg, rd.GetGeneration(), req.GetRange().GetGeneration())
+
+	if r := req.GetRange(); r != nil {
+		if rd.GetGeneration() != r.GetGeneration() {
+			return nil, status.OutOfRangeErrorf("%s: generation: %d requested: %d", constants.RangeNotCurrentMsg, rd.GetGeneration(), req.GetRange().GetGeneration())
+		}
 	}
 
 	var replicaDesc *rfpb.ReplicaDescriptor
-	for _, replica := range req.GetRange().GetReplicas() {
+	for _, replica := range rd.GetReplicas() {
 		if replica.GetReplicaId() == req.GetReplicaId() {
 			if replica.GetNhid() == s.NHID() {
-				// return UnavailableError, so TryReplicas can skip this replica
-				return nil, status.UnavailableErrorf("c%dn%d is on the node %q, cannot remove itself", req.GetRange().GetRangeId(), req.GetReplicaId(), s.NHID())
+				return nil, status.FailedPreconditionErrorf("cannot remove leader c%dn%d", rangeID, req.GetReplicaId())
 			}
 			replicaDesc = replica
 			break
@@ -2309,6 +2325,9 @@ func (s *Store) RemoveReplica(ctx context.Context, req *rfpb.RemoveReplicaReques
 
 	var err error
 	if replicaDesc != nil {
+		if req.GetRange() == nil {
+			return nil, status.FailedPreconditionErrorf("c%dn%d is in the range descritpor, an up-to-date range descriptor is required to remove it", rangeID, req.GetReplicaId())
+		}
 		// First, update the range descriptor information to reflect the
 		// the node being removed.
 		rd, err = s.removeReplicaFromRangeDescriptor(ctx, replicaDesc.GetRangeId(), replicaDesc.GetReplicaId(), req.GetRange())
@@ -2317,7 +2336,7 @@ func (s *Store) RemoveReplica(ctx context.Context, req *rfpb.RemoveReplicaReques
 		}
 	}
 
-	if err = s.syncRequestDeleteReplica(ctx, req.GetRange().GetRangeId(), req.GetReplicaId()); err != nil {
+	if err = s.syncRequestDeleteReplica(ctx, rangeID, req.GetReplicaId()); err != nil {
 		return nil, status.InternalErrorf("nodehost.SyncRequestDeleteReplica failed for c%dn%d: %s", req.GetRange().GetRangeId(), req.GetReplicaId(), err)
 	}
 

proto/raft.proto

@@ -587,10 +587,24 @@ message AddReplicaResponse {
   RangeDescriptor range = 1;
 }
 
+// RemoveReplicaRequest specifies a request to remove the replica from raft
+// memberhsip, and remove the replica from the range descriptor. It does *not*
+// remove the data from the machine.
 message RemoveReplicaRequest {
+  // Either specify range or range_id:
+  //  -- When a range is provided, it is expected that the range is up-to-date.
+  //  The
+  // call removes the replica from the range descriptor and also from the raft
+  // membership.
+  //  -- When a range_id is provided, it is expected that the replica_id is
+  // already removed from the range descriptor. For example, when we detected a
+  // zombie.
   RangeDescriptor range = 1;
+  uint64 range_id = 3;
+
   uint64 replica_id = 2;
 }
+
 message RemoveReplicaResponse {
   // The range with the specified node removed.
   RangeDescriptor range = 1;