Remove error return on !exists by steve-mt · Pull Request #38465 · moby/moby

steve-mt · 2018-12-31T15:02:38Z

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.

- 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
Remove feature parity between Linux & Windows when mounting directory that doesn't already exist.

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

steve-mt · 2018-12-31T15:08:13Z

@thaJeztah I have created a new PR from #35159 to try and fix the tests, but I am slightly confused by the orignal fix.

On windows_parser.go we have the following change

diff --git a/volume/mounts/windows_parser.go b/volume/mounts/windows_parser.go
index ac610440436a..2ba9f0daedab 100644
--- a/volume/mounts/windows_parser.go
+++ b/volume/mounts/windows_parser.go
@@ -251,10 +251,7 @@ func (p *windowsParser) validateMountConfigReg(mnt *mount.Mount, destRegex strin
 		if err != nil {
 			return &errMountConfig{mnt, err}
 		}
-		if !exists {
-			return &errMountConfig{mnt, errBindSourceDoesNotExist(mnt.Source)}
-		}
-		if !isdir {
+		if exists && !isdir {
 			return &errMountConfig{mnt, fmt.Errorf("source path must be a directory")}
 		}

But don't we have the same check in for linux_parser.go why do need to remove the check for windows and not for the linux parser for it to create the non existant directory?

Would love some guideince! 🙇

thaJeztah · 2018-12-31T15:21:23Z

Hm, interesting; I'd have to dig a bit, but could it be that the original PR was actually changing the wrong API? (i.e. the --mount option should not accept non-existing source-paths, but the -v / --volume flag should 🤔)

@cpuguy83 you're more familiar with these; perhaps you can point faster to the right location for the change?

steve-mt · 2019-01-02T16:40:38Z

Ok so after setting up a windows environments on my Mac I tested the following and it does seem to be the error that this PR removes. But we also throw that error on linux_parser.go which the same logic.

Can it be the directory is being created before these checks are happening?

thaJeztah · 2019-01-02T17:42:50Z

Hm, good point; that would explain yes. Looking if this is where that's happening (but not clear yet where the difference is made between windows and linux 🤔 );

moby/volume/mounts/mounts.go

Lines 144 to 163 in b3e9f7b

    
           if m.Type == mounttypes.TypeBind { 
        
           	// Before creating the source directory on the host, invoke checkFun if it's not nil. One of 
        
           	// the use case is to forbid creating the daemon socket as a directory if the daemon is in 
        
           	// the process of shutting down. 
        
           	if checkFun != nil { 
        
           		if err := checkFun(m); err != nil { 
        
           			return "", err 
        
           		} 
        
           	} 
        
           	// idtools.MkdirAllNewAs() produces an error if m.Source exists and is a file (not a directory) 
        
           	// also, makes sure that if the directory is created, the correct remapped rootUID/rootGID will own it 
        
           	if err := idtools.MkdirAllAndChownNew(m.Source, 0755, rootIDs); err != nil { 
        
           		if perr, ok := err.(*os.PathError); ok { 
        
           			if perr.Err != syscall.ENOTDIR { 
        
           				return "", errors.Wrapf(err, "error while creating mount source path '%s'", m.Source) 
        
           			} 
        
           		} 
        
           	} 
        
           }

steve-mt · 2019-01-03T16:32:27Z

Ok, I think I understand what is happening now. Inside of linux_parser.go

moby/volume/mounts/linux_parser.go

Lines 84 to 89 in e78a3dc

    
           if validateBindSourceExists { 
        
           	exists, _, _ := currentFileInfoProvider.fileInfo(mnt.Source) 
        
           	if !exists { 
        
           		return &errMountConfig{mnt, errBindSourceDoesNotExist(mnt.Source)} 
        
           	} 
        
           }

When -v is being a passed we call ParseMountRaw which calls p.parseMountSpec(spec, false) so it does not validate if the source directory exists since validateBindSourceExists is false. The only time it is validated is when we call ParseMountSpec

I would imagine we want the same logic for windows is that correct for ParseMountSpec & PaeseMountRaw

thaJeztah · 2019-01-03T17:08:29Z

I would imagine we want the same logic for windows is that correct

Yes, the intent of the original PR was to make -v /non/existent/dir:/container-path work the same on Windows and Linux (i.e., /non/existent/dir being created on the host)

steve-mt · 2019-01-03T17:28:19Z

Yes, the intent of the original PR was to make -v /non/existent/dir:/container-path work the same on Windows and Linux (i.e., /non/existent/dir being created on the host)

@thaJeztah understood, but we need to add the extra check to see if it's a --mount or -v right now the windows accepts both, unlike linux.

thaJeztah · 2019-01-03T17:40:02Z

Right, yes, --mount should never create the host-directory.

cpuguy83 · 2019-01-03T17:51:09Z

Both are supported on both platforms. The only thing that needs to change is to coelesce the handling for when the host directory doesn't exist when "-v" is used.

Dockerfile.windows

GordonTheTurtle · 2019-01-07T15:49:24Z

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 "create-dirctory-on-host-for-windows" [email protected]:steveazz/moby.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354568008
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.

steve-mt · 2019-01-07T15:52:45Z

@cpuguy83 @thaJeztah I think this is ready to review can one of you take a look, appreciate any feedback! 🙇

steve-mt · 2019-01-07T15:57:01Z

I see the following failures for pwerpc build, which I am not sure it is related. Is that expected to fail?

steve-mt · 2019-01-07T16:45:29Z

There are some legit test failures that I need to look at

codecov · 2019-01-08T10:31:38Z

Codecov Report

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

@@            Coverage Diff            @@
##             master   #38465   +/-   ##
=========================================
  Coverage          ?   36.61%           
=========================================
  Files             ?      608           
  Lines             ?    45178           
  Branches          ?        0           
=========================================
  Hits              ?    16541           
  Misses            ?    26350           
  Partials          ?     2287

steve-mt · 2019-01-09T13:50:04Z

Ok, the final failure is not related to this PR and I am not sure if it should be fixed or removed altogether.

Looking at the failure below:

12:40:48 --- FAIL: TestParseMountRawSplit (0.00s)
12:40:48     parser_test.go:385: case 0
12:40:48     parser_test.go:385: case 1
12:40:48     parser_test.go:385: case 2
12:40:48     parser_test.go:385: case 3
12:40:48     parser_test.go:385: case 4
12:40:48     parser_test.go:385: case 5
12:40:48     parser_test.go:385: case 6
12:40:48     parser_test.go:385: case 7
12:40:48     parser_test.go:385: case 8
12:40:48     parser_test.go:385: case 0
12:40:48     parser_test.go:385: case 1
12:40:48     parser_test.go:385: case 2
12:40:48     parser_test.go:385: case 3
12:40:48     parser_test.go:385: case 4
12:40:48     parser_test.go:385: case 5
12:40:48     parser_test.go:385: case 6
12:40:48     parser_test.go:385: case 7
12:40:48     parser_test.go:385: case 8
12:40:48     parser_test.go:385: case 9
12:40:48     parser_test.go:385: case 10
12:40:48     parser_test.go:385: case 11
12:40:48     parser_test.go:385: case 12
12:40:48     parser_test.go:389: Expected error, was nil, for spec c:\foo\bar:\\.\pipe\foo
12:40:48     parser_test.go:385: case 0
12:40:48     parser_test.go:385: case 1
12:40:48     parser_test.go:385: case 2
12:40:48     parser_test.go:385: case 3
12:40:48     parser_test.go:385: case 4
12:40:48     parser_test.go:385: case 5
12:40:48     parser_test.go:385: case 6
12:40:48     parser_test.go:385: case 7
12:40:48     parser_test.go:385: case 8
12:40:48     parser_test.go:385: case 9
12:40:48     parser_test.go:385: case 10
12:40:48     parser_test.go:385: case 11

Looking a bit closer, it's this specific case that is failing. After checking out master, the error that is returned is &errors.errorString{s:"invalid volume specification: 'c:\\foo\\bar:\\\\.\\pipe\\foo': invalid mount config for type \"bind\": bind source path does not exist: c:\\foo\\bar"} so it's not really the expected error, it should be that the spec is invalid since one of them is a pipe and the other is not. Since this PR fixes the fact that the directory is created, it's not failing since we might have another bug somewhere.

Inside windowsDetectMountType we try and figure out what mount type it is, by taking the source, which in this case ends up being bind since the source if a directory not a named pipe. So we never hit the following validation

What is the best way to procced with this?

steve-mt · 2019-01-11T15:11:00Z

@cpuguy83 I hope I am not genearting noise for you, but mind if you review this one?

cpuguy83 · 2019-02-02T00:43:44Z

@steveazz It looks like the test was passing for invalid reasons before the change.
I have a feeling that it should be invalid to mount a dir to a named pipe.

cpuguy83 · 2019-02-02T00:47:47Z

I updated the tests so at least it's easier to tell what is going on when it fails.

cpuguy83 · 2019-02-02T00:48:39Z

Still need to deal with making sure dir -> named pipe fails (I'm assuming this should not be allowed... /cc @jhowardmsft)

tallandtree · 2019-10-31T13:44:46Z

Hi, just wondering if and when this will be solved. Can anyone give an update?
@cpuguy83 or @steveazz or @thaJeztah ?

steve-mt · 2019-10-31T14:04:10Z

@tallandtree there is one comment from me in #38465 (comment) that I'd like guidance on so we get the CI green after that it should be ready for review.

thaJeztah · 2019-10-31T14:28:11Z

Let me try getting this discussed in the maintainers meeting / review session (I might have another look at it before that).

In the meantime; could you rebase this PR? We replaced our CI with a new Jenkins, so it would be helpful to have it up to date, and have a fresh run of CI

steve-mt · 2019-10-31T16:41:44Z

Let me try getting this discussed in the maintainers meeting / review session (I might have another look at it before that).

In the meantime; could you rebase this PR? We replaced our CI with a new Jenkins, so it would be helpful to have it up to date, and have a fresh run of CI

Sure, I will try and find time to update it this week!

steve-mt · 2019-11-08T06:16:40Z

Ok I've updated an update the fixes the failure for TestParseMountRawSplit with 60f63c84bb38be6a9be8514b949c906692e8a05f but now I see a new failure in https://ci.docker.com/public/blue/rest/organizations/jenkins/pipelines/moby/branches/PR-38465/runs/4/nodes/53/steps/234/log/ I will spend some more time this weekend investigating to see if I can get the pipeline green 🙇

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: Steve Azzopardi <[email protected]>

Add a new check to validate if the source path of the volume binding exists. This is a is not always validated since in `ParseMountRaw` we don't want to validate this. This brings feature parity between the Linux parse and Windows parser, where a directory is created if the source path doesn't exists. Signed-off-by: Steve Azzopardi <[email protected]>

For consistency, testtability and reuseability sake move the string to a func that returns a formatted error. Signed-off-by: Steve Azzopardi <[email protected]>

Create a new mock specific for the test `TestValidateMount` which returns `exist` to `false` only when it's equal to `testSourcePathDoesNotExist` so that the validation is done correctly. Signed-off-by: Steve Azzopardi <[email protected]>

Signed-off-by: Brian Goff <[email protected]>

When the mount type is set to `bind` check that the target is not a named pipe. Signed-off-by: Steve Azzopardi <[email protected]>

steve-mt · 2019-11-16T15:36:22Z

@thaJeztah I managed to make the pipeline green 🎉 I had to refactor the initial implementation a bit to make it successful and not modify any existing logic. I'm not 100% sure about the code quality, I welcome any feedback!

steve-mt · 2021-10-15T19:50:22Z

Going to close this PR for now since it seems like we lost traction on this! However feel free to re-use my code if you need this!

GordonTheTurtle added the status/0-triage label Dec 31, 2018

steve-mt mentioned this pull request Dec 31, 2018

Remove error return on !exists in validateMountConfigReg #35159

Closed

steve-mt changed the title ~~Remove error return on !exists~~ WIP: Remove error return on !exists Jan 4, 2019

lowenna reviewed Jan 4, 2019

View reviewed changes

Dockerfile.windows Outdated Show resolved Hide resolved

steve-mt force-pushed the create-dirctory-on-host-for-windows branch from 287ec46 to bf1a422 Compare January 7, 2019 15:49

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jan 7, 2019

steve-mt force-pushed the create-dirctory-on-host-for-windows branch from bf1a422 to f061263 Compare January 7, 2019 15:50

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

thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Jan 7, 2019

steve-mt force-pushed the create-dirctory-on-host-for-windows branch from f061263 to f656e1f Compare January 8, 2019 10:31

steve-mt force-pushed the create-dirctory-on-host-for-windows branch 4 times, most recently from e398eff to c3c28ae Compare January 8, 2019 17:00

cpuguy83 force-pushed the create-dirctory-on-host-for-windows branch from 563e403 to bb5e756 Compare February 2, 2019 00:47

steve-mt changed the title ~~WIP: Remove error return on !exists~~ Remove error return on !exists Feb 21, 2019

thaJeztah added platform/windows status/2-code-review and removed status/0-triage labels Oct 31, 2019

steve-mt force-pushed the create-dirctory-on-host-for-windows branch 3 times, most recently from fcea52d to 60f63c8 Compare November 8, 2019 06:15

steve-mt force-pushed the create-dirctory-on-host-for-windows branch 4 times, most recently from 0329d36 to df5800a Compare November 13, 2019 19:41

Steve Azzopardi and others added 5 commits November 16, 2019 15:27

Move source must be dir err to func

ab1cd4c

For consistency, testtability and reuseability sake move the string to a func that returns a formatted error. Signed-off-by: Steve Azzopardi <[email protected]>

Add specific mock for TestValidateMount

f1c6708

Create a new mock specific for the test `TestValidateMount` which returns `exist` to `false` only when it's equal to `testSourcePathDoesNotExist` so that the validation is done correctly. Signed-off-by: Steve Azzopardi <[email protected]>

Make parser tests a little easier to read and decipher for test fails

e08e702

Signed-off-by: Brian Goff <[email protected]>

Add check for pipe for bind volume

e0293f3

When the mount type is set to `bind` check that the target is not a named pipe. Signed-off-by: Steve Azzopardi <[email protected]>

steve-mt force-pushed the create-dirctory-on-host-for-windows branch from df5800a to e0293f3 Compare November 16, 2019 14:31

steve-mt closed this Oct 15, 2021

Conversation

steve-mt commented Dec 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steve-mt commented Dec 31, 2018

Uh oh!

thaJeztah commented Dec 31, 2018

Uh oh!

steve-mt commented Jan 2, 2019

Uh oh!

thaJeztah commented Jan 2, 2019

Uh oh!

steve-mt commented Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Jan 3, 2019

Uh oh!

steve-mt commented Jan 3, 2019

Uh oh!

thaJeztah commented Jan 3, 2019

Uh oh!

cpuguy83 commented Jan 3, 2019

Uh oh!

Uh oh!

GordonTheTurtle commented Jan 7, 2019

Uh oh!

steve-mt commented Jan 7, 2019

Uh oh!

steve-mt commented Jan 7, 2019

Uh oh!

steve-mt commented Jan 7, 2019

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

steve-mt commented Jan 9, 2019

Uh oh!

steve-mt commented Jan 11, 2019

Uh oh!

cpuguy83 commented Feb 2, 2019

Uh oh!

cpuguy83 commented Feb 2, 2019

Uh oh!

cpuguy83 commented Feb 2, 2019

Uh oh!

tallandtree commented Oct 31, 2019

Uh oh!

steve-mt commented Oct 31, 2019

Uh oh!

thaJeztah commented Oct 31, 2019

Uh oh!

steve-mt commented Oct 31, 2019

Uh oh!

steve-mt commented Nov 8, 2019

Uh oh!

steve-mt commented Nov 16, 2019

Uh oh!

steve-mt commented Oct 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

steve-mt commented Dec 31, 2018 •

edited

Loading

steve-mt commented Jan 3, 2019 •

edited

Loading

codecov bot commented Jan 8, 2019 •

edited

Loading