STOR-2040: CLI command to display bound pvc filesystem usage percentage #1854

gmeghnag · 2024-08-22T21:44:43Z

The code makes use of a Prometheus query to implement the oc adm top persistentvolumeclaims and show usage statistics for bound persistentvolumeclaims, as follows:

oc adm top pvc --insecure-skip-tls-verify=true -n reproducer-pvc
NAMESPACE      NAME               USAGE(%) 
reproducer-pvc pvc-reproducer-pvc 98.28    
reproducer-pvc pvc-test-pvc       14.56

It supports the following flags:

    -A, --all-namespaces=false:
	If present, list the pvc usage across all namespaces. Namespace in current context is ignored even if
	specified with --namespace

    --insecure-skip-tls-verify=false:
	If true, the server's certificate will not be checked for validity. This will make your HTTPS connections
	insecure

openshift-ci-robot · 2024-08-22T21:44:46Z

@gmeghnag: This pull request references STOR-2040 which is a valid jira issue.

In response to this:

The code makes use of a Prometheus query to implement the oc adm top persistentvolumeclaims and show usage statistics for bound persistentvolumeclaims, as follows:

oc adm top pvc --insecure-skip-tls-verify=true -n reproducer-pvc
NAMESPACE      NAME               USAGE(%) 
reproducer-pvc pvc-reproducer-pvc 98.28    
reproducer-pvc pvc-test-pvc       14.56

It supports the following flags:

   -A, --all-namespaces=false:
  If present, list the pvc usage across all namespaces. Namespace in current context is ignored even if
  specified with --namespace

   --insecure-skip-tls-verify=false:
  If true, the server's certificate will not be checked for validity. This will make your HTTPS connections
  insecure

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 openshift-eng/jira-lifecycle-plugin repository.

ardaguclu · 2024-08-26T05:47:25Z

Thank you for spending time on this nice and useful feature. But that requires a KEP first to sync on the feature and implementation.

gmeghnag · 2024-08-27T15:30:19Z

Hey @ardaguclu, what is a KEP? And how to create one? Thanks :)

ardaguclu · 2024-08-28T05:26:29Z

Hey @ardaguclu, what is a KEP? And how to create one? Thanks :)

I think, you'd write an enhancement proposal under https://github.com/openshift/enhancements/tree/master/enhancements/oc and we'll discuss the design and align on it. Once it merges, we can return to this PR.

dobsonj

Thanks a lot @gmeghnag :) This is nice work, I just have a few suggestions.

pkg/cli/admin/top/persistentvolumeclaims.go

dobsonj · 2024-10-29T16:53:38Z

/retest

ropatil010 · 2024-10-30T06:33:49Z

/test e2e-agnostic-ovn-cmd

gmeghnag · 2024-10-30T16:12:31Z

/retest

gmeghnag · 2024-10-31T12:02:49Z

DO NOT MERGE, I have to fix first https://issues.redhat.com/browse/OCPBUGS-44011

gmeghnag · 2024-11-06T11:18:41Z

https://issues.redhat.com/browse/OCPBUGS-44011 fixed.

gmeghnag · 2024-11-07T08:20:09Z

/retest

pkg/cli/admin/top/persistentvolumeclaims.go

ardaguclu

I still believe that this is a cool feature and thanks for spending time on this. I think it requires major code refactorings though.

/label tide/merge-method-squash

ardaguclu · 2024-11-19T10:25:57Z

pkg/cli/admin/top/persistentvolumeclaims.go

@@ -0,0 +1,382 @@
+package top


Since you'll be the maintainer of this command, there should be a separate OWNERS file like this https://github.com/openshift/oc/blob/master/pkg/cli/admin/inspectalerts/OWNERS

This comment is still valid. We need OWNERS file to maintain and triage the issues with respect to this comment.

pkg/cli/admin/top/persistentvolumeclaims.go

gmeghnag · 2024-11-19T15:28:49Z

@ardaguclu Thank you a lot for the code review!

Before spending time answering your questions and proceeding with the code modifications I would like to ask @tsmetana if doing so will make this PR merged, because, from our last chat I understood that you (with the storage leads) chose to go upstream first with this functionality, and doing so will probably need a different approach since Prometheus is not provided by default in vanilla Kubernetes.

In short, I don't want to spend time modifying the code if this PR will not be merged anyway.
Instead, if you think we can merge this PR in a future oc release, I'll work on that tomorrow.

Thanks

