feat: add support for label generation for ephemeral volumes in pod#2891
feat: add support for label generation for ephemeral volumes in pod#2891eminaktas wants to merge 1 commit intokubernetes:mainfrom
Conversation
|
This issue is currently awaiting triage. If kube-state-metrics contributors determine this is a relevant issue, they will accept it by applying the The DetailsInstructions 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. |
|
Welcome @eminaktas! |
|
Thanks for your contributions! Other than the inline comment, you'd also need to update the docs here: docs/metrics/workload/pod-metrics.md |
fe39dfb to
affb934
Compare
|
Thinking about this a bit more, today these metrics are dropped for pods with ephemeral storage. So this change would mean those metrics will now start flowing in, so there would be a slight increase in metric volume (not necessarily cardinality). We should call that out in cardinality section of the PR description. |
affb934 to
722f5d2
Compare
722f5d2 to
1179498
Compare
1179498 to
71477d1
Compare
|
Hi @nmn3m @mrueg @dgrisonnet — could you please take a look at this PR when you get a chance? Thanks! |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bhope, eminaktas The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
Adds support for reporting PVC names for Pod ephemeral volumes through the existing kube_pod_spec_volumes_persistentvolumeclaims_* metrics, aligning exported labels with Kubernetes’ deterministic PVC naming for ephemeral volumes.
Changes:
- Extend
kube_pod_spec_volumes_persistentvolumeclaims_info(and_readonly) to emit series forspec.volumes[].ephemeral. - Update unit/integration tests to expect the new help text and the additional ephemeral-volume series.
- Update Pod metrics documentation to reflect the expanded metric scope.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
internal/store/pod.go |
Emits persistentvolumeclaim=<pod>-<volume> labels for ephemeral volumes in the PVC volume metrics. |
internal/store/pod_test.go |
Adds test coverage asserting metrics include an ephemeral volume and derived PVC name. |
pkg/app/server_test.go |
Updates expected HELP text for the affected metrics in the full scrape cycle test. |
docs/metrics/workload/pod-metrics.md |
Updates metric descriptions to mention ephemeral volumes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
71477d1 to
5b84233
Compare
|
New changes are detected. LGTM label has been removed. |
5b84233 to
f59ae4f
Compare
bhope
left a comment
There was a problem hiding this comment.
Overall looks good. Let's add the documentation in the metric help string for 0 value to avoid user misinterpretation.
Signed-off-by: Emin Aktas <eminaktas34@gmail.com>
f59ae4f to
f8d87a8
Compare
TY. Updated doc for the metric helper. |
| } else if v.Ephemeral != nil { | ||
| ms = append(ms, &metric.Metric{ | ||
| LabelKeys: []string{"volume", "persistentvolumeclaim"}, | ||
| LabelValues: []string{ephemeral.VolumeClaimName(p, &v)}, |
There was a problem hiding this comment.
Sorry, just realized, this may panic at metrics writer (before tests) as it would expect two labels.
| LabelValues: []string{ephemeral.VolumeClaimName(p, &v)}, | |
| LabelValues: []string{v.Name, ephemeral.VolumeClaimName(p, &v)}, |
| } else if v.Ephemeral != nil { | ||
| ms = append(ms, &metric.Metric{ | ||
| LabelKeys: []string{"volume", "persistentvolumeclaim"}, | ||
| LabelValues: []string{ephemeral.VolumeClaimName(p, &v)}, |
There was a problem hiding this comment.
Same as above
| LabelValues: []string{ephemeral.VolumeClaimName(p, &v)}, | |
| LabelValues: []string{v.Name, ephemeral.VolumeClaimName(p, &v)}, |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else if v.Ephemeral != nil { | ||
| ms = append(ms, &metric.Metric{ | ||
| LabelKeys: []string{"volume", "persistentvolumeclaim"}, | ||
| LabelValues: []string{ephemeral.VolumeClaimName(p, &v)}, |
There was a problem hiding this comment.
Same issue for the *_readonly metric: LabelKeys contains ("volume", "persistentvolumeclaim") but LabelValues only supplies one string for ephemeral volumes, which will panic when encoding metrics. Include both label values in the correct order.
| LabelValues: []string{ephemeral.VolumeClaimName(p, &v)}, | |
| LabelValues: []string{v.Name, ephemeral.VolumeClaimName(p, &v)}, |
Add support for generating the persistentvolumeclaim label for ephemeral volumes in pod volume metrics.
Kubernetes creates PVCs for ephemeral volumes using the naming convention -. This change aligns kube-state-metrics with the upstream implementation to ensure the generated label matches the actual PVC name created by Kubernetes.
Upstream references:
Ephemeral controller PVC creation:
https://github.com/kubernetes/kubernetes/blob/21b427c29960dac4e362d6a9f92817d0b01d78ec/pkg/controller/volume/ephemeral/controller.go#L261
PVC naming helper:
https://github.com/kubernetes/component-helpers/blob/b20f683efd864cebf95ec4a62f4c271cc69b9571/storage/ephemeral/ephemeral.go#L41-L43
Fixes #2490
What this PR does / why we need it: Add support for exposing ephemeral PVCs in
kube_pod_spec_volumes_persistentvolumeclaims_info.How does this change affect the cardinality of KSM: increases
Which issue(s) this PR fixes: Fixes #2490