Bug 1871646: Fix location of Minimal Deployment message and calculation for Internal Mode deployment #6404

bipuladh · 2020-08-23T18:51:55Z

Fixes calculation for Internal mode where if the total CPU < 30 then only the minimal deployment will occur.
Fixes calculation for an Attached mode where if the CPU per node < 10 then the minimal deployment will occur.
Moves the message as an alert to the bottom.

openshift-ci-robot · 2020-08-23T18:51:56Z

@bipuladh: Bugzilla bug 1871646 is in a bug group that is not in the allowed groups for this repo.
Allowed groups for this repo are:

qe_staff
redhat

In response to this:

Bug 1871646: Fix location of Minimal Deployment message and calculation for Internal Mode deployment

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/test-infra repository.

bipuladh · 2020-08-23T18:55:12Z

/bugzilla refresh

openshift-ci-robot · 2020-08-23T18:55:18Z

@bipuladh: This pull request references Bugzilla bug 1871646, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

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/test-infra repository.

cloudbehl · 2020-08-24T05:10:30Z

How does total make sense if the node itself doesn't satisfy the criteria? Let say you have 15 nodes with 2vcpu's?

bipuladh · 2020-08-24T05:16:53Z

@cloudbehl As per @oritwas situations like that should work for Internal Mode deployments.

cloudbehl · 2020-08-24T05:30:19Z

frontend/packages/ceph-storage-plugin/src/utils/common-ocs-install-el.tsx

+    isInline
+  >
+    {isInternalMode
+      ? 'The selected nodes do not match the OCS storage cluster minimum recommended requirements of an aggregated 30 CPUs. If the selection won’t be modified, a minimal cluster will be deployed and may face some performance issues.'


Memory requirements are not mentioned.

As we are checking the memory requirements in shouldDeployInternalAsMinimal user will not able to make out why error is coming if the memory requirements are failing.

@shirimordechay please have a look at this

If there is a memory requirement we should mention it, I think the message should fit the user selection. After the "recommended requirement of..." we should add the requirement that doesn't fit. the rest of the warning text will stay the same

In this use case, the text will be: ‘The selected nodes do not match the OCS storage cluster minimum recommended requirements of a [total 60 GiB Memory]. If the selection won’t be modified, a minimal cluster will be deployed and may face some performance issues.’

For cloud/ LSO requirement text should be changed accordingly.

cloudbehl · 2020-08-24T08:39:43Z

frontend/packages/ceph-storage-plugin/src/utils/install.ts

@@ -58,12 +58,30 @@ export const getAssociatedNodes = (pvs: K8sResourceKind[]): string[] => {
  return Array.from(nodes);
 };

+export const shouldDeployInternalAsMinimal = (nodes: NodeKind[]) => {


Suggested change

export const shouldDeployInternalAsMinimal = (nodes: NodeKind[]) => {

export const DeployCloudAsMinimal = (nodes: NodeKind[]) => {

you can deploy internal mode in VMWare as well.

It's not a react component. (first character no need to be uppercase)

cloudbehl · 2020-08-24T08:39:58Z

frontend/packages/ceph-storage-plugin/src/utils/install.ts

+  );
+  return totalCPU < 30 || totalMemory < 60;
+};
+
 export const shouldDeployMinimally = (nodes: NodeKind[]) =>


Suggested change

export const shouldDeployMinimally = (nodes: NodeKind[]) =>

export const DeployBaremetalAsMinimal = (nodes: NodeKind[]) =>

You can deploy Attached devices in AWS as well.

I will keep it as shouldDeployAttachedAsMinimal to keep it consistent with the other function

frontend/packages/ceph-storage-plugin/src/utils/install.ts

cloudbehl · 2020-08-24T14:13:20Z

frontend/packages/ceph-storage-plugin/src/utils/install.ts

  nodes.reduce((acc, curr) => {
    const cpus = humanizeCpuCores(getNodeCPUCapacity(curr)).value;
    const memoryRaw = getNodeAllocatableMemory(curr);
    const memory = humanizeBinaryBytes(convertToBaseValue(memoryRaw)).value;
-    if (memory < 60 || cpus < 16) {
+    if (memory < 60 || cpus < 10) {


same as above

Same as above

gnehapk · 2020-08-25T08:24:46Z

@bipuladh As discussed, please change the getOCSRequestDatafunction, as its replacing the spec property in case of minimal deployement and hence, breaking the ocs cluster creation on attached devices.

bipuladh · 2020-08-25T08:39:35Z

@bipuladh As discussed, please change the getOCSRequestDatafunction, as its replacing the spec property in case of minimal deployement and hence, breaking the ocs cluster creation on attached devices.

Noted and added.

gnehapk · 2020-08-25T09:18:36Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/create-form.tsx

+    if (timeoutID.current !== null) {
+      clearTimeout(timeoutID.current);
+    }
+    if (nodes.length >= 3) {


Suggested change

if (nodes.length >= 3) {

if (nodes.length >= minSelectedNode) {

gnehapk · 2020-08-25T09:59:38Z

/lgtm

bipuladh · 2020-08-25T10:19:59Z

/hold

…al Mode deployment

openshift-ci-robot · 2020-08-26T11:05:03Z

@bipuladh: This pull request references Bugzilla bug 1871646, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1871646: Fix location of Minimal Deployment message and calculation for Internal Mode deployment

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/test-infra repository.

bipuladh · 2020-08-26T11:05:43Z

/hold cancel

gnehapk · 2020-08-26T12:15:04Z

/lgtm

openshift-ci-robot · 2020-08-26T12:15:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bipuladh, 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~~ [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

openshift-bot · 2020-08-26T15:20:40Z

/retest

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

openshift-bot · 2020-08-26T19:14:29Z

/retest

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

openshift-ci-robot · 2020-08-26T21:39:31Z

@bipuladh: All pull requests linked via external trackers have merged:

openshift/console#6404

Bugzilla bug 1871646 has been moved to the MODIFIED state.

In response to this:

Bug 1871646: Fix location of Minimal Deployment message and calculation for Internal Mode deployment

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/test-infra repository.

openshift-ci-robot requested review from rawagner and shyRozen August 23, 2020 18:52

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

openshift-ci-robot added the bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. label Aug 23, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Aug 23, 2020

cloudbehl reviewed Aug 24, 2020

View reviewed changes

bipuladh force-pushed the fix_minimal_deployment branch 2 times, most recently from 30528d8 to 48da5ea Compare August 24, 2020 13:55

cloudbehl reviewed Aug 24, 2020

View reviewed changes

bipuladh force-pushed the fix_minimal_deployment branch from 48da5ea to 3ec2809 Compare August 25, 2020 08:37

cloudbehl approved these changes Aug 25, 2020

View reviewed changes

gnehapk reviewed Aug 25, 2020

View reviewed changes

bipuladh force-pushed the fix_minimal_deployment branch from 3ec2809 to cd66904 Compare August 25, 2020 09:26

openshift-ci-robot assigned gnehapk Aug 25, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 25, 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 Aug 25, 2020

gnehapk mentioned this pull request Aug 25, 2020

Bug 1872279: Added e2e tests for ocs install for Attached Devices #6426

Merged

bipuladh force-pushed the fix_minimal_deployment branch from cd66904 to 0bfb707 Compare August 26, 2020 10:52

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

Fix location of Minimal Deployment message and calculation for Intern…

afb3d45

…al Mode deployment

bipuladh force-pushed the fix_minimal_deployment branch from 0bfb707 to afb3d45 Compare August 26, 2020 11:05

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

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

openshift-merge-robot merged commit 9cc959b into openshift:master Aug 26, 2020

spadgett added this to the v4.6 milestone Sep 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1871646: Fix location of Minimal Deployment message and calculation for Internal Mode deployment #6404

Bug 1871646: Fix location of Minimal Deployment message and calculation for Internal Mode deployment #6404

bipuladh commented Aug 23, 2020 •

edited

openshift-ci-robot commented Aug 23, 2020

bipuladh commented Aug 23, 2020

openshift-ci-robot commented Aug 23, 2020

cloudbehl commented Aug 24, 2020

bipuladh commented Aug 24, 2020

cloudbehl Aug 24, 2020

cloudbehl Aug 24, 2020

bipuladh Aug 24, 2020

shirimordechay Aug 24, 2020

cloudbehl Aug 24, 2020

bipuladh Aug 24, 2020

bipuladh Aug 24, 2020

cloudbehl Aug 24, 2020

bipuladh Aug 24, 2020

bipuladh Aug 24, 2020

cloudbehl Aug 24, 2020

bipuladh Aug 24, 2020

gnehapk commented Aug 25, 2020

bipuladh commented Aug 25, 2020

gnehapk Aug 25, 2020

gnehapk commented Aug 25, 2020

bipuladh commented Aug 25, 2020

openshift-ci-robot commented Aug 26, 2020

bipuladh commented Aug 26, 2020

gnehapk commented Aug 26, 2020

openshift-ci-robot commented Aug 26, 2020

openshift-bot commented Aug 26, 2020

openshift-bot commented Aug 26, 2020

openshift-ci-robot commented Aug 26, 2020

	export const shouldDeployInternalAsMinimal = (nodes: NodeKind[]) => {
	export const DeployCloudAsMinimal = (nodes: NodeKind[]) => {

	export const shouldDeployMinimally = (nodes: NodeKind[]) =>
	export const DeployBaremetalAsMinimal = (nodes: NodeKind[]) =>

	if (nodes.length >= 3) {
	if (nodes.length >= minSelectedNode) {

Bug 1871646: Fix location of Minimal Deployment message and calculation for Internal Mode deployment #6404

Bug 1871646: Fix location of Minimal Deployment message and calculation for Internal Mode deployment #6404

Conversation

bipuladh commented Aug 23, 2020 • edited

openshift-ci-robot commented Aug 23, 2020

bipuladh commented Aug 23, 2020

openshift-ci-robot commented Aug 23, 2020

cloudbehl commented Aug 24, 2020

bipuladh commented Aug 24, 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

gnehapk commented Aug 25, 2020

bipuladh commented Aug 25, 2020

Choose a reason for hiding this comment

gnehapk commented Aug 25, 2020

bipuladh commented Aug 25, 2020

openshift-ci-robot commented Aug 26, 2020

bipuladh commented Aug 26, 2020

gnehapk commented Aug 26, 2020

openshift-ci-robot commented Aug 26, 2020

openshift-bot commented Aug 26, 2020

openshift-bot commented Aug 26, 2020

openshift-ci-robot commented Aug 26, 2020

bipuladh commented Aug 23, 2020 •

edited