Bug 1801604: Add disk validation to virtual hardware on create-vm-wizard #4231

irosenzw · 2020-02-06T15:53:36Z

Add disk bus validation for the virtual hardware tab on create VM wizard

Signed-off-by: Ido Rosenzwig irosenzw@redhat.com

frontend/packages/kubevirt-plugin/src/utils/validations/vm/disk.ts

frontend/packages/kubevirt-plugin/src/utils/validations/template/types.ts

...rt-plugin/src/components/create-vm-wizard/tabs/virtual-hardware-tab/virtual-hardware-tab.tsx

atiratree · 2020-02-07T20:06:56Z

.../kubevirt-plugin/src/components/create-vm-wizard/redux/validations/storage-tab-validation.ts

-  const iStorages = iGetStorages(state, id);
+
+  const iStorages = iGetStorages(state, id).filter((iStorage) =>
+    iGetIn(iStorage, ['disk', 'disk']),


can this rather be everything except cdroms?

In theory it can be LUN or FLOPPY, although it's highly unlikely I think we should still keep this filter to be sure we get only storage objects of type DISK.

we are showing all the other devices as well, so we should show the validations for them too IMO. Even if it is more unlikely to have them.

it should be a complement of the cdrom table so the user can see all the information

Yes I agree, we should show all the validations for all the types.

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/selectors/template.ts

frontend/packages/kubevirt-plugin/src/components/modals/disk-modal/disk-modal.tsx

frontend/packages/kubevirt-plugin/src/components/vm-disks/utils.ts

atiratree · 2020-02-07T20:10:36Z

frontend/packages/kubevirt-plugin/src/utils/validations/template/template-validations.ts

@@ -73,6 +73,18 @@ export class TemplateValidations {
    });
  };

+  // Return a valid bus, return the defaultBus if it's valid.
+  getAValidBus = (defaultBus?: DiskBus): DiskBus => {


can we rename it to getDefaultBus and have zero arguments here?

I'd like to keep this method (we can change the name if you like)
the purpose is to verify the bus we are using as default, thus is has to have an argument.
I except the comment to have an additional method, with no arguments, to get the first valid bus

irosenzw · 2020-02-09T14:25:39Z

/retest

openshift-ci-robot · 2020-02-11T10:30:59Z

@irosenzw: This pull request references Bugzilla bug 1801604, 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.

In response to this:

Bug 1801604: Add disk validation to virtual hardware on create-vm-wizard

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.

atiratree · 2020-02-12T15:22:56Z

.../kubevirt-plugin/src/components/create-vm-wizard/redux/validations/storage-tab-validation.ts

-  const iStorages = iGetStorages(state, id);
+
+  const iStorages = iGetStorages(state, id).filter((iStorage) =>
+    iGetIn(iStorage, ['disk', 'disk']),


we are showing all the other devices as well, so we should show the validations for them too IMO. Even if it is more unlikely to have them.

it should be a complement of the cdrom table so the user can see all the information

atiratree · 2020-02-12T15:25:38Z

frontend/packages/kubevirt-plugin/src/utils/validations/template/template-validations.ts

@@ -73,6 +73,27 @@ export class TemplateValidations {
    });
  };

