Merge #39697

39697: storage: fix replica removal when learners not active r=danhhz a=tbg I introduced a bug in #39640 which would fail removal of replicas whenever preemptive snapshots were used, since we'd accidentally send a preemptive snapshot which would end up with a "node already in descriptor" error. See: #39640 (review) Release note: None Co-authored-by: Tobias Schottdorf <[email protected]>
cockroachdb · Aug 16, 2019 · a00215b · a00215b
2 parents 7880622 + 492daa4
commit a00215b
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 2 deletions.
diff --git a/pkg/server/version_cluster_test.go b/pkg/server/version_cluster_test.go
@@ -34,6 +34,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 type testClusterWithHelpers struct {
@@ -392,6 +393,17 @@ func TestClusterVersionUpgrade(t *testing.T) {
 	tc := setupMixedCluster(t, knobs, versions, dir)
 	defer tc.TestCluster.Stopper().Stop(ctx)
 
+	{
+		// Regression test for the fix for this issue:
+		// https://github.com/cockroachdb/cockroach/pull/39640#pullrequestreview-275532068
+		//
+		// This can be removed when VersionLearnerReplicas is always-on.
+		k := tc.ScratchRange(t)
+		tc.AddReplicasOrFatal(t, k, tc.Target(2))
+		_, err := tc.RemoveReplicas(k, tc.Target(2))
+		require.NoError(t, err)
+	}
+
 	// Set CLUSTER SETTING cluster.preserve_downgrade_option to oldVersion to prevent upgrade.
 	if err := tc.setDowngrade(0, oldVersion.String()); err != nil {
 		t.Fatalf("error setting CLUSTER SETTING cluster.preserve_downgrade_option: %s", err)

diff --git a/pkg/storage/replica_command.go b/pkg/storage/replica_command.go
@@ -916,8 +916,11 @@ func (r *Replica) ChangeReplicas(
 		if len(chgs) != 1 {
 			return nil, errors.Errorf("need exactly one change, got %+v", chgs)
 		}
-		target := chgs[0].Target
-		return r.addReplicaLegacyPreemptiveSnapshot(ctx, target, desc, priority, reason, details)
+		if chgs[0].ChangeType == roachpb.ADD_REPLICA {
+			return r.addReplicaLegacyPreemptiveSnapshot(ctx, chgs[0].Target, desc, priority, reason, details)
+		}
+		// We're removing a single voter.
+		return r.finalizeChangeReplicas(ctx, desc, priority, reason, details, chgs)
 	}
 
 	if adds := chgs.Additions(); len(adds) > 0 {