Bug 1868426: Unfilter OS dropdown in VM wizard when choosing flavor or workload #6316

irosenzw · 2020-08-12T14:44:10Z

When switching OS, the flavor and workload will be set to NULL
thus the user would have to pick suitable flavor & workload
for the new OS.

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

atiratree · 2020-08-12T16:09:30Z

frontend/packages/kubevirt-plugin/src/selectors/vm-template/combined-dependent.ts

-      getTemplatesOfLabelType(templates, TEMPLATE_TYPE_BASE),
-      [getWorkloadLabel(workload), getFlavorLabel(flavor)],
-    );
+    ]);


why?
this will cause to return the wrong OSs not reflecting the workload and flavor selection.

https://bugzilla.redhat.com/show_bug.cgi?id=1868426

@suomiy hi,
from UX side this is wrong to change the selection options of a field by using another field below it.

from a user perspective a common problem this can fix is:
a - a user selects an OS then selects a Flavor or Workload
b - a user want to fix their selection of OS, but the correct OS is missing from the list.

the fix:
a - a user select and OS
b - the user select some flavor
c - all OS options are presented and they can currect the OS selection.

this is not about UX, but about correct template selection. The user should be aware of comon templates and create his own if such template is missing (based on os/flavor etc.)

disscussing more in BZ

Note:
in the BZ discussion we decided that a viable fix for the problem wil be to:

1: override the user selection of flavor/workload upon OS selection

Ido, can we override the values to "null" (or what ever the default value is) ?
this will make sure users know that the values where changed ?

yeah good idea

openshift-ci-robot · 2020-08-12T16:44:49Z

@irosenzw: This pull request references Bugzilla bug 1868426, which is invalid:

expected the bug to target the "4.6.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1868426: Unfilter OS dropdown in VM wizard when choosing flavor or workload

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.

irosenzw · 2020-08-12T16:45:33Z

/bugzilla refresh

openshift-ci-robot · 2020-08-12T16:45:40Z

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

yaacov · 2020-08-12T18:02:27Z

/retest

yaacov · 2020-08-13T06:28:21Z

/retest

yaacov · 2020-08-13T10:02:02Z

/retest

yaacov · 2020-08-13T11:52:09Z

/retest

yaacov · 2020-08-13T12:17:00Z

/retest

openshift-ci-robot · 2020-08-13T14:22:24Z

@irosenzw: This pull request references Bugzilla bug 1868426, 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 1868426: Unfilter OS dropdown in VM wizard when choosing flavor or workload

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.

yaacov · 2020-08-13T14:36:58Z

/lgtm

atiratree · 2020-08-13T14:58:25Z

/hold

atiratree

IMO this PR still can be improved on.

atiratree · 2020-08-13T14:59:11Z

...rt-plugin/src/components/create-vm-wizard/redux/state-update/vm-settings-tab-state-update.ts

+        isHidden: asHidden(!isWindows, VMSettingsField.OPERATING_SYSTEM),
+        value: isWindows,
+      },
+      [VMSettingsField.WORKLOAD_PROFILE]: { value: null },


we should not reset the values if we can supply the right template

additional idea: we could also show a small warning to the user that a combination is invalid and reset the warning on any new value selection

thoughts?

I agree with the first comment.
for the warning I think it's redundant. when following the first comment behavior, it is clear that if the workload/flavor would set to null it is because the combination is invalid, thus no need for any warning message.

what do you think @matthewcarleton ?

Note:
two os may have the same flavor label, but it may mean different things, for example:
flavor: tiny os:ubuntu => mem: 2Gi
flavor: tiny os:fedora => mem: 1Gi

IMHO, not re-seting the flavor each time we change an os is not nice to users

https://github.com/kubevirt/common-templates/blob/e127881c7fb7f77ba84f0571478d4507e3e6b8e7/generate-templates.yaml#L204
https://github.com/kubevirt/common-templates/blob/e127881c7fb7f77ba84f0571478d4507e3e6b8e7/generate-templates.yaml#L162

why is it not nice? the user just sees the label tiny and probably does not know which template will be used, ie does not have an association with how much memory. And if he does know, it shouldn't matter either. The user is selecting a tiny flavor for a reason.

why is it not nice?

All the times I ordered a VM, I used numbers (or looked up doc to see the numbers) to specify the mem/cpu/disk I needed , I think that a user like me, will be confused if they choose 2Gb (tiny on ubuntu) and got 1Gb (tiny on fedora) while they should have chosen small because it's the 2Gb for fedora.

But I don't feel strongly about this, so just ignore if you feel differently, the bug is about having the ability to choose any OS, so any fix that will show all the OS options for any given flavour/workload is good for me :-)

I know it is a bit confusing, I just meant it in the context of this PR. Sure if we plan to redesign this feature, we should tackle this problem in a separate effort. As this concerns the basic flow as well, and not just this fix.

atiratree · 2020-08-17T11:16:23Z

...rt-plugin/src/components/create-vm-wizard/redux/state-update/vm-settings-tab-state-update.ts

+        dispatch(
+          vmWizardInternalActions[InternalActionType.UpdateVmSettings](id, {
+            [VMSettingsField.FLAVOR]: { value: null },
+            [VMSettingsField.MEMORY]: {


this is done by flavorUpdater. Let's move the flavorUpdater down if we need its functionality later

atiratree · 2020-08-17T11:17:50Z

...rt-plugin/src/components/create-vm-wizard/redux/state-update/vm-settings-tab-state-update.ts

+  if (os && !iTemplate) {
+    const keepFlavor = Object.assign({}, relevantOptions);
+    Object.assign(keepFlavor, { workload: null });
+    // Check for a valid common-template that contain the chosen Flavor


let's not try to find combinations which could work. We don't know if the user prefers to keep his flavor or workload so let's reset both if we have to reset one of them.

atiratree · 2020-08-17T11:21:26Z

...rt-plugin/src/components/create-vm-wizard/redux/state-update/vm-settings-tab-state-update.ts

+        );
+      } else {
+        // Reset both Workload and Flavor as no valid common-template has been found
+        dispatch(


btw workloadConsistencyUpdater does a similar thing. It might be easier for understanding to have the same logic in one place for both

atiratree · 2020-08-17T12:59:42Z

...rt-plugin/src/components/create-vm-wizard/redux/state-update/vm-settings-tab-state-update.ts

    osUpdater,
    baseImageUpdater,
    cloneCommonBaseDiskImageUpdater,
    workloadConsistencyUpdater,
    provisioningSourceUpdater,
    nativeK8sUpdater,
+    flavorUpdater,


please move the code as well. We want to ensure that the updaters can be read from top to bottom in the same order they are executed in

atiratree · 2020-08-17T15:04:33Z

...rt-plugin/src/components/create-vm-wizard/redux/state-update/vm-settings-tab-state-update.ts

    );
  }
 };

+const provisioningSourceUpdater = ({ id, prevState, dispatch, getState }: UpdateOptions) => {


+1 for moving this as well

atiratree · 2020-08-17T15:04:57Z

...rt-plugin/src/components/create-vm-wizard/redux/state-update/vm-settings-tab-state-update.ts

  const state = getState();
-  if (!hasVMSettingsValueChanged(prevState, state, id, VMSettingsField.WORKLOAD_PROFILE)) {
+  if (
+    !hasVMSettingsValueChanged(prevState, state, id, VMSettingsField.WORKLOAD_PROFILE) &&


this can be done in one call, look at the args of this func

atiratree · 2020-08-17T15:08:22Z

...rt-plugin/src/components/create-vm-wizard/redux/state-update/vm-settings-tab-state-update.ts

    return;
  }
  const selectors = iGetRelevantTemplateSelectors(state, id);
  const iUserTemplates = iGetLoadedCommonData(state, id, VMWizardProps.userTemplates);
  const iCommonTemplates = iGetLoadedCommonData(state, id, VMWizardProps.commonTemplates);

  if (!iGetRelevantTemplate(iUserTemplates, iCommonTemplates, selectors)) {
-    // reset workload profile if no relevant template found - could be triggered by provider prefil
+    // reset workload and flavor profile if no relevant template found - could be triggered by provider prefil


Suggested change

// reset workload and flavor profile if no relevant template found - could be triggered by provider prefil

// reset workload and flavor profile if no relevant template found - could be triggered by provider prefil or os selection

When switching OS, the flavor or the workload values will be set to NULL if there is no common-template that respects the new OS with one of them. Signed-off-by: Ido Rosenzwig <irosenzw@redhat.com>

atiratree · 2020-08-17T15:27:33Z

/lgtm

openshift-ci-robot · 2020-08-17T15:27:50Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,yaacov]

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

atiratree · 2020-08-17T15:32:11Z

/hold cancel

yaacov · 2020-08-17T18:00:08Z

/retest

openshift-bot · 2020-08-17T18:11:29Z

/retest

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

openshift-bot · 2020-08-17T22:01:33Z

/retest

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

openshift-bot · 2020-08-18T04:18:38Z

/retest

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

openshift-ci-robot · 2020-08-19T17:40:54Z

@irosenzw: All pull requests linked via external trackers have merged: openshift/console#6316. Bugzilla bug 1868426 has been moved to the MODIFIED state.

In response to this:

Bug 1868426: Unfilter OS dropdown in VM wizard when choosing flavor or workload

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 added the component/kubevirt Related to kubevirt-plugin label Aug 12, 2020

openshift-ci-robot requested review from pcbailey and vojtechszocs August 12, 2020 14:44

atiratree reviewed Aug 12, 2020

View reviewed changes

irosenzw changed the title ~~Unfilter OS dropdown in VM wizard when choosing flavor or workload~~ Bug 1868426: Unfilter OS dropdown in VM wizard when choosing flavor or workload Aug 12, 2020

openshift-ci-robot added bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Aug 12, 2020

openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Aug 12, 2020

irosenzw force-pushed the unfilter-OS-in-VM-wizard branch from 0f3e506 to 1753dee Compare August 13, 2020 14:21

openshift-ci-robot assigned yaacov Aug 13, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 13, 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 13, 2020

atiratree suggested changes Aug 13, 2020

View reviewed changes

irosenzw force-pushed the unfilter-OS-in-VM-wizard branch from 1753dee to bd5790a Compare August 17, 2020 09:32

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

atiratree suggested changes Aug 17, 2020

View reviewed changes

irosenzw force-pushed the unfilter-OS-in-VM-wizard branch from bd5790a to f21715f Compare August 17, 2020 11:55

irosenzw force-pushed the unfilter-OS-in-VM-wizard branch from f21715f to 1245a0c Compare August 17, 2020 12:48

atiratree reviewed Aug 17, 2020

View reviewed changes

irosenzw force-pushed the unfilter-OS-in-VM-wizard branch 3 times, most recently from 8e16c1c to 29e519b Compare August 17, 2020 14:53

atiratree suggested changes Aug 17, 2020

View reviewed changes

Unfilter OS dropdown in VM wizard when choosing flavor or workload

ace1e6c

When switching OS, the flavor or the workload values will be set to NULL if there is no common-template that respects the new OS with one of them. Signed-off-by: Ido Rosenzwig <irosenzw@redhat.com>

irosenzw force-pushed the unfilter-OS-in-VM-wizard branch from 29e519b to ace1e6c Compare August 17, 2020 15:16

openshift-ci-robot assigned atiratree Aug 17, 2020

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

openshift-merge-robot merged commit c895f55 into openshift:master Aug 19, 2020

spadgett added this to the v4.6 milestone Aug 20, 2020

	// reset workload and flavor profile if no relevant template found - could be triggered by provider prefil
	// reset workload and flavor profile if no relevant template found - could be triggered by provider prefil or os selection

Bug 1868426: Unfilter OS dropdown in VM wizard when choosing flavor or workload #6316

Bug 1868426: Unfilter OS dropdown in VM wizard when choosing flavor or workload #6316

Conversation

irosenzw commented Aug 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

openshift-ci-robot commented Aug 12, 2020

irosenzw commented Aug 12, 2020

openshift-ci-robot commented Aug 12, 2020

yaacov commented Aug 12, 2020

yaacov commented Aug 13, 2020

yaacov commented Aug 13, 2020

yaacov commented Aug 13, 2020

yaacov commented Aug 13, 2020

openshift-ci-robot commented Aug 13, 2020

yaacov commented Aug 13, 2020

atiratree commented Aug 13, 2020

atiratree left a 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

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

atiratree commented Aug 17, 2020

openshift-ci-robot commented Aug 17, 2020

atiratree commented Aug 17, 2020

yaacov commented Aug 17, 2020

openshift-bot commented Aug 17, 2020

openshift-bot commented Aug 17, 2020

openshift-bot commented Aug 18, 2020

openshift-ci-robot commented Aug 19, 2020

irosenzw commented Aug 12, 2020 •

edited