Hi @ambarve. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

I see that mount/mount_windows.go is still WCIFS-only. Is it intentional? You can end up there by multiple codepaths, for example func (b *Bundle) Delete() in runtime/v2/bundle.go through cleanupAfterDeadShim. How this is supposed to work when CimFS snapshotter is used?

We just discussed the same topic in #8789: containerd has several mount/unmount codepaths that bypass the snapshotter and ended up with "containerd snapshotter API needs to be improved". But in that case it can be delayed (there's only one snapshotter anyway). But I don't see how current logic can work with WCIFS+CimFS.

@slonopotamus CimFs works evne without making any changes to the existing mount APIs because

1. we currently don't support layer extraction / image building with CimFs. Containerd primarily mounts the snapshots for those operations and since CimFs doesn't support it, this isn't a problem. (But I agree that attempt to mount CimFs snapshots should throw a proper error)
2. During container cleanup hcsshim ensures that all the mounted CimFs snapshots are properly cleaned.

After thinking about this more, I am going to update this PR to include the mount/unmount code for CimFs snapshots. That will simplify hcsshim cleanup logic too.

Just in case: I'm not insisting on any changes in current PR, I'm just trying to build a mental model about all this mount stuff in my head)

@AkihiroSuda & @slonopotamus, I have thought about the Mounts of CimFs based snapshots and I have decided to not include support for Mounting/Unmounting of CimFs based snapshots in this PR.
Currently, the windows snapshotter's Mount/Unmount functionality in containerd is only used during the image creation process i.e. those Un/mount functions aren't used at all during a container's startup and teardown process. Mounting/Unmounting of container image layers is completely handled by hcsshim as of now. Since, we currently don't support export of CimFs layers, building windows container images with CimFs layers isn't an option anyway and hcsshim handles the Mounting/Unmounting of CimFs layers for container startup/teardown. Adding the CimFs snapshot mount/unmount functionality in containerd won't provide any benefit right now.
Moreover, as mentioned earlier, Mounting/Unmounting of windows snapshots done by containerd & hcsshim are completely independent. i.e. if a layer is mounted by hcsshim, containerd has not idea about that mount and cannot clean it up in case the shim process crashes. This means we probably need a generic mount manager type of solution which will allow containerd to keep track of all the mounts (including the mounts made by the shim processes). However, design and implementation of such a mount manager plugin will take more time and there is no real advantage of holding this PR off until then.

Let me know what you think.

P.S: This PR is still marked as WIP because the hcsshim commit that this PR vendors in is still from my private branch. But rest of the code is ready to review.

