Mount snapshots on Windows #8043
Conversation
|
Hi @gabriel-samfira. 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. |
gabriel-samfira
force-pushed
the
wcow_mount_layers_rebased
branch
2 times, most recently
from
d5f6575 to
03ffa12
Compare
|
Dropped commit gabriel-samfira@6dae7f9 as we're using the bind filter, which doesn't use junction points. Thus, this issue should not manifest. Also, with the change to continuity (containerd/continuity#212), we'll also get a proper diff between layers. |
|
The snapshot tests require the |
|
This commit: 03ffa12 Adds two new files, one of which is generated code via @helsaawy Do you think something like this should be added to |
|
@gabriel-samfira Ping me when this is ready! |
You bet! The more reviews and scrutiny this gets, the less stressed I'll be that I made a silly mistake somewhere which will blow up later 😄 . |
gabriel-samfira
force-pushed
the
wcow_mount_layers_rebased
branch
6 times, most recently
from
d39ab8e to
b0a9d5f
Compare
|
Hi @dcantah ! If you have the time, feel free to review this branch. Sadly, I didn't get all the "metadata" files in the previous PR to continuity. I opened a new one here: containerd/continuity#219. In this branch, I have a temporary hack that points to my continuity branch, but other than that, this should be ready to review. Have a look when you get a chance! Thanks! |
|
Woo! I'll definitely carve out some time this week |
|
Integration tests seem to have passed! 😄 . Well...everything aside from a couple of Will mark this ready for review as soon as containerd/continuity#219 merges. |
gabriel-samfira
force-pushed
the
wcow_mount_layers_rebased
branch
from
f506c0f to
2c79882
Compare
|
Oh, we aren't seeing the test failures that lead to bd51841? Awesome! I wonder if that was actually related to the HCS/WCIFS support, and using Bind Filter has fixed it? Or perhaps there was some other bug that has been fixed in-passing. (I could never reproduce it in hcsshim directly, but I also never ruled out a HCS-internal race condition) Oh. A quick look at the test run suggests we might need to bump the Windows Integration test-run time-limit, it's currently 50m, and one of the four runs hit this in a very late stage (critest execution). The other three are also really close, so I don't think this was a failure or problem, it's just that with the new tests enabled from the snapshot suite, the expected runtime is longer. I'm guessing 60 minutes would be enough, but to avoid false-negatives, perhaps 90m? I am viscerally excited by the progress here. ^_^ |
If it was "File in use" it may have been an older bug. I remember fixing one like that here: Not sure if it is the same one you were seeing back in 2021 😄 . Increasing the timeout sounds prudent (we had to do it before on Windows). Will probably do that in a separate PR. |
@aznashwan was kind enough to open a PR for this here: #8183 Thanks @aznashwan ! |
|
Marking this as ready for review, as there is just one simple PR it depends on, and it would be useful to get more eyes on this sooner rather than later. |
As opposed to a writable layer derived from a base layer, the volume path of a base layer, once activated and prepared will not be a WCIFS volume, but the actual path on disk to the snapshot. We cannot directly mount this folder, as that would mean a client may gain access and potentially damage important metadata files that would render the layer unusabble. For base layers we need to mount the Files folder which must exist in any valid base windows-layer. Signed-off-by: Gabriel Adrian Samfira <[email protected]>
Signed-off-by: Gabriel Adrian Samfira <[email protected]>
gabriel-samfira
force-pushed
the
wcow_mount_layers_rebased
branch
from
f5067be to
93dc6e6
Compare
Signed-off-by: Gabriel Adrian Samfira <[email protected]>
gabriel-samfira
force-pushed
the
wcow_mount_layers_rebased
branch
from
93dc6e6 to
1279ad8
Compare
Signed-off-by: Gabriel Adrian Samfira <[email protected]>
Signed-off-by: Gabriel Adrian Samfira <[email protected]>
kzys
left a comment
kzys
left a comment
There was a problem hiding this comment.
Looks good to me. Only have nits and I'm fine addressing them later.
|
Thank you @kzys ! Addressing the nits now. |
* Improve error messages
* remove a check for the existance of unmount target. We probably
should not mask that the target was missing.
Signed-off-by: Gabriel Adrian Samfira <[email protected]>
Signed-off-by: Gabriel Adrian Samfira <[email protected]>
|
/lgtm |
|
Anyone able to check this in? We have 4 approvals from maintainers |
|
Woo, and I cannot emphasise this enough, hoo! |
There are [insert_favorite_beverage] I need to send your way 😄. |
|
@gabriel-samfira @TBBle thanks for making this and your patience. |
What kind of hazy ipa's do you have in your area? |
|
Thrilled to see this land finally 🥳 |
9 participants
This PR is just a rebase of #4419.
@TBBle is currently on sabbatical so I will try to move the two PRs (this one and the one in microsoft/hcsshim#1637), forward.
All needed context for this PR is available in #4419 .
In addition to the great work done in #4419, this PR makes use of the Bind Filter in Windows, which gives us functionality similar to what we have on Linux via bind mounts. It allows us to avoid some Windows special cases.
Depends on: Add paths to windows metadataFiles continuity#212Note: This PR will fail to build without the above mentioned dependency. Marking it as draft for now. Also, there is still some cleanup I need to do (ex: remove unused functions), which will be addressed shortly.