Remove error return on !exists in validateMountConfigReg by cdhunt · Pull Request #35159 · moby/moby

cdhunt · 2017-10-10T15:27:43Z

We found while developing Docker Executor for Windows support for GitLab CI that the CI Runner code takes advantage of the docker daemon behavior to create directories on the host when mounting volumes. With Windows Containers, this throws an error, however, the logic to create the missing directory is platform agnostic.

- What I did
Removed the hard check if the directory exists on the host in mount config validation.
- How I did it
Changed the condition logic.
- How to verify it
Mount a volume to a directory that does not exist on the host. It should be created and not throw an error.
- Description for the changelog

Fix the inconsistent behavior between Linux and Windows containers when mounting to a directory that does not already exist.

thaJeztah · 2017-10-11T15:43:02Z

I seem to recall auto-creation was only kept around on Linux for backward compatibility (it was deprecated for a long time, but was too much of a breaking change to remove), however, on Windows it never worked, and because of that (at least at the time) it was decided to not enable it.

Some background in #19953, #21666, #16349

/cc @cpuguy83

cdhunt · 2017-10-11T16:31:12Z

Thanks, @thaJeztah. I think I concur with a lot of the sentiment in #21666 (for both sides). Specifically, we're trying to avoid interacting with the host directly and only use the Docker API. I'm pretty sure the GitLab CI Runner is already largely dependent on this feature for Linux hosts. I have validated the create logic does work on Server 2016 build 14393.1358.amd64fre.rs1_release.170602-2252.

thecloudtaylor · 2017-10-13T20:58:22Z

It seems like this behavior is here to stay on the Linux side (for good/for bad/for otherwise) for me parity across platforms is a really valuable principal especially as we roll out LCOW. The original PR (Moby: 19953) on listed the motivation for the change as "it is not reasonable.in fact,we can skip non-existant volume host path and remove "MkdirAll" from Setup.". Which doesn't give me much to go on as to why auto-creation is the wrong policy.

It seems this also tied into a change was made in runC (runc: 493). I can see a very reasonable position that runC fail in this case vs auto creation but that Docker or even ContainerD could support the auto creation of the directory as a management policy in effect.

All that to say without an good reason to depart from parity with Linux I would be in favor of accepting this (or a similar) change.

cc: @jhowardmsft @darrenstahlmsft

cpuguy83 · 2017-10-13T21:09:11Z

I agree parity would be good here so long as it works.

PatrickLang · 2017-12-07T01:01:38Z

pinging myself

thaJeztah · 2017-12-14T19:55:19Z

We discussed this in the maintainers meeting, and we're okay with lifting this restriction for Windows

ping @cdhunt can you update the PR (remove the commented-out code), so that we can move this forward?

GordonTheTurtle · 2017-12-14T20:06:38Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "master" [email protected]:cdhunt/moby.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842353698296
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

thaJeztah · 2017-12-19T15:55:09Z

volume/volume_test.go

Do we have a "positive" test for this? (i.e. this test-case should now pass, correct?)

Code wise, there's nothing new to test. The coverage is the same either way. I did change the functional test to validate no error is thrown. Do you still think we should include the positive tests?

Well, we previously tested that this should fail, but I couldn't find a test in the Windows valid list that now verifies that the test passes with a non-existing path (but I may have overlooked)

I've added valid tests for Windows and LCOW.

lowenna · 2018-01-05T00:19:09Z

Sorry, trying to catch up with my backlog. I guess my only concern with this is that docker users can now arbitrarily create folders on the host through the docker API which they can't necessarily clean up. Is that not a concern?

cpuguy83 · 2018-01-05T17:37:23Z

This is pretty consistent with linux.

thaJeztah · 2018-01-08T11:55:23Z

users can now arbitrarily create folders on the host through the docker API

Correct; it's not ideal, but it's what a lot of people use. We are recommending the --mount option, which doesn't have this behavior.

which they can't necessarily clean up

Can they be removed from a privileged/elevated session?

@cdhunt looks like this needs a rebase; also looks like there's a commit that isn't signed off

cdhunt · 2018-01-08T17:38:15Z

@thaJeztah I've rebased and it looks like all commits are signed.

lowenna · 2018-01-19T02:46:31Z

This is pretty consistent with linux.

@cpuguy83 Right, but that wasn't what I was asking. I was asking if it's the right thing to be doing.

cpuguy83 · 2018-08-13T23:48:06Z

@cdhunt Can you squash your commits?

cdhunt · 2018-08-16T15:42:23Z

@cpuguy83 Could you provide some guidance for that? When I try to squash 2 commits I end up with thousands of changes. I'm struggling to tame my messy branch history.

cpuguy83 · 2018-08-16T15:54:24Z

git rebase -i HEAD~5 and squash all down to the first commit.

thaJeztah · 2018-08-16T15:57:22Z

There's some information about squashing here as well; https://docs.docker.com/v17.06/opensource/workflow/work-issue/#pull-and-rebase-frequently

