8000 Don't log irrelevant zone hints message on no endpoints by milesbxf · Pull Request #132680 · kubernetes/kubernetes · GitHub
[go: up one dir, main page]
Skip to content

Don't log irrelevant zone hints message on no endpoints #132680

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
Merged
merged 2 commits into from
Jul 8, 2025
Merged

Don't log irrelevant zone hints message on no endpoints #132680

merged 2 commits into from
Jul 8, 2025

Conversation

milesbxf
Copy link
Contributor
@milesbxf milesbxf commented Jul 2, 2025

What type of PR is this?

/kind bug

What this PR does / why we need it:

See #132678 - kube-proxy is currently logging erroneous messages about ignoring topology hints when an endpointslice is empty. This short circuits the function and returns the default traffic distribution if there are no endpoints.

Which issue(s) this PR is related to:

Fixes #132678

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@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/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 2, 2025
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 2, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @milesbxf. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /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 by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added area/kube-proxy sig/network Categorizes an issue or PR as relevant to SIG Network. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 2, 2025
@milesbxf
8000 Copy link
Contributor Author
milesbxf commented Jul 2, 2025

/sig network

@milesbxf
Copy link
Contributor Author
milesbxf commented Jul 2, 2025

cc. @danwinship as it looks like you last touched this code

@aojea
Copy link
Member
aojea commented Jul 2, 2025

This is called from

kubernetes/pkg/proxy/topology.go

Line 44 in 95bff1b

func CategorizeEndpoints(endpoints []Endpoint, svcInfo ServicePort, nodeName string, nodeLabels map[string]string) (clusterEndpoints, localEndpoints, allReachableEndpoints []Endpoint, hasAnyEndpoints bool) {
so I wonder if we want to be also defensive there and return faster if there are no endpoints

/assign @danwinship

@danwinship
Copy link
Contributor

Yeah, it would probably be good to just bail out of CategorizeEndpoints early if there aren't any endpoints.

But we should still fix topologyModeFromHints... Or at least, if it's going to give the wrong answer for 0 endpoints, we should document that, but it seems dumb to document it rather than fixing it.

danwinship
danwinship reviewed Jul 2, 2025
View reviewed changes
Copy link
Contributor
@danwinship danwinship left a comment

Choose a reason for hiding this comment

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

Also, this needs an addition to the unit test in topology_test.go

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 3, 2025
@milesbxf
Copy link
Contributor Author
milesbxf commented Jul 3, 2025

Unit test added (thanks also for the clarification suggestion on the comment)

Yeah, it would probably be good to just bail out of CategorizeEndpoints early if there aren't any endpoints.

But we should still fix topologyModeFromHints... Or at least, if it's going to give the wrong answer for 0 endpoints, we should document that, but it seems dumb to document it rather than fixing it.

Happy to add this too - what are the expected return values if there are no endpoints? Would return nil, nil, nil, false do the trick (empty endpoints, hasAnyEndpoints=false)?

@aojea
Copy link
Member
aojea commented Jul 3, 2025

/ok-to-test

please squash the commits , and you can add a separate commit with the CategorizeEndpoints changes

what are the expected return values if there are no endpoints? Would return nil, nil, nil, false do the trick (empty endpoints, hasAnyEndpoints=false)?

those are named variables so you can just return

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 3, 2025
@milesbxf
Don't log irrelevant zone hints message on no endpoints
1ac5f66 
Update pkg/proxy/topology.go

Co-authored-by: Dan Winship <danwinship@redhat.com>

Add unit test case
@milesbxf milesbxf force-pushed the topology-hints-dont-log-for-empty-endpointslices branch from 8a1761c to 060f4ce Compare July 4, 2025 10:14
danwinship
danwinship reviewed Jul 4, 2025
View reviewed changes
Copy link
Contributor
@danwinship danwinship left a comment

Choose a reason for hiding this comment

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

< 10000 div class="comment-body markdown-body js-comment-body soft-wrap css-overflow-wrap-anywhere user-select-contain d-block">

looks good, just nitpicks
@milesbxf milesbxf force-pushed the topology-hints-dont-log-for-empty-endpointslices branch from 060f4ce to fbd2bd4 Compare July 7, 2025 10:28
@milesbxf
Copy link
Contributor Author
milesbxf commented Jul 7, 2025

looks good, just nitpicks

Thanks - I think I've addressed everything here + squashed

danwinship
danwinship approved these changes Jul 7, 2025
View reviewed changes
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, milesbxf

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2025
@milesbxf milesbxf force-pushed the topology-hints-dont-log-for-empty-endpointslices branch from fbd2bd4 to 071b9f3 Compare July 8, 2025 10:14
@milesbxf milesbxf force-pushed the topology-hints-dont-log-for-empty-endpointslices branch from 071b9f3 to 1cec0ac Compare July 8, 2025 10:14
@aojea
Copy link
Member
aojea commented Jul 8, 2025

/lgtm

/test pull-kubernetes-e2e-gce

Does not look related

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: d291a683add2902945e3869817432bf34b191fbc

@k8s-ci-robot k8s-ci-robot merged commit beb3d92 into kubernetes:master Jul 8, 2025
13 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.34 milestone Jul 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danwinship danwinship 52F4 danwinship approved these changes

@adrianmoisey adrianmoisey Awaiting requested review from adrianmoisey

@aojea aojea Awaiting requested review from aojea

Assignees

@danwinship danwinship

@aojea aojea

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kube-proxy cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. sig/network Categorizes an issue or PR as relevant to SIG Network. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.34
Development

Successfully merging this pull request may close these issues.

kube-proxy incorrectly logs "Ignoring same-zone topology hints" when there are no endpoints
4 participants
@milesbxf @k8s-ci-robot @aojea @danwinship
0