storage: fix replica removal when learners not active

I introduced a bug in cockroachdb#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:

cockroachdb#39640 (review)

Release note: None

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)

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 {