From 492daa404b4deec73aa214b5d7a3a961371b3652 Mon Sep 17 00:00:00 2001 From: Tobias Schottdorf Date: Fri, 16 Aug 2019 17:53:39 +0200 Subject: [PATCH] storage: fix replica removal when learners not active 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: https://github.com/cockroachdb/cockroach/pull/39640#pullrequestreview-275532068 Release note: None --- pkg/server/version_cluster_test.go | 12 ++++++++++++ pkg/storage/replica_command.go | 7 +++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/pkg/server/version_cluster_test.go b/pkg/server/version_cluster_test.go index 73bf3f8ac18c..ae27d84d29eb 100644 --- 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 index 0ff7956dca16..2ac35a0f0c12 100644 --- 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 {