StoragePool creation changes for new 2 Nodes + 1 Arbiter cluster #2154

aruniiird · 2025-07-17T05:45:43Z

When we detect a 2 Nodes + 1 Arbiter cluster, we should only show, '2-way Replication' in 'Data protection policy' list

How is '2 Nodes + 1 Arbiter' cluster detected?
There is an entry in 'Infrastructure' CR's controlPlaneTopology status, value should be HighlyAvailableArbiter.

aruniiird · 2025-07-17T05:48:19Z

packages/ocs/storage-pool/body.tsx

+      if (state.isTwoNodeOneArbiterCluster) {
+        return replica === '2';
+      } else if (state.isArbiterCluster) {
+        return replica === '4';
+      } else {
+        return replica !== '4';
+      }


The other way of writting the same looks like below,

(state.isTwoNodeOneArbiterCluster && replica === '2') || (state.isArbiterCluster && !state.isTwoNodeOneArbiterCluster && replica === '4') || (!state.isArbiterCluster && !state.isTwoNodeOneArbiterCluster && replica !== '4')

A simple if condition seemed to be much readable and understandable code.
Please let me know your thoughts

packages/ocs/storage-pool/body.tsx

openshift-ci · 2025-07-20T14:30:14Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aruniiird
Once this PR has been reviewed and has the lgtm label, please assign alfonsomthd for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

packages/ocs/storage-pool/body.tsx

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

...orage-system/create-storage-system-steps/capacity-and-nodes-step/capacity-and-nodes-step.tsx

alfonsomthd · 2025-07-22T09:11:36Z

...em/create-storage-system-steps/create-local-volume-set-step/create-local-volume-set-step.tsx

+        <p className="pf-v5-u-pt-sm">
+          <strong>{t('Note:')} </strong>


Suggested change



{t('Note:')} 



{t('Note:')}

This is not my change, I just enclosed it under a condition.
We don't have to show this Note/Text if it is a TNA cluster.
I think  tag is only for the Note: section (of the message), rest of the message should come in normal way.

Better to use a PF class.

alfonsomthd · 2025-07-22T09:17:21Z

packages/shared/src/types/storage.ts

