Show only the allowed storage interfaces on Disk-modal #3973

irosenzw · 2020-01-15T14:35:38Z

Fixes: CNV-2914

Enforce the validations from common and user templates
so the user can choose only from the allowed values in the Disk-modal
at the storage tab.
Add the validations to the store.
Implement enum validation methods.

Depends on: #3909

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

- unify and enhance template selections for validation and create requests - use GiB instead of GB for memory to allign with validations - show validation intervals

openshift-ci-robot · 2020-01-15T14:36:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: irosenzw
To complete the pull request process, please assign jelkosz
You can assign the PR to them by writing /assign @jelkosz in a comment when ready.

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

Details

Needs approval from an approver in each of these files:

frontend/packages/kubevirt-plugin/OWNERS

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

yaacov · 2020-01-15T14:59:04Z

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/redux/reducers.ts

shouldn't it be fromJS(payload.value), ?

No, because the payload isn't an immutable object but a TemplateValidations[] object.

yaacov · 2020-01-15T15:02:21Z

...src/components/create-vm-wizard/redux/stateUpdate/vmSettings/vm-settings-tab-state-update.ts

not needing const state = getState(); in this function is strange ...

I do not think it is needed to store validations in the redux.

There are problems with this approach:

redux should not store duplicate data

this function is missing all the dependencies for this to work correctly

this might be another place for performance issues in the future

@suomiy , I don't understand this comment at all. Can you elaborate ?
why the validations consider duplicate data ?
what are the dependencies I missed?
what performance issues you are referring to? can you give an example ?

@suomiy, how do you suggest to get the validations to the disk-modal without using Redux store ?

@suomiy , I don't understand this comment at all. Can you elaborate ?
why the validations consider duplicate data ?

because they are already stored in the template

what are the dependencies I missed?
what performance issues you are referring to? can you give an example ?

usertemplate and templates and that is why it could be a performance issue (we could still use it if we decide to, but it is better to be careful)

@suomiy, how do you suggest to get the validations to the disk-modal without using Redux store ?

from the templates

Fixes: CNV-2914 - Enforce the validations from common and user templates so the user can choose only from the allowed values in the Disk-modal at the storage tab. - Add the validations to the store. - Implement enum validation methods. Depends on: openshift#3909 Signed-off-by: Ido Rosenzwig irosenzw@redhat.com

atiratree · 2020-01-16T23:55:18Z

...src/components/create-vm-wizard/redux/stateUpdate/vmSettings/vm-settings-tab-state-update.ts

I do not think it is needed to store validations in the redux.

There are problems with this approach:

redux should not store duplicate data

this function is missing all the dependencies for this to work correctly

this might be another place for performance issues in the future

atiratree · 2020-01-16T23:56:20Z

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/redux/reducers.ts

      );
+    case InternalActionType.SetTemplateValidations:
+      return state.setIn(
+        [dialogID, 'commonData', 'dataIDReferences', 'templateValidations'],


just a note: dataIDReferences is for storing references of already present data in the redux store only. Not the original data.

atiratree · 2020-01-16T23:57:38Z

...virt-plugin/src/components/create-vm-wizard/redux/validations/utils/templates-validations.ts

  if (templates.size > 0 && os && workload) {
    // templates are sorted by relevance if only flavor is missing
-    return getValidationsFromTemplates([templates.first()]);
+    return getValidationsFromTemplates(templates.toArray());


I don't see a reason why this is necessary. Please update to the newest version

atiratree · 2020-01-17T00:02:46Z

...plugin/src/components/create-vm-wizard/tabs/storage-tab/vm-wizard-storage-modal-enhanced.tsx

    },
  ];

+  const getAllowedBusses = (): Set<string> => {


should not be needed, there should be just one templateValidation at this point.

atiratree · 2020-01-17T00:05:57Z

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

  const vmLikeFinal = getLoadedData(vmLikeEntityLoading, vmLikeEntity); // default old snapshot before loading a new one
  const vm = asVM(vmLikeFinal);

+  const allowedBusses = getOperatingSystem(vmLikeEntity).startsWith('win')


I think we should rather allow all buses in the VM detail disk modal, than to hardcode some

This is for the modal in the Disk details page.
If a user uses a windows OS it shouldn't have the option to pick scsi bus - just virtio or sata

atiratree · 2020-01-17T00:13:09Z

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

-              {DiskBus.getAll().map((b) => (
-                <FormSelectOption key={b.getValue()} value={b.getValue()} label={b.toString()} />
-              ))}
+              {!allowedBusses || allowedBusses.size === 0


this does not make semantically sense. either we say

allowedBusses will default to all when null

or they will reflect the real meaning at all times - actual allowed busses

atiratree · 2020-01-17T00:14:51Z

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

+                  ))
+                : DiskBus.getAll().map(
+                    (b) =>
+                      Array.from(allowedBusses).includes(b.getValue()) && (


set has especially method has for this task, so there is no need for array creation

also there should not be code duplication for FormSelectOption creation

atiratree · 2020-01-17T00:18:57Z

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

+    // Empty array means all values are allowed
+
+    // Get all the validations which has the 'values' key and aren't optional
+    const relevantValidations = this.getRelevantValidations(jsonPath).filter(


latest change moves the filter to getRelevantValidations

atiratree · 2020-01-17T00:22:12Z

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

    return { min, max, isMinInclusive, isMaxInclusive, isValid };
  };

+  getAllowedBusses = (): Set<string> => this.getAllowedEnumValues(ValidationJSONPath.BUS);


does not take into account the name of the rule - e.g. windows-virtio-bus, windows-disk-bus which has a meaning with consequences

I don't understand what you wanted to say in this comment

ok, let's not do that and just enforce the rules

openshift-ci-robot · 2020-02-13T00:08:28Z

@irosenzw: PR needs rebase.

Details

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.

kubevirt: enhance memory validation and template selection

772c050

- unify and enhance template selections for validation and create requests - use GiB instead of GB for memory to allign with validations - show validation intervals

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 15, 2020

openshift-ci-robot requested review from rawagner and yaacov January 15, 2020 14:36

openshift-ci-robot added the component/kubevirt Related to kubevirt-plugin label Jan 15, 2020

irosenzw force-pushed the add-windows-disk-validation branch 2 times, most recently from c389565 to 1015dc9 Compare January 15, 2020 14:49

yaacov reviewed Jan 15, 2020

View reviewed changes

irosenzw force-pushed the add-windows-disk-validation branch from 1015dc9 to 1b7ce12 Compare January 16, 2020 15:46

irosenzw changed the title ~~[WIP] Show only the allowed storage interfaces on Disk-modal~~ Show only the allowed storage interfaces on Disk-modal Jan 16, 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 Jan 16, 2020

atiratree suggested changes Jan 17, 2020

View reviewed changes

atiratree reviewed Jan 17, 2020

View reviewed changes

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

irosenzw closed this Feb 20, 2020

Show only the allowed storage interfaces on Disk-modal #3973

Show only the allowed storage interfaces on Disk-modal #3973

Uh oh!

Conversation

irosenzw commented Jan 15, 2020

Uh oh!

openshift-ci-robot commented Jan 15, 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!

irosenzw Jan 19, 2020 • 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!

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!

openshift-ci-robot commented Feb 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

irosenzw Jan 19, 2020 •

edited

Loading