Optimize Windows and LCOW snapshotters to only create scratch layer on the final snapshot #4643
Conversation
|
Hi @dcantah. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
I'd like to get feedback on this PR as assuming the key format will stay the way it is to solve this is a gross hack but I can't think of another way to handle this without bringing labels into the equation. |
dcantah
force-pushed
the
feedback-lcow-snapshotter
branch
from
c715bb4 to
bfce5c9
Compare
|
Build succeeded.
|
|
ping @kevpar |
snapshots/lcow/lcow.go
Outdated
There was a problem hiding this comment.
@dmcgowan - Can we get this to be a define somewhere to make this less fragile?
There was a problem hiding this comment.
Talked with @dcantah about adding a snapshot label when the active snapshot is just for extraction.
There was a problem hiding this comment.
Correct, I didn't get around to changing this PR yesterday but another approach to this is we could have a WithScratchSnapshot (or whatever we want to call it) snapshotter option we can pass in for the Prepare call for the r/w sandbox.vhdx snapshot that would just set a label we can check in the snapshotter to determine whether to make the scratch instead. This seems a little better than relying on the "extract-" key format staying static
There was a problem hiding this comment.
Ok after implementing it, I don't like the label approach either... Here's a branch with the work dcantah@d570e89 but let's see what @dmcgowan says about this approach first.
There was a problem hiding this comment.
@dmcgowan / @crosbymichael - Do either of you have an opinion on this or should we just stick with the key approach here? Thanks for the insights!
There was a problem hiding this comment.
I don't have an opinion on it. I think we have done keys like this is the past so seems ok to me
|
@dcantah - Thanks for doing this! |
|
@jterry75 For sure :) We can benefit from this for WCOW also as the exact same thing ends up happening past the base layer. I'll likely add that also today |
|
@cpuguy83 do you wanna review this one? |
dcantah
force-pushed
the
feedback-lcow-snapshotter
branch
from
bfce5c9 to
dfd9f9f
Compare
|
Build succeeded.
|
…n the final snapshot For LCOW currently we copy (or create) the scratch.vhdx for every single snapshot so there ends up being a sandbox.vhdx in every directory seemingly unnecessarily. With the default scratch size of 20GB the size on disk is about 17MB so there's a 17MB overhead per layer plus the time to copy the file with every snapshot. Only the final sandbox.vhdx is actually used so this would be a nice little optimization. For WCOW we essentially do the exact same except copy the blank vhdx from the base layer. Signed-off-by: Daniel Canter <[email protected]>
dcantah
force-pushed
the
feedback-lcow-snapshotter
branch
from
dfd9f9f to
a91c298
Compare
dcantah
changed the title
|
Build succeeded.
|
|
LGTM |
5 participants
For LCOW currently we copy (or create) the scratch.vhdx for every single snapshot
so there ends up being a sandbox.vhdx in every directory seemingly unnecessarily. With the default scratch
size of 20GB the size on disk is about 17MB so there's a 17MB overhead per layer plus the time to copy the
file with every snapshot. Only the final sandbox.vhdx is actually used so this would be a nice little
optimization.
For WCOW we essentially do the exact same except copy the blank vhdx from the base layer.
Signed-off-by: Daniel Canter [email protected]