+  };
+};
+
+export type PoolSpecType = {


Instead of creating a new type, please complete the missing spec in https://github.com/alfonsomthd/odf-console/blob/master/packages/ocs/types.ts#L13

BTW the Type siffix for types is redundant (not necessary).

This is not yet addressed

packages/shared/src/utils/common.ts

When we detect a 2 Nodes + 1 Arbiter cluster, we should only show, '2-way Replication' in 'Data protection policy' list How is '2 Nodes + 1 Arbiter' cluster detected? There is an entry in 'Infrastructure' CR's `controlPlaneTopology` status, value should be `HighlyAvailableArbiter`. Added changes for 'StorageSystem' creation steps. Signed-off-by: Arun Kumar Mohan <[email protected]>

aruniiird · 2025-07-23T11:34:31Z

Currently I have TWO issues,
a. The green arbiter label does not have the expected color and not aligned according to the UX design

UX design

Actual UI

b. Second issue is when you click on the Sortable arrow alonge the Name label (see the above actual UI screenshot), it is going in to a rotating state with a message PersistentVolumes are being provisioned on the selected nodes.

alfonsomthd · 2025-07-23T12:06:53Z

packages/ocs/storage-pool/body.tsx

@@ -83,6 +87,12 @@ export const StoragePoolStatus: React.FC<StoragePoolStatusProps> = ({
  );
 };

+export const isTwoNodePlusArbiterTopology = (


Either move it to packages/shared/src/utils/common.ts (preferred) or remove the export as it is used in the same file.

alfonsomthd · 2025-07-23T12:13:57Z

...orage-system/create-storage-system-steps/capacity-and-nodes-step/capacity-and-nodes-step.tsx

+          {isTwoNodesOneArbiterClusterEnabled && (
+            <GridItem span={10}>
+              <Alert
+                title="2-Nodes and 1-Arbiter setup detected"


Suggested change

title="2-Nodes and 1-Arbiter setup detected"

title={t('2-Nodes and 1-Arbiter setup detected')}

alfonsomthd · 2025-07-23T12:15:06Z

...orage-system/create-storage-system-steps/capacity-and-nodes-step/capacity-and-nodes-step.tsx

+            <GridItem span={10}>
+              <Alert
+                title="2-Nodes and 1-Arbiter setup detected"
+                variant="warning"


Suggested change

variant="warning"

variant={AlertVariant.warning}

alfonsomthd · 2025-07-23T12:23:04Z

...-storage-system/create-storage-system-steps/capacity-and-nodes-step/selected-nodes-table.tsx

+  const rolesArr = roles as string[];
+  const isArbiter = rolesArr.some((r) => r === 'arbiter');


Suggested change

const rolesArr = roles as string[];

const isArbiter = rolesArr.some((r) => r === 'arbiter');

const isArbiter = (roles as WizardNodeState['roles']).some((r) => r === 'arbiter');

alfonsomthd · 2025-07-23T12:24:22Z

...-storage-system/create-storage-system-steps/capacity-and-nodes-step/selected-nodes-table.tsx

@@ -63,6 +67,11 @@ const SelectedNodesTableRow = ({ obj, activeColumnIDs }) => {
          resourceModel={NodeModel}
          resourceName={name}
        />
+        {isArbiter && (
+          <Label color="green" variant="filled">
+            Arbiter


Suggested change

Arbiter

{t('Arbiter')}

alfonsomthd · 2025-07-23T12:26:05Z

...em/create-storage-system-steps/create-local-volume-set-step/create-local-volume-set-step.tsx

@@ -95,6 +96,7 @@ const ConfirmationModal: React.FC<ConfirmationModalProps> = ({
  stepIdReached,
  ns,
  nodes,
+  isThisTwoNodesOneArbiterCluster,


Suggested change

isThisTwoNodesOneArbiterCluster,

isTwoNodesOneArbiterCluster,

alfonsomthd · 2025-07-23T12:26:54Z

...em/create-storage-system-steps/create-local-volume-set-step/create-local-volume-set-step.tsx

@@ -346,6 +361,7 @@ export const CreateLocalVolumeSet: React.FC<CreateLocalVolumeSetProps> = ({
          setErrorMessage={setLvsError}
          storageClassName={storageClass.name}
          stepIdReached={stepIdReached}
+          isThisTwoNodesOneArbiterCluster={isTwoNodesOneArbiterCluster}


Suggested change

isThisTwoNodesOneArbiterCluster={isTwoNodesOneArbiterCluster}

isTwoNodesOneArbiterCluster={isTwoNodesOneArbiterCluster}

alfonsomthd · 2025-07-23T12:33:34Z

packages/odf/components/utils/common.ts

+  const minNodes = isTwoNodesOneArbiterClusterDetected
+    ? MINIMUM_NODES_FOR_TNA_CLUSTER
+    : MINIMUM_NODES;


I see this logic repeated several times: you can create a shared util (DRY principle).

alfonsomthd · 2025-07-23T12:35:49Z

packages/odf/components/utils/common.ts

+  // if it is a TNA cluster, set replica count to 2
+  const deviceSetReplica: number = isTwoNodesOneArbiterClusterEnabled
+    ? 2
+    : getDeviceSetReplica(stretchClusterChecked, flexibleScaling, nodes);


IMHO this logic should be inside getDeviceSetReplica to avoid potential errors when this function is called in other places.

alfonsomthd

Left some comments.

openshift-ci · 2025-07-23T12:49:28Z

@aruniiird: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/odf-console-e2e-aws	`d6f2525`	link	true	`/test odf-console-e2e-aws`

Full PR test history. Your PR 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-sigs/prow repository. I understand the commands that are listed here.

SanjalKatiyar · 2025-07-23T13:04:22Z

Couple of things:

On "Create local volume set" step we should display two-node-arbiter detected alert as well (this flow was missed by UX team entirely).
On "Capacity and nodes" step, message u r showing under "Selected nodes" is wrong. Arbiter shouldn't be included as part of CPU/Memory calculation, also arbiter node won't be labelled as well.

aruniiird commented Jul 17, 2025

View reviewed changes

TimothyAsirJeyasing reviewed Jul 17, 2025

View reviewed changes

packages/ocs/storage-pool/body.tsx Outdated Show resolved Hide resolved

aruniiird force-pushed the storage-pool-creation-ui-changes-for-2node-1arb-cluster-setup branch from 7970644 to 259e6c7 Compare July 20, 2025 14:30

aruniiird force-pushed the storage-pool-creation-ui-changes-for-2node-1arb-cluster-setup branch from 259e6c7 to cd0d893 Compare July 22, 2025 08:35

openshift-merge-robot added the needs-rebase label Jul 22, 2025

alfonsomthd requested review from alfonsomthd, SanjalKatiyar and bipuladh July 22, 2025 08:38

alfonsomthd reviewed Jul 22, 2025

View reviewed changes

packages/ocs/storage-pool/body.tsx Outdated Show resolved Hide resolved

alfonsomthd reviewed Jul 22, 2025

View reviewed changes

packages/odf/components/create-storage-system/create-steps.tsx Outdated Show resolved Hide resolved

alfonsomthd reviewed Jul 22, 2025

View reviewed changes

...orage-system/create-storage-system-steps/capacity-and-nodes-step/capacity-and-nodes-step.tsx Outdated Show resolved Hide resolved

alfonsomthd reviewed Jul 22, 2025

View reviewed changes

packages/shared/src/utils/common.ts Outdated Show resolved Hide resolved

aruniiird force-pushed the storage-pool-creation-ui-changes-for-2node-1arb-cluster-setup branch from cd0d893 to 72d3d9b Compare July 22, 2025 12:02

openshift-merge-robot removed the needs-rebase label Jul 22, 2025

aruniiird force-pushed the storage-pool-creation-ui-changes-for-2node-1arb-cluster-setup branch 3 times, most recently from 2ada5c3 to c1f916f Compare July 23, 2025 10:59

aruniiird force-pushed the storage-pool-creation-ui-changes-for-2node-1arb-cluster-setup branch from c1f916f to d6f2525 Compare July 23, 2025 11:10

alfonsomthd reviewed Jul 23, 2025

View reviewed changes

	title="2-Nodes and 1-Arbiter setup detected"
	title={t('2-Nodes and 1-Arbiter setup detected')}

		const rolesArr = roles as string[];
		const isArbiter = rolesArr.some((r) => r === 'arbiter');

	const rolesArr = roles as string[];
	const isArbiter = rolesArr.some((r) => r === 'arbiter');
	const isArbiter = (roles as WizardNodeState['roles']).some((r) => r === 'arbiter');

	isThisTwoNodesOneArbiterCluster,
	isTwoNodesOneArbiterCluster,

	isThisTwoNodesOneArbiterCluster={isTwoNodesOneArbiterCluster}
	isTwoNodesOneArbiterCluster={isTwoNodesOneArbiterCluster}

StoragePool creation changes for new 2 Nodes + 1 Arbiter cluster #2154

Are you sure you want to change the base?

StoragePool creation changes for new 2 Nodes + 1 Arbiter cluster #2154

Uh oh!

Conversation

aruniiird commented Jul 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openshift-ci bot commented Jul 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aruniiird commented Jul 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alfonsomthd Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alfonsomthd left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jul 23, 2025

Uh oh!

SanjalKatiyar commented Jul 23, 2025

Uh oh!

Uh oh!

alfonsomthd Jul 23, 2025 •

edited

Loading