[kube-proxy] add log verbosity to endpoint topology hint loop. #123322

bjhaid · 2024-02-15T13:56:40Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

We enabled topology hint on one of our services and this log line was emitted ~92 million times in one day from one cluster tripping our log quota for that cluster, as it is the log line cannot be disabled via the -v flag because it does not specify verbosity.

I think more log locations need to set verbosity at which they are logged, but this one is currently hurting the most.

Which issue(s) this PR fixes:

Fixes #

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 · 2024-02-15T13:56:49Z

Hi @bjhaid. 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/test-infra repository.

thockin · 2024-02-15T17:02:01Z

Thanks!

/lgtm
/approve

k8s-ci-robot · 2024-02-15T17:02:09Z

LGTM label has been added.

Git tree hash: 9acea8b3a387700d16f806d8ba2df268b862473b

k8s-ci-robot · 2024-02-15T17:02:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bjhaid, priestjim, thockin

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:

~~pkg/proxy/OWNERS~~ [thockin]

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

danwinship · 2024-02-15T17:05:33Z

Does this really solve anything? It seems like the real problem is that we need to throttle this notification

aojea · 2024-02-15T17:06:06Z

pkg/proxy/topology.go

@@ -166,7 +166,7 @@ func canUseTopology(endpoints []Endpoint, svcInfo ServicePort, nodeLabels map[st
 			continue
 		}
 		if endpoint.ZoneHints().Len() == 0 {
-			klog.InfoS("Skipping topology aware endpoint filtering since one or more endpoints is missing a zone hint", "endpoint", endpoint)
+			klog.V(2).InfoS("Skipping topology aware endpoint filtering since one or more endpoints is missing a zone hint", "endpoint", endpoint)


should we switch all log statements to V(2) ?

ping @robscott

I think yes, outside of all the startup logs most or all log sites should be V(N)

do you want to change it?

I mean, the two other logs statements in this function, they will have the same issue

do you want to change it?

I didn't want to start combing through all kube-proxy logs sites to update in this PR

I mean, the two other logs statements in this function, they will have the same issue

I can update those as well, just started with the most annoying, the other 2 combined for ~6M lines/day which is way less than the one I started with. I'll push changes to update the other log statements as well.

updated the other 2 locations in this file. If I can find time I'll look into sending a PR for all of kube-proxy, but I can't guarantee that will happen today.

Yeah this seems like a good improvement, thanks @bjhaid!

bjhaid · 2024-02-15T18:10:13Z

Does this really solve anything? It seems like the real problem is that we need to throttle this notification

the logs is maybe useful for debugging, but it is noise for me as a cluster operator. I believe one should be able to turn off all none fatal/bootup logs. So yes while throttling might help, the log and a bunch of logs from kube-proxy today is noise, I almost never look at them unless I am troubleshooting stuff and I'll like to be able to turn most of them of and on as I wish.

We enabled topology hint on one of our services and this log line was emitted ~92 million times in one day from one cluster tripping our log quota for that cluster, as it is the log line cannot be disabled via the `-v` flag because it does not specify verbosity. I think more log locations need to set verbosity at which they are logged, but this one is currently hurting the most.

aojea · 2024-02-15T19:51:12Z

/ok-to-test

@danwinship I think that is fair ask to be able to disable them , are you ok with this?

robscott

Thanks @bjhaid! This is a great fix - I'd also be open to @danwinship's suggestion of some kind of throttling here, but setting a log level like this does seems like a great improvement that we could build on in the future.

/lgtm

robscott · 2024-02-15T20:05:23Z

pkg/proxy/topology.go

@@ -166,7 +166,7 @@ func canUseTopology(endpoints []Endpoint, svcInfo ServicePort, nodeLabels map[st
 			continue
 		}
 		if endpoint.ZoneHints().Len() == 0 {
-			klog.InfoS("Skipping topology aware endpoint filtering since one or more endpoints is missing a zone hint", "endpoint", endpoint)
+			klog.V(2).InfoS("Skipping topology aware endpoint filtering since one or more endpoints is missing a zone hint", "endpoint", endpoint)


Yeah this seems like a good improvement, thanks @bjhaid!

k8s-ci-robot · 2024-02-15T20:08:01Z

LGTM label has been added.

Git tree hash: e854e0f912af8cff0490b1b035871d0ba41664dc

robscott · 2024-02-15T20:08:27Z

/triage accepted
/priority important-soon

…f-#123322-upstream-release-1.28 Automated cherry pick of #123322: add log verbosity to endpoint topology hint

…f-#123322-upstream-release-1.29 Automated cherry pick of #123322: add log verbosity to endpoint topology hint

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Feb 15, 2024

k8s-ci-robot requested review from aojea and freehan February 15, 2024 13:57

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 Feb 15, 2024

priestjim approved these changes Feb 15, 2024

View reviewed changes

k8s-ci-robot assigned thockin Feb 15, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 15, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 15, 2024

aojea reviewed Feb 15, 2024

View reviewed changes

bjhaid force-pushed the bjhaid-topology-verbosity branch from d4da510 to 71479b5 Compare February 15, 2024 18:26

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 15, 2024

k8s-ci-robot requested a review from thockin February 15, 2024 18:26

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Feb 15, 2024

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 15, 2024

robscott reviewed Feb 15, 2024

View reviewed changes

k8s-ci-robot assigned robscott Feb 15, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 15, 2024

k8s-ci-robot merged commit ad6477e into kubernetes:master Feb 15, 2024
14 checks passed

k8s-ci-robot added this to the v1.30 milestone Feb 15, 2024

bjhaid deleted the bjhaid-topology-verbosity branch February 15, 2024 21:01

mengjiao-liu mentioned this pull request Apr 17, 2024

When Topology Aware Hints are disabled, kube-proxy shouldn't spam the logs #124341

Closed

This was referenced Apr 18, 2024

Automated cherry pick of #123322: add log verbosity to endpoint topology hint #124354

Merged

Automated cherry pick of #123322: add log verbosity to endpoint topology hint #124355

Merged

k8s-ci-robot added a commit that referenced this pull request Apr 25, 2024

Merge pull request #124355 from liangyuanpeng/automated-cherry-pick-o…

402fbee

…f-#123322-upstream-release-1.28 Automated cherry pick of #123322: add log verbosity to endpoint topology hint

k8s-ci-robot added a commit that referenced this pull request Apr 25, 2024

Merge pull request #124354 from liangyuanpeng/automated-cherry-pick-o…

d138c02

…f-#123322-upstream-release-1.29 Automated cherry pick of #123322: add log verbosity to endpoint topology hint

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[kube-proxy] add log verbosity to endpoint topology hint loop. #123322

[kube-proxy] add log verbosity to endpoint topology hint loop. #123322

bjhaid commented Feb 15, 2024

k8s-ci-robot commented Feb 15, 2024

thockin commented Feb 15, 2024

k8s-ci-robot commented Feb 15, 2024

k8s-ci-robot commented Feb 15, 2024

danwinship commented Feb 15, 2024

aojea Feb 15, 2024

aojea Feb 15, 2024

bjhaid Feb 15, 2024

aojea Feb 15, 2024

aojea Feb 15, 2024

bjhaid Feb 15, 2024

bjhaid Feb 15, 2024

robscott Feb 15, 2024

bjhaid commented Feb 15, 2024

aojea commented Feb 15, 2024

robscott left a comment

robscott Feb 15, 2024

k8s-ci-robot commented Feb 15, 2024

robscott commented Feb 15, 2024

[kube-proxy] add log verbosity to endpoint topology hint loop. #123322

[kube-proxy] add log verbosity to endpoint topology hint loop. #123322

Conversation

bjhaid commented Feb 15, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Feb 15, 2024

thockin commented Feb 15, 2024

k8s-ci-robot commented Feb 15, 2024

k8s-ci-robot commented Feb 15, 2024

danwinship commented Feb 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjhaid commented Feb 15, 2024

aojea commented Feb 15, 2024

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 15, 2024

robscott commented Feb 15, 2024