Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Windows] Fix local mounter on Windows #3783

Merged
merged 3 commits into from
Jun 9, 2023

Conversation

gabriel-samfira
Copy link
Collaborator

@gabriel-samfira gabriel-samfira commented Apr 7, 2023

This change leverages the recent changes in containerd to properly mount windows-layers. The containerd mounter cannot handle bind mounts. These mounts will be implemented in buildkit using the bind filter.

Depends on:

Note: this branch will not build until #3782 merges.

@gabriel-samfira gabriel-samfira changed the title Fix mount layers on host [Windows] Fix local mounter on Windows Apr 7, 2023
@crazy-max crazy-max added this to the v0.12.0 milestone May 26, 2023
Unless we're extracting an image to a layer, we should never return it
directly. We must always interact with it via a mount point, as Windows
layers hold a number of metadata files which should never be mutated
directly. When reading/manipulating the contents of a layer we should
always pass through a mount.

Allow the containerd mount.Mount() to properly mount the layer before we
interact with it.

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
@gabriel-samfira gabriel-samfira marked this pull request as ready for review June 8, 2023 09:33
@gabriel-samfira
Copy link
Collaborator Author

cc: @TBBle (if you have some time)

@gabriel-samfira
Copy link
Collaborator Author

Apropos, with this and #3908, a very basic Dockerfile/LLB can be built on Windows:

FROM mcr.microsoft.com/windows/nanoserver:ltsc2022

USER ContainerAdministrator
ADD test.txt C:\\test.txt

Anything that does not need the executor or readUser() (separate PRs I need to update)

ro = true
break
}
// The Windows snapshotter does not have any notion of bind mounts. We emulate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why the simplified case of returning m.Source is removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, if we're certain that bind mounts will always refer to local folders, and will never be UNC paths or a \\?\Volume{}, we can simply return the source here for rw bind mounts.

If however, we're not certain that bind mounts will be local folders, it's safer to use the bind filter here to mount the source first, as that will give us a mount point we can actually access. The ApplyBindFilter() function can also mount volumes, network shares, UNC paths, etc, not just folders.

I will return the source here for rw bind mounts with a note that if there's ever a need to mount anything other than a folder, we can just let the bind filter mount it even if it's rw.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change made. PTAL.

}
// The Windows snapshotter does not have any notion of bind mounts. We emulate
// bind mounts here using the bind filter.
if err := bindfilter.ApplyFileBinding(dir, m.Source, m.ReadOnly()); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something that should eventually end up in containerd's Mount() implementation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a longer discussion about this here: containerd/containerd#8043 (comment)

The short version is: it's unlikely.

The slightly longer version is: Windows doesn't really have bind mounts in the same way as Linux does. It also doesn't use them to bind folders from the host to locations inside the container in the same way. Windows can mount files and folders from the host, inside the container, but it uses different mechanisms to do this (Host Compute System).

Trying to shoehorn Linux style bind mounts inside the Windows version of the containerd mounter via a mount.Mount{} would create confusion and would never really be equivalent in terms of functionality. In essence, if we use a large enough hammer, we can squeeze in a round shape inside a square shape, but it wouldn't be the wisest course of action.

The simplest and least likely thing to cause issues moving forward is to treat this case locally, here. We don't use bind mounts here to expose things to container. We just use them to manipulate layer contents without spinning up a temporary container.

Where we do need to use mounts of files or folders inside containers (for example, the get-user-info.exe re-exec will be a ro mount of the buildkitd.exe binary) we'll use HCS to handle that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows doesn't really have bind mounts in the same way as Linux does.

It's not like it has "mount API" for windows-layer as well. Afaics it is also just a made-up name with custom handling in containerd.

Copy link
Collaborator Author

@gabriel-samfira gabriel-samfira Jun 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does. The windows-layer (WCIFS - Windows Container Isolation File System) creates a \\?\Volume{} that is composed from all parent layers plus the scratch layer. That volume can be mounted inside a folder like any other partition using the usual public APIs inside Windows. It's not called "mounting", it's called "adding an access path", but the functionality in this case is equivalent.

Edit: Also, we can use the wclayer utility that comes with hcsshim to "mount" windows layers.

Copy link
Member

@tonistiigi tonistiigi Jun 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant type=windows-layer has a meaning to do a bunch of hcsshim calls that make the files available. Afaics the string value of "type" is never passed to kernel.

Same way type=bind could have a meaning to do bindfilter.ApplyFileBinding calls to make files available.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct. We never pass that to the kernel, because that's the only type that is currently understood by the Windows snapshotter. The bind type however would not mean anything to the Windows snapshotter. If we add it to containerd at any point in time, we'd need to do it in a way that makes sense to the Windows snapshotter. It was briefly part of containerd/containerd#8043 and we may add it again at some point.

For now, if the bind type has a meaning for other projects (such as buildkit) it can easily be emulated where it's needed by calling the bind filter. That is actually one of the reasons I added the functionality in go-winio.

The need for this was evident in buildkit, but adding it to containerd raised some concerns (see above mentioned comment), and given how long it took to get that PR to a point where it could be merged (initial PR was proposed in 2020 and merged recently), we opted to keep containerd lean and let it worry only about types the Windows snapshotter understood.

In case of a rw bind mount, we return the source directly.

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
@tonistiigi tonistiigi merged commit d988f59 into moby:master Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants