Allow mounting a sub-directory of a named volume #39041

adshmh · 2019-04-09T19:17:58Z

fixes [feature] Allow mounting sub-directories of named volumes #32582

- What I did
Added ability to mount a subdir of a volume. Part of the work required to address #32582

- How I did it
Added the required support to api, volume/mounts package, and the daemon/cluster.

- How to verify it
An integration test has been added under integration/container/mounts_linux_test.go

- Description for the changelog
Allow mounting a sub-directory of a named volume

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

adshmh · 2019-04-09T19:21:34Z

This feature also needs PRs on docker/swarmkit and docker/cli. I will follow up with PRs for this feature against those 2 repos if this PR is approved as a first draft.

thaJeztah

Thank you! I only had a quick look at the changes, but left some comments

thaJeztah · 2019-04-10T12:17:49Z

integration/container/mounts_linux_test.go

This should work on a remote daemon as well?

Thank you for the review. Yes. This was because I was starting a daemon directly, which was not needed. I will update the test to only use a client from the test environment.

thaJeztah · 2019-04-10T12:26:01Z

integration/container/mounts_linux_test.go

We might need some more test-cases (probably good to combine them in a single test, using subtests). Thinking of;

A container using the same volume twice but with different sub-paths (also to make sure reference-counting works correctly (although I'm not sure if that makes a difference - just a quick thought)

A test where a non-existing sub-path is used (i.e., make sure if produces an error)

Multiple containers using the same volume + sub-paths? (not sure if needed; just thinking out loud; is reference-counting the same?)

What happens if the volume is used twice (either the same container or different containers), and the chosen sub-paths lead to "nested mounts"? (i.e. mount sub-path /foo/bar/baz and /foo/bar ?)

does this lead to problems, such as "unable to unmount" if one container is stopped?

what happens with NoCopy=false if the container that uses /foo/bar happens to have a subdirectory (or file) named /foo/bar/baz ?

Perhaps there's more/other (corner) cases that we should check for.

edit: I just noticed you have a test for the non-existent path below; still might be worth looking for some additional cases 😅

Thanks, I will look into adding more test cases (looks like most of these cases need integration tests), while waiting for more review comments.

I have added a test for one container mounting the same volume twice, with different sub-paths. I think a single container will count as 1 reference to the volume, regardless of how many times it mounts the volume using different sub-paths, since the reference counting for each volume is based on container ID.

Non-existent sub-path test, as you mentioned in the comment, is covered by unit tests (I added an integration test too, but perhaps it can be removed once it has served its purpose of verifying the propagation of non-existent sub-path error)

With the current implementation proposed by this PR, the sub-path logic is in the MountPoint layer, and thus will not affect the reference counting (volume name + container id combination), i.e. mounting the same volume in different containers will have the same effect on reference counts with or without sub-path option (please let me know if this requires an explicit integration test).

Regarding overlap of sub-paths:

With one or multiple containers mounting a single volume, I could not find a scenario where overlap of sub-paths would affect ability to un-mount the volume, as the the reference count remains the same. (unless there is another mechanism/aspect that I have missed). The PR now includes an integration test for verifying.

Copying contents from the FS of a container’s image into a volume currently requires the to-be-populated volume to be empty, i.e. there won’t be an overlap between the image’s FS and the specified sub-dir. The checks performed on mounts, prior to any copying to volumes, includes verifying the existence of the specified sub-path. (I have added an integration test to warn against possible changes to this behaviour).

thaJeztah · 2019-04-10T12:27:39Z

integration/container/mounts_linux_test.go

Wondering; does container.Run not fail by itself already? (i.e., do we need the poll.WaitOn to check manually?) 🤔

Looking at the code (and running a few quick tests), container.Run does not fail if the command running in the container exits with non-zero error code, which is what this test uses to make sure the mounting of the sub-dir was done correctly in the second container.

thaJeztah · 2019-04-10T12:30:55Z

volume/mounts/windows_parser.go

I see there's other lines like this in this file, so probably this is correct (just thinking about the LCOW situation, but not sure if we hit this part of the code for that)

It seems the LCOW parser uses the windows parser, except for a different regular expression for the destination:

moby/volume/mounts/lcow_parser.go

Lines 20 to 22 in 6d18c6a

type lcowParser struct {

windowsParser

}

moby/volume/mounts/lcow_parser.go

Lines 32 to 33 in 6d18c6a

func (p *lcowParser) ParseMountSpec(cfg mount.Mount) (*MountPoint, error) {

return p.parseMountSpec(cfg, rxLCOWDestination, false, lcowSpecificValidators)

I will update the test case in parser_test.go to cover the conversion of the path separator.

thaJeztah · 2019-04-10T12:32:24Z

@cpuguy83 @kolyshkin ptal

Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>

codecov · 2019-04-16T17:34:46Z

Codecov Report

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

@@            Coverage Diff            @@
##             master   #39041   +/-   ##
=========================================
  Coverage          ?   37.04%           
=========================================
  Files             ?      612           
  Lines             ?    45421           
  Branches          ?        0           
=========================================
  Hits              ?    16828           
  Misses            ?    26306           
  Partials          ?     2287

Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>

adshmh · 2019-04-16T21:20:16Z

Other than the expected failure on vendor, all the other failures are due to the following integration test:

15:44:28 ----------------------------------------------------------------------
15:44:28 FAIL: docker_cli_pull_test.go:273: DockerSuite.TestPullWindowsImageFailsOnLinux
15:44:28 
15:44:28 assertion failed: expected error to contain "cannot be used on this platform", got "\nCommand:  /usr/local/cli/docker pull microsoft/nanoserver\nExitCode: 1\nError:    exit status 1\nStdout:   Using default tag: latest\n\nStderr:   Error response from daemon: manifest for microsoft/nanoserver:latest not found: manifest unknown: manifest unknown\n\n\nFailures:\nExitCode was 1 expected 0\nExpected no error"
15:44:28

AkihiroSuda · 2020-05-04T19:57:13Z

needs rebase

AkihiroSuda · 2020-05-04T20:01:23Z

integration/container/mounts_linux_test.go

+	err = client.ContainerRemove(ctx, cID, types.ContainerRemoveOptions{Force: true})
+	assert.NilError(t, err)
+	return volumeName
+}


We should have tests for SubPath: "../../../../../../../../../.."

should we use something like https://github.com/cyphar/filepath-securejoin ?

AkihiroSuda · 2020-05-04T20:04:25Z

volume/mounts/mounts.go

+	}
+
+	if !strings.HasPrefix(realPath, volumePath) {
+		return "", errors.Errorf("directory %s outside of the volume root %s", realPath, volumePath)


Let's prohibit all .. symbols statically before calling filepath.Join

GordonTheTurtle added the status/0-triage label Apr 9, 2019

thaJeztah added area/volumes Volumes status/2-code-review status/needs-vendoring and removed status/0-triage labels Apr 10, 2019

thaJeztah requested changes Apr 10, 2019

View reviewed changes

add subpath option to api and mounts package

5ce524a

Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>

adshmh force-pushed the feat-32582-mount-subpath branch from cfdcd8b to b51c044 Compare April 16, 2019 17:34

add subpath volume option to daemon

f65a159

Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>

adshmh force-pushed the feat-32582-mount-subpath branch from b51c044 to f65a159 Compare April 16, 2019 18:04

GordonTheTurtle assigned johnstep Apr 24, 2019

thaJeztah mentioned this pull request Jun 25, 2019

Proposal: mount from image #30449

Closed

AkihiroSuda reviewed May 4, 2020

View reviewed changes

thaJeztah mentioned this pull request Jun 15, 2023

volumes: Implement subpath mount #45687

Merged

thaJeztah closed this in #45687 Jan 19, 2024

	func (p lcowParser) ParseMountSpec(cfg mount.Mount) (MountPoint, error) {
	return p.parseMountSpec(cfg, rxLCOWDestination, false, lcowSpecificValidators)

Allow mounting a sub-directory of a named volume #39041

Allow mounting a sub-directory of a named volume #39041

Uh oh!

Conversation

adshmh commented Apr 9, 2019 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adshmh commented Apr 9, 2019

Uh oh!

thaJeztah left a comment

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!

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!

thaJeztah commented Apr 10, 2019

Uh oh!

codecov bot commented Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

adshmh commented Apr 16, 2019

Uh oh!

AkihiroSuda commented May 4, 2020

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adshmh commented Apr 9, 2019 •

edited by thaJeztah

Loading

codecov bot commented Apr 16, 2019 •

edited

Loading