-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
add port range support to docker compose #1191
Conversation
Thanks for contributing!
|
return ["%s%s" % (port, protocol)] | ||
|
||
if len(parts) == 2: | ||
return ["%s%s" % (p, protocol) for p in range(int(parts[0]), int(parts[1]) + 1)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can anyone confirm this needs to be handled by the client?
It seems like it should be handled by dockerd. I can't find any reference to it in the remote api docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I looked at the docker API, the data structure that I saw there only contained one port, that's why I chose to implement it on the client.
see ExposedPorts at https://docs.docker.com/reference/api/docker_remote_api_v1.17/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also looking at the docker client source code, you can also see that the ports are used individually.
see runconfig/parse.go line 229-238 (the line with ParsePortRange)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for integration tests - I can add those later tonight, but let's first decide if the general idea is acceptable. i.e. it should be done here, vs the docker server and docker-py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like Docker does indeed do it client-side, so that's where we'll need to do it too. Moving it to the server is a discussion to be had on docker/docker.
Paging @shin-: does processing of port ranges belong in docker-py, in your opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, possibly. But probably as a function in the tools module rather than inside the API methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shin- Thanks, I agree - I think we can move build_port_bindings
and its dependencies (to_port_range
, split_port
, add_port_mapping
) all over to docker.utils
, perhaps as a submodule (docker.utils.ports
).
@uvgroovy Could you submit a PR to docker-py?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aanand So basically move build_port_bindings and deps to docker.utils.ports and then import it in compose/service.py?
I can do that, just one question -
doing that will temporary break the code, unless requirments-dev is updated.
so basically, i think we need to:
- add build_port_bindings to docker-py
- release new docker-py
- remove build_port_bindings from compose/service.py, import it from docker-py and update requirements-dev.txt to the new docker-py version
sounds good? any comments\feedback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should go:
- Submit PR to docker-py
- Update this PR with the code changes and set the docker-py requirement in
requirements.txt
to point at your repo and branch with a git URL (see the pyinstaller requirement inrequirements-dev.txt
for an example) - When the docker-py PR is merged, update the git URL to point at the docker-py merge commit
- When the next docker-py is released, update
requirements.txt
to the stable version - Merge this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good.
Hey @uvgroovy, just wanted to check in with you to see if this was still moving forward. Do you need any help with testing, etc.? Let me know. I'd be glad to help any way I can. |
Hi, Sorry for the delay, jewish holidays are slowing me down... |
I updated the version of requests to version 2.5.2 to match the one required from docker-py; but now, I encounter this error: |
I tried to reproduce this error on my local development environment with your branch, but was unable to. Can you provide any more details as to what your setup looks like and how you are invoking your tests? I am running: OSX Yosemite 10.10.2 |
So basically, When I tried to build compose with the docker-py repo directly. this is my requirements.txt:
it complained that requests 2.2.1 is too old:
so I changed the requirements.txt for compose in my branch to contain requests 2.5.2 and got the error i mentioned.
|
hey guys whats happening with this? |
Same issue as #1075 and #1269 - we're waiting for an upstream fix in requests (https://github.com/kennethreitz/requests/pull/2533) to make it into a release. |
@aanand , perhaps can you assist with merging the docker-py PR? |
requests 2.6 was released with a fix to this issue. updating both docker-py and compose to 2.6.2 seems to be working (tried it in my own laptop and the tests pass) - but here jenkins is specifically looking for requests>=2.2.1,<2.6 I couldn't find a reference to requests "<2.6" in the repo it self. Any idea why that happens\how to fix this? |
Sorry I missed this. Needs rebasing on master, then it should pick up the new requests version range. |
i re-did my commit on the master from upstream compose. all should be well now |
How will this behave if we scale containers that are greater than the number of ports available? |
@aanm not sure what you are asking. this PR just allows you to specify multiple ports in the configuration. it is as if you wrote them individually (i.e. the code 'translates' the port range to multiple single ports for the rest of the program). |
@uvgroovy Currently, compose sends a user error if you try to scale a container that has ports defined. For example if you have |
@aanm The same will happen, yes. It would be nice to be able to specify a set of ports where each one is assigned to a single container in the service, but it's not what the Docker CLI's port range syntax means, so we'd need to come up with an alternate way of specifying it. |
@aanand I think it would also be nice to NOT throw an error on the case I was talking about, either with a range or a single port specification. I think it would be better to just throw a warning. I'm saying this because compose + swarm fail if we try to scale up with a single port defined. |
Replacing the error with a warning seems appropriate now that swarm makes that case possible. |
@aanand re-did the commit to the updated compose master. |
Thanks! LGTM |
Strike that - still needs docs. |
Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
@aanand sorry for the delay (i missed your last comment) - I added documentation to yml.md - please let me know if that works. Also, while it seems that tests failed, it actually looks like an IT issue: |
@@ -102,7 +102,7 @@ An entry with the ip address and hostname will be created in `/etc/hosts` inside | |||
### ports | |||
|
|||
Expose ports. Either specify both ports (`HOST:CONTAINER`), or just the container | |||
port (a random host port will be chosen). | |||
port (a random host port will be chosen). You can specify a port range instead of a single port (`START-END`). If you use a range for the container ports, you may specify a range for the host ports as well. both ranges must be of equal size. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @moxiegirl
My take: this seems to suggest on first reading that you can specify a range of container ports but a single host port, which I don't think is correct. Perhaps:
Expose ports. You can specify just a container port (which will be mapped to a random host port), or both (HOST:CONTAINER
).
You can optionally pass in ranges of port numbers. If you're specifying host ports as well as container ports, the host and container port ranges must be the same size.
the lastest release 1.4.0rc2 still not support port range. |
Merged in #1827 |
fixes #1102
tests passed