Bug 1813198: Windows CDs use VirtIO instead of sata #5334

glekner · 2020-05-07T12:56:09Z

This should later be fixed when common-templates have validations for CDs

openshift-ci-robot · 2020-05-07T12:56:16Z

@glekner: This pull request references Bugzilla bug 1813198, 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.5.0) matches configured target release for branch (4.5.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1813198: Windows CDs use VirtIO instead of sata

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.

glekner · 2020-05-12T10:25:37Z

@suomiy @yaacov can you review?

glekner · 2020-05-12T11:06:06Z

this should fix warning messages - kubevirt/common-templates#148

atiratree · 2020-05-12T12:35:27Z

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

@@ -86,7 +94,7 @@ const VirtualHardwareTabFirehose: React.FC<VirtualHardwareTabFirehoseProps> = ({
  const diskWrapper = DiskWrapper.initializeFromSimpleData({
    name: availableCDName,
    type: DiskType.CDROM,
-    bus: templateValidations.getDefaultBus(),
+    bus: os?.startsWith('win') ? DiskBus.SATA : templateValidations.getDefaultBus(),


so this will be later deprecated in favor of common templates right? Can you please write a TODO comment somewhere with the PR link: kubevirt/common-templates#148. Together with bugzilla bug to track this.

can we do toLowerCase and includes instead?

atiratree · 2020-05-12T12:37:06Z

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

@@ -185,6 +194,7 @@ type VirtualHardwareConnectedProps = VirtualHardwareTabFirehoseProps & {

 const stateToProps = (state, { wizardReduxID }) => ({
  namespace: iGetCommonData(state, wizardReduxID, VMWizardProps.activeNamespace),
+  os: iGetVmSettingAttribute(state, wizardReduxID, VMSettingsField.OPERATING_SYSTEM),


please use iGetVmSettingValue instead

atiratree · 2020-05-12T12:40:38Z

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

@@ -581,6 +587,7 @@ export type DiskModalProps = {
  showInitialValidation?: boolean;
  isCreateTemplate?: boolean;
  isEditing?: boolean;
+  isWindows?: boolean;


please, let's not add more properties to this component. We can pass the desired value inside the disk value in place where we need it.

atiratree · 2020-05-12T12:42:23Z

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

@@ -458,7 +464,7 @@ export const DiskModal = withHandlePromise((props: DiskModalProps) => {
              isValid={!isValidationError(busValidation)}
              value={asFormSelectValue(bus)}
              id={asId('interface')}
-              isDisabled={isDisabled('interface')}
+              isDisabled={isDisabled('interface') || isWindowsCD}


IMO we should still allow the user to change it. Anyway generally, it is better to pass these values in editConfig instead from outside

Why give the illusion that the user can change the interface?
the only wanted bus for windows && cd is SATA so it should be fixed to SATA.
thats why this change shouldn't be deprecated, IMO

what about scsi? IMo we should use only what comes from the template and don't have a special logic like this one

atiratree · 2020-05-12T12:48:48Z

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

@@ -458,7 +464,7 @@ export const DiskModal = withHandlePromise((props: DiskModalProps) => {
              isValid={!isValidationError(busValidation)}


btw shouldn't we change TemplateValidations to react to cd-roms in this PR as well. And allow sata's for now until common-templates are fixed?

glekner · 2020-05-17T12:14:07Z

@suomiy I'm trying to make this work on my local env with the updated templates.

I didn't want to pass a disk to the TemplateValidation constructor as it seems out of place, I ended up cloning methods for CD type. This works for creating a VM from scratch, but when choosing a Windows Template I created, I still see the unsupported buses. can you take a look?

Thanks!

atiratree · 2020-05-18T13:37:54Z

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

@@ -47,6 +51,17 @@ export class TemplateValidations {
    return new Set(allowedBuses.length === 0 ? DiskBus.getAll() : allowedBuses);
  };

+  getAllowedCDBuses = (


Can't we just pass the DiskType to all these functions so we don't have to duplicate the functionality?

still relevant, can we just pass the DiskType and not have the same logic twice?

atiratree · 2020-05-18T13:38:28Z

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

 import { CommonTemplatesValidation } from '../../../types/template';
 import {
  IntervalValidationResult,
  MemoryIntervalValidationResult,
 } from './interval-validation-result';
 import { DiskBusValidationResult } from './disk-bus-validation-result';
+import { DiskWrapper } from 'packages/kubevirt-plugin/src/k8s/wrapper/vm/disk-wrapper';

 export class ValidationJSONPath extends ObjectEnum<string> {
  static readonly CPU = new ValidationJSONPath('jsonpath::.spec.domain.cpu.cores');
  static readonly MEMORY = new ValidationJSONPath(
    'jsonpath::.spec.domain.resources.requests.memory',
  );
  static readonly BUS = new ValidationJSONPath('jsonpath::.spec.domain.devices.disks[*].disk.bus');


can we name this one DiskBus?

atiratree · 2020-05-18T13:40:04Z

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

    );
  };

  validateBus = (
-    bus: DiskBus,
+    disk: DiskWrapper,


can we send just the bus and the type? So we don't propagate Wrapper API to here

atiratree · 2020-05-18T14:09:47Z

I didn't want to pass a disk to the TemplateValidation constructor as it seems out of place, I ended up cloning methods for CD type. This works for creating a VM from scratch, but when choosing a Windows Template I created, I still see the unsupported buses. can you take a look?

This happens because the created template doesn't include the validations anymore. We should probably copy the validations when we are creating the template from common templates.

atiratree · 2020-05-18T16:40:41Z

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

@@ -14,7 +14,12 @@ export class ValidationJSONPath extends ObjectEnum<string> {
  static readonly MEMORY = new ValidationJSONPath(
    'jsonpath::.spec.domain.resources.requests.memory',
  );
-  static readonly BUS = new ValidationJSONPath('jsonpath::.spec.domain.devices.disks[*].disk.bus');
+  static readonly DISKBUS = new ValidationJSONPath(


Suggested change

static readonly DISKBUS = new ValidationJSONPath(

static readonly DISK_BUS = new ValidationJSONPath(

+ CD_BUS

atiratree · 2020-05-18T16:41:42Z

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

      if (!resultValidation.isValid && resultValidation.type === ValidationErrorType.Error) {
        someBusChanged = true;
-        finalDisk = new DiskWrapper(disk, true)
+        finalDisk = diskWrapper


can you please revert this change? This is done, so there is no unnecessary copying when the validation passes fine

atiratree · 2020-05-18T16:43:11Z

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

@@ -47,6 +51,17 @@ export class TemplateValidations {
    return new Set(allowedBuses.length === 0 ? DiskBus.getAll() : allowedBuses);
  };

+  getAllowedCDBuses = (


still relevant, can we just pass the DiskType and not have the same logic twice?

atiratree · 2020-05-18T16:45:16Z

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

    const otherRecommendedBuses = otherTempValidations.getRecommendedBuses();
+    const otherRecommendedCDBuses = otherTempValidations.getRecommendedCDBuses();

    return (


maybe we could do a lazy compare here? First get AllowedBuses and compare if not equal return. Then get aand compare recommended. Etc.

atiratree · 2020-05-18T16:46:37Z

frontend/report.20200518.185441.7687.0.001.json

@@ -0,0 +1,764 @@
+


runaway file?

glekner · 2020-05-18T19:33:30Z

applied suggestions and added validations to templates
@suomiy

atiratree · 2020-05-19T09:04:21Z

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

@@ -97,11 +97,16 @@ export const internalStorageDiskBusUpdater = ({
        VMWizardStorageType.WINDOWS_GUEST_TOOLS,
      ].includes(type)
    ) {
-      const resultValidation = newValidations.validateBus(new DiskWrapper(disk).getDiskBus());
+      const resultValidation = newValidations.validateBus(
+        new DiskWrapper(disk).getType(),


you can make a variable for these two lines

atiratree · 2020-05-19T09:05:05Z

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

      if (!resultValidation.isValid && resultValidation.type === ValidationErrorType.Error) {
        someBusChanged = true;
        finalDisk = new DiskWrapper(disk, true)
-          .appendTypeData({ bus: newValidations.getDefaultBus().getValue() })
+          .appendTypeData({
+            bus: newValidations.getDefaultBus(new DiskWrapper(disk).getType()).getValue(),


and for getting the type here

atiratree · 2020-05-19T09:06:52Z

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

+      const resultValidation = newValidations.validateBus(
+        new DiskWrapper(disk).getType(),
+        new DiskWrapper(disk).getDiskBus(),
+      );
      if (!resultValidation.isValid && resultValidation.type === ValidationErrorType.Error) {
        someBusChanged = true;
        finalDisk = new DiskWrapper(disk, true)


the main difference is that the final disk is mutated, so it needs to be copied to its own wrapper

atiratree · 2020-05-19T09:07:52Z

frontend/packages/kubevirt-plugin/src/constants/vm/constants.ts

@@ -5,6 +5,7 @@ export const ANNOTATION_FIRST_BOOT = 'kubevirt.ui/firstBoot';
 export const ANNOTATION_DESCRIPTION = 'description';
 export const ANNOTATION_PXE_INTERFACE = 'kubevirt.ui/pxeInterface';
 export const CUSTOM_FLAVOR = 'Custom';
+export const VALIDATIONS = 'validations';


+1 for the constant. Can we add ANNOTATION to its name?

atiratree · 2020-05-19T09:09:38Z

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

@@ -50,6 +51,7 @@ export const initializeCommonMetadata = (
  if (template) {
    entity.addLabel(LABEL_USED_TEMPLATE_NAME, getName(template));
    entity.addLabel(LABEL_USED_TEMPLATE_NAMESPACE, getNamespace(template));
+    entity.addAnotation(VALIDATIONS, getAnnotations(template)?.[VALIDATIONS]);


can you please make a new function called initializeCommonTemplateMetadata and put it there instead?
This function is also used for creating VMs and we are not really expecting VMs to include validations.

atiratree · 2020-05-19T09:11:43Z

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

@@ -63,50 +70,67 @@ export class TemplateValidations {
    if (this === otherTempValidations) {
      return true;
    }
+    const isBusesEqual = (buses, otherBuses) =>


+1 for having a function. Can we move it one layer up to the class and make it private?

atiratree · 2020-05-19T09:12:50Z

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

-      [...allowedBuses].every((bus) => otherAllowedBuses.has(bus)) &&
-      [...recommendedBuses].every((bus) => otherRecommendedBuses.has(bus))
-    );
+    const allowedBuses = this.getAllowedBuses(DiskType.DISK);


We could also do [DiskType.DISK, DiskType.CDROM].every but I do not mind either way

atiratree · 2020-05-19T11:14:53Z

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

@@ -97,11 +97,15 @@ export const internalStorageDiskBusUpdater = ({
        VMWizardStorageType.WINDOWS_GUEST_TOOLS,
      ].includes(type)
    ) {
-      const resultValidation = newValidations.validateBus(new DiskWrapper(disk).getDiskBus());
+      const diskType = new DiskWrapper(disk).getType();


better, but still the diskWrapper can be reused

atiratree · 2020-05-19T11:16:20Z

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

+  entity: VMWrapper | VMTemplateWrapper,
+  template?: TemplateKind,
+) => {
+  initializeCommonMetadata(settings, entity, template);


please use the same pattern as VM and call the initializeCommonMetadata separately

atiratree · 2020-05-19T11:17:18Z

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

@@ -91,3 +92,18 @@ export const initializeCommonVMMetadata = (
    );
  }
 };
+
+export const initializeCommonTemplateMetadata = (
+  settings: {


in that case you can remove the settings here and it will simplify the API

atiratree · 2020-05-19T11:18:08Z

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

@@ -91,3 +92,18 @@ export const initializeCommonVMMetadata = (
    );
  }
 };
+
+export const initializeCommonTemplateMetadata = (


this function should be called - to initialize common template metadata

atiratree · 2020-05-19T11:19:58Z

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

@@ -67,3 +67,6 @@ export const combineIntegerValidationResults = (
  }
  return asValidationObject(message, finalType);
 };
+
+export const isSetEqual = (set: Set<any>, otherSet: Set<any>) =>


this can be rather moved to src/utils/common.ts

atiratree · 2020-05-19T12:09:49Z

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

@@ -91,3 +94,11 @@ export const initializeCommonVMMetadata = (
    );
  }
 };
+
+export const initializeCommonTemplateMetadata = (
+  entity: VMWrapper | VMTemplateWrapper,


Suggested change

entity: VMWrapper | VMTemplateWrapper,

entity: VMTemplateWrapper,

atiratree · 2020-05-19T12:11:15Z

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

+  template?: TemplateKind,
+) => {
+  entity.addLabel(TEMPLATE_TYPE_LABEL, TEMPLATE_TYPE_VM);
+  entity.addAnotation(ANNOTATION_VALIDATIONS, getAnnotations(template)?.[ANNOTATION_VALIDATIONS]);


we should check for undefined/null here so we don't add epmty annotations label

glekner · 2020-05-19T12:55:50Z

/retest

atiratree · 2020-05-19T13:17:37Z

/lgtm

openshift-ci-robot · 2020-05-19T13:17:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: glekner, 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-05-19T13:55:46Z

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

In response to this:

Bug 1813198: Windows CDs use VirtIO instead of sata

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-05-19T20:00:25Z

/cherry-pick release-4.4

openshift-cherrypick-robot · 2020-05-19T20:00:30Z

@yaacov: #5334 failed to apply on top of branch "release-4.4":

Using index info to reconstruct a base tree...
A	frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/redux/state-update/storage-tab-state-update.ts
M	frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/virtual-hardware-tab/virtual-hardware-tab.tsx
M	frontend/packages/kubevirt-plugin/src/components/modals/disk-modal/disk-modal.tsx
M	frontend/packages/kubevirt-plugin/src/constants/vm/constants.ts
M	frontend/packages/kubevirt-plugin/src/k8s/requests/vm/create/common.ts
M	frontend/packages/kubevirt-plugin/src/k8s/requests/vm/create/create.ts
M	frontend/packages/kubevirt-plugin/src/k8s/requests/vm/create/default-template.ts
A	frontend/packages/kubevirt-plugin/src/utils/common.ts
M	frontend/packages/kubevirt-plugin/src/utils/validations/template/template-validations.ts
M	frontend/packages/kubevirt-plugin/src/utils/validations/vm/disk.ts
Falling back to patching base and 3-way merge...
Auto-merging frontend/packages/kubevirt-plugin/src/utils/validations/vm/disk.ts
Auto-merging frontend/packages/kubevirt-plugin/src/utils/validations/template/template-validations.ts
CONFLICT (content): Merge conflict in frontend/packages/kubevirt-plugin/src/utils/validations/template/template-validations.ts
CONFLICT (modify/delete): frontend/packages/kubevirt-plugin/src/utils/common.ts deleted in HEAD and modified in Fixes 1813198: Windows CDs use VirtIO instead of sata. Version Fixes 1813198: Windows CDs use VirtIO instead of sata of frontend/packages/kubevirt-plugin/src/utils/common.ts left in tree.
Auto-merging frontend/packages/kubevirt-plugin/src/k8s/requests/vm/create/default-template.ts
CONFLICT (content): Merge conflict in frontend/packages/kubevirt-plugin/src/k8s/requests/vm/create/default-template.ts
Auto-merging frontend/packages/kubevirt-plugin/src/k8s/requests/vm/create/create.ts
CONFLICT (content): Merge conflict in frontend/packages/kubevirt-plugin/src/k8s/requests/vm/create/create.ts
Auto-merging frontend/packages/kubevirt-plugin/src/k8s/requests/vm/create/common.ts
CONFLICT (content): Merge conflict in frontend/packages/kubevirt-plugin/src/k8s/requests/vm/create/common.ts
Auto-merging frontend/packages/kubevirt-plugin/src/constants/vm/constants.ts
Auto-merging frontend/packages/kubevirt-plugin/src/components/modals/disk-modal/disk-modal.tsx
Auto-merging frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/virtual-hardware-tab/virtual-hardware-tab.tsx
Auto-merging frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/redux/stateUpdate/vmSettings/storage-tab-state-update.ts
CONFLICT (content): Merge conflict in frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/redux/stateUpdate/vmSettings/storage-tab-state-update.ts
error: Failed to merge in the changes.
Patch failed at 0001 Fixes 1813198: Windows CDs use VirtIO instead of sata

In response to this:

/cherry-pick release-4.4

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-05-19T20:01:50Z

@glekner hi, ^^ looks like this should be backported by hand :-(

openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels May 7, 2020

openshift-ci-robot added the component/kubevirt Related to kubevirt-plugin label May 7, 2020

openshift-ci-robot requested review from rhrazdil and atiratree May 7, 2020 12:56

glekner force-pushed the fix-1813198 branch from 8cbec5f to 2f0692c Compare May 7, 2020 13:47

glekner force-pushed the fix-1813198 branch from 2f0692c to 7632f69 Compare May 12, 2020 11:58

atiratree suggested changes May 12, 2020

View reviewed changes

glekner force-pushed the fix-1813198 branch from 7632f69 to 5927c7a Compare May 17, 2020 12:12

atiratree suggested changes May 18, 2020

View reviewed changes

glekner force-pushed the fix-1813198 branch from 5927c7a to 0973f35 Compare May 18, 2020 16:24

atiratree suggested changes May 18, 2020

View reviewed changes

glekner force-pushed the fix-1813198 branch 2 times, most recently from 7a2aa21 to afc35f1 Compare May 18, 2020 19:30

atiratree suggested changes May 19, 2020

View reviewed changes

glekner force-pushed the fix-1813198 branch from afc35f1 to e1647b6 Compare May 19, 2020 10:53

atiratree suggested changes May 19, 2020

View reviewed changes

glekner force-pushed the fix-1813198 branch 2 times, most recently from f5ca0c3 to 56eeec3 Compare May 19, 2020 12:07

atiratree suggested changes May 19, 2020

View reviewed changes

Fixes 1813198: Windows CDs use VirtIO instead of sata

23d9cd2

glekner force-pushed the fix-1813198 branch from 56eeec3 to 23d9cd2 Compare May 19, 2020 12:16

atiratree approved these changes May 19, 2020

View reviewed changes

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

openshift-ci-robot assigned atiratree May 19, 2020

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

openshift-merge-robot merged commit defbd3a into openshift:master May 19, 2020

yaacov mentioned this pull request May 20, 2020

Bug 1817394: Windows CDs use VirtIO instead of sata #5496

Merged

spadgett added this to the v4.5 milestone May 22, 2020

		@@ -458,7 +464,7 @@ export const DiskModal = withHandlePromise((props: DiskModalProps) => {
		isValid={!isValidationError(busValidation)}

	static readonly DISKBUS = new ValidationJSONPath(
	static readonly DISK_BUS = new ValidationJSONPath(

	entity: VMWrapper \| VMTemplateWrapper,
	entity: VMTemplateWrapper,

Bug 1813198: Windows CDs use VirtIO instead of sata #5334

Bug 1813198: Windows CDs use VirtIO instead of sata #5334

Conversation

glekner commented May 7, 2020

openshift-ci-robot commented May 7, 2020

glekner commented May 12, 2020

glekner commented May 12, 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

glekner commented May 17, 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

atiratree commented May 18, 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

glekner commented May 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glekner commented May 19, 2020

atiratree commented May 19, 2020

openshift-ci-robot commented May 19, 2020

openshift-ci-robot commented May 19, 2020

yaacov commented May 19, 2020

openshift-cherrypick-robot commented May 19, 2020

yaacov commented May 19, 2020