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

[1.12] docker service update --publish-rm "[port]:[port]" does not unpublish ports #25200

Closed
mike-hearn opened this issue Jul 29, 2016 · 11 comments
Labels
area/networking area/swarm area/ux kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed. priority/P1 Important: P1 issues are a top priority and a must-have for the next release. status/claimed version/1.12
Milestone

Comments

@mike-hearn
Copy link

Output of docker version:

Client:
 Version:      1.12.0
 API version:  1.24
 Go version:   go1.6.3
 Git commit:   8eab29e
 Built:        Thu Jul 28 22:11:10 2016
 OS/Arch:      linux/amd64

Server:
 Version:      1.12.0
 API version:  1.24
 Go version:   go1.6.3
 Git commit:   8eab29e
 Built:        Thu Jul 28 22:11:10 2016
 OS/Arch:      linux/amd64

Output of docker info:

root@testserver:~# docker info
Containers: 20
 Running: 10
 Paused: 0
 Stopped: 10
Images: 1
Server Version: 1.12.0
Storage Driver: aufs
 Root Dir: /var/lib/docker/aufs
 Backing Filesystem: extfs
 Dirs: 51
 Dirperm1 Supported: true
Logging Driver: json-file
Cgroup Driver: cgroupfs
Plugins:
 Volume: local
 Network: host null bridge overlay
Swarm: active
 NodeID: 7g9n07v9wcqh215l8fr9jku9q
 Is Manager: true
 ClusterID: 2hmvi41my9ljcj9w88sega02f
 Managers: 1
 Nodes: 1
 Orchestration:
  Task History Retention Limit: 5
 Raft:
  Snapshot interval: 10000
  Heartbeat tick: 1
  Election tick: 3
 Dispatcher:
  Heartbeat period: 5 seconds
 CA configuration:
  Expiry duration: 3 months
 Node Address: 104.131.182.84
Runtimes: runc
Default Runtime: runc
Security Options: apparmor seccomp
Kernel Version: 4.4.0-31-generic
Operating System: Ubuntu 16.04.1 LTS
OSType: linux
Architecture: x86_64
CPUs: 1
Total Memory: 488.5 MiB
Name: testserver
ID: SHMX:TWPS:TBUX:AE2Z:DCRW:U27U:DUKX:4AS2:BV3L:ZHF3:AN7W:H5AL
Docker Root Dir: /var/lib/docker
Debug Mode (client): false
Debug Mode (server): false
Registry: https://index.docker.io/v1/
WARNING: No swap limit support
Labels:
 provider=digitalocean
Insecure Registries:
 127.0.0.0/8

Additional environment details (AWS, VirtualBox, physical, etc.):

Digital ocean. Single droplet as the manager in a single-node swarm.

Steps to reproduce the issue:

  1. Create a droplet on digital ocean with docker-machine

    $ docker-machine create -d digitalocean --digitalocean-image "ubuntu-16-04-x64" testserver
    Running pre-create checks...
    
    [etc etc etc]
    
    Docker is up and running!
    To see how to connect your Docker Client to the Docker Engine running on this virtual machine, run: docker-machine env testserver
    
  2. Create a swarm

    root@testserver:~# docker swarm init --advertise-addr $(dig +short myip.opendns.com @resolver1.opendns.com)
    Swarm initialized: current node (7g9n07v9wcqh215l8fr9jku9q) is now a manager.
    
  3. Create a service with 10 replicas, but no publish port

    root@testserver:~# docker service create --replicas 10 --name nginxtest nginxdemos/hello
    0mhsp06y8be6e7ctuzu9r9avq
    root@testserver:~# docker service ps nginxtest
    ID                         NAME          IMAGE             NODE        DESIRED STATE  CURRENT STATE             ERROR
    aijolpdm6vvn8jl4mcetvt31e  nginxtest.1   nginxdemos/hello  testserver  Running        Preparing 11 seconds ago
    6quaslvqls98h8jbksiblxed1  nginxtest.2   nginxdemos/hello  testserver  Running        Preparing 11 seconds ago
    aoukmdeypxzl1sqr29u5yctjt  nginxtest.3   nginxdemos/hello  testserver  Running        Preparing 11 seconds ago
    3cbxvjrdn7m8tkahpvscmfvul  nginxtest.4   nginxdemos/hello  testserver  Running        Preparing 11 seconds ago
    4o0zaff98aozqv3t1af9s5tk0  nginxtest.5   nginxdemos/hello  testserver  Running        Preparing 11 seconds ago
    6uc1v37hkua2771ncm8lnvbv3  nginxtest.6   nginxdemos/hello  testserver  Running        Preparing 11 seconds ago
    0u7cr49qy4ctdf7a4omlv6rbp  nginxtest.7   nginxdemos/hello  testserver  Running        Preparing 11 seconds ago
    8urs0t0nncetjxtmuxcb8mzuq  nginxtest.8   nginxdemos/hello  testserver  Running        Preparing 11 seconds ago
    a1y4uffudc7b6pwe89gb3506t  nginxtest.9   nginxdemos/hello  testserver  Running        Preparing 11 seconds ago
    3a9s1uusb2xa20fd5t4u91x47  nginxtest.10  nginxdemos/hello  testserver  Running        Preparing 11 seconds ago
    
  4. Add a publish port of "80:80", and run inspect to verify the port has been added:

    root@testserver:~# docker service update --publish-add "80:80" nginxtest
    nginxtest
    
    root@testserver:~# docker service inspect nginxtest --pretty
    ID:     0mhsp06y8be6e7ctuzu9r9avq
    Name:       nginxtest
    Mode:       Replicated
     Replicas:  10
    Update status:
     State:     completed
     Started:   22 seconds ago
     Completed: 5 seconds ago
     Message:   update completed
    Placement:
    UpdateConfig:
     Parallelism:   1
     On failure:    pause
    ContainerSpec:
     Image:     nginxdemos/hello
    Resources:
    Ports:
     Protocol = tcp
     TargetPort = 80
     PublishedPort = 80
    
  5. Remove the publish port:

    root@testserver:~# docker service update --publish-rm "80:80" nginxtest
    nginxtest
    

Describe the results you received:

If I run inspect again, port 80 still appears to be assigned to the service:

root@testserver:~# docker service inspect nginxtest --pretty
ID:     0mhsp06y8be6e7ctuzu9r9avq
Name:       nginxtest
Mode:       Replicated
 Replicas:  10
Placement:
UpdateConfig:
 Parallelism:   1
 On failure:    pause
ContainerSpec:
 Image:     nginxdemos/hello
Resources:
Ports:
 Protocol = tcp
 TargetPort = 80
 PublishedPort = 80

If I subsequently try to re-add the port, I get this error:

root@testserver:~# docker service update --publish-add "80:80" nginxtest
Error response from daemon: rpc error: code = 3 desc = EndpointSpec: duplicate ports provided

Describe the results you expected:

I expected the ports to be unpublished from the service.

@icecrime
Copy link
Contributor

Cc @dnephin @vieux!

@dnephin
Copy link
Member

dnephin commented Jul 29, 2016

From the usage text:

"Remove a published port by its target port"

The important bit is target port. So --publish-rm 80 should work. This is likely going to be a common mistake, so we can probably do something to make this easier to discover. Either error if there is a colon in the value, or allow the port spec, parse it, compare both target and published port, and raise an error if the published port doesn't match. Maybe also add a warning letting users know they don't need to include published port (if we do that).

@mike-hearn
Copy link
Author

That doesn't seem to be working either:

root@testserver:~# docker service update --publish-rm 80 nginxtest
nginxtest
root@testserver:~# docker service inspect nginxtest --pretty
ID:             7ub4g7cs24eoxh9viaqfcf3w0
Name:           nginxtest
Mode:           Replicated
 Replicas:      10
Update status:
 State:         updating
 Started:       1 seconds ago
 Message:       update in progress
Placement:
UpdateConfig:
 Parallelism:   1
 On failure:    pause
ContainerSpec:
 Image:         nginxdemos/hello
Resources:
Ports:
 Protocol = tcp
 TargetPort = 80
 PublishedPort = 80

@jhorwit2
Copy link
Contributor

I noticed something interesting. If your service is in an overlay network this functionality works fine, but if you didn't specify the network and only have ingress it won't.

@mike-hearn
Copy link
Author

