From c05180010143bd3983e547754f81e2413391c870 Mon Sep 17 00:00:00 2001 From: Benjamin Wang Date: Sat, 25 Nov 2023 15:06:35 +0000 Subject: [PATCH 1/4] stop writing v2store We already generate v2 snapshot using v3store to be compatible with 3.5. It makes no sense to write any data into v2store, which isn't used at all. Signed-off-by: Benjamin Wang --- server/etcdserver/api/membership/cluster.go | 26 --------------------- 1 file changed, 26 deletions(-) diff --git a/server/etcdserver/api/membership/cluster.go b/server/etcdserver/api/membership/cluster.go index f022d29c44e..d28efb09781 100644 --- a/server/etcdserver/api/membership/cluster.go +++ b/server/etcdserver/api/membership/cluster.go @@ -263,9 +263,6 @@ func (c *RaftCluster) Recover(onSet func(*zap.Logger, *semver.Version)) { if c.be != nil { c.version = c.be.ClusterVersionFromBackend() c.members, c.removed = c.be.MustReadMembersFromBackend() - } else { - c.version = clusterVersionFromStore(c.lg, c.v2store) - c.members, c.removed = membersFromStore(c.lg, c.v2store) } c.buildMembershipMetric() @@ -390,9 +387,6 @@ func (c *RaftCluster) ValidateConfigurationChange(cc raftpb.ConfChange) error { func (c *RaftCluster) AddMember(m *Member, shouldApplyV3 ShouldApplyV3) { c.Lock() defer c.Unlock() - if c.v2store != nil { - mustSaveMemberToStore(c.lg, c.v2store, m) - } if c.be != nil && shouldApplyV3 { c.be.MustSaveMemberToBackend(m) @@ -423,9 +417,6 @@ func (c *RaftCluster) AddMember(m *Member, shouldApplyV3 ShouldApplyV3) { func (c *RaftCluster) RemoveMember(id types.ID, shouldApplyV3 ShouldApplyV3) { c.Lock() defer c.Unlock() - if c.v2store != nil { - mustDeleteMemberFromStore(c.lg, c.v2store, id) - } if c.be != nil && shouldApplyV3 { c.be.MustDeleteMemberFromBackend(id) @@ -467,9 +458,6 @@ func (c *RaftCluster) UpdateAttributes(id types.ID, attr Attributes, shouldApply if m, ok := c.members[id]; ok { m.Attributes = attr - if c.v2store != nil { - mustUpdateMemberAttrInStore(c.lg, c.v2store, m) - } if c.be != nil && shouldApplyV3 { c.be.MustSaveMemberToBackend(m) } @@ -499,12 +487,6 @@ func (c *RaftCluster) PromoteMember(id types.ID, shouldApplyV3 ShouldApplyV3) { c.Lock() defer c.Unlock() - if c.v2store != nil { - m := *(c.members[id]) - m.RaftAttributes.IsLearner = false - mustUpdateMemberInStore(c.lg, c.v2store, &m) - } - if c.be != nil && shouldApplyV3 { c.members[id].RaftAttributes.IsLearner = false c.updateMembershipMetric(id, true) @@ -528,11 +510,6 @@ func (c *RaftCluster) UpdateRaftAttributes(id types.ID, raftAttr RaftAttributes, c.Lock() defer c.Unlock() - if c.v2store != nil { - m := *(c.members[id]) - m.RaftAttributes = raftAttr - mustUpdateMemberInStore(c.lg, c.v2store, &m) - } if c.be != nil && shouldApplyV3 { c.members[id].RaftAttributes = raftAttr c.be.MustSaveMemberToBackend(c.members[id]) @@ -589,9 +566,6 @@ func (c *RaftCluster) SetVersion(ver *semver.Version, onSet func(*zap.Logger, *s c.version = ver sv := semver.Must(semver.NewVersion(version.Version)) serverversion.MustDetectDowngrade(c.lg, sv, c.version) - if c.v2store != nil { - mustSaveClusterVersionToStore(c.lg, c.v2store, ver) - } if c.be != nil && shouldApplyV3 { c.be.MustSaveClusterVersionToBackend(ver) } From 00870a74fdbc5511d7720ce42b89941c301fd443 Mon Sep 17 00:00:00 2001 From: Benjamin Wang Date: Sat, 25 Nov 2023 15:14:16 +0000 Subject: [PATCH 2/4] validate conf change against v3store instead of v2store Signed-off-by: Benjamin Wang --- server/etcdserver/api/membership/cluster.go | 10 +++++++--- server/etcdserver/api/membership/cluster_test.go | 4 +++- server/etcdserver/server.go | 2 +- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/server/etcdserver/api/membership/cluster.go b/server/etcdserver/api/membership/cluster.go index d28efb09781..87dc170dc70 100644 --- a/server/etcdserver/api/membership/cluster.go +++ b/server/etcdserver/api/membership/cluster.go @@ -300,9 +300,13 @@ func (c *RaftCluster) Recover(onSet func(*zap.Logger, *semver.Version)) { // ValidateConfigurationChange takes a proposed ConfChange and // ensures that it is still valid. -func (c *RaftCluster) ValidateConfigurationChange(cc raftpb.ConfChange) error { - // TODO: this must be switched to backend as well. - membersMap, removedMap := membersFromStore(c.lg, c.v2store) +func (c *RaftCluster) ValidateConfigurationChange(cc raftpb.ConfChange, shouldApplyV3 ShouldApplyV3) error { + // It makes no sense to validate a confChange if we don't apply it. + if !shouldApplyV3 { + return nil + } + membersMap, removedMap := c.be.MustReadMembersFromBackend() + id := types.ID(cc.NodeID) if removedMap[id] { return ErrIDRemoved diff --git a/server/etcdserver/api/membership/cluster_test.go b/server/etcdserver/api/membership/cluster_test.go index b06d3302475..62f188e3e74 100644 --- a/server/etcdserver/api/membership/cluster_test.go +++ b/server/etcdserver/api/membership/cluster_test.go @@ -278,6 +278,8 @@ func TestClusterValidateAndAssignIDs(t *testing.T) { func TestClusterValidateConfigurationChange(t *testing.T) { cl := NewCluster(zaptest.NewLogger(t), WithMaxLearners(1)) + be := newMembershipBackend() + cl.SetBackend(be) cl.SetStore(v2store.New()) for i := 1; i <= 4; i++ { var isLearner bool @@ -455,7 +457,7 @@ func TestClusterValidateConfigurationChange(t *testing.T) { }, } for i, tt := range tests { - err := cl.ValidateConfigurationChange(tt.cc) + err := cl.ValidateConfigurationChange(tt.cc, true) if err != tt.werr { t.Errorf("#%d: validateConfigurationChange error = %v, want %v", i, err, tt.werr) } diff --git a/server/etcdserver/server.go b/server/etcdserver/server.go index 4ad4539c6e2..a4a8161c208 100644 --- a/server/etcdserver/server.go +++ b/server/etcdserver/server.go @@ -1935,7 +1935,7 @@ func removeNeedlessRangeReqs(txn *pb.TxnRequest) { // applyConfChange applies a ConfChange to the server. It is only // invoked with a ConfChange that has already passed through Raft func (s *EtcdServer) applyConfChange(cc raftpb.ConfChange, confState *raftpb.ConfState, shouldApplyV3 membership.ShouldApplyV3) (bool, error) { - if err := s.cluster.ValidateConfigurationChange(cc); err != nil { + if err := s.cluster.ValidateConfigurationChange(cc, shouldApplyV3); err != nil { cc.NodeID = raft.None s.r.ApplyConfChange(cc) From b56f91c4e0a3b0dc62ae967ba6936f1fe5604ea9 Mon Sep 17 00:00:00 2001 From: Benjamin Wang Date: Sat, 25 Nov 2023 15:26:43 +0000 Subject: [PATCH 3/4] remove unused functions Signed-off-by: Benjamin Wang --- server/etcdserver/api/membership/storev2.go | 42 --------------------- 1 file changed, 42 deletions(-) diff --git a/server/etcdserver/api/membership/storev2.go b/server/etcdserver/api/membership/storev2.go index 4a1e702aa58..438ce766232 100644 --- a/server/etcdserver/api/membership/storev2.go +++ b/server/etcdserver/api/membership/storev2.go @@ -78,18 +78,6 @@ func mustSaveMemberToStore(lg *zap.Logger, s v2store.Store, m *Member) { } } -func mustDeleteMemberFromStore(lg *zap.Logger, s v2store.Store, id types.ID) { - if _, err := s.Delete(MemberStoreKey(id), true, true); err != nil { - lg.Panic( - "failed to delete member from store", - zap.String("path", MemberStoreKey(id)), - zap.Error(err), - ) - } - - mustAddToRemovedMembersInStore(lg, s, id) -} - func mustAddToRemovedMembersInStore(lg *zap.Logger, s v2store.Store, id types.ID) { if _, err := s.Create(RemovedMemberStoreKey(id), false, "", false, v2store.TTLOptionSet{ExpireTime: v2store.Permanent}); err != nil { @@ -101,21 +89,6 @@ func mustAddToRemovedMembersInStore(lg *zap.Logger, s v2store.Store, id types.ID } } -func mustUpdateMemberInStore(lg *zap.Logger, s v2store.Store, m *Member) { - b, err := json.Marshal(m.RaftAttributes) - if err != nil { - lg.Panic("failed to marshal raftAttributes", zap.Error(err)) - } - p := path.Join(MemberStoreKey(m.ID), raftAttributesSuffix) - if _, err := s.Update(p, string(b), v2store.TTLOptionSet{ExpireTime: v2store.Permanent}); err != nil { - lg.Panic( - "failed to update raftAttributes", - zap.String("path", p), - zap.Error(err), - ) - } -} - func mustUpdateMemberAttrInStore(lg *zap.Logger, s v2store.Store, m *Member) { b, err := json.Marshal(m.Attributes) if err != nil { @@ -184,18 +157,3 @@ func MemberStoreKey(id types.ID) string { func MemberAttributesStorePath(id types.ID) string { return path.Join(MemberStoreKey(id), attributesSuffix) } - -func clusterVersionFromStore(lg *zap.Logger, st v2store.Store) *semver.Version { - e, err := st.Get(path.Join(storePrefix, "version"), false, false) - if err != nil { - if isKeyNotFound(err) { - return nil - } - lg.Panic( - "failed to get cluster version from store", - zap.String("path", path.Join(storePrefix, "version")), - zap.Error(err), - ) - } - return semver.Must(semver.NewVersion(*e.Node.Value)) -} From 9881db08c652783961798f100619c73385e230ea Mon Sep 17 00:00:00 2001 From: Benjamin Wang Date: Sat, 25 Nov 2023 15:48:08 +0000 Subject: [PATCH 4/4] remove unit test cases for v2store Signed-off-by: Benjamin Wang --- .../etcdserver/api/membership/cluster_test.go | 65 +------------------ 1 file changed, 1 insertion(+), 64 deletions(-) diff --git a/server/etcdserver/api/membership/cluster_test.go b/server/etcdserver/api/membership/cluster_test.go index 62f188e3e74..175ef1a5953 100644 --- a/server/etcdserver/api/membership/cluster_test.go +++ b/server/etcdserver/api/membership/cluster_test.go @@ -17,14 +17,12 @@ package membership import ( "encoding/json" "fmt" - "path" "reflect" "testing" "github.com/stretchr/testify/assert" "go.uber.org/zap/zaptest" - "go.etcd.io/etcd/client/pkg/v3/testutil" "go.etcd.io/etcd/client/pkg/v3/types" "go.etcd.io/etcd/server/v3/etcdserver/api/v2store" "go.etcd.io/etcd/server/v3/mock/mockstore" @@ -519,52 +517,6 @@ func TestNodeToMemberBad(t *testing.T) { } } -func TestClusterAddMember(t *testing.T) { - st := mockstore.NewRecorder() - c := newTestCluster(t, nil) - c.SetStore(st) - c.AddMember(newTestMember(1, nil, "node1", nil), true) - - wactions := []testutil.Action{ - { - Name: "Create", - Params: []any{ - path.Join(StoreMembersPrefix, "1", "raftAttributes"), - false, - `{"peerURLs":null}`, - false, - v2store.TTLOptionSet{ExpireTime: v2store.Permanent}, - }, - }, - } - if g := st.Action(); !reflect.DeepEqual(g, wactions) { - t.Errorf("actions = %v, want %v", g, wactions) - } -} - -func TestClusterAddMemberAsLearner(t *testing.T) { - st := mockstore.NewRecorder() - c := newTestCluster(t, nil) - c.SetStore(st) - c.AddMember(newTestMemberAsLearner(1, []string{}, "node1", []string{"http://node1"}), true) - - wactions := []testutil.Action{ - { - Name: "Create", - Params: []any{ - path.Join(StoreMembersPrefix, "1", "raftAttributes"), - false, - `{"peerURLs":[],"isLearner":true}`, - false, - v2store.TTLOptionSet{ExpireTime: v2store.Permanent}, - }, - }, - } - if g := st.Action(); !reflect.DeepEqual(g, wactions) { - t.Errorf("actions = %v, want %v", g, wactions) - } -} - func TestClusterMembers(t *testing.T) { cls := newTestCluster(t, []*Member{ {ID: 1}, @@ -585,21 +537,6 @@ func TestClusterMembers(t *testing.T) { } } -func TestClusterRemoveMember(t *testing.T) { - st := mockstore.NewRecorder() - c := newTestCluster(t, nil) - c.SetStore(st) - c.RemoveMember(1, true) - - wactions := []testutil.Action{ - {Name: "Delete", Params: []any{MemberStoreKey(1), true, true}}, - {Name: "Create", Params: []any{RemovedMemberStoreKey(1), false, "", false, v2store.TTLOptionSet{ExpireTime: v2store.Permanent}}}, - } - if !reflect.DeepEqual(st.Action(), wactions) { - t.Errorf("actions = %v, want %v", st.Action(), wactions) - } -} - func TestClusterUpdateAttributes(t *testing.T) { name := "etcd" clientURLs := []string{"http://127.0.0.1:4001"} @@ -905,7 +842,7 @@ func TestIsReadyToPromoteMember(t *testing.T) { // 1/1 members ready, should succeed (quorum = 1, new quorum = 2) []*Member{ newTestMember(1, nil, "1", nil), - newTestMemberAsLearner(2, nil, "2", nil), + newTestMemberAsLearner(2, []string{}, "2", []string{}), }, 2, true,