mount: add BindOptions.NonRecursive (API v1.40) by AkihiroSuda · Pull Request #38003 · moby/moby

AkihiroSuda · 2018-10-10T12:00:49Z

This allows non-recursive bind-mount, i.e. mount(2) with "bind" rather than "rbind".

Signed-off-by: Akihiro Suda [email protected]

- What I did

Support non-recursive bind-mount.
Related to #37838 .

- How I did it

Added NonRecursive to BindOptions.
Bumped up API version to v1.40
Swarm-mode will be supported via a separate PR, because of mutual vendoring

- How to verify it

CLI: docker/cli#1430

$ mountpoint /run
/run is a mountpoint
$ docker run --rm --mount type=bind,src=/,target=/host,readonly busybox touch /host/run/compromise_success
$ docker run --rm --mount type=bind,src=/,target=/host,readonly,bind-nonrecursive busybox touch /host/run/compromise_fail
touch: /host/run/compromise_fail: Read-only file system
$ sudo ls /run/compromise_*
/run/compromise_success

- Description for the changelog

mount: add BindOptions.NonRecursive (API v1.40)

- A picture of a cute animal (not mandatory but encouraged)
https://upload.wikimedia.org/wikipedia/commons/2/28/Kaiserpinguine_mit_Jungen.jpg

codecov · 2018-10-10T15:11:07Z

Codecov Report

❗ No coverage uploaded for pull request base (master@12bba16). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #38003   +/-   ##
=========================================
  Coverage          ?   35.92%           
=========================================
  Files             ?      610           
  Lines             ?    45473           
  Branches          ?        0           
=========================================
  Hits              ?    16337           
  Misses            ?    26897           
  Partials          ?     2239

thaJeztah · 2018-10-10T19:11:03Z

ping @justincormack @kolyshkin ptal

thaJeztah

Left some comments about the API changes; I'm not too familiar with the actual changes for mounting, so I'll leave that to @cpuguy83 and @kolyshkin

thaJeztah · 2018-10-11T15:06:26Z

docs/api/version-history.md

Can you also mention;

GET /containers/<id>/json

GET /containers/json

And when swarmkit changes are done;

POST /services/create

POST /services/{id}/update

GET /services

GET /services/{id}

thaJeztah · 2018-10-11T15:11:13Z

api/types/mount/mount.go

Does omitempty actually work for a bool (not a pointer)? If not; perhaps we should make it a pointer so that it can be kept empty (and we can distinguish false from not set)

We should also make sure that this option is ignored on older API versions; i.e., if a request is sent to API version < 1.40, the NoRecursive option should be set to nil (or false); similar to

moby/api/server/router/container/container_routes.go

Lines 463 to 466 in 8e610b2

// When using API 1.24 and under, the client is responsible for removing the container

if hostConfig != nil && versions.LessThan(version, "1.25") {

hostConfig.AutoRemove = false

}

Same for createService;

moby/api/server/router/swarm/cluster_routes.go

Lines 189 to 195 in 9f296d1

if versions.LessThan(cliVersion, "1.39") {

if service.TaskTemplate.ContainerSpec != nil {

// Sysctls for docker swarm services weren't supported before

// API version 1.39

service.TaskTemplate.ContainerSpec.Sysctls = nil

}

}

And for updateService;

moby/api/server/router/swarm/cluster_routes.go

Lines 232 to 238 in 9f296d1

if versions.LessThan(cliVersion, "1.39") {

if service.TaskTemplate.ContainerSpec != nil {

// Sysctls for docker swarm services weren't supported before

// API version 1.39

service.TaskTemplate.ContainerSpec.Sysctls = nil

}

}

https://play.golang.org/p/gEZoDu7LdG

Does omitempty actually work for a bool (not a pointer)?

Seems yes

AkihiroSuda · 2018-10-12T09:37:19Z

Updated PR. (Swarm-mode will be separate PR)

AkihiroSuda · 2018-10-27T03:43:37Z

thanks, rebased

AkihiroSuda · 2018-10-31T03:56:58Z

renamed to NonRecursive

thaJeztah

Thanks @AkihiroSuda - I left some comments.

