Skip to content

Move exposed ports and port bindings from Endpoint to Sandbox#810

Merged
sanimej merged 1 commit intomoby:masterfrom
aboch:se
Mar 7, 2016
Merged

Move exposed ports and port bindings from Endpoint to Sandbox#810
sanimej merged 1 commit intomoby:masterfrom
aboch:se

Conversation

@aboch
Copy link
Copy Markdown
Contributor

@aboch aboch commented Dec 9, 2015

Signed-off-by: Alessandro Boch aboch@docker.com

@mavenugo
Copy link
Copy Markdown
Contributor

@aboch I believe this PR is not ready for review yet. Please move the label appropriately when it is ready for review.

@aboch
Copy link
Copy Markdown
Contributor Author

aboch commented Dec 23, 2015

@mavenugo @mrjana
The PR is now ready for review.

Please consider this:

  • Code does not fail if a remote driver does not respond to the two new driverapi methods calls
  • Docker daemon code will send the expose ports and port mapping configs during NewSandbox call and also during the CreateEndpoint call, for backward compatibility with current remote driver
  • If a current remote driver does not set the default gw in response to a Join and relies on libnetwork default gw network logic, things will still work because the def gw endpoint will get the port configs directly from the sandbox options

@mavenugo
Copy link
Copy Markdown
Contributor

related to moby/moby#18222

@thaJeztah
Copy link
Copy Markdown
Member

ping @mavenugo @aboch looks like this needs a rebase; what's the status on this one, because we have the related issue on the 1.10 milestone in docker 😇

@mavenugo
Copy link
Copy Markdown
Contributor

@thaJeztah this is an involved change and we are not comfortable to bring it in for 1.10 release. Can you point me to the issue in docker that is related to this PR ?

@thaJeztah
Copy link
Copy Markdown
Member

@mavenugo Right above, moby/moby#18222

@tiborvass
Copy link
Copy Markdown
Contributor

Ping @aboch for rebase

@mavenugo
Copy link
Copy Markdown
Contributor

@tiborvass as suggested above : #810 (comment), we have to see if we can find another simpler solution for moby/moby#18222.

@aboch from the look of it, moby/moby#18222 seems to be a random issue and doesn't call for this redesign. Could you please take a look at it again ?

mavenugo added a commit to mavenugo/docker that referenced this pull request Jan 26, 2016
moby/libnetwork#810 provides the more complete
solution for moving the Port-mapping ownership away from endpoint and
into Sandbox. But, this PR makes the best use of existing libnetwork
design and get a step closer to the gaol.

Signed-off-by: Madhu Venugopal <madhu@docker.com>
tiborvass pushed a commit to tiborvass/docker that referenced this pull request Jan 26, 2016
moby/libnetwork#810 provides the more complete
solution for moving the Port-mapping ownership away from endpoint and
into Sandbox. But, this PR makes the best use of existing libnetwork
design and get a step closer to the gaol.

Signed-off-by: Madhu Venugopal <madhu@docker.com>
aditirajagopal pushed a commit to aditirajagopal/docker that referenced this pull request Feb 8, 2016
moby/libnetwork#810 provides the more complete
solution for moving the Port-mapping ownership away from endpoint and
into Sandbox. But, this PR makes the best use of existing libnetwork
design and get a step closer to the gaol.

Signed-off-by: Madhu Venugopal <madhu@docker.com>
return nil
}

func (sb *sandbox) getFirstEndpointWithGateway() *endpoint {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you pls remove this function ... it is not being used

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, will remove

@aboch
Copy link
Copy Markdown
Contributor Author

aboch commented Mar 3, 2016

@mavenugo
Taken care of your comments, PTAL.

@mavenugo
Copy link
Copy Markdown
Contributor

mavenugo commented Mar 4, 2016

Thanks for handling the comments.

LGTM.

Signed-off-by: Alessandro Boch <aboch@docker.com>
@aboch
Copy link
Copy Markdown
Contributor Author

aboch commented Mar 6, 2016

@sanimej
Took care of your comment. PTAL when you get a chance. Thanks.

if err := sb.updateGateway(gwep); err != nil {
return err
}
if ep == sb.getGatewayEndpoint() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please retain the check we had to not call updateGateway() if the current ep is not same as gwep. Not having the check will lead to unnecessary calls to updateGateway()

LGTM other than this one comment.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missed the == in the if check. LGTM

sanimej pushed a commit that referenced this pull request Mar 7, 2016
Move exposed ports and port bindings from Endpoint to Sandbox
@sanimej sanimej merged commit 080bedd into moby:master Mar 7, 2016
@aboch aboch deleted the se branch March 7, 2016 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants