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

ptabor · 2021-04-12T16:15:43Z

Implemented hooks mechanism in the backend, in particular pre-commit hook that:

guarantees code execution pre-every commit, even if the commit is triggered automatically on time or batch-size.
is potential place to add additional validations.

Integrated it with cindex, such that every transaction committed to backend is writing most recent consistent_index.

Makes sure that even automatically trigger commits of batch-transactions stays "really" consistent a.d. the most recent WAL log index applied. Prior to this PR membership and version edits were in particular not bumping up the 'cindex'.

Part of: #12913

server/etcdserver/cindex/cindex.go

server/mvcc/backend/backend_test.go

server/mvcc/backend/hooks.go

server/mvcc/backend/hooks_test.go

server/mvcc/backend/testing/betesting.go

wpedrak

LGTM. Few minor comments.

ptabor · 2021-04-21T07:41:08Z

@jingyih: I rebased the PR and its ready for review.

server/mvcc/backend/hooks_test.go

Every transaction committed to backend is writing most recent consistent_index. Makes sure that even automatically trigger commits of batch-transactions stays "really" consistent a.d. the most recent WAL log index applied.

…n care by hook.

ptabor · 2021-05-05T07:07:48Z

Not sure I fully understand this statement. It sounds to me that we improved / fixed something? Could you elaborate?

@jingyih
When I enabled it, I revealed several bugs (test flakes), that seems to be hidden by cindex lagging in DB:

I think that membership and version edits were in particular not bumping up the 'cindex' (as mvcc edits of backends were updating it, but not the direct edits to backend)

Added to PR description.

@gyuho please take a look at the proposed, more descriptive, names

jingyih · 2021-05-05T07:32:05Z

@ptabor Thanks for the explanation. This makes sense to me. In the past all membership related raft entries (or more generally, all cluster related) are not tied to consistent index, as the consistent index was designed for kv store (as opposed to metadata bucket which stores cluster related info). With your changes, it makes the handling of raft entries of different types more consistent.

LGTM

server/mvcc/backend/batch_tx.go

tangcong · 2022-03-29T15:16:27Z

server/mvcc/kvstore_txn.go

@@ -104,7 +104,6 @@ func (tw *storeTxnWrite) Put(key, value []byte, lease lease.LeaseID) int64 {
 func (tw *storeTxnWrite) End() {
 	// only update index if the txn modifies the mvcc state.
 	if len(tw.changes) != 0 {
-		tw.s.saveIndex(tw.tx)


It looks like this breaks the atomicity of consistent index and writing transactions. @ptabor @ahrtr @serathius #13844

…rease Reason to store CI and term in backend was to make db fully independent snapshot, it was never meant to interfere with apply logic. Skip of CI was introduced for v2->v3 migration where we wanted to prevent it from decreasing when replaying wal in etcd-io#5391. By mistake it was added to apply flow during refactor in etcd-io#12855 (comment). Consistency index and term should only be negotiated and used by raft to make decisions. Their values should only driven by raft state machine and backend should only be responsible for storing them.

ptabor changed the title ~~Backend hooks: precommit updates consistency_index~~ (no)StoreV2 (Part 4): Backend hooks: precommit updates consistency_index Apr 12, 2021

ptabor mentioned this pull request Apr 12, 2021

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

Merged

ptabor force-pushed the 20210409-backend-hooks branch from cec56af to f589fa2 Compare April 12, 2021 17:02