Adds Pool creation modal for storageClass form #6024

a2batic · 2020-07-20T05:36:16Z

Enable user to create a Pool while creating OCS backed storage Class.

Jira: https://issues.redhat.com/browse/RHSTOR-1154
Signed-off-by: Kanika Murarka kmurarka@redhat.com
Co-authored-by: Paul Cuzner pcuzner@redhat.com

gnehapk · 2020-07-24T05:45:52Z

...ckages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-constant.ts

@@ -0,0 +1,10 @@
+export const replicas = {
+  ReplicaTWO: '2',
+  ReplicaTHREE: '3',


There is already one constant defined for replica 3 here - https://github.com/openshift/console/blob/master/frontend/packages/ceph-storage-plugin/src/constants/index.ts#L17

gnehapk · 2020-07-24T05:56:29Z

...packages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-modal.tsx

+import { CEPH_STORAGE_NAMESPACE, OCS_INTERNAL_CR_NAME } from '../../../constants/index';
+
+import { useK8sWatchResource } from '@console/internal/components/utils/k8s-watch-hook';
+import { referenceForModel } from '@console/internal/module/k8s';


These 2 imports need to be moved up before the relative ones.

gnehapk · 2020-07-24T05:57:35Z

...packages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-modal.tsx

+const PoolStatusComponent: React.FC<PoolStatusComponentProps> = ({ status, name, error = '' }) => {
+  return (
+    <div>
+      {status === 'progress' && (


We should create a constant for all the status, an enum may be.

gnehapk · 2020-07-24T06:02:38Z

...packages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-modal.tsx

+          <EmptyStateBody>Pool {name} was successfully created</EmptyStateBody>
+        </EmptyState>
+      )}
+      {status === 'failed' && (


switch might be better here.

...packages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-modal.tsx

gnehapk · 2020-07-24T06:10:17Z

...packages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-modal.tsx

+  );
+
+  React.useEffect(() => {
+    k8sGet(OCSServiceModel, OCS_INTERNAL_CR_NAME, CEPH_STORAGE_NAMESPACE)


K8sGet -> fetchK8s, Just replacing the word will make it work. No extra efforts.

gnehapk · 2020-07-24T06:15:56Z

...packages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-modal.tsx

+        setPoolStatus('created');
+        setIsSubmit(false);
+        setTimeClear(true);
+      } else if (newPoolLoaded && newPool?.status?.phase === 'ReconcileFailed') {


Create an enum for all possible phase values.

gnehapk · 2020-07-24T06:19:44Z

...packages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-modal.tsx

+        const timeoutTimer = setTimeout(() => {
+          setPoolStatus('timeout');
+          setIsSubmit(false);
+        }, 15000);


15 * SECOND, Constant for SECOND already present.

...packages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-modal.tsx

gnehapk · 2020-07-24T06:29:59Z

...packages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-modal.tsx

+  const [isCompressed, setCompression] = React.useState(false);
+  const [deviceClass, setdeviceClass] = React.useState('');
+  const [poolStatus, setPoolStatus] = React.useState('');
+  const [storageClusterLoading, setStorageClusterLoading] = React.useState(true);


May be you can create an object like

const storageClustetState = { loading: true, error: false, ready: false, }

and use this whole object as state. It will look more cleaner.

gnehapk · 2020-07-24T06:35:46Z

...packages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-modal.tsx

+        </EmptyState>
+      )}
+      {status === 'timeout' && (
+        <EmptyState>


<EmptyState> <EmptyStateIcon icon={ExclamationCircleIcon} className="ceph-storage-pool__error-icon" /> <EmptyStateBody> {error || `An error occurred, Pool ${name} was not created`} </EmptyStateBody> </EmptyState>

It make sense to create a common component for this, since its used at many places. for eg.
<EmptyStateContainer icon={} message={} /> WDYT?

gnehapk · 2020-07-24T06:38:36Z

...packages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-modal.tsx

+                if (poolStatus === 'created') {
+                  onPoolCreation(newPoolName);
+                }
+                close();


Please move this to a function and use it here.

gnehapk · 2020-07-24T06:40:40Z

...packages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-modal.tsx

+        !poolStatus &&
+        !storageClusterError &&
+        storageClusterReady &&
+        cephClusterObj[0]?.status?.phase === 'Ready' ? (


Moving these conditions to a function and calling here, might be a better approach as so many conditions are checked.

gnehapk · 2020-07-24T06:43:02Z

...kages/ceph-storage-plugin/src/components/ocs-storage-class-form/_ocs-storage-class-form.scss

@@ -0,0 +1,27 @@
+//@import '../../../../console-shared/src/styles/skeleton-screen.scss';


Why _ for the file name?

gnehapk · 2020-07-24T06:51:34Z

...ackages/ceph-storage-plugin/src/components/ocs-storage-class-form/ocs-storage-class-form.tsx

+      </div>
+    );
+  }
+  return <>{loaded || poolDataLoaded || <LoadingInline />}</>;


Shouldn't this be {(loaded || poolDataLoaded) && <LoadingInline />}

cloudbehl · 2020-07-24T08:14:50Z

...ckages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-constant.ts

+
+export const modalTitle = 'Create New Pool';
+export const modalDescription =
+  'Pools are logical groups of Ceph objects. Such objects live inside of Ceph, or rather they live inside RADOS.';


we don't mention Ceph & RADOS as they are internal entities. @yuvalgalanti ^^

cloudbehl · 2020-07-24T08:16:36Z

...ckages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-constant.ts

+export const modalDescription =
+  'Pools are logical groups of Ceph objects. Such objects live inside of Ceph, or rather they live inside RADOS.';
+export const notReadyDesc =
+  'The creation of an OCS storage cluster is still in progress or have failed, please try again after the storage cluster is ready to use.';


Are we checking the storage cluster or Ceph Cluster both are different entities? The storage cluster might be ready even when ceph cluster is not & user will be baffled with this message on why this is happening.

cloudbehl · 2020-07-27T05:47:29Z

...packages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-modal.tsx

+
+const PoolStatusComponent: React.FC<PoolStatusComponentProps> = ({ status, name, error = '' }) => {
+  return (
+    <div>


Suggested change

<div>

<>

cloudbehl · 2020-07-27T05:47:40Z

...packages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-modal.tsx

+          </EmptyStateBody>
+        </EmptyState>
+      )}
+    </div>


Suggested change

</div>

</>

cloudbehl · 2020-07-27T05:50:11Z

...packages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-modal.tsx

+  const [timeoutClear, setTimeClear] = React.useState(false);
+  const [timer, setTimer] = React.useState<NodeJS.Timer>(null);
+
+  const poolResource = React.useMemo(() => {


Why are we pooling single pool?

We are pooling to get status of pool, before closing the modal.

cloudbehl · 2020-07-27T05:52:05Z

...packages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-modal.tsx

+    k8sGet(OCSServiceModel, OCS_INTERNAL_CR_NAME, CEPH_STORAGE_NAMESPACE)
+      .then((cluster: OCSStorageClusterKind) => {
+        setStorageClusterLoading(false);
+        if (cluster.status.phase === 'Ready') {


Cluster might not be ready because of something with NooBaa & will not let user create pool?

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

gnehapk · 2020-07-27T13:27:27Z

...packages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-modal.tsx

+  ModalComponentProps,
+  ModalFooter,
+} from '@console/internal/components/factory/modal';
+import { apiVersionForModel } from '@console/kubevirt-plugin/integration-tests/tests/utils/selectors';


this needs to be imported from @console/internal/module/k8s and not kubevirt-plugin plugin.

Good catch!!

Jira: https://issues.redhat.com/browse/RHSTOR-1154 Signed-off-by: Kanika Murarka kmurarka@redhat.com Co-authored-by: Paul Cuzner pcuzner@redhat.com

cloudbehl · 2020-07-28T08:35:01Z

/lgtm

openshift-ci-robot · 2020-07-28T08:35:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a2batic, cloudbehl, gnehapk

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~~ [cloudbehl,gnehapk]

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

openshift-bot · 2020-07-28T10:53:02Z

/retest

Please review the full test history for this PR and help us cut down flakes.

a2batic · 2020-07-28T12:34:01Z

/test e2e-gcp-console

openshift-bot · 2020-07-28T13:29:06Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-28T13:42:20Z

/retest

Please review the full test history for this PR and help us cut down flakes.

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

openshift-ci-robot requested review from cloudbehl and gnehapk July 20, 2020 05:36

a2batic marked this pull request as draft July 20, 2020 05:36

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 20, 2020

a2batic changed the title ~~Adds Pool creation modal for storageClass form~~ [WIP] Adds Pool creation modal for storageClass form Jul 20, 2020

a2batic force-pushed the pool-component branch 2 times, most recently from 9d31d11 to afc43b1 Compare July 23, 2020 20:21

a2batic marked this pull request as ready for review July 23, 2020 20:21

a2batic changed the title ~~[WIP] Adds Pool creation modal for storageClass form~~ Adds Pool creation modal for storageClass form Jul 23, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 23, 2020

gnehapk reviewed Jul 24, 2020

View reviewed changes

...packages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-modal.tsx Show resolved Hide resolved

gnehapk reviewed Jul 24, 2020

View reviewed changes

...packages/ceph-storage-plugin/src/components/modals/storage-pool-modal/storage-pool-modal.tsx Outdated Show resolved Hide resolved

gnehapk reviewed Jul 24, 2020

View reviewed changes

cloudbehl reviewed Jul 27, 2020

View reviewed changes

a2batic force-pushed the pool-component branch 2 times, most recently from 8624739 to 4fd777b Compare July 27, 2020 12:40

a2batic requested review from gnehapk and cloudbehl July 27, 2020 12:59

gnehapk reviewed Jul 27, 2020

View reviewed changes

a2batic force-pushed the pool-component branch 2 times, most recently from 31715a3 to cdd6f90 Compare July 28, 2020 05:53

Adds Pool creation modal for storageClass form

e199c13

Jira: https://issues.redhat.com/browse/RHSTOR-1154 Signed-off-by: Kanika Murarka kmurarka@redhat.com Co-authored-by: Paul Cuzner pcuzner@redhat.com

a2batic force-pushed the pool-component branch from cdd6f90 to e199c13 Compare July 28, 2020 06:37

gnehapk approved these changes Jul 28, 2020

View reviewed changes

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

openshift-ci-robot assigned cloudbehl Jul 28, 2020

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

openshift-merge-robot merged commit 0594dab into openshift:master Jul 28, 2020

spadgett added this to the v4.6 milestone Jul 30, 2020

		@@ -0,0 +1,27 @@
		//@import '../../../../console-shared/src/styles/skeleton-screen.scss';

Adds Pool creation modal for storageClass form #6024

Adds Pool creation modal for storageClass form #6024

Conversation

a2batic commented Jul 20, 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

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

gnehapk Jul 24, 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

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

cloudbehl commented Jul 28, 2020

openshift-ci-robot commented Jul 28, 2020

openshift-bot commented Jul 28, 2020

a2batic commented Jul 28, 2020

openshift-bot commented Jul 28, 2020

openshift-bot commented Jul 28, 2020

a2batic commented Jul 20, 2020 •

edited

gnehapk Jul 24, 2020 •

edited