From 6268e3776023d879beed39b9d704467856cb1997 Mon Sep 17 00:00:00 2001 From: Arul Ajmani Date: Thu, 18 Jul 2024 19:27:23 -0400 Subject: [PATCH] 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 https://github.com/cockroachdb/cockroach/issues/125265 Release note: None --- .../client_atomic_membership_change_test.go | 2 +- pkg/raft/confchange/datadriven_test.go | 5 ++- pkg/raft/confchange/quick_test.go | 5 ++- pkg/raft/confchange/restore_test.go | 5 ++- pkg/raft/raft.go | 31 ++++++++----------- pkg/raft/raft_test.go | 2 +- pkg/raft/tracker/progress.go | 5 +++ pkg/raft/tracker/tracker.go | 4 +-- pkg/roachpb/metadata_replicas_test.go | 5 ++- 9 files changed, 30 insertions(+), 34 deletions(-) diff --git a/pkg/kv/kvserver/client_atomic_membership_change_test.go b/pkg/kv/kvserver/client_atomic_membership_change_test.go index 6410ec78ef3f..ba0ce5bc6700 100644 --- a/pkg/kv/kvserver/client_atomic_membership_change_test.go +++ b/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, diff --git a/pkg/raft/confchange/datadriven_test.go b/pkg/raft/confchange/datadriven_test.go index 9bf2cd0d151a..2aeef89f6200 100644 --- a/pkg/raft/confchange/datadriven_test.go +++ b/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 diff --git a/pkg/raft/confchange/quick_test.go b/pkg/raft/confchange/quick_test.go index 72ca0b4aea28..30e0ea496dbc 100644 --- a/pkg/raft/confchange/quick_test.go +++ b/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, diff --git a/pkg/raft/confchange/restore_test.go b/pkg/raft/confchange/restore_test.go index 58e4dbc30ceb..0114a787867d 100644 --- a/pkg/raft/confchange/restore_test.go +++ b/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) diff --git a/pkg/raft/raft.go b/pkg/raft/raft.go index 348c8ba1311c..bf6555177b41 100644 --- a/pkg/raft/raft.go +++ b/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,11 +434,9 @@ 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, @@ -447,7 +444,7 @@ func newRaft(c *Config) *raft { 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() diff --git a/pkg/raft/raft_test.go b/pkg/raft/raft_test.go index f1ab34cc93bd..b4e30cf718f8 100644 --- a/pkg/raft/raft_test.go +++ b/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{}{} } diff --git a/pkg/raft/tracker/progress.go b/pkg/raft/tracker/progress.go index 0d35738d8ce8..bb0c896789ca 100644 --- a/pkg/raft/tracker/progress.go +++ b/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)) diff --git a/pkg/raft/tracker/tracker.go b/pkg/raft/tracker/tracker.go index 4393e65a2f06..f21e769fbd5f 100644 --- a/pkg/raft/tracker/tracker.go +++ b/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 } diff --git a/pkg/roachpb/metadata_replicas_test.go b/pkg/roachpb/metadata_replicas_test.go index 2a950629fe7d..479766fcbf3b 100644 --- a/pkg/roachpb/metadata_replicas_test.go +++ b/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, },