Skip to content
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

Allow mount type npipe on service/stack #37400

Merged
merged 2 commits into from
Sep 26, 2018

Conversation

olljanat
Copy link
Contributor

@olljanat olljanat commented Jul 5, 2018

- What I did
Added support named pipes support to cluster executor.

- How I did it
Added npipe type same way than it is added to engine on #33852

- How to verify it

  1. Create service to Windows using Windows mounting format:
docker service create --name windows --mount 'type=npipe,source=\\.\pipe\docker_engine,destination=\\.\pipe\docker_engine' microsoft/nanoserver:1803
  1. Deploy test stack using type=npipe:
version: "3.3"
services:
  web:
    image: ollijanatuinen/portainer:windows1803-amd64-npipe-3
    ports:
      - "9000:9000"
    networks:
      - test
    volumes:
      - type: npipe
        source: \\.\pipe\docker_engine
        target: \\.\pipe\docker_engine

networks:
  test:
    driver: overlay

- Description for the changelog

Fixes: #34795

@olljanat olljanat changed the title Allow Windows name pipes on bind mount Allow Windows named pipes on bind mount Jul 5, 2018
@codecov
Copy link

codecov bot commented Jul 6, 2018

Codecov Report

Merging #37400 into master will increase coverage by <.01%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##           master   #37400      +/-   ##
==========================================
+ Coverage    36.1%   36.11%   +<.01%     
==========================================
  Files         610      610              
  Lines       45115    45120       +5     
==========================================
+ Hits        16291    16295       +4     
- Misses      26584    26588       +4     
+ Partials     2240     2237       -3

@thaJeztah
Copy link
Member

The DNS lookup failures on s390x are known, and are being investigated; may be something with the kernel configuration on the s390x and powerpc machines (a reboot of those machines will fix that)

07:55:14 W: Failed to fetch http://cdn-fastly.deb.debian.org/debian/dists/stretch/InRelease  Could not resolve 'cdn-fastly.deb.debian.org'
07:55:14 W: Failed to fetch http://security.debian.org/debian-security/dists/stretch/updates/InRelease  Could not resolve 'security.debian.org'
07:55:14 W: Failed to fetch http://cdn-fastly.deb.debian.org/debian/dists/stretch-updates/InRelease  Could not resolve 'cdn-fastly.deb.debian.org'
07:55:14 W: Some index files failed to download. They have been ignored, or old ones used instead.

@thaJeztah
Copy link
Member

Haven't seen this one before I think; https://jenkins.dockerproject.org/job/Docker-PRs-WoW-RS1/21347/console, could be related to the changes in this PR?

08:31:49 PANIC: docker_api_containers_test.go:1705: DockerSuite.TestContainersAPICreateMountsValidation
08:31:49 
08:31:49 case 0
08:31:49 case 1
08:31:49 case 2
08:31:49 case 3
08:31:49 case 4
08:31:49 case 5
08:31:49 case 6
08:31:49 case 7
08:31:49 case 8
08:31:49 ... Panic: runtime error: invalid memory address or nil pointer dereference (PC=0x458341)
08:31:49 
08:31:49 d:/CI/CI-1e14ec04c/go/src/runtime/asm_amd64.s:573
08:31:49   in call32
08:31:49 d:/CI/CI-1e14ec04c/go/src/runtime/panic.go:502
08:31:49   in gopanic
08:31:49 d:/CI/CI-1e14ec04c/go/src/runtime/panic.go:63
08:31:49   in panicmem
08:31:49 d:/CI/CI-1e14ec04c/go/src/runtime/signal_windows.go:167
08:31:49   in sigpanic
08:31:49 docker_api_containers_test.go:1894
08:31:49   in DockerSuite.TestContainersAPICreateMountsValidation
08:31:49 d:/CI/CI-1e14ec04c/go/src/runtime/asm_amd64.s:573
08:31:49   in call32
08:31:49 d:/CI/CI-1e14ec04c/go/src/reflect/value.go:447
08:31:49   in Value.call
08:31:49 d:/CI/CI-1e14ec04c/go/src/reflect/value.go:308
08:31:49   in Value.Call
08:31:49 c:/gopath/src/github.com/docker/docker/vendor/github.com/go-check/check/check.go:816
08:31:49   in suiteRunner.forkTest.func1
08:31:49 c:/gopath/src/github.com/docker/docker/vendor/github.com/go-check/check/check.go:672
08:31:49   in suiteRunner.forkCall.func1
08:31:49 d:/CI/CI-1e14ec04c/go/src/runtime/asm_amd64.s:2361
08:31:49

@olljanat
Copy link
Contributor Author

olljanat commented Jul 6, 2018

@thaJeztah ok. thanks, now I know which ones to focus and which not.

@cpuguy83
Copy link
Member

cpuguy83 commented Jul 6, 2018

I think I would prefer having a proper type for this, support for this is already in the moby API's at least. Just need to add to swarmkit.

For reference https://github.com/moby/moby/blob/master/api/types/mount/mount.go#L19

@olljanat
Copy link
Contributor Author

olljanat commented Jul 6, 2018

@cpuguy83 true. I noticed same and commented to original issue #34795 (comment)

I'm just looking for it :)

@olljanat olljanat changed the title Allow Windows named pipes on bind mount WIP: Allow mount type npipe on Windows Jul 6, 2018
@GordonTheTurtle GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jul 6, 2018
@GordonTheTurtle
Copy link

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 "34795-allow-npipe" git@github.com:olljanat/moby.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842353872088
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.

@GordonTheTurtle GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature and removed dco/no Automatically set by a bot when one of the commits lacks proper signature labels Jul 6, 2018
@GordonTheTurtle GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jul 6, 2018
@olljanat
Copy link
Contributor Author

olljanat commented Jul 6, 2018

Modified solution to use MountTypeNamedPipe.

Now all envs gives error

daemon/cluster/executor/container/validate.go:35:8: undefined: api.MountTypeNamedPipe

which is valid until moby/swarmkit#2691 is merged.

@olljanat
Copy link
Contributor Author

olljanat commented Jul 7, 2018

@thaJeztah how I can actually test to build this one with moby/swarmkit#2691 ?
I can see that there is lot of "bump vendor" messages on git history but I didn't find guide how to do that on my local env.

EDIT: Found it. Need to modify vendor.conf and run hack/vendor.sh on Linux and then sync these changes to Windows machine.

Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com>
Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com>
@olljanat
Copy link
Contributor Author

olljanat commented Sep 16, 2018

@cpuguy83 now PR to swarmkit have been merged and I included vendor update for it to this PR.

@vdemeester I also tried to include cli tests on olljanat@b72a08b but I removed it now as it looked to causing more issues than solving (it was implemented based on Tmpfs tests and I'm not sure how to convert it to new format and I also noticed that it would fail because current Windows build server is running RS1 and named pipes mounting needs at least RS3, look: #37862)

Let me know if you think that more tests are needed to included.

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM 🐯
cc @johnstep @jhowardmsft

@olljanat
Copy link
Contributor Author

I noticed that swarmkit was updated to needed level on PR #3790 so I dropped it from this PR.

@thaJeztah / @johnstep / @jhowardmsft can you trigger rebuild for these failing builds (they looks failing to some some issue which is not caused by this PR) and review / do merging (or what actually are next step) ?

@yongtang
Copy link
Member

Looks like all test passed so merge.

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

Successfully merging this pull request may close these issues.

6 participants