CLI: Add --mount support for create and run - #41337
CLI: Add --mount support for create and run#41337David Bennett (dkbennett) wants to merge 7 commits into
Conversation
Add a Docker-style --mount option to `wslc container run` and `wslc container create`. The flag accepts comma-separated key=value pairs (type=bind|volume|tmpfs, source/src, target/destination/dst, readonly/ro) and is routed into the existing volume/tmpfs plumbing. - Parse --mount into a ParsedMount (ArgumentValidation) - Register the Mount argument for run/create - Wire parsed mounts into ContainerOptions (ContainerTasks) - Add localization strings (MountArgDescription, InvalidMountError) - Add e2e tests (tmpfs, named volume, readonly-via-inspect, invalid type) and update run/create help-text expectations Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds Docker-compatible --mount parsing and plumbing for wslc container run / wslc container create, translating validated mount specs into the existing bind/volume/tmpfs execution paths and adding unit + E2E coverage plus localized error strings.
Changes:
- Introduces a common
mount::Specmodel and Docker-grammar--mountparser undersrc/windows/common/. - Wires
--mountinto argument validation and container option construction, including duplicate-destination rejection across--mount/--volume/--tmpfs. - Adds table-driven unit tests and new E2E scenarios for
--mount.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/windows/wslc/WSLCCLIMountParserUnitTests.cpp | Adds table-driven unit tests for --mount parsing and destination de-duplication behavior. |
| test/windows/wslc/e2e/WSLCE2EContainerRunTests.cpp | Adds E2E coverage for --mount tmpfs/volume/readonly and failure cases. |
| src/windows/wslc/tasks/ContainerTasks.cpp | Plumbs parsed mount specs from CLI args into ContainerOptions and validates uniqueness. |
| src/windows/wslc/services/ContainerService.cpp | Translates mount::Spec into launcher calls for bind/volume/tmpfs. |
| src/windows/wslc/services/ContainerModel.h | Extends ContainerOptions with Mounts and declares destination uniqueness validation. |
| src/windows/wslc/services/ContainerModel.cpp | Reuses named-volume validation from common parser and implements duplicate-destination detection. |
| src/windows/wslc/commands/ContainerRunCommand.cpp | Adds --mount to container run arguments. |
| src/windows/wslc/commands/ContainerCreateCommand.cpp | Adds --mount to container create arguments. |
| src/windows/wslc/arguments/SpecParsing.cpp | Adds a standard header include used by parsing utilities. |
| src/windows/wslc/arguments/ArgumentValidation.cpp | Validates/parses --mount and surfaces localized user-facing errors. |
| src/windows/wslc/arguments/ArgumentDefinitions.h | Declares the new --mount argument in the X-macro table. |
| src/windows/wslc/arguments/ArgumentConvertedTypes.h | Adds the converted type alias mapping for parsed mount specs. |
| src/windows/common/MountSpecParsing.h | Declares the mount grammar version, spec model, and parsing/normalization helpers. |
| src/windows/common/MountSpecParsing.cpp | Implements Docker-compatible --mount parsing and tmpfs option formatting. |
| src/windows/common/CMakeLists.txt | Adds the new common parser sources/headers to the build. |
| localization/strings/en-US/Resources.resw | Adds localized strings for invalid mount syntax and duplicate mount destinations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/windows/common/MountSpecParsing.cpp:583
FormatTmpfsOptionsalso omits an explicitly providedtmpfs-size=0by skipping size when the parsed value is 0. If the user passestmpfs-size=0, that intent should be preserved and forwarded (and kept consistent with existing--tmpfsbehavior, which can passsize=0).
if (mount.TmpfsSizeBytes.has_value() && mount.TmpfsSizeBytes.value() != 0)
{
options.emplace_back(std::format("size={}", FormatDockerTmpfsSize(mount.TmpfsSizeBytes.value())));
}
src/windows/common/MountSpecParsing.cpp:579
FormatTmpfsOptionsdrops an explicitly providedtmpfs-mode=0000because it omits the mode option when the parsed value is 0. That changes user-requested semantics (and differs from--tmpfs, which forwards options verbatim), sincemode=0is a meaningful tmpfs setting.
This issue also appears on line 580 of the same file.
if (mount.TmpfsMode.has_value() && mount.TmpfsMode.value() != 0)
{
options.emplace_back(std::format("mode={:o}", mount.TmpfsMode.value()));
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/windows/common/MountSpecParsing.cpp:14
- New files should use the repository’s single-line copyright header format (
// Copyright (C) Microsoft Corporation. All rights reserved.). This file currently uses the older block header style.
/*++
Copyright (c) Microsoft. All rights reserved.
src/windows/common/MountSpecParsing.h:14
- New files should use the repository’s single-line copyright header format (
// Copyright (C) Microsoft Corporation. All rights reserved.). This header currently uses the older block header style.
/*++
Copyright (c) Microsoft. All rights reserved.
test/windows/wslc/WSLCCLIMountParserUnitTests.cpp:14
- New files should use the repository’s single-line copyright header format (
// Copyright (C) Microsoft Corporation. All rights reserved.). This test file currently uses the older block header style.
/*++
Copyright (c) Microsoft. All rights reserved.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/windows/common/MountSpecParsing.cpp:159
- Minor typo in the digit set passed to find_last_of: it includes an extra '0' ("01234567890. "), which is confusing to readers even though it likely doesn’t change behavior.
const auto separator = input.find_last_of("01234567890. ");
src/windows/wslc/arguments/ArgumentValidation.cpp:234
- The mount parser’s ValidationException::Reason() is composed of hard-coded English strings (from MountSpecParsing.cpp) and is surfaced directly to users via WSLCCLI_InvalidMountError. This means a significant portion of the user-facing error text is not localizable, which conflicts with the PR’s stated goal of localized errors for unsupported/invalid mount specs.
catch (const mount::ValidationException& ex)
{
throw ArgumentException(Localization::WSLCCLI_InvalidMountError(value, ex.Reason()));
}
| switch (mountSpec.MountType) | ||
| { | ||
| case mount::Type::Bind: | ||
| containerLauncher.AddVolume(mountSpec.Source, mountSpec.Target, mountSpec.ReadOnly); |
There was a problem hiding this comment.
routing --mount type=bind through AddVolume might cause the backend to create the host directory when it does not exist. Docker --mount functionality fails when there's a missing bind source. So a typo could silently create and mount an empty directory. Let's preserve the distinction from --volume. Add an e2e test confirming that the directory is not created.
Example:
With --mount, the following is expected to fail:
wslc container run --mount "type=bind,source=C:\nonexistent-dir,target=/data" alpine
With --volume, the following is expected to succeed and create the nonexistent-dir:
wslc container run --volume "C:\nonexistent-dir:/data" alpine
since in this case both are routed through AddVolume, we will lose this distinction... which docker intentionally distinguishes.
There was a problem hiding this comment.
I have to wire up a new service entry for HostConfig.Mount so that should resolve the issue with bind and volume, and I will add a test for ensuring no empty directory for this case.
|
|
||
| if (mount.Target.find(':') != std::string::npos) | ||
| { | ||
| ThrowInvalid(L"target paths containing ':' are not supported."); |
There was a problem hiding this comment.
While this is correct for the "--volume", "-v", and "--tmpfs", docker does allow : in target names for --mount, because --mount uses comma-separated key-value fields.
For example,
--mount type=bind,source=,target=/path:mntdir
--mount type=volume,source=,target=/path:voldir
--mount type=tmpfs,target=/path:tmpfs
are all supported.
There was a problem hiding this comment.
Will fix and add parsing test cases for each of those.
| } | ||
| catch (const mount::ValidationException& ex) | ||
| { | ||
| throw ArgumentException(Localization::WSLCCLI_InvalidMountError(value, ex.Reason())); |
There was a problem hiding this comment.
we have hardcoded english error strings in MountParsingSpec.cpp, so the text in ex.Reason() will not be translated, correct? could the parser return type and values and then the localized string can be built here?
| containerLauncher.AddTmpfs(mountSpec.Target, mount::FormatTmpfsOptions(mountSpec)); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
this will require us to make correspoinding changes to container inspect side as well, correct? we should check if proper coverage for inspect is present with the new mount types we are supporting
| case Type::Volume: | ||
| if (mount.Source.empty()) | ||
| { | ||
| ThrowInvalid(L"anonymous volume mounts are not supported."); |
There was a problem hiding this comment.
would be good to mention this gap in --mount help description
| { | ||
| try | ||
| { | ||
| mount::ValidateMountCollection(options.Mounts); |
There was a problem hiding this comment.
mounts are validated here, then another set is allocated for dests and processed again. i think for --volume and --tmpfs, parsing happens here and in ContainerService. it would be good to convert all the flags into one mount collection and then validation, dupes, etc. can be done on that one collection
| { | ||
| std::wstring_view Name; | ||
| Field Id; | ||
| Family OptionFamily; |
There was a problem hiding this comment.
i don't see the OptionFamily being used. is it an anticipatory placeholder or something?
| { | ||
| throw ArgumentException(Localization::WSLCCLI_InvalidMountError(value, ex.Reason())); | ||
| } | ||
| }); |
There was a problem hiding this comment.
also, every ValidationException here will result in an "Invalid argument". Could some of the ones like 'volume-nocopy', 'volume-opt' etc. be reported as 'not supported' by wslc?
| hostDirectory.wstring(), | ||
| DebianImage.NameAndTag(), | ||
| fileName)); | ||
| result.Verify({.Stdout = L"WSLC Mount Bind Test", .Stderr = L"", .ExitCode = 0}); |
There was a problem hiding this comment.
based on previous comments around missing source path for bind, the tests should cover that
| result = RunWslc(std::format( | ||
| L"container run --rm --mount type=volume,source={},target=/data {} cat /data/value", WslcVolumeName, DebianImage.NameAndTag())); | ||
| result.Verify({.Stdout = L"original", .Stderr = L"", .ExitCode = 0}); | ||
| } |
There was a problem hiding this comment.
tests to validate corresponding inspect output
| L"container run --rm --mount type=volume,source={},target=/data {} cat /data/value", WslcVolumeName, DebianImage.NameAndTag())); | ||
| result.Verify({.Stdout = L"original", .Stderr = L"", .ExitCode = 0}); | ||
| } | ||
|
|
There was a problem hiding this comment.
based on previous comments, tests around colon-containing destinations paths
| L"/wslc-tmpfs/data\"", | ||
| DebianImage.NameAndTag())); | ||
| result.Verify({.Stdout = L"tmpfs_test", .Stderr = L"", .ExitCode = 0}); | ||
| } |
There was a problem hiding this comment.
we should add/augment e2e tests to verify that tmpfs size/mode are applied correctly, instead of only testing that the mount is usable
| @@ -141,6 +143,24 @@ static wsl::windows::common::RunningWSLCContainer CreateInternal(Terminal& termi | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
for when we add sdk support for this, it might make sense to not convert into the legacy AddVolume, AddNamedVolume, AddTmps calls... maybe a single typed mount structure that the launcher/backend can accept would be good. that way CLI and API can go through on reusable path
There was a problem hiding this comment.
Yes all of the shared usage of existing Volume, NamedVolume, and Tmpfs calls are going to be replaced by AddMount which will have the distinction and ensure it goes into the Mounts config, along with E2E tests that verify the inspect output matches that expectation. We won't be reusing any of those for this code with my next update, but I'm noting all the differences as explicit tests to add.
Summary of the Pull Request
Adds Docker-compatible
--mountsupport towslc container runandwslc container create.The parser supports bind, named-volume, and tmpfs mounts, including Docker aliases, CSV quoting, read-only mounts, and supported tmpfs options. It produces a common typed mount model, rejects unsupported mount features explicitly, and detects duplicate destinations across
--mount,--volume, and--tmpfs.PR Checklist
Detailed Description of the Pull Request / Additional comments
Why parse
--mountin WSLC?This is consistent with how Docker CLI handles
--mount, and it is necessary for the same fundamental reason. Docker CLI does not pass the raw--mountkey/value string to Docker Engine. ItsMountOptparser validates the CLI grammar and converts it into structuredmount.Mountobjects, which are sent to the Engine throughHostConfig.Mounts. The Engine API consumes typed mount configuration, not Docker CLI syntax.WSLC must perform the equivalent parsing and translation because its backend boundary is also structured. The runtime and COM transport accept type-specific mount data, not an opaque Docker CLI string that could be forwarded for Docker Engine to interpret.
WSLC additionally has work that must happen before the Engine request can be constructed:
WSLC also requires an additional capability gate. Docker CLI can represent the full
mount.MountAPI object, but the current WSLC transport cannot faithfully carry every Docker mount type and option. After applying Docker-compatible syntax validation, WSLC must reject unsupported features before translation. Otherwise, accepted input could lose information silently and reach the Engine with semantics different from what the user requested.For these reasons, forwarding the fields for Docker Engine to sort out is not possible with the current architecture. Docker CLI itself does not work that way, there is no WSLC backend boundary that accepts the original
--mountstring, and WSLC needs the parsed values to prepare the backend request.The common parser is intentionally scoped in two layers:
docker/cliv25.0.3.This preserves familiar Docker CLI behavior while avoiding silent semantic loss.
The parser lives in
src/windows/commonand returns a transport-neutral typed mount specification containing the mount type, source, target, read-only state, and supported tmpfs settings. The CLI currently invokes it during argument validation, but it has no dependency on CLI execution types. This keeps the parsing and capability policy reusable if a future SDK or runtime API needs to accept Docker-style mount strings.An SDK API would normally expose typed mount fields directly rather than requiring callers to construct CLI syntax. That typed API can map to the same common mount model, keeping CLI and SDK behavior aligned while allowing the text-parser call site to move into the runtime later without rewriting the parser.
The source explicitly pins the grammar to
docker/cliv25.0.3 so the parsing table can be reviewed when the bundled Docker backend changes.Implementation
mount::Specmodel that is parsed once during CLI argument validation.src/windows/common/MountSpecParsing.cppandMountSpecParsing.h.Validation Steps Performed
WSLCCLIMountParserUnitTests: 5/5 test methods passed, exercising 123 table-driven parser cases.Container_Run_Mount_*end-to-end tests: 5/5 passed.