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 CSI volume attributes to PV info metric #98

Conversation

machadovilaca
Copy link
Member

@machadovilaca machadovilaca commented Oct 10, 2023

What this PR does / why we need it:

This PR adds a new csi_mounter and csi_map_options for CSI PV in kube_persistentvolume_info metric, to track the mounter used by the volume, and the respective map options configured

How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)

  • adds 2 new labels for kube_persistentvolume_info
  • but these label values should be static for most of the lifetime of a volume, therefore, since we already have the volume name in the metric, metric total cardinality is not expected to increase

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 #

Signed-off-by: João Vilaça <jvilaca@redhat.com>
@openshift-ci
Copy link

openshift-ci bot commented Oct 10, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: machadovilaca
Once this PR has been reviewed and has the lgtm label, please assign dgrisonnet for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@simonpasquier
Copy link

👋 any reason why it can't be submitted upstream first? In general, we don't carry downstream patches.
cc @rexagod

@openshift-ci
Copy link

openshift-ci bot commented Oct 10, 2023

@machadovilaca: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-agnostic-cmo 56eebc1 link true /test e2e-agnostic-cmo

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@machadovilaca
Copy link
Member Author

@simonpasquier I opened kubernetes#2133, but the opinion was that these are very CSI drivers specific and would be hard to add them there

@rexagod
Copy link
Member

rexagod commented Oct 17, 2023

Hey @machadovilaca, we don't really merge any "out-of-tree" changes downstream, since that would diverge and open us on unpredictable front given how the areas affecting such a patch will change upstream from one release to another. IMO getting this in upstream would be the way to go, using --metric-opt-in-list as you've commented.

@simonpasquier
Copy link

IIUC --opt-in-metrics-list is different because the goal is to enable optional metrics while the change is about adding/enabling new labels to an already existing metric.

@rexagod
Copy link
Member

rexagod commented Oct 19, 2023

ACK, I was under the impression that since adding these labels to the existing metric is getting some pushback, we could possibly introduce a new metric that folks can opt-in to. However, I see that this could open us (upstream) up for merging one-off metrics for similar use-cases, and thus will need to be evaluated on a case-by-case basis giving priority to the fact that even the hidden metrics are widely beneficial, if not as much as the ones enabled by default. An --opt-in-labels-list flag might be an alternative worth exploring as well.

@machadovilaca
Copy link
Member Author

submitted a new opt-in metric
kubernetes#2133

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants