New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support parsing of named pipes for compose volumes #560

Merged
merged 1 commit into from Oct 3, 2017

Conversation

Projects
None yet
6 participants
@dnephin
Collaborator

dnephin commented Sep 26, 2017

Related to moby/moby#34795

Fixes the client side parsing of volume specs to support windows named pipes.

Support parsing of named pipes for compose volumes.
Signed-off-by: Daniel Nephin <dnephin@docker.com>
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 26, 2017

Codecov Report

Merging #560 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #560      +/-   ##
==========================================
+ Coverage    49.5%   49.51%   +<.01%     
==========================================
  Files         208      208              
  Lines       17153    17156       +3     
==========================================
+ Hits         8491     8494       +3     
  Misses       8230     8230              
  Partials      432      432

codecov-io commented Sep 26, 2017

Codecov Report

Merging #560 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #560      +/-   ##
==========================================
+ Coverage    49.5%   49.51%   +<.01%     
==========================================
  Files         208      208              
  Lines       17153    17156       +3     
==========================================
+ Hits         8491     8494       +3     
  Misses       8230     8230              
  Partials      432      432
@StefanScherer

Thanks for starting this PR.
To have Docker as a tool that gets out of the users way please support (unix) slashes as well. ❤️ 🙏 😃

@@ -112,6 +112,11 @@ func isFilePath(source string) bool {
return true
}
// windows named pipes
if strings.HasPrefix(source, `\\`) {

This comment has been minimized.

@StefanScherer

StefanScherer Sep 26, 2017

Contributor

I wonder if we can make this slash-insensitive (like case-insensitive, but for slashes ;-) ).
From a Mac/Linux client remote controlling a Windows swarm node I guess many people still are used to unix slashes.

I also use eg. docker run -v //./pipe/docker_engine://./pipe/docker_engine -it -p 8080:8080 stefanscherer/visualizer-windows:insider right now and it works pretty good as Golang itself can handle backslashes and slashes in paths.

Slashes because bash needs extra escaped backslashes as you can see here.

´´´
$ docker run -v \.\pipe\docker_engine:\.\pipe\docker_engine -it -p 8080:8080 stefanscherer/visualizer-windows:insider
docker: Error response from daemon: invalid bind mount spec "\.pipedocker_engine:\.pipedocker_engine": invalid volume specification: '.pipedocker_engine:.pipedocker_engine'.



(oops as you can see even GitHub looses some backslashes although I'm using three backticks here)
@StefanScherer

StefanScherer Sep 26, 2017

Contributor

I wonder if we can make this slash-insensitive (like case-insensitive, but for slashes ;-) ).
From a Mac/Linux client remote controlling a Windows swarm node I guess many people still are used to unix slashes.

I also use eg. docker run -v //./pipe/docker_engine://./pipe/docker_engine -it -p 8080:8080 stefanscherer/visualizer-windows:insider right now and it works pretty good as Golang itself can handle backslashes and slashes in paths.

Slashes because bash needs extra escaped backslashes as you can see here.

´´´
$ docker run -v \.\pipe\docker_engine:\.\pipe\docker_engine -it -p 8080:8080 stefanscherer/visualizer-windows:insider
docker: Error response from daemon: invalid bind mount spec "\.pipedocker_engine:\.pipedocker_engine": invalid volume specification: '.pipedocker_engine:.pipedocker_engine'.



(oops as you can see even GitHub looses some backslashes although I'm using three backticks here)

This comment has been minimized.

@dnephin

dnephin Sep 26, 2017

Collaborator

Normal slash is already handled in this function on the line above (111). A single / already returns true.

@dnephin

dnephin Sep 26, 2017

Collaborator

Normal slash is already handled in this function on the line above (111). A single / already returns true.

This comment has been minimized.

@StefanScherer

StefanScherer Sep 26, 2017

Contributor

Nice!

@StefanScherer

StefanScherer Sep 26, 2017

Contributor

Nice!

@vdemeester vdemeester requested a review from simonferquel Sep 26, 2017

@StefanScherer

This comment has been minimized.

Show comment
Hide comment
@StefanScherer

StefanScherer Sep 26, 2017

Contributor

It's so convenient to pick the Docker CLI from CircleCI build. So I gave it a try

$ wget https://5607-88013053-gh.circle-artifacts.com/0/work/build/docker-darwin-amd64
$ chmod +x docker-darwin-amd64
$ ./docker-darwin-amd64 service create --name whoami --mount type=bind,source=//./pipe/docker_engine,destination=//./pipe/docker_engine stefanscherer/whoami:insider
vkwsqpgcgm3nso31nw5no8fcx
overall progress: 0 out of 1 tasks 
1/1: invalid mount target, must be an absolute path: //./pipe/docker_engine 

And with backslashes

$ ./docker-darwin-amd64 service create --name whoami --mount 'type=bind,source=\\.\pipe\docker_engine,destination=\\.\pipe\docker_engine' stefanscherer/whoami:insider
cruvatiet5yhuxta2teppqw8b
overall progress: 0 out of 1 tasks 
1/1: invalid mount target, must be an absolute path: \\.\pipe\docker_engine 

running against a Windows Insider with Docker engine 17.09.0-rc2 installed.

Is this another check in cli or in engine?

Contributor

StefanScherer commented Sep 26, 2017

It's so convenient to pick the Docker CLI from CircleCI build. So I gave it a try

$ wget https://5607-88013053-gh.circle-artifacts.com/0/work/build/docker-darwin-amd64
$ chmod +x docker-darwin-amd64
$ ./docker-darwin-amd64 service create --name whoami --mount type=bind,source=//./pipe/docker_engine,destination=//./pipe/docker_engine stefanscherer/whoami:insider
vkwsqpgcgm3nso31nw5no8fcx
overall progress: 0 out of 1 tasks 
1/1: invalid mount target, must be an absolute path: //./pipe/docker_engine 

And with backslashes

$ ./docker-darwin-amd64 service create --name whoami --mount 'type=bind,source=\\.\pipe\docker_engine,destination=\\.\pipe\docker_engine' stefanscherer/whoami:insider
cruvatiet5yhuxta2teppqw8b
overall progress: 0 out of 1 tasks 
1/1: invalid mount target, must be an absolute path: \\.\pipe\docker_engine 

running against a Windows Insider with Docker engine 17.09.0-rc2 installed.

Is this another check in cli or in engine?

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Sep 26, 2017

Collaborator

Yes, as I mentioned in moby/moby#34795 (comment), this only fixes the client side. docker service create does not do any parsing on the client side. This still needs to be fixed in moby/moby (daemon/cluster/executor/container/validate.go).

Collaborator

dnephin commented Sep 26, 2017

Yes, as I mentioned in moby/moby#34795 (comment), this only fixes the client side. docker service create does not do any parsing on the client side. This still needs to be fixed in moby/moby (daemon/cluster/executor/container/validate.go).

@thaJeztah

LGTM

@vdemeester

LGTM 🐸

@vdemeester vdemeester merged commit 448d56a into docker:master Oct 3, 2017

9 checks passed

ci/circleci: cross Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: shellcheck Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: validate Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 50%)
Details
codecov/project 49.51% (+<.01%) compared to b77f3fd
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
dco-signed All commits are signed

@GordonTheTurtle GordonTheTurtle added this to the 17.11.0 milestone Oct 3, 2017

@dnephin dnephin deleted the dnephin:fix-bind-mount-named-pipe-compose branch Oct 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment