Skip to content
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

*: make sure snapshot save downloads SHA256 checksum #11896

Merged
merged 3 commits into from
May 16, 2020

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented May 15, 2020

We've seen some cases where hash was missing from snapshots (still investigating).

This makes sure clientv3/snapshot downloads checksum from the server.

@gyuho
Copy link
Contributor Author

gyuho commented May 15, 2020

@codecov-io
Copy link

Codecov Report

Merging #11896 into master will increase coverage by 0.57%.
The diff coverage is 88.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11896      +/-   ##
==========================================
+ Coverage   65.65%   66.23%   +0.57%     
==========================================
  Files         403      403              
  Lines       37145    37160      +15     
==========================================
+ Hits        24388    24613     +225     
+ Misses      11249    11034     -215     
- Partials     1508     1513       +5     
Impacted Files Coverage Δ
clientv3/maintenance.go 42.85% <0.00%> (ø)
clientv3/snapshot/v3_snapshot.go 57.63% <76.92%> (-0.77%) ⬇️
etcdserver/api/v3rpc/maintenance.go 70.80% <100.00%> (+3.58%) ⬆️
auth/range_perm_cache.go 51.42% <0.00%> (-17.15%) ⬇️
clientv3/concurrency/mutex.go 61.64% <0.00%> (-5.48%) ⬇️
clientv3/leasing/txn.go 88.09% <0.00%> (-3.18%) ⬇️
clientv3/client.go 72.30% <0.00%> (-2.34%) ⬇️
etcdserver/v3_server.go 72.70% <0.00%> (-1.92%) ⬇️
etcdserver/api/v2http/client.go 88.32% <0.00%> (-1.28%) ⬇️
raft/raft.go 90.37% <0.00%> (-0.64%) ⬇️
... and 20 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 3488e25...cf8e8a6. Read the comment docs.

Copy link

@wongma7 wongma7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the great code comments!

br += int64(n)
sent += int64(n)

// if total is x * snapshotSendBufferSize. it is possible that
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appreciate the comment.

// No, server sends EOF with an empty response
// after it sends SHA digest at the end
// TODO: investigate why this had happened...
if resp == nil {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we think the code should be unreachable, is it possible to log a warning here?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be unreachable because if the server sends EOF it will get consumed by pw.CloseWithError(err) which treats EOF as a non-issue i.e. pw.CloseWithError(io.EOF) is basically the same as pw.Close()

@gyuho gyuho force-pushed the snapshot branch 2 times, most recently from c8e7d9c to eac0b25 Compare May 15, 2020 03:55
gyuho added 3 commits May 14, 2020 22:57
Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
I've seen some cases SHA blobs are missing (still investigating).
Adding a check to make sure snapshot save always downloads
hash digests for integrity checks.

Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
@gyuho
Copy link
Contributor Author

gyuho commented May 15, 2020

@jpbetz @jingyih I would like to get this in for our upcoming 3.4 release.

Copy link
Member

@spzala spzala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gyuho thanks, lgtm and great job with all the doc/log messages!

@gyuho gyuho merged commit 31b6e5a into etcd-io:master May 16, 2020
@gyuho gyuho deleted the snapshot branch May 16, 2020 02:41
gyuho added a commit that referenced this pull request May 17, 2020
 ref. #11896

Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
gyuho added a commit that referenced this pull request May 17, 2020
 ref. #11896

Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
gyuho added a commit that referenced this pull request May 18, 2020
ref. #11896

Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
gyuho added a commit that referenced this pull request May 18, 2020
ref. #11896

Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
gyuho added a commit that referenced this pull request May 18, 2020
ref. #11896

Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
gyuho added a commit that referenced this pull request May 18, 2020
ref. #11896

Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
gyuho added a commit that referenced this pull request May 18, 2020
ref. #11896

Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
renormalize added a commit to renormalize/etcd-client that referenced this pull request Aug 16, 2024
* Snapshot hash is sent after the first occurence of `r.remaining_bytes() == 0`.
  See etcd-io/etcd#11896 for more information.

* If the snapshot operation is performed as shown in the example previously,
  restarting the etcd on the stored data from this won't succeed unless etcd
  is started with `--skip-hash-check=true`.
  This isn't necessary anymore since the final `msg.message().await` fetches
  the snapshot hash information.
renormalize added a commit to renormalize/etcd-client that referenced this pull request Aug 16, 2024
* Snapshot hash is sent after the first occurance of `r.remaining_bytes() == 0`.
  See etcd-io/etcd#11896 for more information.

* If the snapshot operation is performed as shown in the example previously,
  restarting the etcd on the stored data from this won't succeed unless etcd
  is started with `--skip-hash-check=true`.
  This isn't necessary anymore since the final `msg.message().await` fetches
  the snapshot hash information.
davidli2010 pushed a commit to renormalize/etcd-client that referenced this pull request Aug 18, 2024
* Snapshot hash is sent after the first occurance of `r.remaining_bytes() == 0`.
  See etcd-io/etcd#11896 for more information.

* If the snapshot operation is performed as shown in the example previously,
  restarting the etcd on the stored data from this won't succeed unless etcd
  is started with `--skip-hash-check=true`.
  This isn't necessary anymore since the final `msg.message().await` fetches
  the snapshot hash information.
davidli2010 pushed a commit to etcdv3/etcd-client that referenced this pull request Aug 18, 2024
…n. (#83)

* Snapshot hash is sent after the first occurance of `r.remaining_bytes() == 0`.
  See etcd-io/etcd#11896 for more information.

* If the snapshot operation is performed as shown in the example previously,
  restarting the etcd on the stored data from this won't succeed unless etcd
  is started with `--skip-hash-check=true`.
  This isn't necessary anymore since the final `msg.message().await` fetches
  the snapshot hash information.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants