Auto sync members in v3store is IsLearner differs between v2 and v3 store

ahrtr · ahrtr · commit 507f6d66e982 · 2025-03-21T19:45:57.000Z
Signed-off-by: Benjamin Wang &lt;benjamin.ahrtr@gmail.com&gt;
diff --git a/server/etcdserver/api/membership/cluster.go b/server/etcdserver/api/membership/cluster.go
@@ -524,26 +524,72 @@ func (c *RaftCluster) PromoteMember(id types.ID, shouldApplyV3 ShouldApplyV3) {
 		isLearner.Set(0)
 	}
 
-	if c.be != nil && shouldApplyV3 {
-		c.members[id].RaftAttributes.IsLearner = false
-		c.updateMembershipMetric(id, true)
-		c.be.MustSaveMemberToBackend(c.members[id])
+	if c.be != nil {
+		m := c.members[id]
+		if shouldApplyV3 {
+			m.RaftAttributes.IsLearner = false
+			c.updateMembershipMetric(id, true)
+			c.be.MustSaveMemberToBackend(m)
 
-		c.lg.Info(
-			"promote member",
-			zap.String("cluster-id", c.cid.String()),
-			zap.String("local-member-id", c.localID.String()),
-			zap.String("promoted-member-id", id.String()),
-		)
+			c.lg.Info(
+				"promote member",
+				zap.String("cluster-id", c.cid.String()),
+				zap.String("local-member-id", c.localID.String()),
+				zap.String("promoted-member-id", id.String()),
+			)
+		} else {
+			// Workaround the issues which have already been affected by
+			// https://github.com/etcd-io/etcd/issues/19557. The learner
+			// promotion request had been applied to v3store, but not saved
+			// to v2snapshot yet when in 3.5. Once upgrading to 3.6, the
+			// patch here ensure the issue can be automatically fixed.
+			if m.IsLearner {
+				m.RaftAttributes.IsLearner = false
+				c.lg.Info("Forcibly apply member promotion request", zap.String("member", fmt.Sprintf("%+v", *m)))
+				c.be.MustSaveMemberToBackend(m)
+			} else {
+				c.lg.Info(
+					"ignore already promoted member",
+					zap.String("cluster-id", c.cid.String()),
+					zap.String("local-member-id", c.localID.String()),
+				)
+			}
+		}
 	} else {
 		c.lg.Info(
-			"ignore already promoted member",
+			"ignore already promoted member due to backend being nil",
 			zap.String("cluster-id", c.cid.String()),
 			zap.String("local-member-id", c.localID.String()),
 		)
 	}
 }
 
+// SyncLearnerPromotionIfNeeded provides a workaround solution to fix the issues
+// which have already been affected by https://github.com/etcd-io/etcd/issues/19557.
+func (c *RaftCluster) SyncLearnerPromotionIfNeeded() {
+	c.Lock()
+	defer c.Unlock()
+
+	v2Members, _ := membersFromStore(c.lg, c.v2store)
+	v3Members, _ := c.be.MustReadMembersFromBackend()
+
+	for id, v3Member := range v3Members {
+		v2Member, ok := v2Members[id]
+		if !ok {
+			// This isn't an error. The conf change on the member hasn't been saved to the v2 snapshot yet.
+			c.lg.Info("Detected member only in v3store but missing in v2store", zap.String("member", fmt.Sprintf("%+v", *v3Member)))
+			continue
+		}
+
+		if !v2Member.IsLearner && v3Member.IsLearner {
+			syncedV3Member := v3Member.Clone()
+			syncedV3Member.IsLearner = false
+			c.lg.Warn("Syncing member in v3store", zap.String("member", fmt.Sprintf("%+v", *syncedV3Member)))
+			c.be.MustHackySaveMemberToBackend(syncedV3Member)
+		}
+	}
+}
+
 func (c *RaftCluster) UpdateRaftAttributes(id types.ID, raftAttr RaftAttributes, shouldApplyV3 ShouldApplyV3) {
 	c.Lock()
 	defer c.Unlock()
diff --git a/server/etcdserver/api/membership/cluster_test.go b/server/etcdserver/api/membership/cluster_test.go
@@ -23,6 +23,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"go.uber.org/zap/zaptest"
 
 	"go.etcd.io/etcd/client/pkg/v3/testutil"
@@ -1048,3 +1049,128 @@ func TestClusterStore(t *testing.T) {
 		})
 	}
 }
+
+func TestSyncLearnerPromotion(t *testing.T) {
+	tcs := []struct {
+		name string
+
+		storeV2Members []*Member
+		backendMembers []*Member
+
+		expectV3Members map[types.ID]*Member
+	}{
+		{
+			name: "v3store should keep unchanged if the member doesn't exist in v2store",
+			storeV2Members: []*Member{
+				{
+					ID: 100,
+					RaftAttributes: RaftAttributes{
+						PeerURLs:  []string{"http://10.0.0.10:2380"},
+						IsLearner: false,
+					},
+				},
+			},
+			backendMembers: []*Member{
+				{
+					ID: 200,
+					RaftAttributes: RaftAttributes{
+						PeerURLs:  []string{"http://10.0.0.9:2380"},
+						IsLearner: true,
+					},
+				},
+			},
+			expectV3Members: map[types.ID]*Member{
+				200: {
+					ID: 200,
+					RaftAttributes: RaftAttributes{
+						PeerURLs:  []string{"http://10.0.0.9:2380"},
+						IsLearner: true,
+					},
+				},
+			},
+		},
+		{
+			name: "v3store should keep unchanged if v2store.IsLearner is true",
+			storeV2Members: []*Member{
+				{
+					ID: 100,
+					RaftAttributes: RaftAttributes{
+						PeerURLs:  []string{"http://10.0.0.9:2380"},
+						IsLearner: true,
+					},
+				},
+			},
+			backendMembers: []*Member{
+				{
+					ID: 100,
+					RaftAttributes: RaftAttributes{
+						PeerURLs:  []string{"http://10.0.0.9:2380"},
+						IsLearner: true,
+					},
+				},
+			},
+			expectV3Members: map[types.ID]*Member{
+				100: {
+					ID: 100,
+					RaftAttributes: RaftAttributes{
+						PeerURLs:  []string{"http://10.0.0.9:2380"},
+						IsLearner: true,
+					},
+				},
+			},
+		},
+		{
+			name: "v3store should be updated if v2.IsLearner is false and v3store.IsLearner is true",
+			storeV2Members: []*Member{
+				{
+					ID: 100,
+					RaftAttributes: RaftAttributes{
+						PeerURLs:  []string{"http://10.0.0.9:2380"},
+						IsLearner: false,
+					},
+				},
+			},
+			backendMembers: []*Member{
+				{
+					ID: 100,
+					RaftAttributes: RaftAttributes{
+						PeerURLs:  []string{"http://10.0.0.9:2380"},
+						IsLearner: true,
+					},
+				},
+			},
+			expectV3Members: map[types.ID]*Member{
+				100: {
+					ID: 100,
+					RaftAttributes: RaftAttributes{
+						PeerURLs:  []string{"http://10.0.0.9:2380"},
+						IsLearner: false,
+					},
+				},
+			},
+		},
+	}
+	for _, tc := range tcs {
+		t.Run(tc.name, func(t *testing.T) {
+			lg := zaptest.NewLogger(t)
+			be := newMembershipBackend()
+
+			for _, m := range tc.backendMembers {
+				be.MustSaveMemberToBackend(m)
+			}
+
+			st := v2store.New()
+			for _, m := range tc.storeV2Members {
+				mustSaveMemberToStore(lg, st, m)
+			}
+
+			cluster := NewCluster(lg)
+			cluster.SetBackend(be)
+			cluster.SetStore(st)
+
+			cluster.SyncLearnerPromotionIfNeeded()
+			v3Members, _ := cluster.be.MustReadMembersFromBackend()
+			require.Equal(t, tc.expectV3Members, v3Members)
+		})
+	}
+}
diff --git a/server/etcdserver/api/membership/membership_test.go b/server/etcdserver/api/membership/membership_test.go
@@ -85,6 +85,10 @@ func (b *backendMock) MustSaveMemberToBackend(m *Member) {
 	b.members[m.ID] = m
 }
 
+func (b *backendMock) MustHackySaveMemberToBackend(m *Member) {
+	b.members[m.ID] = m
+}
+
 func (b *backendMock) TrimMembershipFromBackend() error {
 	b.members = make(map[types.ID]*Member)
 	b.removed = make(map[types.ID]bool)
diff --git a/server/etcdserver/api/membership/store.go b/server/etcdserver/api/membership/store.go
@@ -39,6 +39,7 @@ type ClusterVersionBackend interface {
 type MemberBackend interface {
 	MustReadMembersFromBackend() (map[types.ID]*Member, map[types.ID]bool)
 	MustSaveMemberToBackend(*Member)
+	MustHackySaveMemberToBackend(*Member)
 	TrimMembershipFromBackend() error
 	MustDeleteMemberFromBackend(types.ID)
 }
diff --git a/server/etcdserver/bootstrap.go b/server/etcdserver/bootstrap.go
@@ -448,6 +448,11 @@ func (c *bootstrappedCluster) Finalize(cfg config.ServerConfig, s *bootstrappedS
 	}
 	c.cl.SetStore(s.st)
 	c.cl.SetBackend(schema.NewMembershipBackend(cfg.Logger, s.backend.be))
+
+	// Workaround the issues which have already been affected
+	// by https://github.com/etcd-io/etcd/issues/19557.
+	c.cl.SyncLearnerPromotionIfNeeded()
+
 	if s.wal.haveWAL {
 		c.cl.Recover(api.UpdateCapability)
 		if c.databaseFileMissing(s) {
diff --git a/server/storage/backend/verify.go b/server/storage/backend/verify.go
@@ -62,6 +62,14 @@ func verifyLockEnabled() bool {
 
 func insideApply() bool {
 	stackTraceStr := string(debug.Stack())
+
+	// Exclude the case of `MustHackySaveMemberToBackend`, which is
+	// used to workaround the situations which are already affected
+	// by https://github.com/etcd-io/etcd/issues/19557.
+	if strings.Contains(stackTraceStr, "MustHackySaveMemberToBackend") {
+		return false
+	}
+
 	return strings.Contains(stackTraceStr, ".applyEntries")
 }
 
diff --git a/server/storage/schema/membership.go b/server/storage/schema/membership.go
@@ -57,6 +57,22 @@ func (s *membershipBackend) MustSaveMemberToBackend(m *membership.Member) {
 	tx.UnsafePut(Members, mkey, mvalue)
 }
 
+// MustHackySaveMemberToBackend updates the member in a hacky way.
+// It's only used to fix the issues which are already affected by
+// https://github.com/etcd-io/etcd/issues/19557.
+func (s *membershipBackend) MustHackySaveMemberToBackend(m *membership.Member) {
+	mkey := BackendMemberKey(m.ID)
+	mvalue, err := json.Marshal(m)
+	if err != nil {
+		s.lg.Panic("failed to marshal member", zap.Error(err))
+	}
+
+	tx := s.be.BatchTx()
+	tx.LockOutsideApply()
+	defer tx.Unlock()
+	tx.UnsafePut(Members, mkey, mvalue)
+}
+
 // TrimClusterFromBackend removes all information about cluster (versions)
 // from the v3 backend.
 func (s *membershipBackend) TrimClusterFromBackend() error {
diff --git a/tests/e2e/ctl_v3_member_test.go b/tests/e2e/ctl_v3_member_test.go
diff --git a/tests/go.mod b/tests/go.mod

Original file line number	Diff line number	Diff line change
`@@ -39,6 +39,7 @@ type ClusterVersionBackend interface {`
`39`	`39`	`type MemberBackend interface {`
`40`	`40`	`MustReadMembersFromBackend() (map[types.ID]*Member, map[types.ID]bool)`
`41`	`41`	`MustSaveMemberToBackend(*Member)`
	`42`	`+ MustHackySaveMemberToBackend(*Member)`
`42`	`43`	`TrimMembershipFromBackend() error`
`43`	`44`	`MustDeleteMemberFromBackend(types.ID)`
`44`	`45`	`}`