Fix polarity of NodePort logic to avoid leaked ports #43259
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The result of this was that an update to a Service would release the NodePort temporarily (the repair loop would fix it in a minute). During that window, another Service could get allocated that Port.
k8s-ci-robot
added
the
cncf-cla: yes
k8s-github-robot
added
approved
|
This change is |
thockin
added
release-note-none
|
@k8s-bot verify test this |
|
@k8s-bot gci gce e2e test this |
anhowe
added a commit
to anhowe/kubernetes
that referenced
this pull request
Mar 17, 2017
|
@k8s-bot gci gce e2e test this |
|
User confirms this fixed the problem. Once LGTM I will offer a 1.5.x patch |
|
/lgtm Thanks for the quick fix @thockin |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, thockin
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
Any way to simulate this race from a unit test? |
|
yeah, I need to get a test ready for this, but I didn't want to block getting this up. |
|
Automatic merge from submit-queue |
justinsb
reviewed
Mar 17, 2017
| @@ -436,7 +436,7 @@ func (rs *REST) Update(ctx genericapirequest.Context, name string, objInfo rest. | |||
| // The comparison loops are O(N^2), but we don't expect N to be huge | |||
| // (there's a hard-limit at 2^16, because they're ports; and even 4 ports would be a lot) | |||
| for _, oldNodePort := range oldNodePorts { | |||
| if !contains(newNodePorts, oldNodePort) { | |||
| if contains(newNodePorts, oldNodePort) { | |||
There was a problem hiding this comment.
Gah - I should stop doing double-negatives :-(
|
@thockin do we not need to cherry-pick this into release-1.6 as well? |
|
shoot, so many balls in the air - YES, we need t pick this into 1.5 and
1.6, and I don't even know the current process for this stage of the
release cycle.
@ethernetdan ?
…On Fri, Mar 17, 2017 at 2:20 PM, Cole Mickens ***@***.***> wrote:
@thockin <https://github.com/thockin> do we not need to cherry-pick this
into release-1.6 as well?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#43259 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFVgVIuVuruvmcdg4DaAPjy2YUyfVONaks5rmvkdgaJpZM4MgCip>
.
|
|
Let me know... I can go ahead and generate and send the cherry-pick for 1.6... it looks like you got it into 1.5 already. |
|
Is it the same process for 1.6 release branch?
…On Mon, Mar 20, 2017 at 10:43 AM, Cole Mickens ***@***.***> wrote:
Let me know... I can go ahead and generate and send the cherry-pick for
1.6... it looks like you got it into 1.5 already
<#43270>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#43259 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFVgVEBJtevQpHm14-WSJRmy7HHFp3k6ks5rnrq_gaJpZM4MgCip>
.
|
|
+++ Creating a pull request on GitHub at
thockin:automated-cherry-pick-of-#43259-upstream-release-1.6
Error creating pull request: Unprocessable Entity (HTTP 422)
No commits between kubernetes:release-1.6 and
thockin:automated-cherry-pick-of-#43259-upstream-release-1.6
Did it get bulk merged?
…On Mon, Mar 20, 2017 at 12:09 PM, Tim Hockin ***@***.***> wrote:
Is it the same process for 1.6 release branch?
On Mon, Mar 20, 2017 at 10:43 AM, Cole Mickens ***@***.***>
wrote:
> Let me know... I can go ahead and generate and send the cherry-pick for
> 1.6... it looks like you got it into 1.5 already
> <#43270>.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#43259 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AFVgVEBJtevQpHm14-WSJRmy7HHFp3k6ks5rnrq_gaJpZM4MgCip>
> .
>
|
|
I don't really understand how, but your commit is in release-1.6... I manually checked the commit history and looked at the current state of the code... Sorry for the noise, I guess it's all already taken care of. |
|
@thockin @colemickens we are still fastforwarding changes into release-1.6 from master. Will stop after rc.1 comes out or the release, whichever comes first. |
|
OK. but merge queue is only taking milestoned PRs, right? :)
…On Mon, Mar 20, 2017 at 12:28 PM, Dan Gillespie ***@***.***> wrote:
@thockin <https://github.com/thockin> @colemickens
<https://github.com/colemickens> we are still fastforwarding changes into
release-1.6 from master. Will stop after rc.1 comes out or the release,
whichever comes first.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#43259 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFVgVLFWre9d0WN1qQRyBJzddJ3G5FDEks5rntN4gaJpZM4MgCip>
.
|
|
It is but it's implemented as a blacklist. We probably want to change that 😃 |
|
yeah, that should be fixed.
…On Mon, Mar 20, 2017 at 12:39 PM, Dan Gillespie ***@***.***> wrote:
It is but it's implemented as a blacklist
<https://github.com/enisoc/test-infra/blob/e287770/mungegithub/submit-queue/deployment/kubernetes/configmap.yaml#L115>.
We probably want to change that 😃
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#43259 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFVgVLXNh8GFyZCboA_c4NFYYBR-Xubcks5rntX-gaJpZM4MgCip>
.
|
thockin
added
release-note
thockin
changed the title
|
Commit found in the "release-1.5" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The result of this was that an update to a Service would release the
NodePort temporarily (the repair loop would fix it in a minute). During
that window, another Service could get allocated that Port.
Fixes #43233