Wow, yep. Great catch @jhorwit2. I just verified:

root@testserver:~# docker service rm nginxtest
nginxtest
root@testserver:~# docker network create -d overlay testnetwork
0youublge6plq5hoc9m15608s
root@testserver:~# docker service create --replicas 10 --name nginxtest --network testnetwork nginxdemos/hello
617t8jgip3kfiphi2vgposr3l
root@testserver:~# docker service update --publish-add "80:80" nginxtest
nginxtest
root@testserver:~# docker service inspect nginxtest --pretty
ID:             617t8jgip3kfiphi2vgposr3l
Name:           nginxtest
Mode:           Replicated
 Replicas:      10
Update status:
 State:         updating
 Started:       2 seconds ago
 Message:       update in progress
Placement:
UpdateConfig:
 Parallelism:   1
 On failure:    pause
ContainerSpec:
 Image:         nginxdemos/hello
Resources:
Networks: 0youublge6plq5hoc9m15608sPorts:
 Protocol = tcp
 TargetPort = 80
 PublishedPort = 80
root@testserver:~# docker service update --publish-rm 80 nginxtest
nginxtest
root@testserver:~# docker service inspect nginxtest --pretty
ID:             617t8jgip3kfiphi2vgposr3l
Name:           nginxtest
Mode:           Replicated
 Replicas:      10
Update status:
 State:         completed
 Started:       3 minutes ago
 Completed:     3 minutes ago
 Message:       update completed
Placement:
UpdateConfig:
 Parallelism:   1
 On failure:    pause
ContainerSpec:
 Image:         nginxdemos/hello
Resources:

@thaJeztah thaJeztah added the kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed. label Jul 29, 2016
@dperny
Copy link
Contributor

dperny commented Aug 1, 2016

I can reproduce this. There don't seem to be any errors generated. I'm am investigating the problem.

dperny added a commit to dperny/swarmkit-1 that referenced this issue Aug 2, 2016
Fixes moby/moby#25200. This change should trigger an allocation when
the service has ports attached but the spec says none should be.

Signed-off-by: Drew Erny <drew.erny@docker.com>
aaronlehmann pushed a commit to moby/swarmkit that referenced this issue Aug 4, 2016
Fixes moby/moby#25200. This change should trigger an allocation when
the service has ports attached but the spec says none should be.

Signed-off-by: Drew Erny <drew.erny@docker.com>
(cherry picked from commit 5982397)
@thaJeztah
Copy link
Member

ping @mrjana @dperny I'm reopening this, because the change in SwarmKit was not vendored yet; could you make sure to add a "closes" in the vendor bump PR that brings in moby/swarmkit#1301 ?

@thaJeztah thaJeztah reopened this Aug 4, 2016
@thaJeztah thaJeztah added the priority/P1 Important: P1 issues are a top priority and a must-have for the next release. label Aug 5, 2016
@thaJeztah
Copy link
Member

er, @vdemeester looks like this auto-closed from your repository 😂

@thaJeztah thaJeztah reopened this Aug 10, 2016
@vdemeester
Copy link
Member

wat ? 😅 😂 😱

@aaronlehmann
Copy link
Contributor

moby/swarmkit#1301 is there now.

@thaJeztah
Copy link
Member

Third time lucky!

yongtang added a commit to yongtang/docker that referenced this issue Dec 2, 2016
…TargetPort>`

Currently `--publish-rm` only accepts `<TargetPort>` or `<TargetPort>[/Protocol]`
though there are some confusions.

Since `--publish-add` accepts `<PublishedPort>:<TargetPort>[/Protocol]`, some user
may provide `--publish-rm 80:80`. However, there is no error checking so the incorrect
provided argument is ignored silently.

This fix adds the check to make sure `--publish-rm` only accepts `<TargetPort>[/Protocol]`
and returns error if the format is invalid.

The `--publish-rm` itself may needs to be revisited to have a better UI/UX experience,
see discussions on:
moby/swarmkit#1396
moby#25200 (comment)
moby#25338 (comment)

This fix is short term measure so that end users are not misled by the silently ignored error
of `--publish-rm`.

This fix is related to (but is not a complete fix):
moby/swarmkit#1396

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
xianlubird pushed a commit to xianlubird/docker that referenced this issue Dec 23, 2016
…TargetPort>`

