Fix changing host target port#2376
Conversation
|
hey cool 2376 that's the default docker TLS port. |
Codecov Report
@@ Coverage Diff @@
## master #2376 +/- ##
==========================================
- Coverage 62.08% 61.78% -0.31%
==========================================
Files 49 128 +79
Lines 6842 21114 +14272
==========================================
+ Hits 4248 13045 +8797
- Misses 2163 6663 +4500
- Partials 431 1406 +975 |
fcrisciani
left a comment
fcrisciani
left a comment
There was a problem hiding this comment.
LGTM, agree that the logic looks a bit fuzzy, still not have super clear what was happening before like the isPortsAllocated was returning true so was not clearing the previous state?
| portStates := allocatedPorts{} | ||
| hostTargetPorts := map[uint32]struct{}{} | ||
| for _, portState := range s.Endpoint.Ports { | ||
| if portState.PublishMode == api.PublishModeIngress { |
There was a problem hiding this comment.
will it be nicer with a switch as below?
| @@ -344,18 +353,28 @@ func (pa *portAllocator) isPortsAllocatedOnInit(s *api.Service, onInit bool) boo | |||
| // Iterate portConfigs with PublishedPort == 0 (low priority) | |||
| for _, portConfig := range s.Spec.Endpoint.Ports { | |||
| // Ignore ports which are not PublishModeIngress | |||
|
Yeah my understanding is that every case where a port was added or removed caused an allocation. But if a port was added and a port was removed, there was no logic to handle that, because host ports changes are usually handled in a different function. But because this seemed more like the add/remove case than the host port changed case, I went with changing this function. Sorry if that's a little unspecific, I'm on mobile. |
| // a host port that's not in the service, then we need to do | ||
| // allocation. if we get the same target port but something else | ||
| // has changed, then HostPublishPortsNeedUpdate will cover that | ||
| // case. don't ask me why this is handled in two different code |
There was a problem hiding this comment.
nit: @dperny can you add this as a TODO(dperny) or something so it's easy to search for later?
There was a problem hiding this comment.
it's not really a TODO, it's just a comment. it's handled correctly in the other case i'm just explaining what it is. i'm adding "don't ask me" because i don't want someone to to a git blame in a year and ask my why this is the case.
dperny
force-pushed
the
fix-changing-host-port
branch
from
071e07d to
d5bb8bf
Compare
| } | ||
| case api.PublishModeHost: | ||
| // check if the target port is already in the port config. if it | ||
| // is, then it's our problem. |
There was a problem hiding this comment.
Should this read "not our problem"?
|
Looks correct to me, but we should really fix #2143 once and for all. |
dperny
force-pushed
the
fix-changing-host-port
branch
from
d5bb8bf to
1602055
Compare
|
apparently i'm the biggest idiot and forgot about the PR. thought it was already merged. i don't know what happened. |
Fixes a bug where if a service has the same number of host-mode published ports with PublishedPort 0, changes to the spec would not reflect in the service object. Signed-off-by: Drew Erny <drew.erny@docker.com>
5 participants
Fixes a bug where if a service has the same number of host-mode published ports with PublishedPort 0, changes to the spec would not reflect in the service object.
Also altered the unit tests to be more informative, and added a new unit test case for this bug.