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

Fix Topology Aware Hints support for Kube-Proxy #100804

Merged
merged 1 commit into from Apr 12, 2021
Merged

Fix Topology Aware Hints support for Kube-Proxy #100804

merged 1 commit into from Apr 12, 2021

Conversation

aojea
Copy link
Member

@aojea aojea commented Apr 4, 2021 •
edited by liggitt

/kind bug

What this PR does / why we need it:

Fix kube-proxy implementation of Topology Aware Hints,
implemented in #99522

Fix regression in 1.21 in kube-proxy implementation of Topology Aware Hints

KEP: https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/2433-topology-aware-hints
Enhancement Issue: kubernetes/enhancements#2433
/sig network

/priority critical-urgent

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/network Categorizes an issue or PR as relevant to SIG Network. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 4, 2021
@aojea
Copy link
Member Author

aojea commented Apr 4, 2021

/assign @robscott

@k8s-ci-robot k8s-ci-robot added area/ipvs area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Apr 4, 2021
@aojea aojea force-pushed the topohints branch 2 times, most recently from a618036 to 6d3a4b2 Compare April 4, 2021 17:26
@robscott
Copy link
Member

robscott commented Apr 4, 2021

Thanks @aojea! Although there's lots of good improvements in here, I'd rather keep this fix small in scope so we can justify getting it into 1.21. If I'm understanding the issue, the true fix is contained in the small add node handlers to the metaproxier commit. The commits following that (copying labels and adding e2e test) feel like they would be good to have but may not be strictly required for this fix.

I think we should pull out the EndpointSlice v1 upgrade from this PR. The scope is a bit larger than the diff suggests, it's not just a 1:1 mapping between topology and deprecatedTopology. I think that will take more work and testing and does not feel like it's required as part of this fix.

robscott
robscott reviewed Apr 4, 2021
View reviewed changes
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this! I'm a bit hesitant about the scope of this PR, but really appreciate the core bug fix here.

test/e2e/network/topology.go Outdated Show resolved Hide resolved
test/e2e/network/topology.go Outdated Show resolved Hide resolved
test/e2e/network/topology.go Outdated Show resolved Hide resolved
test/e2e/network/topology.go Outdated Show resolved Hide resolved
@aojea
Copy link
Member Author

aojea commented Apr 5, 2021

Thanks for fixing this! I'm a bit hesitant about the scope of this PR, but really appreciate the core bug fix here.

Yeah, I just added everything in different commits to the PR so you can pick the important stuff, I wasn't sure what was needed and what can be postponed.

I will keep only the fix here and we can discuss the other stuff for 1.22

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 5, 2021
@aojea aojea mentioned this pull request Apr 5, 2021
Closed
@aojea
Copy link
Member Author

aojea commented Apr 5, 2021

/area kube-proxy

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@aojea
Copy link
Member Author

aojea commented Apr 9, 2021

/retest

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

5 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@aojea
Copy link
Member Author

aojea commented Apr 10, 2021

/retest

2 similar comments
@aojea
Copy link
Member Author

aojea commented Apr 10, 2021

/retest

@yangjunmyfm192085
Copy link
Contributor

/retest

@aojea
Copy link
Member Author

aojea commented Apr 10, 2021

the job has an issue, hold until we fix it
it will be #100992 or kubernetes/test-infra#21755 XD

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

4 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@aojea
Copy link
Member Author

aojea commented Apr 11, 2021

this is constantly failing, can it be the cause?

@aojea
Copy link
Member Author

aojea commented Apr 11, 2021

/retest

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Apr 12, 2021 •
edited

@aojea: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-conformance-kind-ipv6-parallel 1eb6584fabf75d4b1a2c603db239b0cfa79582f2 link /test pull-kubernetes-conformance-kind-ipv6-parallel
pull-kubernetes-e2e-gci-gce-ipvs 1eb6584fabf75d4b1a2c603db239b0cfa79582f2 link /test pull-kubernetes-e2e-gci-gce-ipvs
pull-kubernetes-e2e-ubuntu-gce-network-policies 1eb6584fabf75d4b1a2c603db239b0cfa79582f2 link /test pull-kubernetes-e2e-ubuntu-gce-network-policies

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 3490913 into kubernetes:master Apr 12, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Apr 12, 2021
@robscott robscott mentioned this pull request Apr 13, 2021
Merged
k8s-ci-robot added a commit that referenced this pull request Apr 28, 2021
@k8s-ci-robot
Merge pull request #101054 from robscott/automated-cherry-pick-of-#10…
7b66206 
…0804-release-1.21

Automated cherry pick of #100804: add node handlers to the metaproxier
@liggitt liggitt added the kind/regression Categorizes issue or PR as related to a regression from a prior release. label Apr 27, 2022
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robscott robscott robscott left review comments

@freehan freehan freehan left review comments

@andrewsykim andrewsykim Awaiting requested review from andrewsykim

@danwinship danwinship Awaiting requested review from danwinship

Assignees

@robscott robscott

@freehan freehan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/ipvs area/kube-proxy area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/regression Categorizes issue or PR as related to a regression from a prior release. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.22
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@aojea @robscott @k8s-ci-robot @fejta-bot @yangjunmyfm192085 @freehan @liggitt