Improve snapshot status #15979

cenkalti · 2023-05-29T16:40:39Z

Adding unmarshaling for items in "key" bucket for extra safety.

lavacat

Overall lgtm, but needs a test

etcdutl/snapshot/util.go

cenkalti · 2023-05-30T19:14:49Z

@lavacat I added a test. Can you take a look again please?

cenkalti · 2023-06-08T16:48:43Z

/cc: @chaochn47

etcdutl/snapshot/v3_snapshot.go

chaochn47 · 2023-06-08T18:03:28Z

If the CI is green, I think this should attract more reviewers to take a look and approve the PR.

You can git commit --amend --no-edit and force pushed it to restart the test for now :\

I've tried in #15659 but with no luck.

jmhbnz

Hey @cenkalti - It looks like a minor update to fix the static analysis see below.

Can you also please rebase this pr.

etcdutl/snapshot/v3_snapshot_test.go

ahrtr · 2024-01-18T19:13:55Z

I recall we refactored the bytesToRev in another PR, please try to reuse the existing function. thx

cenkalti · 2024-01-18T19:37:38Z

Sure, I'll rebase this PR and make sure all checks pass.

cenkalti · 2024-01-19T04:44:06Z

/retest

cenkalti · 2024-01-19T06:36:25Z

server/storage/mvcc/revision.go

+	sub := int64(binary.BigEndian.Uint64(bytes[9:]))
+	if main < -1 || sub < 0 { // finishedCompactRev can be -1
+		return BucketKey{}, errors.New("negative revision")
+	}


Revision numbers must be positive. Negative values are invalid. I added these extra checks to be able to detect corruptions on database when checking snapshot status on disk.

Suggest not to change the signature. Instead we can add assertion (panicking when it breaks any invariant properties)

Also I see your comment "// finishedCompactRev can be -1". Can you elaborate in what exact case finishedCompactRev may be -1?

It's initially set to one:
https://github.com/cenkalti/etcd/blob/0316c5af0e7e9dc02e206e8f26df9fc4c5bc2118/server/storage/mvcc/kvstore.go#L100

I don't see any case etcd may persist the -1.

Reverted back the signature change. Now it's panicking in case of invalid revision bytes.

server/storage/mvcc/store_test.go

cenkalti · 2024-01-19T07:12:48Z

/retest

cenkalti · 2024-01-19T14:59:58Z

All tests passing. Ready for review again.

server/storage/mvcc/revision.go

cenkalti · 2024-01-25T20:37:52Z

@ahrtr I squashed all my commits.

etcdutl/snapshot/v3_snapshot.go

serathius · 2024-01-26T09:43:52Z

etcdutl/snapshot/v3_snapshot_test.go

+// TestSnapshotStatus is the happy case.
+// It inserts pre-defined number of keys and asserts the output hash of status command.
+// The expected hash value must not be changed.
+// If it changes, there must be some backwards incompatible change introduced.


Could we merge the tests before we change logic? It's nice to separate the refactors and tests that are meant to prove nothing changed.

I don't think that's necessary because refactors doesn't affect the hash value in any way. They're only about adding extra error conditions.

ahrtr

lgtm

Thanks

etcdutl/snapshot/v3_snapshot.go

etcdutl/snapshot/v3_snapshot_test.go

jmhbnz · 2024-03-28T18:10:43Z

Discussed during sig-etcd triage meeting, it looks like this older pr was very close to merge, @cenkalti could you please rebase to address conflicts?

cenkalti · 2024-03-28T18:27:34Z

I'll do it this weekend.

cenkalti · 2024-03-30T15:09:23Z

Rebased. Ready to merge.

jmhbnz · 2024-06-06T18:12:22Z

Hi @cenkalti can you please rebase this? Thanks

jmhbnz · 2024-06-08T05:23:30Z

/retest

ivanvc · 2024-06-13T00:10:27Z

This PR was very close to merging. It has been rebased a couple of times. Does it make sense to move forward? There is at least an open comment from @fuweid: https://github.com/etcd-io/etcd/pull/15979/files#r1467819098.

We have several reviewers but only approval from @ahrtr.

What would be required to move forward with this pull request?

fuweid · 2024-06-13T03:13:15Z

There is at least an open comment from @fuweid: https://github.com/etcd-io/etcd/pull/15979/files#r1467819098.

It's just my two cents for using %w. In the whole project, it's using : %w.
If err= prefix is easy for grep in command line, it's fine.

Signed-off-by: Cenk Alti <cenkalti@gmail.com>

cenkalti · 2024-06-15T14:12:13Z

I fixed the error messages and rebased again.

codecov-commenter · 2024-06-15T14:29:16Z

Codecov Report

Attention: Patch coverage is 68.57143% with 11 lines in your changes missing coverage. Please review.

Project coverage is 68.85%. Comparing base (15f9a59) to head (23c4428).

❗ Current head 23c4428 differs from pull request most recent head ea46253

Please upload reports for the commit ea46253 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/revision.go	`91.48% <80.00%> (-3.39%)`	⬇️
etcdutl/snapshot/v3_snapshot.go	`57.43% <64.00%> (+8.53%)`	⬆️

... and 25 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #15979      +/-   ##
==========================================
+ Coverage   68.75%   68.85%   +0.10%     
==========================================
  Files         416      416              
  Lines       35128    35151      +23     
==========================================
+ Hits        24152    24203      +51     
+ Misses       9569     9542      -27     
+ Partials     1407     1406       -1

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 15f9a59...ea46253. Read the comment docs.

jmhbnz

LGTM - Thanks @cenkalti

lavacat reviewed May 30, 2023

View reviewed changes

etcdutl/snapshot/util.go Outdated Show resolved Hide resolved

etcdutl/snapshot/util.go Outdated Show resolved Hide resolved

cenkalti force-pushed the snapshot-status branch 2 times, most recently from d634c55 to de8c7aa Compare May 30, 2023 19:14

cenkalti added the area/etcdutl label May 31, 2023

chaochn47 reviewed Jun 8, 2023

View reviewed changes

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

fuweid mentioned this pull request Jul 19, 2023

Refactor common revision code to pkg #16269

Merged

jmhbnz reviewed Jan 18, 2024

View reviewed changes

etcdutl/snapshot/v3_snapshot_test.go Show resolved Hide resolved

cenkalti force-pushed the snapshot-status branch 2 times, most recently from 32f60ef to 3b4dfb6 Compare January 19, 2024 00:50

cenkalti force-pushed the snapshot-status branch 5 times, most recently from 7f32b22 to 91cc26d Compare January 19, 2024 06:32

cenkalti commented Jan 19, 2024

View reviewed changes

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

cenkalti force-pushed the snapshot-status branch from 91cc26d to dded93f Compare January 19, 2024 06:39

cenkalti requested review from lavacat, jmhbnz, chaochn47 and fuweid January 19, 2024 15:00

tjungblu reviewed Jan 19, 2024

View reviewed changes

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

cenkalti force-pushed the snapshot-status branch from ae4b331 to d3a757d Compare January 25, 2024 20:37

serathius reviewed Jan 26, 2024

View reviewed changes

etcdutl/snapshot/v3_snapshot.go Show resolved Hide resolved

serathius reviewed Jan 26, 2024

View reviewed changes

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

serathius reviewed Jan 26, 2024

View reviewed changes

cenkalti force-pushed the snapshot-status branch 2 times, most recently from 574fb2a to 447c25e Compare January 26, 2024 15:39

ahrtr approved these changes Jan 26, 2024

View reviewed changes

cenkalti requested a review from serathius January 26, 2024 15:41

fuweid reviewed Jan 26, 2024

View reviewed changes

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

etcdutl/snapshot/v3_snapshot.go Show resolved Hide resolved

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

etcdutl/snapshot/v3_snapshot_test.go Outdated Show resolved Hide resolved

cenkalti force-pushed the snapshot-status branch 2 times, most recently from 2513bd2 to 7ef0a43 Compare January 26, 2024 16:13

