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

[ahrtr]

Second solution to provide a fix for the users who have already been affected by #19557. It's based on the comment #19586 (comment),

• good side
  • Sync the v3store before the raft loop getting started, and also in appy workflow. So no any potential conflict
• bad side
  • created two exceptions
    • Forcibly apply member promotion to v3store even when shouldApplyV3 is false
    • Use LookOutsideApply inside the apply workflow, so we have to disable the verification in e2e test case.
  • updated the apply workflow...

The first solution is #19586

• good side
  • did not change the apply workflow
  • it's only executed on boostrap, it's an one-time operation
• bad side
  • It updates the backend outside apply workflow, so has potential conflict. But actually it's protected by lock, so it's still safe.

Both solutions work. If there is no strong objection or preference, let's go for solution 2 (this PR)

cc @fuweid @ivanvc @jmhbnz @serathius @siyuanfoundation

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr

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]

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

[ahrtr]

cc @fuweid @ivanvc @jmhbnz @serathius @siyuanfoundation

Hi folks, can you spend at least 1 hour each day to move this forward?

[serathius]

Not 100% sure about consequences of calling sync here. I don't know if we really don't run any goroutines in background at this point, and proving so might be very hard.

By proposing to run it during bootstrap I meant within bootstrap.go

[ahrtr]

This is release-3.5 isntead of main or release-3.6, there is no bootstrap.go. Note that we need to do this after corruption check,

https://github.com/ahrtr/etcd/blob/2f55a7c5144d084541114340eb058fb3a37ad9b5/server/embed/etcd.go#L260-L281

[serathius]

How do you think this change will interact with corrupt check? Even if we do the sync before corrupt check on one member, next member will start with corrupt check before sync.

[ahrtr]

I don't know if we really don't run any goroutines in background at this point, and proving so might be very hard.

We call SyncLearnerPromotionIfNeeded before starting the raft loop, before starting all the [periodically] jobs (i.e. monitorVersions, linearizableReadLoop, monitorKVHash, etc), also before starting to serve client & peer requests. It's safe. What's your concern here?

How do you think this change will interact with corrupt check?

If there is data corrupt, we should leave it as it is.

[fuweid]

I am thinking if we still need this migration.

I found there is publish API call during starting. Since we already allow to override member information, this publish can help us migration (if I understand it correctly).

etcd/server/etcdserver/server.go

Line 2132 in 3c65dfa

Path: membership.MemberAttributesStorePath(s.id),

• If there is available snapshot, the v2store contains valid data. UpdateAtribute will override existing invalid one. (REF: [3.5] tests: add TestIssue19557FixMemberDataAfterUpgrade #19627)
• If there is no snapshot, WAL replay can fix this issue as well.

[ahrtr]

@serathius please let me know if you still have any concern on my previous comment. I think we should be all good now.

I am thinking if we still need this migration.

I found there is publish API call during starting. Since we already allow to override member information, this publish can help us migration (if I understand it correctly).

@fuweid I don't think we are on the same page. The publish logic is about attribute, what we are resolving/talking about in this PR (and issue 19557) is about IsLearner, which is part of RaftAttribute.

[ahrtr]

override member information

Yes, this is true. thx for pointing out this.

Although the publish logic is only supposed to update attribute, but the implementation just overwrites the whole member info into v3store (bbolt), so it can automatically fix the issues which are affected by #19557 on top of the patch #19563

Just raised another PR with only e2e test cases #19629, which I believe is better than #19627

[ahrtr]

For release-3.6, we don't need any further validation (i.e. requiring 3.5.20+ before upgrading to 3.6) any more, reasons:

• If there are 2 or more (already promoted) learners, then etcd will crash anyway.
• If there is only 1 (already promoted) learner, then etcd will automatically fix it due to the same reason as mentioned above.

So for release-3.6, we only need to add two e2e test cases:

• the kubeadm style upgrade case
• verify etcd is able to automatically fix the issue when upgrading to 3.6 for the one learner case, similar to [release-3.5] Add e2e test to verify etcd is able to automatically fix the issue #19629

Note that we still need to document the requirement of requiring 3.5.20+ before upgrading to 3.6, to avoid the potential upgrading failure (as mentioned above, etcd will crash if there are 2 or more learners).