Allow scaling services that have port binding of the form `host_ip::container_port` #1659

viranch · 2015-07-06T14:55:56Z

Compose gives up scaling if a service has a port binding of the form: 192.168.1.2::80 (bind to a specific host IP but a random host port, for the container port 80) with the error:

Service "foo" cannot be scaled because it specifies a port on the host. If multiple containers for this service were created, the port would clash.

Remove the ":" from the port definition in docker-compose.yml so Docker can choose a random port for each container.

Compose wrongly assumes that a host port is specified because there's a : in the port binding.

This PR fixes this.

aanand · 2015-07-06T15:18:45Z

Thanks.

Rather than inspecting the string to see if there's an external port specified, it'd be better call split_port() and inspect its output.
It'd be good to have some unit tests for can_be_scaled.

aanand · 2015-07-07T13:37:20Z

compose/service.py

@@ -679,7 +679,8 @@ def labels(self, one_off=False):

    def can_be_scaled(self):
        for port in self.options.get('ports', []):
-            if ':' in str(port):
+            external = split_port(port)[1]
+            if external and not (type(external) == tuple and external[1] == None):


I think that if you're checking that type(external) == tuple, then checking it for truthiness is unnecessary. And I think isinstance is better than comparing types. So this can just be:

if isinstance(external, tuple) and external[1] == None:

Oh, and PEP 8 says to use is None, not == None.

Comparisons to singletons like None should always be done with is or is not , never the equality operators

then checking it for truthiness is unnecessary

You mean external[1] == None is not required? I think it is, external[1] corresponds to host port, if its specified can_be_scaled should fail, else succeed.

viranch · 2015-07-08T19:52:27Z

compose/service.py

+        # TODO: this function should ideally move to docker.utils.ports
+        # get rid of it once its accepted into docker-py
+        def has_external_port(external_binding):
+            if external_binding is None:


We don't need to check this at all IMO because docker-py's split_port never returns a list with None elements as the 2nd return value (it's either a just None, or a list of strings or tuples).

If it's going to be a public function in docker-py, it needs to be able to handle all valid port bindings, not just the subset created by split_port().

If we do what I have proposed in https://github.com/docker/compose/pull/1659/files#r34191487 then we don't need this has_external_port() function at all, because my proposed function simplifies readability. The whole reason for having this function in the first place was it was a little complex from readability standpoint.

viranch · 2015-07-08T20:02:33Z

I think we might also need (unit?) tests covering port ranges for can_be_scaled(), given docker-py's ports util supports it.

viranch · 2015-08-01T10:27:44Z

@aanand Ping :) Let me know about my comments on the diff, I'll rebase from master given the refactor of can_be_scaled() in 9ffe69a

viranch · 2015-08-08T08:29:39Z

Rebased with latest master, seems like the CI build failed due to docker errors.

+ docker run --rm -t --privileged --volume=/var/jenkins_home/workspace/Compose-PRs/.git:/code/.git --name compose-prs1012 --volume=/var/run/docker.sock:/var/run/docker.sock -e DOCKER_VERSIONS=all -e TAG=compose:454b52f --entrypoint=script/ci compose:454b52f --verbose
Running lint checks
Running tests against Docker 1.7.1
Waiting for Docker to start...
Docker failed to start
time="2015-08-08T08:26:11.302291821Z" level=warning msg="Udev sync is not supported. This will lead to unexpected behavior, data loss and errors. For more information, see https://docs.docker.com/reference/commandline/cli/#daemon-storage-driver-option" 
time="2015-08-08T08:26:11.303233892Z" level=error msg="There are no more loopback devices available." 
time="2015-08-08T08:26:11.303417028Z" level=fatal msg="Error starting daemon: error initializing graphdriver: loopback mounting failed" 
Build step 'Execute shell' marked build as failure

dnephin · 2015-08-27T23:48:19Z

Resolves #1102

dnephin · 2015-10-16T20:21:49Z

tests/unit/service_test.py

+            'foo',
+            image='foo',
+            ports=["127.0.0.1:1000:2000"])
+        self.assertEqual(service.specifies_host_port(), True)


Minor nit, now that we're using py.test we actually get better error messages if you use assert service.specifies_host_port() or assert not ....

Would you mind changing the assertions to use bare assert ?

Rest of the service_test.py still uses self.assertEqual, I don't want to break the convention. What you're suggesting could go in as a separate PR that updates all tests IMO.

viranch · 2015-10-17T18:50:01Z

Oops, bad push closed this, reopening.

…host port Signed-off-by: Viranch Mehta <viranch.mehta@gmail.com>

