Add sync_proxy_rules_no_endpoints_total metric #108930
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
release-note
|
/test all |
k8s-ci-robot
added
area/ipvs
sig/instrumentation
|
/assign andrewsykim |
MaxRenaud
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
andrewsykim
reviewed
Mar 23, 2022
pkg/proxy/ipvs/proxier.go
Outdated
| localNoEndpoints int64 | ||
| // externalNoEndpoints represents the number of rules that couldn't be applied due to | ||
| // the absence of any endpoints. | ||
| externalNoEndpoints int64 |
There was a problem hiding this comment.
nit: I would name this clusterNoEndpoints instead
pkg/proxy/iptables/proxier.go
Outdated
| @@ -1327,6 +1329,11 @@ func (proxier *Proxier) syncProxyRules() { | |||
| } | |||
|
|
|||
| if !hasEndpoints { | |||
| if svc.NodeLocalInternal() { | |||
There was a problem hiding this comment.
FYI there may be some changes need after #106497 is merged. I don't expect the conflict to be big but just a heads up
andrewsykim
reviewed
Mar 23, 2022
pkg/proxy/ipvs/proxier.go
Outdated
| @@ -273,6 +273,12 @@ type Proxier struct { | |||
| // Inject for test purpose. | |||
| networkInterfacer utilproxy.NetworkInterfacer | |||
| gracefuldeleteManager *GracefulTerminationManager | |||
| // localNoEndpoints represents the number of rules that couldn't be applied due to | |||
| // the absence of local endpoints. | |||
There was a problem hiding this comment.
a comment reference the metric this is used for would be helpful, same for clusterNoEndpoints
pkg/proxy/iptables/proxier.go
Outdated
| @@ -967,6 +967,8 @@ func (proxier *Proxier) syncProxyRules() { | |||
| } | |||
| } | |||
|
|
|||
| serviceNoEndpointsTotalInternal := 0 | |||
There was a problem hiding this comment.
A comment here for the metric this is used for would be helpful
|
/retest unrelated |
|
@MaxRenaud: The
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/assign @dgrisonnet |
k8s-ci-robot
removed
the
lgtm
Done |
danwinship
reviewed
Mar 30, 2022
pkg/proxy/iptables/proxier.go
Outdated
Comment on lines
1374
to
1373
| args = append(args[:0], | ||
| "-A", string(policyLocalChain), | ||
| "-m", "comment", "--comment", | ||
| fmt.Sprintf(`"%s has no local endpoints"`, svcNameString), | ||
| "-j", | ||
| string(KubeMarkDropChain), | ||
| ) | ||
| proxier.natRules.Write(args) |
There was a problem hiding this comment.
hm... that use of args is unnecessary with the current code...
Suggested change
| args = append(args[:0], | |
| "-A", string(policyLocalChain), | |
| "-m", "comment", "--comment", | |
| fmt.Sprintf(`"%s has no local endpoints"`, svcNameString), | |
| "-j", | |
| string(KubeMarkDropChain), | |
| ) | |
| proxier.natRules.Write(args) | |
| proxier.natRules.Write( | |
| "-A", string(policyLocalChain), | |
| "-m", "comment", "--comment", | |
| fmt.Sprintf(`"%s has no local endpoints"`, svcNameString), | |
| "-j", | |
| string(KubeMarkDropChain), | |
| ) |
There was a problem hiding this comment.
I have just moved the code. Is it OK if we keep it as is for this PR? I'm getting an exception since we are past the freeze and my justification for the low risk is that we are simply adding a metric.
There was a problem hiding this comment.
this will tickle a rebase on my PR which touches the args usage, too, I think
| {"10.0.1.3", "host2"}, | ||
| }, | ||
| expectedSyncProxyRulesNoLocalEndpointsTotalInternal: 1, | ||
| expectedSyncProxyRulesNoLocalEndpointsTotalExternal: 1, |
There was a problem hiding this comment.
would be good to have some test where the metric is more than 1
There was a problem hiding this comment.
Can we do this as a quick follow-up? It involves refactoring the test to include dynamically generating services. Since we're after the freeze, I'd like to get this one in
|
I think this is OK, even if some minor fixups would be nice. /lgtm |
k8s-ci-robot
added
lgtm
k8s-ci-robot
removed
lgtm
|
/milestone v1.24 (exception for this PR was approved) |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/retest |
|
@MaxRenaud: The following tests failed, say
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. |
|
This is ready to merge. It's been rebased, modified to fit with the conflicts, tested, and all comments were addressed. The absolute latest this can be merged is "01:00 UTC Saturday 2nd April 2022" in order to make it into 1.24. That translates to 18:00 PDT or 21:00 EDT today. |
k8s-ci-robot
added
the
lgtm
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind feature
What this PR does / why we need it: Adds a metric when a service with a local traffic_policy has no available endpoints. The metric has 2 labels:
Which issue(s) this PR fixes:
Fixes #2086
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: