Merge cockroachdb#133203

133203: raft: only call `SupportFrom` on voters in current config r=nvanbenschoten a=nvanbenschoten

Informs cockroachdb#125264.

This commit updates the raft `FortificationTracker` to only call `StoreLiveness.SupportFrom` on voters in the current configuration. This avoids unnecessary calls to the `StoreLiveness` subsystem for non-voters, which can be expensive. It also avoids calling `SupportFrom` on removed voters, which as of cockroachdb#133199, can lead to log warnings.

Release note: None

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

pkg/raft/BUILD.bazel

@@ -27,6 +27,7 @@ go_library(
         "//pkg/raft/tracker",
         "//pkg/util/hlc",
         "@com_github_cockroachdb_errors//:errors",
+        "@com_github_cockroachdb_redact//:redact",
         "@org_golang_x_exp//maps",
     ],
 )

pkg/raft/quorum/joint.go

@@ -45,6 +45,20 @@ func (c JointConfig) IDs() map[pb.PeerID]struct{} {
 	return m
 }
 
+// Visit calls the given function for each unique voter ID in the joint
+// configuration.
+func (c JointConfig) Visit(f func(pb.PeerID)) {
+	for id := range c[0] {
+		f(id)
+	}
+	for id := range c[1] {
+		if _, ok := c[0][id]; ok {
+			continue // skip duplicate
+		}
+		f(id)
+	}
+}
+
 // Describe returns a (multi-line) representation of the commit indexes for the
 // given lookuper.
 func (c JointConfig) Describe(l AckedIndexer) string {

pkg/raft/quorum/quorum_test.go

@@ -6,6 +6,7 @@
 package quorum
 
 import (
+	"slices"
 	"testing"
 
 	pb "github.com/cockroachdb/cockroach/pkg/raft/raftpb"
@@ -134,3 +135,21 @@ func TestLeadSupportExpirationJointConfig(t *testing.T) {
 		require.Equal(t, tc.exp, j.LeadSupportExpiration(tc.support))
 	}
 }
+
+func TestJointConfigVisit(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	j := JointConfig{
+		MajorityConfig{1: struct{}{}, 2: struct{}{}, 3: struct{}{}},
+		MajorityConfig{2: struct{}{}, 3: struct{}{}, 4: struct{}{}},
+	}
+
+	var visited []pb.PeerID
+	j.Visit(func(id pb.PeerID) {
+		visited = append(visited, id)
+	})
+	slices.Sort(visited)
+
+	require.Equal(t, []pb.PeerID{1, 2, 3, 4}, visited)
+}

pkg/raft/raft.go

@@ -36,6 +36,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/raft/raftstoreliveness"
 	"github.com/cockroachdb/cockroach/pkg/raft/tracker"
 	"github.com/cockroachdb/errors"
+	"github.com/cockroachdb/redact"
 	"golang.org/x/exp/maps"
 )
 
@@ -1440,14 +1441,14 @@ func (r *raft) Step(m pb.Message) error {
 				// leader it does not update its term or grant its vote.
 				{
 					// Log why we're ignoring the Request{,Pre}Vote.
-					var inHeartbeatLeaseMsg string
-					var inFortifyLeaseMsg string
-					var sep string
+					var inHeartbeatLeaseMsg redact.RedactableString
+					var inFortifyLeaseMsg redact.RedactableString
+					var sep redact.SafeString
 					if inHeartbeatLease {
-						inHeartbeatLeaseMsg = fmt.Sprintf("recently received communication from leader (remaining ticks: %d)", r.electionTimeout-r.electionElapsed)
+						inHeartbeatLeaseMsg = redact.Sprintf("recently received communication from leader (remaining ticks: %d)", r.electionTimeout-r.electionElapsed)
 					}
 					if inFortifyLease {
-						inFortifyLeaseMsg = fmt.Sprintf("supporting fortified leader %d at epoch %d", r.lead, r.leadEpoch)
+						inFortifyLeaseMsg = redact.Sprintf("supporting fortified leader %d at epoch %d", r.lead, r.leadEpoch)
 					}
 					if inFortifyLease && inHeartbeatLease {
 						sep = " and "

pkg/raft/tracker/fortificationtracker.go

@@ -164,26 +164,31 @@ func (ft *FortificationTracker) computeLeadSupportUntil(state pb.StateType) hlc.
 	if state != pb.StateLeader {
 		panic("computeLeadSupportUntil should only be called by the leader")
 	}
+	if len(ft.fortification) == 0 {
+		return hlc.Timestamp{} // fast-path for no fortification
+	}
 
 	// TODO(arul): avoid this map allocation as we're calling LeadSupportUntil
 	// from hot paths.
 	supportExpMap := make(map[pb.PeerID]hlc.Timestamp)
-	for id, supportEpoch := range ft.fortification {
-		curEpoch, curExp := ft.storeLiveness.SupportFrom(id)
-		// NB: We can't assert that supportEpoch <= curEpoch because there may be a
-		// race between a successful MsgFortifyLeaderResp and the store liveness
-		// heartbeat response that lets the leader know the follower's store is
-		// supporting the leader's store at the epoch in the MsgFortifyLeaderResp
-		// message.
-		if curEpoch == supportEpoch {
-			supportExpMap[id] = curExp
+	ft.config.Voters.Visit(func(id pb.PeerID) {
+		if supportEpoch, ok := ft.fortification[id]; ok {
+			curEpoch, curExp := ft.storeLiveness.SupportFrom(id)
+			// NB: We can't assert that supportEpoch <= curEpoch because there may be
+			// a race between a successful MsgFortifyLeaderResp and the store liveness
+			// heartbeat response that lets the leader know the follower's store is
+			// supporting the leader's store at the epoch in the MsgFortifyLeaderResp
+			// message.
+			if curEpoch == supportEpoch {
+				supportExpMap[id] = curExp
+			}
 		}
-	}
+	})
 	return ft.config.Voters.LeadSupportExpiration(supportExpMap)
 }
 
 // CanDefortify returns whether the caller can safely[1] de-fortify the term
-// based on the sate tracked by the FortificationTracker.
+// based on the state tracked by the FortificationTracker.
 //
 // [1] Without risking regressions in the maximum that's ever been indicated to
 // the layers above. Or, more simply, without risking regression of leader