refactoring (support multiple StorageSystems per ODF) #1106

SanjalKatiyar · 2023-11-23T05:58:00Z

Removed OCS related flags from "features.ts" file.
Added those flags as a part of redux store and created releated utils.
Created Hook to read these flags.
Updated Wizard to support multiple Ceph based clusters (only 1 internal + 1 external needed for now).
Updated KMS resources creation utils (Secrets) so that CSI and OCS resources are de-coupled.
Added a Namespace field to OCS wizard redux (used across entire wizard flow).
Updated dashboards so that each StorageCluster should only show related components, not any/all (NooBaa/RGW).
Updated StorageCluster dashboard Routes by adding Namespace as well.
Updated all queries using "odf_system_" and "ceph_" metrics, relying on "managedBy" label for now (Block/File/Object).
Updated StorageClass creation flow (ceph-rbd/fs) with a dropdown for corresponding StorageSystem selection.
Updated "HealthOverview" (injected to OCP dashboard's Status card currently) to incorporate status of all Ceph clusters.

SanjalKatiyar · 2023-12-05T07:45:06Z

packages/odf/components/create-storage-system/footer.tsx

+const OCS_INTERNAL_CR_NAME = 'ocs-storagecluster';
+const OCS_EXTERNAL_CR_NAME = 'ocs-external-storagecluster';


only keeping it here as this should not be used at any other place... for now name of the clusters are constant so passing them directly from this file during creation, later if needed, we should make it an user input...

these constant values should not be used in any other flow, except wizard flow for cluster creation...

GowthamShanmugam · 2023-12-07T13:27:26Z

packages/odf/components/odf-dashboard/status-card/status-card.tsx

          );
+          const systemNamespace = getNamespace(storageSystem);
+          const ocsHealthStatus =
+            ocsHealthStatuses[systemName + systemNamespace];


no separator symbol in between? just asking

no need, as I don't want to separate them later into name and namespace...

Lets use templates instead of +

GowthamShanmugam · 2023-12-07T13:31:10Z

