-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: wcow: add support for bind and cache mounts #5708
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
Conversation
8bf0b5a
to
bc2decc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the fix LGTM
bc2decc
to
ec20b95
Compare
I have done some testing on this with cache mounts and it works! But I'm a bit confused by the mount target path. It doesn't seem to be possible to specify an absolute path, specifying a full windows path "C:/Users/ContainerAdministrator/.conan2/p" does not work:
If I specify "/Users/ContainerAdministrator/.conan2/p" is seems to be relative to the current set WORKDIR and not the root of the C:/ disk which I think is inconsistent with the linux implementation. |
ec20b95
to
9c74564
Compare
Thanks for testing, let me take a look at this on Monday.
---
// sent from a tiny device while on the move. forgive the tie pose.
…On Fri, Feb 7, 2025, 15:18 Daniel Nilsson ***@***.***> wrote:
I have done some testing on this with cache mounts and it works! But I'm a
bit confused by the mount target path.
I'm running the binaries from this build:
https://github.com/moby/buildkit/actions/runs/13193504481
It doesn't seem to be possible to specify an absolute path, specifying a
full windows path "C:/mycache" does not work:
RUN --mount=type=cache,id=conan-cache-v1,sharing=locked,target=C:/Users/ContainerAdministrator/.conan2/p `
failed to create shim task: hcs::CreateComputeSystem si4wd93a8a3gf69wkaqhu86uz: The parameter is incorrect.: unknown
If I specify "/Users/ContainerAdministrator/.conan2/p" is seems to be
relative to the current set WORKDIR and not the root of the C:/ disk which
I think is inconsistent with the linux implementation.
—
Reply to this email directly, view it on GitHub
<#5708 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB7ZELBM76SSE6V7EIIZRD2OSQCFAVCNFSM6AAAAABWTGIERSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNBSG43DQNBYGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Thanks :) Here is a small reproduce:
|
9c74564
to
261ee88
Compare
@danielnilsson9 -- you are right, this is not as it is on Linux, eg: FROM alpine
WORKDIR /build
RUN --mount=type=cache,target=/cache ls / && echo -- && ls ./ build log:
Ok, will fix that. Thanks for the catch! |
261ee88
to
b2c2b01
Compare
@danielnilsson9 -- fixed, please take a look. For a consistent experience, we'll add to the documentation that for mount |
b2c2b01
to
00b9867
Compare
@profnandaa Nice! Can confirm it is working as expected now. |
That's a kinda strange rule. Can't I have multiple drives in Windows container and want to mount cache to other letter than |
I see... makes sense. I had noticed for instance when
I didn't know which other more surprises might be there. However, for other drive locations, that should still work okay. Let me run some tests. |
42f8214
to
ba74219
Compare
No issues mounting from a different drive than C: FROM mcr.microsoft.com/windows/nanoserver:ltsc2022
USER ContainerAdministrator
RUN --mount=type=bind,target=/test type \test\example.txt > ls -Name E:\sample\
example.txt
> buildctl build --frontend dockerfile.v0 --local context=E:\sample --local dockerfile=. `
--output type=image,name=docker.io/profnandaa/mount-test,push=false `
--progress plain --no-cache
...
#6 [stage-0 2/2] RUN --mount=type=bind,target=/test type testexample.txt
#6 1.387 mounted!!! from E:\
#6 DONE 1.7s |
6fef8df
to
59b5b09
Compare
NOTE: with this diff, now supports backslashes on windows paths for mount options. However, you have to change the default escape token from #escape=`
FROM mcr.microsoft.com/windows/nanoserver:ltsc2022
USER ContainerAdministrator
RUN --mount=type=cache,target=C:\mycache echo "hello" > \mycache\foo
# then for options on multilines you have to go with ` instead of \
RUN --mount=type=cache,`
target=C:\mycache dir \mycache\foo |
59b5b09
to
39e3ff8
Compare
I'll carry out the rest of the This should be ready for review. /cc. @tonistiigi @crazy-max |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
NOTE: taken it back to draft to cover some cases in |
39e3ff8
to
54924cc
Compare
With the latest update, you can now set
Now, such a dockerfile can build: # escape=`
FROM mcr.microsoft.com/windows/nanoserver:ltsc2022
USER ContainerAdministrator
RUN --mount=type=bind,from=docker.io/wintools/nanoserver,`
src=/,dst=/bin C:\bin\Windows\System32\whoami.exe |
54924cc
to
b94b60c
Compare
26dfae5
to
7ba4fb1
Compare
cache/contenthash/checksum.go
Outdated
@@ -862,6 +862,10 @@ func (cc *cacheContext) scanChecksum(ctx context.Context, m *mount, p string, fo | |||
} | |||
|
|||
func (cc *cacheContext) checksum(ctx context.Context, root *iradix.Node[*CacheRecord], txn *iradix.Txn[*CacheRecord], m *mount, k []byte, followTrailing bool) (*CacheRecord, bool, error) { | |||
// only applies for Windows, it's a no-op on non-Windows. | |||
enableProcessPrivileges() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be in a more top-level function. This one gets called recursively I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me crosscheck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You right, it's recursive here. I've taken it up to a more top-level function now.
executor/oci/spec_windows.go
Outdated
@@ -119,3 +119,9 @@ func generateCDIOpts(_ *cdidevices.Manager, devices []*pb.CDIDevice) ([]oci.Spec | |||
// https://github.com/cncf-tags/container-device-interface/issues/28 | |||
return nil, errors.New("no support for CDI on Windows") | |||
} | |||
|
|||
func getMountType(_ string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this true only for bind? In that case should be normalizeBindType()
with an input parameter comparison.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not just for bind, all of them are referred to as windows-layer
by this point. They all fail with:
failed to create shim task: invalid OCI spec - Type 'windows-layer' not supported: unknown
However, I think I can rename this to normalizeMountType
instead?
@@ -113,12 +114,12 @@ func dispatchRunMounts(d *dispatchState, c *instructions.RunCommand, sources []* | |||
sharing = llb.CacheMountLocked | |||
} | |||
if mount.CacheID == "" { | |||
mount.CacheID = path.Clean(mount.Target) | |||
mount.CacheID = filepath.Clean(mount.Target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filepath
in here doesn't look right as this code can run in a different OS than daemon or the mount.Target
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was my attempt to have the Windows path resolve to the same default cache ID e.g.
C:/temp -> C:\\temp
though this quickly falls apart with cases like /temp -> \\temp
.
Best guarantee is just for the user to set their own IDs if they are using different path schemes. Will add that as part of the documentation note.
Let me revert back to path.Clean
.
Currently, mounts are not supported for WCOW builds, see moby#5678. This commit introduces support for bind and cache mounts. The remaining two require a little more work and consultation with the platform teams for enlightment. WIP Checklist: - [x] Support for bind mounts - [x] Support for cache mounts - [x] add frontend/dockerfile integration tests - [x] add client integration tests (not all, `llb.AddMount` not complete) Fixes moby#5603 Signed-off-by: Anthony Nandaa <[email protected]>
7ba4fb1
to
877d8a7
Compare
After completing support for `llb.AddMount` in moby#5708 -- this made possible to add some more `client` integration tests that were making `llb.AddMount` calls. Most of the tests below are related to this: - [x] testCacheExportCacheKeyLoop - [x] testOCILayoutSource - [x] testBuildMultiMount - [x] testTarExporterSymlink - [x] testCacheExportIgnoreError - [x] testBasicCacheImportExport - [x] testUncompressedLocalCacheImportExport - [x] testImageManifestRegistryCacheImportExport - [x] testBasicLocalCacheImportExport - [x] testBasicInlineCacheImportExport - [x] testRegistryEmptyCacheExport - [x] testCachedMounts - [x] testDuplicateCacheMount - [x] testRunCacheWithMounts - [x] testMountWithNoSource - [x] testProxyEnv - [x] testRelativeMountpoint - [x] testValidateDigestOrigin - [x] testExportAnnotations - [x] testExportAnnotationsMediaTypes - [x] testAttestationDefaultSubject - [x] testAttestationBundle - [x] testLLBMountPerformance Skipped tests (due to registry setup issue, to be addressed separately): - testUncompressedRegistryCacheImportExport - testBasicRegistryCacheImportExport - testMultipleRegistryCacheImportExport Related to moby#5678 Signed-off-by: Anthony Nandaa <[email protected]>
Currently, mounts are not supported for WCOW builds, see #5678. This commit introduces support for bind and cache mounts (through the dockerfile frontend). The remaining two require a little more work and consultation with the platform teams for enlightment.
WIP Checklist:
llb.AddMount
support)Addresses part of #5678
Fixes #5603
Demo
Prep the context directory:
Dockerfile:
Build command:
Build log: