Is something going to be done after this defer at some point in the future? Right now it is not doing anything

I think I had something after the defer in a previous version. Removing it.

Isn't using JSON here still problematic since the comma won't be escaped? What is the main motivation behind encoding with JSON

The intent was just to pack the parent layer paths in a single mount in a way that would be able to get back the same list when mounting. JSON was used because it would still be readable, but any way to Marshal should be ok, as long as all characters in the path are OK.

I don't think comma is a problem, as on Windows this is not actually used by a mount syscall similar to Unix, but rather is parsed by containerd and passed as extra info to HCS to assemble the layer. I might be missing somewhere where this struct is serialized though.

Fair enough, the serialization from go array should only go to protobuf types and back. We have a ctr command which gives the mount command, but we can just have it output something else on Windows since a Unix mount command doesn't make sense here.

What is the expense of calculating this on Windows? Full walk?

The size of the layer will come for free (ish) when I add the followup PR which implements Diff and redo Commit to build an actual layer that works for both process and hyperv isolation modes by diffing the current layer from the parent, and creating a new RO layer.

The size in usage is the one where I still need to work out what to do, and if we even have permission to do a full walk in all cases.

What does a rename do on Windows?

Just changes the name, doesn't do any actual data moves (very cheap). The intent is to have a short transaction, and do the heavy lifting outside the transaction.

Also what is the cost of this destroy? I have some in progress work to make this asynchronous, it would probably be good to do here too

see #1833

As I alluded to in the above comment, this is the costly part of remove. Async would be really great.

I am curious why the parentPath gets passed in twice, once as an argument and again as the first element of the path array?

The function is just a direct map to the platform function, and the argument is not actually used in the platform currently, but can't be changed, as changing platform function signatures without adding a new name is not fun for anyone.

So just legacy.. If I get a chance, I'd like to fix the hcsshim function to not require it, but that requires a breaking change.

Is this for setting quotas?

Yes, the default quota is 20GB (since it needs to be stored in a VHD, it cannot be unbounded). It can be expanded to larger than 20GB, but cannot be shrunk.

Do you want to have those as metrics?

I don't think Windows currently supports metrics in containerd. I don't think these are needed as metrics, but it is something I would love to get working on Windows!

Using prometheus, it should just be a matter of defining and registering the metrics :)

from memory this look like duplicated code from other places in the repo.

Yeah, there's a chunk of shared code here. Is there a good place to store code shared between packages in containerd that might not have use outside containerd? This could be something like func IsCompressedTar(desc ocispec.Descriptor) (bool, error), but I don't see a good place to have that function exist.

Maybe somewhere in the image package?

nit: no need for the () version

fixed

nit: this is only used in snashots/windows/widnows.go it could just be in that file.

removed it, as logging the rollback failures is actually not very useful at all (it only fails if the transaction is already committed, at which point we don't care)

I guess (from reading getFileSystemType()) root[0] is the drive letter. https://golang.org/pkg/path/filepath/#VolumeName would make this more obvious.

Yep. That is much clearer. Didn't know about that function.

This if is unnecessary

return info, err

fixed.

What's the purpose of tracking commited?

Looking at the implementation of Tx.Rollback I don't think this error is ever relevant. The only error is "transaction was already closed", which I think would only occur if commited == true, so this will never log anything.

Maybe remove the committed, and ignore the error from Rollback() ? Then you can always just defer tx.Rollback(), which seems to be what other implementations are doing.

Makes sense. For some reason I thought that rollback shouldn't be called after a commit, but I don't know why. Changing this to be similar to other implementations

Actually, the other snapshotters are doing almost the exact same thing such as https://github.com/containerd/containerd/blob/master/snapshots/overlay/overlay.go#L290 (bool rollback instead of bool committed, but the exact same idea) and https://github.com/containerd/containerd/blob/master/snapshots/overlay/overlay.go#L204 (setting t = nil to abandon the rollback instead of using a bool).

Same thing in naive. Looks like the other snapshotters should also be fixed to just use defer tx.Rollback() regardless to me.

return errdefs.ErrNotImplemented?

yep. fixed.

nit: this can be done after m.GetParentPaths() to avoid allocating the memory if not necessary

Using prometheus, it should just be a matter of defining and registering the metrics :)

nit: could be a single var block with the above

nit: would do that first to avoid loading the dll if not needed

This target is completely unused, but that makes using this function kind of awkward. The concern is that code cannot rely on calling this function to get access to the data through the mount. This could complicate builder use cases which are trying to build off the snapshot pattern. Could this code be unified with the code in windows/hcsshim.go which does mounting to provide the mount at a destination path? Doing the equivalent of a mount + bind mount to achieve this would be fine if that is even possible.

Agreed it is awkward to not mount at target.

It is not possible to specify the mount location currently, as the layer is mounted as a volume on the host, not at a path on the host. I think it might be possible to mount the resulting volume on the host at a given path, but this would only be delaying solving the same problem again later, as LCOW layers cannot be mounted on the host regardless due to limitations in NTFS. See #1359 for more context on this.

On Windows, it makes much more sense to get the mounted path, rather than provide a path to mount at. I'm not sure if mounting the volume on the host would fix this for WCOW, but I can attempt if we think it is worth fixing it here before we deal with the LCOW situation.

Let's discuss further later, possibly start to include @tonistiigi in our sync ups