Extend kube-state-metrics to support Custom Resource metrics #1644

Garrybest · 2021-11-29T12:00:43Z

What this PR does / why we need it:
Now kube-state-metrics does not support Kubernetes Custom Resource metrics. According to #1640, I try to make kube-state-metrics to be more flexible to support Custom Resource. Users only need to:

Depend this repo in their own project.
Implement the new interface customresource.RegistryFactory of their Custom Resources as a plugin factory to tell kube-state-metrics how to display custom resource metrics.
Register the factories as a parameter by calling RunKubeStateMetrics.

The inspiration is from kube-scheduler out-of-tree plugin registry. The well-know project scheduler-plugin is based on this cool function. It adds new scheduler plugins without modifying any source codes in kube-scheduler.

I have taken Custom Resource samplev1alpha1.Foo in samplecontroller as an example in the interface comments and unit test.

How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
It does not directly change or affect the cardinality of existing metrics.

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 #1640

k8s-ci-robot · 2021-11-29T12:00:51Z

Welcome @Garrybest!

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. 😃

Garrybest · 2021-11-29T12:11:00Z

The PR has been divided into 4 parts.

Design a Custom Resource RegistryFactory for out-of-tree implements. Anyone who wants to extend kube-state-metrics should implement this interface to tell us how to get and display their metrics.
Reform Builder to adapt custom resource registry. Now the Builder could register custom resource metric stores.
Reform main function to adapt custom resource registry. Users could not depend main pkg, so I move the codes into a new pkg app and call the custom resource register functions for Builder.
Add a custom resource unit test based on foo object in samplecontroller. The obj samplev1alpha1.Foo could be a nice example so I add k8s.io/sample-controller in go.mod and make a best practice in server_test.go.

Garrybest · 2021-11-29T12:12:56Z

/cc @fpetkovski

Serializator · 2021-11-29T12:13:03Z

internal/store/builder.go

