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.

Sign up for GitHub

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

Jump to bottom

Revert "Revert "syncNetworkUtil in kubelet and fix loadbalancerSourceRange on GCE #30814

Merged
merged 3 commits into from
Aug 18, 2016
Merged

Revert "Revert "syncNetworkUtil in kubelet and fix loadbalancerSourceRange on GCE #30814

merged 3 commits into from
Aug 18, 2016

Conversation

freehan
Copy link
Contributor

@freehan freehan commented Aug 17, 2016 •
edited by k8s-oncall
Loading

Reverts #30729

This change is Reviewable

@freehan freehan added this to the v1.3 milestone Aug 17, 2016
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Aug 17, 2016
@freehan
Copy link
Contributor Author

freehan commented Aug 17, 2016

just bump into another problem. Will ping you when this is ready

@freehan freehan added cherrypick-candidate release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Aug 17, 2016
@thockin thockin added priority/backlog Higher priority than priority/awaiting-more-evidence. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 17, 2016
@thockin
Copy link
Member

thockin commented Aug 17, 2016

@fabioy for cherry pick consideration

@freehan
Copy link
Contributor Author

freehan commented Aug 18, 2016 •
edited
Loading

changing KUBE-XLB name back to KUBE-FW base on discussion

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 18, 2016
@freehan
Copy link
Contributor Author

freehan commented Aug 18, 2016

KUBE-FW has N rules per sourceRanges
KUBE-XLB has M rules per local endpoints

If we combine them, KUBE-XLB will end up with N * M rules. Any other tricks to solve this?

@k8s-github-robot k8s-github-robot mentioned this pull request Aug 18, 2016
Closed
@thockin
Copy link
Member

thockin commented Aug 18, 2016

I had to write it all out to understand, but now I do. Yech. The names
still make me grumpy. OK

On Wed, Aug 17, 2016 at 6:26 PM, Minhan Xia notifications@github.com
wrote:

KUBE-FW has N rules per sourceRanges
KUBE-XLB has M rules per local endpoints

If we combine them, KUBE-XLB will end up with N * M rules. Any other
tricks to solve this?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30814 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVOFSUIR9mr3OVLiP0LzYhaNlzM0Eks5qg7TEgaJpZM4Jm4n6
.

@thockin thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 18, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2016
@girishkalele
Copy link

Potentially ipsets would solve this - reducing the N rules to 1 source ip-set rule - but we then need a whole new library to manage the ipset.

@thockin
Copy link
Member

thockin commented Aug 18, 2016

yeah, optimize as a second pass.

On Thu, Aug 18, 2016 at 10:04 AM, Girish Kalele notifications@github.com
wrote:

Potentially ipsets would solve this - reducing the N rules to 1 source
ip-set rule - but we then need a whole new library to manage the ipset.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30814 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVJ41NVeiLv-VdqjrFH9RtwvqM1Ldks5qhJCugaJpZM4Jm4n6
.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 18, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2016
@freehan
Copy link
Contributor Author

freehan commented Aug 18, 2016 •
edited
Loading

Rebased. Only generated code changed. Reapplying lgtm

@freehan freehan added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 18, 2016
@k8s-bot
Copy link

k8s-bot commented Aug 18, 2016

GCE e2e build/test passed for commit 392a92c.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Aug 18, 2016

GCE e2e build/test passed for commit 392a92c.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 7523669 into kubernetes:master Aug 18, 2016
@fabioy
Copy link
Contributor

fabioy commented Aug 18, 2016

Cherrypick approved. I'm assuming this supercedes #30486 and I can drop the "cherrypick candidate" label from that one?

@fabioy fabioy added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Aug 18, 2016
@thockin
Copy link
Member

thockin commented Aug 18, 2016

Correct

On Thu, Aug 18, 2016 at 3:53 PM, Fabio Yeon notifications@github.com
wrote:

Cherrypick approved. I'm assuming this supercedes #30486
#30486 and I can drop the
"cherrypick candidate" label from that one?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30814 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVBdF9NMP2EfVat6jAcNlk-yu4TYNks5qhOKDgaJpZM4Jm4n6
.

@fabioy
Copy link
Contributor

fabioy commented Aug 18, 2016

Ugh, too many conflicts, couldn't create an automatic cherrypick. Please create one and I'll try to get this in quickly. Thanks.

@freehan
Copy link
Contributor Author

freehan commented Aug 18, 2016

Can we let this sit overnight?

@thockin
Copy link
Member

thockin commented Aug 18, 2016

yes please

On Thu, Aug 18, 2016 at 4:05 PM, Minhan Xia notifications@github.com
wrote:

Can we let this sit overnight?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30814 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVI43cr4S4OHPfH3n1-VgkzhnhQcgks5qhOU6gaJpZM4Jm4n6
.

@freehan freehan mentioned this pull request Aug 19, 2016
Merged
@freehan freehan added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 19, 2016
@thockin
Copy link
Member

thockin commented Aug 19, 2016

This really needs e2e coverage

fabioy added a commit that referenced this pull request Aug 22, 2016
@fabioy
Merge pull request #31019 from freehan/automated-cherry-pick-of-#3081…
50f4077 
…4-upstream-release-1.3

Automated cherry pick of #30814
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.3" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

@liggitt
Copy link
Member

liggitt commented Aug 23, 2016

just saw this go into 1.3 in #31019. is there an urgent reason this was needed in 1.3? the linked issues didn't seem to be high priority enough to justify such a large pick

@fabioy
Copy link
Contributor

fabioy commented Aug 23, 2016

Combined, they fix two networking issues, which was deemed fix worthy. This issue was passed over previous patch releases until it was deemed ok.

@freehan freehan mentioned this pull request Aug 23, 2016
Closed
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@fabioy
Merge pull request kubernetes#31019 from freehan/automated-cherry-pic…
22b30b6 
…k-of-#30814-upstream-release-1.3

Automated cherry pick of kubernetes#30814
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@thockin thockin

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.