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

Fix issues of multiple published ports mapping to the same target port #1835

Merged
merged 1 commit into from
Jan 10, 2017

Conversation

yongtang
Copy link
Member

This fix tries to address the issue raised in moby/moby#29730 where a service with multiple published ports mapping to the same target port (e.g., --publish 5000:80 --publish 5001:80) can't be allocated.

The reason for the issue is that, getPortConfigKey is used for both allocated ports and configured (may or may not be allocated) ports. However, getPortConfigKey will not take into consideration the PublishedPort field, which actually could be different for different allocated ports.

This fix saves a map of portKey:portNum:portState, instead of currently used portKey:portState so that multiple published ports could be processed.

A test case has been added in the unit test. The newly added test case will only work with this PR.

Signed-off-by: Yong Tang [email protected]

/cc @vdemeester @aaronlehmann @thaJeztah @stevvooe @mavenugo @icecrime @cpuguy83

@codecov-io
Copy link

codecov-io commented Dec 27, 2016

Current coverage is 54.62% (diff: 94.87%)

Merging #1835 into master will decrease coverage by 0.02%

@@             master      #1835   diff @@
==========================================
  Files           102        102          
  Lines         17025      17041    +16   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           9304       9309     +5   
- Misses         6585       6593     +8   
- Partials       1136       1139     +3   

Sunburst

Powered by Codecov. Last update f355ca1...2f92077

allocatedPorts[getPortConfigKey(portState)] = portState
portKey := getPortConfigKey(portState)
if _, ok := allocatedPorts[portKey]; !ok {
allocatedPorts[portKey] = make(map[uint32]*api.PortConfig)
Copy link
Member

Choose a reason for hiding this comment

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

Wondering; if getPortConfigKey() is intended to return the unique identifier for a port; should PublishedPort be just added to what getPortConfigKey() returns, instead of adding a map here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering; if getPortConfigKey() is intended to return the unique identifier for a port; should PublishedPort be just added to what getPortConfigKey() returns, instead of adding a map here?

This is a bit tricky. If PublishedPort was set to 0 when the service was created (to choose an ingress port dynamically), it will be set to 0 inside the spec and the actual allocated port inside s.Endpoint. These items must still be matched with each other, which is why getPortConfigKey intentionally excludes PublishedPort.

I think this is the right general approach.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right; so there's the situation -p 80 -p 80, i.e., publish port 80 on two random ports. Might be worth thinking if that's something we want to support, as it's gonna be tricky in various cases. (thinking out loud here)

Copy link

Choose a reason for hiding this comment

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

I agree with @thaJeztah, even before considering the -p X/80 -p Y/80 problem, already the -p 80 -p 80 cannot be satisfied by the current logic, if I understand it correctly, because it relays on the assumption of a unique config per target port/protocol. This seem the root cause for both the issues (again IIUIC).
Maybe we should rethink the whole logic with that in mind from the beginning.
@yongtang does your fix also address the -p 80 -p 80 use case ?

@aaronlehmann
Copy link
Collaborator

This looks correct, but I'm wondering we can do some refactoring to keep this long method from getting more complex.

Perhaps allocatedPorts could be its own type:

type allocatedPorts map[api.PortConfig]map[uint32]*api.PortConfig

It could have getter and setter methods defined on it abstract out tasks like creating the inner map and checking for a non-zero PublishedPort entry for a given PortConfig.

Just an idea and I'm not 100% sure it's the right way to go, but I imagine it will make this part of the code easier to read.

Also, it would be great to add a Docker integration test that tries to publish a port to two ingress ports, making sure it works end-to-end. I'm not sure if any other parts of the code make assumptions that would cause problems here. A test proving that it works would be valuable.

@yongtang
Copy link
Member Author

Thanks @aaronlehmann @aboch @thaJeztah for the review. The PR has been updated. Now -p 80 -p 80 case should have been addressed as well. The way to address the mixture of -p 5000:80 -p 5001:80 -p 80 -p 80 is to remove saved allocated ports when it matches the portConfig. Also, non-dynamic ports are removed first. Dynamic ports are removed after that. This will prevent the dynamic ports config mapped to non-dynamic allocated ports.

A PR in docker has also been created to add the integration test:
moby/moby#29787

Please take a look and let me know if there are any issues.

}

var portConfigs []*api.PortConfig

// Process the portConfig with portConfig.PublishMode != api.PublishModeIngress
// Iterate portConfigs with PublishModeIngress
for _, portConfig := range s.Spec.Endpoint.Ports {
// If the PublishMode is not Ingress simply pick up
// the port config.
if portConfig.PublishMode != api.PublishModeIngress {
portConfigs = append(portConfigs, portConfig)
continue
Copy link
Collaborator

Choose a reason for hiding this comment

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

This continue doesn't look right.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @aaronlehmann. I take a look and realized that the first loop and the second loop could be merged into one loop, as PublishMode != PublishModeIngress and PublishedPort != 0 does not interference with each other. So now the loops has been reduced from 3 to 2 (continue has been kept because of this).
(The processing of PublishedPort == 0 still has to be separate).

// Ignore ports which are not PublishModeIngress (already processed)
if portConfig.PublishMode != api.PublishModeIngress {
continue
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this loop should iterate over portConfigs so it doesn't need to check the PublishMode.

Copy link
Member Author

Choose a reason for hiding this comment

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

@aaronlehmann Not sure we could iterate over portConfigs, as portConfigs until now stores the PublisheMode != PublishModeIngress, while we need to process the following PublisheMode == PublishModeIngress?

// Ignore ports which are not PublishModeIngress (already processed)
if portConfig.PublishMode != api.PublishModeIngress {
continue
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, iterating over portConfigs could simplify the loop.

@yongtang yongtang force-pushed the 29730-multiple-published-port branch from c786e14 to 6b7c5df Compare January 2, 2017 13:10
if !ok {
return nil
}
// Dlete state from allocatedPorts
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Done.


if !ok {
return nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check doesn't seem necessary. If p.PublishedPort was not found, v will be nil and delete will do nothing. Feel free to keep this as-is if you prefer it that way; just pointing out a possible simplification.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @aaronlehmann Done.

// then we are not allocated
for publishedPort, v := range portStateMap {
if publishedPort != 0 {
// Dlete state from allocatedPorts
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@@ -73,7 +121,7 @@ func newPortSpace(protocol api.PortConfig_Protocol) (*portSpace, error) {
}, nil
}

// getPortConfigKey returns a map key for doing set operations with
// getPortConfigkey returns a map key for doing set operations with
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment should start with getPortConfigKey

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

for _, portState := range s.Endpoint.Ports {
if portState.PublishMode != api.PublishModeIngress {
continue
}

allocatedPorts[getPortConfigKey(portState)] = portState
portStates.addState(portState)
Copy link
Collaborator

Choose a reason for hiding this comment

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

if portState.PublishMode == api.PublishModeIngress {
        portStates.addState(portState)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

for _, portState := range s.Endpoint.Ports {
if portState.PublishMode != api.PublishModeIngress {
continue
}

allocatedPorts[getPortConfigKey(portState)] = portState
portStates.addState(portState)
Copy link
Collaborator

Choose a reason for hiding this comment

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

if portState.PublishMode == api.PublishModeIngress {
        portStates.addState(portState)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

return nil
}
// Dlete state from allocatedPorts
delete(ps[portKey], p.PublishedPort)
Copy link
Collaborator

Choose a reason for hiding this comment

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

delete(portStateMap, p.PublishedPort)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

for publishedPort, v := range portStateMap {
if publishedPort != 0 {
// Dlete state from allocatedPorts
delete(ps[portKey], publishedPort)
Copy link
Collaborator

Choose a reason for hiding this comment

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

delete(portStateMap, p.PublishedPort)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@aaronlehmann
Copy link
Collaborator

Overall LGTM, just some comments on typos and cosmetics.

@yongtang yongtang force-pushed the 29730-multiple-published-port branch from 6b7c5df to 70032f9 Compare January 4, 2017 00:04
@yongtang
Copy link
Member Author

yongtang commented Jan 4, 2017

Thanks @aaronlehmann for the review. The comments have been addressed. Please take a look.

for _, portConfig := range s.Spec.Endpoint.Ports {
// If the PublishMode is not Ingress simply pick up
// the port config.
if portConfig.PublishMode != api.PublishModeIngress {
portConfigs = append(portConfigs, portConfig)
continue
}
if portConfig.PublishedPort != 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just thought of a minor thing. If we make this an else if, we can remove the continue, and I think that makes the code slightly easier to follow. When I first read this it took me a few seconds to realize that it only ran for PublishMode == Ingress, and I think if this was an else if it would be more obvious.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @aaronlehmann. The PR has been updated.

// For all other cases prefer the portConfig
portConfigs = append(portConfigs, portConfig)
// For all other cases prefer the portConfig
portConfigs = append(portConfigs, portConfig)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here; an if/else formulation might make the code ever so slightly clearer.

I apologize for picking nits over this kind of trivial thing. Normally I don't care very much about something like else vs continue. But this function is relatively complicated, and I'm trying to do all I can to make it intuitive.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @aaronlehmann. Also added additional comments to help explain the logic of the function.

@aaronlehmann
Copy link
Collaborator

LGTM

@yongtang yongtang force-pushed the 29730-multiple-published-port branch from 70032f9 to 2f92077 Compare January 4, 2017 00:48
@aaronlehmann
Copy link
Collaborator

ping @aboch @thaJeztah @LK4D4 for review

}
ps[portKey][p.PublishedPort] = p
}

Copy link

Choose a reason for hiding this comment

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

It maybe only me but I feel it will help if we add a comment here saying what this method does and what it means if it return a nil or a non nil object. So to spare the reader from reading the implementation to understand that.

@aboch
Copy link

aboch commented Jan 6, 2017

Thanks @yongtang

Code changes look good to me.

Up to you if you want to improve the comments now.

@yongtang yongtang force-pushed the 29730-multiple-published-port branch from 2f92077 to 555f411 Compare January 7, 2017 01:45
@yongtang
Copy link
Member Author

yongtang commented Jan 7, 2017

Thanks @aboch the PR has been updated with added comments.

This fix tries to address the issue raised in moby/moby#29370
where a service with multiple published ports mapping to the same target
port (e.g., `--publish 5000:80 --publish 5001:80`) can't be allocated.

The reason for the issue is that, `getPortConfigKey` is used for both
allocated ports and configured (may or may not be allocated) ports.
However, `getPortConfigKey` will not take into consideration the
`PublishedPort` field, which actually could be different for different
allocated ports.

This fix saves a map of `portKey:portNum:portState`,  instead of currently
used `portKey:portState` so that multiple published ports could be processed.

A test case has been added in the unit test. The newly added test case
will only work with this PR.

Signed-off-by: Yong Tang <[email protected]>
@yongtang yongtang force-pushed the 29730-multiple-published-port branch from 555f411 to 5fe66da Compare January 7, 2017 05:49
@aaronlehmann aaronlehmann merged commit c971468 into moby:master Jan 10, 2017
@yongtang yongtang deleted the 29730-multiple-published-port branch January 10, 2017 11:49
yongtang added a commit to yongtang/swarmkit that referenced this pull request Jan 30, 2017
This commit adds the `allocatedPorts` for cherry-pick.
The `allocatedPorts` was added in PR moby#1835, which is
not part of the v1.13.1 cherry pick.

Signed-off-by: Yong Tang <[email protected]>
@aaronlehmann aaronlehmann added this to the 1.13.2 milestone Feb 18, 2017
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