(no)StoreV2 (Part 2): Prepare to read membership information from backend #12820

ptabor · 2021-03-30T12:54:04Z

Preparation work to read membership information from backend instead of v2store.

This change fixes:

2 issues with etcdctl snapshot restore that was not properly updating membership information in backend.
the fact that we were testing upgrade against 3.3 (instead of 3.4)
the fact that V2 etcdctl backup was produciong internally inconsistent membership information between v2 & v2 backend.

This change does not impacts behavior of etcd binary itself, and the membership information is still read for v2store.
Separate PR that changes this will follow.

codecov-io · 2021-04-03T11:26:47Z

Codecov Report

Merging #12820 (c61c97d) into master (a40a6e9) will decrease coverage by 0.85%.
The diff coverage is 65.53%.

@@            Coverage Diff             @@
##           master   #12820      +/-   ##
==========================================
- Coverage   66.43%   65.57%   -0.86%     
==========================================
  Files         410      424      +14     
  Lines       32739    33226     +487     
==========================================
+ Hits        21749    21787      +38     
- Misses       9081     9353     +272     
- Partials     1909     2086     +177

Flag	Coverage Δ
all	`65.57% <65.53%> (-0.86%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
server/mvcc/backend/batch_tx.go	`59.42% <0.00%> (-3.66%)`	⬇️
server/etcdserver/api/membership/cluster.go	`67.09% <38.46%> (-13.09%)`	⬇️
server/etcdserver/api/membership/store.go	`44.31% <46.66%> (-5.69%)`	⬇️
etcdctl/snapshot/v3_snapshot.go	`61.35% <68.00%> (-1.39%)`	⬇️
etcdctl/ctlv2/command/backup_command.go	`73.65% <90.19%> (+7.69%)`	⬆️
etcdctl/ctlv3/command/migrate_command.go	`61.53% <100.00%> (ø)`
server/etcdserver/api/membership/member.go	`83.33% <100.00%> (-5.31%)`	⬇️
server/etcdserver/cluster_util.go	`52.57% <100.00%> (-15.81%)`	⬇️
server/etcdserver/raft.go	`81.08% <100.00%> (-4.30%)`	⬇️
server/etcdserver/api/v3compactor/revision.go	`0.00% <0.00%> (-76.93%)`	⬇️
... and 199 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a40a6e9...c61c97d. Read the comment docs.

etcdctl/snapshot/v3_snapshot.go

server/etcdserver/api/membership/member.go

jingyih · 2021-04-07T22:18:12Z

Let's be careful about this. IIRC, etcdserver does not use consistent index to guard the apply of config change entries.

etcd/server/etcdserver/server.go

Line 2028 in e24e72c

case raftpb.EntryConfChange:

If we switch to restore membership from v3 backend, this means we could potentially re-apply the config changes?

ptabor · 2021-04-07T23:08:58Z

Good catch that we don't have the same shouldApplyV3 guard that we do for regular entries:

etcd/server/etcdserver/server.go

Line 2060 in e24e72c

shouldApplyV3 = true

Taking in consideration that StoreV2 membership information is written in exact same place where Backend is updated:

etcd/server/etcdserver/api/membership/cluster.go

Lines 376 to 381 in e24e72c

    
           if c.v2store != nil { 
        
           	mustSaveMemberToStore(c.lg, c.v2store, m) 
        
           } 
        
           if c.be != nil { 
        
           	mustSaveMemberToBackend(c.lg, c.be, m) 
        
           }

If there is a problem, we would have it already. Maybe membership edit operations are re-entrant and for that reason its working ?

jingyih · 2021-04-08T02:19:43Z

V2 store is not as persistent as the V3 backend. V2 store is in memory. Upon restart, it is initially recovered from the last snapshot (and then raft will replay the WAL entries since the last snapshot), whereas initially V3 backend has all the data up to the applied index.

…lied on v2store ClusterVersionSet, ClusterMemberAttrSet, DowngradeInfoSet functions are writing both to V2store and backend. Prior this CL there were in a branch not executed if shouldApplyV3 was false, e.g. during restore when Backend is up-to-date (has high consistency-index) while v2store requires replay from WAL log. The most serious consequence of this bug was that v2store after restore could have different index (revision) than the same exact store before restore, so potentially different content between replicas. Also this change is supressing double-applying of Membership (ClusterConfig) changes on Backend (store v3) - that lackilly are not part of MVCC/KeyValue store, so they didn't caused Revisions to be bumped. Inspired by jingyih@ comment: etcd-io#12820 (comment)

ptabor · 2021-04-12T16:41:29Z

@jingyih: Thank you for the comments. Based on this I discovered more serious issue: ClusterVersion information was not saved to v2store when restoring with 'recent consistency_index'.

(no)StoreV2 (Part 3): Applying consistency fix: ClusterVersionSet (and co) might get not applied on v2store #12854 fixes the double-apply and no v2store apply problems
(no)StoreV2 (Part 4): Backend hooks: precommit updates consistency_index #12855 introduces an pre-commit hook, that guaranteed cindex persistence and consistency
I removed from this PR actual 'read' from backend in ./etcd binary. I will follow up with it in a separate PR, when this pre-work is DONE.

…lied on v2store ClusterVersionSet, ClusterMemberAttrSet, DowngradeInfoSet functions are writing both to V2store and backend. Prior this CL there were in a branch not executed if shouldApplyV3 was false, e.g. during restore when Backend is up-to-date (has high consistency-index) while v2store requires replay from WAL log. The most serious consequence of this bug was that v2store after restore could have different index (revision) than the same exact store before restore, so potentially different content between replicas. Also this change is supressing double-applying of Membership (ClusterConfig) changes on Backend (store v3) - that lackilly are not part of MVCC/KeyValue store, so they didn't caused Revisions to be bumped. Inspired by jingyih@ comment: etcd-io#12820 (comment)

ptabor · 2021-04-15T22:40:29Z

@jingyih @hexfusion: Please take a look.

jingyih · 2021-04-20T23:11:02Z

etcdctl/snapshot/v3_snapshot.go

-	be := backend.NewDefaultBackend(dbpath)
-	defer be.Close()
-
-	ci := cindex.NewConsistentIndex(be.BatchTx())


Why is setting consistent index removed from this function?

Good observation. Trying to develop a good test that would catch such inconsistency...

@jingyih: PTAL. I fixed this problem and added a test that would detected it. See server/verify/verify.go.

correct 'backend' (bbolt) context in aspect of membership. Prior to this change the 'restored' backend used to still contain: - old memberid (mvcc deletion used, why the membership is in bolt bucket, but not mvcc part): ``` mvs := mvcc.NewStore(s.lg, be, lessor, ci, mvcc.StoreConfig{CompactionBatchLimit: math.MaxInt32}) defer mvs.Close() txn := mvs.Write(traceutil.TODO()) btx := be.BatchTx() del := func(k, v []byte) error { txn.DeleteRange(k, nil) return nil } // delete stored members from old cluster since using new members btx.UnsafeForEach([]byte("members"), del) ``` - didn't get new members added.

…backend.

gyuho · 2021-04-27T16:58:50Z

server/etcdserver/api/membership/store.go

+
+// TrimMembershipFromBackend removes all information about members &
+// removed_members from the v3 backend.
+func TrimMembershipFromBackend(lg *zap.Logger, be backend.Backend) error {


Can we log some info and errors here? lg is not used?

jingyih

LGTM

ptabor force-pushed the 20210326-membership-from-be branch 2 times, most recently from 4843d3e to d3fa68f Compare April 2, 2021 20:39

ptabor assigned hexfusion and ptabor Apr 2, 2021

ptabor requested a review from hexfusion April 2, 2021 20:41

ptabor force-pushed the 20210326-membership-from-be branch 3 times, most recently from 962d0e3 to e9f6cd6 Compare April 3, 2021 10:27

wpedrak reviewed Apr 7, 2021

View reviewed changes

etcdctl/snapshot/v3_snapshot.go Outdated Show resolved Hide resolved

wpedrak reviewed Apr 7, 2021

View reviewed changes

server/etcdserver/api/membership/member.go Outdated Show resolved Hide resolved

ptabor force-pushed the 20210326-membership-from-be branch from e9f6cd6 to c61c97d Compare April 7, 2021 18:36

ptabor requested a review from jingyih April 7, 2021 23:20

ptabor force-pushed the 20210326-membership-from-be branch 3 times, most recently from 8e3393d to 8343d16 Compare April 8, 2021 11:27

ptabor mentioned this pull request Apr 12, 2021

(no)StoreV2 (Part 3): Applying consistency fix: ClusterVersionSet (and co) might get not applied on v2store #12854

Merged

ptabor changed the title ~~(no)StoreV2: Read membership information from backend~~ (no)StoreV2 (Part 5): Read membership information from backend Apr 12, 2021

ptabor changed the title ~~(no)StoreV2 (Part 5): Read membership information from backend~~ (no)StoreV2 (Part 5): Prepare to read membership information from backend Apr 12, 2021

ptabor changed the title ~~(no)StoreV2 (Part 5): Prepare to read membership information from backend~~ (no)StoreV2 (Part 2): Prepare to read membership information from backend Apr 12, 2021

ptabor force-pushed the 20210326-membership-from-be branch from 8343d16 to 486a3c1 Compare April 12, 2021 16:37

ptabor force-pushed the 20210326-membership-from-be branch from 486a3c1 to 16e6aec Compare April 12, 2021 18:46

ptabor added this to the etcd-v3.5 milestone Apr 17, 2021

jingyih reviewed Apr 20, 2021

View reviewed changes

ptabor force-pushed the 20210326-membership-from-be branch 3 times, most recently from e1ffa9a to a18e995 Compare April 22, 2021 07:51

ptabor added 5 commits April 27, 2021 17:17

etcd-dump-logs: Print full confState as json for debugging purposes.

aa65973

Membership: Add additional methods to trim/manage membership data in …

7ae3d25

…backend.

e2e tests: More logging and expect adopted to 3.4.

4725567

Simplify membership interface: Does not pass the 'unused' token.

a70386a

ptabor force-pushed the 20210326-membership-from-be branch from a18e995 to ac295e5 Compare April 27, 2021 15:24

ptabor requested a review from jingyih April 27, 2021 15:28

ptabor force-pushed the 20210326-membership-from-be branch 2 times, most recently from dd73aba to befaf41 Compare April 27, 2021 16:25

gyuho reviewed Apr 27, 2021

View reviewed changes

v2 etcdctl backup: producing consistent state of membership

0675219

ptabor force-pushed the 20210326-membership-from-be branch from 9ef142c to 0675219 Compare April 27, 2021 17:34

jingyih reviewed Apr 28, 2021

View reviewed changes

ptabor merged commit d2722ff into etcd-io:master Apr 28, 2021

ptabor deleted the 20210326-membership-from-be branch April 28, 2021 05:53

ptabor mentioned this pull request May 5, 2021

(no)StoreV2 (Part 4): Backend hooks: precommit updates consistency_index #12855

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(no)StoreV2 (Part 2): Prepare to read membership information from backend #12820

(no)StoreV2 (Part 2): Prepare to read membership information from backend #12820

ptabor commented Mar 30, 2021 •

edited

Loading

codecov-io commented Apr 3, 2021 •

edited

Loading

jingyih commented Apr 7, 2021

ptabor commented Apr 7, 2021

jingyih commented Apr 8, 2021

ptabor commented Apr 12, 2021 •

edited

Loading

ptabor commented Apr 15, 2021

jingyih Apr 20, 2021

ptabor Apr 26, 2021

ptabor Apr 27, 2021

gyuho Apr 27, 2021 •

edited

Loading

ptabor Apr 27, 2021

jingyih left a comment

(no)StoreV2 (Part 2): Prepare to read membership information from backend #12820

(no)StoreV2 (Part 2): Prepare to read membership information from backend #12820

Conversation

ptabor commented Mar 30, 2021 • edited Loading

codecov-io commented Apr 3, 2021 • edited Loading

Codecov Report

jingyih commented Apr 7, 2021

ptabor commented Apr 7, 2021

jingyih commented Apr 8, 2021

ptabor commented Apr 12, 2021 • edited Loading

ptabor commented Apr 15, 2021

jingyih Apr 20, 2021

Choose a reason for hiding this comment

ptabor Apr 26, 2021

Choose a reason for hiding this comment

ptabor Apr 27, 2021

Choose a reason for hiding this comment

gyuho Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

ptabor Apr 27, 2021

Choose a reason for hiding this comment

jingyih left a comment

Choose a reason for hiding this comment

ptabor commented Mar 30, 2021 •

edited

Loading

codecov-io commented Apr 3, 2021 •

edited

Loading

ptabor commented Apr 12, 2021 •

edited

Loading

gyuho Apr 27, 2021 •

edited

Loading