Fix issues of multiple published ports mapping to the same target port by yongtang · Pull Request #1835 · moby/swarmkit

yongtang · 2016-12-27T21:38:44Z

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 yong.tang.github@outlook.com

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

codecov-io · 2016-12-27T22:20:51Z

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

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

thaJeztah · 2016-12-28T08:28:46Z

manager/allocator/networkallocator/portallocator.go

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


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?

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.

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)

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 · 2016-12-28T10:33:18Z

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 · 2016-12-30T01:16:37Z

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.

aaronlehmann · 2016-12-30T07:52:21Z

manager/allocator/networkallocator/portallocator.go

 		// the port config.
 		if portConfig.PublishMode != api.PublishModeIngress {
 			portConfigs = append(portConfigs, portConfig)
 			continue


This continue doesn't look right.

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).

aaronlehmann · 2016-12-30T07:53:59Z

manager/allocator/networkallocator/portallocator.go

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


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

@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?

aaronlehmann · 2016-12-30T07:54:39Z

manager/allocator/networkallocator/portallocator.go

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


Same here, iterating over portConfigs could simplify the loop.

aaronlehmann · 2017-01-03T22:01:05Z

manager/allocator/networkallocator/portallocator.go

+		if !ok {
+			return nil
+		}
+		// Dlete state from allocatedPorts


Thanks. Done.

aaronlehmann · 2017-01-03T22:02:55Z

manager/allocator/networkallocator/portallocator.go

+
+		if !ok {
+			return nil
+		}


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.

Thanks @aaronlehmann Done.

aaronlehmann · 2017-01-03T22:03:15Z

manager/allocator/networkallocator/portallocator.go

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


aaronlehmann · 2017-01-03T22:03:31Z

manager/allocator/networkallocator/portallocator.go

 }

-// getPortConfigKey returns a map key for doing set operations with
+// getPortConfigkey returns a map key for doing set operations with


Comment should start with getPortConfigKey

aaronlehmann · 2017-01-03T22:08:50Z

manager/allocator/networkallocator/portallocator.go

 		}
-
-		allocatedPorts[getPortConfigKey(portState)] = portState
+		portStates.addState(portState)


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

aaronlehmann · 2017-01-03T22:17:30Z

manager/allocator/networkallocator/portallocator.go

 		}
-
-		allocatedPorts[getPortConfigKey(portState)] = portState
+		portStates.addState(portState)


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

aaronlehmann · 2017-01-03T22:20:57Z

manager/allocator/networkallocator/portallocator.go

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


delete(portStateMap, p.PublishedPort)

aaronlehmann · 2017-01-03T22:21:03Z

manager/allocator/networkallocator/portallocator.go

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


delete(portStateMap, p.PublishedPort)

aaronlehmann · 2017-01-03T22:21:57Z

Overall LGTM, just some comments on typos and cosmetics.

yongtang · 2017-01-04T00:07:18Z

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

aaronlehmann · 2017-01-04T00:28:38Z

manager/allocator/networkallocator/portallocator.go

 			portConfigs = append(portConfigs, portConfig)
 			continue
 		}
+		if portConfig.PublishedPort != 0 {


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.

Thanks @aaronlehmann. The PR has been updated.

aaronlehmann · 2017-01-04T00:32:34Z

manager/allocator/networkallocator/portallocator.go

-		// For all other cases prefer the portConfig
-		portConfigs = append(portConfigs, portConfig)
+			// For all other cases prefer the portConfig
+			portConfigs = append(portConfigs, portConfig)


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.

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

aaronlehmann · 2017-01-04T00:37:02Z

LGTM

aaronlehmann · 2017-01-06T02:05:18Z

ping @aboch @thaJeztah @LK4D4 for review

aboch · 2017-01-06T18:37:15Z

manager/allocator/networkallocator/portallocator.go

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


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 · 2017-01-06T19:09:30Z

Thanks @yongtang

Code changes look good to me.

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

yongtang · 2017-01-07T01:46:52Z

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 <yong.tang.github@outlook.com>

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 <yong.tang.github@outlook.com>

yongtang mentioned this pull request Dec 27, 2016

[1.13.x] same container port published twice prevent service to run moby/moby#29730

Closed

thaJeztah reviewed Dec 28, 2016

View reviewed changes

yongtang force-pushed the 29730-multiple-published-port branch from dda04ab to c786e14 Compare December 30, 2016 01:07

yongtang mentioned this pull request Dec 30, 2016

Fix issues of multiple published ports mapping to the same target port moby/moby#29787

Merged

aaronlehmann reviewed Dec 30, 2016

View reviewed changes

yongtang force-pushed the 29730-multiple-published-port branch from c786e14 to 6b7c5df Compare January 2, 2017 13:10

aaronlehmann reviewed Jan 3, 2017

View reviewed changes

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

aaronlehmann reviewed Jan 4, 2017

View reviewed changes

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

aboch reviewed Jan 6, 2017

View reviewed changes

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

yongtang force-pushed the 29730-multiple-published-port branch from 555f411 to 5fe66da Compare January 7, 2017 05:49

aaronlehmann merged commit c971468 into moby:master Jan 10, 2017

yongtang deleted the 29730-multiple-published-port branch January 10, 2017 11:49

yongtang mentioned this pull request Jan 30, 2017

[v1.13.1] Cherry-Pick [#1875]: Fix issue in service update of published ports in host mode #1910

Merged

aaronlehmann added this to the 1.13.2 milestone Feb 18, 2017

aaronlehmann added the process/cherry-picked label Feb 18, 2017

This was referenced Feb 18, 2017

[1.13] Vendor swarmkit 30a4278 moby/moby#31143

Merged

update CHANGELOG for 17.03.0-ce moby/moby#31205

Merged

Conversation

yongtang commented Dec 27, 2016

Uh oh!

codecov-io commented Dec 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 54.62% (diff: 94.87%)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Dec 28, 2016

Uh oh!

yongtang commented Dec 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Jan 3, 2017

Uh oh!

yongtang commented Jan 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Jan 4, 2017

Uh oh!

aaronlehmann commented Jan 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aboch commented Jan 6, 2017

codecov-io commented Dec 27, 2016 •

edited

Loading