Erm... (not gonna make myself popular with you 😅) I was just recalling a discussion we had on another PR about options for tmpfs. (this PR: #36720)

Thinking now if we should have a dedicated boolean for this in the API, or take the same approach as was used in that PR.

@AkihiroSuda @cpuguy83 @kolyshkin wdyt?

daemon/volumes_unix.go

api/swagger.yaml

AkihiroSuda · 2018-11-02T05:48:44Z

Thinking now if we should have a dedicated boolean for this in the API, or take the same approach as was used in that PR.

I feel this PR is really specific to BindOptions

This allows non-recursive bind-mount, i.e. mount(2) with "bind" rather than "rbind". Swarm-mode will be supported in a separate PR because of mutual vendoring. Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda · 2018-11-06T08:52:23Z

updated

cpuguy83 · 2018-11-06T23:10:15Z

integration/container/mounts_linux_test.go

 }
+
+func TestContainerBindMountNonRecursive(t *testing.T) {
+	skip.If(t, testEnv.DaemonInfo.OSType != "linux" || testEnv.IsRemoteDaemon())


Could probably get rid of the remote daemon check if we do all the mounting in a container with shared propagation.

Could you be more specific? Do you mean DIND?

thaJeztah

LGTM after @cpuguy83's comment https://github.com/moby/moby/pull/38003/files#r231328643 is addressed (but I'd be ok with keeping that for a follow-up)

cpuguy83 · 2018-11-07T01:20:26Z

Yeah LGTM as is.
On mobile so I can't merge with failing ci.

From moby/moby#38003 Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda · 2018-11-07T07:53:44Z

Swarmkit PR: moby/swarmkit#2780

From moby/moby#38003 Signed-off-by: Akihiro Suda <[email protected]>

Full diff: moby/swarmkit@8af8c42...1a0ebd4 relevant changes: - swarmkit#2771 Allow using Configs as CredentialSpecs - swarmkit#2804 Make Service.UpdateStatus non-ambiguous - swarmkit#2805 Refactor condition in restart supervisor - swarmkit#2780 api: add BindOptions.NonRecursive - related to moby#38003 - swarmkit#2790 Fix possible panic if NetworkConfig is nil - swarmkit#2797 Include old error-message for backward compatibility - related to swarmkit#2779 / moby#38140 / moby#38142 Signed-off-by: Sebastiaan van Stijn <[email protected]>

AkihiroSuda · 2019-02-23T15:18:36Z

PR for Swarm-mode: #38788

Full diff: moby/swarmkit@8af8c42...1a0ebd4 relevant changes: - swarmkit#2771 Allow using Configs as CredentialSpecs - swarmkit#2804 Make Service.UpdateStatus non-ambiguous - swarmkit#2805 Refactor condition in restart supervisor - swarmkit#2780 api: add BindOptions.NonRecursive - related to moby#38003 - swarmkit#2790 Fix possible panic if NetworkConfig is nil - swarmkit#2797 Include old error-message for backward compatibility - related to swarmkit#2779 / moby#38140 / moby#38142 Signed-off-by: Sebastiaan van Stijn <[email protected]>

AkihiroSuda requested a review from vdemeester as a code owner October 10, 2018 12:00

GordonTheTurtle added the status/0-triage label Oct 10, 2018

This was referenced Oct 10, 2018

support --mount type=bind,bind-nonrecursive,... docker/cli#1430

Merged

RFC: recursive read-only bind-mount emulation using FUSE #37838

Closed

AkihiroSuda added area/security impact/api impact/changelog labels Oct 10, 2018

AkihiroSuda force-pushed the non-recursive-bind branch 5 times, most recently from 7e55bd0 to 50f54ca Compare October 10, 2018 15:11

yongtang added status/2-code-review and removed status/0-triage labels Oct 11, 2018

thaJeztah requested changes Oct 11, 2018

View reviewed changes

AkihiroSuda force-pushed the non-recursive-bind branch from 50f54ca to f18d34b Compare October 12, 2018 09:37

AkihiroSuda added rebuild/janky labels Oct 12, 2018

GordonTheTurtle removed rebuild/janky labels Oct 12, 2018

AkihiroSuda added the rebuild/janky label Oct 12, 2018

GordonTheTurtle removed the rebuild/janky label Oct 12, 2018

AkihiroSuda force-pushed the non-recursive-bind branch 2 times, most recently from 0c71dfc to d9f5a2a Compare October 14, 2018 00:22

AkihiroSuda added rebuild/windowsRS1 labels Oct 14, 2018

