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

Add the ability to request a specific IP address when creating a new endpoint #201

Closed
wants to merge 1 commit into from

Conversation

@boucher
Copy link

boucher commented May 23, 2015

When restoring a container, we want to request the same IP address we had before. I'm not sure if this is really the best way to do that, so I'm open to changing this if there are any alternate proposals.

@boucher boucher force-pushed the boucher:request-ip branch from 8f8540d to c81a620 May 23, 2015
@mavenugo

This comment has been minimized.

Copy link
Contributor

mavenugo commented May 23, 2015

@boucher Thanks for the patch. Please refer to #161 and your approach handles one part of the fix. #199 solves one other part. I will be fixing the docker side of the fix today.

Please fix the build error. And please also always write a test-case for any code change.

@boucher

This comment has been minimized.

Copy link
Author

boucher commented May 23, 2015

Thanks for the quick feedback. Does FromMap in #199 make this unnecessary? If so I'm happy to close it, otherwise I'll look into adding a test case.

@boucher boucher force-pushed the boucher:request-ip branch from c81a620 to 102269b May 24, 2015
@boucher

This comment has been minimized.

Copy link
Author

boucher commented May 24, 2015

Fixed the build issue and added a test case.

@mavenugo

This comment has been minimized.

Copy link
Contributor

mavenugo commented May 24, 2015

@boucher this is still useful and we can make use of it as one of the blocks of the bigger puzzle. thanks for the contribution. will review this shortly.

@boucher boucher force-pushed the boucher:request-ip branch 3 times, most recently from 307a9f1 to ce3ba9b May 24, 2015
…endpoint.

Docker-DCO-1.1-Signed-off-by: Ross Boucher <rboucher@gmail.com> (github: boucher)
@boucher boucher force-pushed the boucher:request-ip branch from ce3ba9b to 6dec499 May 24, 2015
@boucher

This comment has been minimized.

Copy link
Author

boucher commented May 26, 2015

@mavenugo Have you had a chance to take a look?

@mavenugo

This comment has been minimized.

Copy link
Contributor

mavenugo commented May 26, 2015

@boucher today I will. stay tuned.

@mavenugo

This comment has been minimized.

Copy link

mavenugo commented on drivers/bridge/bridge.go in 6dec499 May 26, 2015

Being an user provided label with no translation in between, this is going to be a string.

@mavenugo

This comment has been minimized.

Copy link

mavenugo commented on drivers/bridge/bridge.go in 6dec499 May 26, 2015

and hence, ec.IPAddress = net.ParseIP(ip)

@mavenugo

This comment has been minimized.

Copy link

mavenugo commented on drivers/bridge/bridge_test.go in 6dec499 May 26, 2015

Please change the tests to reflect the change from IPAddress to string

@boucher

This comment has been minimized.

Copy link
Author

boucher commented May 26, 2015

I'm not sure I follow, but if I understand what you're saying, you're suggesting that the people setting the option would set it as a string? I'm not sure why that's the case, since the other options don't seem to be set as strings (e.g. MacAddress), and I'm not setting it as a string in my upstream use in Docker.

@mavenugo

This comment has been minimized.

Copy link
Contributor

mavenugo commented May 27, 2015

@boucher that's because, MacAddress is a well-known option in docker run & hence it is formatted programmatically before sending it to libnetwork. Whereas, we are not adding --ip flag in docker run for this use-case, but trying to use the user label mechanism to pass the info transparently. PTAL : moby/moby#13476
If we have to introduce a --ip flag in docker run, then we could format it as well. But we are trying to avoid adding special driver specific flags in docker to control the flag sprawl, by the use of labels.

Having said that, there is some concern to the general approach of using the labels as seen in the above PR.

@boucher

This comment has been minimized.

Copy link
Author

boucher commented May 27, 2015

Fair enough. It still seems like you'd want docker to be responsible for the label -> ip translation/parsing, so that it could better define the behavior of what should happen given an invalid IP address.

@boucher

This comment has been minimized.

Copy link
Author

boucher commented May 27, 2015

Just to clarify my earlier comment about how I'm using this in Docker: when you restore a container, criu will try to set its ip address to whatever it was when it was checkpointed, so we want to request that specific IP when we restore. The information is stored/retrieved straight from the container's state, so it's already parsed. Exposing this feature to users is definitely cool, but not actually why I need this.

@boucher

This comment has been minimized.

Copy link
Author

boucher commented May 27, 2015

All that being said, the change is obviously pretty simple so I'm happy to update the PR.

@mavenugo

This comment has been minimized.

Copy link
Contributor

mavenugo commented May 27, 2015

@boucher hate to say this, but can you please hold on with the changes ?
We have to make sure that we address the comments from moby/moby#13476 before we could make this change.

@boucher

This comment has been minimized.

Copy link
Author

boucher commented Jun 16, 2015

@mavenugo Any updates on the various other pending issues here?

@mavenugo

This comment has been minimized.

Copy link
Contributor

mavenugo commented Jul 22, 2015

@boucher PTAL #161 and moby/moby#13476. Based on all the discussions and gathered opinions, using the label approach to assigning IP is not the preferred method. Docker maintainers prefers a more abstract way to separate user intent from operational intent.
Based on this feedback and various other discussions on a flexible ip address management, we feel that having a pluggable IPAM will help a great deal. Please share your opinion.
If you agree, we can close this PR and work together on the pluggable IPAM solution.

@aboch

This comment has been minimized.

Copy link
Contributor

aboch commented Jan 6, 2016

Things have changed since this PR was opened: With the introduction of the IPAM entity in libnetwork, container IP addresses are managed by the IPAM driver or explicitly chosen by user and passed via IPAM specific EndpoinOption to libnetwork.

Therefore I am closing this PR as it passes the user specified address in a way which no longer fits in libnetwork model.

@aboch aboch closed this Jan 6, 2016
@xusxlinux

This comment has been minimized.

Copy link

xusxlinux commented May 3, 2018

1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.