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

Ensure Vault can access the underlying snapshotInstaller. #501

Merged
merged 1 commit into from Apr 27, 2022

Conversation

ncabatoff
Copy link
Contributor

This code: https://github.com/hashicorp/vault/blob/main/physical/raft/fsm.go#L785
is broken since #490. To allow Vault to upgrade to a current Raft release, I'm introducing a new interface that exposes the underlying ReadCloser clients passed in for a restore.

@ncabatoff ncabatoff requested a review from rboyer April 26, 2022 13:52
@@ -119,12 +119,28 @@ func newCountingReader(r io.Reader) *countingReader {

type countingReadCloser struct {
*countingReader
io.Closer
readCloser io.ReadCloser
Copy link
Member

@rboyer rboyer Apr 26, 2022

Choose a reason for hiding this comment

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

So the gist here is that the vault restore code expected that the io.ReadCloser arg passed to FSM.Restore was of a known type before, so it could call methods other than just Read and Close on it, and since #490 wrapped your io.ReadCloser you need a way to unwrap it. I guess this also means the restore progress reporting is going to be strange or non-existent for vault since you won't be using the Read method provided by countingReader at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, true, hadn't thought about that. Hmm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually no, I think it won't be an issue. We're only casting to snapshotInstaller so we can call the Install method on it to rename the file in place. The writing of the file to disk is still done by the raft library, so we should still see progress reporting. I'll verify this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Verified:

2022-04-26T13:39:07.959-0400 [INFO]  raft-c050bf27-97cb-224d-9c25-e8c4f0526924: snapshot restore progress: id=2-204-1650994747692 last-index=204 last-term=2 size-in-bytes=0 read-bytes=0 percent-complete="NaN%"
...
2022-04-26T13:39:08.079-0400 [INFO]  raft-ab3535b0-0eef-3c41-8ceb-6bf7a3cf4385: snapshot network transfer progress: read-bytes=1880 percent-complete="100.00%"

Copy link
Member

@rboyer rboyer left a comment

Choose a reason for hiding this comment

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

LGTM

@ncabatoff ncabatoff merged commit 7fc5a41 into main Apr 27, 2022
@ncabatoff ncabatoff deleted the allow-restore-to-access-readcloser branch April 27, 2022 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants