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

Never set hairpin mode on every interface #36990

Merged
merged 1 commit into from
Dec 6, 2016
Merged

Never set hairpin mode on every interface #36990

merged 1 commit into from
Dec 6, 2016

Conversation

bboreham
Copy link
Contributor

@bboreham bboreham commented Nov 17, 2016 •
edited by thockin
Loading

What this PR does / why we need it:

Abandon setting hairpin mode if finding the peer interface fails; simply return an error.

There are many reasons why finding the peer could fail - "ethtool not installed" is popular. Going ahead and changing the hairpin setting on every bridge-connected interface on the machine may have unwanted effects on other things installed on the machine (e.g. kubernetes/kops#879)

Which issue this PR fixes : fixes #19766

Special notes for your reviewer:

Release note:

Kubelet will no longer set hairpin mode on every interface on the machine when an error occurs in setting up hairpin for a specific interface.

/cc @thockin who appears to have requested this implementation at #13628 (comment)

This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot ok to test" on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands will still work. Regular contributors should join the org to skip this step.

If you have questions or suggestions related to this bot's behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Nov 17, 2016
@yujuhong yujuhong assigned thockin and unassigned yujuhong Nov 17, 2016
@thockin
Copy link
Member

thockin commented Nov 18, 2016

@k8s-bot ok to test

@thockin thockin added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Nov 18, 2016
@thockin
Copy link
Member

thockin commented Nov 18, 2016

LGTM, but I am going to retitle for a relnote

@thockin
Copy link
Member

thockin commented Nov 18, 2016

oh, I see you already put one, thanks :)

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit 4f392eb3663a2d34eeb8def7aea17dea76465ac5. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@bboreham
Copy link
Contributor Author

It looks to me like this failed with a flake identical to #36153

@k8s-bot gce etcd3 e2e test this

@k8s-ci-robot
Copy link
Contributor

@bboreham: you can't request testing unless you are a kubernetes member.

In response to this comment:

It looks to me like this failed with a flake identical to #36153

@k8s-bot gce etcd3 e2e test this

If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository.

@thockin
Copy link
Member

thockin commented Nov 22, 2016

@k8s-bot gce etcd3 e2e test this

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 4f392eb3663a2d34eeb8def7aea17dea76465ac5. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot
Copy link

@k8s-bot test this issue: #IGNORE

Tests have been pending for 24 hours

@bboreham
Abandon setting hairpin mode if finding the peer interface fails
0cfd09e 
Instead of setting it on every bridge-connected interface which may
have unwanted effects on unrelated things installed on the machine.
@bboreham bboreham force-pushed the never-hairpin-all-interfaces branch from 4f392eb to 0cfd09e Compare December 6, 2016 14:34
@bboreham
Copy link
Contributor Author

bboreham commented Dec 6, 2016

Rebased. Can't hurt, right?

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 6, 2016
@thockin thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 6, 2016
@k8s-github-robot
Copy link

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 36990, 37494, 38152, 37561, 38136)

@k8s-github-robot k8s-github-robot merged commit 3c1a8f0 into kubernetes:master Dec 6, 2016
@bboreham bboreham mentioned this pull request Dec 9, 2016
Closed
@bboreham bboreham mentioned this pull request Dec 18, 2016
Closed
@bboreham bboreham mentioned this pull request Jan 24, 2017
Closed
@chrislovecnm
Copy link
Contributor

@thockin I am guessing that this amazing fix was not cherry picked? Say it ain't so ;( I got his release notes let me look.

@itskingori itskingori mentioned this pull request Jan 30, 2017
Closed
@adrianmoisey adrianmoisey mentioned this pull request Feb 1, 2017
Closed
@itskingori
Copy link
Member

@chrislovecnm We've cherry-picked this fix for release-1.5 in #40788. We're not sure of the internal processes to get this through (testing et al). cc: @thockin

Ps: We're experiencing this issue on 1.4.7 so we could cherry pick it also for release-1.4 as well ... but I guess we can upgrade to 1.5 if it goes in.

@freehan freehan mentioned this pull request Feb 1, 2017
Merged
@freehan
Copy link
Contributor

freehan commented Feb 1, 2017

Created PR to cherrypick this into 1.4 #40816

k8s-github-robot pushed a commit that referenced this pull request Feb 3, 2017
Merge pull request #40816 from freehan/automated-cherry-pick-of-#3699…
3e2ebb2 
…0-upstream-release-1.4

Automatic merge from submit-queue

Automated cherry pick of #36990

Cherry pick of #36990 on release-1.4.

#36990: Abandon setting hairpin mode if finding the peer interface
@adrianmoisey
Copy link
Member

@freehan thanks for cherry picking into 1.4.
Do you know how I can get my 1.5 cherry pick (#40788) pushed through?

@freehan
Copy link
Contributor

freehan commented Feb 3, 2017

@adrianmoisey I think it is better for you to use the automatic cherry-pick script and push another PR

hack/cherry_pick_pull.sh

@adrianmoisey adrianmoisey mentioned this pull request Feb 6, 2017
Merged
@saad-ali saad-ali added this to the v1.5 milestone Feb 7, 2017
@saad-ali saad-ali added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cherrypick-candidate labels Feb 7, 2017
@itskingori itskingori mentioned this pull request Feb 8, 2017
Closed
k8s-github-robot pushed a commit that referenced this pull request Feb 10, 2017
Merge pull request #40993 from adrianmoisey/automated-cherry-pick-of-…
6b3c9cf 
…#36990-origin-release-1.5

Automatic merge from submit-queue

Automated cherry pick of #36990

Cherry pick of #36990 on release-1.5.

#36990: Abandon setting hairpin mode if finding the peer interface
@k8s-cherrypick-bot
Copy link

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
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.

kubelet sets veth hairpin mode regardless of network plugin