New VM from template flow #6937

rawagner · 2020-10-15T08:39:06Z

TODO (for followups):

check certificate before uploading Bug 1887783: PVC upload cannot continue after approve the certificate #6922
unify boot source types with advanced wizard
share form validations with advanced wizard
unify error/loading/success states with advanced wizard
share VM creation flow with advanced wizard (requests/vm)

glekner

small review

glekner · 2020-10-26T10:54:47Z

...d/packages/kubevirt-plugin/src/components/modals/add-template-source/add-template-source.tsx

+  CLONE: 'Clone existing PVC',
+};
+
+const DATA_SOURCES = [


we should probably create a new SelectDropdownObjectEnum for this object.
see storage-ui-source.ts for example.

glekner · 2020-10-26T10:57:30Z

...d/packages/kubevirt-plugin/src/components/modals/add-template-source/add-template-source.tsx

+
+                <Form>
+                  <FormRow fieldId="form-data-source" title="Data source type" isRequired>
+                    <Select


We should use the new <FormPFSelect /> instead of all new <Select />.
It handles the isOpen state for you

that's handy. Fixed.

glekner · 2020-10-29T12:05:35Z

frontend/packages/kubevirt-plugin/src/components/create-vm/forms/boot-source-form.tsx

+      </FormRow>
+      {state.dataSource?.value === DataSource.UPLOAD && (
+        <FormRow fieldId="form-ds-file" title="Upload source" isRequired>
+          <FileUpload


we should add type restrictions and onDropReject handlers

dropzoneProps={{ accept: '.iso,.img,.qcow2,.gz,.xz', onDropRejected: () => setIsFileRejected(true), onDropAccepted: () => setIsFileRejected(false), }}

About the filetypes, heads up:
https://bugzilla.redhat.com/show_bug.cgi?id=1894374

atiratree · 2020-11-13T13:13:01Z

frontend/packages/kubevirt-plugin/src/components/create-vm/forms/boot-source-form.tsx

+    [dispatch, storageClasses],
+  );
+
+  React.useEffect(() => {


this is still crashing the page

Uncaught error Invariant Violation: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops. at http://localhost:9000/static/vendors~main-e7b448dfa7e4529ddc20.js:403810:28 at checkForNestedUpdates (http://localhost:9000/static/vendors~main-e7b448dfa7e4529ddc20.js:403813:7) at scheduleUpdateOnFiber (http://localhost:9000/static/vendors~main-e7b448dfa7e4529ddc20.js:402084:3) at dispatchAction (http://localhost:9000/static/vendors~main-e7b448dfa7e4529ddc20.js:396511:5) at http://localhost:9000/static/kubevirt~kubevirt-create-vm-836eedccd7f68b69d796.js:533:5 at http://localhost:9000/static/kubevirt~kubevirt-create-vm-836eedccd7f68b69d796.js:550:7 at commitHookEffectList (http://localhost:9000/static/vendors~main-e7b448dfa7e4529ddc20.js:400681:26) at commitPassiveHookEffects (http://localhost:9000/static/vendors~main-e7b448dfa7e4529ddc20.js:400711:11) at HTMLUnknownElement.callCallback (http://localhost:9000/static/vendors~main-e7b448dfa7e4529ddc20.js:381042:14) at Object.invokeGuardedCallbackDev (http://localhost:9000/static/vendors~main-e7b448dfa7e4529ddc20.js:381092:16) window.onerror @ Uncaught Invariant Violation: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops. at http://localhost:9000/static/vendors~main-e7b448dfa7e4529ddc20.js:403810:28 at checkForNestedUpdates (http://localhost:9000/static/vendors~main-e7b448dfa7e4529ddc20.js:403813:7) at scheduleUpdateOnFiber (http://localhost:9000/static/vendors~main-e7b448dfa7e4529ddc20.js:402084:3) at dispatchAction (http://localhost:9000/static/vendors~main-e7b448dfa7e4529ddc20.js:396511:5) at http://localhost:9000/static/kubevirt~kubevirt-create-vm-836eedccd7f68b69d796.js:533:5 at http://localhost:9000/static/kubevirt~kubevirt-create-vm-836eedccd7f68b69d796.js:550:7 at commitHookEffectList (http://localhost:9000/static/vendors~main-e7b448dfa7e4529ddc20.js:400681:26) at commitPassiveHookEffects (http://localhost:9000/static/vendors~main-e7b448dfa7e4529ddc20.js:400711:11) at HTMLUnknownElement.callCallback (http://localhost:9000/static/vendors~main-e7b448dfa7e4529ddc20.js:381042:14) at Object.invokeGuardedCallbackDev (http://localhost:9000/static/vendors~main-e7b448dfa7e4529ddc20.js:381092:16) The above error occurred in the <BootSourceForm> component: in BootSourceForm (created by BootSource) in div (created by GridItem) in GridItem (created by BootSource) in div (created by Grid) in Grid (created by BootSource) in div (created by StackItem) in StackItem (created by BootSource) in div (created by Stack) in Stack (created by BootSource) in div (created by Data) in Data (created by StatusBox) in StatusBox (created by BootSource) in BootSource (created by CreateVM) in div (created by WizardBody) in div (created by WizardBody) in WizardBody (created by WizardToggle) in div (created by WizardToggle) in div (created by WizardToggle) in WizardToggle (created by Wizard) in div (created by Wizard) in Wizard (created by CreateVM) in div (created by CreateVM) in CreateVM (created by AsyncComponent) in AsyncComponent (created by Context.Consumer) in Route (created by LazyRoute) in LazyRoute (created by AppContents_) in Switch (created by AppContents_) in div (created by AppContents_) in div (created by AppContents_) in section (created by PageSection) in PageSection (created by AppContents_) in AppContents_ (created by ConnectFunction) in ConnectFunction (created by App_) in div (created by DrawerContent) in div (created by DrawerMain) in DrawerMain (created by DrawerContent) in DrawerContent (created by NotificationDrawer) in div (created by Drawer) in Drawer (created by NotificationDrawer) in NotificationDrawer (created by ConnectedNotificationDrawer_) in ConnectedNotificationDrawer_ (created by ConnectFunction) in ConnectFunction (created by App_) in main (created by Page) in div (created by Page) in Page (created by App_) in div (created by DrawerContentBody) in DrawerContentBody (created by QuickStartDrawer) in div (created by DrawerContent) in div (created by DrawerMain) in DrawerMain (created by DrawerContent) in DrawerContent (created by QuickStartDrawer) in div (created by Drawer) in Drawer (created by QuickStartDrawer) in QuickStartDrawer (created by ConnectFunction) in ConnectFunction (created by App_) in EnhancedProvider (created by App_) in EnhancedProvider (created by App_) in DetectPerspective (created by ConnectFunction) in ConnectFunction (created by App_) in App_ (created by withExtensions(App_)) in withExtensions(App_) (created by withI18nextTranslation(withExtensions(App_))) in withI18nextTranslation(withExtensions(App_)) (created by Context.Consumer) in Route in Switch in Router in Provider in Suspense

atiratree · 2020-11-13T13:17:25Z

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/create-vm-wizard.tsx

+  const name = (userMode === VMWizardMode.VM && searchParams.get('name')) || '';
+  const commonTemplateName = searchParams.get('common-template') || '';
+  const startVM = searchParams.get('startVM') === 'true' || false;
+  let source: BootSourceParams;


better, although I think it would be more compact/readable and extensible if we passed all of the initialData in JSON.

and we can guard the whole initial data with userMode === VMWizardMode.VM and se it to empty otherwise

atiratree · 2020-11-13T13:21:57Z

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

@@ -266,12 +279,31 @@ export const DirectCommonDataProps = new Set<ChangedCommonDataProp>([
  VMWizardProps.openshiftCNVBaseImages,
 ]);

+export enum URLParams {


can we make the name more explicit and move it to src/constants? as it serves more as an interface and there is no need for the consumer of this to import the create-vm-wizard

moved to src/constants/url-params. Is that ok with you ?

atiratree · 2020-11-13T13:30:44Z

...lugin/src/components/create-vm-wizard/redux/state-update/prefill-vm-template-state-update.ts

+        sourceType = ProvisionSource.DISK.getValue();
+      }
+
+      dispatch(


this should just set the value as else branch does instead of a dispatch. So we have just one atomic dispatch at the end.

atiratree · 2020-11-13T13:32:31Z

...lugin/src/components/create-vm-wizard/redux/state-update/prefill-vm-template-state-update.ts

+    const hasCustomSource =
+      source?.url || source?.container || (source?.pvcName && source?.pvcNamespace);
+    if (hasCustomSource) {
+      let sourceType: string;


this can be of type ProvisionSource and getValue can be called just before we use the value

atiratree · 2020-11-13T13:55:52Z

...virt-plugin/src/components/create-vm-wizard/redux/initial-state/storage-tab-initial-state.ts


-    return getUrlStorage(toShallowJS(iStorageClassConfigMap, undefined, true));
+  if (provisionSource === ProvisionSource.URL) {
+    if (source?.url) {


I find two issue with current approach which might be confusing to the user.

There are actually 4 different Container types

classic containerDisk:

create template -> container source

create simple vm from base template -> container source -> switch to advanced wizard

container(registry) dataVolume

create simple vm from base template -> container source

container(registry) dataVolume + additional disk

create simple vm from base template -> container source + cdrom

classic containerDisk + additional disk:

create simple vm from base template -> container source + cdrom -> switch to advanced wizard

I think it might seem to the user that s/he is getting the same thing, but the underlying representation is different each time.

Also I think the user would expect that the final result would not change by simply changing the view from simple to advanced.

Anyway this is not an easy topic. Maybe we should discuss this in a meeting or in a standup?

atiratree · 2020-11-13T14:01:06Z

...bevirt-plugin/src/components/create-vm-wizard/redux/state-update/storage-tab-state-update.ts

      dispatch(
        vmWizardInternalActions[InternalActionType.UpdateStorage](id, {
          id: oldSourceStorage ? oldSourceStorage.id : getNextIDResolver(getStorages(state, id))(),
          ...newSourceStorage,
        }),
      );
+      if (newSourceStorage.disk.cdrom && !additionalStorage) {
+        const emptyDisk = {
+          id: getNextIDResolver(getStorages(getState(), id))(),


lets reuse and save the nextIDResolver so we dont have to call getStorages twice

atiratree · 2020-11-13T14:09:10Z

frontend/packages/kubevirt-plugin/src/k8s/requests/vm/create/simple-create.ts

+    const rootDisk = new DiskWrapper(
+      vmWrapper.getDisks().find((d) => d.name === ROOT_DISK_NAME),
+      true,
+    ).init({ bootOrder: 1 });


In #6937 (comment) I meant that we could call the setters from the init.

I would keep the setters and rather call setBootOrder here instead of init. Init should be meant for initializing empty wrappers, but this disk has been already initialized.

atiratree · 2020-11-13T14:12:46Z

frontend/packages/kubevirt-plugin/src/components/create-vm/create-vm.tsx

+import '../create-vm-wizard/create-vm-wizard.scss';
+import './create-vm.scss';
+
+enum WIZARD_STEPS {


+1. I also meant the enum itself as WIZARD_STEPS is quite generic name.

We might also choose a better name for create-vm as we also have create-vm-wizard and both are wizard.
Thoughts?

atiratree · 2020-11-13T15:31:40Z

@rawagner ok, let's resolve the other issues in followups

atiratree · 2020-11-18T19:49:50Z

frontend/packages/kubevirt-plugin/src/utils/url.ts

  }

  const paramsString = params.toString() ? `?${params}` : '';

  return `/k8s/ns/${namespace || 'default'}/virtualization/${type}${paramsString}`;
 };
+
+export const parseInitialData = (searchParams: URLSearchParams): InitialData => {


looks much better +1 for having the functions in the same place

and here parseVMWizardInitialData ?

atiratree · 2020-11-18T19:51:26Z

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

@@ -266,12 +279,21 @@ export const DirectCommonDataProps = new Set<ChangedCommonDataProp>([
  VMWizardProps.openshiftCNVBaseImages,
 ]);

+export type InitialData = {


I would prefer if we could move this outside of the wizard since it is a general structure shared with outside as well (same as url params)

moved outside together with BootSourceParams type.

atiratree · 2020-11-18T20:04:42Z

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

@@ -266,12 +279,31 @@ export const DirectCommonDataProps = new Set<ChangedCommonDataProp>([
  VMWizardProps.openshiftCNVBaseImages,
 ]);

+export enum URLParams {


atiratree

did a quick pass. Looks pretty good, if we omit the postponed issues.

regarding this issue: #6937 (comment)
Are we going to fix this here or in a followup ?

rawagner · 2020-11-18T20:30:15Z

did a quick pass. Looks pretty good, if we omit the postponed issues.

regarding this issue: #6937 (comment)
Are we going to fix this here or in a followup ?

Thanks Filip! i I will fix #6937 (comment). Regarding #6937 (comment) lets meet this week and fix it in a followup

atiratree · 2020-11-19T12:11:02Z

frontend/packages/kubevirt-plugin/src/types/url.ts

@@ -0,0 +1,17 @@
+export type InitialData = {


we could work on the naming a bit. What about VMWizardInitialData and VMWizardBootSourceParams ?

sounds good, renamed all

atiratree · 2020-11-19T12:12:19Z

frontend/packages/kubevirt-plugin/src/utils/url.ts

  }

  const paramsString = params.toString() ? `?${params}` : '';

  return `/k8s/ns/${namespace || 'default'}/virtualization/${type}${paramsString}`;
 };
+
+export const parseInitialData = (searchParams: URLSearchParams): InitialData => {


and here parseVMWizardInitialData ?

rawagner · 2020-11-19T12:12:46Z

/retest

atiratree · 2020-11-19T12:46:03Z

/lgtm

rawagner · 2020-11-19T13:59:48Z

/retest

glekner · 2020-11-19T17:19:44Z

/lgtm

openshift-ci-robot · 2020-11-19T17:20:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: glekner, rawagner, 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/OWNERS~~ [rawagner]

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

openshift-merge-robot · 2020-11-19T23:05:37Z

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

Test name	Commit	Details	Rerun command
ci/prow/analyze	`705ad0d`	link	`/test analyze`

Full PR test history. Your PR dashboard.

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-bot · 2020-11-20T01:54:02Z

/retest

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

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 Oct 15, 2020

openshift-ci-robot requested review from TheRealJon and yaacov October 15, 2020 08:39

openshift-ci-robot added component/kubevirt Related to kubevirt-plugin approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 15, 2020

rawagner force-pushed the vm_templates_create branch 5 times, most recently from 37e0e5a to 5e8542e Compare October 22, 2020 07:52

glekner reviewed Oct 26, 2020

View reviewed changes

rawagner force-pushed the vm_templates_create branch 12 times, most recently from 4969457 to 1b5e13c Compare October 29, 2020 11:46

glekner reviewed Oct 29, 2020

View reviewed changes

rawagner force-pushed the vm_templates_create branch 5 times, most recently from ebe9bc6 to c4f193f Compare October 29, 2020 13:29

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 Nov 13, 2020

rawagner force-pushed the vm_templates_create branch from 946e923 to eabe015 Compare November 13, 2020 14:44

rawagner changed the title ~~Vm templates create~~ New VM from template flow Nov 13, 2020

atiratree suggested changes Nov 13, 2020

View reviewed changes

rawagner force-pushed the vm_templates_create branch 3 times, most recently from e179b3d to d94c68e Compare November 18, 2020 13:16

atiratree reviewed Nov 18, 2020

View reviewed changes

atiratree suggested changes Nov 18, 2020

View reviewed changes

rawagner force-pushed the vm_templates_create branch from d94c68e to cf44d4f Compare November 19, 2020 08:03

atiratree reviewed Nov 19, 2020

View reviewed changes

rawagner force-pushed the vm_templates_create branch from cf44d4f to da79087 Compare November 19, 2020 12:24

openshift-ci-robot assigned atiratree Nov 19, 2020

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

New VM from template flow

705ad0d

rawagner force-pushed the vm_templates_create branch from da79087 to 705ad0d Compare November 19, 2020 17:05

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

openshift-ci-robot assigned glekner Nov 19, 2020

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

openshift-merge-robot merged commit b89136f into openshift:master Nov 20, 2020

spadgett added this to the v4.7 milestone Nov 30, 2020

New VM from template flow #6937

New VM from template flow #6937

Conversation

rawagner commented Oct 15, 2020 • edited

glekner 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

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 Nov 13, 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

atiratree left a comment

Choose a reason for hiding this comment

rawagner commented Nov 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rawagner commented Nov 19, 2020

atiratree commented Nov 19, 2020

rawagner commented Nov 19, 2020

glekner commented Nov 19, 2020

openshift-ci-robot commented Nov 19, 2020

openshift-merge-robot commented Nov 19, 2020

openshift-bot commented Nov 20, 2020

rawagner commented Oct 15, 2020 •

edited