Re-enable Snapshots tab #6807

glekner · 2020-10-01T14:39:27Z

No description provided.

atiratree · 2020-10-02T11:37:00Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-details-page.tsx

@@ -41,6 +49,8 @@ export const breadcrumbsForVMPage = (match: any) => () => [

 export const VirtualMachinesDetailsPage: React.FC<VirtualMachinesDetailsPageProps> = (props) => {
  const { name, ns: namespace } = props.match.params;
+  const [snapData, snapLoaded, snapErr] = useK8sGet(VirtualMachineSnapshotModel);
+  const isSnapshotSupported = snapData && snapLoaded && !snapErr;


can you please look at the original description of this bug https://bugzilla.redhat.com/show_bug.cgi?id=1881125 ? And fix the loading when reintroducing the snapshots again?

glekner · 2020-10-04T13:34:50Z

@suomiy we now look for snapshots only in the relevant namespace. Is this solution sufficient?

atiratree · 2020-10-05T10:11:25Z

@glekner we should check if the CRD is present

pcbailey · 2020-10-05T12:45:54Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-details-page.tsx

 import { VMEnvironmentFirehose } from './vm-environment/vm-environment-page';
 import { VMNics } from '../vm-nics';
 import { PendingChangesWarningFirehose } from './pending-changes-warning';
+import { VMSnapshotsPage } from '../vm-snapshots/vm-snapshots';
+import {


nitpick: This should be above the kubevirt package imports. Of course, we can catch it in a follow-up if you don't have any other changes to make.

glekner · 2020-10-06T12:01:41Z

added useModelLoaded https://github.com/openshift/console/pull/6807/files#diff-1c59bec23f195454582776c62b6085baR64
@suomiy @rawagner

atiratree · 2020-10-06T13:18:55Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-details-page.tsx

@@ -41,6 +49,7 @@ export const breadcrumbsForVMPage = (match: any) => () => [

 export const VirtualMachinesDetailsPage: React.FC<VirtualMachinesDetailsPageProps> = (props) => {
  const { name, ns: namespace } = props.match.params;
+  const isSnapshotsAvailable = useModelLoaded(VirtualMachineSnapshotModel.kind);


shouldn't referenceForModel be used here?

depends on the use case, some would use referenceForModel(model) but in our case the resource in store is called VMSnapshotModel so we need VMSnapshotModel.kind

it might get loaded only with the group+version to the store so we should use referenceForModel to be safe

atiratree · 2020-10-06T13:20:12Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-details-page.tsx

@@ -95,6 +102,7 @@ export const VirtualMachinesDetailsPage: React.FC<VirtualMachinesDetailsPageProp
    consolePage,
    nicsPage,
    disksPage,
+    ...(isSnapshotsAvailable ? [snapshotsPage] : []),


are we expecting more pages? IMO simple if would be more readable.

this is a one line and it is a convention.
check https://github.com/openshift/console/blob/master/frontend/webpack.config.ts#L192 for example

I wouldn't cite webpack.config.ts as a convention. I know there are both use cases in the code base.

This one is less elegant.
Do as you want - I can live with both.

atiratree · 2020-10-06T13:21:28Z

frontend/public/components/utils/k8s-watch-hook.ts

@@ -61,6 +61,9 @@ const useModelsLoaded = (): boolean => {
  return ref.current;
 };

+export const useModelLoaded = (model: string): boolean =>
+  !!useSelector<RootState, K8sKind>(({ k8s }) => k8s.getIn(['RESOURCES', 'models', model]));


we could check for empty model first

react hooks shouldn't be called conditionally. eslint will shout at you

yes, unless the condition is inside

atiratree · 2020-10-06T13:22:04Z

frontend/public/components/utils/k8s-watch-hook.ts

@@ -61,6 +61,9 @@ const useModelsLoaded = (): boolean => {
  return ref.current;
 };

+export const useModelLoaded = (model: string): boolean =>


+1 hook might be useful

You can use https://github.com/openshift/console/blob/master/frontend/packages/console-shared/src/hooks/useK8sModel.ts instead

yeah that sound much better

rawagner · 2020-10-07T13:04:30Z

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

@@ -91,7 +98,22 @@ export const VMSnapshotsPage: React.FC<VMLikeEntityTabProps> = ({ obj: vmLikeEnt
    [namespace],
  );

-  const [snapshots, snapshotsLoaded, snapshotsError] = useK8sWatchResource<VMSnapshot[]>(resource);
+  const restoreResource: WatchK8sResource = React.useMemo(


useK8sWatchResource doesnt require memoized resource anymore :)

atiratree · 2020-10-07T13:21:29Z

frontend/packages/kubevirt-plugin/src/components/vm-snapshots/snapshot-row.tsx

@@ -66,12 +56,14 @@ const getActions = (
    return [];
  }

-  const actions = [menuActionRestore, menuActionDelete];
+  const actions = [menuActionDelete];


what about the snapshot detail later?

snapshot detail?
the restore is now a dedicated button for each row, not a kebab option

are we planning to have also snapshot details?

frontend/packages/kubevirt-plugin/src/components/vm-snapshots/utils.ts

@@ -4,6 +4,8 @@ import { Kebab } from '@console/internal/components/utils';
 export const snapshotsTableColumnClasses = [


glekner · 2020-10-12T10:28:06Z

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

@@ -64,6 +64,13 @@ export const VMSnapshotsTable: React.FC<VMSnapshotsTableProps> = ({
            sortField: 'status.readyToUse',
            transforms: [sortable],
          },
+          {
+            title: 'Last restored',


@suomiy
since the latest restore date is computed inside the row component (because one snapshot can have multiple restore objects) we can't sort the table by it. do you have any suggestions?

https://github.com/openshift/console/pull/6807/files#diff-d4bb250ed498bbf43eeee21385193b71R146

@glekner you can use sortFunc prop

done, hope it won't cause performance issues
https://github.com/openshift/console/pull/6807/files#diff-b2d4cf524cc534d555f71cccff72c6ecR172

I am not a big fan of expecting that the core code should handle our CRDs. Wouldn't it be more straightforward to just resolve the time here and pass it as a data like we do in our other tables?

glekner · 2020-10-12T12:24:45Z

Updated to match latest design

Restore failed popover:

Restore in progress popover:

can you review @suomiy @matthewcarleton @yfrimanm

yfrimanm · 2020-10-12T13:28:56Z

Thanks @glekner
Nit: For the restoring popover, maybe we can make it clearer with: ''Virtual-machine-name 2019-8-28 is being restored from this snapshot''.

atiratree · 2020-10-12T16:54:34Z

frontend/packages/kubevirt-plugin/src/components/vm-snapshots/snapshot-row.tsx

@@ -66,12 +56,14 @@ const getActions = (
    return [];
  }

-  const actions = [menuActionRestore, menuActionDelete];
+  const actions = [menuActionDelete];


are we planning to have also snapshot details?

atiratree · 2020-10-12T16:57:47Z

frontend/packages/kubevirt-plugin/src/components/vm-snapshots/snapshot-row.tsx

+  const snapshotReady = isVMSnapshotReady(snapshot);
+
+  if (snapshotError) {
+    return <ErrorStatus title={'Create Failed'}>{snapshotError?.message}</ErrorStatus>;


can we also visit a snapshot detail when error occurs? like we can do with restore detail?

user can access the snapshot details by clicking on its name in the table

In that case, we are missing the restore action in the detail

atiratree · 2020-10-12T17:06:55Z

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

@@ -6,4 +7,6 @@ export type VMSnapshotRowCustomData = {
  vmLikeEntity: VMLikeEntityKind;
  columnClasses: string[];
  isDisabled: boolean;
+  restores: VMRestore[];
+  withProgress: (promise: Promise<any>) => void;


why do we need withProgress in snapshots?

why not? its a resource creation like any other modal?

because this feature is used only by the wizard

ah you are right, It is used by the detail pages as well

atiratree · 2020-10-12T17:17:07Z

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

@@ -64,6 +64,13 @@ export const VMSnapshotsTable: React.FC<VMSnapshotsTableProps> = ({
            sortField: 'status.readyToUse',
            transforms: [sortable],
          },
+          {
+            title: 'Last restored',


I am not a big fan of expecting that the core code should handle our CRDs. Wouldn't it be more straightforward to just resolve the time here and pass it as a data like we do in our other tables?

atiratree · 2020-10-12T17:21:46Z

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

+  const [restores, restoresLoaded, restoresError] = useK8sWatchResource<VMRestore[]>(
+    restoreResource,
+  );
+  const sortedRestores = React.useMemo(


it might be easier to create a map with getVmRestoreSnapshotName key here for better consumption.
Then it wouldn't need to be even sorted as you could just save the relevant restore.

which snapshotName? one vm can have multiple snapshots with different names? a row for each.

yes, but relevant restore shoul be only one , right?

glekner · 2020-10-15T14:11:11Z

mapped restores by key
changed kebab options to:

Last Restore details (if available)
Delete Snapshot

@suomiy can you review?
the common kebab options can be confusing here because of the "Dual" resource page of snapshots/kebab. if a user wants to play with them, they should visit a specific resource page

atiratree · 2020-10-19T09:21:12Z

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

+
+  const mappedRelevantRestores = React.useMemo(
+    () =>
+      restores.reduce((restoreMap, currentRestore) => {


this would be simpler

restores.reduce((restoreMap, currentRestore) => { const relevantRestore = restoreMap[getVmRestoreSnapshotName(currentRestore)]; if (!relevantRestore || new Date(getVmRestoreTime(relevantRestore)).getTime() < new Date(getVmRestoreTime(currentRestore)).getTime()) { restoreMap[getVmRestoreSnapshotName(currentRestore)] = currentRestore } return restoreMap }, {}),

atiratree · 2020-10-19T09:26:15Z

the common kebab options can be confusing here because of the "Dual" resource page of snapshots/kebab. if a user wants to play with them, they should visit a specific resource page

I was under an impression that it would make more sense to show snapshots as one logical resources (ie as an aggregation of resources like vm/vmi does).
Do you think it makes sense to introduce VMRestore resource to the users? And what advantages does it have?

glekner · 2020-10-20T18:24:07Z

Created custom Details Page for VM Snapshot with Description if available
Added Restore action, Status and Last Restore(same new component like the table)
Removed Last Restore Details kebab option in a Snapshot row
Added breadcrumbs for VM context
Added Description input to Snapshot modal

@suomiy @matthewcarleton @yfrimanm can you take a look

atiratree

@glekner we probably should ask for delete confirmation of snapshots when we delete the VM. The same we do with disks. To prevent stale snapshots being stored in the system.
Thoughts?

Also not sure if we want an owner reference on them.

atiratree · 2020-10-21T09:29:04Z

frontend/packages/kubevirt-plugin/src/components/vm-snapshots/snapshot-details.tsx

+        <div className="row">
+          <div className="col-sm-6">
+            <ResourceSummary resource={obj} showAnnotations={false}>
+              {obj?.metadata?.annotations?.description && (


I think the convention is to show NA when the data is not there

also it might be useful to make it editable

atiratree · 2020-10-21T09:40:38Z

frontend/packages/kubevirt-plugin/src/components/vm-snapshots/snapshot-details.tsx

+    return [
+      {
+        name: 'Virtualization',
+        path: `/k8s/ns/${namespace || 'default'}/virtualization`,


let's make a variable vmNamespace for this

atiratree · 2020-10-21T09:43:05Z

frontend/packages/kubevirt-plugin/src/components/vm-snapshots/snapshot-details.tsx

+    isDisabled: !isVMSnapshotReady(snapshot),
+    callback: () => snapshotRestoreModal({ snapshot }),
+  }),
+  ...common,


there is a discrepancy in the names Snapshot vs Virtual Machine Snapshot

atiratree · 2020-10-21T09:54:17Z

frontend/packages/kubevirt-plugin/src/components/vm-snapshots/snapshot-row.tsx

-        snapshot,
-      }).result,
-    ),
-});

 const menuActionDelete = (


could we have all the actions in the same place? eg menu-actions file? And use a subset of them in places where we need them.

We don't need any of it actually, the only kebab option as of now is Delete.so now just using
const { Delete } = Kebab.factory

atiratree · 2020-10-21T09:55:21Z

frontend/packages/kubevirt-plugin/src/components/vm-snapshots/snapshot-row.tsx

-  ) : (
-    <Status status={readyToUse ? 'Ready' : 'Not Ready'} />
-  );
+export const VMSnapshotStatus: React.FC<VMSnapshotStatusProps> = ({ snapshot, restore }) => {


can you please move the status to standalone file?

atiratree · 2020-10-21T10:03:58Z

frontend/packages/kubevirt-plugin/src/components/vm-snapshots/snapshot-details.tsx

+
+  const [restores, restoresLoaded] = useK8sWatchResource<VMRestore[]>(restoreResource);
+
+  const mappedRelevantRestores = React.useMemo(


could we make a hook for this, since we are using ti twice?

you could include useK8sWatchResource into the new hook and it would return [mappedRestores, loaded, error]

glekner · 2020-10-21T11:35:27Z

@suomiy @rawagner

Implemented suggestions
Created useMappedVMRestores hook
Made DescriptionModal generic to reuse in the snapshot details.

rawagner · 2020-10-21T12:06:53Z

frontend/packages/kubevirt-plugin/src/components/vm-snapshots/use-mapped-vm-restores.ts

+    [restores],
+  );
+
+  return [mappedRelevantRestores, restoresLoaded, restoresError];


you can return directly from React.useMemo so you wont create a new array every time

atiratree · 2020-10-21T12:35:33Z

frontend/packages/kubevirt-plugin/src/components/vm-snapshots/vm-snapshot-details.tsx

+      },
+      {
+        name: `${vmName} Snapshots`,
+        path: `/k8s/ns/${match.params.ns ||


can use the variable here as well

rawagner · 2020-10-22T08:45:03Z

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


-export const VMDescriptionModal = withHandlePromise((props: VMDescriptionModalProps) => {
-  const { vmLikeEntity, inProgress, errorMessage, handlePromise, close, cancel } = props;
+const DescriptionModal = withHandlePromise((props: DescriptionModalProps) => {


Suggested change

const DescriptionModal = withHandlePromise((props: DescriptionModalProps) => {

const DescriptionModal = withHandlePromise<DescriptionModalProps>((props) => {

rawagner · 2020-10-22T08:45:30Z

frontend/packages/kubevirt-plugin/src/components/modals/description-modal/index.ts

@@ -0,0 +1 @@
+export * from './description-modal';


lets avoid creating new index files, they cause cyclic deps and we will be removing them in the future

rawagner · 2020-10-22T08:45:44Z

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

+          <FormRow title="Description" fieldId={asId('desc')}>
+            <TextArea
+              value={description}
+              onChange={(d) => setDescription(d)}


Suggested change

onChange={(d) => setDescription(d)}

onChange={setDescription}

rawagner · 2020-10-22T08:47:29Z

frontend/packages/kubevirt-plugin/src/components/vm-snapshots/vm-snapshot-details.tsx

+      resources={[resource]}
+      menuActions={menuActions}
+      pages={pages}
+      breadcrumbsFor={(obj) => breadcrumbsForSnapshots(obj)}


Suggested change

breadcrumbsFor={(obj) => breadcrumbsForSnapshots(obj)}

breadcrumbsFor={breadcrumbsForSnapshots}

rawagner · 2020-10-22T08:51:41Z

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

@@ -26,7 +25,7 @@ export const getLabelValue = (entity: K8sResourceKind, label: string): string =>

 // Annotations
 export const getAnnotations = (
-  vm: VMGenericLikeEntityKind,
+  vm: K8sResourceKind,


Suggested change

vm: K8sResourceKind,

vm: K8sResourceCommon,

rawagner · 2020-10-22T08:52:56Z

/lgtm

openshift-ci-robot · 2020-10-22T08:53:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: glekner, rawagner

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-ci-robot requested review from jelkosz and pcbailey October 1, 2020 14:39

openshift-ci-robot added the component/kubevirt Related to kubevirt-plugin label Oct 1, 2020

atiratree suggested changes Oct 2, 2020

View reviewed changes

glekner force-pushed the enable-snapshots-feature branch from 072377c to 8b7e188 Compare October 4, 2020 09:32

openshift-ci-robot added the component/shared Related to console-shared label Oct 4, 2020

pcbailey reviewed Oct 5, 2020

View reviewed changes

glekner force-pushed the enable-snapshots-feature branch from 8b7e188 to cfa1281 Compare October 6, 2020 11:58

openshift-ci-robot added the component/core Related to console core functionality label Oct 6, 2020

glekner force-pushed the enable-snapshots-feature branch from cfa1281 to 393ba0b Compare October 6, 2020 11:58

glekner force-pushed the enable-snapshots-feature branch from 393ba0b to d7c82fb Compare October 6, 2020 12:11

atiratree reviewed Oct 6, 2020

View reviewed changes

glekner force-pushed the enable-snapshots-feature branch from d7c82fb to 7671197 Compare October 7, 2020 11:29

rawagner reviewed Oct 7, 2020

View reviewed changes

atiratree reviewed Oct 7, 2020

View reviewed changes

glekner force-pushed the enable-snapshots-feature branch 4 times, most recently from 4be4c72 to 94050a7 Compare October 12, 2020 10:19

glekner commented Oct 12, 2020

View reviewed changes

glekner force-pushed the enable-snapshots-feature branch 3 times, most recently from 3c6c405 to 53faa72 Compare October 12, 2020 12:19

glekner force-pushed the enable-snapshots-feature branch from 53faa72 to c3b95a0 Compare October 12, 2020 12:25

atiratree suggested changes Oct 12, 2020

View reviewed changes

glekner force-pushed the enable-snapshots-feature branch 2 times, most recently from 9d33c17 to 2e359cc Compare October 15, 2020 14:09

atiratree reviewed Oct 19, 2020

View reviewed changes

glekner force-pushed the enable-snapshots-feature branch 4 times, most recently from 3cda155 to 67b9267 Compare October 20, 2020 18:05

glekner force-pushed the enable-snapshots-feature branch from 67b9267 to b17f80b Compare October 21, 2020 09:02

atiratree suggested changes Oct 21, 2020

View reviewed changes

glekner force-pushed the enable-snapshots-feature branch 2 times, most recently from cde5803 to 279df00 Compare October 21, 2020 11:34

glekner force-pushed the enable-snapshots-feature branch from 279df00 to 8be1738 Compare October 21, 2020 11:41

rawagner reviewed Oct 21, 2020

View reviewed changes

atiratree reviewed Oct 21, 2020

View reviewed changes

re-enable snapshots tab

3123c26

glekner force-pushed the enable-snapshots-feature branch from 8be1738 to 3123c26 Compare October 21, 2020 13:20

rawagner reviewed Oct 22, 2020

View reviewed changes

openshift-ci-robot assigned rawagner Oct 22, 2020

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

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

openshift-merge-robot merged commit 36c50a9 into openshift:master Oct 22, 2020

spadgett added this to the v4.7 milestone Oct 26, 2020

		@@ -4,6 +4,8 @@ import { Kebab } from '@console/internal/components/utils';
		export const snapshotsTableColumnClasses = [


		const [restores, restoresLoaded] = useK8sWatchResource<VMRestore[]>(restoreResource);

		const mappedRelevantRestores = React.useMemo(

	const DescriptionModal = withHandlePromise((props: DescriptionModalProps) => {
	const DescriptionModal = withHandlePromise<DescriptionModalProps>((props) => {

	onChange={(d) => setDescription(d)}
	onChange={setDescription}

	breadcrumbsFor={(obj) => breadcrumbsForSnapshots(obj)}
	breadcrumbsFor={breadcrumbsForSnapshots}

Re-enable Snapshots tab #6807

Re-enable Snapshots tab #6807

Conversation

glekner commented Oct 1, 2020

Choose a reason for hiding this comment

glekner commented Oct 4, 2020 • edited

atiratree commented Oct 5, 2020

Choose a reason for hiding this comment

glekner commented Oct 6, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

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

yfrimanm commented Oct 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree Oct 13, 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

glekner commented Oct 15, 2020

Choose a reason for hiding this comment

atiratree commented Oct 19, 2020

glekner commented Oct 20, 2020 • edited

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

glekner commented Oct 21, 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

rawagner commented Oct 22, 2020

openshift-ci-robot commented Oct 22, 2020

glekner commented Oct 4, 2020 •

edited

atiratree Oct 13, 2020 •

edited

glekner commented Oct 20, 2020 •

edited