raft: remove StepDownOnRemoval

This was added in ee0fe9d and has defaulted to true in cockroachdb
since 4dcbdcd for clusters with v23.2 active. Now that compatibility
with pre-v23.2 is no longer required, we can remove the know from the
raft layer.

Epic: None
Release note: None

pkg/kv/kvserver/store.go

@@ -87,7 +87,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/log/logcrash"
 	"github.com/cockroachdb/cockroach/pkg/util/log/severity"
-	"github.com/cockroachdb/cockroach/pkg/util/metamorphic"
 	"github.com/cockroachdb/cockroach/pkg/util/metric"
 	"github.com/cockroachdb/cockroach/pkg/util/mon"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
@@ -334,12 +333,6 @@ var SnapshotSendLimit = settings.RegisterIntSetting(
 	settings.NonNegativeInt,
 )
 
-// raftStepDownOnRemoval is a metamorphic test parameter that makes Raft leaders
-// step down on demotion or removal. Following an upgrade, clusters may have
-// replicas with mixed settings, because it's only changed when initializing
-// replicas. Varying it makes sure we handle this state.
-var raftStepDownOnRemoval = metamorphic.ConstantWithTestBool("raft-step-down-on-removal", true)
-
 // TestStoreConfig has some fields initialized with values relevant in tests.
 func TestStoreConfig(clock *hlc.Clock) StoreConfig {
 	return testStoreConfig(clock, clusterversion.Latest.Version())
@@ -409,16 +402,9 @@ func newRaftConfig(
 		Storage:                     strg,
 		Logger:                      logger,
 		StoreLiveness:               storeLiveness,
-
-		// We only set this on replica initialization, so replicas without
-		// StepDownOnRemoval may remain on 23.2 nodes until they restart. That's
-		// totally fine, we just can't rely on this behavior until 24.1, but
-		// we currently don't either.
-		StepDownOnRemoval: raftStepDownOnRemoval,
-
-		PreVote:     true,
-		CheckQuorum: storeCfg.RaftEnableCheckQuorum,
-		CRDBVersion: storeCfg.Settings.Version,
+		PreVote:                     true,
+		CheckQuorum:                 storeCfg.RaftEnableCheckQuorum,
+		CRDBVersion:                 storeCfg.Settings.Version,
 	}
 }
 

pkg/raft/raft.go

@@ -260,13 +260,6 @@ type Config struct {
 	// See: https://github.com/etcd-io/raft/issues/80
 	DisableConfChangeValidation bool
 
-	// StepDownOnRemoval makes the leader step down when it is removed from the
-	// group or demoted to a learner.
-	//
-	// This behavior will become unconditional in the future. See:
-	// https://github.com/etcd-io/raft/issues/83
-	StepDownOnRemoval bool
-
 	// StoreLiveness is a reference to the store liveness fabric.
 	StoreLiveness raftstoreliveness.StoreLiveness
 
@@ -428,7 +421,6 @@ type raft struct {
 	// when raft changes its state to follower or candidate.
 	randomizedElectionTimeout int
 	disableProposalForwarding bool
-	stepDownOnRemoval         bool
 
 	tick func()
 	step stepFunc
@@ -464,7 +456,6 @@ func newRaft(c *Config) *raft {
 		preVote:                     c.PreVote,
 		disableProposalForwarding:   c.DisableProposalForwarding,
 		disableConfChangeValidation: c.DisableConfChangeValidation,
-		stepDownOnRemoval:           c.StepDownOnRemoval,
 		storeLiveness:               c.StoreLiveness,
 		crdbVersion:                 c.CRDBVersion,
 	}
@@ -2276,19 +2267,18 @@ func (r *raft) switchToConfig(cfg quorum.Config, progressMap tracker.ProgressMap
 	r.isLearner = pr != nil && pr.IsLearner
 
 	if (pr == nil || r.isLearner) && r.state == StateLeader {
-		// This node is leader and was removed or demoted, step down if requested.
+		// This node is leader and was removed or demoted, step down.
 		//
 		// We prevent demotions at the time writing but hypothetically we handle
 		// them the same way as removing the leader.
 		//
 		// TODO(tbg): ask follower with largest Match to TimeoutNow (to avoid
 		// interruption). This might still drop some proposals but it's better than
 		// nothing.
-		if r.stepDownOnRemoval {
-			// NB: Similar to the CheckQuorum step down case, we must remember our
-			// prior stint as leader, lest we regress the QSE.
-			r.becomeFollower(r.Term, r.lead)
-		}
+		//
+		// NB: Similar to the CheckQuorum step down case, we must remember our
+		// prior stint as leader, lest we regress the QSE.
+		r.becomeFollower(r.Term, r.lead)
 		return cs
 	}
 

pkg/raft/rafttest/interaction_env_handler_add_nodes.go

@@ -65,8 +65,6 @@ func (env *InteractionEnv) handleAddNodes(t *testing.T, d datadriven.TestData) e
 				arg.Scan(t, i, &cfg.MaxCommittedSizePerReady)
 			case "disable-conf-change-validation":
 				arg.Scan(t, i, &cfg.DisableConfChangeValidation)
-			case "step-down-on-removal":
-				arg.Scan(t, i, &cfg.StepDownOnRemoval)
 			case "crdb-version":
 				var key string
 				arg.Scan(t, i, &key)

pkg/raft/testdata/confchange_v1_remove_leader_stepdown.txt

@@ -3,10 +3,9 @@ log-level none
 ----
 ok
 
-# Run a V1 membership change that removes the leader, asking it
-# to step down on removal.
+# Run a V1 membership change that removes the leader.
 # Bootstrap n1, n2, n3.
-add-nodes 3 voters=(1,2,3) index=2 step-down-on-removal=true
+add-nodes 3 voters=(1,2,3) index=2
 ----
 ok