From 26a1e60eab6ededbf0f4b85739f8c4afed98fc5e Mon Sep 17 00:00:00 2001 From: Tobias Schottdorf Date: Wed, 17 Jul 2019 12:53:28 +0200 Subject: [PATCH 1/3] raft: return non-nil Inflights in raft status Recent refactoring to the String() method of `Progress` hit an NPE because we return nil Inflights as part of the Raft status. Just fix this at the source and properly populate the Raft status instead of teaching String() to ignore nil. A real Progress always has a non-nil Inflights. --- raft/rawnode_test.go | 22 +++++++++++++++++----- raft/status.go | 8 +++----- raft/tracker/inflights.go | 8 ++++++++ 3 files changed, 28 insertions(+), 10 deletions(-) diff --git a/raft/rawnode_test.go b/raft/rawnode_test.go index 425324f46..781e72844 100644 --- a/raft/rawnode_test.go +++ b/raft/rawnode_test.go @@ -439,14 +439,26 @@ func TestRawNodeRestartFromSnapshot(t *testing.T) { // no dependency check between Ready() and Advance() func TestRawNodeStatus(t *testing.T) { - storage := NewMemoryStorage() - rawNode, err := NewRawNode(newTestConfig(1, nil, 10, 1, storage), []Peer{{ID: 1}}) + s := NewMemoryStorage() + rn, err := NewRawNode(newTestConfig(1, []uint64{1}, 10, 1, s), nil) if err != nil { t.Fatal(err) } - status := rawNode.Status() - if status == nil { - t.Errorf("expected status struct, got nil") + if status := rn.Status(); status.Progress != nil { + t.Fatalf("expected no Progress because not leader: %+v", status.Progress) + } + if err := rn.Campaign(); err != nil { + t.Fatal(err) + } + status := rn.Status() + if status.Lead != 1 { + t.Fatal("not lead") + } + if status.RaftState != StateLeader { + t.Fatal("not leader") + } + if exp, act := *rn.raft.prs.Progress[1], status.Progress[1]; !reflect.DeepEqual(exp, act) { + t.Fatalf("want: %+v\ngot: %+v", exp, act) } } diff --git a/raft/status.go b/raft/status.go index bf4898c9e..79e589e8a 100644 --- a/raft/status.go +++ b/raft/status.go @@ -37,11 +37,9 @@ func getProgressCopy(r *raft) map[uint64]tracker.Progress { m := make(map[uint64]tracker.Progress) r.prs.Visit(func(id uint64, pr *tracker.Progress) { var p tracker.Progress - p, pr = *pr, nil /* avoid accidental reuse below */ - - // The inflight buffer is tricky to copy and besides, it isn't exposed - // to the client, so pretend it's nil. - p.Inflights = nil + p = *pr + p.Inflights = pr.Inflights.Clone() + pr = nil m[id] = p }) diff --git a/raft/tracker/inflights.go b/raft/tracker/inflights.go index 9e209e21e..1a056341a 100644 --- a/raft/tracker/inflights.go +++ b/raft/tracker/inflights.go @@ -40,6 +40,14 @@ func NewInflights(size int) *Inflights { } } +// Clone returns an *Inflights that is identical to but shares no memory with +// the receiver. +func (in *Inflights) Clone() *Inflights { + ins := *in + ins.buffer = append([]uint64(nil), in.buffer...) + return &ins +} + // Add notifies the Inflights that a new message with the given index is being // dispatched. Full() must be called prior to Add() to verify that there is room // for one more message, and consecutive calls to add Add() must provide a From 7ce934cbec3945ae8f7c38f866304e2db518171c Mon Sep 17 00:00:00 2001 From: Tobias Schottdorf Date: Wed, 17 Jul 2019 14:29:45 +0200 Subject: [PATCH 2/3] raft: return active config in Status This is useful for debug purposes, and more so once we support joint quorums. --- raft/rawnode_test.go | 8 ++++++++ raft/status.go | 2 ++ 2 files changed, 10 insertions(+) diff --git a/raft/rawnode_test.go b/raft/rawnode_test.go index 781e72844..bfb67bd5d 100644 --- a/raft/rawnode_test.go +++ b/raft/rawnode_test.go @@ -21,6 +21,7 @@ import ( "reflect" "testing" + "go.etcd.io/etcd/raft/quorum" "go.etcd.io/etcd/raft/raftpb" "go.etcd.io/etcd/raft/tracker" ) @@ -460,6 +461,13 @@ func TestRawNodeStatus(t *testing.T) { if exp, act := *rn.raft.prs.Progress[1], status.Progress[1]; !reflect.DeepEqual(exp, act) { t.Fatalf("want: %+v\ngot: %+v", exp, act) } + expCfg := tracker.Config{Voters: quorum.JointConfig{ + quorum.MajorityConfig{1: {}}, + nil, + }} + if !reflect.DeepEqual(expCfg, status.Config) { + t.Fatalf("want: %+v\ngot: %+v", expCfg, status.Config) + } } // TestRawNodeCommitPaginationAfterRestart is the RawNode version of diff --git a/raft/status.go b/raft/status.go index 79e589e8a..7002da535 100644 --- a/raft/status.go +++ b/raft/status.go @@ -28,6 +28,7 @@ type Status struct { SoftState Applied uint64 + Config tracker.Config Progress map[uint64]tracker.Progress LeadTransferee uint64 @@ -54,6 +55,7 @@ func getStatusWithoutProgress(r *raft) Status { s.HardState = r.hardState() s.SoftState = *r.softState() s.Applied = r.raftLog.applied + s.Config = r.prs.Config.Clone() return s } From 6b0322549f54102a9faf43cdb358f6e8e418da1d Mon Sep 17 00:00:00 2001 From: Tobias Schottdorf Date: Thu, 18 Jul 2019 16:28:37 +0200 Subject: [PATCH 3/3] raft: replace StatusWithoutProgress with BasicStatus Now that a Config is also added to the full status, the old name did not convey the intention, which was to get a Status without an associated allocation. --- raft/rawnode.go | 17 ++++++++--------- raft/rawnode_test.go | 10 ++++------ raft/status.go | 22 +++++++++++++++------- 3 files changed, 27 insertions(+), 22 deletions(-) diff --git a/raft/rawnode.go b/raft/rawnode.go index 77183b793..c2bc7c132 100644 --- a/raft/rawnode.go +++ b/raft/rawnode.go @@ -218,18 +218,17 @@ func (rn *RawNode) Advance(rd Ready) { rn.commitReady(rd) } -// Status returns the current status of the given group. -func (rn *RawNode) Status() *Status { +// Status returns the current status of the given group. This allocates, see +// BasicStatus and WithProgress for allocation-friendlier choices. +func (rn *RawNode) Status() Status { status := getStatus(rn.raft) - return &status + return status } -// StatusWithoutProgress returns a Status without populating the Progress field -// (and returns the Status as a value to avoid forcing it onto the heap). This -// is more performant if the Progress is not required. See WithProgress for an -// allocation-free way to introspect the Progress. -func (rn *RawNode) StatusWithoutProgress() Status { - return getStatusWithoutProgress(rn.raft) +// BasicStatus returns a BasicStatus. Notably this does not contain the +// Progress map; see WithProgress for an allocation-free way to inspect it. +func (rn *RawNode) BasicStatus() BasicStatus { + return getBasicStatus(rn.raft) } // ProgressType indicates the type of replica a Progress corresponds to. diff --git a/raft/rawnode_test.go b/raft/rawnode_test.go index bfb67bd5d..3a6c392cc 100644 --- a/raft/rawnode_test.go +++ b/raft/rawnode_test.go @@ -44,7 +44,7 @@ func (a *rawNodeAdapter) TransferLeadership(ctx context.Context, lead, transfere func (a *rawNodeAdapter) Stop() {} // RawNode returns a *Status. -func (a *rawNodeAdapter) Status() Status { return *a.RawNode.Status() } +func (a *rawNodeAdapter) Status() Status { return a.RawNode.Status() } // RawNode takes a Ready. It doesn't really have to do that I think? It can hold on // to it internally. But maybe that approach is frail. @@ -610,7 +610,7 @@ func TestRawNodeBoundedLogGrowthWithPartition(t *testing.T) { checkUncommitted(0) } -func BenchmarkStatusProgress(b *testing.B) { +func BenchmarkStatus(b *testing.B) { setup := func(members int) *RawNode { peers := make([]uint64, members) for i := range peers { @@ -627,8 +627,6 @@ func BenchmarkStatusProgress(b *testing.B) { for _, members := range []int{1, 3, 5, 100} { b.Run(fmt.Sprintf("members=%d", members), func(b *testing.B) { - // NB: call getStatus through rn.Status because that incurs an additional - // allocation. rn := setup(members) b.Run("Status", func(b *testing.B) { @@ -650,10 +648,10 @@ func BenchmarkStatusProgress(b *testing.B) { } }) - b.Run("StatusWithoutProgress", func(b *testing.B) { + b.Run("BasicStatus", func(b *testing.B) { b.ReportAllocs() for i := 0; i < b.N; i++ { - _ = rn.StatusWithoutProgress() + _ = rn.BasicStatus() } }) diff --git a/raft/status.go b/raft/status.go index 7002da535..adc60486d 100644 --- a/raft/status.go +++ b/raft/status.go @@ -21,15 +21,22 @@ import ( "go.etcd.io/etcd/raft/tracker" ) +// Status contains information about this Raft peer and its view of the system. +// The Progress is only populated on the leader. type Status struct { + BasicStatus + Config tracker.Config + Progress map[uint64]tracker.Progress +} + +// BasicStatus contains basic information about the Raft peer. It does not allocate. +type BasicStatus struct { ID uint64 pb.HardState SoftState - Applied uint64 - Config tracker.Config - Progress map[uint64]tracker.Progress + Applied uint64 LeadTransferee uint64 } @@ -47,24 +54,25 @@ func getProgressCopy(r *raft) map[uint64]tracker.Progress { return m } -func getStatusWithoutProgress(r *raft) Status { - s := Status{ +func getBasicStatus(r *raft) BasicStatus { + s := BasicStatus{ ID: r.id, LeadTransferee: r.leadTransferee, } s.HardState = r.hardState() s.SoftState = *r.softState() s.Applied = r.raftLog.applied - s.Config = r.prs.Config.Clone() return s } // getStatus gets a copy of the current raft status. func getStatus(r *raft) Status { - s := getStatusWithoutProgress(r) + var s Status + s.BasicStatus = getBasicStatus(r) if s.RaftState == StateLeader { s.Progress = getProgressCopy(r) } + s.Config = r.prs.Config.Clone() return s }