Skip to content

Add kubelet_pod_emptydir_volume_{used,size_limit}_bytes Prometheus metrics #121489

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

machine424
Copy link
Contributor

@machine424 machine424 commented Oct 24, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

Exposes emptyDir usage and size limit per Pod as Prometheus metrics

Which issue(s) this PR fixes:

Fixes #69507

Special notes for your reviewer:

  • Do we need to expose thecapacityBytes as well (I don't think it's really informative)? What about cases when emptyDir.sizeLimit is set? DONE => for the capacity, maybe in a follow-up PR if it turns out relevant.
  • Could unit test more cases.
  • Linked the new collector in pkg/kubelet/kubelet.go, not sure if it's sufficient, any doc about that is welcome.

Does this PR introduce a user-facing change?

Add kubelet_pod_emptydir_volume_{used,size_limit}_bytes Prometheus metrics to expose the used and limit bytes for volumes on the default medium, per Pod. 

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


@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. 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 Oct 24, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @machine424. 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 Oct 24, 2023
podNamespace := podStat.PodRef.Namespace

if podStat.VolumeStats == nil {
// TODO: another level?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

WDYT

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

/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 Oct 24, 2023
@machine424
Copy link
Contributor Author

/cc @dgrisonnet

@kannon92
Copy link
Contributor

/ok-to-test
/release-note-edit Add kubelet_pod_emptydir_volume_used_bytes Prometheus metric to expose emptyDir used bytes Per Pod

@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. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 24, 2023
@machine424
Copy link
Contributor Author

I fixed the duplicate metrics collector registration attempted, it was an unfortunate copy/past.

For future readers:

printing the conflicting collectors at /vendor/github.com/prometheus/client_golang/prometheus/registry.go is helpful (I don't know why we don't expose them by default)

 func (err AlreadyRegisteredError) Error() string {
-       return "duplicate metrics collector registration attempted"
+       return fmt.Sprintf("duplicate metrics collector registration attempted %+v %+v", err.ExistingCollector, err.NewCollector)
 }

@machine424
Copy link
Contributor Author

/retest-required

@machine424 machine424 changed the title Add kubelet_pod_emptydir_volume_{used,size_limit_bytes}_bytes Prometheus metrics Add kubelet_pod_emptydir_volume_{used,size_limit}_bytes Prometheus metrics Aug 28, 2024
@kannon92
Copy link
Contributor

/release-note-edit

Add kubelet_pod_emptydir_volume_{used,size_limit_bytes}_bytes Prometheus metrics to expose emptyDir used and limit bytes per Pod

@k8s-ci-robot
Copy link
Contributor

@kannon92: /release-note-edit must be used with a release note block.

In response to this:

/release-note-edit

Add kubelet_pod_emptydir_volume_{used,size_limit_bytes}_bytes Prometheus metrics to expose emptyDir used and limit bytes per Pod

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-sigs/prow repository.

Copy link
Contributor

@kannon92 kannon92 left a comment

Choose a reason for hiding this comment

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

Do we want to distinguish between memory backed volumes and normal empty dir?

@kannon92
Copy link
Contributor

/release-note-edit

Add kubelet_pod_emptydir_volume_{used,size_limit_bytes}_bytes Prometheus metrics to expose emptyDir used and limit bytes per Pod

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 28, 2024
@kannon92
Copy link
Contributor

btw @machine424 you can always edit your PR in the release-notes section to add a release-note. I had to remember the syntax to do it from the PR..

…e limit

per pod/volume.

Only volumes on the default medium are considered.

Signed-off-by: machine424 <ayoubmrini424@gmail.com>
@machine424
Copy link
Contributor Author

Do we want to distinguish between memory backed volumes and normal empty dir?

This could be useful, but I’ll stick to volumes on the default medium for now (I just added that explicitly to the metrics help text). Given that some maintainers already reviewed/started reviewing, I prefer not to make any major changes.

Also, given how we’re very cautious about cardinality, I wouldn’t add another dimension now (especially since StorageMediumHugePagesPrefix can be unbound). I’d leave that for a follow-up PR if needed.

@mrueg
Copy link
Member

mrueg commented Oct 8, 2024

Any chance to get this into the next kubernetes release? I think those metrics would be very useful.

@dgrisonnet
Copy link
Member

I'll bring it up to the next sig meeting

@dgrisonnet
Copy link
Member

Coming back from the SIG meeting, the group was in favor of this change and approved the cardinality of these metrics. I am assigning @dashpole for instrumentation and kubelet review.

/unassign @logicalhan
/assign @dashpole


for _, volumeStat := range podStat.VolumeStats {
if volume, found := podVolumes[volumeStat.Name]; found {
// Only consider volumes on the default medium.
Copy link
Contributor

Choose a reason for hiding this comment

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

I see there is already a VolumeStatsUsedBytesKey for Persistent volumes. Are emptyDir volumes the only volumes that are missing metrics today?

Copy link
Contributor Author

@machine424 machine424 Jan 3, 2025

Choose a reason for hiding this comment

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

Not the only ones, many other volumes don't have metrics.
I suggested in #121489 (comment) to deprecate the the existing per source metrics and have a generic metrics (with the sources as metrics), that will be easier to discover and to extend, but that effort will need more effort from maintainers.

for _, volumeStat := range podStat.VolumeStats {
if volume, found := podVolumes[volumeStat.Name]; found {
// Only consider volumes on the default medium.
if volume.EmptyDir != nil && volume.EmptyDir.Medium == v1.StorageMediumDefault {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be useful for medium to be a label? It seems like memory-backed empty-dir usage and limits would also be useful, since we already have computed that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@machine424
Copy link
Contributor Author

Unfortunately, this has taken much longer than expected (not blaming anyone, I get that it’s not a top priority for everyone). It's been harder for me to dive back into the PR, and it feels like we’re spinning our wheels.

I won't be able to keep working on this without compromising on the quality.

If you think this isn't ready yet, I’m okay with closing the PR and letting someone else take over, maybe deprecate the existing metrics and switch to a generic metric, as mentioned in #121489 (comment) is the way to go.

@rexagod
Copy link
Member

rexagod commented Feb 6, 2025

/assign @dgrisonnet

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 7, 2025
@frittentheke
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 8, 2025
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 6, 2025
@frittentheke
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/feature Categorizes issue or PR as related to a new feature. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Development

Successfully merging this pull request may close these issues.

Exposing ephemeral storage metrics to prometheus