-
Notifications
You must be signed in to change notification settings - Fork 10.2k
[release-3.5] server: cleanup zombie membership information #20995
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
e47654e to
affcd21
Compare
affcd21 to
359c4eb
Compare
| // createClustewithZombieMembers creates a cluster with zombie members in v3store. | ||
| // | ||
| // There are two ways to create zombie members: | ||
| // | ||
| // 1. Restoring a cluster using a snapshot taken from version <= v3.4.39. | ||
| // | ||
| // The `etcdctl snapshot restore` tool in versions <= v3.4.39 contains a bug | ||
| // that fails to clean up old member information during restore. This leaves | ||
| // behind stale (zombie) member entries in the v3store. | ||
| // | ||
| // 2. Using ForceNewCluster on a cluster running a version < v3.5.22. | ||
| // | ||
| // If ETCD commits a ConfChangeAddNode entry to the v3store but crashes before | ||
| // the corresponding hard state is persisted to the WAL, then after restarting | ||
| // with ForceNewCluster, the member entries in the v3store are not cleaned up, | ||
| // resulting in zombie members. This issue was fixed in [v3.5.22][1]. | ||
| // | ||
| // It may be possible to create zombie member in versions < [v3.4.0][2] through a | ||
| // single-node ForceNewCluster restart. For example, suppose we start with a | ||
| // one-node cluster. We add a second node, but the second node fails to join | ||
| // and the cluster loses quorum. We then restart the first node with ForceNewCluster, | ||
| // which results in a zombie member entry for the second node. However, it | ||
| // is still unlikely for three zombie members (like [issue-20967][3]) to | ||
| // appear at the same time. | ||
| // | ||
| // So in theory, this could happen when using ForceNewCluster. However, it is | ||
| // extremely unlikely in practice because multiple members are running, and it | ||
| // would require all of them to panic at the same time. It would also require | ||
| // selecting one of the crashed members to restart with ForceNewCluster, where | ||
| // that member had failed to persist the hard state to the WAL—an improbable | ||
| // combination of events (what a coincidence). | ||
| // | ||
| // So, in this function, we use the first method to create zombie members for | ||
| // testing. | ||
| // | ||
| // REF: | ||
| // | ||
| // [1]: https://github.com/etcd-io/etcd/commit/ccf66456e7237ef5251d9fdc96f1624d1aa4daf1 | ||
| // [2]: https://github.com/etcd-io/etcd/issues/14370 | ||
| // [3]: https://github.com/etcd-io/etcd/issues/20967 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the comment per #20967 (comment)
| func CleanupZombieMembersIfNeeded(lg *zap.Logger, be backend.Backend, st v2store.Store) { | ||
| v2Members, _ := membersFromStore(lg, st) | ||
| v3Members, _ := membersFromBackend(lg, be) | ||
|
|
||
| for id, v3Member := range v3Members { | ||
| _, ok := v2Members[id] | ||
| if !ok { | ||
| lg.Warn("Removing zombie member from v3store", zap.String("member", fmt.Sprintf("%+v", *v3Member))) | ||
| if err := unsafeDeleteMemberFromBackend(be, id); err != nil { | ||
| lg.Warn("failed to delete zombie member from backend", zap.String("member-id", id.String()), zap.Error(err)) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking probably we should sync from v2store to v3store to address the following cases:
- zombie members in v3store (exist in v3, but not in v2). It's already reproduced and confirmed.
- missing members in v3store (exist in v2, but not in v3). We haven't see any reproduction for such case so far. Since v2store is the only source of truth for membership data for v3.5.x and older versions, so we should completely trust v2store, so probably we should take a proactive measure to sync from v2store to v3store for such case as well. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we should completely trust v2store, so probably we should take a proactive measure to sync from v2store to v3store for such case as well. WDYT?
Agree so we can sync after confChange
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, I am thinking probably we should handle the case missing members in v3store (exist in v2, but not in v3) as well, although we haven't seen any reproduction for such case so far.
| } | ||
|
|
||
| // restoreSnapshotIntoMemberDir restores snapshot into the given member's data dir. | ||
| func restoreSnapshotIntoMemberDir(t *testing.T, etcdctlBin string, snapshotPath string, initialCluster string, proc e2e.EtcdProcess) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add such function into tests/framework/e2e/etcdctl.go. I won't insist on it since it's stable releases, as we won't add any new features, so I don't expect any major refatoring.
359c4eb to
8a45a7a
Compare
| func CleanupZombieMembersIfNeeded(lg *zap.Logger, be backend.Backend, st v2store.Store) { | ||
| v2Members, _ := membersFromStore(lg, st) | ||
| v3Members, _ := membersFromBackend(lg, be) | ||
|
|
||
| for id, v3Member := range v3Members { | ||
| _, ok := v2Members[id] | ||
| if !ok { | ||
| lg.Warn("Removing zombie member from v3store", zap.String("member", fmt.Sprintf("%+v", *v3Member))) | ||
| if err := unsafeDeleteMemberFromBackend(be, id); err != nil { | ||
| lg.Warn("failed to delete zombie member from backend", zap.String("member-id", id.String()), zap.Error(err)) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, I am thinking probably we should handle the case missing members in v3store (exist in v2, but not in v3) as well, although we haven't seen any reproduction for such case so far.
Signed-off-by: Wei Fu <[email protected]>
There are three places where zombie membership information can be cleaned up: 1. ApplierV2 -> UpdateAttributes Ideally, each member updates its own attributes only once before serving requests. Since this happens only at startup and at low frequency, it is safe to clean up zombie members during this operation. Additionally, pending logs are always applied before attributes are updated, so there is no data race. 2. Applying a snapshot via Raft message When a member receives a snapshot through a Raft message, the v2store is synchronized with the v3store. This makes it safe to detect and clean up zombie membership entries at that time. 3. Applying ConfChange Check and cleanup zombie members after ConfChange. Ideally, there aren't too many ConfChange, the impact on performance should be minimal. Signed-off-by: Wei Fu <[email protected]>
8a45a7a to
47f9b5a
Compare
|
@ahrtr for this comment,
I think we can keep it as it is right now. based on current situation, v3store has more information than v2store. If we don't have such case like v2store contains more information than v3, we can assume it's not going to happen. |
Hopefully we won't have to fix a follow-up issue and generate a follow-up blog. |
ahrtr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr, fuweid 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:
Approvers can indicate their approval by writing |
There are three places where zombie membership information can be cleaned up:
Fixes: #20967
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.