Mount snapshots on Windows

[TBBle]

This builds on ✔ #4415, on ✔#4399, and on ✔#4395. I think it's much easier to review the PRs in order.

Full credit to @darstahl and @crosbymichael, on whose shoulders I stood to implement this, as well as any other contributors to #2366 and #2287.

This reimplements the bare-minimum of #2366 to support Mount and Unmount under Windows, with the following approach:

• An Active snapshot without a parent is a (read-write) bind mount to a 'Files' subdirectory, implemented as an NTFS symlink.
  • This depends on WCOW Base layer creation and export microsoft/hcsshim#901 to implement Commit on such parentless layers.
  • Importing a base layer is a special case, as it should never be mounted via the localmounter: it's created, imported into, and committed in a single operation.
• An Active snapshot with a parent is a windows-layer mount, exposed by HCS as a volume and mounted as an NTFS volume mount.
• A View snapshot is a temporary read-write child of its Committed snapshot parent, mounted as a windows-layer per the previous point, then thrown away after use. So you can write to the resulting mount point, but it won't affect the target layer's data.
  • This isn't ideal, both because it means tests that check read-only state of mounts fail, and also because it's messy. But AFAIK, NTFS does not offer any kind of read-only Reparse Point that can be used here, so we would need this to be supported by a new filesystem filter, or implemented in WCIFS (the file system filter used to expose existing read-write layers as volumes)
  • This is compliant with the documentation for Snapshotter.View though, which allows this implementation approach.

In order to unmount a windows-layer without access to the original []mount.Mount, we store the original layer path in an Alternative Data Stream on the mount-point. Happily, we do not require the parents list to tear down the stack, so we don't need to store structured data here.

It's a shame, really. If HCS exposed an inverse for hcsshim.GetLayerMountPath (microsoft/hcsshim#909), then we wouldn't need the ADS storage, as Windows exposes an API to get the Volume back from a Volume mount, and that would let me find the Layer in-question.

With a bit of further work to remove some Linux-isms and work around various Go bugs/NTFS behaviours, this lets us enable the Snapshot test suite on Windows.

WIP work remaining:

• Landing WCOW Base layer creation and export microsoft/hcsshim#901, and either integrating or building a follow-up PR for the issue described in WCOW Base layer creation and export microsoft/hcsshim#901 (comment).
• Find and fix or workaround the thing that makes parallel snapshotter test suite runs fail. Probably in hcsshim, or as a platform bug, but maybe we just need a mutex somewhere in our use of hcsshim for now.
• Verify that the fix for my leaked handle in my hcsshim PR actually fixed the RS5 parallel snapshotter test suite bug, and revert the HACK that serialises the test suite.
• Silence the xattr log spam via context.Resource generates unavoidable log-spam on Windows continuity#179
• Upgrade continuity for Correctly ignore error wrapping ErrNotSupported continuity#207. (WIP in this branch using a direct hash-reference until there is an upstream release)
• Unhack the wcow_workaround directory either here, or in Should the default pathDriver walk though volume mount points on Windows? continuity#174
• Possibly pull a bunch of the not-Windows-specific testsuite changes, e.g., paired mount log message, 'Naive', unmounting mounts before trying to manipulate their underlying layers, out into a separate PR, so they can be considered outside of the context of WCOW layers. Although that might make them harder to justify, so that's a "maybe, or maybe not".
• Maybe resolve whatever's causing us to not be able to use the changes from Scratch size customization and UVM scratch creation for WCOW snapshotter #4912 when faking read-only (View) mounts. If it's a platform difference, then it'll go unresolved as long as we support platforms where it doesn't work.
• Get the "snapshot unmount/cleanup" logic from sys.ForceRemoveAll to also apply to snapshots.Windows.TestWindows. Perhaps expose sys.cleanupWCOWLayers and call it from the test cleanup function after closing the snapshotter.
• Land Volume Mount Point management functions wrapped nicely for Go microsoft/go-winio#187 and use it instead of having a local copy in containerd.

This PR delivers a working filesystem layer for moby/buildkit#616, and once it lands, the rest of BuildKit's WCOW support should be internal to BuildKit.

[theopenlab-ci]

Build succeeded.

• containerd-build-arm64 : FAILURE in 2m 13s (non-voting)

[jterry75]

@kevpar - PTAL

[theopenlab-ci]

Build succeeded.

• containerd-build-arm64 : FAILURE in 2m 16s (non-voting)

[jterry75]

@ambarve - PTAL

[TBBle]

The Windows Integration test failures look relevant (possibly something is not unmounting properly?) so I'll look into those when I come back to this.

Edit: Isolated PR #4425 shows the same behaviour, so it definitely needs investigation.

Edit again: Found the problem; unmounting from a non-mount point (or "") is not an error, and containerd relies on this to clean up bundles. Fix coming shortly (pending confirmation on #4425 CI run)

[TBBle]

In case it helps get reviewed faster, I reckon this change could be rebased without #4415, #4399, and #4395, although I haven't tested that.

It would be handy to land this early, because this is the only PR that touches mount/mount_windows.go, which is vendored into BuildKit, and otherwise blocks the ability to implement COPY, ADD, or WORKDIR for WCOW there.

Edit: Opened a draft PR as #4425, so I can get a recheck of the CI failure above, to rule it in or out as a problem with this PR.

[theopenlab-ci]

Build succeeded.

• containerd-build-arm64 : FAILURE in 2m 07s (non-voting)

[theopenlab-ci]

Build succeeded.

• containerd-build-arm64 : FAILURE in 5m 02s (non-voting)

[theopenlab-ci]

Build succeeded.

• containerd-build-arm64 : FAILURE in 4m 58s (non-voting)

[TBBle]

This could probably be made safer using winio.DecodeReparsePoint, so we don't treat a symlink to itself as a mount point. See https://github.com/microsoft/go-winio/blob/3fe6c526287328ec859e2c1258458b685198f204/backuptar/tar.go#L160-L171 for an example usage.

That would allow us to confirm that the mount point's mounted volume does point back to the same scratch layer we read from the alternate data stream.

(See #4419 (comment))

[TBBle]

Per microsoft/hcsshim#852 (comment), use windows.UTF16PtrFromString.

In fact, update this whole file to be the same as the one I added to hcsshim.

Or put together a PR for these and related functions to address microsoft/go-winio#176, and then consume them from there. get microsoft/go-winio#187 landed and consume it from there,

[quangkieu]

Hey @kevpar @ambarve, I wonder if you have time to look into this pull request?

[TBBle]

I had a look at the snapshot test suite, and all the suite's tests assume you can create a snapshot stack from scratch by writing to files on-disk. Looking at the changes to the tests in #2366 to make them work, it seems there's a function hcsshim.ProcesBaseLayer which (if I'm reading this correctly) can turn a directory with a subdir named Files/ into a base-layer.

That might make this feasible by using a bind-mount to Files for parent-less Active snapshots, and then Commit would be using hcsshim.ProcessBaseLayer to make it a valid parent.

Yes, that's it, combined with the files in func Base() Applier from vendor/github.com/containerd/continuityfs/fsttest/file_windows.go. Or at least, hcsshim.ProcessBaseLayer completes successfully.

Helpfully, someone has just dropped a batch of knowledge on me in microsoft/hcsshim#853 (comment) which matches this, and also demonstrates a minimal base-layer export format.

[TBBle]

Converted to draft, see #4415 (comment)

[gabriel-samfira]

I wounder if it wouldn't be worth implementing something using ProjFS (or some other mechanism), and get proper control over how the source is mounted instead of trying to work around the limitations of junctions.

(awesome stuff by the way, and thanks for working on all of this!)

[TBBle]

I was thinking about ProjFS the other day, but since AFAIK it expects you to copy the relevant file from the backing store into the directory it's mounted at to be used by the system, it seems likely to be wasteful if I used it here when I already have the files available on the filesystem via the WCIFS driver.

I think a more-likely improvement would be being able to mount a WCIFS filesystem stack read-only, i.e. with no sandbox. I expect that would require OS-level support, unless there's something hidden in the HCS or computestorage APIs already that would allow this, and we just don't expose it in hcsshim.

Using ProjFS instead of WCIFS to actually mount a layer stack on the other hand would be a fascinating and terrifying exercise. 100% that would be an external snapshotter implementation that would need to be bug-for-bug compatible with WCIFS and anything else used to expose layers into the HCS computer systems (VM, container, or host-based). More like a research project than likely to become the way, I think. And it would still have the problem that the current WCIFS on-disk format has all the files we need already extracted. It already has a triple-the-disk-space issue when exporting a layer (although I reckon once I have the Snapshotter test suite passing here, I can get that down to double-the-disk-space), and using ProjFS would introduce an up-to-double-the-disk-space issue in the mount-time behaviour.

Final thought, the liberal use of "legacy" in the related hcsshim code and discussions suggests that someone might have plans for a different layer-storage format, and that might be more-conducive to something like ProjFS for its host-mount system. But given that this stuff happens with Windows Server support lifecycle iterations, I don't expect any such work to materialise before I land this. (I also didn't expect to not have landed this last year, though, so take my expectations with a grain of salt. ^_^)

[gabriel-samfira]

Yeah, it wouldn't be the best option. I was thinking it might be of use for RO bind mounts, but that's not the only thing that is missing and would require RO access. WCIFS sounds like the path forward, but I am not sure if that filter driver has any flag that would allow setting a RO flag. I don't think it would be a huge technical challenge to add, but it might take a non-trivial amount of time until it propagates to all supported versions of Windows.

[TBBle]

Something else to look into is whether we can replace the symlink/junction-point appear with the Bind Filter.

I had seen that before, but only came across this API (and noticed it supports readonly binds) when just now preparing a hcsshim/WCIFS feature request (microsoft/hcsshim#1520).

We'd still like the WCIFS/hcsshim feature request, of course, since Bind Filter doesn't solve the problem of needing a scratch directory for HcsAttachLayerStorageFilter to initialise WCIFS. But it could be used to optimise the "no parents" cases, as is done in the containerd overlay driver, at least.

[gabriel-samfira]

Which is already a huge plus. It would eliminate the need for a bunch of workarounds. Great find!

What are the blocking issues that remain to be solved to land both this and the hcsshim PR this depends on? If I can, I would like to help out.

[gabriel-samfira]

It seems that continuity PR (reverted the wcow_workaround commit) helps with comparing files, but now fails due to not being able to checksum hidden system files. See test output:

https://paste.samfira.com/public/p/dznNWyKUHCKT9s2rEJPKRhRK

Only SYSTEM has access to those files. Explicitly adding permissions for the user running the tests to access those files, should work. I'm wondering if we should create a list of files/folders that should be ecluded from comparison. $Recicle.Bin comes to mind, as well as things like pagefile.sys. What are your thoughts on this?

[gabriel-samfira]

Setting proper dacl allows us to create a manifest of the entire mount point, and now the fun starts. We have some extra paths inside that mount point that makes the test fail:

PS C:\containerd> sh.exe -c "EXTRA_TESTFLAGS=-test.run=TestSnapshotterClient make integration"
+ integration
Windows test image: mcr.microsoft.com/windows/nanoserver:ltsc2022 , Windows build version: 20348
time="2022-09-27T07:45:07-07:00" level=info msg="running tests against containerd" revision=03536e7d953416c460ab076d497eadade3084ff1.m runtime= snapshotter= version=v1.6.0-715-g03536e7d9.m
time="2022-09-27T07:45:07-07:00" level=info msg="start to pull seed image" image="mcr.microsoft.com/windows/nanoserver:ltsc2022"
time="2022-09-27T07:45:08-07:00" level=info msg=request digest="sha256:09629875cd6ee57fa551626fa2963d8b54718ba9acbf28e7cb5d684a9cb754d7" mediatype=application/vnd.docker.image.rootfs.foreign.diff.tar.gzip size=118131331 url="https://mcr.microsoft.com/v2/windows/nanoserver/blobs/sha256:09629875cd6ee57fa551626fa2963d8b54718ba9acbf28e7cb5d684a9cb754d7"
=== RUN   TestSnapshotterClient
=== RUN   TestSnapshotterClient/Basic
    helpers.go:88: unmount C:\Users\ADMINI~1\AppData\Local\Temp\snapshot-suite-SnapshotterClient-2124266799\work\preparing
    helpers.go:88: unmount C:\Users\ADMINI~1\AppData\Local\Temp\snapshot-suite-SnapshotterClient-2124266799\work\nextlayer
    testsuite.go:216: failure reason: directory diff between C:\Users\ADMINI~1\AppData\Local\Temp\fstest2813155162 and C:\Users\ADMINI~1\AppData\Local\Temp\snapshot-suite-SnapshotterClient-2124266799\work\nextlayer
        + \WcSandboxState
        + \WcSandboxState\Hives
        + \WcSandboxState\Hives\DefaultUser_Delta
        + \WcSandboxState\Hives\Sam_Delta
        + \WcSandboxState\Hives\Security_Delta
        + \WcSandboxState\Hives\Software_Delta
        + \WcSandboxState\Hives\System_Delta
        + \WcSandboxState\initialized
        + \Windows
        + \Windows\System32
        + \Windows\System32\config
        + \Windows\System32\config\DEFAULT
        + \Windows\System32\config\SAM
        + \Windows\System32\config\SECURITY
        + \Windows\System32\config\SOFTWARE
        + \Windows\System32\config\SYSTEM
    helpers.go:69: drwxrwxrwx          0 C:\Users\ADMINI~1\AppData\Local\Temp\snapshot-suite-SnapshotterClient-2124266799
    helpers.go:69: drwxrwxrwx          0 C:\Users\ADMINI~1\AppData\Local\Temp\snapshot-suite-SnapshotterClient-2124266799\root
    helpers.go:69: drwxrwxrwx          0 C:\Users\ADMINI~1\AppData\Local\Temp\snapshot-suite-SnapshotterClient-2124266799\work
    helpers.go:69: drwxrwxrwx          0 C:\Users\ADMINI~1\AppData\Local\Temp\snapshot-suite-SnapshotterClient-2124266799\work\nextlayer

Your current workaround may be needed after all, but this time for different reasons 😄 .

[gabriel-samfira]

It seems the \WcSandboxState\Hives files are layered registry keys. Those are essentially deltas that get applied on top of the base hives similarly to how overlayfs works.

I think it's safe to ignore those when comparing directory diffs.

[TBBle]

I'm surprised WcSandboxState exists at all, I'd have assumed that only existed when the layers are used by a running container. I wonder, are those files all empty? Since no OS has been booted in that image, I can't see how there'd be any "delta" to capture, but maybe a zero delta has non-zero size.

It would make sense to me that for purposes of the snapshotter diff test, \WcSandboxState and \Windows be ignored; I'd say specifically \Windows\System32\config\XXX for the five registry hives (which I expect are trivially bind-mounted from the directory next to Files in the underlying layer image) but then having to ignore the containing directories sounds messy.

An alternative would be to do something clever with the compared-against version, using an applier or something to generate empty Deltas and initialized in WcSandboxState, and copy in the (empty in this case) hives from the underlying layer into Windows\System32\config\. That would mean we know exactly how those directories behave (since we can reproduce the behaviour and get a clean diff) but might tie the tests to a behaviour that varies across OS versions, and also means effectively reverse-engineering that behaviour. I don't think it's worth the effort to take this path (invoking Hyrum's Law in the process), compared to excluding a fairly-fixed list of directories.

A third alternative might be to adapt the tests so that on Windows, the in-image version is working on a subdirectory of the layer mount-point; this would be better than my existing hack as the appliers wouldn't see the different path, so it'd be less intrusive and a more-contained difference.

And another approach, perhaps: What if we ignore all SYSTEM files in the comparison, or even ignore "all files we can't open", as mentioned in #4419 (comment), since I would guess all the WCIFS-injected files are marked this way. This might not pan out as it'll still see the containing directories as new though.

Flowing on from this (and again getting low on the value/effort ratio) if the comparison treated any SYSTEM-marked (or otherwise unreadable) file in the layer as equal to a file of the same path in the fstest directory, irrespective of contents, then a fairly simple applier could be used for the fstest directory to create zero-length dummies for the WCIFS-injected files, without needing to care about their contents. That should be reasonably simple to implement, but I suspect would require another continuity change. (Although I haven't checked the code, sorry...)

[gabriel-samfira]

I think the best way to move forward is to either exclude the folders in the above output, or use a sub-folder. Excluding those paths seems like the better choice.

From what I can tell, HCS creates those diffs when it prepares the layer. I have not checked the contents of the files in WcSandboxState but I suspect those will get created for every layer regardless of whether or not something changes.

We could just chalk it off to magic being done by hcs and just skip those.

As a side note, I opened a PR against your hcsshim branch that replaces the go-hivex dummy files with ones generated by the offline registry library.

[TBBle]

This test disagrees with the API docs, as the latter states that the mount from a View may be read-only, and that writes to the mount must be ignored, not that they must fail.

I think what it should do is if the View result is not read-only (i.e. if the write doesn't fail), request the View again and ensure that the file written does not exist in the second View.

[gabriel-samfira]

The snapshotter tests that use fstest.CheckDirectoryEqualWithApplier() will never work on Windows, due to the post processing that happens on commit, and because of the layered registry keys that get set up during prepare. There is some magic happening on Windows that does not happen on Linux. So we'll need to adapt the tests to exclude the hives that get added during hcsshim.ConvertToBaseLayer() as well as the files from \WcSandboxState.

[kzys]

@TBBle Would you mind if I close this PR? @gabriel-samfira rebased this as #8043.

[TBBle]

Closing in favour of #8043, thanks to @gabriel-samfira for completing this work while I'm (still) on sabbatical.