packages/odf/components/system-list/odf-system-list.tsx

  return systems.reduce<SystemMetrics>((acc, curr) => {
-    acc[curr.metadata.name] = {
+    acc[getName(curr) + getNamespace(curr)] = {


This name + namespace key is used in multiple places, it is better to create one util function to form a key out of an argument with some separator symbol in between, so we can change this key format anytime. And we can easily figureout what are all the places need to be changed.

#1106 (comment)

GowthamShanmugam · 2023-12-07T14:22:58Z

packages/odf/utils/odf.ts

+export const isMCGStandaloneCluster = (storageCluster: StorageClusterKind) => {
+  return (
+    storageCluster?.spec?.multiCloudGateway?.reconcileStrategy === 'standalone'
+  );
+};


Suggested change

export const isMCGStandaloneCluster = (storageCluster: StorageClusterKind) => {

return (

storageCluster?.spec?.multiCloudGateway?.reconcileStrategy === 'standalone'

);

};

export const isMCGStandaloneCluster = (storageCluster: StorageClusterKind) =>

storageCluster?.spec?.multiCloudGateway?.reconcileStrategy === 'standalone';

GowthamShanmugam · 2023-12-07T14:23:38Z

packages/odf/utils/odf.ts

+export const isExternalCluster = (storageCluster: StorageClusterKind) => {
+  return !_.isEmpty(storageCluster?.spec?.externalStorage);
+};


Suggested change

export const isExternalCluster = (storageCluster: StorageClusterKind) => {

return !_.isEmpty(storageCluster?.spec?.externalStorage);

};

export const isExternalCluster = (storageCluster: StorageClusterKind) =>

!_.isEmpty(storageCluster?.spec?.externalStorage);

GowthamShanmugam · 2023-12-07T14:24:39Z

packages/odf/utils/odf.ts

+export const isClusterIgnored = (storageCluster: StorageClusterKind) => {
+  return storageCluster?.status?.phase === 'Ignored';
+};


Suggested change

export const isClusterIgnored = (storageCluster: StorageClusterKind) => {

return storageCluster?.status?.phase === 'Ignored';

};

export const isClusterIgnored = (storageCluster: StorageClusterKind) =>

storageCluster?.status?.phase === 'Ignored';

GowthamShanmugam · 2023-12-07T14:25:13Z

packages/odf/utils/odf.ts

+export const isNFSEnabled = (storageCluster: StorageClusterKind) => {
+  return storageCluster?.spec?.nfs?.enable === true;
+};


Suggested change

export const isNFSEnabled = (storageCluster: StorageClusterKind) => {

return storageCluster?.spec?.nfs?.enable === true;

};

export const isNFSEnabled = (storageCluster: StorageClusterKind) =>

storageCluster?.spec?.nfs?.enable === true;

GowthamShanmugam · 2023-12-07T14:27:55Z

packages/shared/src/dashboards/capacity-card/capacity-card.tsx

@@ -317,7 +322,7 @@ const CapacityCard: React.FC<CapacityCardProps> = ({
            const isPercentage = !!item?.totalValue;
            return (
              <CapacityCardRow
-                key={item.name}
+                key={item.name + item.namespace}


same here, better create util function to get the key with separator like name-namesapce

we don't need a separator as I do not want to parse/separate name and namespace later... also util might not be of any use because at some components we already have access to name/namespace and we just need to combine them, other place they are the CR and some places like this one a custom object... so single util will not gonna work any way...

packages/shared/src/selectors/k8s.ts

SanjalKatiyar · 2023-12-08T08:12:04Z

/test odf-console-e2e-aws

GowthamShanmugam · 2023-12-11T08:33:26Z

LGTM

SanjalKatiyar · 2023-12-11T17:15:59Z

CI un-blocked by: #1125 (PR#1125 is needed because of OCP react-router-dom updates, else components using imports will break).

bipuladh · 2023-12-12T05:06:58Z

packages/ocs/block-pool/CreateBlockPool.tsx

@@ -66,21 +62,24 @@ const CreateBlockPool: React.FC<CreateBlockPoolProps> = ({
  const { t } = useCustomTranslation();

  const history = useHistory();
+  const { namespace: poolNs } = useParams<ODFSystemParams>();


Suggested change

const { namespace: poolNs } = useParams<ODFSystemParams>();

const { namespace } = useParams<ODFSystemParams>();

Lets just use namespace.

bipuladh · 2023-12-12T05:08:42Z

packages/mco/components/mco-dashboard/storage-system/system-capacity-card/capacity-card.tsx

-  { systemName, targetKind, clusterName, totalValue, usedValue, clusterURL },
+  {
+    systemName,
+    namespace: systemNamespace,


Is there any reason to rename the variable? IMO namespace is pretty clear to understand.

we have 2 namespaces now: odfNamespace (where ODF is installed) and systemNamespace (where system is getting created), even though both are not used here still IMO it is good to rename things accordingly, it is harmless, rather a bit more intuitive :(

bipuladh · 2023-12-12T05:09:38Z

packages/ocs/block-pool/BlockPoolDetailsPage.tsx

@@ -30,18 +29,15 @@ export const BlockPoolDetailsPage: React.FC<BlockPoolDetailsPageProps> = ({

  const { poolName } = match.params;
  const location = useLocation();
+  const { namespace: poolNs } = useParams<ODFSystemParams>();


Suggested change

const { namespace: poolNs } = useParams<ODFSystemParams>();

const { namespace } = useParams<ODFSystemParams>();

I think it is clear as it is.

bipuladh · 2023-12-12T05:10:49Z

packages/ocs/block-pool/BlockPoolListPage.tsx

@@ -226,13 +222,15 @@ const RowRenderer: React.FC<RowProps<StoragePoolKind, CustomData>> = ({
 }) => {
  const { t } = useCustomTranslation();

+  const { systemFlags } = useODFSystemFlagsSelector();
+  const isExternalSS = systemFlags[getNamespace(obj)]?.isExternalMode;


Suggested change

const isExternalSS = systemFlags[getNamespace(obj)]?.isExternalMode;

const isExternalStorageSystem = systemFlags[getNamespace(obj)]?.isExternalMode;

bipuladh · 2023-12-12T05:15:35Z

packages/odf/redux/provider-hooks/useODFSystemFlags.ts

+    };
+  }, [scs, ccs, coss, nss, allLoaded, anyError]);
+
+  return useDeepCompareMemoize(payload);


Suggested change

return useDeepCompareMemoize(payload);

return useDeepCompareMemoize(payload, true);

that's intensional to minimise number of dispatches to the redux... payload will not be too long or too deep object...

bipuladh · 2023-12-12T05:37:17Z

packages/odf/components/topology/Topology.tsx

+    <div className="odf-topology__message-button">
+      <BlueInfoCircleIcon />{' '}
+      {showMessage &&
+        t('This view is only supported for Internal mode cluster.')}{' '}


Suggested change

t('This view is only supported for Internal mode cluster.')}{' '}

t('This view is only supported for Internal mode cluster.')}

I need that extra space between this text and the show/hide button...

bipuladh · 2023-12-12T05:38:15Z

packages/odf/components/topology/topology.scss

@@ -27,6 +27,16 @@
    z-index: 2;
 }

+.odf-topology__message-button {
+    color: #393F44;


Use PF variables.

something which I copied from already existing class odf-topology__back-button, just above this one... this is something which needs to be corrected once at all the places (probably is separate CSS based PR)...

bipuladh · 2023-12-12T05:55:49Z

...storage-system/create-storage-system-steps/review-and-create-step/review-and-create-step.tsx

  hasOCS,
+  // both internal and external exists
+  haveMultipleClusters,


Suggested change

haveMultipleClusters,

hasMultipleClusters,

bipuladh · 2023-12-12T05:58:54Z

packages/odf/components/odf-dashboard/status-card/status-card.tsx

          );
+          const systemNamespace = getNamespace(storageSystem);
+          const ocsHealthStatus =
+            ocsHealthStatuses[systemName + systemNamespace];


Lets use templates instead of +

bipuladh · 2023-12-12T06:00:06Z

packages/ocs/dashboards/block-pool/raw-capacity-card.tsx

+  const { namespace: clusterNs } = useParams<ODFSystemParams>();
+  const { systemFlags } = useODFSystemFlagsSelector();
+  const managedByOCS = systemFlags[clusterNs]?.ocsClusterName;
+


Lets use it as it is. Redux store is passed as a context so do we need multiple contexts?

bipuladh · 2023-12-12T11:53:11Z

/lgtm

Removed OCS related flags from "features.ts" file. Added those flags as a part of redux store and created releated utils. Created Hook to read these flags. Updated Wizard to support multiple Ceph based clusters (only 1 internal + 1 external needed for now). Updated KMS resources creation utils (Secrets) so that CSI and OCS resources are de-coupled. Added a Namespace field to OCS wizard redux (used across entire wizard flow). Updated dashboards so that each StorageCluster should only show related components, not any/all (NooBaa/RGW). Updated StorageCluster dashboard Routes by adding Namespace as well. Updated all queries using "odf_system_" and "ceph_" metrics, relying on "managedBy" label for now (Block/File/Object). Updated StorageClass creation flow (ceph-rbd/fs) with a dropdown for corresponding StorageSystem selection. Updated "HealthOverview" (injected to OCP dashboard's Status card currently) to incorporate status of all Ceph clusters.

bipuladh · 2023-12-12T15:04:54Z

/lgtm

openshift-ci · 2023-12-12T15:05:04Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [SanjalKatiyar,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 bot added do-not-merge/work-in-progress approved labels Nov 23, 2023

SanjalKatiyar force-pushed the multiple_sc_refactor_ss_support branch 5 times, most recently from 70d7ccc to 1163588 Compare November 27, 2023 16:17

openshift-merge-robot added the needs-rebase label Nov 27, 2023

SanjalKatiyar force-pushed the multiple_sc_refactor_ss_support branch 7 times, most recently from 20754a1 to c71a477 Compare November 30, 2023 12:50

openshift-merge-robot removed the needs-rebase label Nov 30, 2023

SanjalKatiyar force-pushed the multiple_sc_refactor_ss_support branch 4 times, most recently from 86a81cc to 36bebf9 Compare December 1, 2023 13:55

openshift-merge-robot added the needs-rebase label Dec 4, 2023

SanjalKatiyar force-pushed the multiple_sc_refactor_ss_support branch from 36bebf9 to fc03a5a Compare December 4, 2023 09:54

openshift-merge-robot removed the needs-rebase label Dec 4, 2023

SanjalKatiyar force-pushed the multiple_sc_refactor_ss_support branch 4 times, most recently from f6c8d86 to fd93a4c Compare December 5, 2023 07:02

SanjalKatiyar changed the title ~~[WIP] refactoring (support multiple StorageSystems per ODF)~~ refactoring (support multiple StorageSystems per ODF) Dec 5, 2023

openshift-ci bot removed the do-not-merge/work-in-progress label Dec 5, 2023

SanjalKatiyar commented Dec 5, 2023

View reviewed changes

GowthamShanmugam reviewed Dec 7, 2023

View reviewed changes

packages/shared/src/selectors/k8s.ts Show resolved Hide resolved

SanjalKatiyar requested a review from bipuladh December 8, 2023 05:25

SanjalKatiyar force-pushed the multiple_sc_refactor_ss_support branch from fd93a4c to 2d46312 Compare December 8, 2023 06:04

SanjalKatiyar requested a review from GowthamShanmugam December 8, 2023 06:04

SanjalKatiyar force-pushed the multiple_sc_refactor_ss_support branch 2 times, most recently from 1480d67 to a37b1e1 Compare December 8, 2023 10:39

bipuladh reviewed Dec 12, 2023

View reviewed changes

SanjalKatiyar force-pushed the multiple_sc_refactor_ss_support branch 2 times, most recently from bcafa7a to 5b9fc91 Compare December 12, 2023 08:50

openshift-ci bot assigned bipuladh Dec 12, 2023

openshift-ci bot added the lgtm label Dec 12, 2023

SanjalKatiyar force-pushed the multiple_sc_refactor_ss_support branch from 5b9fc91 to 617a945 Compare December 12, 2023 14:28

openshift-ci bot removed the lgtm label Dec 12, 2023

SanjalKatiyar force-pushed the multiple_sc_refactor_ss_support branch from 617a945 to 5516bd9 Compare December 12, 2023 14:35

openshift-ci bot added the lgtm label Dec 12, 2023

openshift-merge-bot bot merged commit b8e0554 into red-hat-storage:master Dec 12, 2023
6 checks passed

		const OCS_INTERNAL_CR_NAME = 'ocs-storagecluster';
		const OCS_EXTERNAL_CR_NAME = 'ocs-external-storagecluster';

	const { namespace: poolNs } = useParams<ODFSystemParams>();
	const { namespace } = useParams<ODFSystemParams>();

	const isExternalSS = systemFlags[getNamespace(obj)]?.isExternalMode;
	const isExternalStorageSystem = systemFlags[getNamespace(obj)]?.isExternalMode;

	return useDeepCompareMemoize(payload);
	return useDeepCompareMemoize(payload, true);

	t('This view is only supported for Internal mode cluster.')}{' '}
	t('This view is only supported for Internal mode cluster.')}

refactoring (support multiple StorageSystems per ODF) #1106

refactoring (support multiple StorageSystems per ODF) #1106

Conversation

SanjalKatiyar commented Nov 23, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GowthamShanmugam Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SanjalKatiyar commented Dec 8, 2023

GowthamShanmugam commented Dec 11, 2023

SanjalKatiyar commented Dec 11, 2023 • edited Loading

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

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

bipuladh commented Dec 12, 2023

bipuladh commented Dec 12, 2023

openshift-ci bot commented Dec 12, 2023

SanjalKatiyar commented Nov 23, 2023 •

edited

Loading

GowthamShanmugam Dec 7, 2023 •

edited

Loading

SanjalKatiyar commented Dec 11, 2023 •

edited

Loading