Bug 1803780: Validate disk buses on created VMs #4335

irosenzw · 2020-02-17T12:56:49Z

Validate disk bus type for created VMs storage disks
using validations written in the template
the VMs were created from.

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

openshift-ci-robot · 2020-02-17T12:56:55Z

@irosenzw: This pull request references Bugzilla bug 1803780, 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 1803780: Validate disk buses on created VMs

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-02-18T10:02:53Z

/retest

atiratree · 2020-02-18T11:19:21Z

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

@@ -117,6 +120,7 @@ const DiskModalFirehose: React.FC<DiskModalFirehoseProps> = (props) => {
        vmLikeEntity={vmLikeEntity}
        namespace={namespace}
        onNamespaceChanged={(n) => setNamespace(n)}
+        templateValidations={restProps.templateValidations}


no need for this line

atiratree · 2020-02-18T11:19:40Z

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

@@ -61,6 +62,7 @@ const DiskModalFirehoseComponent: React.FC<DiskModalFirehoseComponentProps> = (p
      volume={volumeWrapper}
      dataVolume={dataVolumeWrapper}
      onSubmit={onSubmit}
+      templateValidations={restProps.templateValidations}


atiratree · 2020-02-18T11:21:05Z

frontend/packages/kubevirt-plugin/src/components/vm-disks/disk-row.tsx

@@ -80,11 +84,11 @@ const getActions = (

  const isTemplate = vmLikeEntity && !isVM(vmLikeEntity);
  if (disk.isEditingSupported(isTemplate)) {
-    actions.push(menuActionEdit);
+    actions.push(menuActionEdit(disk, vmLikeEntity, templateValidations, opts));


can we please pass it in the opts?

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

atiratree · 2020-02-18T12:44:23Z

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

@@ -86,3 +90,14 @@ export const getTemplateOperatingSystems = (templates: TemplateKind[]) => {

 export const getTemplateWorkloadProfiles = (templates: TemplateKind[]) =>
  getTemplatesLabelValues(templates, TEMPLATE_WORKLOAD_LABEL);
+
+export const getTemplateOfVM = (


we don't need all these selectors

atiratree · 2020-02-18T12:46:22Z

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

@@ -62,6 +63,9 @@ export const getOperatingSystemName = (vmLike: VMGenericLikeEntityKind) =>
    `${TEMPLATE_OS_NAME_ANNOTATION}/${getOperatingSystem(vmLike)}`,
  );

+export const getTemplateName = (vm: VMGenericLikeEntityKind) =>


there is already a function getVMTemplateNamespacedName - can we refactor it so it uses these constants?

this function is not needed anymore. Can we remove it?

atiratree · 2020-02-18T12:47:08Z

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

  const [isLocked, setIsLocked] = useSafetyFirst(false);
  const withProgress = wrapWithProgress(setIsLocked);
+  const relevantTemplate = getTemplateOfVM(vmLikeEntity, commonTemplates.data, userTemplates.data);
+  const templateValidations = new TemplateValidations(
+    iGetTemplateValidations(fromJS(relevantTemplate)),


can we create a basic selector getTemplateValidations instead?

atiratree · 2020-02-18T12:50:32Z

frontend/packages/kubevirt-plugin/src/components/vm-disks/disk-row.tsx

+        diskValidations && {
+          name: diskValidations.validations.name,
+          size: diskValidations.validations.size,
+          diskInterface: diskValidations.validations.diskInterface,


lets show the other validations in source

there is a source validation field

not on UIDiskValidation type.
Here I'm using the result from validateDisk which returns UIDiskValidation object and not StorageSimpleDataValidation

+1 for fixing the source mapping

atiratree · 2020-02-18T12:54:43Z

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

@@ -147,7 +176,7 @@ export const VMDisks: React.FC<VMDisksProps> = ({ vmLikeEntity, pvcs, datavolume
      )}
      <div className="co-m-pane__body">
        <VMDisksTable
-          data={getStoragesData({ vmLikeEntity, pvcs, datavolumes })}
+          data={getStoragesData({ vmLikeEntity, pvcs, datavolumes, templateValidations })}
          customData={{


lets pass the templateValidations to customData and not bundle it with each disk as it is the same entity

spadgett · 2020-02-20T15:19:41Z

/bugzilla refresh

openshift-ci-robot · 2020-02-20T15:19:45Z

@spadgett: This pull request references Bugzilla bug 1803780, which is invalid:

expected the bug to target the "4.5.0" release, but it targets "4.4.0" 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:

/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.

irosenzw · 2020-02-24T09:23:57Z

/retest

irosenzw · 2020-02-25T13:43:17Z

/bugzilla refresh

openshift-ci-robot · 2020-02-25T13:43:21Z

@irosenzw: This pull request references Bugzilla bug 1803780, which is valid.

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.

atiratree · 2020-02-26T12:17:02Z

frontend/packages/kubevirt-plugin/src/components/vm-disks/disk-row.tsx

+        diskValidations && {
+          name: diskValidations.validations.name,
+          size: diskValidations.validations.size,
+          diskInterface: diskValidations.validations.diskInterface,


there is a source validation field

atiratree · 2020-02-26T12:21:46Z

frontend/packages/kubevirt-plugin/src/components/vm-disks/disk-row.tsx

-  actions.push(menuActionDelete);
-  return actions.map((a) => a(disk, vmLikeEntity, opts));
+  actions.push(menuActionDelete(disk, vmLikeEntity, opts));
+  return actions;


IMO the map approach is better

less code duplication

one will not forget to add it when adding another action

atiratree · 2020-02-26T12:23:10Z

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

@@ -173,6 +200,12 @@ export const VMDisksFirehose: React.FC<VMLikeEntityTabProps> = ({ obj: vmLikeEnt
      namespace,
      prop: 'datavolumes',
    }),
+    getResource(TemplateModel, {
+      name: vmTemplate.name,


Suggested change

name: vmTemplate.name,

name: vmTemplate?.name,

namespace: vmTemplate?.namespace,

atiratree · 2020-02-26T12:23:42Z

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

@@ -62,6 +63,9 @@ export const getOperatingSystemName = (vmLike: VMGenericLikeEntityKind) =>
    `${TEMPLATE_OS_NAME_ANNOTATION}/${getOperatingSystem(vmLike)}`,
  );

+export const getTemplateName = (vm: VMGenericLikeEntityKind) =>


this function is not needed anymore. Can we remove it?

atiratree · 2020-02-26T12:24:38Z

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

@@ -163,6 +189,7 @@ export const VMDisks: React.FC<VMDisksProps> = ({ vmLikeEntity, pvcs, datavolume

 export const VMDisksFirehose: React.FC<VMLikeEntityTabProps> = ({ obj: vmLikeEntity }) => {
  const namespace = getNamespace(vmLikeEntity);
+  const vmTemplate = getVMTemplateNamespacedName(vmLikeEntity);


can you please refactor this function so it uses LABEL_USED_TEMPLATE_NAME and LABEL_USED_TEMPLATE_NAMESPACE constants

it uses these constants. am I missing something ?

I already posted a PR which fixed that

atiratree · 2020-02-26T12:26:26Z

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

  const [isLocked, setIsLocked] = useSafetyFirst(false);
  const withProgress = wrapWithProgress(setIsLocked);
+  const templateValidations = getTemplateValidationsFromTemplate(vmTemplate.data);


Suggested change

const templateValidations = getTemplateValidationsFromTemplate(vmTemplate.data);

const templateValidations = new TemplateValidations(getTemplateValidations(getLoadedData(vmTemplate, [])));

atiratree · 2020-02-26T12:27:28Z

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

@@ -55,6 +58,9 @@ export const getTemplatesLabelValues = (templates: TemplateKind[], label: string
  return labelValues;
 };

+export const getTemplateValidationsFromTemplate = (vmTemplate: TemplateKind): TemplateValidations =>
+  new TemplateValidations(iGetTemplateValidations(fromJS(vmTemplate)));


this should be a basic selector. It is very inefficient to create an immutable to just select some stuff

openshift-ci-robot · 2020-03-01T10:50:53Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Needs approval from an approver in each of these files:

frontend/OWNERS

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-03-01T11:00:57Z

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

Test name	Commit	Details	Rerun command
ci/prow/verify	`729e3d0`	link	`/test verify`

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.

Bug 1807212: Fix async bug where models not loaded in time

atiratree · 2020-03-02T11:43:55Z

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

+export const getTemplateValidationsFromTemplate = (
+  vmTemplate: TemplateKind,
+): TemplateValidations => {
+  const result = _.get(vmTemplate, ['metadata', 'annotations', 'validations'], []);


we can use ts optional chaining here and probably do not need the default

atiratree · 2020-03-02T11:44:49Z

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

+): TemplateValidations => {
+  const result = _.get(vmTemplate, ['metadata', 'annotations', 'validations'], []);
+  try {
+    return new TemplateValidations(JSON.parse(result));


IMO, I would just return the raw data and not construct the object TemplateValidations here

+it would be consistent with the iGetTemplateValidations version

atiratree · 2020-03-02T11:47:20Z

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

+  }
+};
+
+export const getTemplateFlavors = (vmTemplates: TemplateKind[]) =>


all this stuff doesn't belong here - wrong rebase/squash?

atiratree · 2020-03-02T11:47:42Z

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

@@ -24,3 +37,99 @@ export const getVMTemplate = (
      )
    : undefined;
 };
+
+export const getTemplatesLabelValues = (templates: TemplateKind[], label: string) => {


doesn't belong here

atiratree · 2020-03-02T11:50:16Z

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

+  datavolumes,
+  vmTemplate,
+}) => {
+  // console.log('vmTemplate', vmTemplate);


forgotten comment?

atiratree · 2020-03-02T11:53:35Z

frontend/integration-tests/tests/crud.scenario.ts

@@ -405,6 +405,8 @@ describe('Kubernetes resource CRUD operations', () => {
            '/k8s/ns/openshift-monitoring/monitoring.coreos.com~v1~Alertmanager/main',
            '/settings/cluster',
            '/monitoring/query-browser',
+            // Test loading search page for a kind with no static model.


code from different PR

atiratree · 2020-03-02T11:53:44Z

frontend/public/components/search.tsx

@@ -145,7 +145,12 @@ const SearchPage_: React.FC<SearchProps> = (props) => {
  };

  const getToggleText = (item: string) => {
-    const { labelPlural, apiVersion, apiGroup } = modelFor(item);
+    const model = modelFor(item);
+    // API discovery happens asynchronously. Avoid runtime errors if the model hasn't loaded.


atiratree · 2020-03-02T11:55:33Z

frontend/packages/kubevirt-plugin/src/components/vm-disks/disk-row.tsx

+        diskValidations && {
+          name: diskValidations.validations.name,
+          size: diskValidations.validations.size,
+          diskInterface: diskValidations.validations.diskInterface,


+1 for fixing the source mapping

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 17, 2020

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 17, 2020

openshift-ci-robot requested review from mareklibra and atiratree February 17, 2020 12:57

openshift-ci-robot added the component/kubevirt Related to kubevirt-plugin label Feb 17, 2020

atiratree suggested changes Feb 18, 2020

View reviewed changes

atiratree reviewed Feb 18, 2020

View reviewed changes

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

irosenzw force-pushed the validate-disk-bus-for-already-created-vms branch from 29b7cce to 729e3d0 Compare February 23, 2020 14:41

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 Feb 25, 2020

atiratree suggested changes Feb 26, 2020

View reviewed changes

irosenzw force-pushed the validate-disk-bus-for-already-created-vms branch from 729e3d0 to ded47b8 Compare February 27, 2020 14:24

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

irosenzw force-pushed the validate-disk-bus-for-already-created-vms branch from ded47b8 to a7972dc Compare March 1, 2020 10:50

Merge pull request openshift#4512 from zherman0/bz1807212

feddb0a

Bug 1807212: Fix async bug where models not loaded in time

irosenzw force-pushed the validate-disk-bus-for-already-created-vms branch from a7972dc to feddb0a Compare March 1, 2020 13:02

atiratree reviewed Mar 2, 2020

View reviewed changes

irosenzw closed this Mar 3, 2020

atiratree mentioned this pull request Mar 3, 2020

Bug 1803780: Validate disk buses on created VMs #4600

Merged

	name: vmTemplate.name,
	name: vmTemplate?.name,
	namespace: vmTemplate?.namespace,

	const templateValidations = getTemplateValidationsFromTemplate(vmTemplate.data);
	const templateValidations = new TemplateValidations(getTemplateValidations(getLoadedData(vmTemplate, [])));

Bug 1803780: Validate disk buses on created VMs #4335

Bug 1803780: Validate disk buses on created VMs #4335

Conversation

irosenzw commented Feb 17, 2020

openshift-ci-robot commented Feb 17, 2020

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

spadgett commented Feb 20, 2020

openshift-ci-robot commented Feb 20, 2020

irosenzw commented Feb 24, 2020

irosenzw commented Feb 25, 2020

openshift-ci-robot commented Feb 25, 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

openshift-ci-robot commented Mar 1, 2020

openshift-ci-robot commented Mar 1, 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

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 Mar 1, 2020 •

edited