cenkalti force-pushed the snapshot-status branch from 7ef0a43 to 2602928 Compare March 30, 2024 04:49

cenkalti force-pushed the snapshot-status branch from 2602928 to 2ded6d5 Compare June 7, 2024 19:05

k8s-ci-robot added the needs-rebase label Jun 13, 2024

Improve snapshot status

ea46253

Signed-off-by: Cenk Alti <cenkalti@gmail.com>

cenkalti force-pushed the snapshot-status branch from 2ded6d5 to ea46253 Compare June 15, 2024 14:11

k8s-ci-robot added size/L and removed needs-rebase labels Jun 15, 2024

jmhbnz approved these changes Jun 15, 2024

View reviewed changes

jmhbnz merged commit debc8fb into etcd-io:main Jun 15, 2024
48 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve snapshot status #15979

Improve snapshot status #15979

cenkalti commented May 29, 2023

lavacat left a comment

cenkalti commented May 30, 2023

cenkalti commented Jun 8, 2023

chaochn47 commented Jun 8, 2023 •

edited

Loading

jmhbnz left a comment

ahrtr commented Jan 18, 2024

cenkalti commented Jan 18, 2024

cenkalti commented Jan 19, 2024

cenkalti Jan 19, 2024

ahrtr Jan 24, 2024

ahrtr Jan 24, 2024

cenkalti Jan 25, 2024

ahrtr Jan 25, 2024

cenkalti Jan 25, 2024

cenkalti commented Jan 19, 2024

cenkalti commented Jan 19, 2024

cenkalti commented Jan 25, 2024

serathius Jan 26, 2024 •

edited

Loading

cenkalti Jan 26, 2024

ahrtr left a comment

jmhbnz commented Mar 28, 2024

cenkalti commented Mar 28, 2024

cenkalti commented Mar 30, 2024

jmhbnz commented Jun 6, 2024

jmhbnz commented Jun 8, 2024

ivanvc commented Jun 13, 2024

fuweid commented Jun 13, 2024

cenkalti commented Jun 15, 2024

codecov-commenter commented Jun 15, 2024

jmhbnz left a comment

Improve snapshot status #15979

Improve snapshot status #15979

Conversation

cenkalti commented May 29, 2023

lavacat left a comment

Choose a reason for hiding this comment

cenkalti commented May 30, 2023

cenkalti commented Jun 8, 2023

chaochn47 commented Jun 8, 2023 • edited Loading

jmhbnz left a comment

Choose a reason for hiding this comment

ahrtr commented Jan 18, 2024

cenkalti commented Jan 18, 2024

cenkalti commented Jan 19, 2024

cenkalti Jan 19, 2024

Choose a reason for hiding this comment

ahrtr Jan 24, 2024

Choose a reason for hiding this comment

ahrtr Jan 24, 2024

Choose a reason for hiding this comment

cenkalti Jan 25, 2024

Choose a reason for hiding this comment

ahrtr Jan 25, 2024

Choose a reason for hiding this comment

cenkalti Jan 25, 2024

Choose a reason for hiding this comment

cenkalti commented Jan 19, 2024

cenkalti commented Jan 19, 2024

cenkalti commented Jan 25, 2024

serathius Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

cenkalti Jan 26, 2024

Choose a reason for hiding this comment

ahrtr left a comment

Choose a reason for hiding this comment

jmhbnz commented Mar 28, 2024

cenkalti commented Mar 28, 2024

cenkalti commented Mar 30, 2024

jmhbnz commented Jun 6, 2024

jmhbnz commented Jun 8, 2024

ivanvc commented Jun 13, 2024

fuweid commented Jun 13, 2024

cenkalti commented Jun 15, 2024

codecov-commenter commented Jun 15, 2024

Codecov Report

jmhbnz left a comment

Choose a reason for hiding this comment

chaochn47 commented Jun 8, 2023 •

edited

Loading

serathius Jan 26, 2024 •

edited

Loading