kubevirt: deprecate using StorageClassConfigMap in inappropriate placess + SC/volume-access modes fixes #4850

atiratree · 2020-03-30T13:10:09Z

introduce useStorageClassConfigMap hook
move volume/access modes resolution into the places where dv/pvc gets created
- from modal
- from import
prefer visible volume/access mode
deprecate assertDefaultModes
add storageClassConfigMap to common data in VMWizard
fix disk modal
- allow empty modes
- allow empty or no storage class
- fix enums
show volume/access modes in ReviewTab
remove async and get requests from modal patches
show CDs in disks tab to allow editing of volume/access modes
allow editing storages until the DV gets created
simplify create/import VM request flow

atiratree · 2020-03-30T13:12:49Z

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

@@ -391,6 +404,8 @@ export const CreateVMWizardPageComponent: React.FC<CreateVMWizardPageComponentPr
    );
  }

+  const storageClassConfigMap = useStorageClassConfigMapWrapped();


this hook should load the correct config map for us. IMO it shouldn't be a problem to do this only once per wizard as the map should not change dynamically. And even if it does change it probably makes sense to have a consistent behaviour for a single instance of a wizard

atiratree · 2020-03-30T13:13:48Z

.../src/components/create-vm-wizard/redux/stateUpdate/vmSettings/providers/vmware-prefill-vm.ts

@@ -120,6 +128,8 @@ export const getDisks = (parsedVm): VMWizardStorage[] => {
          size: size.value,
          unit: size.unit,
        })
+        .setVolumeMode(getDefaultSCVolumeMode(storageClassConfigMap))


all the SC defaults should gets resolved when the storage gets created

atiratree · 2020-03-30T13:14:28Z

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

    isEditing,
    errorMessage,
    handlePromise,
    close,
    cancel,
    templateValidations,
+    storageClassConfigMap: _storageClassConfigMap,


should be agnostic to the source of the config map

atiratree · 2020-03-30T13:15:11Z

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

    }
-  }, [isEditing, source, storageClassName]);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [isLoaded(_storageClassConfigMap)]);


modal disabled until it loads ant then it should set the defaults only once and only for a new storage

atiratree · 2020-03-30T13:16:07Z

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

+      if (newVolumeMode !== volumeMode) {
+        setVolumeMode(newVolumeMode);
+      }
+      if (newAMode !== accessMode || newVolumeMode !== volumeMode) {


notify the user when we change hidden fields by the SC

atiratree · 2020-03-30T13:17:12Z

frontend/packages/kubevirt-plugin/src/constants/vm/storage/access-mode.ts

-  static readonly READ_ONLY = new AccessMode('ReadOnlyMany');
+  static readonly READ_WRITE_ONCE = new AccessMode('ReadWriteOnce', 'Single User (RWO)');
+  static readonly READ_WRITE_MANY = new AccessMode('ReadWriteMany', 'Shared Access (RWX)');
+  static readonly READ_ONLY_MANY = new AccessMode('ReadOnlyMany', 'Read Only (ROX)');


IMO we should use the k8s names as it is can be confusing for a programmer to map these to the k8s ones

@pcbailey

This definitely works for me. I was just making sure that the k8s names were used in the UI because that's what the designers decided was best. I like the way you refactored this.

atiratree · 2020-03-30T13:18:00Z

frontend/packages/kubevirt-plugin/src/k8s/patches/vm/vm-disk-patches.ts

-  }
-  return getVMLikePatches(vmLikeEntity, (vm) => {
+): Patch[] =>
+  getVMLikePatches(vmLikeEntity, (vm) => {


we should not be doing these resolves anymore behind the users back

atiratree · 2020-03-30T13:18:22Z

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

-          getDefaultSCVolumeMode(storageClassConfigMap, dataVolumeWrapper.getStorageClassName()),
-          getDefaultSCAccessModes(storageClassConfigMap, dataVolumeWrapper.getStorageClassName()),
-        );
+        .setNamespace(isPlainDataVolume ? namespace : undefined);


same here and in the v2v storages

pcbailey

One substantial change in sc-defaults.ts, a question about the UX in disk-modal.tsx, a few nitpicks, and some comments. Overall, I really like the changes you made.

pcbailey · 2020-03-31T14:45:54Z

frontend/packages/kubevirt-plugin/src/hooks/storage-class-config-map.ts

+          }
+        }
+      }
+      if (!controller.current.signal.aborted) {


nit: Could you add an empty line here? I think it helps readability to add some vertical space between blocks like this, especially when there's a lot of nesting.

done, I also removed the react ref and moved the controller inside the useEffect. As I realized there is probably not a use case for canceling the request outside of this hook.

pcbailey · 2020-03-31T14:46:27Z

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

-    return acc;
-  }, {}),
+  ...[...DetectCommonDataChanges]
+    .filter((v) => v !== VMWizardProps.storageClassConfigMap) // pased directly


nit: In the comment, 'pased' should be 'passed'.

pcbailey · 2020-03-31T18:44:39Z

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

@@ -197,27 +190,27 @@ export const DiskModal = withHandlePromise((props: DiskModalProps) => {
  if (source.requiresDatavolume()) {
    resultDataVolume = DataVolumeWrapper.initializeFromSimpleData({
      name: resultDataVolumeName,
-      storageClassName,
+      storageClassName: storageClassName || null, // || null are for merge to work


nit: In the comment "|| null are for merge to work" would be more correctly written as "|| null is to enable merging"

pcbailey · 2020-03-31T18:44:47Z

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

  }

  let resultPersistentVolumeClaim;
  if (source.requiresNewPVC()) {
    resultPersistentVolumeClaim = new PersistentVolumeClaimWrapper()
      .init({
        name,
-        storageClassName,
+        storageClassName: storageClassName || null, // || null are for merge to work


nit: In the comment "|| null are for merge to work" would be more correctly written as "|| null is to enable merging"

pcbailey · 2020-03-31T18:49:15Z

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

+    // eslint-disable-next-line eqeqeq
+    if (newStorageClassName != storageClassName) {
+      setStorageClassName(newStorageClassName);
+      const newAMode = getDefaultSCAccessModes(storageClassConfigMap, newStorageClassName)[0];


Why shorten the variable name here? Did it push you over the character limit?

pcbailey · 2020-03-31T19:03:32Z

frontend/packages/kubevirt-plugin/src/constants/vm/storage/access-mode.ts

-  static readonly READ_ONLY = new AccessMode('ReadOnlyMany');
+  static readonly READ_WRITE_ONCE = new AccessMode('ReadWriteOnce', 'Single User (RWO)');
+  static readonly READ_WRITE_MANY = new AccessMode('ReadWriteMany', 'Shared Access (RWX)');
+  static readonly READ_ONLY_MANY = new AccessMode('ReadOnlyMany', 'Read Only (ROX)');


This definitely works for me. I was just making sure that the k8s names were used in the UI because that's what the designers decided was best. I like the way you refactored this.

pcbailey · 2020-04-01T01:11:47Z

frontend/packages/kubevirt-plugin/src/hooks/storage-class-config-map.ts

+      if (!controller.current.signal.aborted) {
+        setStorageClassConfigMap(null);
+        setError(
+          `Could not load storage config map in following namespaces: ${joinGrammaticallyListOfItems(


Should this say "Could not load storage class config map..."?

pcbailey · 2020-04-01T01:13:14Z

frontend/packages/kubevirt-plugin/src/k8s/patches/vm/vm-cdrom-patches.ts

-            getDefaultSCVolumeMode(storageClassConfigMap, storageClassName),
-            getDefaultSCAccessModes(storageClassConfigMap, storageClassName),
-          )
+          .setVolumeMode(getDefaultSCVolumeMode(storageClassConfigMap, storageClassName))


I definitely like this better.

pcbailey · 2020-04-01T01:20:48Z

frontend/packages/kubevirt-plugin/src/selectors/config-map/sc-defaults.ts

@@ -35,12 +35,12 @@ export const getDefaultSCVolumeMode = (
    return volumeMode;
  }

-  return storageClassName === 'local-sc' ? VolumeMode.FILESYSTEM : VolumeMode.BLOCK;
+  return storageClassName === 'local-sc' ? VolumeMode.BLOCK : VolumeMode.FILESYSTEM;


I talked to Alex Wels on the storage team. He said that if nothing is returned from the config map then we can default to FileSystem and RWO since every storage class should support those settings. So, we can drop the check for local-sc and just return FileSystem.

good to know

pcbailey · 2020-04-01T01:22:28Z

frontend/packages/kubevirt-plugin/src/selectors/config-map/sc-defaults.ts

@@ -54,5 +54,7 @@ export const getDefaultSCAccessModes = (
    return [accessMode];
  }

-  return storageClassName === 'local-sc' ? [AccessMode.SINGLE_USER] : [AccessMode.SHARED_ACCESS];
+  return storageClassName === 'local-sc'


See above. We can just return RWO if the config map doesn't return anything.

…ess + SC/volume-access modes fixes - introduce useStorageClassConfigMap hook - move volume/access modes resolution into the places where dv/pvc gets created - from modal - from import - prefer visible volume/access mode - deprecate assertDefaultModes - add storageClassConfigMap to common data in VMWizard - fix disk modal - allow empty modes - allow empty or no storage class - fix enums - show volume/access modes in ReviewTab - remove async and get requests from modal patches - show CDs in disks tab to allow editing of volume/access modes - allow editing storages until the DV gets created - simplify create/import VM request flow

atiratree · 2020-04-01T09:06:13Z

fixed review comments

atiratree · 2020-04-01T09:11:42Z

I added a small enhancement to this PR.

I noticed that we do not notify user in any way when we change access/volume modes when the storage class changes. The user might not know this has occurred and run into some problems in the future.

With this change, the drawer will open when any of the modes change. But it will stay closed when the modes stay the same, upon the storage class change.

Thoughts? @matthewcarleton @Ranelim

yaacov · 2020-04-01T11:19:09Z

This PR adds the CDs and Environment Vars to the discs list.

VMs consume CDs and Environment Vars resources as discs, so it makes seance to add them here, but CD's and Environment are special cases of discs, for example:
a - They have specialized edit and delete options that are not accessible from the disks view.
b - In other places ( e.g. overview, dashboard we separate discs from cds and secrets )
c - We have specialized modals and tables for CDs and Environment Vars.

@matthewcarleton @Ranelim my questions are:
a - Do we have a design to integrate the CDs and Envs in the discks list ?
b - Should we specialize the lines to change the edit/delete options ?
c - Should we indicate that this resources are not "regular" discs ?
d - Should we separate this "disc" objects to 3 different tables ( discs, cds, envs ) ?

atiratree · 2020-04-01T11:52:41Z

This PR adds the CDs and Environment Vars to the discs list.

VMs consume CDs and Environment Vars resources as discs, so it makes seance to add them here, but CD's and Environment are special cases of discs, for example:
a - They have specialized edit and delete options that are not accessible from the disks view.

The disks tab doesn't conflict with these. It does not have a power to change some fields (e.g. config map), but it can change other fields which these other dialogs cannot.

b - In other places ( e.g. overview, dashboard we separate discs from cds and secrets )

agree this should get accounted for

c - We have specialized modals and tables for CDs and Environment Vars.

these modals do not allow editing bus and access volume modes, so it is necessary to have such option for these disks through disk tab.

yaacov · 2020-04-01T12:55:29Z

but it can change other fields which these other dialogs cannot.

Do we want this behavioured, maybe this options were omitted because we want to be opinionated, do we have a design/discussion about this options ?
a - what options we want to expose and what to hide ?
b - if we choose to expose this options, how we do it ?

these modals do not allow editing bus and access volume modes, so it is necessary to have such option for these disks through disk tab.

Maybe a better approach whould be to expose this options (if we want to expose them) in the specific tabs/modals ?

For example this is what happen if I change a secret disk name:

yaacov · 2020-04-01T13:17:49Z

/lgtm

IMHO we will need a follow-up PR for:
a - disable editing name of secrets and config maps.
b - add some indication of the disk type (e.g. secret, cd ... )

openshift-ci-robot · 2020-04-01T13:18:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

yaacov · 2020-04-01T13:19:04Z

c - take care of links from the overview card ...

@matthewcarleton we will need some design thought on adding the new discs typs

atiratree · 2020-04-01T13:39:54Z

Maybe a better approach whould be to expose this options (if we want to expose them) in the specific tabs/modals ?

For example this is what happen if I change a secret disk name:

IMHO we will need a follow-up PR for:
a - disable editing name of secrets and config maps.

this bug was already present before this PR. I posted a fix here: #4873

IMO we should allow changing the bus and the name for all sources as it is available for them. Btw you can do the same through the YAML

matthewcarleton · 2020-04-01T15:37:31Z

@yaacov so just to confirm for a follow up PR we'll need designs to cover how to edit different types in the disks tab (CDs, Environments)?

matthewcarleton · 2020-04-01T15:38:48Z

I added a small enhancement to this PR.

I noticed that we do not notify user in any way when we change access/volume modes when the storage class changes. The user might not know this has occurred and run into some problems in the future.

With this change, the drawer will open when any of the modes change. But it will stay closed when the modes stay the same, upon the storage class change.

Thoughts? @matthewcarleton @Ranelim

Nice catch @suomiy !
Do you think this warrants an inline notification? They may not understand that something changed just because the area is expanded. Also for not sighted users they wouldn't have any indicator right?

atiratree · 2020-04-01T16:14:11Z

so just to confirm for a follow up PR we'll need designs to cover how to edit different types in the disks tab (CDs, Environments)?

and probably adding them? since we can edit them in the table

Do you think this warrants an inline notification? They may not understand that something changed just because the area is expanded. Also for not sighted users they wouldn't have any indicator right?

maybe. I though maybe a highlighting would do but notification would probably give the best idea to all types of users. Could you please take a look at it?
Please take into consideration that just one of these fields might change or both.

Also we are preselecting these values according to the config map when you create new disk and not showing the advanced drawer. I suppose we can probably keep this behaviour, but just in case.

yaacov · 2020-04-01T16:49:06Z

@yaacov so just to confirm for a follow up PR we'll need designs to cover how to edit different types in the disks tab (CDs, Environments)?

@matthewcarleton hi, thanks for the replay, yes, we will need help to align the current changes in the disks tab functionality with the UX design.

We agreed off-line with @jelkosz and @suomiy that we will push this as is, and adjust what needed in follow-up PRs.

The open questions I have are:
1 - In the Overview inventory card we show Discs + Secrets/SA/CM as one group and CDs as another, in the new disks tab we list all entities together, we need help to reconcile the the two views.

2 - In the Details tab we have "Edit CD" modal that is different in one field from the "Edit CD" we have in the disks tab, do we align them ?

3 - in the Environment tab we add/edit/delete the Secrets/CM/SA, now we can do it also from the disks tab, is it ok ? do we need to alert users that deleting some disk will actually change the secrets available for this VM ?

openshift-ci-robot requested review from pcbailey and yaacov March 30, 2020 13:10

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

atiratree commented Mar 30, 2020

View reviewed changes

pcbailey reviewed Apr 1, 2020

View reviewed changes

atiratree force-pushed the kv.modesFix branch from be7a122 to 04a8d37 Compare April 1, 2020 09:01

openshift-ci-robot assigned yaacov Apr 1, 2020

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

openshift-merge-robot merged commit 1258c7a into openshift:master Apr 1, 2020

atiratree mentioned this pull request Apr 2, 2020

kubevirt: fix SC access/volume modes for default URL storage #4893

Merged

spadgett added this to the v4.5 milestone Apr 2, 2020

kubevirt: deprecate using StorageClassConfigMap in inappropriate placess + SC/volume-access modes fixes #4850

kubevirt: deprecate using StorageClassConfigMap in inappropriate placess + SC/volume-access modes fixes #4850

Conversation

atiratree commented Mar 30, 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

atiratree Mar 30, 2020 • edited

Choose a reason for hiding this comment

pcbailey 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

atiratree commented Apr 1, 2020 • edited

atiratree commented Apr 1, 2020

yaacov commented Apr 1, 2020

atiratree commented Apr 1, 2020

yaacov commented Apr 1, 2020

yaacov commented Apr 1, 2020

openshift-ci-robot commented Apr 1, 2020

yaacov commented Apr 1, 2020

atiratree commented Apr 1, 2020

matthewcarleton commented Apr 1, 2020

matthewcarleton commented Apr 1, 2020 • edited

atiratree commented Apr 1, 2020

yaacov commented Apr 1, 2020

atiratree Mar 30, 2020 •

edited

atiratree commented Apr 1, 2020 •

edited

matthewcarleton commented Apr 1, 2020 •

edited