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 expanded syntax of ports in `docker stack deploy` #30476

Merged
merged 2 commits into from Feb 11, 2017

Conversation

@yongtang
Member

yongtang commented Jan 26, 2017

- What I did

NOTE: The description has been updated based on #30476 (comment)

This fix tries to address the issue in #30447 where it was not possible to specify mode=host for ports in compose.

- How I did it

This fix addresses the issue by adding support for expanded ports syntax:

version: "3.1"
services:
  nginx:
    image: nginx
    ports:
      - mode: host
        target: 80
        published: 9005

- How to verify it

Several unit test has been added to cover the changes.

- Description for the changelog

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

This fix fixes #30447.

This fix is related to #29961/#30103.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Jan 26, 2017

Member

Thanks for looking into this. Unfortunately I don't think this is the right approach.

I agree we should support a "long form" syntax for this field, but I don't think it should be the csv format. The csv format is a compromise because of limitations on the CLI. For the Compose format we should use a mapping structure with keys and values:

ports:
  - mode: host
    target: 80
    published: 9005

I'm about to push a PR that does this for volumes, it should be up later today.

Another related issue is #29193

Member

dnephin commented Jan 26, 2017

Thanks for looking into this. Unfortunately I don't think this is the right approach.

I agree we should support a "long form" syntax for this field, but I don't think it should be the csv format. The csv format is a compromise because of limitations on the CLI. For the Compose format we should use a mapping structure with keys and values:

ports:
  - mode: host
    target: 80
    published: 9005

I'm about to push a PR that does this for volumes, it should be up later today.

Another related issue is #29193

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 30, 2017

Member

Thanks @dnephin for the insight and help. I will spend some time to try to get the new mapping structure working. Still trying to learn the mechanism of compose schema though I think I am close.

Member

yongtang commented Jan 30, 2017

Thanks @dnephin for the insight and help. I will spend some time to try to get the new mapping structure working. Still trying to learn the mechanism of compose schema though I think I am close.

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Jan 30, 2017

Member

@yongtang you might be interested in #30521 which is some cleanup of the compose loader, and https://github.com/dnephin/docker/tree/add-expanded-mount-format-to-stack-deploy which is the branch for expanded mount format (that I thought I would have finished last week).

Member

dnephin commented Jan 30, 2017

@yongtang you might be interested in #30521 which is some cleanup of the compose loader, and https://github.com/dnephin/docker/tree/add-expanded-mount-format-to-stack-deploy which is the branch for expanded mount format (that I thought I would have finished last week).

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 30, 2017

Member

Yes, I agree with @dnephin here; @yongtang should we close this PR, or were you planning to update this one with the new approach?

Member

thaJeztah commented Jan 30, 2017

Yes, I agree with @dnephin here; @yongtang should we close this PR, or were you planning to update this one with the new approach?

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 30, 2017

Member

Thanks @dnephin and @thaJeztah for the help, and pointing me to #30521. Very useful to understand the work flow of compose schema. I will update the PR with the new mapping structure as discussed.

Member

yongtang commented Jan 30, 2017

Thanks @dnephin and @thaJeztah for the help, and pointing me to #30521. Very useful to understand the work flow of compose schema. I will update the PR with the new mapping structure as discussed.

@yongtang yongtang changed the title from Support long syntax of ports in `docker stack deploy` to Support expanded syntax of ports in `docker stack deploy` Jan 31, 2017

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 31, 2017

Member

@dnephin @thaJeztah @vdemeester The PR has been updated with the support for expanded port syntax. I only changed Compose schema for 3.1. Not entirely sure if this is the expected behavior. So please take a look and let me know if there are any issues.

Member

yongtang commented Jan 31, 2017

@dnephin @thaJeztah @vdemeester The PR has been updated with the support for expanded port syntax. I only changed Compose schema for 3.1. Not entirely sure if this is the expected behavior. So please take a look and let me know if there are any issues.

@dnephin

Design LGTM,

few comments

Show outdated Hide outdated cli/compose/schema/data/config_schema_v3.1.json Outdated
Show outdated Hide outdated cli/compose/loader/loader_test.go Outdated
Show outdated Hide outdated cli/compose/schema/data/config_schema_v3.1.json Outdated
Show outdated Hide outdated cli/compose/schema/data/config_schema_v3.1.json Outdated
Show outdated Hide outdated cli/compose/loader/loader.go Outdated
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Feb 1, 2017

