Add VMs to Topology View #5041

jeff-phillips-18 · 2020-04-14T18:26:52Z

Fixes:
https://issues.redhat.com/browse/ODC-3329
https://issues.redhat.com/browse/ODC-3497

Analysis / Root cause:
As a developer I want to quickly know which of my workloads are based on VMs.

As a developer I expect every node in topology to be selectable and show some details in the side panel. I want to quickly access information about my VMs.

Solution Description:
Add virtual machine items to the topology view. When a VM node is selected, show its details in the side panel.

Screen shots / Gifs for design review:

Browser conformance:

Chrome
Firefox
Safari
Edge

cc @openshift/team-devconsole-ux @serenamarie125 @sspeiche @jelkosz

/kind feature

serenamarie125

@jeff-phillips-18 can you add screenshots of the Actions menu & if you've implemented the context sensitive actions within topology?

serenamarie125 · 2020-04-14T22:27:38Z

fyi @Veethika @matthewcarleton

jeff-phillips-18 · 2020-04-15T10:47:48Z

@serenamarie125

matthewcarleton · 2020-04-15T18:44:59Z

@serenamarie125

This is looking great @jeff-phillips-18 !
A few questions:
Should the badge be "V" or "VM"
Can we change it so in the details it only says "Pod" and not "Pods"
For the actions I think we can drop the repetitive "Virtual Machine"

jeff-phillips-18 · 2020-04-15T20:31:47Z

This is looking great @jeff-phillips-18 !
A few questions:
Should the badge be "V" or "VM"
Can we change it so in the details it only says "Pod" and not "Pods"
For the actions I think we can drop the repetitive "Virtual Machine"

This is all done within the plugin already. The badge comes from the model, the side panel details show is the same as the details panel for a VM, and the actions are defined in the existing kebab actions.

matthewcarleton · 2020-04-15T22:45:11Z

This is looking great @jeff-phillips-18 !
A few questions:
Should the badge be "V" or "VM"
Can we change it so in the details it only says "Pod" and not "Pods"
For the actions I think we can drop the repetitive "Virtual Machine"

This is all done within the plugin already. The badge comes from the model, the side panel details show is the same as the details panel for a VM, and the actions are defined in the existing kebab actions.

ah, ok thanks. I'll file a bug on the CNV side then.
So I would see the same results in the details view via the admin perspective then?

jeff-phillips-18 · 2020-04-16T11:06:05Z

ah, ok thanks. I'll file a bug on the CNV side then.
So I would see the same results in the details view via the admin perspective then?

Correct

matthewcarleton · 2020-04-16T13:37:46Z

ah, ok thanks. I'll file a bug on the CNV side then.
So I would see the same results in the details view via the admin perspective then?

Correct

oh interesting this is what we see in the VM details view for the badge and the pod. Would this be customized to achieve it?

jelkosz · 2020-04-16T14:59:17Z

frontend/packages/kubevirt-plugin/src/topology/components/nodes/VmNode.tsx

+  VM_STATUS_V2V_CONVERSION_IN_PROGRESS,
+  VM_STATUS_V2V_CONVERSION_PENDING,
+  VM_STATUS_VMI_WAITING,
+} from '../../../statuses/vm/constants';


very recently 2 new more statuses were added: VM_STATUS_V2V_VM_IMPORT_IN_PROGRESS (should go under kubevirt-m-importing) and VM_STATUS_V2V_VM_IMPORT_ERROR (should go under kubevirt-m-error)

jeff-phillips-18 · 2020-04-16T15:35:14Z

@matthewcarleton Fixed the badge in the side panel.

@jelkosz Added those statuses.

jeff-phillips-18 · 2020-04-16T15:44:57Z

@matthewcarleton The Pod on the Details tab is the same. For the Resources tab we use a shared component to list the pods for a resource, hence Pods even if it is a list of one. If you feel strongly, I can adjust it to be able to show just Pod for this particular case.

matthewcarleton · 2020-04-16T16:35:01Z

@jeff-phillips-18 it's very nitpicky I know so if will delay this getting in then don't worry about it. It really just comes down to VMs only ever using one pod so having Pod(s) could be a bit misleading.

serenamarie125 · 2020-04-17T01:11:04Z

@jeff-phillips-18 it's very nitpicky I know so if will delay this getting in then don't worry about it. It really just comes down to VMs only ever using one pod so having Pod(s) could be a bit misleading.

@matthewcarleton Jeff and I talked a bit about this. Here were the points we discussed:

We could show Pod if there is a single pod, and show Pods when there are more than 1. The problem with that is if people scale up and down, I think it would be really weird to see the title change as well. When we look at a List View page today, the page title is plural regardless of the # shown.
We could special case for VMs to always be Pod, but then it won't be consistent with other resources when there is only a single pod.

jeff-phillips-18 · 2020-04-17T11:47:10Z

/retest

matthewcarleton · 2020-04-17T12:08:58Z

@jeff-phillips-18 it's very nitpicky I know so if will delay this getting in then don't worry about it. It really just comes down to VMs only ever using one pod so having Pod(s) could be a bit misleading.

@matthewcarleton Jeff and I talked a bit about this. Here were the points we discussed:

We could show Pod if there is a single pod, and show Pods when there are more than 1. The problem with that is if people scale up and down, I think it would be really weird to see the title change as well. When we look at a List View page today, the page title is plural regardless of the # shown.

We could special case for VMs to always be Pod, but then it won't be consistent with other resources when there is only a single pod.

ah I see what you're saying. I'm ok with Pods then.

christianvogt · 2020-04-17T17:39:15Z

frontend/packages/console-shared/src/utils/transformResourceData.ts

