Bug 1908598: Fix events filter for persitent dashboard #6856

a2batic · 2020-10-06T19:58:43Z

Activity card component cleanup
Filters the PVC event only for volume.beta.kubernetes.io/storage-provisioner: (ceph provisioner) PVCs

Jira: https://issues.redhat.com/browse/RHSTOR-1106
Signed-off-by: Kanika Murarka kmurarka@redhat.com

a2batic · 2020-10-06T19:59:15Z

afreen23 · 2020-10-07T04:36:25Z

...orage-plugin/src/components/dashboard-page/storage-dashboard/activity-card/activity-card.tsx

+    ) : (
+      <div className="co-status-card__alerts-body">
+        <div className="co-status-card__alert-item co-status-card__alert-item--loading">
+          <div className="skeleton-activity__dashboard" />
+          <div className="skeleton-activity__dashboard" />
+          <div className="skeleton-activity__dashboard" />
+          <div className="skeleton-activity__dashboard" />
+          <div className="skeleton-activity__dashboard" />
+        </div>
+      </div>


Can we pass an additional prop to the RecentEventsBody to handle this ?
@rawagner ^^ wdyt

you could merge pvcs.loaded prop with events.loaded. Something like

const events = {...resources.events, loaded: resources.events.loaded && resources.pvcs.loaded} return ( <RecentEventsBody events={events} filter={ocsEventNamespaceKindFilter} /> )

afreen23 · 2020-10-07T05:01:41Z

frontend/packages/ceph-storage-plugin/src/constants/index.ts

@@ -19,6 +19,7 @@ export const ATTACHED_DEVICES_ANNOTATION = 'cluster.ocs.openshift.io/local-devic
 export const DASHBOARD_LINK = '/dashboards/persistent-storage';
 export const AVAILABLE = 'Available';
 export const OSD_REMOVAL_TEMPLATE = 'ocs-osd-removal';
+export const PVC_FILTER_ANNOTATION = 'volume.beta.kubernetes.io/storage-provisioner';


Suggested change

export const PVC_FILTER_ANNOTATION = 'volume.beta.kubernetes.io/storage-provisioner';

export const PVC_PROVISIONER_ANNOTATION = 'volume.beta.kubernetes.io/storage-provisioner';

afreen23 · 2020-10-07T05:09:53Z

...orage-plugin/src/components/dashboard-page/storage-dashboard/activity-card/activity-card.tsx

+      const pvcItems = resources?.pvcs as FirehoseResult<PersistentVolumeClaimKind[]>;
+      if (eventKind === PersistentVolumeClaimModel.kind) {
+        const pvcObj = _.find(
+          pvcItems.data,
+          (obj: PersistentVolumeClaimKind) => obj?.metadata?.name === event?.involvedObject?.name,
+        );
+        const annotations = getAnnotations(pvcObj);
+        return (
+          annotations &&
+          annotations[PVC_FILTER_ANNOTATION] &&
+          isCephProvisioner(annotations[PVC_FILTER_ANNOTATION])
+        );
+      }
+      return eventNamespace === CEPH_STORAGE_NAMESPACE;
+    };
+


It would be better to create a map for pvcName-annotation prior and then utilize in this filter function. Both events and pvc can go high in numbers and you will end up with n^2

FYI the filter will show only events for existing PVCs. Its a tradeoff between showing events for all PVCs - non ceph ones but includes deleted ones too and only existing ceph PVCs. Is that really what you prefer ?

Yes, thats what we want to do here as per UX

The reason being , OCS supports ceph based storage classes only. PV/PVCs created by other storage classes should be filtered out.

@afreen23, as per @rawagner's comment, if we keep this filter, the events of deleted PVC will no longer be listed (being Ceph or non-ceph), the Ceph one as well which were appearing before will also disappear as the events won't match any PVC in pvc list, doesn't that take away the purpose of showing events?

We sorted this offline, commenting for reference here: Its a tradeoff but we need to have ocs based events only in the dashboard. All events can be looked anytime in console.

cloudbehl · 2020-10-07T06:02:00Z

@a2batic Probably not just for PVC but also the pods that in this namespace.(Pods events in openshift-storage+PVC events)

afreen23 · 2020-10-07T08:48:50Z

@a2batic Probably not just for PVC but also the pods that in this namespace.(Pods events in openshift-storage+PVC events)

We return the events for openshift-storage namespace here which covers pods

console/frontend/packages/ceph-storage-plugin/src/components/dashboard-page/storage-dashboard/activity-card/activity-card.tsx

Line 74 in fdfe2e7

return eventNamespace === CEPH_STORAGE_NAMESPACE;

afreen23 · 2020-10-20T13:57:03Z

...orage-plugin/src/components/dashboard-page/storage-dashboard/activity-card/activity-card.tsx

+      if (eventKind === PersistentVolumeClaimModel.kind) {
+        return validPVC().includes(eventObjectName);
+      }
+      return eventNamespace === CEPH_STORAGE_NAMESPACE;


Suggested change

if (eventKind === PersistentVolumeClaimModel.kind) {

return validPVC().includes(eventObjectName);

}

return eventNamespace === CEPH_STORAGE_NAMESPACE;

return cephPVC[eventObjectName] || eventNamespace === CEPH_STORAGE_NAMESPACE;

If we are checking on the name of PVC, then we dont need to check for kind.
Also, instead of having a function called everytime, you can create a set/map at once and use that.

We need to check for validPVC() array only for PVC name, we need not check it for every resource.
Also, for validPVC(), useCallback is used, so number of calls are reduced, not sure we should maintenance a map, as we are already maintaining an array.

afreen23 · 2020-10-20T19:40:01Z

...orage-plugin/src/components/dashboard-page/storage-dashboard/activity-card/activity-card.tsx

+    const validPVC = React.useCallback((): string[] => {
+      const pvcList = resources.pvcs
+        ? (resources?.pvcs?.data as PersistentVolumeClaimKind[]).filter((obj) => {
+            const annotations = getAnnotations(obj);
+            return (
+              annotations &&
+              annotations[PVC_PROVISIONER_ANNOTATION] &&
+              isCephProvisioner(annotations[PVC_PROVISIONER_ANNOTATION])
+            );
+          })
+        : [];
+      const pvcNames = _.reduce(
+        pvcList,
+        (res, pvcObj) => {
+          const name = getName(pvcObj);
+          res.push(name);
+          return res;
+        },
+        [],
+      );
+      return pvcNames;
+    }, [resources]);


Since you are memoizing the value, so you should use "useMemo` , but I dont think any would help, since resources is an object and you will get the calculation always running.

Also I would fetch the names at once instead creating a pvc list, then getting pvc names.

I have not checked, but withDashboardResources memoizes the values I guess.

bipuladh · 2020-12-14T05:00:46Z

...orage-plugin/src/components/dashboard-page/storage-dashboard/activity-card/activity-card.tsx

+        ? (resources?.pvcs?.data as PersistentVolumeClaimKind[]).filter((obj) => {
+            const annotations = getAnnotations(obj);
+            return (
+              annotations &&
+              annotations[PVC_PROVISIONER_ANNOTATION] &&
+              isCephProvisioner(annotations[PVC_PROVISIONER_ANNOTATION])
+            );


Suggested change

? (resources?.pvcs?.data as PersistentVolumeClaimKind[]).filter((obj) => {

const annotations = getAnnotations(obj);

return (

annotations &&

annotations[PVC_PROVISIONER_ANNOTATION] &&

isCephProvisioner(annotations[PVC_PROVISIONER_ANNOTATION])

);

? (resources?.pvcs?.data as PersistentVolumeClaimKind[]).filter((obj) =>

isCephProvisioner(getAnnotations(obj)?.[PVC_PROVISIONER_ANNOTATION])

bipuladh · 2020-12-14T05:02:53Z

...orage-plugin/src/components/dashboard-page/storage-dashboard/activity-card/activity-card.tsx

+      const pvcNames = _.reduce(
+        pvcList,
+        (res, pvcObj) => {
+          const name = getName(pvcObj);
+          res.push(name);
+          return res;
+        },
+        [],
+      );
+      return pvcNames;
+      // eslint-disable-next-line react-hooks/exhaustive-deps


Suggested change

const pvcNames = _.reduce(

pvcList,

(res, pvcObj) => {

const name = getName(pvcObj);

res.push(name);

return res;

},

[],

);

return pvcNames;

// eslint-disable-next-line react-hooks/exhaustive-deps

return pvcList?.map(getName);

I also don't see the need to disable the eslint rule.

bipuladh · 2020-12-14T05:15:39Z

...orage-plugin/src/components/dashboard-page/storage-dashboard/activity-card/activity-card.tsx

+    const validPVC = React.useMemo((): string[] => {
+      const pvcList = resources.pvcs
+        ? (resources?.pvcs?.data as PersistentVolumeClaimKind[]).filter((obj) => {
+            const annotations = getAnnotations(obj);
+            return (
+              annotations &&
+              annotations[PVC_PROVISIONER_ANNOTATION] &&
+              isCephProvisioner(annotations[PVC_PROVISIONER_ANNOTATION])
+            );
+          })
+        : [];
+      const pvcNames = _.reduce(
+        pvcList,
+        (res, pvcObj) => {
+          const name = getName(pvcObj);
+          res.push(name);
+          return res;
+        },
+        [],
+      );
+      return pvcNames;
+      // eslint-disable-next-line react-hooks/exhaustive-deps
+    }, [resources.pvcs]);


resources.pvcs is an array of objects, I would suggest you to use deepCompareMemoize and use the memoized value of resources. pvcs.
You could completely eliminate this useMemo block by parsing the pvcList normally and then memoizing the list of PVC names. That should be more efficient.

Suggested change

const validPVC = React.useMemo((): string[] => {

const pvcList = resources.pvcs

? (resources?.pvcs?.data as PersistentVolumeClaimKind[]).filter((obj) => {

const annotations = getAnnotations(obj);

return (

annotations &&

annotations[PVC_PROVISIONER_ANNOTATION] &&

isCephProvisioner(annotations[PVC_PROVISIONER_ANNOTATION])

);

})

: [];

const pvcNames = _.reduce(

pvcList,

(res, pvcObj) => {

const name = getName(pvcObj);

res.push(name);

return res;

},

[],

);

return pvcNames;

// eslint-disable-next-line react-hooks/exhaustive-deps

}, [resources.pvcs]);

const validPVC = (resources.pvcs?.data || [] as PersistentVolumeClaimKind[]).filter(

(obj) => isCephProvisioner(getAnnotations(obj)?.[PVC_PROVISIONER_ANNOTATION]))

.map(getName);

const memoizedPVCNames = useDeepCompareMemoize(validPVC, true);

Use memoizedPVCNames everywhere.

bipuladh · 2020-12-14T05:16:31Z

...orage-plugin/src/components/dashboard-page/storage-dashboard/activity-card/activity-card.tsx

+      if (eventKind === PersistentVolumeClaimModel.kind) {
+        return validPVC.includes(eventObjectName);
+      }
+      return eventNamespace === CEPH_STORAGE_NAMESPACE;


Suggested change

if (eventKind === PersistentVolumeClaimModel.kind) {

return validPVC.includes(eventObjectName);

}

return eventNamespace === CEPH_STORAGE_NAMESPACE;

return (eventKind === PersistentVolumeClaimModel.kind) ? validPVC.includes(eventObjectName) : eventNamespace === CEPH_STORAGE_NAMESPACE;

Jira: https://issues.redhat.com/browse/RHSTOR-1106 Signed-off-by: Kanika Murarka <kmurarka@redhat.com>

a2batic · 2020-12-16T11:14:36Z

/test analyze

a2batic · 2020-12-16T12:32:33Z

/test analyze

bipuladh

/lgtm

openshift-ci-robot · 2020-12-17T05:55:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a2batic, bipuladh

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:

~~frontend/packages/ceph-storage-plugin/OWNERS~~ [a2batic,bipuladh]

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

openshift-ci-robot · 2020-12-17T06:42:10Z

@a2batic: This pull request references Bugzilla bug 1908598, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1908598: Fix events filter for persitent 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.

openshift-ci-robot · 2020-12-17T08:30:37Z

@a2batic: All pull requests linked via external trackers have merged:

openshift/console#6856

Bugzilla bug 1908598 has been moved to the MODIFIED state.

In response to this:

Bug 1908598: Fix events filter for persitent 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.

openshift-ci-robot requested review from bipuladh and cloudbehl October 6, 2020 19:58

openshift-ci-robot added the component/ceph Related to ceph-storage-plugin label Oct 6, 2020

openshift-ci-robot assigned afreen23 Oct 6, 2020

afreen23 suggested changes Oct 7, 2020

View reviewed changes

a2batic force-pushed the filter-events branch from fdfe2e7 to 3d1d287 Compare October 20, 2020 13:41

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 20, 2020

a2batic requested review from afreen23 and rawagner October 20, 2020 15:25

afreen23 suggested changes Oct 20, 2020

View reviewed changes

a2batic force-pushed the filter-events branch from 3d1d287 to ee36f51 Compare December 14, 2020 03:56

bipuladh suggested changes Dec 14, 2020

View reviewed changes

Fix events filter for persitent dashboard

b2ea878

Jira: https://issues.redhat.com/browse/RHSTOR-1106 Signed-off-by: Kanika Murarka <kmurarka@redhat.com>

a2batic force-pushed the filter-events branch from ee36f51 to b2ea878 Compare December 16, 2020 07:50

bipuladh approved these changes Dec 17, 2020

View reviewed changes

openshift-ci-robot assigned bipuladh Dec 17, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2020

a2batic changed the title ~~Fix events filter for persitent dashboard~~ Bug 1908598: Fix events filter for persitent dashboard Dec 17, 2020

openshift-ci-robot added bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Dec 17, 2020

openshift-merge-robot merged commit e0cba4a into openshift:master Dec 17, 2020

spadgett added this to the v4.7 milestone Jan 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1908598: Fix events filter for persitent dashboard #6856

Bug 1908598: Fix events filter for persitent dashboard #6856

a2batic commented Oct 6, 2020

a2batic commented Oct 6, 2020

afreen23 Oct 7, 2020

rawagner Oct 7, 2020

afreen23 Oct 7, 2020

afreen23 Oct 7, 2020

rawagner Oct 7, 2020 •

edited

afreen23 Oct 7, 2020

afreen23 Oct 7, 2020

a2batic Oct 12, 2020

afreen23 Oct 20, 2020

cloudbehl commented Oct 7, 2020

afreen23 commented Oct 7, 2020

afreen23 Oct 20, 2020

afreen23 Oct 20, 2020

a2batic Dec 14, 2020

afreen23 Oct 20, 2020

bipuladh Dec 14, 2020

bipuladh Dec 14, 2020

bipuladh Dec 14, 2020

bipuladh Dec 14, 2020

a2batic commented Dec 16, 2020

a2batic commented Dec 16, 2020

bipuladh left a comment

openshift-ci-robot commented Dec 17, 2020

openshift-ci-robot commented Dec 17, 2020

openshift-ci-robot commented Dec 17, 2020

	export const PVC_FILTER_ANNOTATION = 'volume.beta.kubernetes.io/storage-provisioner';
	export const PVC_PROVISIONER_ANNOTATION = 'volume.beta.kubernetes.io/storage-provisioner';

Bug 1908598: Fix events filter for persitent dashboard #6856

Bug 1908598: Fix events filter for persitent dashboard #6856

Conversation

a2batic commented Oct 6, 2020

a2batic commented Oct 6, 2020

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

rawagner Oct 7, 2020 • edited

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

cloudbehl commented Oct 7, 2020

afreen23 commented Oct 7, 2020

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

a2batic commented Dec 16, 2020

a2batic commented Dec 16, 2020

bipuladh left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Dec 17, 2020

openshift-ci-robot commented Dec 17, 2020

openshift-ci-robot commented Dec 17, 2020

rawagner Oct 7, 2020 •

edited