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

feat: Add local storage labels to kube_persistentvolume_info #1814

Merged
merged 3 commits into from
Aug 24, 2022

Conversation

nabokihms
Copy link
Member

Signed-off-by: m.nabokikh maksim.nabokikh@flant.com

What this PR does / why we need it:
Local volumes in Kubernetes differ from other volumes, e.g., they do not have the max cap as other persistent volumes. Instead, local PVs can consume all the available space on the disk. Thus, there should be a way to distinguish local PVs (and other rules to monitor them).

How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
Yes, it make local storage volumes metrics looks like kube_persistentvolume_info{local_path="/mnt/data", local_fs="ext4"}

Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 23, 2022
Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>
@nabokihms
Copy link
Member Author

Maybe it would be best to also add the host_path_ labels family, because these are also indicate local volumes.

@dgrisonnet
Copy link
Member

At this point, I wonder if it was really a good idea to add all the FS types to the _info metric. It is growing an insane amount of labels with makes it kind of unreadable. Not that metrics themselves should be readable but metrics are easier to discover than labels. Considering the amount of possible volumes sources (https://pkg.go.dev/k8s.io/api/core/v1#PersistentVolumeSource), it might just be better to create one metric per source.

@nabokihms
Copy link
Member Author

nabokihms commented Aug 23, 2022

The idea to add specific labels to a dedicated metric seems more than ok to me.

Something like:

kube_persistentvolume_info{type="csi", persistentvolume="test-1"}
kube_persistentvolume_csi_info{persistentvolume="test-1", driver="ABC", volume_handle="csi-volume-handle"}
kube_persistentvolume_info{type="local", persistentvolume="test-2"}
kube_persistentvolume_local_info{persistentvolume="test-2", path="/mnt", fs="ext4"}

@dgrisonnet Does it seem ok to you? If so, I can dig to implement it more generic way.

Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 24, 2022
@nabokihms
Copy link
Member Author

I reverted table changes and add host path labels.

@nabokihms nabokihms requested review from dgrisonnet and removed request for fpetkovski August 24, 2022 07:18
@dgrisonnet
Copy link
Member

LGTM, thank you @nabokihms for the contribution :)

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgrisonnet, nabokihms

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 Aug 24, 2022
@dgrisonnet
Copy link
Member

Yeah your suggestion is pretty similar to what I had in mind, I'll start a github discussion around that topic.

@k8s-ci-robot k8s-ci-robot merged commit ebc0e18 into kubernetes:master Aug 24, 2022
@dgrisonnet dgrisonnet mentioned this pull request Aug 24, 2022
@nabokihms
Copy link
Member Author

@dgrisonnet I'd like to participate, if it is possible.
😄

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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants