Merge cockroachdb#131445 cockroachdb#132349 cockroachdb#132574

131445: raft: forbid direct voter removal without demotion r=nvanbenschoten a=nvanbenschoten

Informs cockroachdb#129796.
Informs cockroachdb#125355.

This commit is a natural continuation of 7f37eaa (from 5 years ago). The commit made it so that the replicateQueue would never issue a ChangeReplicas request that removed a voter replica. Instead, the replicate queue would always demote the voter replica to a learner replica first.

This commit makes it so that raft will no longer allow direct voter removal in a proposed configuration change. Instead, it requires the voter (who may be the leader) to first be demoted to a learner. This prevents abuse and strengthens the guarantee that we will never perform such unsafe configuration changes.

Release note: None

132349: server: Fix NPE in spanStatsFanOut r=kyle-a-wong a=kyle-a-wong

a NPE was surfaced when doing a spanstats request on an unfinalized (mixed version) cluster.

To fix, defensive checks are added to defend against potential nil responses and references to non-existant map entries for a request span stat

Fixes: cockroachdb#132130
Release note (bug fix): Fixes a bug where a span stats request on a mixed version cluster resulted in an NPE

132574: tablemetadatacache: fix TestTableMetadataUpdateJobProgressAndMetrics r=kyle-a-wong a=kyle-a-wong

This test has a high degree of variability in run time, likely due to the generation for so many tables. To fix, new tables
are no longer generated in the test. Instead, the batch size for the table metadata update job has is lowered to effectively trigger the same behavior

Epic: [CRDB-37558](https://cockroachlabs.atlassian.net/browse/CRDB-37558)
Release note: None

Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Kyle Wong <[email protected]>

pkg/kv/kvserver/client_raft_test.go

@@ -4084,6 +4084,69 @@ func TestLeaseHolderRemoveSelf(t *testing.T) {
  }
 }
 
+// TestVoterRemovalWithoutDemotion verifies that a voter replica cannot be
+// removed directly without first being demoted to a learner.
+func TestVoterRemovalWithoutDemotion(t *testing.T) {
+ defer leaktest.AfterTest(t)()
+ defer log.Scope(t).Close(t)
+
+ // Inject a filter which skips the demotion of a voter replica when removing
+ // it from the range. This will trigger the raft-level check which ensures
+ // that a voter replica cannot be removed directly.
+ type noDemotionKey struct{}
+ var removalTarget roachpb.ReplicationTarget
+ testingProposalFilter := func(args kvserverbase.ProposalFilterArgs) *kvpb.Error {
+ if args.Ctx.Value(noDemotionKey{}) != nil {
+ if state := args.Cmd.ReplicatedEvalResult.State; state != nil && state.Desc != nil {
+ repl, ok := state.Desc.GetReplicaDescriptor(removalTarget.StoreID)
+ if ok && repl.Type == roachpb.VOTER_DEMOTING_LEARNER {
+ t.Logf("intercepting proposal, skipping voter demotion: %+v", args.Cmd)
+ _, ok := state.Desc.RemoveReplica(repl.NodeID, repl.StoreID)
+ require.True(t, ok)
+ }
+ }
+ }
+ return nil
+ }
+
+ ctx := context.Background()
+ tc := testcluster.StartTestCluster(t, 2,
+ base.TestClusterArgs{
+ ReplicationMode: base.ReplicationManual,
+ ServerArgs: base.TestServerArgs{
+ Knobs: base.TestingKnobs{
+ Store: &kvserver.StoreTestingKnobs{
+ TestingProposalFilter: testingProposalFilter,
+ },
+ },
+ },
+ })
+ defer tc.Stopper().Stop(ctx)
+ removalTarget = tc.Target(1)
+
+ key := []byte("a")
+ tc.SplitRangeOrFatal(t, key)
+ tc.AddVotersOrFatal(t, key, removalTarget)
+
+ var beforeDesc roachpb.RangeDescriptor
+ db := tc.Servers[0].SystemLayer().DB()
+ require.NoError(t, db.GetProto(ctx, keys.RangeDescriptorKey(key), &beforeDesc))
+
+ // First attempt to remove the voter without demotion. Should fail.
+ expectedErr := "cannot remove voter .* directly; must first demote to learner"
+ noDemotionCtx := context.WithValue(ctx, noDemotionKey{}, struct{}{})
+ removeVoter := kvpb.MakeReplicationChanges(roachpb.REMOVE_VOTER, removalTarget)
+ _, err := db.AdminChangeReplicas(noDemotionCtx, key, beforeDesc, removeVoter)
+ require.Error(t, err)
+ require.Regexp(t, expectedErr, err)
+
+ // Now demote the voter to a learner and then remove it.
+ desc, err := db.AdminChangeReplicas(ctx, key, beforeDesc, removeVoter)
+ require.NoError(t, err)
+ _, ok := desc.GetReplicaDescriptor(removalTarget.StoreID)
+ require.False(t, ok)
+}
+
 // TestRemovedReplicaError verifies that a replica that has been removed from a
 // range returns a RangeNotFoundError if it receives a request for that range
 // (not RaftGroupDeletedError, and even before the ReplicaGCQueue has run).

pkg/kv/kvserver/replica_raft.go

@@ -528,6 +528,31 @@ func checkReplicationChangeAllowed(
  return err
  }
 
+ // Check against direct voter removal. Voters must first be demoted to
+ // learners before they can be removed for at least two reasons:
+ // 1. the leader (or any voter) may be needed to vote for a candidate who
+ // has not yet applied the configuration change. This is a liveness issue
+ // if the leader/voter is immediately removed without stepping down to a
+ // learner first and waiting for a second configuration change to
+ // succeed.
+ // For details, see: https://github.com/cockroachdb/cockroach/pull/42251.
+ // 2. the leader may have fortified its leadership term, binding the
+ // liveness of the leader replica to the leader's store's store liveness
+ // heartbeats. Removal of the leader replica from a store while that
+ // store continues to heartbeat in the store liveness fabric will lead to
+ // the leader disappearing without any other replica deciding that the
+ // leader is gone and stepping up to campaign.
+ //
+ // This same check exists in the pkg/raft library, but we disable it with
+ // DisableConfChangeValidation.
+ for _, repl := range desc.Replicas().Voters().Descriptors() {
+ if _, ok := proposedDesc.Replicas().GetReplicaDescriptorByID(repl.ReplicaID); !ok {
+ err := errors.Errorf("cannot remove voter %s directly; must first demote to learner", repl)
+ err = errors.Mark(err, errMarkInvalidReplicationChange)
+ return err
+ }
+ }
+
  return nil
 }
 

pkg/raft/confchange/BUILD.bazel

@@ -5,6 +5,7 @@ go_library(
  srcs = [
  "confchange.go",
  "restore.go",
+ "validate.go",
  ],
  importpath = "github.com/cockroachdb/cockroach/pkg/raft/confchange",
  visibility = ["//visibility:public"],
@@ -22,6 +23,7 @@ go_test(
  "datadriven_test.go",
  "quick_test.go",
  "restore_test.go",
+ "validate_test.go",
  ],
  data = glob(["testdata/**"]),
  embed = [":confchange"],
@@ -31,5 +33,6 @@ go_test(
  "//pkg/raft/tracker",
  "@com_github_cockroachdb_datadriven//:datadriven",
  "@com_github_cockroachdb_errors//:errors",
+ "@com_github_stretchr_testify//require",
  ],
 )

pkg/raft/confchange/validate.go

@@ -0,0 +1,83 @@
+// Copyright 2024 The Cockroach Authors.
+//
+// Use of this software is governed by the CockroachDB Software License
+// included in the /LICENSE file.
+
+package confchange
+
+import (
+ "github.com/cockroachdb/cockroach/pkg/raft/quorum"
+ pb "github.com/cockroachdb/cockroach/pkg/raft/raftpb"
+ "github.com/cockroachdb/errors"
+)
+
+// ValidationContext is a context struct that that is passed to ValidateProp to
+// allow it to validate a proposed ConfChange against the current state of the
+// Raft group.
+type ValidationContext struct {
+ CurConfig *quorum.Config
+ Applied uint64
+ PendingConfIndex uint64
+ DisableValidationAgainstCurConfig bool
+}
+
+// ValidateProp validates a proposed ConfChange against the current state of the
+// Raft group. It returns an error if the proposed change is invalid and must
+// not be proposed.
+func ValidateProp(ctx ValidationContext, cc pb.ConfChangeV2) error {
+ // Per the "Apply" invariant in the config change safety argument[^1],
+ // the leader must not append a config change if it hasn't applied all
+ // config changes in its log.
+ //
+ // [^1]: https://github.com/etcd-io/etcd/issues/7625#issuecomment-489232411
+ if ctx.PendingConfIndex > ctx.Applied {
+ return errors.Errorf("possible unapplied conf change at index %d (applied to %d)",
+ ctx.PendingConfIndex, ctx.Applied)
+ }
+
+ // The DisableValidationAgainstCurConfig flag allows disabling config change
+ // constraints that are guaranteed by the upper state machine layer (incorrect
+ // ones will apply as no-ops). See raft.Config.DisableConfChangeValidation for
+ // an explanation of why this may be used.
+ if !ctx.DisableValidationAgainstCurConfig {
+ curConfig := ctx.CurConfig
+
+ // Check that the proposed ConfChange is handling the joint state correctly.
+ // When in a joint state, the next config change must be to leave the joint
+ // state. When not in a joint state, the config change must not be asking us
+ // to leave the joint state.
+ alreadyJoint := len(curConfig.Voters[1]) > 0
+ wantsLeaveJoint := cc.LeaveJoint()
+ if alreadyJoint && !wantsLeaveJoint {
+ return errors.New("must transition out of joint config first")
+ } else if !alreadyJoint && wantsLeaveJoint {
+ return errors.New("not in joint state; refusing empty conf change")
+ }
+
+ // Check against direct voter removal. Voters must first be demoted to
+ // learners before they can be removed.
+ voterRemovals := make(map[pb.PeerID]struct{})
+ for _, cc := range cc.Changes {
+ switch cc.Type {
+ case pb.ConfChangeRemoveNode:
+ if _, ok := incoming(curConfig.Voters)[cc.NodeID]; ok {
+ voterRemovals[cc.NodeID] = struct{}{}
+ }
+ case pb.ConfChangeAddNode, pb.ConfChangeAddLearnerNode:
+ // If the node is being added back as a voter or learner, don't consider
+ // it a removal. Note that the order of the changes to the voterRemovals
+ // map is important here — we need the map manipulation to follow the
+ // slice order and mirror the handling of changes in Changer.apply.
+ delete(voterRemovals, cc.NodeID)
+ case pb.ConfChangeUpdateNode:
+ default:
+ return errors.Errorf("unexpected conf type %d", cc.Type)
+ }
+ }
+ if len(voterRemovals) > 0 {
+ return errors.New("voters must be demoted to learners before being removed")
+ }
+ }
+
+ return nil
+}