+  // Return a valid bus, return the defaultBus if it's valid.
+  getAValidBus = (defaultBus?: DiskBus): DiskBus => {


Suggested change

getAValidBus = (defaultBus?: DiskBus): DiskBus => {

getAValidBus = (defaultBus = DiskBus.VIRTIO): DiskBus => {

could we use this approach?

atiratree · 2020-02-12T15:26:53Z

frontend/packages/kubevirt-plugin/src/utils/validations/template/template-validations.ts

+    return null;
+  };
+
+  getDefaultBus = (): DiskBus => {


these two methods are confusing and I am not sure which one I should use on the first look.

I am sorry, but can we revert this to the former state and have just one method called getDefaultBus?

atiratree · 2020-02-12T15:28:21Z

frontend/packages/kubevirt-plugin/src/utils/validations/vm/disk.ts

@@ -138,8 +139,7 @@ export const validateDisk = (
      validations.pvc = validatePVCName(pvcName, usedPVCNames);
    }

-    // TODO: implement CDROM disk bus validation
-    if (disk.getType() === DiskType.DISK) {
+    if (diskType === DiskType.DISK || diskType === DiskType.CDROM) {


this is useful for LUN as well. Can we do this?

Suggested change

if (diskType === DiskType.DISK || diskType === DiskType.CDROM) {

if (disk.getType() !== DiskType.FLOPPY) {

atiratree · 2020-02-12T15:40:42Z

frontend/packages/kubevirt-plugin/src/utils/validations/template/template-validations.ts

@@ -73,6 +73,27 @@ export class TemplateValidations {
    });
  };

+  // Return a valid bus, return the defaultBus if it's valid.


not entirely correct. Can we please remove the comment to lower the confusion?

atiratree · 2020-02-12T16:28:20Z

frontend/packages/kubevirt-plugin/src/utils/validations/template/template-validations.ts

@@ -73,6 +73,14 @@ export class TemplateValidations {
    });
  };

+  getDefaultBus = (defaultBus = DiskBus.VIRTIO): DiskBus => {
+    const allowedBuses = this.getAllowedBuses();
+    if (allowedBuses.size === 0) {


not sure if we should keep it here or just return the defaultBus here for the function consistency. As this branch should never execute

irosenzw · 2020-02-12T18:14:54Z

/retest

atiratree · 2020-02-12T18:44:34Z

.../kubevirt-plugin/src/components/create-vm-wizard/redux/validations/storage-tab-validation.ts

@@ -71,6 +71,7 @@ export const validateStorages = (options: UpdateOptions) => {
 export const setStoragesTabValidity = (options: UpdateOptions) => {
  const { id, dispatch, getState } = options;
  const state = getState();
+
  const iStorages = iGetStorages(state, id);


should be everything except cdroms

Signed-off-by: Ido Rosenzwig <irosenzw@redhat.com>

atiratree · 2020-02-12T19:23:08Z

/lgtm

openshift-ci-robot · 2020-02-12T19:23:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irosenzw, suomiy

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/kubevirt-plugin/OWNERS~~ [suomiy]

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 · 2020-02-12T22:43:14Z

@irosenzw: All pull requests linked via external trackers have merged. Bugzilla bug 1801604 has been moved to the MODIFIED state.

In response to this:

Bug 1801604: Add disk validation to virtual hardware on create-vm-wizard

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 · 2020-02-12T23:44:20Z

@irosenzw: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-gcp-console	`57556b1`	link	`/test e2e-gcp-console`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. component/kubevirt Related to kubevirt-plugin labels Feb 6, 2020

openshift-ci-robot requested review from rhrazdil and atiratree February 6, 2020 15:55

atiratree suggested changes Feb 7, 2020

View reviewed changes

irosenzw force-pushed the cdrom-disk-validation branch 2 times, most recently from 7b00a8d to 7d5c87c Compare February 9, 2020 11:53

irosenzw changed the title ~~Add disk validation to virtual hardware on create-vm-wizard~~ Bug 1801604: Add disk validation to virtual hardware on create-vm-wizard Feb 11, 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 Feb 11, 2020

atiratree reviewed Feb 12, 2020

View reviewed changes

irosenzw force-pushed the cdrom-disk-validation branch from 7d5c87c to ac633e0 Compare February 12, 2020 16:07

atiratree reviewed Feb 12, 2020

View reviewed changes

irosenzw force-pushed the cdrom-disk-validation branch from ac633e0 to 63873c2 Compare February 12, 2020 17:04

atiratree reviewed Feb 12, 2020

View reviewed changes

Add disk validation to virtual hardware

57556b1

Signed-off-by: Ido Rosenzwig <irosenzw@redhat.com>

irosenzw force-pushed the cdrom-disk-validation branch from 63873c2 to 57556b1 Compare February 12, 2020 19:16

openshift-ci-robot assigned atiratree Feb 12, 2020

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

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

openshift-merge-robot merged commit bcb1fa3 into openshift:master Feb 12, 2020

spadgett added this to the v4.4 milestone Feb 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1801604: Add disk validation to virtual hardware on create-vm-wizard #4231

Bug 1801604: Add disk validation to virtual hardware on create-vm-wizard #4231

irosenzw commented Feb 6, 2020

atiratree Feb 7, 2020

irosenzw Feb 9, 2020

atiratree Feb 12, 2020 •

edited

irosenzw Feb 12, 2020

atiratree Feb 7, 2020

irosenzw Feb 9, 2020

irosenzw commented Feb 9, 2020

openshift-ci-robot commented Feb 11, 2020

atiratree Feb 12, 2020 •

edited

atiratree Feb 12, 2020

atiratree Feb 12, 2020

atiratree Feb 12, 2020

atiratree Feb 12, 2020

atiratree Feb 12, 2020

irosenzw commented Feb 12, 2020

atiratree Feb 12, 2020

atiratree commented Feb 12, 2020

openshift-ci-robot commented Feb 12, 2020

openshift-ci-robot commented Feb 12, 2020

openshift-ci-robot commented Feb 12, 2020

	getAValidBus = (defaultBus?: DiskBus): DiskBus => {
	getAValidBus = (defaultBus = DiskBus.VIRTIO): DiskBus => {

	if (diskType === DiskType.DISK \|\| diskType === DiskType.CDROM) {
	if (disk.getType() !== DiskType.FLOPPY) {

Bug 1801604: Add disk validation to virtual hardware on create-vm-wizard #4231

Bug 1801604: Add disk validation to virtual hardware on create-vm-wizard #4231

Conversation

irosenzw commented Feb 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree Feb 12, 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

irosenzw commented Feb 9, 2020

openshift-ci-robot commented Feb 11, 2020

atiratree Feb 12, 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

irosenzw commented Feb 12, 2020

Choose a reason for hiding this comment

atiratree commented Feb 12, 2020

openshift-ci-robot commented Feb 12, 2020

openshift-ci-robot commented Feb 12, 2020

openshift-ci-robot commented Feb 12, 2020

atiratree Feb 12, 2020 •

edited

atiratree Feb 12, 2020 •

edited