-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add extraction progress output to GetFSFromImage/GetFSFromLayers #11
Conversation
Whoops, just noticed this:
I'll push a fix later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits, but LGTM otherwise. I don't need to review again 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM
Left a couple nits & suggestions
Q: did you consider passing a io.TeeReader
to the extraction function? That might give you insight into the progress when io.Copy
occurs here (I haven't validated this).
pkg/util/fs_util.go
Outdated
ReadCloser: r, | ||
after: time.Second, | ||
print: func(int64) { | ||
logrus.Infof("Extracting layer %d...", i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside: I think it'd be even more useful to have a timestamp at the start of every log line so we could see how long things took after the fact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could consider enabling that on the logger lever, but I don't think we should diverge for these log lines. Usually timestamps would be available e.g. via container logs or similar. Not against showing an elapsed human readable time, but then again I want to keep this simple 😄.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I'm not following what this would achieve? We're already monitoring reads via the printing reader and writes we can't know anything about. |
In |
@dannykopping Ah, I see. This is something I wanted to do, but as I mentioned in the PR description, it's not possible. We only know the compressed size and the number of uncompressed bytes read. 😔 Correlating these will always be inaccurate. |
This PR allows printing extraction progress during unpack. Since we can't access the underlying reader for the compressed data, we can only print accurate progress at every layer change. Instead we print
...
every 1 second (triggered via read) to indicate that the extraction is still in progress.Fixes coder/envbuilder#124