+// WithGenerateCustomResourceStoresFunc configures a custom generate custom resource store function
+func (b *Builder) WithGenerateCustomResourceStoresFunc(f ksmtypes.BuildCustomResourceStoresFunc, u bool) {
+	b.buildCustomResourceStoresFunc = f
+	b.useAPIServerCache = u


b.useAPIServerCache is used by (store.*Builder).WithGenerateStoresFunc as well, what would happen in the following scenario?

store := ... store.WithGenerateStoresFunc(..., false) store.WithGenerateCustomResourceStoresFunc(..., true)

(store.*Builder).useAPIServerCache is implicitly overwritten by the invocation of (store.*Builder).useAPIServerCache though the expectation could be that they both keep a separate property / variable for this (e.g. (store.*Builder).useAPIServerCacheForCustomResources).

It could also be that (store.*Builder).useAPIServerCache should not be too tightly coupled with the function and thus needs to be configured separately by adding an extra method to do so.

Or this might be an indication the custom resources like this deserve their own builder (instance), this way you wouldn't need to add respective methods for custom resources to the existing builder but create two separate builders and combine the stores later on.

Thanks @Serializator for reminding! I have not even noticed that. I prefer this solution.

It could also be that (store.*Builder).useAPIServerCache should not be too tightly coupled with the function and thus needs to be configured separately by adding an extra method to do so.

I agree when looking specifically at (store.*Builder).useAPIServerCache but I'd like to "widen" the "scope" of my initial feedback and look at the bigger picture in terms of extensibility of store.Builder itself.

I haven't looked through the issue or pull request completely so please correct me if I'm wrong! CR metrics are basically metrics like the built-in metrics by KSM but registrered a different way.

Why did you make the choice to extend the existing store.Builder rather than create a separate instance of it specifically for registering and working with CR metrics, and later on make some kind of "composite" store which combines the built-in and the CR metrics?

I'm asking in terms of future extensability. If there is ever a need to add an additional way of registering metrics then methods will again be added to the store.Builder to allow for it ("extending" the store.Builder) rather than "building upon it" and keepign the implementation of store.Builder light and clean.

I'd love to hear your point of view and opinion on this! I might be completely wrong 😉

Hi @Serializator, it's really a good question.

I choose to extend store.Builder because IMO actually the Custom Resource is very similar to Kubernetes built-in resource. The only thing different is that we don't know their type, what their metrics look like, how to list and watch them(the client code is generated by code-generator). Once people tell us the necessary information, it behaves like a common built-in resource. It could be built and stored similarly, shared the same logic with store.Builder.

Rather than create a separate instance of it specifically for registering and working with CR metrics, and later on make some kind of "composite" store which combines the built-in and the CR metrics

I think this solution is more complicated. As I mentioned, the CR and built-in resource could share the same building logic once we have the information in registry_factory.go. Actually, what I'm thinking is all metrics could be registered by this interface including built-in resource. From the aspect of extensability, the inner resource could be added by this method. Actually, I suppose removing the built-in resource registry from store.Builder and keep the CustomResourceStores may be more light and clean?

Just some personal idea, lol 😄

In that sense I understand and agree (not that you need my approval 😉) in terms of the extending the store.Builder and the why behind it.

(store.Builder).buildCustomResourceStores though is really similar to (store.Builder).buildStores, with the only difference being the "client" which is passed to the listWatchFunc. The crutch in this case is in the fact that (store.Builder).buildStores is aware of the client (k8s.io/client-go/kubernetes.Interface because (store.Builder).buildStores is responsible for calling the listWatchFunc function. Disappointing (😞 😆), maybe refactor in the future to abstract away the client (as this can differ now with the CR metrics).

Hi @Serializator. You are right! We have the same opinions😃. That also distresses me!

These two functions are so similar but unfortunately we have no ideas about what type the customResourceClient is. If we merge them together, all types of listWatchFunc in internal/store directory must be revised. This revision may be a little bit huge and does not focus on custom resource extension. So I decided to make a optimization in the future. That would be better for reviewing.

I have added a TODO comment. We could optimize it later😉.

pkg/customresource/registry_factory.go

internal/store/builder.go

fpetkovski

This is a great start, thanks for taking the initiative 👍

pkg/app/server.go

fpetkovski

Good stuff, just one small nit from my side

main.go

fpetkovski · 2021-12-08T14:08:03Z

Great work, /lgtm

/hold for others to take a look as well

chrischdi · 2021-12-09T14:07:53Z

First of all: thank you for working on this 👍 With this interface it is also possible to use the controller-runtime client :-)

I did take a look at it and have some feedback, but I think this could get fixed in future Pull Requests:

The flag --resources does not list the resource names of the CRs. Filtering for the CRs does work so its just about the help text.
Also in regards to that: maybe it would be handy if it is possible to disable the standard kube-state-metrics metric families by default (e.g. the main.go of kube-state-metrics could pass the core metrics to RunKubeStateMetrics the same way we can do for Custom Resources).

Edit: addition: it is possible to (copy and) re-define the flags and set --resources to custom values.

Signed-off-by: Garrybest <[email protected]>

Garrybest · 2021-12-10T07:57:28Z

Hi @chrischdi, I really agree with you.

Now the flag --resources only list all built-in resources. I don't make a big revision here because I try to focus on CR registery and interface definition in this PR. So I just add all CRs into enabledResources but do not print them by default.

It is a good idea for us to fix it in the future PR. Thanks @chrischdi for your kind reminds. 😄

chrischdi · 2021-12-10T11:28:49Z

Hi @chrischdi, I really agree with you.

Now the flag --resources only list all built-in resources. I don't make a big revision here because I try to focus on CR registery and interface definition in this PR. So I just add all CRs into enabledResources but do not print them by default.

It is a good idea for us to fix it in the future PR. Thanks @chrischdi for your kind reminds. smile

👍 yes I think that's totally fine.

I already found a workaround which I think is more than good enough:
https://github.com/chrischdi/cluster-api-state-metrics/blob/chrischdi/ksm-pr-adopt/main.go#L42

Garrybest · 2021-12-10T16:43:16Z

I already found a workaround which I think is more than good enough:
https://github.com/chrischdi/cluster-api-state-metrics/blob/chrischdi/ksm-pr-adopt/main.go#L42

Great👍, that's what I'm thinking before! See #1644 (comment). The built-in resource could also implement the registry interface and be passed to RunKubeStateMetrics as a parameter like what you did. And the problem we mentioned could also be solved.

Garrybest · 2021-12-14T03:15:04Z

Hi @brancz @dgrisonnet, would you please take a look? My appreciation. 😄

Signed-off-by: Garrybest <[email protected]>

Garrybest · 2021-12-15T03:59:51Z

internal/store/builder.go

+// WithCustomResourceStoreFactories returns configures a custom resource stores factory
+func (b *Builder) WithCustomResourceStoreFactories(fs ...customresource.RegistryFactory) {
+	for i := range fs {
+		f := fs[i]


Hi @chrischdi, I made a mistake here in the previous version. The f must be copied because the value of availableStores is a func. Otherwise, it will be registed the last ones when custom resources are more than one. I have fixed here, please pay attention. My apology.

fpetkovski · 2021-12-16T08:54:36Z

/lgtm

k8s-ci-robot · 2021-12-16T08:54:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fpetkovski, Garrybest

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~~ [fpetkovski]

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

Garrybest · 2021-12-24T01:23:15Z

Can we move forward?

fpetkovski · 2022-01-04T10:05:47Z

/hold cancel

Makes use of implementation at github.com/kubernetes/kube-state-metrics/pull/1644. Adds features of kube-state-metrics like sharding. Signed-off-by: Schlotter, Christian <[email protected]>

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 29, 2021

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 29, 2021

k8s-ci-robot requested review from brancz and dgrisonnet November 29, 2021 12:01

k8s-ci-robot requested a review from fpetkovski November 29, 2021 12:12

Serializator reviewed Nov 29, 2021

View reviewed changes

Garrybest force-pushed the pr_crd branch 2 times, most recently from 2ea2300 to 12822db Compare November 29, 2021 15:22

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2021

Garrybest force-pushed the pr_crd branch from 12822db to 4a2ebf0 Compare December 2, 2021 02:25

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2021

Serializator reviewed Dec 4, 2021

View reviewed changes

pkg/customresource/registry_factory.go Show resolved Hide resolved

Serializator reviewed Dec 4, 2021

View reviewed changes

internal/store/builder.go Outdated Show resolved Hide resolved

Serializator reviewed Dec 4, 2021

View reviewed changes

internal/store/builder.go Show resolved Hide resolved

Garrybest force-pushed the pr_crd branch 2 times, most recently from b536f14 to bab4161 Compare December 6, 2021 06:53

dgrisonnet mentioned this pull request Dec 6, 2021

Extend kube-state-metrics to support Custom Resource metrics #1640

Closed

Garrybest force-pushed the pr_crd branch from bab4161 to 0c6b32b Compare December 7, 2021 02:29

fpetkovski reviewed Dec 7, 2021

View reviewed changes

pkg/app/server.go Outdated Show resolved Hide resolved

Garrybest force-pushed the pr_crd branch from 0c6b32b to 15864be Compare December 7, 2021 14:09

fpetkovski reviewed Dec 8, 2021

View reviewed changes

main.go Outdated Show resolved Hide resolved

Garrybest force-pushed the pr_crd branch 2 times, most recently from 073a1d2 to 6f0d3e7 Compare December 8, 2021 09:09

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

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2021

Design a custom resource RegistryFactory for out-of-tree implements

577bf2e

Signed-off-by: Garrybest <[email protected]>

Garrybest force-pushed the pr_crd branch from 6f0d3e7 to 9990c3d Compare December 10, 2021 07:46

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 10, 2021

Garrybest added 3 commits December 14, 2021 21:31

Reform Builder to adapt custom resource registry

3ca06c2

Signed-off-by: Garrybest <[email protected]>

Reform main function to adapt custom resource registry

854bc7c

Signed-off-by: Garrybest <[email protected]>

Add a custom resource test based on foo object in samplecontroller

acd5e74

Signed-off-by: Garrybest <[email protected]>

Garrybest force-pushed the pr_crd branch from 9990c3d to acd5e74 Compare December 14, 2021 13:32

Garrybest commented Dec 15, 2021

View reviewed changes

k8s-ci-robot assigned fpetkovski Dec 16, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 16, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 16, 2021

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

k8s-ci-robot merged commit 081cc3c into kubernetes:master Jan 4, 2022

Garrybest deleted the pr_crd branch January 4, 2022 10:21

chrischdi mentioned this pull request Jan 4, 2022

Implement Factory to reduce custom code mercedes-benz/cluster-api-state-metrics#6

Merged

juliogreff mentioned this pull request May 31, 2022

Upgrade kube-state-metrics to 2.4.2 DataDog/datadog-agent#12216

Merged

9 tasks

This was referenced Feb 23, 2023

Reload CustomResourceState Config File on Change #1928

Merged

Incompatible changes about custom resource metrics #2013

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend kube-state-metrics to support Custom Resource metrics #1644

Extend kube-state-metrics to support Custom Resource metrics #1644

Garrybest commented Nov 29, 2021

k8s-ci-robot commented Nov 29, 2021

Garrybest commented Nov 29, 2021 •

edited

Loading

Garrybest commented Nov 29, 2021

Serializator Nov 29, 2021 •

edited

Loading

Garrybest Nov 29, 2021

Serializator Nov 29, 2021

Garrybest Nov 29, 2021

Serializator Dec 4, 2021

Garrybest Dec 5, 2021

Garrybest Dec 6, 2021

fpetkovski left a comment

fpetkovski left a comment

fpetkovski commented Dec 8, 2021

chrischdi commented Dec 9, 2021 •

edited

Loading

Garrybest commented Dec 10, 2021

chrischdi commented Dec 10, 2021

Garrybest commented Dec 10, 2021

Garrybest commented Dec 14, 2021

Garrybest Dec 15, 2021

fpetkovski commented Dec 16, 2021

k8s-ci-robot commented Dec 16, 2021

Garrybest commented Dec 24, 2021

fpetkovski commented Jan 4, 2022

Extend kube-state-metrics to support Custom Resource metrics #1644

Extend kube-state-metrics to support Custom Resource metrics #1644

Conversation

Garrybest commented Nov 29, 2021

k8s-ci-robot commented Nov 29, 2021

Garrybest commented Nov 29, 2021 • edited Loading

Garrybest commented Nov 29, 2021

Serializator Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

Garrybest Nov 29, 2021

Choose a reason for hiding this comment

Serializator Nov 29, 2021

Choose a reason for hiding this comment

Garrybest Nov 29, 2021

Choose a reason for hiding this comment

Serializator Dec 4, 2021

Choose a reason for hiding this comment

Garrybest Dec 5, 2021

Choose a reason for hiding this comment

Garrybest Dec 6, 2021

Choose a reason for hiding this comment

fpetkovski left a comment

Choose a reason for hiding this comment

fpetkovski left a comment

Choose a reason for hiding this comment

fpetkovski commented Dec 8, 2021

chrischdi commented Dec 9, 2021 • edited Loading

Garrybest commented Dec 10, 2021

chrischdi commented Dec 10, 2021

Garrybest commented Dec 10, 2021

Garrybest commented Dec 14, 2021

Garrybest Dec 15, 2021

Choose a reason for hiding this comment

fpetkovski commented Dec 16, 2021

k8s-ci-robot commented Dec 16, 2021

Garrybest commented Dec 24, 2021

fpetkovski commented Jan 4, 2022

Garrybest commented Nov 29, 2021 •

edited

Loading

Serializator Nov 29, 2021 •

edited

Loading

chrischdi commented Dec 9, 2021 •

edited

Loading