viranch · 2015-10-17T19:26:39Z

Rebased with latest master.

…st IP but no host port

dnephin · 2015-10-19T19:33:09Z

LGTM

mnowster · 2015-10-23T16:11:26Z

LGTM. Thanks @viranch 👍

Allow scaling services that have port binding of the form `host_ip::container_port`

GordonTheTurtle added status/0-needs-triage dco/no labels Jul 6, 2015

viranch force-pushed the hostip-random-port branch from ae1e9d5 to db52ee2 Compare July 6, 2015 14:57

GordonTheTurtle added dco/no and removed dco/no labels Jul 6, 2015

viranch force-pushed the hostip-random-port branch from dccf151 to 4fd875f Compare July 6, 2015 15:01

GordonTheTurtle removed the dco/no label Jul 6, 2015

GordonTheTurtle added the dco/no label Jul 7, 2015

viranch force-pushed the hostip-random-port branch from 946c623 to 65e1cab Compare July 7, 2015 13:28

GordonTheTurtle removed the dco/no label Jul 7, 2015

aanand reviewed Jul 7, 2015
View reviewed changes

viranch force-pushed the hostip-random-port branch 4 times, most recently from f02b5ab to b60ee3a Compare July 8, 2015 19:26

viranch reviewed Jul 8, 2015
View reviewed changes

viranch force-pushed the hostip-random-port branch from b60ee3a to 454b52f Compare August 8, 2015 08:24

viranch force-pushed the hostip-random-port branch from 454b52f to 520d29a Compare August 9, 2015 19:57

dnephin added the kind/enhancement label Aug 26, 2015

dnephin reviewed Oct 16, 2015
View reviewed changes

viranch closed this Oct 17, 2015

viranch force-pushed the hostip-random-port branch from 520d29a to ae3c66b Compare October 17, 2015 18:47

viranch reopened this Oct 17, 2015

viranch force-pushed the hostip-random-port branch from c00e710 to 9158a1f Compare October 17, 2015 18:53

Fix specifies_host_port() to handle port binding with host IP but no …

258c8bc

…host port Signed-off-by: Viranch Mehta <viranch.mehta@gmail.com>

viranch force-pushed the hostip-random-port branch from 9158a1f to 258c8bc Compare October 17, 2015 19:10

viranch changed the title ~~Allow service to specify host IP with random host port in port bindings~~ Allow scaling services that have port binding of the form host_ip::container_port Oct 17, 2015

viranch changed the title ~~Allow scaling services that have port binding of the form host_ip::container_port~~ Allow scaling services that have port binding of the form host_ip::container_port Oct 17, 2015

viranch added a commit to viranch/compose that referenced this pull request Oct 18, 2015

Fixes docker#1659: can_be_scaled() should handle port binding with ho…

8d29176

…st IP but no host port

dnephin self-assigned this Oct 19, 2015

mnowster added a commit that referenced this pull request Oct 23, 2015

Merge pull request #1659 from viranch/hostip-random-port

5d60fbe

Allow scaling services that have port binding of the form `host_ip::container_port`

mnowster merged commit 5d60fbe into docker:master Oct 23, 2015

dnephin added this to the next release milestone Nov 12, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow scaling services that have port binding of the form `host_ip::container_port` #1659

Allow scaling services that have port binding of the form `host_ip::container_port` #1659

viranch commented Jul 6, 2015

aanand commented Jul 6, 2015

aanand Jul 7, 2015

aanand Jul 7, 2015

viranch Jul 7, 2015

viranch Jul 8, 2015

aanand Aug 3, 2015

viranch Aug 5, 2015

viranch commented Jul 8, 2015

viranch commented Aug 1, 2015

viranch commented Aug 8, 2015

dnephin commented Aug 27, 2015

dnephin Oct 16, 2015

viranch Oct 17, 2015

viranch commented Oct 17, 2015

viranch commented Oct 17, 2015

dnephin commented Oct 19, 2015

mnowster commented Oct 23, 2015

Allow scaling services that have port binding of the form host_ip::container_port #1659

Allow scaling services that have port binding of the form host_ip::container_port #1659

Conversation

viranch commented Jul 6, 2015

aanand commented Jul 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viranch commented Jul 8, 2015

viranch commented Aug 1, 2015

viranch commented Aug 8, 2015

dnephin commented Aug 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viranch commented Oct 17, 2015

viranch commented Oct 17, 2015

dnephin commented Oct 19, 2015

mnowster commented Oct 23, 2015

Allow scaling services that have port binding of the form `host_ip::container_port` #1659

Allow scaling services that have port binding of the form `host_ip::container_port` #1659