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.

Sign up for GitHub

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
Merged

Ensure Vault can access the underlying snapshotInstaller. #501

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions progress.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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%"

}

func newCountingReadCloser(rc io.ReadCloser) *countingReadCloser {
return &countingReadCloser{
countingReader: newCountingReader(rc),
Closer: rc,
readCloser: rc,
}
}

func (c countingReadCloser) Close() error {
return c.readCloser.Close()
}

func (c countingReadCloser) WrappedReadCloser() io.ReadCloser {
return c.readCloser
}

// ReadCloserWrapper allows access to an underlying ReadCloser from a wrapper.
type ReadCloserWrapper interface {
io.ReadCloser
WrappedReadCloser() io.ReadCloser
}

var _ ReadCloserWrapper = &countingReadCloser{}