raft: move ProgressTracker construction into switchToConfig

Previously, we were reaching into the ProgressTracker to set the
ProgressMap when switching configurations. This patch makes the
ProgressMap an argument to the constructor for the ProgressTracker.
We then construct and assign a new ProgressTracker when switching
configurations. This makes more sense in a world where the
ProgressTracker doesn't need to hold on to state between configuration
changes.

Informs #125265

Release note: None

pkg/kv/kvserver/client_atomic_membership_change_test.go

@@ -89,7 +89,7 @@ func TestAtomicReplicationChange(t *testing.T) {
 				// a hair earlier.
 				cfg := quorum.MakeEmptyConfig()
 				cfg, _, err = confchange.Restore(confchange.Changer{
-					ProgressMap:      tracker.MakeProgressTracker(&cfg).Progress,
+					ProgressMap:      tracker.MakeEmptyProgressMap(),
 					Config:           cfg,
 					MaxInflight:      1,
 					MaxInflightBytes: 0,

pkg/raft/confchange/datadriven_test.go

@@ -32,10 +32,9 @@ import (
 
 func TestConfChangeDataDriven(t *testing.T) {
 	datadriven.Walk(t, "testdata", func(t *testing.T, path string) {
-		cfg := quorum.MakeEmptyConfig()
 		c := Changer{
-			Config:           cfg,
-			ProgressMap:      tracker.MakeProgressTracker(&cfg).Progress,
+			Config:           quorum.MakeEmptyConfig(),
+			ProgressMap:      tracker.MakeEmptyProgressMap(),
 			MaxInflight:      10,
 			MaxInflightBytes: 0,
 			LastIndex:        0, // incremented in this test with each cmd

pkg/raft/confchange/quick_test.go

@@ -93,10 +93,9 @@ func TestConfChangeQuick(t *testing.T) {
 
 	wrapper := func(invoke testFunc) func(setup initialChanges, ccs confChanges) (*Changer, error) {
 		return func(setup initialChanges, ccs confChanges) (*Changer, error) {
-			cfg := quorum.MakeEmptyConfig()
 			c := &Changer{
-				Config:           cfg,
-				ProgressMap:      tracker.MakeProgressTracker(&cfg).Progress,
+				Config:           quorum.MakeEmptyConfig(),
+				ProgressMap:      tracker.MakeEmptyProgressMap(),
 				MaxInflight:      10,
 				MaxInflightBytes: 0,
 				LastIndex:        10,

pkg/raft/confchange/restore_test.go

@@ -89,10 +89,9 @@ func TestRestore(t *testing.T) {
 	cfg := quick.Config{MaxCount: 1000}
 
 	f := func(cs pb.ConfState) bool {
-		cfg := quorum.MakeEmptyConfig()
 		chg := Changer{
-			Config:      cfg,
-			ProgressMap: tracker.MakeProgressTracker(&cfg).Progress,
+			Config:      quorum.MakeEmptyConfig(),
+			ProgressMap: tracker.MakeEmptyProgressMap(),
 			LastIndex:   10,
 		}
 		cfg, progressMap, err := Restore(chg, cs)

pkg/raft/raft.go

@@ -420,7 +420,6 @@ func newRaft(c *Config) *raft {
 		raftLog:                     raftlog,
 		maxMsgSize:                  entryEncodingSize(c.MaxSizePerMsg),
 		maxUncommittedSize:          entryPayloadSize(c.MaxUncommittedEntriesSize),
-		config:                      quorum.MakeEmptyConfig(),
 		electionTimeout:             c.ElectionTick,
 		heartbeatTimeout:            c.HeartbeatTick,
 		logger:                      c.Logger,
@@ -435,19 +434,17 @@ func newRaft(c *Config) *raft {
 	}
 	lastID := r.raftLog.lastEntryID()
 
-	r.trk = tracker.MakeProgressTracker(&r.config)
-
-	cfg, trk, err := confchange.Restore(confchange.Changer{
-		Config:           r.config,
-		ProgressMap:      r.trk.Progress,
+	cfg, progressMap, err := confchange.Restore(confchange.Changer{
+		Config:           quorum.MakeEmptyConfig(),
+		ProgressMap:      tracker.MakeEmptyProgressMap(),
 		MaxInflight:      r.maxInflight,
 		MaxInflightBytes: r.maxInflightBytes,
 		LastIndex:        lastID.index,
 	}, cs)
 	if err != nil {
 		panic(err)
 	}
-	assertConfStatesEquivalent(r.logger, cs, r.switchToConfig(cfg, trk))
+	assertConfStatesEquivalent(r.logger, cs, r.switchToConfig(cfg, progressMap))
 
 	if !IsEmptyHardState(hs) {
 		r.loadState(hs)
@@ -1908,12 +1905,10 @@ func (r *raft) restore(s snapshot) bool {
 		return false
 	}
 
-	// Reset the configuration and add the (potentially updated) peers in anew.
-	r.config = quorum.MakeEmptyConfig()
-	r.trk = tracker.MakeProgressTracker(&r.config)
-	cfg, trk, err := confchange.Restore(confchange.Changer{
-		Config:           r.config,
-		ProgressMap:      r.trk.Progress,
+	cfg, progressMap, err := confchange.Restore(confchange.Changer{
+		// Reset the configuration and add the (potentially updated) peers in anew.
+		Config:           quorum.MakeEmptyConfig(),
+		ProgressMap:      tracker.MakeEmptyProgressMap(), // empty ProgressMap to go with our empty config
 		MaxInflight:      r.maxInflight,
 		MaxInflightBytes: r.maxInflightBytes,
 		LastIndex:        r.raftLog.lastIndex(),
@@ -1925,7 +1920,7 @@ func (r *raft) restore(s snapshot) bool {
 		panic(fmt.Sprintf("unable to restore config %+v: %s", cs, err))
 	}
 
-	assertConfStatesEquivalent(r.logger, cs, r.switchToConfig(cfg, trk))
+	assertConfStatesEquivalent(r.logger, cs, r.switchToConfig(cfg, progressMap))
 
 	last := r.raftLog.lastEntryID()
 	r.logger.Infof("%x [commit: %d, lastindex: %d, lastterm: %d] restored snapshot [index: %d, term: %d]",
@@ -1941,7 +1936,7 @@ func (r *raft) promotable() bool {
 }
 
 func (r *raft) applyConfChange(cc pb.ConfChangeV2) pb.ConfState {
-	cfg, trk, err := func() (quorum.Config, tracker.ProgressMap, error) {
+	cfg, progressMap, err := func() (quorum.Config, tracker.ProgressMap, error) {
 		changer := confchange.Changer{
 			Config:           r.config,
 			ProgressMap:      r.trk.Progress,
@@ -1962,7 +1957,7 @@ func (r *raft) applyConfChange(cc pb.ConfChangeV2) pb.ConfState {
 		panic(err)
 	}
 
-	return r.switchToConfig(cfg, trk)
+	return r.switchToConfig(cfg, progressMap)
 }
 
 // switchToConfig reconfigures this node to use the provided configuration. It
@@ -1971,9 +1966,9 @@ func (r *raft) applyConfChange(cc pb.ConfChangeV2) pb.ConfState {
 // requirements.
 //
 // The inputs usually result from restoring a ConfState or applying a ConfChange.
-func (r *raft) switchToConfig(cfg quorum.Config, trk tracker.ProgressMap) pb.ConfState {
+func (r *raft) switchToConfig(cfg quorum.Config, progressMap tracker.ProgressMap) pb.ConfState {
 	r.config = cfg
-	r.trk.Progress = trk
+	r.trk = tracker.MakeProgressTracker(&r.config, progressMap)
 
 	r.logger.Infof("%x switched to configuration %s", r.id, r.config)
 	cs := r.config.ConfState()

pkg/raft/raft_test.go

@@ -3750,7 +3750,7 @@ func newNetworkWithConfig(configFunc func(*Config), peers ...stateMachine) *netw
 				learners[i] = true
 			}
 			v.id = id
-			v.trk = tracker.MakeProgressTracker(&v.config)
+			v.trk = tracker.MakeProgressTracker(&v.config, tracker.MakeEmptyProgressMap())
 			if len(learners) > 0 {
 				v.config.Learners = map[pb.PeerID]struct{}{}
 			}

pkg/raft/tracker/progress.go

@@ -356,6 +356,11 @@ func (pr *Progress) String() string {
 // ProgressMap is a map of *Progress.
 type ProgressMap map[pb.PeerID]*Progress
 
+// MakeEmptyProgressMap constructs and returns an empty ProgressMap.
+func MakeEmptyProgressMap() ProgressMap {
+	return make(ProgressMap)
+}
+
 // String prints the ProgressMap in sorted key order, one Progress per line.
 func (m ProgressMap) String() string {
 	ids := make([]pb.PeerID, 0, len(m))

pkg/raft/tracker/tracker.go

@@ -37,11 +37,11 @@ type ProgressTracker struct {
 }
 
 // MakeProgressTracker initializes a ProgressTracker.
-func MakeProgressTracker(config *quorum.Config) ProgressTracker {
+func MakeProgressTracker(config *quorum.Config, progressMap ProgressMap) ProgressTracker {
 	p := ProgressTracker{
 		Config:   config,
+		Progress: progressMap,
 		Votes:    map[pb.PeerID]bool{},
-		Progress: map[pb.PeerID]*Progress{},
 	}
 	return p
 }

pkg/roachpb/metadata_replicas_test.go

@@ -337,11 +337,10 @@ func TestReplicaDescriptorsCanMakeProgressRandom(t *testing.T) {
 		})
 
 		raftCanMakeProgress, skip := func() (res bool, skip bool) {
-			emptyCfg := quorum.MakeEmptyConfig()
 			cfg, _, err := confchange.Restore(
 				confchange.Changer{
-					Config:           emptyCfg,
-					ProgressMap:      tracker.MakeProgressTracker(&emptyCfg).Progress,
+					Config:           quorum.MakeEmptyConfig(),
+					ProgressMap:      tracker.MakeEmptyProgressMap(),
 					MaxInflight:      1,
 					MaxInflightBytes: 0,
 				},