tsmetana · 2024-11-21T14:34:40Z

@gmeghnag labelling the feature as experimental is exactly what would make it merge even if we want to implement this also in upstream kubectl. The experimental / TP nature gives us the possibility to have the feature early in oc, but be able to change it later in case the kubectl implementation would differ in an incompatible way.

Thanks for the awesome work!

gmeghnag · 2024-11-21T16:08:33Z

@ardaguclu @tsmetana I've updated the PR, please let me know if anything else is needed! (:

gmeghnag · 2024-11-21T20:06:44Z

/retest-required

ardaguclu · 2024-11-22T09:58:05Z

Thank you. I quickly skimmed over the current code but we need to iterate a couple of times to be ready for merge. I think, it would be safer to defer this to 4.19 as unfortunately I don't have time for 4.18.

pkg/cli/admin/top/persistentvolumeclaims.go

ardaguclu · 2024-12-16T13:03:54Z

pkg/cli/admin/top/persistentvolumeclaims.go

+	for _, promOutputDataResult := range promOutput.Data.Result {
+		namespaceName := promOutputDataResult.Metric["namespace"]
+		pvcName := promOutputDataResult.Metric["persistentvolumeclaim"]
+		usagePercentage := promOutputDataResult.Value[1]


How do we know promOutputDataResult.Value's length is greater than 1 and we don't panic?

Because this is a "dogma" from Prometheus output, see here [1][2]:

Instant vectors are returned as result type vector. The corresponding result property has the following format:

[ { "metric": { "<label_name>": "<label_value>", ... }, "value": [ <unix_time>, "<sample_value>" ], "histogram": [ <unix_time>, <histogram> ] }, ... ]

[1] https://prometheus.io/docs/prometheus/latest/querying/api/#instant-queries
[2] https://prometheus.io/docs/prometheus/latest/querying/api/#instant-vectors

ardaguclu · 2025-01-21T12:31:36Z

pkg/cli/admin/top/persistentvolumeclaims.go

@@ -0,0 +1,382 @@
+package top


This comment is still valid. We need OWNERS file to maintain and triage the issues with respect to this comment.

pkg/cli/admin/top/persistentvolumeclaims.go

ardaguclu · 2025-01-21T12:44:48Z

pkg/cli/admin/top/persistentvolumeclaims.go

+	return body, nil
+}
+
+type PromOutput struct {


We can unexport this and the other nested ones.

pkg/cli/admin/top/OWNERS

pkg/cli/admin/top/persistentvolumeclaims.go

gmeghnag · 2025-01-23T12:24:53Z

@ardaguclu I updated the code with the modification you suggested; let me know if anything else is needed. [:

ardaguclu · 2025-01-23T13:33:47Z

Could you please squash this to one comment?. Merge commits shouldn't be listed in commits.

gmeghnag · 2025-01-23T16:36:15Z

"Could you please squash this to one comment?. Merge commits shouldn't be listed in commits."
@ardaguclu ~~on it~~ done! 👍

ardaguclu · 2025-01-26T14:29:32Z

Thank you
/approve

It is better to get lgtm from storage team.

jsafrane · 2025-01-31T13:06:59Z

/lgtm

openshift-ci · 2025-01-31T13:07:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, gmeghnag, jsafrane

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

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

tsmetana · 2025-02-03T08:45:16Z

/label acknowledge-critical-fixes-only

openshift-ci · 2025-02-03T13:01:48Z

@gmeghnag: all tests passed!

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

openshift-bot · 2025-02-03T16:59:25Z

[ART PR BUILD NOTIFIER]

Distgit: openshift-enterprise-cli
This PR has been included in build openshift-enterprise-cli-container-v4.19.0-202502031606.p0.gcbc8edc.assembly.stream.el9.
All builds following this will include this PR.

openshift-bot · 2025-02-03T17:05:30Z

[ART PR BUILD NOTIFIER]

Distgit: ose-tools
This PR has been included in build ose-tools-container-v4.19.0-202502031606.p0.gcbc8edc.assembly.stream.el9.
All builds following this will include this PR.

openshift-bot · 2025-02-03T18:05:38Z

[ART PR BUILD NOTIFIER]

Distgit: ose-cli-artifacts
This PR has been included in build ose-cli-artifacts-container-v4.19.0-202502031606.p0.gcbc8edc.assembly.stream.el9.
All builds following this will include this PR.

openshift-bot · 2025-02-03T19:07:13Z

[ART PR BUILD NOTIFIER]

Distgit: openshift-enterprise-deployer
This PR has been included in build openshift-enterprise-deployer-container-v4.19.0-202502031836.p0.gcbc8edc.assembly.stream.el9.
All builds following this will include this PR.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 22, 2024

openshift-ci bot requested review from ardaguclu and soltysh August 22, 2024 21:45

dobsonj reviewed Aug 29, 2024

View reviewed changes

pkg/cli/admin/top/persistentvolumeclaims.go Outdated Show resolved Hide resolved

pkg/cli/admin/top/persistentvolumeclaims.go Outdated Show resolved Hide resolved

pkg/cli/admin/top/persistentvolumeclaims.go Outdated Show resolved Hide resolved

tsmetana mentioned this pull request Oct 23, 2024

STOR-2040: oc adm top persistentvolumeclaim openshift/enhancements#1704

Merged

tsmetana reviewed Nov 8, 2024

View reviewed changes

pkg/cli/admin/top/persistentvolumeclaims.go Outdated Show resolved Hide resolved

ardaguclu reviewed Nov 19, 2024

View reviewed changes

openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 19, 2024

ardaguclu reviewed Dec 16, 2024

View reviewed changes

ardaguclu reviewed Jan 21, 2025

View reviewed changes

pkg/cli/admin/top/OWNERS Outdated Show resolved Hide resolved

ardaguclu reviewed Jan 21, 2025

View reviewed changes

pkg/cli/admin/top/OWNERS Outdated Show resolved Hide resolved

ardaguclu reviewed Jan 21, 2025

View reviewed changes

pkg/cli/admin/top/persistentvolumeclaims.go Outdated Show resolved Hide resolved

STOR-2040: CLI command to display bound pvc filesystem usage percentage

72cf52f

gmeghnag force-pushed the STOR-2040 branch from 9cc2cba to 72cf52f Compare January 24, 2025 10:55

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 26, 2025

openshift-ci bot assigned jsafrane Jan 31, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 31, 2025

openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Feb 3, 2025

openshift-merge-bot bot merged commit cbc8edc into openshift:master Feb 3, 2025
14 checks passed

STOR-2040: CLI command to display bound pvc filesystem usage percentage #1854

STOR-2040: CLI command to display bound pvc filesystem usage percentage #1854

Conversation

gmeghnag commented Aug 22, 2024

openshift-ci-robot commented Aug 22, 2024 • edited by openshift-ci bot Loading

ardaguclu commented Aug 26, 2024

gmeghnag commented Aug 27, 2024

ardaguclu commented Aug 28, 2024

dobsonj left a comment

Choose a reason for hiding this comment

dobsonj commented Oct 29, 2024

ropatil010 commented Oct 30, 2024

gmeghnag commented Oct 30, 2024

gmeghnag commented Oct 31, 2024

gmeghnag commented Nov 6, 2024

gmeghnag commented Nov 7, 2024

ardaguclu left a comment

Choose a reason for hiding this comment

ardaguclu Nov 19, 2024

Choose a reason for hiding this comment

ardaguclu Jan 21, 2025

Choose a reason for hiding this comment

gmeghnag commented Nov 19, 2024

tsmetana commented Nov 21, 2024

gmeghnag commented Nov 21, 2024

gmeghnag commented Nov 21, 2024

ardaguclu commented Nov 22, 2024

ardaguclu Dec 16, 2024

Choose a reason for hiding this comment

gmeghnag Jan 21, 2025

Choose a reason for hiding this comment

ardaguclu Jan 21, 2025

Choose a reason for hiding this comment

ardaguclu Jan 21, 2025

Choose a reason for hiding this comment

gmeghnag commented Jan 23, 2025 • edited Loading

ardaguclu commented Jan 23, 2025

gmeghnag commented Jan 23, 2025 • edited Loading

ardaguclu commented Jan 26, 2025

jsafrane commented Jan 31, 2025

openshift-ci bot commented Jan 31, 2025

tsmetana commented Feb 3, 2025

openshift-ci bot commented Feb 3, 2025

openshift-bot commented Feb 3, 2025

openshift-bot commented Feb 3, 2025

openshift-bot commented Feb 3, 2025

openshift-bot commented Feb 3, 2025

openshift-ci-robot commented Aug 22, 2024 •

edited by openshift-ci bot

Loading

gmeghnag commented Jan 23, 2025 •

edited

Loading

gmeghnag commented Jan 23, 2025 •

edited

Loading