[release-3.5] Fix the learner promotion changes not being persisted into v3store (bbolt) #19563

ahrtr · 2025-03-10T12:42:27Z

Link to #19557

Also added a generic verification on ensure the membership data in v3store is always in sync with v2store.

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

ahrtr · 2025-03-10T19:57:16Z

cc @fuweid @ivanvc @serathius @jmhbnz @siyuanfoundation PTAL

This PR is the first item of the proposed actions mentioned in #19557 (comment)

Once this gets merged, I will work on the second item "auto sync v3store based on v2store on bootstrap" in release-3.5 instead of release-3.6.

ahrtr · 2025-03-10T22:23:49Z

/retest

serathius · 2025-03-11T08:15:03Z

This PR seems to fix the bug in learner promotion, but doesn't patches store for clusters that already encountered the bug. Meaning that they will still need to run the etcdutl check members && etcdutl sync members commands, making it critical command before upgrade to v3.6 or risking cluster shuttdown.

EDIT: Fixed "leader promotion" --> "learner promotion".

ahrtr · 2025-03-11T08:20:40Z

This PR seems to fix the bug in leader promotion

"leader promotion" --> "learner promotion".

Yes, exactly.

but doesn't patches store for clusters that already encountered the bug.

It will be covered in a followup PR, also mentioned this multiple times. Please see the second item as mentioned in #19557 (comment)

Meaning that they will still need to run the etcdutl check members && etcdutl sync members commands, making it critical command before upgrade to v3.6 or risking cluster shuttdown.

Already clarified this several times, please see the proposed action and also #19557 (comment)

tests/e2e/ctl_v2_test.go

serathius · 2025-03-11T10:42:35Z

server/etcdserver/api/membership/store.go

@@ -220,8 +220,23 @@ func unsafeSaveMemberToStore(lg *zap.Logger, s v2store.Store, m *Member) error {
 		lg.Panic("failed to marshal raftAttributes", zap.Error(err))
 	}
 	p := path.Join(MemberStoreKey(m.ID), raftAttributesSuffix)
-	_, err = s.Create(p, false, string(b), false, v2store.TTLOptionSet{ExpireTime: v2store.Permanent})
-	return err
+	if _, err = s.Create(p, false, string(b), false, v2store.TTLOptionSet{ExpireTime: v2store.Permanent}); err != nil {


This seems like a separate issue. Thanks for adding validation! However I'm worried about trying to fix two issues in one PR thus not putting enough attention into understanding each of them and introducing proper testing.

Can we separate PR for this one?

This seems like a separate issue.

Yes, it's a separate (but related) minor issue. When adding a new member,

v3store persists the whole member,

but v2store only persists the raftAttributes (peerURL, isLearner); so it causes a temprary inconsistency between v2 and v3store, and it depends on the following publish logic to update the attributes (name, clientURL), so it converge soon.

If we don't fix it in this PR, then the verification will fail, otherwise I will have to update the verification to ignore the attribute and only verify the raftAttributes(including peerURLs, isLearner).

Since from users perspective, this isn't an issue. We can also leave it as it's and only verify raftAttribute.

If we don't fix it in this PR, then the verification will fail, otherwise I will have to update the verification to ignore the attribute and only verify the raftAttributes(including peerURLs, isLearner).

That's why I'm suggesting to send PR to just fix this so we can merge it fast. This could also be backported to v3.4.

Since this isn't an issue from user perspective, so suggest to keep it as it's. This issue only affects version 3.5 (and possibly 3.4, though unverified). Since it is a minor issue and not a concern from the user's perspective, let's just let it fade into history.

Since this isn't an issue from user perspective, so suggest to keep it as it's

I mean that I will revert the change.

reverted the change.

server/etcdserver/server_test.go

tests/e2e/v2store_deprecation_test.go

…v2store and v3store Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>

When promoting a learner, the member is definitely already exist. Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>

server/etcdserver/api/membership/cluster_test.go

ahrtr · 2025-03-11T16:47:58Z

cc @jmhbnz @spzala @serathius @wenjiaswe

cc @ivanvc @fuweid @siyuanfoundation

tests/e2e/ctl_v2_test.go

Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>

ahrtr · 2025-03-11T19:52:04Z

cc @jmhbnz @spzala @serathius @wenjiaswe

serathius · 2025-03-12T09:15:33Z

server/etcdserver/api/membership/cluster_test.go

@@ -1049,10 +1049,9 @@ func TestAddMemberSyncsBackendAndStoreV2(t *testing.T) {
 			backendMembers: []*Member{alice},
 		},
 		{
-			name:           "Adding member should fail if it exists in both",
+			name:           "Adding member should success if it exists in both",


This test case was meant to prevent same member being added twice, while we are fixing the issue of v3store rejecting learner being promoted. Should the test be updated to separate those two cases?

For v3store (bbolt), when we add a member or promote a learner, etcd just marshals the member and write the data into bbolt, if it already exists, just overwrites it. This is the behaviour in both release-3.4, release-3.6 and main. But In release-3.5, it returned an error if it already exists, it's a bug and now we fix it in this PR.

Also FYI.

When we add a member again, the ValidateConfigurationChange will fail.

We also have separate unit test case to verify the v2store will return error when adding a member again. (see line 1057 in cluster_test.go).

Should the test be updated to separate those two cases?

No need. Please see my response above.

k8s-ci-robot · 2025-03-12T12:02:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, fuweid, serathius, siyuanfoundation

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ahrtr,serathius]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added approved size/M labels Mar 10, 2025

ahrtr changed the title ~~add verification to check whether membership data is in sync between …~~ [release-3.5] add verification to check whether membership data is in sync between … Mar 10, 2025

ahrtr force-pushed the 3.5_learner_20250310 branch from af23911 to 6ee8abe Compare March 10, 2025 14:49

k8s-ci-robot added size/L and removed size/M labels Mar 10, 2025

ahrtr force-pushed the 3.5_learner_20250310 branch from ba0b6ac to 87f5cb2 Compare March 10, 2025 15:47

ahrtr changed the title ~~[release-3.5] add verification to check whether membership data is in sync between …~~ [release-3.5] Fix the learner promotion changes not being persisted into v3store (bbolt) Mar 10, 2025

ahrtr added the release/v3.5 label Mar 10, 2025

ahrtr force-pushed the 3.5_learner_20250310 branch 3 times, most recently from 746bb27 to 2ca9b2f Compare March 10, 2025 19:33

ahrtr force-pushed the 3.5_learner_20250310 branch from 2ca9b2f to ab38710 Compare March 10, 2025 22:02

ahrtr mentioned this pull request Mar 10, 2025

[release-3.4] consistent-index not persisted when committing the member deletion operation #19571

Open

4 tasks

ahrtr force-pushed the 3.5_learner_20250310 branch from ab38710 to 49f26c3 Compare March 10, 2025 22:41

ahrtr mentioned this pull request Mar 11, 2025

[release-3.5] Learner promotion isn't persisted to v3store (bbolt) #19557

Open

serathius reviewed Mar 11, 2025

View reviewed changes

tests/e2e/ctl_v2_test.go Show resolved Hide resolved

serathius reviewed Mar 11, 2025

View reviewed changes

server/etcdserver/server_test.go Outdated Show resolved Hide resolved

serathius reviewed Mar 11, 2025

View reviewed changes

tests/e2e/v2store_deprecation_test.go Outdated Show resolved Hide resolved

ahrtr mentioned this pull request Mar 11, 2025

[release-3.5] test: Update server unit test cases to set a backend #19573

Merged

ahrtr force-pushed the 3.5_learner_20250310 branch from 49f26c3 to 1fadfb1 Compare March 11, 2025 13:56

ahrtr added 2 commits March 11, 2025 13:56

add verification to check whether membership data is in sync between …

d32c22d

…v2store and v3store Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>

overwrite the member if already exist

e3cd57a

When promoting a learner, the member is definitely already exist. Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>

ahrtr force-pushed the 3.5_learner_20250310 branch from 1fadfb1 to e3cd57a Compare March 11, 2025 13:57

k8s-ci-robot removed the size/L label Mar 11, 2025

k8s-ci-robot added the size/M label Mar 11, 2025

ahrtr force-pushed the 3.5_learner_20250310 branch from 1609443 to 31b7495 Compare March 11, 2025 15:19

serathius reviewed Mar 11, 2025

View reviewed changes

server/etcdserver/api/membership/cluster_test.go Show resolved Hide resolved

ahrtr force-pushed the 3.5_learner_20250310 branch from 31b7495 to cd24fe8 Compare March 11, 2025 15:52

siyuanfoundation approved these changes Mar 11, 2025

View reviewed changes

tests/e2e/ctl_v2_test.go Outdated Show resolved Hide resolved

fix failed test cases

a1cb13b

Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>

ahrtr force-pushed the 3.5_learner_20250310 branch from cd24fe8 to a1cb13b Compare March 11, 2025 16:53

fuweid approved these changes Mar 11, 2025

View reviewed changes

serathius reviewed Mar 12, 2025

View reviewed changes

serathius approved these changes Mar 12, 2025

View reviewed changes

ahrtr merged commit 2b9598b into etcd-io:release-3.5 Mar 12, 2025
24 checks passed

ahrtr deleted the 3.5_learner_20250310 branch March 12, 2025 12:21

ivanvc mentioned this pull request Mar 14, 2025

Plan to release v3.5.20 #19605

Closed

1 task

ahrtr mentioned this pull request Mar 19, 2025

[release-3.5] [Solution 2] Auto sync members in v3store if Islearner is the only field that differs between v2store and v3store #19606

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[release-3.5] Fix the learner promotion changes not being persisted into v3store (bbolt) #19563

[release-3.5] Fix the learner promotion changes not being persisted into v3store (bbolt) #19563

ahrtr commented Mar 10, 2025 •

edited

Loading

ahrtr commented Mar 10, 2025

ahrtr commented Mar 10, 2025

serathius commented Mar 11, 2025 •

edited

Loading

ahrtr commented Mar 11, 2025

serathius Mar 11, 2025 •

edited

Loading

ahrtr Mar 11, 2025

serathius Mar 11, 2025

ahrtr Mar 11, 2025

ahrtr Mar 11, 2025

ahrtr Mar 11, 2025

ahrtr commented Mar 11, 2025

ahrtr commented Mar 11, 2025

serathius Mar 12, 2025

ahrtr Mar 12, 2025 •

edited

Loading

ahrtr Mar 12, 2025

k8s-ci-robot commented Mar 12, 2025

[release-3.5] Fix the learner promotion changes not being persisted into v3store (bbolt) #19563

[release-3.5] Fix the learner promotion changes not being persisted into v3store (bbolt) #19563

Conversation

ahrtr commented Mar 10, 2025 • edited Loading

ahrtr commented Mar 10, 2025

ahrtr commented Mar 10, 2025

serathius commented Mar 11, 2025 • edited Loading

ahrtr commented Mar 11, 2025

serathius Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

ahrtr Mar 11, 2025

Choose a reason for hiding this comment

serathius Mar 11, 2025

Choose a reason for hiding this comment

ahrtr Mar 11, 2025

Choose a reason for hiding this comment

ahrtr Mar 11, 2025

Choose a reason for hiding this comment

ahrtr Mar 11, 2025

Choose a reason for hiding this comment

ahrtr commented Mar 11, 2025

ahrtr commented Mar 11, 2025

serathius Mar 12, 2025

Choose a reason for hiding this comment

ahrtr Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

ahrtr Mar 12, 2025

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 12, 2025

ahrtr commented Mar 10, 2025 •

edited

Loading

serathius commented Mar 11, 2025 •

edited

Loading

serathius Mar 11, 2025 •

edited

Loading

ahrtr Mar 12, 2025 •

edited

Loading