Epic: Support for multiple deviceClass on baremetal clusters #1662

vbnrh · 2024-11-07T16:50:50Z

https://www.figma.com/proto/s4m2DvEQXcqpl8LEYGRIvw/4795%3A-Support-for-multiple-device-classes-in-the-same-cluster?page-id=8722%3A11106&node-id=8727-18578&node-type=frame&viewport=239%2C165%2C0.05&t=5AAuMMTCAVb0rVTK-1&scaling=scale-down&content-scaling=fixed&starting-point-node-id=8722%3A11107

vbnrh · 2024-11-08T06:23:45Z

The alert that says, it may increase your expenses has been removed

alfonsomthd · 2024-11-08T07:25:21Z

The alert that says, it may increase your expenses has been removed

@vbnrh Then the screenshots are outdated, right? Can you put an updated one?
Also, I would put Fixes: <Jira-issue that contains the figma link.

vbnrh · 2024-11-08T08:28:35Z

The alert that says, it may increase your expenses has been removed

@vbnrh Then the screenshots are outdated, right? Can you put an updated one? Also, I would put Fixes: <Jira-issue that contains the figma link.

Its the same, just the message on top got removed.
Figma link the description

vbnrh · 2024-11-08T08:29:24Z

https://issues.redhat.com/browse/RHSTOR-4795?focusedId=25317056&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel

packages/odf/components/actions/csv-actions.ts

packages/odf/components/attach-storage-storagesystem/state.ts

packages/odf/components/attach-storage-storagesystem/attach-storage.tsx

SanjalKatiyar · 2024-11-11T06:39:14Z

packages/odf/components/attach-storage-storagesystem/lso-storageclass-dropdown.tsx

+  const deviceSets: DeviceSet[] = storageCluster?.spec?.storageDeviceSets || [];
+  const installStorageClass =
+    deviceSets?.[0]?.dataPVCTemplate?.spec?.storageClassName;
+  const preSelectionFilter = React.useCallback(
+    (storageClasses: StorageClassResourceKind[]) =>
+      storageClasses.find((sc) => getName(sc) === installStorageClass),
+    [installStorageClass]
+  );


This check still doesn't make sense. Requirement of this feature is to use a "new" StorageClass, so we shouldn't auto-select "existing" StorageClass.

If anything we should filter out the "existing" StorageClasses from the dropdown.

SanjalKatiyar · 2024-11-11T06:43:57Z

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

+  const [csv, csvLoaded, csvLoadError] = useFetchCsv({
+    specName: LSO_OPERATOR,
+  });


don't fetch CSV per row... do it once on the table level and pass it down to each row...

SanjalKatiyar · 2024-11-11T06:44:34Z

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

+    customKebabItems.push({
+      key: 'ATTACH_STORAGE',
+      value: t('Attach Storage'),
+      component: undefined,


"undefined" ??

Suggested change

component: undefined,

component: undefined,

There is no component to render, need to navigate to url

Component is field is somehow required even though it is optional in the properties

Fixed it, the typing was not done properly

SanjalKatiyar · 2024-11-11T06:46:08Z

packages/shared/src/kebab/kebab.tsx

+    } else if (actionKey === 'ATTACH_STORAGE') {
+      const url = `/odf/system/ns/${getNamespace(resource)}/${referenceForModel(
+        StorageClusterModel
+      )}/${getName(resource)}/~attachstorage`;
+      navigate(url);


this is shared directory, we can't have specific checks like actionKey === 'ATTACH_STORAGE' here...

SanjalKatiyar · 2024-11-11T07:28:08Z

On Expandstorage UI

Show only LSO storageClasses that are not in the deviceSet

This make sense, LGTM.

on AddCapacity UI

Show only LSO storageClasses that is on device Set

Better to confirm with Eran whether we want to touch existing "Add Capacity" UI or keep that as it is (so that existing non-advanced user won't get confused).
(I personally don't want to make any changes to existing "Add Capacity" UI).

parth-gr · 2024-11-11T07:55:50Z

Better to confirm with Eran whether we want to touch existing "Add Capacity" UI or keep that as it is (so that existing non-advanced user won't get confused).
(I personally don't want to make any changes to existing "Add Capacity" UI)

IMO for LSO usescase we should not add the new deviceSet using "Add Capacity", so we do not use the new LSO storageclass,
If we add new deviceSet from there the same device classes OSDs will be generated,
But the OSDs originally from differ in configuration and if used together, might be issues with balancing and performance.

But will open a thread for it

SanjalKatiyar · 2024-11-11T11:14:44Z

packages/odf/components/attach-storage-storagesystem/lso-storageclass-dropdown.tsx

+  const getStorageClassesNotInDeviceSet = React.useCallback(
+    (
+      storageClasses: StorageClassResourceKind[],
+      deviceSets: DeviceSet[]
+    ): string[] => {
+      const allStorageClasses = storageClasses.map(getName);
+      const deviceSetStorageClasses = deviceSets
+        .map((ds) => ds.dataPVCTemplate.spec.storageClassName)
+        .filter((name): name is string => Boolean(name));
+
+      return allStorageClasses.filter(
+        (scName) => !deviceSetStorageClasses.includes(scName)
+      );
+    },
+    [existingStorageClasses, storageCluster]
+  );
+
+  const preSelectionFilter = React.useCallback(
+    (storageClasses: StorageClassResourceKind[]) => {
+      const storageClassesNotInDeviceSet = getStorageClassesNotInDeviceSet(
+        storageClasses,
+        storageCluster?.spec?.storageDeviceSets || []
+      );
+      return storageClassesNotInDeviceSet[0];
+    },
+    [[getStorageClassesNotInDeviceSet]]
+  );


storageClassDropdownFilter is already filtering all the existing StorageClasses, you directly select 0th of that list.

Suggested change

const getStorageClassesNotInDeviceSet = React.useCallback(

(

storageClasses: StorageClassResourceKind[],

deviceSets: DeviceSet[]

): string[] => {

const allStorageClasses = storageClasses.map(getName);

const deviceSetStorageClasses = deviceSets

.map((ds) => ds.dataPVCTemplate.spec.storageClassName)

.filter((name): name is string => Boolean(name));

return allStorageClasses.filter(

(scName) => !deviceSetStorageClasses.includes(scName)

);

},

[existingStorageClasses, storageCluster]

);

const preSelectionFilter = React.useCallback(

(storageClasses: StorageClassResourceKind[]) => {

const storageClassesNotInDeviceSet = getStorageClassesNotInDeviceSet(

storageClasses,

storageCluster?.spec?.storageDeviceSets || []

);

return storageClassesNotInDeviceSet[0];

},

[[getStorageClassesNotInDeviceSet]]

);

const preSelectionFilter = React.useCallback(

(storageClasses: StorageClassResourceKind[]) => storageClasses?.[0];

},

[]

);

no need to filter again...

SanjalKatiyar · 2024-11-11T11:19:12Z

LGTM other than comment #1662 (comment).
cc @bipuladh @alfonsomthd @GowthamShanmugam for final review and tagging.

alfonsomthd · 2024-11-11T12:17:18Z

The alert that says, it may increase your expenses has been removed

@vbnrh Then the screenshots are outdated, right? Can you put an updated one? Also, I would put Fixes: <Jira-issue that contains the figma link.

Can you please upload an updated screenshot?

vbnrh · 2024-11-11T13:01:04Z

The alert that says, it may increase your expenses has been removed

@vbnrh Then the screenshots are outdated, right? Can you put an updated one? Also, I would put Fixes: <Jira-issue that contains the figma link.

Can you please upload an updated screenshot?

packages/odf/components/attach-storage-storagesystem/attach-storage.tsx

packages/odf/components/attach-storage-storagesystem/attach-storage-footer.tsx

packages/odf/components/attach-storage-storagesystem/attach-storage.tsx

packages/odf/components/attach-storage-storagesystem/lso-storageclass-dropdown.tsx

vbnrh · 2024-11-12T05:19:06Z

@bipuladh @SanjalKatiyar @alfonsomthd All comments addressed

packages/odf/components/attach-storage-storagesystem/lso-storageclass-dropdown.tsx

Signed-off-by: vbadrina <[email protected]>

vbnrh · 2024-11-13T11:43:20Z

/test odf-console-e2e-aws

vbnrh · 2024-11-14T02:44:33Z

/test odf-console-e2e-aws

parth-gr · 2024-11-14T06:53:38Z

cc @SanjalKatiyar

SanjalKatiyar · 2024-11-14T07:04:45Z

/approve
/lgtm

openshift-ci · 2024-11-14T07:04:52Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

SanjalKatiyar · 2024-11-14T10:15:37Z

/test odf-console-e2e-aws

vbnrh · 2024-11-14T12:21:16Z

/test odf-console-e2e-aws

vbnrh force-pushed the multiple-deviceclass-feature branch from d8de79c to c11f382 Compare November 8, 2024 02:21

SanjalKatiyar requested review from GowthamShanmugam, alfonsomthd, bipuladh and SanjalKatiyar and removed request for GowthamShanmugam November 8, 2024 05:46

vbnrh force-pushed the multiple-deviceclass-feature branch 10 times, most recently from a28a775 to 0a49e1d Compare November 8, 2024 12:47

SanjalKatiyar requested changes Nov 8, 2024

View reviewed changes

openshift-ci bot assigned SanjalKatiyar Nov 8, 2024

vbnrh force-pushed the multiple-deviceclass-feature branch 3 times, most recently from 81b95fe to 2655757 Compare November 11, 2024 06:23

SanjalKatiyar reviewed Nov 11, 2024

View reviewed changes

vbnrh force-pushed the multiple-deviceclass-feature branch from 2655757 to 6baf6e9 Compare November 11, 2024 11:09

SanjalKatiyar reviewed Nov 11, 2024

View reviewed changes

vbnrh force-pushed the multiple-deviceclass-feature branch from 6baf6e9 to 5b1187b Compare November 11, 2024 11:41

parth-gr mentioned this pull request Nov 11, 2024

ux: add a new expandStorage handler red-hat-storage/ocs-operator#2876

Merged

SanjalKatiyar reviewed Nov 11, 2024

View reviewed changes

packages/odf/components/attach-storage-storagesystem/attach-storage.tsx Outdated Show resolved Hide resolved

bipuladh reviewed Nov 12, 2024

View reviewed changes

vbnrh force-pushed the multiple-deviceclass-feature branch from 5b1187b to 6c5b40e Compare November 12, 2024 05:18

vbnrh force-pushed the multiple-deviceclass-feature branch from 6c5b40e to 7553da0 Compare November 12, 2024 05:44

vbnrh requested review from bipuladh and SanjalKatiyar November 12, 2024 09:41

SanjalKatiyar reviewed Nov 13, 2024

View reviewed changes

packages/odf/components/attach-storage-storagesystem/lso-storageclass-dropdown.tsx Outdated Show resolved Hide resolved

vbnrh force-pushed the multiple-deviceclass-feature branch from 7553da0 to 9a70dc0 Compare November 13, 2024 09:46

feat: add support for multiple deviceClass for baremetal

4eab654

Signed-off-by: vbadrina <[email protected]>

vbnrh force-pushed the multiple-deviceclass-feature branch from 9a70dc0 to 4eab654 Compare November 13, 2024 09:57

openshift-ci bot added the lgtm label Nov 14, 2024

openshift-ci bot added the approved label Nov 14, 2024

openshift-merge-bot bot merged commit cb13092 into red-hat-storage:master Nov 14, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Epic: Support for multiple deviceClass on baremetal clusters #1662

Epic: Support for multiple deviceClass on baremetal clusters #1662

vbnrh commented Nov 7, 2024

vbnrh commented Nov 8, 2024

alfonsomthd commented Nov 8, 2024

vbnrh commented Nov 8, 2024

vbnrh commented Nov 8, 2024

SanjalKatiyar Nov 11, 2024 •

edited

Loading

SanjalKatiyar Nov 11, 2024

vbnrh Nov 11, 2024

SanjalKatiyar Nov 11, 2024

vbnrh Nov 11, 2024

SanjalKatiyar Nov 11, 2024

vbnrh Nov 11, 2024

vbnrh Nov 11, 2024

vbnrh Nov 11, 2024

SanjalKatiyar Nov 11, 2024

vbnrh Nov 11, 2024

SanjalKatiyar commented Nov 11, 2024

parth-gr commented Nov 11, 2024

SanjalKatiyar Nov 11, 2024

SanjalKatiyar Nov 11, 2024

SanjalKatiyar commented Nov 11, 2024

alfonsomthd commented Nov 11, 2024

vbnrh commented Nov 11, 2024

vbnrh commented Nov 12, 2024

vbnrh commented Nov 13, 2024

vbnrh commented Nov 14, 2024

parth-gr commented Nov 14, 2024

SanjalKatiyar commented Nov 14, 2024

openshift-ci bot commented Nov 14, 2024

SanjalKatiyar commented Nov 14, 2024

vbnrh commented Nov 14, 2024

Epic: Support for multiple deviceClass on baremetal clusters #1662

Epic: Support for multiple deviceClass on baremetal clusters #1662

Conversation

vbnrh commented Nov 7, 2024

vbnrh commented Nov 8, 2024

alfonsomthd commented Nov 8, 2024

vbnrh commented Nov 8, 2024

vbnrh commented Nov 8, 2024

SanjalKatiyar Nov 11, 2024 • 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

SanjalKatiyar commented Nov 11, 2024

parth-gr commented Nov 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SanjalKatiyar commented Nov 11, 2024

alfonsomthd commented Nov 11, 2024

vbnrh commented Nov 11, 2024

vbnrh commented Nov 12, 2024

vbnrh commented Nov 13, 2024

vbnrh commented Nov 14, 2024

parth-gr commented Nov 14, 2024

SanjalKatiyar commented Nov 14, 2024

openshift-ci bot commented Nov 14, 2024

SanjalKatiyar commented Nov 14, 2024

vbnrh commented Nov 14, 2024

SanjalKatiyar Nov 11, 2024 •

edited

Loading