GordonTheTurtle removed the rebuild/windowsRS1 label Oct 14, 2018

AkihiroSuda added the rebuild/experimental label Oct 27, 2018

GordonTheTurtle removed the rebuild/experimental label Oct 27, 2018

AkihiroSuda changed the title ~~mount: add BindOptions.NoRecursive (API v1.40)~~ mount: add BindOptions.NonRecursive (API v1.40) Oct 31, 2018

AkihiroSuda force-pushed the non-recursive-bind branch from 824e04c to 42ad996 Compare October 31, 2018 03:56

AkihiroSuda added the rebuild/janky label Oct 31, 2018

GordonTheTurtle removed the rebuild/janky label Oct 31, 2018

thaJeztah requested changes Nov 1, 2018

View reviewed changes

daemon/volumes_unix.go Outdated Show resolved Hide resolved

daemon/volumes_unix.go Outdated Show resolved Hide resolved

api/swagger.yaml Outdated Show resolved Hide resolved

thaJeztah added the area/volumes Volumes label Nov 1, 2018

mount: add BindOptions.NonRecursive (API v1.40)

596cdff

This allows non-recursive bind-mount, i.e. mount(2) with "bind" rather than "rbind". Swarm-mode will be supported in a separate PR because of mutual vendoring. Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda force-pushed the non-recursive-bind branch from 42ad996 to 596cdff Compare November 6, 2018 08:52

cpuguy83 reviewed Nov 6, 2018

View reviewed changes

thaJeztah approved these changes Nov 7, 2018

View reviewed changes

thaJeztah merged commit bd224b5 into moby:master Nov 7, 2018

AkihiroSuda added a commit to AkihiroSuda/swarmkit that referenced this pull request Nov 7, 2018

api: add BindOptions.NonRecursive

bca09de

From moby/moby#38003 Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda mentioned this pull request Nov 7, 2018

api: add BindOptions.NonRecursive moby/swarmkit#2780

Merged

AkihiroSuda added a commit to AkihiroSuda/swarmkit that referenced this pull request Nov 7, 2018

api: add BindOptions.NonRecursive

8175894

From moby/moby#38003 Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda added a commit to AkihiroSuda/swarmkit that referenced this pull request Jan 5, 2019

api: add BindOptions.NonRecursive

70f9ded

From moby/moby#38003 Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda added a commit to AkihiroSuda/swarmkit that referenced this pull request Jan 5, 2019

api: add BindOptions.NonRecursive

5338701

From moby/moby#38003 Signed-off-by: Akihiro Suda <[email protected]>

thaJeztah mentioned this pull request Feb 1, 2019

Bump SwarmKit to 1a0ebd43b2d156983a695f90e56f4ecba6ced902 #38520

Merged

AkihiroSuda mentioned this pull request Jul 16, 2019

service: support --mount type=bind,bind-nonrecursive #38788

Merged

thaJeztah mentioned this pull request Sep 23, 2019

Subfolders of read-only volume are writeable docker/for-linux#788

Closed

3 tasks

	// When using API 1.24 and under, the client is responsible for removing the container
	if hostConfig != nil && versions.LessThan(version, "1.25") {
	hostConfig.AutoRemove = false
	}

	if versions.LessThan(cliVersion, "1.39") {
	if service.TaskTemplate.ContainerSpec != nil {
	// Sysctls for docker swarm services weren't supported before
	// API version 1.39
	service.TaskTemplate.ContainerSpec.Sysctls = nil
	}
	}

Conversation

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

Uh oh!

Uh oh!

codecov bot commented Oct 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah commented Oct 10, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 11, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 11, 2018

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Oct 12, 2018

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Oct 12, 2018

Uh oh!

AkihiroSuda commented Oct 27, 2018

Uh oh!

AkihiroSuda commented Oct 31, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AkihiroSuda commented Nov 2, 2018

Uh oh!

AkihiroSuda commented Nov 6, 2018

Uh oh!

cpuguy83 Nov 6, 2018

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Nov 7, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Nov 7, 2018

Uh oh!

AkihiroSuda commented Nov 7, 2018

Uh oh!

AkihiroSuda commented Feb 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

AkihiroSuda commented Oct 10, 2018 •

edited

Loading

codecov bot commented Oct 10, 2018 •

edited

Loading