mount: add `bind-recursive=<bool|string>` and deprecate `bind-nonrecursive=<bool>` #4316

AkihiroSuda · 2023-05-29T12:35:49Z

For:

Support recursively read-only (RRO) mounts moby/moby#45278

See opts/mount_test.go:TestMountOptSetBindRecursive() for the behavior:

bind-recursive=true, bind-recursive=enabled: read-only mounts are recursively read-only if Engine >= v25 && kernel >= v5.12
bind-recursive=false, bind-recursive=disabled: alias of bind-nonrecursive=true. Now this form is preferred over bind-nonrecursive=true.
bind-recursive=writable: conforms to Docker v24; read-only mounts are recursively mounted but not recursively read-only
bind-recursive=readonly: force recursively read-only, or raise an error

Documentation will be added separately after reaching consensus on the design.

Backup (2023-05-29)

AkihiroSuda@6ebf6ba

- What I did

Added the CLI and the docs for:

Support recursively read-only (RRO) mounts moby/moby#45278

- How I did it

The -v options are implemented on the daemon side.

ro-non-recursive
ro-force-recursive (alias: rro)

The following "CSV" options for --mount are newly added in this CLI-side PR:

bind-readonly-nonrecursive (alias: bind-ro-nonrecursive) (Do not confuse this with the exiswting bind-nonrecursive)
bind-readonly-forcerecursive (alias: bind-ro-forcerecursive)

- How to verify it
Run docker run --mount type=bind,source=/mnt,target=/mnt,bind-ro-forcerecursive,bind-propagation=rprivate, and make sure the mount is made recursively read-only.

- Description for the changelog

Add CLI and docs for recursively read-only mounts

- A picture of a cute animal (not mandatory but encouraged)
🐧

codecov-commenter · 2023-05-29T12:39:47Z

Codecov Report

Merging #4316 (fc6976d) into master (05bec8d) will increase coverage by 0.00%.
The diff coverage is 50.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4316   +/-   ##
=======================================
  Coverage   59.70%   59.71%           
=======================================
  Files         288      288           
  Lines       24816    24858   +42     
=======================================
+ Hits        14817    14843   +26     
- Misses       9113     9128   +15     
- Partials      886      887    +1

AkihiroSuda · 2023-06-08T01:10:57Z

@thaJeztah PTAL 🙏

thaJeztah · 2023-06-09T10:24:41Z

Oh! Thanks for the ping, had this one on my list, started looking, and then forgot 😅

cli/command/service/opts.go

thaJeztah · 2023-06-09T10:31:07Z

cli/command/utils.go

+// ValidateMountWithAPIVersion validates a mount with the server API version.
+func ValidateMountWithAPIVersion(m mounttypes.Mount, serverAPIVersion string) error {
+	if m.BindOptions != nil {
+		if m.BindOptions.NonRecursive && versions.LessThan(serverAPIVersion, "1.40") {


I guess the overhead is neglectable, so probably not worth doing; we could consider having a if versions.GreaterThanOrEqualTo(serverAPIVersion, "1.44") somewhere to skip the checks.

docs/reference/commandline/service_create.md

thaJeztah · 2023-06-10T08:12:50Z

docs/reference/commandline/service_create.md

      By default, submounts are recursively bind-mounted as well. However, this behavior can be confusing when a
-      bind mount is configured with <tt>readonly</tt> option, because submounts are not mounted as read-only.
+      bind mount is configured with <tt>readonly</tt> option, because submounts are not mounted as read-only
+      if Docker Engine is older than v25, or Linux kernel is older than v5.12.


We should probably use a full version for these v25.0.0, but probably we should consider rewriting this section to assume "latest" (it now works).

docs/reference/run.md

thaJeztah · 2023-06-10T08:35:46Z

docs/reference/commandline/service_create.md

    </td>
  </tr>
+  <tr>
+    <td><b>bind-readonly-nonrecursive</b> or <b>bind-ro-nonrecursive</b></td>


I'm considering if we should (soft) deprecated bind-nonrecursive, and instead have a "recursive mounts" option that takes a (non-boolean) value, e.g. something like;

recursive-mounts=enabled (default)

recursive-mounts=disabled

recursive-mounts=readonly (should error if bind-propagation is set by user (to anything other than rprivate), but perhaps handled by API)

recursive-mounts=force-readonly (should error if bind-propagation is set by user (to anything other than rprivate), but perhaps handled by API)

What about this?

case "bind-recursive": valS := val // Allow boolean as an alias to "enabled" or "disabled" if b, err := strconv.ParseBool(valS); err == nil { if b { valS = "enabled" } else { valS = "disabled" } } switch valS { case "enabled": // read-only mounts are recursively read-only if Engine >= v25 && kernel >= v5. 12 // NOP case "disabled": // alias of bind-nonrecursive=true bindOptions().NonRecursive = true case "readonly-classic": // conforms to Docker v24; read-only mounts are recursively mounted bu t not recursively read-only bindOptions().ReadOnlyNonRecursive = true mount.ReadOnly = true case "readonly-force": // force recursively read-only, or raise an error bindOptions().ReadOnlyForceRecursive = true bindOptions().Propagation = mounttypes.PropagationRPrivate mount.ReadOnly = true default: return fmt.Errorf("invalid value for %s: %s (must be \"enabled\", \"disabled\", \"reado nly-classic\", or \"readonly-force\")", key, val) }

vendor.mod

AkihiroSuda · 2023-09-22T16:52:44Z

@thaJeztah Could you take a look?
#4316 (comment)

thaJeztah · 2023-09-28T13:13:19Z

Sorry, should've posted some comments here. I've been starting to review this one multiple times, and trying to construct a table with possible combinations (and what their result would be), and my head exploded each time 😂

However, the other day it occurred to me what I think was the part that I had trouble with;

the advanced syntax provides two separate options to control the "amount of read-only" and whether recursive mounts are included in the mount (readonly and bind-recursive)
☝️ the confusing bit there was "how do these relate"?

So the goal we're aiming for here is;

provide the "easy" option (-v host-path:container-path:ro) that covers the "90% of use-cases"
provide the same through --mount, but allow more advanced options for the 10% of use-cases (giving them more control)

However I think (and that's the part I need to look at again, as I was considering this away from my computer), to achieve the same result as 1., but using the advanced syntax (2.), I now MUST provide all the options (i.e. when using --mount ).

While it's good to allow the user to have more fine-grained control, my ideal would be to make that "optional"; even with the advanced --mount syntax.

So my thinking is; this:

--mount type=bind,source=host-path,target=container-path,readonly

To be the equivalent of:

-v host-path:container-path:ro

(i.e.; try recursive-readonly, and fallback to non-recursive)

But if the user needs to have more fine-grained control, then allow them to set the bind-recursive options;

e.g.: force recursive read-only:

--mount type=bind,source=host-path,target=container-path,readonly,bind-recursive=readonly-force

Maybe we don't even need the readonly-force, and it could just be readonly (as it's now limited to only the recursive parts)

e.g.: read-only, but no recursive mounts:

--mount type=bind,source=host-path,target=container-path,readonly,bind-recursive=disabled

AkihiroSuda · 2023-09-28T14:20:14Z

Thanks, renamed readonly-force to readonly.

thaJeztah · 2023-09-28T15:33:06Z

opts/mount.go

+			case "readonly-classic": // conforms to Docker v24; read-only mounts are recursively mounted but not recursively read-only
+				bindOptions().ReadOnlyNonRecursive = true
+				mount.ReadOnly = true
+			case "readonly": // force recursively read-only, or raise an error
+				bindOptions().ReadOnlyForceRecursive = true
+				bindOptions().Propagation = mounttypes.PropagationRPrivate
+				mount.ReadOnly = true


I think we're getting close (sorry had to jump out earlier as I had some calls); if we consider the bind-recursive option to be controlling options for recursive mounts (and not for the "main" mount), should we update mount.Readonly at all here?

My thinking (please reply if I'm completely off the mark ❤️);

readonly is the "overall" option; when enabled, by default it makes both the "main" mount, and any nested mounts read-only (i.e. "best-effort recursive-readonly"

bind-recursive=disabled disables nested mounts

bind-recursive=readonly forces nested mounts to be read-only

👆 this can also be done separate from readonly, so --mount type=bind,source=host-path,target=container-path,bind-recursive=readonly (without "global" readonly) would then mean: the main mount is read-write, but nested mounts (if any) would be mounted read-only

👆 ❓ is that combination technically possible?

which leaves readonly-classic if we don't set mount.ReadOnly there, would that make it the same as enabled ? (i.e.; nested mounts are mounted, but "read-write") ? If that's the case, then maybe we don't need that option 🤔

mount.ReadOnly = true is set here, as mount.ReadOnly == false && bindOptions().ReadOnlyXXX = true doesn't seem a valid configuration.

That's the part I was wondering; is it (technically) possible to make the "main" mount (so target) read-write, but making nested mounts read-only?

Basically; my thinking here is to make nested mounts "inherit" the readonly option as a default, with the option to differentiate (through the bind-recursive options). In that case it's (I think) easier to comprehend (by default, they'll all be attempted the same, but if I need them to be different, I can control that through this option)

That's the part I was wondering; is it (technically) possible to make the "main" mount (so target) read-write, but making nested mounts read-only?

Theoretically possible by scanning procfs, but I don't think we want to implement it

neersighted

I think this is a lot better UX-wise, with the recursion divorced from the readonly option. Nonetheless, I think it's not quite there, mostly because confusing combinations are possible and should be caught in validation instead of silently overriding each other.

That's hard, since we don't have validation yet/the way this code is structured makes it rather obtuse. Still, we could e.g. use a fixed source order with a slice that defines a strong ordering, and check that way, or we could think of another method.

Let me know if my comments make sense and I can help with figuring out how to implement them.

neersighted · 2023-09-29T13:32:16Z

opts/mount.go

+				// NOP
+			case "disabled": // alias of bind-nonrecursive=true
+				bindOptions().NonRecursive = true
+			case "readonly-classic": // conforms to Docker v24; read-only mounts are recursively mounted but not recursively read-only


readonly-classic doesn't feel right to me, as we're trying to split the "recursion" from the "readonlyness" with this.

How about writable, which makes it clear that recursive mounts will be writable/that was the legacy behavior?

I also think setting ReadOnly here is a smell -- we should instead have some check that errors when ReadOnly has not been set by the user (e.g. readonly=false,bind-recursive=readonly should be an error in validation).

Updated to "writable", but what about calling this "classic" or "legacy"?

I still think classic or legacy don't really describe what it does, and when documenting this option and the default for old engine versions, it really hammers home the old behavior. I don't object to an alias under those names, but I would prefer avoiding it as I think they're meaningless unless you have an excellent understanding of the kernel/engine history around this.

opts/mount.go

neersighted · 2023-09-29T13:33:08Z

opts/mount.go

+				mount.ReadOnly = true
+			case "readonly": // force recursively read-only, or raise an error
+				bindOptions().ReadOnlyForceRecursive = true
+				bindOptions().Propagation = mounttypes.PropagationRPrivate


I don't think we should overwrite the propagation here unconditionally. I think it's fine to set it implicitly from a UX perspective, but if the user specifies any different propagation it should be a validation error.

Hmm, I do like the trivial validation overall, but I'm not sure about this case. I don't think that requiring the user to set the propagation explicitly is good UX; I'd prefer this to be set implicitly, but to error if the user explicitly sets a different propagation. Unfortunately that doesn't work with the simple validation you've added, but maybe you have some ideas to that end?

The default implicit "rprivate" propagation sometimes flips to "rslave":

Use rslave propagation for mounts from daemon root moby/moby#36055

So I guess we should demand explicit "rprivate" here

Yeah, I had that in mind. What I meant to say is that we want to be explicit at the API level, but I'm not sure that we want to require the user to explicitly specify a propagation mode to use bind-recursive=readonly. However with the current validation, this is messy.

I am okay with what you have; it makes the feature usable, and bind-recursive=readonly is definitely a power-user feature/we're "upgrading" users by default to the new enabled behavior anyway.

How about a // TODO: to implicitly set propagation and error if the user specifies a propagation in a future refactor/UX polish pass?

neersighted · 2023-09-29T15:36:17Z

opts/mount.go

 		return fmt.Errorf("cannot mix 'tmpfs-*' options with mount type '%s'", mount.Type)
 	}

+	if mount.BindOptions != nil && mount.BindOptions.ReadOnlyForceRecursive {


This probably needs a check for ReadOnlyNonRecursive as well, as readonly=false,bind-recursive=writable makes no sense.

as readonly=false,bind-recursive=readonly

Ah, yes, that was the case I mentioned above (and was hoping it would work, as I could see use-cases for a "writable" bind-mount with nested mounts read-only), but currently not a combination that was possible; #4316 (comment)

Just to be sure; in the current implementation, does --mount type=bind,readonly,..... do the same as -v source:target:ro (so best-effort recursive), or do we need to make changes to the readonly option to make that happen (I lost track again on which combination did that, and what parts we did on the daemon side)

This probably needs a check for ReadOnlyNonRecursive as well, as readonly=false,bind-recursive=readonly makes no sense.

Done (I assume you meant readonly=false,bind-recursive=writable)

Just to be sure; in the current implementation, does --mount type=bind,readonly,..... do the same as -v source:target:ro (so best-effort recursive), or do we need to make changes to the readonly option to make that happen (I lost track again on which combination did that, and what parts we did on the daemon side)

The former one.

thaJeztah · 2023-09-29T16:15:44Z

FWIW; we discussed this PR Yesterday, and w.r.t. validation it's possible that we need to make some follow-up changes, because of the way these options are parsed (i.e., order can matter if multiple options interact with the same "API" options behind the scenes).

We are looking at doing a first beta for v25.0 soon (to get the release train warmed up), and I think for that the main goal is to have the UX "sorted" (as good as possible), but after that we can still tweak / fix validation corner cases etc.

neersighted

Overall LGTM with reservations (about the need for the user to explicitly set the propagation mode); as per my review comment, I think a TODO would be fine here, and we should get this in.

…rsive=<bool>` See `opts/mount_test.go:TestMountOptSetBindRecursive()` for the behavior. Documentation will be added separately after reaching consensus on the design. Signed-off-by: Akihiro Suda <[email protected]>

thaJeztah

LGTM

added a docs/revisit label, as we will have to do some docs follow-ups, as well as looking at completion scripts (not sure if we provide completion for these currently)

AkihiroSuda · 2023-10-16T01:14:33Z

docs:

docs: add bind-recursive mount option #4606

thaJeztah · 2023-11-22T10:26:59Z

opts/mount.go

+			valS := val
+			// Allow boolean as an alias to "enabled" or "disabled"
+			if b, err := strconv.ParseBool(valS); err == nil {
+				if b {
+					valS = "enabled"
+				} else {
+					valS = "disabled"
+				}
+			}
+			switch valS {


Opened a PR for discussion to remove the boolean variants (motivation outlined in that PR);

mount: bind-recursive: remove boolean convenience values #4671

The `bind-nonrecursive` option was replaced with the [`bind-recursive`] option (see [cli-4316], [cli-4671]). The option was still accepted, but printed a deprecation warning: bind-nonrecursive is deprecated, use bind-recursive=disabled instead In the v29.0 release, this warning is removed, and returned as an error. Users should use the equivalent `bind-recursive=disabled` option instead. [`bind-recursive`]: https://docs.docker.com/engine/storage/bind-mounts/#recursive-mounts [cli-4316]: docker#4316 [cli-4671]: docker#4671 Signed-off-by: Sebastiaan van Stijn <[email protected]>

The `bind-nonrecursive` option was replaced with the [`bind-recursive`] option (see [cli-4316], [cli-4671]), but the deprecated docs was not updated to mention. Based on abfe4d4 in master [`bind-recursive`]: https://docs.docker.com/engine/storage/bind-mounts/#recursive-mounts [cli-4316]: docker#4316 [cli-4671]: docker#4671 Signed-off-by: Sebastiaan van Stijn <[email protected]> (cherry picked from commit abfe4d4) Signed-off-by: Sebastiaan van Stijn <[email protected]>

AkihiroSuda added this to the 25.0.0 milestone May 29, 2023

AkihiroSuda requested a review from thaJeztah as a code owner May 29, 2023 12:35

AkihiroSuda mentioned this pull request May 29, 2023

Support recursively read-only (RRO) mounts moby/moby#45278

Merged

AkihiroSuda force-pushed the rro branch from c9bd3ab to 6ebf6ba Compare May 29, 2023 12:41

thaJeztah reviewed Jun 10, 2023

View reviewed changes

thaJeztah added impact/changelog kind/enhancement kind/feature status/1-design-review area/ux labels Jun 10, 2023

thaJeztah reviewed Jun 12, 2023

View reviewed changes

vendor.mod Outdated Show resolved Hide resolved

AkihiroSuda marked this pull request as draft June 12, 2023 12:00

neersighted mentioned this pull request Jul 10, 2023

25.0 Release Tracking moby/moby#45926

Closed

26 tasks

AkihiroSuda force-pushed the rro branch from 6ebf6ba to 4bd0451 Compare July 20, 2023 14:07

AkihiroSuda changed the title ~~CLI and docs for recursively read-only mounts~~ mount: add bind-recursive=<bool|string> and deprecate bind-nonrecursive=<bool> Jul 20, 2023

AkihiroSuda marked this pull request as ready for review July 20, 2023 14:10

AkihiroSuda requested a review from thaJeztah July 20, 2023 14:10

AkihiroSuda force-pushed the rro branch from 4bd0451 to 4e2be3a Compare July 20, 2023 14:21

AkihiroSuda mentioned this pull request Jul 20, 2023

volume: remove the short RRO forms in favor of the long forms moby/moby#46037

Merged

AkihiroSuda force-pushed the rro branch from 4e2be3a to 42ffc51 Compare September 28, 2023 14:19

thaJeztah reviewed Sep 28, 2023

View reviewed changes

neersighted requested changes Sep 29, 2023

View reviewed changes

AkihiroSuda force-pushed the rro branch from 42ffc51 to 69965c9 Compare September 29, 2023 14:24

neersighted reviewed Sep 29, 2023

View reviewed changes

AkihiroSuda force-pushed the rro branch from 69965c9 to 4e7c602 Compare September 30, 2023 04:36

neersighted approved these changes Sep 30, 2023

View reviewed changes

mount: add bind-recursive=<bool|string> and deprecate `bind-nonrecu…

fc6976d

…rsive=<bool>` See `opts/mount_test.go:TestMountOptSetBindRecursive()` for the behavior. Documentation will be added separately after reaching consensus on the design. Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda force-pushed the rro branch from 4e7c602 to fc6976d Compare September 30, 2023 17:58

neersighted requested a review from thaJeztah September 30, 2023 20:41

thaJeztah added status/2-code-review docs/revisit and removed status/1-design-review labels Oct 2, 2023

thaJeztah approved these changes Oct 2, 2023

View reviewed changes

thaJeztah merged commit dcc1610 into docker:master Oct 2, 2023

AkihiroSuda mentioned this pull request Oct 16, 2023

docs: add bind-recursive mount option #4606

Merged

AkihiroSuda mentioned this pull request Nov 21, 2023

recursive read-only (RRO) mount: support the Docker v25 form containerd/nerdctl#2651

Open

thaJeztah mentioned this pull request Nov 22, 2023

mount: bind-recursive: remove boolean convenience values #4671

Merged

thaJeztah reviewed Nov 22, 2023

View reviewed changes

thaJeztah mentioned this pull request Mar 9, 2025

client: add API-version dependent validation for mount options moby/moby#49613

Merged

thaJeztah added the impact/deprecation label Aug 5, 2025

thaJeztah mentioned this pull request Aug 5, 2025

remove deprecated bind-nonrecursive option for --mount #6241

Merged

thaJeztah mentioned this pull request Aug 19, 2025

[28.x] docs: add deprecated bind-nonrecursive option for --mount #6308

Merged

mount: add bind-recursive=<bool|string> and deprecate bind-nonrecursive=<bool> #4316

mount: add bind-recursive=<bool|string> and deprecate bind-nonrecursive=<bool> #4316

Uh oh!

Conversation

AkihiroSuda commented May 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented May 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AkihiroSuda commented Jun 8, 2023

Uh oh!

thaJeztah commented Jun 9, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AkihiroSuda commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Sep 28, 2023

Uh oh!

AkihiroSuda commented Sep 28, 2023

Uh oh!

thaJeztah Sep 28, 2023 • 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!

neersighted left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Sep 29, 2023 • 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!

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!

neersighted Sep 29, 2023 • 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

mount: add `bind-recursive=<bool|string>` and deprecate `bind-nonrecursive=<bool>` #4316

mount: add `bind-recursive=<bool|string>` and deprecate `bind-nonrecursive=<bool>` #4316

AkihiroSuda commented May 29, 2023 •

edited

Loading

codecov-commenter commented May 29, 2023 •

edited

Loading

AkihiroSuda commented Sep 22, 2023 •

edited

Loading

thaJeztah Sep 28, 2023 •

edited

Loading

AkihiroSuda Sep 29, 2023 •

edited

Loading

neersighted Sep 29, 2023 •

edited

Loading