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 changing host target port #2376

Merged
merged 1 commit into from Dec 6, 2017

Conversation

Projects
None yet
5 participants
@dperny
Member

dperny commented Sep 15, 2017

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.

@dperny

This comment has been minimized.

Show comment
Hide comment
@dperny

dperny Sep 15, 2017

Member

hey cool 2376 that's the default docker TLS port.

Member

dperny commented Sep 15, 2017

hey cool 2376 that's the default docker TLS port.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Sep 15, 2017

Codecov Report

Merging #2376 into master will decrease coverage by 0.3%.
The diff coverage is 85.71%.

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

codecov bot commented Sep 15, 2017

Codecov Report

Merging #2376 into master will decrease coverage by 0.3%.
The diff coverage is 85.71%.

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

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?

Show outdated Hide outdated manager/allocator/cnmallocator/portallocator.go Outdated
@@ -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

This comment has been minimized.

@fcrisciani

fcrisciani Sep 17, 2017

Member

this needs to be removed I guess

@fcrisciani

fcrisciani Sep 17, 2017

Member

this needs to be removed I guess

This comment has been minimized.

@dperny

dperny Sep 18, 2017

Member

i don't think so.

@dperny

dperny Sep 18, 2017

Member

i don't think so.

@dperny

This comment has been minimized.

Show comment
Hide comment
@dperny

dperny Sep 17, 2017

Member

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.

Member

dperny commented Sep 17, 2017

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.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Sep 20, 2017

Collaborator

Looks correct to me, but we should really fix #2143 once and for all.

Collaborator

aaronlehmann commented Sep 20, 2017

Looks correct to me, but we should really fix #2143 once and for all.

@dperny

This comment has been minimized.

Show comment
Hide comment
@dperny

dperny Dec 6, 2017

Member

apparently i'm the biggest idiot and forgot about the PR. thought it was already merged. i don't know what happened.

Member

dperny commented Dec 6, 2017

apparently i'm the biggest idiot and forgot about the PR. thought it was already merged. i don't know what happened.

Fix changing host target port failed
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>

@anshulpundir anshulpundir merged commit 13145e9 into docker:master Dec 6, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/project 61.78% (target 0%)
Details
dco-signed All commits are signed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment