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

Don't increment "no local endpoints" metric when there are no remote endpoints #109782

Merged
merged 1 commit into from May 5, 2022
Merged

Don't increment "no local endpoints" metric when there are no remote endpoints #109782

merged 1 commit into from May 5, 2022

Conversation

danwinship
Copy link
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it:

The new "no local endpoints" metric gets incremented even for services with no endpoints at all. This seems wrong to me; there is a difference between "Service X with iTP:Local is missing endpoints on some nodes" vs "Service X with iTP:Local has been defined but its endpoints haven't been created yet".

OTOH, the metric seems kind of flaky to me anyway (eg, it is likely to be triggered while the service is initially being deployed, which doesn't seem useful). And the entire idea of the metric seems wrong for the "externalTrafficPolicy" case. In the iTP case, it is usually a bug if the service does not have endpoints on every node. In the eTP case, it would actually be quite surprising for a service to have endpoints on every node...

But anyway, this just fixes the "no local endpoints" vs "no endpoints at all" case... Assuming people agree that it's a bug, which you might not.

I noticed this while refactoring the code, where preserving the current behavior would require more code than implementing this "fixed" behavior.

/assign @andrewsykim @MaxRenaud

Which issue(s) this PR fixes:

none

Does this PR introduce a user-facing change?

The kube-proxy `sync_proxy_rules_no_endpoints_total` metric now only counts local-traffic-policy services which have remote endpoints but not local endpoints.

/sig network

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. sig/network Categorizes an issue or PR as relevant to SIG Network. 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. labels May 4, 2022
@k8s-ci-robot
Copy link
Contributor

@danwinship: 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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label May 4, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. area/ipvs labels May 4, 2022
@danwinship
Copy link
Contributor Author

bot missed this before...
/assign @andrewsykim @MaxRenaud

@aojea
Copy link
Member

aojea commented May 4, 2022

/test pull-kubernetes-verify

the verify error is legit, there is some gofmt problem

@aojea aojea mentioned this pull request May 4, 2022
Closed
@MaxRenaud
Copy link
Contributor

Hi Dan,
I agree that the metric should only be incremented when there are endpoints but they are not suitable for the policy. Thank you for fixing that.
I'll discuss the eTP metric with Andrew.
/test pull-kubernetes-verify
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 4, 2022
@aojea
Copy link
Member

aojea commented May 4, 2022

don't retry test pull-kubernetes-verify please , the failure is legit

The integration failure is https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/109782/pull-kubernetes-integration/1521837161488322560

#109783

@danwinship
Don't increment "no local endpoints" metric when there are no remote …
84ad54f 
…endpoints

A service having no _local_ endpoints when it does have remote
endpoints is different from a service having no endpoints at all.
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 4, 2022
@aojea
Copy link
Member

aojea commented May 5, 2022

/lgtm

/lgtm

carrying over the lgtm since only the gofmt problem was fixed

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 5, 2022
@k8s-ci-robot k8s-ci-robot merged commit 50e1f70 into kubernetes:master May 5, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone May 5, 2022
@danwinship danwinship deleted the no-local-endpoints-metric branch May 5, 2022 15:21
@github-actions github-actions bot mentioned this pull request May 11, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bowei bowei Awaiting requested review from bowei

@khenidak khenidak Awaiting requested review from khenidak

Assignees

@aojea aojea

@MaxRenaud MaxRenaud

@andrewsykim andrewsykim

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/ipvs 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. 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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.25
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@danwinship @k8s-ci-robot @aojea @MaxRenaud @andrewsykim