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!

…2040

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

openshift-ci · 2025-01-21T12:35:08Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gmeghnag
Once this PR has been reviewed and has the lgtm label, please assign ardaguclu for approval. For more information see the 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:

OWNERS

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

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.

ardaguclu · 2025-01-21T12:33:19Z

pkg/cli/admin/top/persistentvolumeclaims.go

+
+const (
+	localhostRecoveryTokenSecret = "localhost-recovery-client-token"
+	kubeApiserverNamespace       = "openshift-kube-apiserver"


I don't think using an arbitrary namespace would be allowed (especially using the openshift-kube-apiserver which is an important one). If there is no bearer tokens, it is better to return error?.

ardaguclu · 2025-01-21T12:34:00Z

pkg/cli/admin/top/persistentvolumeclaims.go

+// RouteGetter is a function that gets a Route.
+type RouteGetter func(ctx context.Context, namespace string, name string, opts metav1.GetOptions) (*routev1.Route, error)
+
+type options struct {


options is unexported which makes sense but why some of its fields are exported?

ardaguclu · 2025-01-21T12:34:47Z

pkg/cli/admin/top/persistentvolumeclaims.go

+	BearerToken   string
+}
+
+func newOptions(streams genericiooptions.IOStreams) *options {


nit: Better to export

ardaguclu · 2025-01-21T12:43:57Z

pkg/cli/admin/top/persistentvolumeclaims.go

+	o.BearerToken = o.ClientConfig.BearerToken
+	if len(o.ClientConfig.BearerToken) == 0 {
+		klog.V(4).Info(fmt.Sprintf(`no token is currently in use for this session, attempting to retrieve token from secret "%s" in namespace "%s"`, localhostRecoveryTokenSecret, kubeApiserverNamespace))
+		secret, err := o.ClientSet.CoreV1().Secrets(kubeApiserverNamespace).Get(context.TODO(), localhostRecoveryTokenSecret, metav1.GetOptions{})


We need to continue with whatever we have defined in kubeconfig and should not try to use any other secret, etc.

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.

ardaguclu · 2025-01-21T12:46:17Z

pkg/cli/admin/top/OWNERS

+
+reviewers:
+  - gmeghnag
+  - ardaguclu


It is better to add approvers and reviewers from which team will maintain this. I'd prefer not to be in here.

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

pkg/cli/admin/top/OWNERS

@@ -0,0 +1,8 @@
+# See the OWNERS docs: https://git.k8s.io/community/contributors/guide/owners.md


pkg/cli/admin/top command exists and it is maintained by workloads team. What about moving this command to pkg/cli/admin/toppvc, etc. and have dedicated OWNERS?.

It is also better to have an alias like this /~https://github.com/openshift/oc/blob/master/pkg/cli/admin/inspectalerts/OWNERS and

oc/OWNERS_ALIASES

Line 9 in e005223

monitoring-approvers:

ardaguclu · 2025-01-21T13:08:22Z

pkg/cli/admin/top/persistentvolumeclaims.go

+
+func (o *options) Run(ctx context.Context, args []string) error {
+	o.BearerToken = o.ClientConfig.BearerToken
+	if len(o.ClientConfig.BearerToken) == 0 {


Also keep in mind that, kubeconfig has many fields to access cluster, not only bearer token. As a result, this command will not be usable if kubeconfig authentication mechanism is certificates, exec credential plugins, etc.

openshift-ci · 2025-01-21T17:00:59Z

@gmeghnag: The following tests 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-aws-ovn-serial	`64325f9`	link	true	`/test e2e-aws-ovn-serial`
ci/prow/e2e-agnostic-ovn-cmd	`64325f9`	link	true	`/test e2e-agnostic-ovn-cmd`
ci/prow/unit	`64325f9`	link	true	`/test unit`

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.

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

ee69f12

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 Show resolved Hide resolved

pkg/cli/admin/top/persistentvolumeclaims.go 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

gmeghnag and others added 2 commits October 24, 2024 12:14

Merge branch 'openshift:master' into STOR-2040

51073ae

fallback to "localhost-recovery-client-token" token

a3cf814

gmeghnag and others added 2 commits November 6, 2024 12:15

Merge branch 'openshift:master' into STOR-2040

6027c8b

OCPBUGS-44011: display output as per the num of pvc counts

2a0cace

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

gmeghnag and others added 3 commits November 21, 2024 17:05

code improvement

bf39f7d

Merge branch 'openshift:master' into STOR-2040

4149a8a

Merge branch 'STOR-2040' of /~https://github.com/gmeghnag/oc into STOR-…

f3eebfa

…2040

ardaguclu reviewed Dec 16, 2024

View reviewed changes

add OWNERS, and minor changes

64325f9

ardaguclu reviewed Jan 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

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

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

ardaguclu Nov 19, 2024

ardaguclu Jan 21, 2025

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

gmeghnag Jan 21, 2025

openshift-ci bot commented Jan 21, 2025

ardaguclu Jan 21, 2025

ardaguclu Jan 21, 2025

ardaguclu Jan 21, 2025

ardaguclu Jan 21, 2025

ardaguclu Jan 21, 2025

ardaguclu Jan 21, 2025

ardaguclu Jan 21, 2025

ardaguclu Jan 21, 2025

ardaguclu Jan 21, 2025

ardaguclu Jan 21, 2025

openshift-ci bot commented Jan 21, 2025

		@@ -0,0 +1,8 @@
		# See the OWNERS docs: https://git.k8s.io/community/contributors/guide/owners.md

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

Are you sure you want to change the base?

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 21, 2025

openshift-ci-robot commented Aug 22, 2024 •

edited by openshift-ci bot

Loading