Member

Thanks @dnephin for the review. The PR has been updated. Please take a look and let me know if there are any additional issues.

Member

yongtang commented Feb 1, 2017

Thanks @dnephin for the review. The PR has been updated. Please take a look and let me know if there are any additional issues.

@dnephin

dnephin approved these changes Feb 1, 2017

LGTM

Thanks!

@vdemeester

LGTM 🐸
@yongtang needs a rebase 😅

yongtang added some commits Jan 31, 2017

Add expanded port syntax to Compose schema and types.
This commit adds expanded port syntax to Compose schema and types
so that it is possible to have
```
ports:
  - mode: host
    target: 80
    published: 9005
```

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Support expanded ports in Compose loader
This commit adds support for expanded ports in Compose loader,
and add several unit tests for loading expanded port format.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Feb 7, 2017

Member

Thanks @vdemeester The PR has been rebased.

Member

yongtang commented Feb 7, 2017

Thanks @vdemeester The PR has been rebased.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 11, 2017

Member

All green and two LGTMS

Member

thaJeztah commented Feb 11, 2017

All green and two LGTMS

@thaJeztah thaJeztah merged commit e5066ef into moby:master Feb 11, 2017

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 30477 has succeeded
Details
janky Jenkins build Docker-PRs 39093 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 10147 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Feb 11, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 11, 2017

Member

@dnephin do the changes to the compose format have to be synced back to the docker-compose repository? Also could you help getting these changes into the docs?

Member

thaJeztah commented Feb 11, 2017

@dnephin do the changes to the compose format have to be synced back to the docker-compose repository? Also could you help getting these changes into the docs?

@yongtang yongtang deleted the yongtang:30447-port-config-long-syntax branch Feb 11, 2017

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Feb 16, 2017

Member

Yes, and I guess so.

Member

dnephin commented Feb 16, 2017

Yes, and I guess so.

@friism

This comment has been minimized.

Show comment
Hide comment
@friism

friism Mar 10, 2017

Contributor

@johnstep if you have a second and if you haven't already, could you test this on Windows? @brandonroyal took a quick look and couldn't get it working.

cc @icecrime

Contributor

friism commented Mar 10, 2017

@johnstep if you have a second and if you haven't already, could you test this on Windows? @brandonroyal took a quick look and couldn't get it working.

cc @icecrime

@sixeyed

This comment has been minimized.

Show comment
Hide comment
@sixeyed

sixeyed Mar 13, 2017

@friism this is working for Windows in 17.04. Sample compose file:

version: "3.1"
services:
  iis:
    image: microsoft/iis
    ports:
      - mode: host
        target: 80
        published: 80

Runs with docker stack deploy --compose-file docker-compose.yml iis.

cc @johnstep.

sixeyed commented Mar 13, 2017

@friism this is working for Windows in 17.04. Sample compose file:

version: "3.1"
services:
  iis:
    image: microsoft/iis
    ports:
      - mode: host
        target: 80
        published: 80

Runs with docker stack deploy --compose-file docker-compose.yml iis.

cc @johnstep.

@friism

This comment has been minimized.

Show comment
Hide comment
@friism

friism Mar 13, 2017

Contributor

@johnstep hey, @sixeyed tested this on Windows with 17.04 (master) server and client and it worked.

@dnephin one funny thing we found (sorry if you already thought of this) is that the error message for pre 17.04 clients is really bad (this is using the compose file that @sixeyed posted above):

services.iis.ports.0 must be a string or number
Contributor

friism commented Mar 13, 2017

@johnstep hey, @sixeyed tested this on Windows with 17.04 (master) server and client and it worked.

@dnephin one funny thing we found (sorry if you already thought of this) is that the error message for pre 17.04 clients is really bad (this is using the compose file that @sixeyed posted above):

services.iis.ports.0 must be a string or number
@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Mar 14, 2017

Member

What's bad about that error message? It seems to provide the necessary information. It has the path to offending value, and the problem with the value.

Member

dnephin commented Mar 14, 2017

What's bad about that error message? It seems to provide the necessary information. It has the path to offending value, and the problem with the value.

@friism

This comment has been minimized.

Show comment
Hide comment
@friism

friism Mar 14, 2017

Contributor

@dnephin it seems not good if the actual solution is "upgrade to Docker 17.04"

Contributor

friism commented Mar 14, 2017

@dnephin it seems not good if the actual solution is "upgrade to Docker 17.04"

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#30476 from yongtang/30447-port-config-long-sy…
…ntax

Support expanded syntax of ports in `docker stack deploy`
@raarts

This comment has been minimized.

Show comment
Hide comment
@raarts

raarts May 23, 2017

Just a note for whoever reads this:

Using the long port format requires version: "3.2", not 3.1.

raarts commented May 23, 2017

Just a note for whoever reads this:

Using the long port format requires version: "3.2", not 3.1.

@karthick0070

This comment has been minimized.

Show comment
Hide comment
@karthick0070

karthick0070 Sep 14, 2017

hi,

@sixeyed I have tried this below stack file but i can't connect from out side of the host. Can you please suggest me

version: "3.2"
services:
 web:
  image: iiswithdb
  ports:
   - 8080:8080
   - mode: host
  deploy:
   replicas: 3

Below is this Docker ps out put. I think its publishing to a different port

CONTAINER ID        IMAGE                                      COMMAND                   CREATED             STATUS                  PORTS                                    NAMES
a24cee0efb74        iiswithdb:latest                           "C:\\ServiceMonitor..."   4 minutes ago       Up Less than a second   80/tcp, 8080/tcp, 0.0.0.0:37799->0/tcp   test_web.2.mnbq86t6bgifkkuycww0udqp5

karthick0070 commented Sep 14, 2017

hi,

@sixeyed I have tried this below stack file but i can't connect from out side of the host. Can you please suggest me

version: "3.2"
services:
 web:
  image: iiswithdb
  ports:
   - 8080:8080
   - mode: host
  deploy:
   replicas: 3

Below is this Docker ps out put. I think its publishing to a different port

CONTAINER ID        IMAGE                                      COMMAND                   CREATED             STATUS                  PORTS                                    NAMES
a24cee0efb74        iiswithdb:latest                           "C:\\ServiceMonitor..."   4 minutes ago       Up Less than a second   80/tcp, 8080/tcp, 0.0.0.0:37799->0/tcp   test_web.2.mnbq86t6bgifkkuycww0udqp5
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Sep 15, 2017

Member

@karthick0070 that format is incorrect; either use the "shorthand" format (just 8080:8080) or the long format (where you specify target: and published:);

version: "3.2"
services:
 web:
  image: iiswithdb
  ports:
    - mode: host
      target: 8080
      published: 8080
  deploy:
   replicas: 3
Member

thaJeztah commented Sep 15, 2017

@karthick0070 that format is incorrect; either use the "shorthand" format (just 8080:8080) or the long format (where you specify target: and published:);

version: "3.2"
services:
 web:
  image: iiswithdb
  ports:
    - mode: host
      target: 8080
      published: 8080
  deploy:
   replicas: 3
@karthick0070

This comment has been minimized.

Show comment
Hide comment
@karthick0070

karthick0070 Sep 18, 2017

@thaJeztah Thanks for your help... it is working fine as excepted.

karthick0070 commented Sep 18, 2017

@thaJeztah Thanks for your help... it is working fine as excepted.

@DianaDai

This comment has been minimized.

Show comment
Hide comment
@DianaDai

DianaDai Dec 18, 2017

I still has this program "services.web.ports.0 must be a string or number" when run this
version: "3.2"
services:
web:
image: iiswithdb
ports:

  • 8080:8080
  • mode: host
    deploy:
    replicas: 3
    my docker version is
    Docker version 17.10.0-ce, build f4ffd25

how can I solve this program @friism @sixeyed @thaJeztah

DianaDai commented Dec 18, 2017

I still has this program "services.web.ports.0 must be a string or number" when run this
version: "3.2"
services:
web:
image: iiswithdb
ports:

  • 8080:8080
  • mode: host
    deploy:
    replicas: 3
    my docker version is
    Docker version 17.10.0-ce, build f4ffd25

how can I solve this program @friism @sixeyed @thaJeztah

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