Currently `--publish-rm` only accepts `<TargetPort>` or `<TargetPort>[/Protocol]`
though there are some confusions.

Since `--publish-add` accepts `<PublishedPort>:<TargetPort>[/Protocol]`, some user
may provide `--publish-rm 80:80`. However, there is no error checking so the incorrect
provided argument is ignored silently.

This fix adds the check to make sure `--publish-rm` only accepts `<TargetPort>[/Protocol]`
and returns error if the format is invalid.

The `--publish-rm` itself may needs to be revisited to have a better UI/UX experience,
see discussions on:
moby/swarmkit#1396
moby#25200 (comment)
moby#25338 (comment)

This fix is short term measure so that end users are not misled by the silently ignored error
of `--publish-rm`.

This fix is related to (but is not a complete fix):
moby/swarmkit#1396

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
vdemeester pushed a commit to vdemeester/docker-cli that referenced this issue May 15, 2017
…TargetPort>`

Currently `--publish-rm` only accepts `<TargetPort>` or `<TargetPort>[/Protocol]`
though there are some confusions.

Since `--publish-add` accepts `<PublishedPort>:<TargetPort>[/Protocol]`, some user
may provide `--publish-rm 80:80`. However, there is no error checking so the incorrect
provided argument is ignored silently.

This fix adds the check to make sure `--publish-rm` only accepts `<TargetPort>[/Protocol]`
and returns error if the format is invalid.

The `--publish-rm` itself may needs to be revisited to have a better UI/UX experience,
see discussions on:
moby/swarmkit#1396
moby/moby#25200 (comment)
moby/moby#25338 (comment)

This fix is short term measure so that end users are not misled by the silently ignored error
of `--publish-rm`.

This fix is related to (but is not a complete fix):
moby/swarmkit#1396

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
andrewhsu pushed a commit to docker/docker-ce that referenced this issue May 19, 2017
…TargetPort>`

Currently `--publish-rm` only accepts `<TargetPort>` or `<TargetPort>[/Protocol]`
though there are some confusions.

Since `--publish-add` accepts `<PublishedPort>:<TargetPort>[/Protocol]`, some user
may provide `--publish-rm 80:80`. However, there is no error checking so the incorrect
provided argument is ignored silently.

This fix adds the check to make sure `--publish-rm` only accepts `<TargetPort>[/Protocol]`
and returns error if the format is invalid.

The `--publish-rm` itself may needs to be revisited to have a better UI/UX experience,
see discussions on:
moby/swarmkit#1396
moby/moby#25200 (comment)
moby/moby#25338 (comment)

This fix is short term measure so that end users are not misled by the silently ignored error
of `--publish-rm`.

This fix is related to (but is not a complete fix):
moby/swarmkit#1396

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Upstream-commit: 8597e231a5caa23247934f7a89c1001d87e26192
Component: cli
andrewhsu pushed a commit to docker/docker-ce that referenced this issue May 19, 2017
…TargetPort>`

Currently `--publish-rm` only accepts `<TargetPort>` or `<TargetPort>[/Protocol]`
though there are some confusions.

Since `--publish-add` accepts `<PublishedPort>:<TargetPort>[/Protocol]`, some user
may provide `--publish-rm 80:80`. However, there is no error checking so the incorrect
provided argument is ignored silently.

This fix adds the check to make sure `--publish-rm` only accepts `<TargetPort>[/Protocol]`
and returns error if the format is invalid.

The `--publish-rm` itself may needs to be revisited to have a better UI/UX experience,
see discussions on:
moby/swarmkit#1396
moby/moby#25200 (comment)
moby/moby#25338 (comment)

This fix is short term measure so that end users are not misled by the silently ignored error
of `--publish-rm`.

This fix is related to (but is not a complete fix):
moby/swarmkit#1396

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Upstream-commit: 1b400f6284b17fffa0495e1c801155fdd0716e98
Component: cli
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/networking area/swarm area/ux kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed. priority/P1 Important: P1 issues are a top priority and a must-have for the next release. status/claimed version/1.12
Projects
None yet
Development

No branches or pull requests

9 participants