OCS installation changes for baremetal infra #5016

gnehapk · 2020-04-13T08:50:02Z

With OCS Baremetal cluster support in picture, following changes are required in ocs installation and add-capacity view -
For no-provisioner storage class(created by LSO flow), requested capacity should not be shown as dynamically provision of storage is not possible

Views look like below(for baremetal infra) -

afreen23 · 2020-04-13T08:57:54Z

A screenshot and description might help.

cloudbehl · 2020-04-13T12:16:31Z

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

What if new PV's comes in while being on a page?

cloudbehl · 2020-04-13T12:17:32Z

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

https://github.com/openshift/console/pull/5016/files#diff-7879af0cb57fd55121c4d9598ff585ecR23

Seems like you have pasted wrong link here @cloudbehl

getPVStorageClass function on line 23.

The GitHub seems buggy in this case.

cloudbehl · 2020-04-13T12:18:36Z

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

use optional chaining

cloudbehl · 2020-04-13T12:22:20Z

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

Can you reduce the complexity?

Also, use optional chaining.

cloudbehl · 2020-04-13T12:23:29Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx

We should show Not available

cloudbehl · 2020-04-13T12:25:49Z

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

if we are not able to get the capacity then ideally it should be not available

cloudbehl · 2020-04-13T12:31:39Z

frontend/public/module/k8s/types.ts

why this change? We should use StorageClassKind object?

cloudbehl · 2020-04-13T12:33:01Z

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

Shouldn't catch be after then?

Suggested change

.catch(() => [])

.catch(() => [])

Not necessary

cloudbehl · 2020-04-13T12:35:43Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx

Suggested change

{isNoProvisionerSC && (

{isNoProvisionerSC ? () : ()}

bipuladh · 2020-04-13T12:48:55Z

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

Suggested change

.catch(() => [])

This catch is not necessary.

bipuladh · 2020-04-13T12:49:55Z

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

use classNames

bipuladh · 2020-04-13T17:16:43Z

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

Suggested change

if (_.isEqual(provisioner, 'kubernetes.io/no-provisioner')) {

if (provisioner === 'kubernetes.io/no-provisioner') {

is there any reason of using this over _.isEqual

bipuladh · 2020-04-13T17:22:21Z

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

Any reason we are using '0 TiB' as the initial value? Why TiB?

bipuladh · 2020-04-13T17:25:45Z

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

Suggested change

setPVsAvailableCapacity(humanizeBinaryBytes(calcPVsCapacity(pvs)).string);

const availableCapacity = humanizeBinaryBytes(calcPVsCapacity(pvs)).string;

setPVsAvailableCapacity(availableCapacity);

bipuladh · 2020-04-13T17:39:51Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx

Suggested change

if (_.isEqual(provisioner, 'kubernetes.io/no-provisioner')) {

if ( provisioner === 'kubernetes.io/no-provisioner')) {

This is the second place I am seeing this being used. Consider putting no-provisioner in the consts file.

bipuladh · 2020-04-13T17:41:54Z

frontend/packages/ceph-storage-plugin/src/components/modals/storage-class-dropdown.tsx

Suggested change

(provisioner: string) => !_.get(sc, 'provisioner').includes(provisioner),

(provisioner: string) => ! sc?.provisioner?.includes(provisioner),

bipuladh · 2020-04-13T17:49:06Z

frontend/packages/ceph-storage-plugin/src/components/modals/storage-class-dropdown.tsx

This would cause problems in the node-list page where you're directly applying this to the setState and it only expects one argument and not two.

handleStorageClass is called on onChange method and it accepts 2 params. Look carefully.

Please look at https://github.com/openshift/console/blob/master/frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx#L329
It's directly connected to the dispatcher which takes only 1 prop. This is an issue.

It has been changed with this PR. See this - https://github.com/openshift/console/pull/5016/files#diff-10211f95caf12c0c4e086214ec83e140R326

bipuladh · 2020-04-13T17:49:27Z

frontend/packages/ceph-storage-plugin/src/components/modals/storage-class-dropdown.tsx

This will cause problems with the existing code.

same as above

https://github.com/openshift/console/blob/master/frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx#L329

bipuladh · 2020-04-13T17:51:27Z

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

Consider another approach for getting this information. Changing storageClass data from string to K8sCommon should do the trick.

Why do you think another approach is required here?

https://github.com/openshift/console/pull/5016/files#r407903025

bipuladh · 2020-04-14T14:53:03Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx

Suggested change

const [pvsAvailableCapacity, setPVsAvailableCapacity] = React.useState<string | number>(

const [pvsAvailableCapacity, setPVsAvailableCapacity] = React.useState<React.ReatText>(

bipuladh · 2020-04-14T14:53:38Z

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

Suggested change

const [pvsAvailableCapacity, setPVsAvailableCapacity] = React.useState<string | number>(

const [pvsAvailableCapacity, setPVsAvailableCapacity] = React.useState<React.ReactText>(

bipuladh · 2020-04-14T14:54:32Z

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

Suggested change

const [storageClass, setStorageClass] = React.useState(null);

const [storageClass, setStorageClass] = React.useState<K8sResourceCommon>(null);

bipuladh · 2020-04-14T14:57:28Z

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

Suggested change

storageClass?.provisioner === noProvisioner

? ''

: 'ceph-add-capacity_sc-dropdown__margin',

{'ceph-add-capacity_sc-dropdown__margin': storageClass?.provisioner === noProvisioner}

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

bipuladh · 2020-04-14T14:59:59Z

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

for watching, I guess we can declare this resource where it is being used. Is this used for the useK8sWatchResource hook?

This is used at two places and this separate file is being created for defining resources. Hence, its preferable to define here.

bipuladh · 2020-04-15T10:47:41Z

...ckages/ceph-storage-plugin/src/components/modals/add-capacity-modal/_add-capacity-modal.scss

Suggested change

.ceph-add-capacity_sc-dropdown {

width: 16rem;

}

.ceph-add-capacity_sc-dropdown__margin {

margin-bottom: 1rem;

}

.ceph-add-capacity_sc-dropdown {

width: 16rem;

&--margin {

margin-bottom: 1rem;

}

}

A modifier makes more sense to me here.

Ack for the modifier. But we don't need to have it nested since we are already following BEM structure

This compiles down to .cpeh-add-capacity__sc-dropdown--margin and .ceph-add-capcity__sc-dropdown. BEM is a naming conventiion. &-- helps us manage the modifier easily.
https://css-tricks.com/using-sass-control-scope-bem-naming/

I know that. I am just referring to Sam's comment here - #3927 (comment)

His comment and this is really different. I am not asking you to nest two classes. I am suggesting define a modifier block in a different manner.

bipuladh · 2020-04-15T10:49:05Z

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

I am curious. Can this field ever be not present in a StorageClassResource @cloudbehl ?

It will be present always. I will remove it.

bipuladh · 2020-04-15T10:50:25Z

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

Suggested change

storageClass?.provisioner === NO_PROVISIONER

? ''

: 'ceph-add-capacity_sc-dropdown__margin',

)}

{'ceph-add-capacity_sc-dropdown--margin': storageClass?.provisioner === NO_PROVISIONER},

)}

afreen23 · 2020-04-13T12:41:44Z

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

What about this text ? we must modify this too.

@yuvalgalanti ^

afreen23 · 2020-04-15T10:49:14Z

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

Why not leverage the enum you created here ?

Suggested change

pvsData.filter((pv) => getPVStorageClass(pv) === sc && pv.status?.phase === 'Available');

pvsData.filter((pv) => getPVStorageClass(pv) === sc && pv.status?.phase === pvsCapacityState.Available);

pvsCapacityState is for storing capacity states, not for pvs status. These are two different things.

I could have created an enum for pvs state, but it doesn't make sense create it just one state Available used at only one place.

afreen23 · 2020-04-15T10:53:11Z

...ckages/ceph-storage-plugin/src/components/modals/add-capacity-modal/_add-capacity-modal.scss

We must follow BEM convention

Suggested change

.ceph-add-capacity_sc-dropdown__margin {

.ceph-add-capacity__sc-dropdown--margin {

afreen23 · 2020-04-15T10:54:56Z

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

Move this up along with other hooks.

afreen23 · 2020-04-15T10:57:54Z

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

I see its just a string, any reason for using React.ReactText ?

it can be both, a number and string

IMO we can keep it as number only and for other states add elements conditionally.

Not available: <div className="text-muted">Not Available</div> Loading: <div className="skeleton-text"></div>

I feel, this approach looks much cleaner as we can avoid these conditional elements.

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

afreen23 · 2020-04-15T11:01:56Z

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

May be you can consider moving this to a separate component, so that PVs will be fetched only in case of baremetal.
Also its required at two places.

Ack. We can take this up in a follow up PR.

afreen23 · 2020-04-15T11:15:21Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx

Can you create an enum/obj of osd sizes in osd-size-dropdown.tsx and then use those values. I see we need these values often now.

something like this:

const SUPPORTED_OSD = { 'Small' ='512Gi', 'Medium'='2Ti', 'Large' = '4Ti', 'Baremetal' = '1' }

There is already an enum OSD_CAPACITY_SIZES for these supported sizes. Make sense to create jsut for default osd sizes i.e. 2Ti and 1

Okay but we can modify that too, lets keep all supported configuration at one place.
We can use these enums in that too and that way we only need to change at one place, when changes are required.

The structure is different and I don't think its a good idea to mix for two different things. I have already created defaultRequestSize enum for handling the default request size.

afreen23 · 2020-04-15T11:20:40Z

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

Any reason for using lodash ?

afreen23 · 2020-04-15T11:23:50Z

...ckages/ceph-storage-plugin/src/components/modals/add-capacity-modal/_add-capacity-modal.scss

correct this too

what needs to corrected here?

ceph-add-capacity__sc-dropdown , as per the convention.

afreen23 · 2020-04-15T11:26:10Z

frontend/packages/ceph-storage-plugin/src/constants/ocs-install.ts

Dont you need to put loading boxes instead of text ?

gnehapk · 2020-04-16T06:48:04Z

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

Note: ? is required as initially storageclass is not being set.

Here and at few other places too. Feel free to check.

openshift-bot · 2020-04-17T00:06:07Z

/retest

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

openshift-bot · 2020-04-17T00:20:18Z

/retest

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

openshift-bot · 2020-04-17T01:37:12Z

/retest

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

openshift-bot · 2020-04-17T01:50:09Z

/retest

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

openshift-bot · 2020-04-17T02:16:07Z

/retest

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

cloudbehl · 2020-04-17T05:14:04Z

/retest

gnehapk · 2020-04-17T05:52:56Z

/test e2e-gcp-console

bipuladh · 2020-04-17T08:06:56Z

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

Suggested change

const isBound = (pvc: K8sResourceKind) => _.get(pvc, 'status.phase') === status.BOUND;

const isBound = (pvc: K8sResourceKind) => pvc.status.phase === status.BOUND;

cloudbehl · 2020-04-17T10:55:19Z

/lgtm

openshift-ci-robot · 2020-04-17T10:55:44Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/packages/ceph-storage-plugin/OWNERS~~ [afreen23,bipuladh,cloudbehl,gnehapk]

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

gnehapk · 2020-04-17T10:56:42Z

/test e2e-gcp-console

openshift-bot · 2020-04-17T11:15:33Z

/retest

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

gnehapk · 2020-04-17T13:14:27Z

/retest

a2batic · 2020-04-17T17:01:43Z

/retest

spadgett · 2020-04-17T21:47:50Z

/hold
for #5100

spadgett · 2020-04-17T23:15:49Z

/hold cancel
/retest

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 13, 2020

openshift-ci-robot requested review from cloudbehl and spadgett April 13, 2020 08:50

openshift-ci-robot added component/ceph Related to ceph-storage-plugin component/core Related to console core functionality labels Apr 13, 2020

gnehapk force-pushed the baremetal-flows branch from 7fa73e6 to d79991a Compare April 13, 2020 09:22

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2020

gnehapk mentioned this pull request Apr 13, 2020

Refactored Node Selection Page for OCS Installation Flow #5017

Merged

cloudbehl reviewed Apr 13, 2020

View reviewed changes

bipuladh suggested changes Apr 13, 2020

View reviewed changes

gnehapk force-pushed the baremetal-flows branch from d79991a to 90befa9 Compare April 14, 2020 14:29

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

gnehapk force-pushed the baremetal-flows branch 2 times, most recently from ff705bc to 0adb062 Compare April 14, 2020 15:01

bipuladh suggested changes Apr 14, 2020

View reviewed changes

gnehapk force-pushed the baremetal-flows branch 2 times, most recently from 21b3f14 to cf6273f Compare April 15, 2020 10:43

gnehapk changed the title ~~[WIP] OCS installation changes for baremetal infra~~ OCS installation changes for baremetal infra Apr 15, 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 Apr 15, 2020

bipuladh reviewed Apr 15, 2020

View reviewed changes

afreen23 reviewed Apr 15, 2020

View reviewed changes

frontend/packages/ceph-storage-plugin/src/constants/ocs-install.ts Outdated

Copy link

afreen23 Apr 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont you need to put loading boxes instead of text ?

gnehapk force-pushed the baremetal-flows branch 3 times, most recently from 22bde29 to 31c83d3 Compare April 16, 2020 06:46

gnehapk commented Apr 16, 2020

View reviewed changes

gnehapk force-pushed the baremetal-flows branch from 31c83d3 to a6cd590 Compare April 16, 2020 06:58

gnehapk force-pushed the baremetal-flows branch from 9eca200 to 2526bf8 Compare April 17, 2020 02:27

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

gnehapk force-pushed the baremetal-flows branch from 2526bf8 to 169e785 Compare April 17, 2020 04:27

cloudbehl approved these changes Apr 17, 2020

View reviewed changes

gnehapk force-pushed the baremetal-flows branch from 169e785 to 219d509 Compare April 17, 2020 07:52

bipuladh suggested changes Apr 17, 2020

View reviewed changes

OCS installation changes for baremetal infra

351d269

gnehapk force-pushed the baremetal-flows branch from 219d509 to 351d269 Compare April 17, 2020 08:24

bipuladh approved these changes Apr 17, 2020

View reviewed changes

afreen23 approved these changes Apr 17, 2020

View reviewed changes

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

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2020

openshift-merge-robot merged commit df9cfad into openshift:master Apr 18, 2020

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

	if (_.isEqual(provisioner, 'kubernetes.io/no-provisioner')) {
	if (provisioner === 'kubernetes.io/no-provisioner') {

	setPVsAvailableCapacity(humanizeBinaryBytes(calcPVsCapacity(pvs)).string);
	const availableCapacity = humanizeBinaryBytes(calcPVsCapacity(pvs)).string;
	setPVsAvailableCapacity(availableCapacity);

	if (_.isEqual(provisioner, 'kubernetes.io/no-provisioner')) {
	if ( provisioner === 'kubernetes.io/no-provisioner')) {

	(provisioner: string) => !_.get(sc, 'provisioner').includes(provisioner),
	(provisioner: string) => ! sc?.provisioner?.includes(provisioner),

	const [pvsAvailableCapacity, setPVsAvailableCapacity] = React.useState<string \| number>(
	const [pvsAvailableCapacity, setPVsAvailableCapacity] = React.useState<React.ReatText>(

	const [storageClass, setStorageClass] = React.useState(null);
	const [storageClass, setStorageClass] = React.useState<K8sResourceCommon>(null);

-              storageClass?.provisioner === noProvisioner
-                ? ''
-                : 'ceph-add-capacity_sc-dropdown__margin',
+              {'ceph-add-capacity_sc-dropdown__margin': storageClass?.provisioner === noProvisioner}

-.ceph-add-capacity_sc-dropdown {
-  width: 16rem;
-}
-.ceph-add-capacity_sc-dropdown__margin {
-  margin-bottom: 1rem;
-}
+.ceph-add-capacity_sc-dropdown {
+  width: 16rem;
+  &--margin {
+    margin-bottom: 1rem;
+    }
+}

-              storageClass?.provisioner === NO_PROVISIONER
-                ? ''
-                : 'ceph-add-capacity_sc-dropdown__margin',
-            )}
+                 {'ceph-add-capacity_sc-dropdown--margin': storageClass?.provisioner === NO_PROVISIONER},
+            )}

	pvsData.filter((pv) => getPVStorageClass(pv) === sc && pv.status?.phase === 'Available');
	pvsData.filter((pv) => getPVStorageClass(pv) === sc && pv.status?.phase === pvsCapacityState.Available);

	.ceph-add-capacity_sc-dropdown__margin {
	.ceph-add-capacity__sc-dropdown--margin {

	const isBound = (pvc: K8sResourceKind) => _.get(pvc, 'status.phase') === status.BOUND;
	const isBound = (pvc: K8sResourceKind) => pvc.status.phase === status.BOUND;

OCS installation changes for baremetal infra #5016

OCS installation changes for baremetal infra #5016

Uh oh!

Conversation

gnehapk commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

afreen23 commented Apr 13, 2020

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!

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!

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!

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!

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!

Choose a reason for hiding this comment

gnehapk commented Apr 13, 2020 •

edited

Loading

gnehapk Apr 15, 2020 •

edited

Loading