Adds Local Volume Set Creation Form and enables LSO Plugin #5014

afreen23 · 2020-04-13T06:52:38Z

Local Volume Set is the new CR to be introduced in Local Storage Operator.
Local Volume Set will allow to filter a set of storage volumes, group them and create a dedicated storage class to consume storage for them.

Designs: https://marvelapp.com/8752a03/screen/67632184

gnehapk · 2020-04-13T06:54:10Z

@afreen23 Can you please attach the screenshots too

bipuladh · 2020-04-13T07:26:42Z

...es/local-storage-operator-plugin/src/components/local-volume-set/create-local-volume-set.tsx

+const volumeTypeOptions = Object.freeze({
+  'SSD/NVMe': 'SSD/NVMe',
+  HDD: 'HDD',
+});
+const volumeModeOptions = Object.freeze({
+  Block: 'Block',
+  Filesystem: 'Filesystem',
+});
+const volumeSizeUnitOptions = Object.freeze({
+  TiB: 'TiB',
+  GiB: 'GiB',
+});


I would prefer enums.

I need to keep the plain objects for future fields

bipuladh · 2020-04-13T07:27:57Z

...es/local-storage-operator-plugin/src/components/local-volume-set/create-local-volume-set.tsx

+  GiB: 'GiB',
+});
+
+const CreateLocalVolumeSet: React.FC = withHandlePromise((props: CreateLocalVolumeSetProps) => {


Suggested change

const CreateLocalVolumeSet: React.FC = withHandlePromise((props: CreateLocalVolumeSetProps) => {

const CreateLocalVolumeSet: React.FC = withHandlePromise<CreateLocalVolumeSetProps>((props) => {

bipuladh · 2020-04-13T07:29:20Z

...es/local-storage-operator-plugin/src/components/local-volume-set/create-local-volume-set.tsx

+      .catch((e) => {
+        // eslint-disable-next-line no-console
+        console.error(e);


This is pointless if you're already showing the error in the ActionBar.

Sure, I think just catch(() -> null) here would be suffice for linter

bipuladh · 2020-04-13T07:30:30Z

...es/local-storage-operator-plugin/src/components/local-volume-set/create-local-volume-set.tsx

+            ]}
+          />
+        </div>
+        <h1 className="co-create-operand__header-text">{`Create ${modelName}`}</h1>


bipuladh · 2020-04-13T07:30:59Z

...es/local-storage-operator-plugin/src/components/local-volume-set/create-local-volume-set.tsx

+            isRequired
+          />
+        </FormGroup>
+        <FormGroup label="Storage Class Name" fieldId="create-lvs--storage-class-name">


Is this not a required field?

afreen23 · 2020-04-13T09:31:14Z

...es/local-storage-operator-plugin/src/components/local-volume-set/create-local-volume-set.tsx

+            <Radio
+              label="All nodes"
+              name="nodes-selection"
+              id="create-lvs--radio-all-nodes"
+              className="lso-create-lvs__all-nodes-radio--padding"
+              value="allNodes"
+              onChange={toggleShowNodesList}
+              description="Selecting all nodes will search for available volume storage on all nodes."
+              defaultChecked
+            />
+            <Radio
+              label="Select nodes"
+              name="nodes-selection"
+              id="create-lvs--radio-select-nodes"
+              value="selectedNodes"
+              onChange={toggleShowNodesList}
+              description="Selecting nodes allow you to limit the search for available volumes to specific nodes."
+            />


These are radio groups. Only one of them will be selected at a time.
The value is not significant here.
showNodesList state takes care of toggling

bipuladh · 2020-04-13T07:34:39Z

...kages/local-storage-operator-plugin/src/components/local-volume-set/nodes-selection-list.tsx

+    const nodeLocation = node.metadata.labels?.['failure-domain.beta.kubernetes.io/zone'] ?? '-';
+    const nodeCpuCapacity = getNodeCPUCapacity(node);
+    const nodeAllocatableMemory = getNodeAllocatableMemory(node);
+    const nodeTaints = node?.spec?.taints?.length ?? 0;


Suggested change

const nodeTaints = node?.spec?.taints?.length ?? 0;

const nodeTaints = node.spec?.taints?.length ?? 0;

bipuladh · 2020-04-13T07:38:02Z

...kages/local-storage-operator-plugin/src/components/local-volume-set/nodes-selection-list.tsx

+};
+
+type SetTableRows = (props: {
+  componentProps: { data: NodeKind[]; filters: { name: string } };


Suggested change

componentProps: { data: NodeKind[]; filters: { name: string } };

componentProps: { data: NodeKind[]; filters: { name: string, labels: { all: string[] } } };

bipuladh · 2020-04-13T07:39:06Z

frontend/packages/local-storage-operator-plugin/src/components/local-volume-set/types.ts

+  apiVersion: string;
+  kind: string;
+  metadata: { name: string };
+  spec: {
+    nodeSelector?: {
+      nodeSelectorTerms: [
+        {
+          matchExpressions: [{ key: string; operator: string; values: string[] }];
+        },
+      ];
+    };


K8sResourceCommon might handle a few of these fields.

bipuladh · 2020-04-13T07:41:33Z

...kages/local-storage-operator-plugin/src/components/local-volume-set/nodes-selection-list.tsx

+      title: 'CPU',
+      props: { className: tableColumnClasses[1] },
+    },
+    {
+      title: 'Memory',
+      props: { className: tableColumnClasses[2] },
+    },
+    {
+      title: 'Location',
+      props: { className: tableColumnClasses[3] },
+    },
+    {
+      title: 'Taints',
+      props: { className: tableColumnClasses[3] },
+    },


Add sorts for all these fields as well.

It as per UXD

@yuvalgalanti why are we only allowing users to sort on the basis of Name? IMO we should sort on CPU and Memory too.

bipuladh · 2020-04-13T08:07:50Z

...kages/local-storage-operator-plugin/src/components/local-volume-set/nodes-selection-list.tsx

+    ];
+    rowUIDMap[uid] = {
+      cells,
+      selected: !!rows?.[uid]?.selected,


Suggested change

selected: !!rows?.[uid]?.selected,

selected: rows?.[uid]?.selected,

afreen23 · 2020-04-13T09:31:14Z

...es/local-storage-operator-plugin/src/components/local-volume-set/create-local-volume-set.tsx

+            <Radio
+              label="All nodes"
+              name="nodes-selection"
+              id="create-lvs--radio-all-nodes"
+              className="lso-create-lvs__all-nodes-radio--padding"
+              value="allNodes"
+              onChange={toggleShowNodesList}
+              description="Selecting all nodes will search for available volume storage on all nodes."
+              defaultChecked
+            />
+            <Radio
+              label="Select nodes"
+              name="nodes-selection"
+              id="create-lvs--radio-select-nodes"
+              value="selectedNodes"
+              onChange={toggleShowNodesList}
+              description="Selecting nodes allow you to limit the search for available volumes to specific nodes."
+            />


These are radio groups. Only one of them will be selected at a time.
The value is not significant here.
showNodesList state takes care of toggling

bipuladh · 2020-04-13T12:18:13Z

...es/local-storage-operator-plugin/src/components/local-volume-set/create-local-volume-set.tsx

+      .then((resource) => {
+        history.push(resourceObjPath(resource, referenceFor(resource)));
+      })
+      .catch(() => null);


Suggested change

.catch(() => null);

No need for the catch statement

This is enforced by linter

It's not. handlePromise handles the catch part for you.

Surely handlePromise sets state and error message on catching error but it also returns a promise. Linters are dumb they dont know that, hence I need to put a dummy catch.
This will not break anything in our case because we have error handled in state.

console/frontend/public/components/utils/promise-component.tsx

Line 72 in b270084

handlePromise: <T>(promise: Promise<T>) => Promise<T>;

bipuladh · 2020-04-13T12:18:57Z

...es/local-storage-operator-plugin/src/components/local-volume-set/create-local-volume-set.tsx

+          <ListPage
+            customData={{ rows, setRows, allSelected, setAllSelected }}
+            showTitle={false}
+            kind="Node"


Suggested change

kind="Node"

kind={NodeKind.kind}

bipuladh · 2020-04-13T12:20:27Z

...es/local-storage-operator-plugin/src/components/local-volume-set/create-local-volume-set.tsx

+            <Button data-test-id="create-lvs-form-create-button" type="submit" variant="primary">
+              Create
+            </Button>
+            <Button
+              data-test-id="create-lvs-form-cancel-button"
+              type="button"
+              variant="secondary"
+              onClick={onCancel}
+            >


Button's really don't need data-test-id since we have a common class for Primary used throughout the console.

I added as per your suggestion on chat, will remove again.

bipuladh · 2020-04-13T12:23:20Z

...kages/local-storage-operator-plugin/src/components/local-volume-set/nodes-selection-list.tsx

+    setAllSelected: React.Dispatch<React.SetStateAction<boolean>>;
+    setRows: React.Dispatch<React.SetStateAction<RowUIDMap>>;
+  };
+  filters: { name: string };


Suggested change

filters: { name: string };

You are not using it here. The type is incomplete.

bipuladh · 2020-04-13T12:24:46Z

frontend/packages/local-storage-operator-plugin/src/components/local-volume-set/types.ts

+    storageClassName: string;
+    volumeMode: string;
+    deviceInclusionSpec: {
+      deviceTypes: ('RawDisk' | 'Partition')[];


Suggested change

deviceTypes: ('RawDisk' | 'Partition')[];

deviceTypes: ('RawDisk' | 'Partition')[];

Please use enum instead of String Literals it's easier to use and understand.

bipuladh · 2020-04-13T12:24:53Z

frontend/packages/local-storage-operator-plugin/src/components/local-volume-set/types.ts

+    volumeMode: string;
+    deviceInclusionSpec: {
+      deviceTypes: ('RawDisk' | 'Partition')[];
+      deviceMechanicalProperty: ('Rotational' | 'Non Rotational')[];


Same as above.

bipuladh · 2020-04-13T12:28:50Z

...es/local-storage-operator-plugin/src/components/local-volume-set/create-local-volume-set.tsx

+        volumeMode,
+        deviceInclusionSpec: {
+          // Only Raw disk supported for 4.5
+          deviceTypes: ['RawDisk'],


What is this magic string RawDisk?

This is hard coded intentionally. The support is for RawDisk only.
Hence, the request data just needs that. No other values are applicable

I would prefer it if it came via an enum.

bipuladh · 2020-04-13T12:29:25Z

...es/local-storage-operator-plugin/src/components/local-volume-set/create-local-volume-set.tsx

+  const [volumeType, setVolumeType] = React.useState('SSD');
+  const [volumeMode, setVolumeMode] = React.useState('Block');


Consider using enums for these values.

bipuladh · 2020-04-13T12:32:11Z

...es/local-storage-operator-plugin/src/components/local-volume-set/create-local-volume-set.tsx

+  SSD: {
+    title: 'SSD / NVMe',
+    property: 'Non Rotational',
+  },
+  HDD: {
+    title: 'HDD',
+    property: 'Rotational',
+  },
+};
+
+const VOLUME_MODE = {
+  Block: {
+    title: 'Block',
+  },
+  Filesystem: {
+    title: 'Filesystem',
+  },
+};


Suggested change

SSD: {

title: 'SSD / NVMe',

property: 'Non Rotational',

},

HDD: {

title: 'HDD',

property: 'Rotational',

},

};

const VOLUME_MODE = {

Block: {

title: 'Block',

},

Filesystem: {

title: 'Filesystem',

},

};

enum VolumeType {

SSD = 'SSD',

HDD = 'HDD'

}

[VolumeType.SSD]: {

title: 'SSD / NVMe',

property: 'Non Rotational',

},

[VolumeType.HDD]: {

title: 'HDD',

property: 'Rotational',

},

};

enum VolumeMode {

BLOCK = 'Block',

FS = 'FileSystem'

};

const VOLUME_MODE = {

[VolumeMode.Block]: {

title: 'Block',

},

[VolumeMode.FS]: {

title: 'Filesystem',

},

};

Surely, I can consider but a bit different .
Thanks!

bipuladh · 2020-04-14T12:17:10Z

...es/local-storage-operator-plugin/src/components/local-volume-set/create-local-volume-set.tsx

+    handlePromise(k8sCreate(LocalVolumeSetModel, requestData))
+      .then((resource) => history.push(resourceObjPath(resource, referenceFor(resource))))
+      .catch(() => null);


Suggested change

handlePromise(k8sCreate(LocalVolumeSetModel, requestData))

.then((resource) => history.push(resourceObjPath(resource, referenceFor(resource))))

.catch(() => null);

return handlePromise(k8sCreate(LocalVolumeSetModel, requestData))

.then((resource) => history.push(resourceObjPath(resource, referenceFor(resource))));

Or just disable the rule that enforces you to do so.

rawagner · 2020-04-14T12:44:13Z

...es/local-storage-operator-plugin/src/components/local-volume-set/create-local-volume-set.tsx

+  const [volumeMode, setVolumeMode] = React.useState(volumeModeDropdownItems.Block);
+  const [maxVolumeLimit, setMaxVolumeLimit] = React.useState('');
+  const [rows, setRows] = React.useState<RowUIDMap>({});
+  const [allSelected, setAllSelected] = React.useState(null);


Suggested change

const [allSelected, setAllSelected] = React.useState(null);

const [allSelected, setAllSelected] = React.useState<boolean>(null);

rawagner · 2020-04-14T12:57:18Z

frontend/public/components/factory/table.tsx

@@ -366,6 +366,13 @@ export type TableProps = {
  Row?: RowFunction;
  Rows?: (...args) => any[];
  'aria-label': string;
+  onSelect?: (


you can use OnSelect already defined in @patternfly/react-table

Suggested change

onSelect?: (

onSelect?: OnSelect

Sure , I forgot to replace it here.
Thanks!

rawagner · 2020-04-14T13:01:33Z

/approve

bipuladh · 2020-04-14T13:17:28Z

...es/local-storage-operator-plugin/src/components/local-volume-set/create-local-volume-set.tsx

+            onChange={setVolumeType}
+          />
+        </FormGroup>
+        <Expandable toggleText="Advanced">


Please put a data-test-id on the toggler.

bipuladh · 2020-04-14T13:19:48Z

...es/local-storage-operator-plugin/src/components/local-volume-set/create-local-volume-set.tsx

+            type={TextInputTypes.text}
+            id="create-lvs--storage-class-name"
+            value={storageClassName}
+            onChange={setStorageClassName}


This would show some errors in the console. onChange send two params whereas setStorageClass only accepts one.

- Local Volume Set is the new CR to be introduced in Local Stoarge Operator. - Local Volume Set will allow to filter a set of storage volumes, group them and create a dedicated storage class to consume storage for them.

bipuladh · 2020-04-14T14:17:46Z

/lgtm

openshift-ci-robot · 2020-04-14T14:18:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afreen23, bipuladh, rawagner

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/OWNERS~~ [rawagner]

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 requested review from bipuladh and TheRealJon April 13, 2020 06:53

openshift-ci-robot added the component/core Related to console core functionality label Apr 13, 2020

afreen23 force-pushed the lso-local-volume-set branch 2 times, most recently from 019839b to 50115c9 Compare April 13, 2020 07:03

bipuladh suggested changes Apr 13, 2020

View reviewed changes

bipuladh reviewed Apr 13, 2020

View reviewed changes

afreen23 commented Apr 13, 2020

View reviewed changes

afreen23 force-pushed the lso-local-volume-set branch 2 times, most recently from 39c2e2f to f8bd0d2 Compare April 13, 2020 10:56

bipuladh mentioned this pull request Apr 13, 2020

Refactored Node Selection Page for OCS Installation Flow #5017

Merged

bipuladh suggested changes Apr 13, 2020

View reviewed changes

afreen23 force-pushed the lso-local-volume-set branch from f8bd0d2 to da9f1ae Compare April 14, 2020 12:08

bipuladh reviewed Apr 14, 2020

View reviewed changes

rawagner reviewed Apr 14, 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 Apr 14, 2020

bipuladh reviewed Apr 14, 2020

View reviewed changes

Adds Local Volume Set Creation Form and enables LSO Plugin

0c2991b

- Local Volume Set is the new CR to be introduced in Local Stoarge Operator. - Local Volume Set will allow to filter a set of storage volumes, group them and create a dedicated storage class to consume storage for them.

afreen23 force-pushed the lso-local-volume-set branch from da9f1ae to 0c2991b Compare April 14, 2020 14:16

openshift-ci-robot assigned bipuladh Apr 14, 2020

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

openshift-merge-robot merged commit 180113b into openshift:master Apr 14, 2020

spadgett added this to the v4.5 milestone Apr 16, 2020

	const CreateLocalVolumeSet: React.FC = withHandlePromise((props: CreateLocalVolumeSetProps) => {
	const CreateLocalVolumeSet: React.FC = withHandlePromise<CreateLocalVolumeSetProps>((props) => {

	const nodeTaints = node?.spec?.taints?.length ?? 0;
	const nodeTaints = node.spec?.taints?.length ?? 0;

	componentProps: { data: NodeKind[]; filters: { name: string } };
	componentProps: { data: NodeKind[]; filters: { name: string, labels: { all: string[] } } };

	selected: !!rows?.[uid]?.selected,
	selected: rows?.[uid]?.selected,

	deviceTypes: ('RawDisk' \| 'Partition')[];
	deviceTypes: ('RawDisk' \| 'Partition')[];

		const [volumeType, setVolumeType] = React.useState('SSD');
		const [volumeMode, setVolumeMode] = React.useState('Block');

	const [allSelected, setAllSelected] = React.useState(null);
	const [allSelected, setAllSelected] = React.useState<boolean>(null);

Adds Local Volume Set Creation Form and enables LSO Plugin #5014

Adds Local Volume Set Creation Form and enables LSO Plugin #5014

Conversation

afreen23 commented Apr 13, 2020 • edited

gnehapk commented Apr 13, 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

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

bipuladh Apr 13, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afreen23 Apr 13, 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

rawagner commented Apr 14, 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

bipuladh commented Apr 14, 2020

openshift-ci-robot commented Apr 14, 2020

afreen23 commented Apr 13, 2020 •

edited

bipuladh Apr 13, 2020 •

edited

afreen23 Apr 13, 2020 •

edited