[WIP] Implement Windows snapshotter and differ #1613

darstahl · 2017-10-06T23:19:05Z

This is a work in progress (nearly finished) implementation of snapshotter and differ on Windows and the corresponding changes in ctr and Windows runtime to use the new snapshotter/differ.

PS H:\git\go\src\github.com\containerd\containerd> ctr.exe pull docker.io/microsoft/nanoserver:latest
docker.io/microsoft/nanoserver:latest:                                            resolved       |++++++++++++++++++++++++++++++++++++++|
manifest-sha256:8b4ab2db6fd93063b50233a17fc0084015a3fcc774d7b17528bb4baf97aa7949: done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:5cd49617cf500abea7b9f47d82b70455d816ae6b497cabc1fc86a9522d19a828:    done           |++++++++++++++++++++++++++++++++++++++|
config-sha256:00f2c6e8b100efb59b4febf77b09b2588b3fc55f45e2e035e8208e1cceb94339:   done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:bce2fbc256ea437a87dadac2f69aabd25bed4f56255549090056c1131fad0277:    done           |++++++++++++++++++++++++++++++++++++++|
elapsed: 25.0s                                                                    total:  374.8  (15.0 MiB/s)
unpacking sha256:8b4ab2db6fd93063b50233a17fc0084015a3fcc774d7b17528bb4baf97aa7949...
done
PS H:\git\go\src\github.com\containerd\containerd> ctr.exe run --rm docker.io/microsoft/nanoserver:latest test
Microsoft Windows [Version 10.0.14393]
(c) 2016 Microsoft Corporation. All rights reserved.

C:\>exit
exit
PS H:\git\go\src\github.com\containerd\containerd>

Work left to be done is as follows:

cc @mlaventure @dmcgowan @stevvooe @crosbymichael

Signed-off-by: Darren Stahl [email protected]

AkihiroSuda · 2017-10-07T07:45:23Z

snapshot/windows/windows.go

if s := strings.ToLower(fsType); s != "ntfs" { return nil, errors.Wrapf(errdefs.ErrInvalidArgument, "%s is on an non-NTFS volume (%s) - only NTFS volumes are supported", root, s) }

AkihiroSuda · 2017-10-07T07:48:06Z

mount/mount_windows.go

Question: why single-mount is unimplementable?
If unimplementable, please add a comment to the source code?

I'll add a comment. This is due to the filesystem filter on Windows. When accessing a file in an underlying layer, there needs to be a target in the current layer, so a hardlink is created, pointing to the lower layer file (essentially). This means that without the lower layers, the current layer would be full of dead links. As a result, mounting a single layer would be all but useless, so the only API exposed to mount layers on Windows requires all layers.

AkihiroSuda · 2017-10-07T07:48:52Z

snapshot/windows/windows.go

Is this constant can be defined in hccshim package?

If not, can we have it exported to use outside of this package (I've left a comment a bit above asking to add a constant)

Yes, this will be a constant in hcsshim package and not hard coded.

mlaventure

Looks like a promising start! 🎉

mlaventure · 2017-10-09T15:43:03Z

client_windows_test.go

you can just remove that function, it does nothing on other platforms either.

mlaventure · 2017-10-09T15:44:43Z

differ/windowsdiffer/differ_windows.go

nit: merge in the var () block below

Done. FYI I plan to move some of the responsibility here to hcsshim, so the differ will still be undergoing some largeish changes

mlaventure · 2017-10-09T15:45:48Z

differ/windowsdiffer/differ_windows.go

nit: s/NewWindowsDiff/It/

also, is that true even unix FS? (e.g. zfs)?

copy paste error. This is expected to only work on NTFS (and is not a generic implementation, but a Windows container specific one)

mlaventure · 2017-10-09T15:47:46Z

differ/windowsdiffer/differ_windows.go

This part (all the way down to digester :=...) should probably be extracted into an utility function.

/cc @dmcgowan @stevvooe to see if it makes sense

Actually once the archiver can get access to the parent ctx in a generic way, I guess this would just be a copy of walkingDiff.Apply()

mlaventure · 2017-10-09T15:51:45Z

differ/windowsdiffer/differ_windows.go

nit: just a preference, I think a switch construct here would make it easier to read (feel free to ignore if you like it that way 😇)

mlaventure · 2017-10-09T16:13:55Z

snapshot/windows/windows.go

do we need to call t.Rollback() here? maybe put it in a defer.

I think a Rollback is needed. Other snapshotters have the same function without a rollback (I see a few missing Rollbacks in Naive and Overlay actually). I think we might need to do a pass on all snapshotters.

I'm not sure I like the defer, as some functions have a return t.Commit(), which looks fine, and could easily be introduced again if changed to err = t.Commit(); return err but would not trigger the defer on failure. I'm not sure what the pattern should be. Maybe a defer with an explicit variable such as doNotRollback which must be set on a Commit succeeding?

I think that if the function returned are named, you should be able to rely on this and only rollback if err != nil.

mlaventure · 2017-10-09T16:23:54Z

snapshot/windows/windows.go

mlaventure · 2017-10-09T16:26:51Z

snapshot/windows/windows.go

shouldn't this be done later? Otherwise we're not going to rollback the transaction here.

mlaventure · 2017-10-09T16:28:43Z

snapshot/windows/windows.go

todo missing?

ID is only needed to get the disk usage. Removed.

mlaventure · 2017-10-09T16:29:30Z

snapshot/windows/windows.go

return "", errors....

stevvooe · 2017-10-10T21:09:18Z

vendor/github.com/Microsoft/go-winio/archive/tar/common.go

Why is there another fork of the tar package?

I'll get back to you on this. There was something that needed to be forked due to layer implementation on Windows IIRC, but this was done a long time ago

stevvooe · 2017-10-10T21:15:39Z

snapshot/windows/windows.go

Could you leave the type definition near the New function?

stevvooe · 2017-10-10T21:34:59Z

snapshot/windows/windows.go

Do we want to upstream this to continuity?

That sounds good to me.

stevvooe · 2017-10-10T21:37:05Z

@darrenstahlmsft This is a really solid start! The snapshotter model seems to be holding up, although we may need to rethink how the related mounts work. Would it be possible to get the whole structure into a single mount call?

tonistiigi · 2017-10-10T21:59:38Z

mount/mount_windows.go

Afaics this function doesn't actually mount anything and the actual mount is in https://github.com/containerd/containerd/pull/1613/files#diff-c3cba80ac2b534161f3d1259da272c21R80 and doesn't have really much to do with snapshots as all the inputs come from a spec. This makes the API very inconsistent for anything that calls this function directly instead of just creating a container what internally calls here and atm there isn't a public method for mounting a snapshot.

This does do the mount to host (PrepareLayer mounts the layers), but on Windows, you cannot specify a target location. Instead, the layers are exposed as a VHD mounted on the host. The linked code just retrieves the already mounted path from HCS.

I agree that this function does not do what it says on the box here on Windows. We'll need to work out a way to expose what this really does via a different function, or otherwise make it do what it says. Having target as a parameter to the mount does not really make sense on Windows layer mounting.

darstahl · 2017-10-11T22:53:18Z

Running tests is timing out on Windows waiting for start. I'm not sure why yet.

darstahl · 2017-10-11T22:53:41Z

Pushed an update to address most of the feeback, and rebased

Next step: Moving archive logic around into more permanent locations

Signed-off-by: Darren Stahl <[email protected]>

darstahl force-pushed the WindowsSnapshotter branch from 38400c4 to 9b3b782 Compare October 6, 2017 23:46

AkihiroSuda reviewed Oct 7, 2017

View reviewed changes

mlaventure reviewed Oct 9, 2017

View reviewed changes

stevvooe reviewed Oct 10, 2017

View reviewed changes

tonistiigi reviewed Oct 10, 2017

View reviewed changes

darstahl force-pushed the WindowsSnapshotter branch 2 times, most recently from 4633a57 to 30b412e Compare October 11, 2017 22:49

Implement Windows snapshotter and differ

3d04e90

Signed-off-by: Darren Stahl <[email protected]>

darstahl force-pushed the WindowsSnapshotter branch from 30b412e to 3d04e90 Compare October 11, 2017 23:44

darstahl mentioned this pull request Oct 23, 2017

Windows use of Archive package #1678

Closed

crosbymichael added this to the 1.1 milestone Nov 13, 2017

darstahl mentioned this pull request Dec 14, 2017

Windows: Roadmap to WCOW #1920

Closed

8 tasks

darstahl closed this Jan 5, 2018

darstahl mentioned this pull request Jan 5, 2018

Implement Windows snapshotter and differ #1969

Merged

3 tasks

TBBle mentioned this pull request Jul 16, 2020

Windows (WCOW) implementation of diff.Comparer #4394

Closed

[WIP] Implement Windows snapshotter and differ #1613

[WIP] Implement Windows snapshotter and differ #1613

Uh oh!

Conversation

darstahl commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda Oct 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darstahl Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlaventure left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darstahl Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Oct 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darstahl commented Oct 6, 2017 •

edited

Loading

AkihiroSuda Oct 7, 2017 •

edited

Loading

darstahl Oct 9, 2017 •

edited

Loading

darstahl Oct 10, 2017 •

edited

Loading

darstahl commented Oct 11, 2017 •

edited

Loading