Improve dmverity-vhd -d performance #2086
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DomAyre
force-pushed
the
tarball
branch
3 times, most recently
from
e6110e9
to
2d64f26
Compare
Signed-off-by: Dominic Ayre <dominicayre@microsoft.com>
Signed-off-by: Dominic Ayre <dominicayre@microsoft.com>
Signed-off-by: Dominic Ayre <dominicayre@microsoft.com>
Signed-off-by: Dominic Ayre <dominicayre@microsoft.com>
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create tar file: %w", err) | ||
| } | ||
| defer tarFile.Close() |
There was a problem hiding this comment.
It looks like defer os.Remove(tarFile.Name()) is also needed to clean up
Signed-off-by: Dominic Ayre <dominicayre@microsoft.com>
Signed-off-by: Dominic Ayre <domayre@outlook.com>
There was a problem hiding this comment.
looks good! I'm good as long as a fallback/option gets added if there isn't enough disk space to save the tar file.
update: and double-checking building on windows works since that's what the release pipeline uses
|
This PR is superseded by #2089 which doesn't need to compromise on disk space, I suggest we close this |
Signed-off-by: Dominic Ayre <domayre@outlook.com> Signed-off-by: Dominic Ayre <dominicayre@microsoft.com>
helsaawy
approved these changes
Apr 2, 2024
|
Closed this as it's superseded by #2089 |
anmaxvl
pushed a commit
that referenced
this pull request
Apr 12, 2024
The current implementation of dmverity-vhd -d has to make one of the following tradeoffs: Runtime: By default this option calls the docker daemon to fetch the entire image for each layer as it doesn't provide an endpoint to get a specific layer Memory: The user can include a -b option that makes this call buffered, keeping the image in memory the whole time, this is much faster but at the cost of keeping the whole image in memory, which is a problem with runners with low memory #2086 Proposed a new tradeoff of disk space, by saving the image to disk and accessing the layers locally, this is a problem for runners with smaller disks as the image is stored twice. This solution makes a single request to the docker daemon, and processes both the layer hashes and the manifest to assign layer numbers in a single pass, making it performant in all three aspects. --------- Signed-off-by: Dominic Ayre <dominicayre@microsoft.com> Signed-off-by: Dominic Ayre <domayre@outlook.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When using the
-doption fordmverity-vhd, HTTP requests are made for each layer of the image. On a 2 core codespace runner, this takes roughly 10-15 seconds per layer, and ends up being the vast majority of the runtime especially for images with many layers.To fix this, this PR instead uses the docker package to save an image into a tar file then follows the same code path as if the user used the
-tflag to specify a local tar file. Whenv1.Layer.Uncompressed()is called, it just opens the file instead of making a new HTTP request viadaemon.(*imageOpener).saveImage(), saving the request time of ~10-15 seconds per layer.On the previously mentioned 2 core runner, this results in execution time going from ~150 seconds to ~20 seconds for a simple 9 layer image.