Always keep the compact revision, and delay compact tombstone revision #18162

ahrtr · 2024-06-12T12:33:20Z

Always keep the compact revision, no matter it's a tomestone revision or not. If it's a tombstone revision, then cleanup it in next compact operation.

PoC of option 2 in https://docs.google.com/document/d/1APJE38DxENsRLLVapRz1CQ6UD4-uYaFutO12Y01VcNw/edit

cc @fuweid

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

Always keep the compact revision, no matter it's a tomestone revision or not. If it's a tombstone revision, then cleanup it in next compact operation. Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>

codecov-commenter · 2024-06-12T12:55:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.81%. Comparing base (a6bb22d) to head (763fee3).
Report is 2 commits behind head on main.

❗ Current head 763fee3 differs from pull request most recent head 891e412

Please upload reports for the commit 891e412 to get more accurate results.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Files	Coverage Δ
server/storage/mvcc/key_index.go	`71.25% <100.00%> (+7.09%)`	⬆️

... and 25 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #18162      +/-   ##
==========================================
- Coverage   68.94%   68.81%   -0.14%     
==========================================
  Files         416      416              
  Lines       35127    35121       -6     
==========================================
- Hits        24219    24169      -50     
- Misses       9518     9556      +38     
- Partials     1390     1396       +6

Continue to review full report in Codecov by Sentry.

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

ahrtr · 2024-06-12T13:02:12Z

@fuweid Please feel free to continue to work on the issue #18089 on top of this PR if it makes sense to you. Pls feel free to ping me on slack if you want discussion.

server/storage/mvcc/key_index.go

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

ahrtr · 2024-06-14T15:58:18Z

These are all the test cases I can think of, please feel free to add more. Most likely some of them have already been covered by existing test cases.

I also added the test cases in the doc (see option 2)

cc @fuweid @serathius

Test cases

unit test on keyIndex

case 1: compact a tombstone revision X, it shouldn't be compacted. Next compaction should cleanup the revision X for both cases below,

1.1 The revision X is the last revision for the key
1.2 The revision X isn't the last revision for the key

case 2: compact a normal revision X, it shouldn't be compacted. In next compaction,

2.1 If the revision X is the last revision for the key, it shouldn't be compacted.
2.2 If the revision X isn't the last revision for the key, it should be compacted.

e2e test on watch

case 1: compact tomestone revisions shouldn't impact watch
rough steps:
1) start a watch client to watch on a key
2) start a goroutine to continuous put or delete a key
3) perform compaction against the different tombstone revisions multiple times, verify that no revisions are dropped.

case 2: compact normal revisions shouldn't impact watch
similar steps as above, but we only execute put operations

case 3: a new watch starting on a compacted tombstone revision should receive the delete event
Refer to the reproduction steps in the issue

e2e test on hash values

case 1: All members should have the same hash value up to the compacted revision after compacting a tombstone revision
rough steps:
1) start a 3 member cluster;
2) execute some put and delete operations;
3) execute compaction on revision X;
4) get hashKV up to revision X from all members, verify the hash values are consistent

1.1 all members have the same etcd version
1.2 mix versions, i.e. main vs 3.5.14 or release-3.5 vs 3.5.14

case 2: all member should have the same hash value up to the compacted revision after compacting a non-tombstone revision
similar steps as above, but execute compact on a non-tombstone revision

2.1 all members have the same etcd version
2.2 mix versions, i.e. main vs 3.5.14 or release-3.5 vs 3.5.14

case 3: All members should have the same hash value up to a non-compacted revision after compacting a tombstone revision
similar steps as case 1, but get hashKv up to a non-compacted revision

3.1 all members have the same etcd version
3.2 mix versions, i.e. main vs 3.5.14 or release-3.5 vs 3.5.14

case 4: All members should have the same hash value up to a non-compacted revision after compacting a non-tombstone revision
similar steps as case 2, but get hashKv up to a non-compacted revision

4.1 all members have the same etcd version
4.2 mix versions, i.e. main vs 3.5.14 or release-3.5 vs 3.5.14

robustness test

Support compaction, and verify the impact on hash values and watch.

ahrtr · 2024-06-21T08:43:43Z

@fuweid are you still working on #18089 based on this PR?

serathius · 2024-06-21T09:40:54Z

Can we start implementing those tests?

The bigger problem then the bug existing is fact that we don't have those tests already. Even before we have a fix, we can just add the test to codify the current behavior. It should give us better visibility into impact of when we implement the fix.

/cc @MadhavJivrajani @siyuanfoundation @fuweid @jmhbnz @henrybear327 @ah8ad3

k8s-ci-robot · 2024-06-21T09:42:13Z

@serathius: GitHub didn't allow me to request PR reviews from the following users: ah8ad3.

Note that only etcd-io members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @MadhavJivrajani @siyuanfoundation @fuweid @jmhbnz @henrybear327 @ah8ad3

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

fuweid · 2024-06-21T16:42:07Z

server/storage/mvcc/key_index.go

@@ -221,11 +221,6 @@ func (ki *keyIndex) compact(lg *zap.Logger, atRev int64, available map[Revision]
 		if revIndex != -1 {
 			g.revs = g.revs[revIndex:]
 		}
-		// remove any tombstone
-		if len(g.revs) == 1 && genIdx != len(ki.generations)-1 {


Hi @ahrtr I'm still working on this.

I handle this in a different way. However, I still need more time to add more test cases for that.

diff --git a/server/storage/mvcc/key_index.go b/server/storage/mvcc/key_index.go index e19068f9c..e8223380e 100644 --- a/server/storage/mvcc/key_index.go +++ b/server/storage/mvcc/key_index.go @@ -223,8 +223,10 @@ func (ki *keyIndex) compact(lg *zap.Logger, atRev int64, available map[Revision] } // remove any tombstone if len(g.revs) == 1 && genIdx != len(ki.generations)-1 { - delete(available, g.revs[0]) - genIdx++ + if g.revs[0].Main < atRev { + delete(available, g.revs[0]) + genIdx++ + } } } @@ -243,7 +245,9 @@ func (ki *keyIndex) keep(atRev int64, available map[Revision]struct{}) { if !g.isEmpty() { // remove any tombstone if revIndex == len(g.revs)-1 && genIdx != len(ki.generations)-1 { - delete(available, g.revs[revIndex]) + if g.revs[revIndex].Main < atRev { + delete(available, g.revs[revIndex]) + } } } }

@@ -223,8 +223,10 @@ func (ki *keyIndex) compact(lg *zap.Logger, atRev int64, available map[Revision] } // remove any tombstone if len(g.revs) == 1 && genIdx != len(ki.generations)-1 { - delete(available, g.revs[0]) - genIdx++ + if g.revs[0].Main < atRev { + delete(available, g.revs[0]) + genIdx++ + } } }

It doesn't make sense, because if g.revs[0].Main < atRev will be always false, so it has the same effect as my change, which is much simpler.

Thanks for pointing it out. Yes, when there is only one record and it's not the last generation, we should always keep it.

fuweid · 2024-06-25T14:44:38Z

server/storage/mvcc/key_index.go

+		// operation. We need to clean it up in this round of compact
+		// operation. Refer to discussion in
+		// https://github.com/etcd-io/etcd/issues/18089.
+		if len(g.revs) > 1 {


I think we don't need this if here. Just let it check the following condition. It's more easy to understand and reason

It seems right. If len(g.revs)==1 is true, it means that lastCompactionRev must be equal to the tombstone rev. In this round of compaction, the tombstone revision must be < compaction rev.

So we can revert this change.

k8s-ci-robot · 2024-08-05T22:07:41Z

@ahrtr: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-etcd-unit-test-386	`891e412`	link	false	`/test pull-etcd-unit-test-386`
pull-etcd-unit-test-amd64	`891e412`	link	true	`/test pull-etcd-unit-test-amd64`
pull-etcd-unit-test-arm64	`30eaf12`	link	true	`/test pull-etcd-unit-test-arm64`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

ah8ad3 · 2024-08-06T05:00:18Z

Can we start implementing those tests?

The bigger problem then the bug existing is fact that we don't have those tests already. Even before we have a fix, we can just add the test to codify the current behavior. It should give us better visibility into impact of when we implement the fix.

/cc @MadhavJivrajani @siyuanfoundation @fuweid @jmhbnz @henrybear327 @ah8ad3

I would like to start working on those tests but i am a little bit lost of track if anybody started it or not is there any update?

Always keep the compact revision, and delay remove tombstone revision

891e412

Always keep the compact revision, no matter it's a tomestone revision or not. If it's a tombstone revision, then cleanup it in next compact operation. Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>

k8s-ci-robot added the size/S label Jun 12, 2024

ahrtr changed the title ~~Always keep the compact revision, and delay remove tombstone revision~~ Always keep the compact revision, and delay compact tombstone revision Jun 12, 2024

ahrtr marked this pull request as draft June 12, 2024 12:37

k8s-ci-robot added the do-not-merge/work-in-progress label Jun 12, 2024

ahrtr commented Jun 13, 2024

View reviewed changes

server/storage/mvcc/key_index.go Outdated Show resolved Hide resolved

revert the change for keep

30eaf12

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

ahrtr mentioned this pull request Jun 18, 2024

Watch dropping an event when compacting on delete #18089

Open

4 tasks

k8s-ci-robot requested review from jmhbnz, henrybear327, MadhavJivrajani, siyuanfoundation and fuweid June 21, 2024 09:42

fuweid reviewed Jun 21, 2024

View reviewed changes

fuweid reviewed Jun 25, 2024

View reviewed changes

ahrtr mentioned this pull request Jul 3, 2024

*: keep tombstone if revision == compactAtRev #18274

Merged

ahrtr closed this Aug 16, 2024

ahrtr deleted the delay_compact_delete_20240612 branch August 16, 2024 12:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always keep the compact revision, and delay compact tombstone revision #18162

Always keep the compact revision, and delay compact tombstone revision #18162

ahrtr commented Jun 12, 2024

codecov-commenter commented Jun 12, 2024

ahrtr commented Jun 12, 2024

ahrtr commented Jun 14, 2024

ahrtr commented Jun 21, 2024

serathius commented Jun 21, 2024 •

edited

Loading

k8s-ci-robot commented Jun 21, 2024

fuweid Jun 21, 2024

ahrtr Jun 25, 2024

fuweid Jun 25, 2024

fuweid Jun 25, 2024

ahrtr Jun 25, 2024

k8s-ci-robot commented Aug 5, 2024

ah8ad3 commented Aug 6, 2024 •

edited

Loading

Always keep the compact revision, and delay compact tombstone revision #18162

Always keep the compact revision, and delay compact tombstone revision #18162

Conversation

ahrtr commented Jun 12, 2024

codecov-commenter commented Jun 12, 2024

Codecov Report

ahrtr commented Jun 12, 2024

ahrtr commented Jun 14, 2024

Test cases

unit test on keyIndex

e2e test on watch

e2e test on hash values

robustness test

ahrtr commented Jun 21, 2024

serathius commented Jun 21, 2024 • edited Loading

k8s-ci-robot commented Jun 21, 2024

fuweid Jun 21, 2024

Choose a reason for hiding this comment

ahrtr Jun 25, 2024

Choose a reason for hiding this comment

fuweid Jun 25, 2024

Choose a reason for hiding this comment

fuweid Jun 25, 2024

Choose a reason for hiding this comment

ahrtr Jun 25, 2024

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 5, 2024

ah8ad3 commented Aug 6, 2024 • edited Loading

serathius commented Jun 21, 2024 •

edited

Loading

ah8ad3 commented Aug 6, 2024 •

edited

Loading