codecov · 2018-08-16T17:33:25Z

Codecov Report

Merging #35159 into master will decrease coverage by 0.23%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #35159      +/-   ##
==========================================
- Coverage   35.85%   35.61%   -0.24%     
==========================================
  Files         606      611       +5     
  Lines       44704    44960     +256     
==========================================
- Hits        16027    16014      -13     
- Misses      26469    26733     +264     
- Partials     2208     2213       +5

thaJeztah · 2018-08-17T13:16:39Z

@cdhunt thanks for squashing! Looks like there's two failures; looks related to this change;

18:34:26 --- FAIL: TestParseMountRawSplit (0.00s)
...
18:34:26 	parser_test.go:389: Expected error, was nil, for spec c:\foo\bar:\\.\pipe\foo

18:34:26 --- FAIL: TestValidateMount (0.00s)
18:34:26 	validate_test.go:58: expected "bind mount source path does not exist: c:\\foo", got %!q(<nil>), case: 8

On Linux, volume binding creates the directory on the host. This conditional check on Windows was preventing Windows Containers from creating the directory producing inconsistent behavior between platforms. Signed-off-by: Chris Hunt <[email protected]>

cdhunt · 2018-08-17T14:22:50Z

@thaJeztah Sorry about that.

thaJeztah · 2018-08-17T14:49:00Z

no worries! Just noted that it was failing (github likely won't send a notification if that's happening).

Looks like one was fixed, but one is still failing; one down; one to go! 🎉

14:40:23 --- FAIL: TestValidateMount (0.00s)
14:40:23 	validate_test.go:58: expected %!q(<nil>), got "invalid mount config for type \"bind\": bind mount source path does not exist: /foo", case: 8

cdhunt · 2018-08-17T15:35:07Z

@thaJeztah Can you point me to that result? Or maybe more specifically the test automation. I'm pretty sure this is coming from the LCOW side and I'm not sure exactly how to run those tests.

thaJeztah · 2018-08-20T07:58:35Z

I'm pretty sure this is coming from the LCOW side and I'm not sure exactly how to run those tests.

CI in this repository currently doesn't run on Windows RS3 and up (so doesn't run LCOW), so likely not related to LCOW (perhaps there's a test-case that shouldn't be run on Linux though)

You can click the links next to the failing CI runs;

Windows RS1 actually showed a panic in TestContainersAPICreateMountsValidation

17:28:48 PANIC: docker_api_containers_test.go:1706: DockerSuite.TestContainersAPICreateMountsValidation
17:28:48 
17:28:48 case 0
17:28:48 case 1
17:28:48 case 2
17:28:48 case 3
17:28:48 ... Panic: runtime error: invalid memory address or nil pointer dereference (PC=0x458341)
17:28:48 
17:28:48 d:/CI/CI-3285a6745/go/src/runtime/asm_amd64.s:573
17:28:48   in call32
17:28:48 d:/CI/CI-3285a6745/go/src/runtime/panic.go:502
17:28:48   in gopanic
17:28:48 d:/CI/CI-3285a6745/go/src/runtime/panic.go:63
17:28:48   in panicmem
17:28:48 d:/CI/CI-3285a6745/go/src/runtime/signal_windows.go:167
17:28:48   in sigpanic
17:28:48 docker_api_containers_test.go:1895
17:28:48   in DockerSuite.TestContainersAPICreateMountsValidation
17:28:48 d:/CI/CI-3285a6745/go/src/runtime/asm_amd64.s:573
17:28:48   in call32
17:28:48 d:/CI/CI-3285a6745/go/src/reflect/value.go:447
17:28:48   in Value.call
17:28:48 d:/CI/CI-3285a6745/go/src/reflect/value.go:308
17:28:48   in Value.Call
17:28:48 c:/gopath/src/github.com/docker/docker/vendor/github.com/go-check/check/check.go:816
17:28:48   in suiteRunner.forkTest.func1
17:28:48 c:/gopath/src/github.com/docker/docker/vendor/github.com/go-check/check/check.go:672
17:28:48   in suiteRunner.forkCall.func1
17:28:48 d:/CI/CI-3285a6745/go/src/runtime/asm_amd64.s:2361
17:28:48   in goexit

BMOD89 · 2018-09-09T07:45:22Z

Any news regarding this topic?

thaJeztah · 2018-10-25T16:15:58Z

@cdhunt were you still working on the test failures?

cdhunt · 2018-11-06T13:08:27Z

@thaJeztah No, I'm not set up to develop and test the affected variations. Thank you for your help over the last 13 months.

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

No worries, thanks @cdhunt !

I'll have a look around as well if there's possibly someone that has time to carry this PR

thaJeztah · 2018-11-06T15:11:58Z

@simonferquel @silvin-lubecki - perhaps of interest to you?

jasonyavorsky · 2018-11-09T09:39:15Z

This is interesting to me. If I was to get the PR in a mergeable state would it be accepted?

thaJeztah · 2018-11-09T10:44:17Z

@fastloris yes, definitely. I think the PR was almost done, but there were some complications to get the tests working correctly; if you're interested in carrying this, that would be appreciated 👍

thaJeztah · 2018-11-09T10:45:18Z

Feel free to "ping" me if you need help (e.g. to restore/copy the branch in this PR; you can download the patch if needed using this URL; https://patch-diff.githubusercontent.com/raw/moby/moby/pull/35159.patch

steve-mt · 2018-12-31T15:09:10Z

I've picked this up and opened a new PR #38465 I have questions about that original fix though to help me fix that failing tests!

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

cdhunt changed the title ~~Remove error return on !exists in validateMountConfigReg~~ [WIP] Remove error return on !exists in validateMountConfigReg Oct 10, 2017

cdhunt force-pushed the master branch from f03cb1d to 4bfb85e Compare October 10, 2017 20:42

cdhunt changed the title ~~[WIP] Remove error return on !exists in validateMountConfigReg~~ Remove error return on !exists in validateMountConfigReg Oct 11, 2017

thaJeztah added status/1-design-review and removed status/0-triage labels Oct 11, 2017

GordonTheTurtle assigned johnstep Oct 25, 2017

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Dec 14, 2017

cdhunt force-pushed the master branch from f528103 to 109765e Compare December 14, 2017 20:52

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Dec 14, 2017

thaJeztah added rebuild/* status/2-code-review and removed status/1-design-review labels Dec 19, 2017

GordonTheTurtle removed the rebuild/* label Dec 19, 2017

thaJeztah reviewed Dec 19, 2017

View reviewed changes

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Dec 20, 2017

cdhunt force-pushed the master branch from 0034c63 to 09fb5b2 Compare January 8, 2018 15:08

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jan 8, 2018

thaJeztah added impact/documentation rebuild/windowsRS1 labels Aug 2, 2018

GordonTheTurtle removed rebuild/powerpc labels Aug 2, 2018

cdhunt force-pushed the master branch from 4a605e5 to 4495d84 Compare August 16, 2018 17:33

cdhunt force-pushed the master branch from 4495d84 to 3271a84 Compare August 16, 2018 17:50

Remove error return on !exists

4bbd2e7

On Linux, volume binding creates the directory on the host. This conditional check on Windows was preventing Windows Containers from creating the directory producing inconsistent behavior between platforms. Signed-off-by: Chris Hunt <[email protected]>

cdhunt force-pushed the master branch from 3271a84 to 4bbd2e7 Compare August 17, 2018 14:21

cdhunt closed this Nov 6, 2018

steve-mt mentioned this pull request Dec 31, 2018

Remove error return on !exists #38465

Closed

Conversation

cdhunt commented Oct 10, 2017

Uh oh!

thaJeztah commented Oct 11, 2017

Uh oh!

cdhunt commented Oct 11, 2017

Uh oh!

thecloudtaylor commented Oct 13, 2017

Uh oh!

cpuguy83 commented Oct 13, 2017

Uh oh!

PatrickLang commented Dec 7, 2017

Uh oh!

thaJeztah commented Dec 14, 2017

Uh oh!

GordonTheTurtle commented Dec 14, 2017

Uh oh!

thaJeztah Dec 19, 2017

Choose a reason for hiding this comment

Uh oh!

cdhunt Dec 19, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah Dec 19, 2017

Choose a reason for hiding this comment

Uh oh!

cdhunt Dec 20, 2017

Choose a reason for hiding this comment

Uh oh!

lowenna commented Jan 5, 2018

Uh oh!

cpuguy83 commented Jan 5, 2018

Uh oh!

thaJeztah commented Jan 8, 2018

Uh oh!

cdhunt commented Jan 8, 2018

Uh oh!

lowenna commented Jan 19, 2018

Uh oh!

cpuguy83 commented Aug 13, 2018

Uh oh!

cdhunt commented Aug 16, 2018

Uh oh!

cpuguy83 commented Aug 16, 2018

Uh oh!

thaJeztah commented Aug 16, 2018

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

thaJeztah commented Aug 17, 2018

Uh oh!

cdhunt commented Aug 17, 2018

Uh oh!

thaJeztah commented Aug 17, 2018

Uh oh!

cdhunt commented Aug 17, 2018

Uh oh!

thaJeztah commented Aug 20, 2018

Uh oh!

BMOD89 commented Sep 9, 2018

Uh oh!

thaJeztah commented Oct 25, 2018

Uh oh!

cdhunt commented Nov 6, 2018

Uh oh!

thaJeztah commented Nov 6, 2018

Uh oh!

thaJeztah commented Nov 6, 2018

Uh oh!

jasonyavorsky commented Nov 9, 2018

Uh oh!

thaJeztah commented Nov 9, 2018

Uh oh!

thaJeztah commented Nov 9, 2018

Uh oh!

steve-mt commented Dec 31, 2018

Uh oh!

Reviewers

codecov bot commented Aug 16, 2018 •

edited

Loading