Allow Lease metrics to be exported across all namespaces #1845

lantingchiang · 2022-09-27T21:26:52Z

What this PR does / why we need it: This PR modifies the Lease store's createLeaseListWatch() method so that the metrics for the Lease custom resource to be exported across all namespaces, as opposed to only the kube-node-lease namespace. This allows us to get metrics on Lease resources used by pods as well.

How does this change affect the cardinality of KSM: increases cardinality by the number of namespaces

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

linux-foundation-easycla · 2022-09-27T21:26:56Z

The committers listed above are authorized under a signed CLA.

✅ login: lantingchiang / name: Lanting Chiang (8f883f0)

k8s-ci-robot · 2022-09-27T21:27:00Z

Welcome @lantingchiang!

It looks like this is your first PR to kubernetes/kube-state-metrics 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kube-state-metrics has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

fpetkovski · 2022-09-28T05:27:35Z

Thanks for this PR. The change makes sense to me, but I am just curious why KSM was exporting lease metrics from that single namespace only.

/lgtm
/hold

fpetkovski · 2022-09-28T05:31:33Z

Looking at the tests, we don't expose the namespace yet: https://github.com/kubernetes/kube-state-metrics/blob/master/internal/store/lease_test.go#L57-L58

We should update the lease store to export the fields that you added to the docs, and update tests accordingly.

lantingchiang · 2022-09-28T21:23:12Z

Thanks for this PR. The change makes sense to me, but I am just curious why KSM was exporting lease metrics from that single namespace only.

I think it's because that namespace contains all node leases, so it was sufficient if pod leases weren't of interest.

fpetkovski

Thanks

/lgtm

lantingchiang · 2022-10-10T17:13:51Z

Hi @dgrisonnet, could I get a review please? Thanks!

lantingchiang · 2022-10-11T21:57:22Z

Exported Lease.Spec.HolderIdentity field because some pod leases don't have a metadata.ownerReferences field.

fpetkovski · 2022-10-12T07:34:39Z

internal/store/lease.go


 				owners := l.GetOwnerReferences()
 				if len(owners) == 0 {
 					return &metric.Family{
 						Metrics: []*metric.Metric{
 							{
 								LabelKeys:   labelKeys,
-								LabelValues: []string{"<none>", "<none>"},
+								LabelValues: []string{"<none>", "<none>", l.Namespace, *l.Spec.HolderIdentity},


Can *l.Spec.HolderIdentity be nil in some cases?

Yes HolderIdentity is optional. Thanks for catching this!

fpetkovski · 2022-10-12T14:13:08Z

internal/store/lease.go

+				var holder string
+				if l.Spec.HolderIdentity == nil {
+					holder = "<none>"
+				} else {
+					holder = *l.Spec.HolderIdentity
+				}
+


Suggested change

var holder string

if l.Spec.HolderIdentity == nil {

holder = "<none>"

} else {

holder = *l.Spec.HolderIdentity

}

var holder string

if l.Spec.HolderIdentity != nil {

holder = *l.Spec.HolderIdentity

}

fpetkovski

Just one small nit, otherwise looks good

fpetkovski · 2022-10-12T14:56:43Z

internal/store/lease.go

-				labelKeys := []string{"owner_kind", "owner_name"}
+				labelKeys := []string{"owner_kind", "owner_name", "namespace", "lease_holder"}
+
+				holder = "<none>"


Sorry if I was not very specific here, but I don't think we use this placeholder anywhere else, so we should not introduce it. It's fine to leave the holder label empty when there is no holder.

Got it! I was trying to match the owner_name and owner_kind values when they're empty, but thanks for the clarification!

fpetkovski · 2022-10-13T15:48:34Z

Thank you for the contribution!

/lgtm

dgrisonnet

/lgtm
/approve
/unhold

k8s-ci-robot · 2022-10-14T09:40:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgrisonnet, fpetkovski, lantingchiang

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:

~~OWNERS~~ [dgrisonnet,fpetkovski]

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

Allow Lease metrics to be exported across all namespaces

8f883f0

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Sep 27, 2022

k8s-ci-robot requested review from dgrisonnet and fpetkovski September 27, 2022 21:27

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 27, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2022

k8s-ci-robot assigned fpetkovski Sep 28, 2022

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 28, 2022

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 28, 2022

update lease store to export namespace

5464a9d

lantingchiang force-pushed the lease branch from 39b3045 to 5464a9d Compare September 28, 2022 15:07

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 28, 2022

fpetkovski reviewed Sep 29, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2022

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2022

lantingchiang force-pushed the lease branch from 109beeb to 8cb0890 Compare October 11, 2022 22:13

fpetkovski reviewed Oct 12, 2022

View reviewed changes

export Lease.Spec.HolderIdentity

ceaad61

lantingchiang force-pushed the lease branch from 8cb0890 to ceaad61 Compare October 12, 2022 13:52

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 12, 2022

fpetkovski reviewed Oct 12, 2022

View reviewed changes

lantingchiang force-pushed the lease branch from bfc67ce to c48de69 Compare October 12, 2022 14:54

fpetkovski reviewed Oct 12, 2022

View reviewed changes

lantingchiang force-pushed the lease branch from c48de69 to d1f18ff Compare October 13, 2022 15:15

account for nil holderIdentity case

29e043e

lantingchiang force-pushed the lease branch from d1f18ff to 29e043e Compare October 13, 2022 15:31

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 13, 2022

dgrisonnet approved these changes Oct 14, 2022

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2022

k8s-ci-robot assigned dgrisonnet Oct 14, 2022

k8s-ci-robot merged commit cb5f88b into kubernetes:master Oct 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Lease metrics to be exported across all namespaces #1845

Allow Lease metrics to be exported across all namespaces #1845

lantingchiang commented Sep 27, 2022

linux-foundation-easycla bot commented Sep 27, 2022 •

edited

Loading

k8s-ci-robot commented Sep 27, 2022

fpetkovski commented Sep 28, 2022

fpetkovski commented Sep 28, 2022

lantingchiang commented Sep 28, 2022

fpetkovski left a comment

lantingchiang commented Oct 10, 2022

lantingchiang commented Oct 11, 2022

fpetkovski Oct 12, 2022

lantingchiang Oct 12, 2022

fpetkovski Oct 12, 2022

fpetkovski left a comment

fpetkovski Oct 12, 2022

lantingchiang Oct 13, 2022

fpetkovski commented Oct 13, 2022

dgrisonnet left a comment

k8s-ci-robot commented Oct 14, 2022

Allow Lease metrics to be exported across all namespaces #1845

Allow Lease metrics to be exported across all namespaces #1845

Conversation

lantingchiang commented Sep 27, 2022

linux-foundation-easycla bot commented Sep 27, 2022 • edited Loading

k8s-ci-robot commented Sep 27, 2022

fpetkovski commented Sep 28, 2022

fpetkovski commented Sep 28, 2022

lantingchiang commented Sep 28, 2022

fpetkovski left a comment

Choose a reason for hiding this comment

lantingchiang commented Oct 10, 2022

lantingchiang commented Oct 11, 2022

fpetkovski Oct 12, 2022

Choose a reason for hiding this comment

lantingchiang Oct 12, 2022

Choose a reason for hiding this comment

fpetkovski Oct 12, 2022

Choose a reason for hiding this comment

fpetkovski left a comment

Choose a reason for hiding this comment

fpetkovski Oct 12, 2022

Choose a reason for hiding this comment

lantingchiang Oct 13, 2022

Choose a reason for hiding this comment

fpetkovski commented Oct 13, 2022

dgrisonnet left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 14, 2022

linux-foundation-easycla bot commented Sep 27, 2022 •

edited

Loading