I like the idea of combining the cimfs snapshotter with the existing windows snapshotter. Originally, we created a separate snapshotter because the cimfs snapshotter used to automatically mount the cim whenever a scratch layer was created with a cim based parent layer (hcsshim didn't mount cims in that case). But now that we don't have this, I am open to the idea of combining these two snapshotters. Let me look into it a bit more.

When you say, "The current windowsSnapshotter can't export/diff imported-from-content-store layers either", what do you mean? A snapshotter isn't responsible for generating the diff, right? For now, the only intended use of this snapshotter is for imported image layers. We currently don't support exorting a cim based layer or a scratch layer (created with UnionFS) to tar but we do plan to add that support in the near future.

Mounting support, especially in a way that handles both legacy (WCIFS) & CimFS based layers is somewhat complicated. For example, current mount support for windows snapshots is only used for locally mounting the snapshots (for image builds etc.). It doesn't handle the snapshot mounts by a shim process. Plus, even if we add the mount support for CimFS based snapshots in containerd right now, that won't be of much use as layer export isn't supported. That's why, we plan to get this PR merged in first and then continue working on adding layer export & mount support.

Btw, I would like to clarify that we won't be using WCIFS with CimFS at all. CimFS will be used with UnionFS.

When you say, "The current windowsSnapshotter can't export/diff imported-from-content-store layers either", what do you mean? A snapshotter isn't responsible for generating the diff, right?

What I mean is that even with the existing WCIFS-based Snapshotter, when you import a layer from the content store, you only have the WCIFS read-only layer (files-on-disk). And hcsshim's v1 layer API can only generate a tar stream from diffing a WCIFS read-write layer (VHDX file) against its parent read-only layer, not from a read-only layer against its parent read-only layer.

So yeah, the Snapshotter doesn't create the diff, I just tie them together in my head. Frankly, I feel like a Snapshot plugin should be able to provide tarstream handlers in a neater way than the existing AsWindowsContainerLayer/applyWindowsContainerLayer hack. Surely there are or are going to be other snapshotters that can generate a tarstream diff more efficiently than the naive walking-diff. (btrfs snapshots include diff-output support using btrfs send but it might not be suitable anyway). (Thinking about that, I suspect that hack was wrong-headed, and the relevant code should be moved directly into the windows differ plugin, since that's the only thing that activates those code paths, and I suspect the remaining dependencies on the rest of the archive library is minimal and/or public symbols. Separate refactoring though. I wish I'd thought of that at the start of the summer holidays, not immediately after the finished.)

Anyway, see

It's possible hcsshim's v2 layer API (computestorage) allows exporting from diffing two WCIFS read-only layers (there's a constant that suggests this is possible), but I never got computestorage's hcsshim projection able to do anything before I went on sabbatical, so I never tried it.

Your comment makes sense to me. We definitely need export before mount (I'm somewhat familiar with the ordering of features needed here. ^_^) so I'm mostly interested in what's future-feasible versus what's impossible, in terms of how this feature is positioned/advertised/used.

I was thinking about this, and it seemed like the CimFS C++ API docs implied that the intent was that a single CIM would hold multiple layers, e.g. the deduplication features and forks, but this implementation is using a CIM-per-layer?

I thought the multiple layers in one big CIM file would be nice, because that would deduplicate between multiple patch versions of the Windows OS second layer, which I suspect is 95% identical between adjacent patches; I haven't checked this though, so that's ±95%.

I will have to investigate a bit more to find out if currently there is a way of generating a diff from two read-only layers. But for CimFs, we do plan on adding that support i.e. we will add a way to read a layer CIM into a tar.

CimFs is designed to deduplicate layer data at the block level. It is also designed to efficiently merge multiple CIMs to generate a merged view of multiple layers. So it makes more sense to create an individual CIM per layer. That way multiple images with common layers, can also reuse the common CIMs. Even if multiple layers of an image contain duplicate files (or files with very minor changes), CimFs should be able to deduplicate those blocks files.

Edit:
My earlier comment about CimFs deduplicating at block level wasn't accurate. Currently, CimFs only dedups at the file level. (However, block based deduplication might be added in the future).

In general, I think the interface between containerd and the hcsshim code (primarily the ociwclayer/cim package) should be less coupled and more explicit in how it functions.

For instance, rather than passing a path to a directory in the CimFS mount, and assuming that for directory <id> there is a cim at cim-layers/<id>.cim, I think it would be better to explicitly give the path to the cim file in the mount.

I hope that this can reduce coupling between the two packages, e.g. so that they no longer need to agree on a common directory structure with cim-layers. This will also make it easier if we want to reorganize the filesystem structure for the CimFS snapshotter, since it can change without needing to update hcsshim code.

The way I view this is the code in hcsshim should be written to be generic as to where it's being used (for instance, it doesn't need to know what a "snapshotter" is or how it's organized). Then the containerd code enforces the specific structure and organization that it wants for the snapshotter.

In general, I think the interface between containerd and the hcsshim code (primarily the ociwclayer/cim package) should be less coupled and more explicit in how it functions.

For instance, rather than passing a path to a directory in the CimFS mount, and assuming that for directory <id> there is a cim at cim-layers/<id>.cim, I think it would be better to explicitly give the path to the cim file in the mount.

I hope that this can reduce coupling between the two packages, e.g. so that they no longer need to agree on a common directory structure with cim-layers. This will also make it easier if we want to reorganize the filesystem structure for the CimFS snapshotter, since it can change without needing to update hcsshim code.

The way I view this is the code in hcsshim should be written to be generic as to where it's being used (for instance, it doesn't need to know what a "snapshotter" is or how it's organized). Then the containerd code enforces the specific structure and organization that it wants for the snapshotter.

Discussed this with Kevin offline. We want to get initial CimFS changes merged before containerd 2.0 release. To avoid any further delays for that plans we will merge the CimFS changes without any further refactoring of the layer management code in hcsshim. However, as soon as these changes are merged, we will make additional changes to refactor layer management functions in hcsshim so that they allow a more flexible interface for different types of snapshotter Mounts when running containers.

PR needs rebase.