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.

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

fix 'pod' in kubelet prober metrics #118549

Merged
merged 1 commit into from Jul 27, 2023

Conversation

a7i
Copy link
Contributor

@a7i a7i commented Jun 7, 2023

What type of PR is this?

/kind bug
/kind regression

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #115766

Special notes for your reviewer:

Kubernetes 1.25 Metric:

prober_probe_total{container="app-nginx",namespace="app",
pod="app-nginx",pod_uid="2de634c5-963f-4ffb-9b15-700f3f9b8bf3",
probe_type="Readiness",result="successful"
} 794

Kubernetes 1.24 Metric:

prober_probe_total{container="app-nginx",namespace="app",
pod="app-nginx-799b74869b-mmhr7",pod_uid="4550420b-6a0c-4d59-a486-f5b3a69ebdf8",
probe_type="Readiness",result="successful"
} 81023

This breaking change causes the metrics to not get processed by datadog core which relies on actual pod name.

Does this PR introduce a user-facing change?

Yes, but it reverts to initial functionality.

Fixes a 1.25 regression to revert kubelet prober metrics `pod` tag to include actual pod name

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


/cc @jackfrancis @SergeyKanzhelev from discussion here

@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. kind/regression Categorizes issue or PR as related to a regression from a prior release. 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. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 7, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @a7i. 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.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jun 7, 2023
@k8s-ci-robot k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 7, 2023
@k8s-ci-robot
Copy link
Contributor

@rajeeshckr: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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.

@bart0sh bart0sh added this to Triage in SIG Node PR Triage Jun 8, 2023
@bart0sh
Copy link
Contributor

bart0sh commented Jun 8, 2023

/triage accepted
/priority important-soon
/ok-to-test

@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. triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 8, 2023
@bart0sh bart0sh moved this from Triage to Needs Reviewer in SIG Node PR Triage Jun 8, 2023
@a7i
Copy link
Contributor Author

a7i commented Jun 8, 2023

/retest-required

@mindw
Copy link

mindw commented Jul 1, 2023

Please consider back porting this fix to supported releases - 1.25+. Thanks!

@dosmanak
Copy link

Hi, thank you for making this simple revert. I wonder why the issue has not been yet targeted by the core team as it seems clear.
I hope in soon merge in the main and possible backport in current release.

@derekwaynecarr
Copy link
Member

I think we should take the revert and return to prior behavior given the regression.

Would like @SergeyKanzhelev to chime in as well since he reviewed prior PR.

/lgtm

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

LGTM label has been added.

Git tree hash: e3edc4be51d2d79b817e50e33f20882a4cc59f75

@derekwaynecarr
Copy link
Member

pending review from a few others.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 26, 2023
@dims
Copy link
Member

dims commented Jul 26, 2023

/milestone v1.28

@k8s-ci-robot k8s-ci-robot added this to the v1.28 milestone Jul 26, 2023
@dims
Copy link
Member

dims commented Jul 26, 2023

/milestone v1.28

Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

/lgtm

The concern was the cardinality explosion as pod name has replica's random part. However we are not making things worse exponentially - we already had a metric for success.

I think the right long term fix will be having this logic as a default, but also allow to override the app name in those metrics based on some attributes

@dims
Copy link
Member

dims commented Jul 27, 2023

/approve
/lgtm

per discussion on slack and here, we need to get this in.

@dims
Copy link
Member

dims commented Jul 27, 2023

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 27, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a7i, dims, SergeyKanzhelev

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 27, 2023
@k8s-ci-robot k8s-ci-robot merged commit d4fde1e into kubernetes:master Jul 27, 2023
12 checks passed
SIG Node PR Triage automation moved this from Needs Reviewer to Done Jul 27, 2023
@a7i a7i deleted the kubelet-prober-metric-pod branch July 27, 2023 01:39
k8s-ci-robot added a commit that referenced this pull request Jul 28, 2023
…upstream-release-1.25

Automated cherry pick of #118549: fix 'pod' in kubelet prober metrics
k8s-ci-robot added a commit that referenced this pull request Jul 28, 2023
…upstream-release-1.26

Automated cherry pick of #118549: fix 'pod' in kubelet prober metrics
k8s-ci-robot added a commit that referenced this pull request Jul 28, 2023
…upstream-release-1.27

Automated cherry pick of #118549: fix 'pod' in kubelet prober metrics
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet 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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Metric prober_probe_total reports invalid pod field
8 participants