+  getServicesForResource,
+} from './resource-utils';
+
+export class TransformResourceData {


We can probably move this to dev-console now?

christianvogt · 2020-04-17T18:48:00Z

frontend/packages/dev-console/src/components/dropdown/ApplicationDropdown.tsx

+    {
+      isList: true,
+      kind: VirtualMachineModel.kind,
+      namespace,
+      prop: 'virtualMachines',
+      optional: true,
+    },


Another area required for inclusion through extensions.
This is so inefficient to understand the app labels :(

I wonder if we could just sift through our redux store of resources instead of fetching all these upfront.
Something to consider as tech debt.

https://issues.redhat.com/browse/ODC-3613

christianvogt · 2020-04-17T18:52:20Z

frontend/packages/dev-console/src/components/topology/Topology.tsx

+  const knativeComponentFactoryRef = React.useRef<KnativeComponentFactory>(
+    new KnativeComponentFactory(serviceBinding),
+  );


The pattern you had before was better so that we don't always create new objects just to have them GC'd.

christianvogt · 2020-04-17T19:09:26Z

frontend/packages/dev-console/src/components/topology/Topology.tsx

+  React.useEffect(() => {
+    if (!displayFiltersRef.current) {
+      displayFiltersRef.current = filters.display;
+      return;
+    }
+
+    if (
+      (filters.display.eventSources && !displayFiltersRef.current.eventSources) ||
+      (filters.display.virtualMachines && !displayFiltersRef.current.virtualMachines)
+    ) {
+      action(() => {
+        visRef.current.getGraph().reset();
+        visRef.current.getGraph().layout();
+      })();
+    }
+    displayFiltersRef.current = filters.display;
+  }, [filters.display]);


This code is too specific when what we really want is to run layout on nodes that change visibility.
As discussed, an event to notify of visibility change to let the layout handle this much like add / remove of nodes.

Also discussed doing such changes in a future issue.

https://issues.redhat.com/browse/ODC-3616

christianvogt · 2020-04-17T19:16:41Z

frontend/packages/kubevirt-plugin/src/plugin.tsx

+    properties: {
+      resources: virtualMachineConfigurations,
+      required: FLAG_KUBEVIRT,
+      utils: getVirtualMachines,


This calls a function which returns an empty object? why?

christianvogt · 2020-04-17T19:17:58Z

frontend/packages/kubevirt-plugin/src/plugin.tsx

@@ -52,6 +57,43 @@ type ConsumedExtensions =

 export const FLAG_KUBEVIRT = 'KUBEVIRT';

+export const virtualMachineConfigurations = (namespace: string): FirehoseResource[] => {


Why export this function? It doesn't seem to be used anywhere externally.

christianvogt · 2020-04-17T19:22:19Z

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

@@ -49,7 +49,7 @@ export const isPodSchedulable = (pod: PodKind) => {
 const isPodReady = (pod: PodKind): boolean =>
  pod?.status?.phase === 'Running' && pod?.status?.containerStatuses?.every((s) => s.ready);

-export const findVMIPod = (
+export const getPodsForVMI = (


Do we really want a util to get all pods to display in the sidebar instead of just the one pod as displayed in their details page?

christianvogt · 2020-04-17T19:25:04Z

frontend/packages/kubevirt-plugin/src/topology/TopologyVmDetailsPanel.tsx

+    <div className="row">
+      <div className="col-sm-6">


Shouldn't we be using patternfly grid instead of bootstrap?

christianvogt · 2020-04-17T19:27:33Z

frontend/packages/kubevirt-plugin/src/topology/TopologyVmPanel.tsx

+      <div className="overview__sidebar-pane-head resource-overview__heading">
+        <h1 className="co-m-pane__heading">
+          <div className="co-m-pane__name co-resource-item">
+            <ResourceIcon className="co-m-resource-icon--lg" kind={vmObj.kind} />
+            {name && (
+              <Link
+                to={resourcePathFromModel(modelFor(vmObj.kind), name, namespace)}
+                className="co-resource-item__resource-name"
+              >
+                {name}
+              </Link>
+            )}
+          </div>
+          {actions?.length && (
+            <div className="co-actions">
+              <ActionsMenu actions={actions} />
+            </div>
+          )}
+        </h1>
+      </div>


These headings keep getting repeated. Create a tech debt to create a reusable component.
And maybe merge it with making the side panel more extensible.

https://issues.redhat.com/browse/ODC-3614

andrewballantyne · 2020-04-17T19:50:23Z

Needs rebase @jeff-phillips-18

christianvogt · 2020-04-18T03:36:47Z

/lgtm
/approve

openshift-ci-robot · 2020-04-18T03:37:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, jeff-phillips-18

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~~ [christianvogt]

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

openshift-bot · 2020-04-18T03:43:23Z

/retest

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

openshift-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. component/core Related to console core functionality labels Apr 14, 2020

openshift-ci-robot requested review from christianvogt and debsmita1 April 14, 2020 18:27

openshift-ci-robot added component/dev-console Related to dev-console component/knative Related to knative-plugin component/kubevirt Related to kubevirt-plugin component/shared Related to console-shared labels Apr 14, 2020

jeff-phillips-18 force-pushed the vm-topology branch 2 times, most recently from f358b18 to d500587 Compare April 14, 2020 20:06

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

jeff-phillips-18 force-pushed the vm-topology branch from d500587 to 07e93a1 Compare April 14, 2020 21:00

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

jeff-phillips-18 force-pushed the vm-topology branch from 07e93a1 to 2c9a1a8 Compare April 14, 2020 21:03

serenamarie125 reviewed Apr 14, 2020

View reviewed changes

jeff-phillips-18 force-pushed the vm-topology branch from 2c9a1a8 to f0c00d2 Compare April 15, 2020 10:48

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

jelkosz reviewed Apr 16, 2020

View reviewed changes

jeff-phillips-18 force-pushed the vm-topology branch from 8c526e1 to 040438e Compare April 16, 2020 15:34

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

jeff-phillips-18 force-pushed the vm-topology branch 2 times, most recently from be3e92e to 53e3685 Compare April 17, 2020 11:06

christianvogt reviewed Apr 17, 2020

View reviewed changes

Add kubevirt topology items and side panel

34cc4b9

jeff-phillips-18 force-pushed the vm-topology branch from 53e3685 to 1262747 Compare April 17, 2020 20:56

updates per review comments

2b011fc

jeff-phillips-18 force-pushed the vm-topology branch from 1262747 to 2b011fc Compare April 17, 2020 22:18

openshift-ci-robot assigned christianvogt Apr 18, 2020

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

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

openshift-merge-robot merged commit 412f8ac into openshift:master Apr 18, 2020

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

jeff-phillips-18 deleted the vm-topology branch December 2, 2020 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add VMs to Topology View #5041

Add VMs to Topology View #5041

jeff-phillips-18 commented Apr 14, 2020

serenamarie125 left a comment

serenamarie125 commented Apr 14, 2020

jeff-phillips-18 commented Apr 15, 2020

matthewcarleton commented Apr 15, 2020

jeff-phillips-18 commented Apr 15, 2020

matthewcarleton commented Apr 15, 2020 •

edited

jeff-phillips-18 commented Apr 16, 2020

matthewcarleton commented Apr 16, 2020

jelkosz Apr 16, 2020

jeff-phillips-18 commented Apr 16, 2020

jeff-phillips-18 commented Apr 16, 2020

matthewcarleton commented Apr 16, 2020

serenamarie125 commented Apr 17, 2020

jeff-phillips-18 commented Apr 17, 2020

matthewcarleton commented Apr 17, 2020

christianvogt Apr 17, 2020

christianvogt Apr 17, 2020

jeff-phillips-18 Apr 17, 2020

christianvogt Apr 17, 2020

christianvogt Apr 17, 2020

christianvogt Apr 17, 2020

jeff-phillips-18 Apr 17, 2020

christianvogt Apr 17, 2020

christianvogt Apr 17, 2020

christianvogt Apr 17, 2020

christianvogt Apr 17, 2020

christianvogt Apr 17, 2020

jeff-phillips-18 Apr 17, 2020

andrewballantyne commented Apr 17, 2020

christianvogt commented Apr 18, 2020

openshift-ci-robot commented Apr 18, 2020

openshift-bot commented Apr 18, 2020

		@@ -52,6 +57,43 @@ type ConsumedExtensions =

		export const FLAG_KUBEVIRT = 'KUBEVIRT';

		export const virtualMachineConfigurations = (namespace: string): FirehoseResource[] => {

Add VMs to Topology View #5041

Add VMs to Topology View #5041

Conversation

jeff-phillips-18 commented Apr 14, 2020

serenamarie125 left a comment

Choose a reason for hiding this comment

serenamarie125 commented Apr 14, 2020

jeff-phillips-18 commented Apr 15, 2020

matthewcarleton commented Apr 15, 2020

jeff-phillips-18 commented Apr 15, 2020

matthewcarleton commented Apr 15, 2020 • edited

jeff-phillips-18 commented Apr 16, 2020

matthewcarleton commented Apr 16, 2020

Choose a reason for hiding this comment

jeff-phillips-18 commented Apr 16, 2020

jeff-phillips-18 commented Apr 16, 2020

matthewcarleton commented Apr 16, 2020

serenamarie125 commented Apr 17, 2020

jeff-phillips-18 commented Apr 17, 2020

matthewcarleton commented Apr 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

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

andrewballantyne commented Apr 17, 2020

christianvogt commented Apr 18, 2020

openshift-ci-robot commented Apr 18, 2020

openshift-bot commented Apr 18, 2020

matthewcarleton commented Apr 15, 2020 •

edited