Introduce kubevirt #1592

mareklibra · 2019-05-21T12:13:35Z

Initial effort of merging https://github.com/kubevirt/web-ui fork features back to upstream.

New kubevirt-plugin is created with bare minimum functionality in terms of listing Virtual Machines.
Additional features will be moved within follow-ups.

Depends on:

TODO:

introduce support for ordering items within left-side menu (for Resource Nav Item) - wip NavSection: allow plugins to define their position #1609
introduce new extension point for yaml templates (initially for Create VM from Yaml) - wip Add YAML template extension #1613
OWNERS file

Some of immediate follow-ups:

VM-status with fetching of related objects
VM Detail page
VM templates (list, detail)
Create VM Wizard

openshift-ci-robot · 2019-05-21T12:13:49Z

Hi @mareklibra. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

vojtechszocs · 2019-05-21T12:23:54Z

@mareklibra Note that #1584 is already merged.

frontend/package.json

mareklibra · 2019-05-23T13:15:46Z

frontend/packages/console-app/package.json

@@ -7,6 +7,7 @@
  "dependencies": {
    "@console/internal": "0.0.0-fixed",
    "@console/plugin-sdk": "0.0.0-fixed",
-    "@console/shared": "0.0.0-fixed"
+    "@console/shared": "0.0.0-fixed",
+    "@console/kubevirt-plugin": "0.0.0-fixed"


part of DO NOT MERGE commit

vojtechszocs · 2019-05-23T15:07:03Z

Looks good overall, I'm working #1586 to close the immediate gaps regarding model definitions and YAML template extension.

vojtechszocs · 2019-05-24T18:40:04Z

introduce new extension point for yaml templates (initially for Create VM from Yaml)

This is addressed by #1613

mareklibra · 2019-05-30T07:16:41Z

Rebased, just #1613 is missing to be merged

vojtechszocs · 2019-05-30T17:21:17Z

Rebased, just #1613 is missing to be merged

Thanks, reviewing this now 👀

vojtechszocs · 2019-05-30T17:27:05Z

@mareklibra There were some changes in #1613 (like YamlTemplate => YAMLTemplate) so please rebase.

vojtechszocs · 2019-05-30T17:33:34Z

frontend/packages/kubevirt-plugin/src/components/__tests__/vm.spec.tsx

@@ -0,0 +1 @@
+// TODO(mlibra)


I'd suggest to add this file later in a follow-up PR.

@christianvogt @spadgett We'd like to start adding React component snapshot tests and utilize Enzyme which is already set up in before-tests.js.

The current proposal is to put snapshot files next to test files, for example:

path/to/widget.tsx path/to/__tests__/widget.spec.tsx path/to/__tests__/widget.spec.snap

To avoid the default path/to/__tests__/__snapshots__/widget.spec.snap behavior, which adds unnecessary noise to existing file structure, we can configure Jest with custom snapshot resolver.

@mareklibra I'd prefer to first set up ^^ snapshot resolver before adding our snapshot tests.

I'm indifferent to the location of snapshots.

Removed. I like the proposed collocation.

#1648 should address test vs. snapshot co-location.

vojtechszocs · 2019-05-30T17:48:54Z

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

+    </div>
+    <div className="dropdown-kebab-pf">
+      {/*
+      <ResourceKebab actions={menuActions} kind={VirtualMachineModel.kind} resource={vm} resources={[


@christianvogt @jelkosz This relates to our discussion about having a single map of models to corresponding actions, to be used consistently throughout the codebase.

For now, resource actions are handled on a local level, i.e. VM list page creating/using its "own" VM actions.

cc @spadgett

I agree that this is something we should look at doing.

+1, this is something we had been planning to do.

vojtechszocs · 2019-05-30T17:52:00Z

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

+VMList.displayName = 'VMList';
+
+const filters = undefined;
+/* TODO(mlibra): introduce extension point for list.tsx and reenable here then


@jelkosz This is a gap we need to close, i.e. make listFilters in public/components/factory/list.tsx extensible.

cc @spadgett

I think new extension point makes sense here.

Will be implemented within a follow-up, but menioning here to keep in mind:
To properly determine VM status, there are additional objects needed (pods and CRs). Recent filtering logic around list.tsx can handle just the single object "listed" object. We will need to find a way how to supply additional resources there.

So it will be handled via follow-ups for:

extension point

VM status filtering

optionally: enhanced logic around list.tsx enabling examination of additional (related) objects

vojtechszocs · 2019-05-30T17:56:18Z

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

+  namespace: string;
+};
+
+export class VirtualMachinesPage extends React.Component<VirtualMachinesPageProps> {


@mareklibra We should prefer using function-based components, i.e. React.FC that can use hooks to support additional concerns like component state. @rawagner already did this in #1591.

We can improve this later on, just keep this in mind.

vojtechszocs · 2019-05-30T17:58:16Z

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

@@ -0,0 +1,60 @@
+import * as _ from 'lodash-es';
+import { Plugin, ResourceNSNavItem, ResourceListPage, ResourceDetailPage, ModelFeatureFlag, YamlTemplate, ModelDefinition } from '@console/plugin-sdk';


Nit-pick: please split into multiple lines for better readability.

vojtechszocs · 2019-05-30T17:59:57Z

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

+import * as models from './models';
+import { vmYamlTemplate } from './yaml-templates';
+
+type ConsumedExtensions = ResourceNSNavItem | ResourceListPage | ResourceDetailPage | ModelFeatureFlag | YamlTemplate | ModelDefinition;


Nit-pick: please split into multiple lines for better readability.

vojtechszocs · 2019-05-30T18:01:00Z

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

+        resource: models.VirtualMachineModel.plural,
+        required: FLAG_KUBEVIRT,
+      },
+      mergeAfter: 'Pods', // rendered name of the resource navigation link in the left-side menu


Nav extension properties will be documented via #1638 so we can remove this comment, I think 😃

vojtechszocs · 2019-05-30T18:04:03Z

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

+    type: 'ResourcePage/List',
+    properties: {
+      model: models.VirtualMachineModel,
+      loader: () => import('./components/vm' /* webpackChunkName: "virtual-machines" */).then(m => m.VirtualMachinesPage),


For consistency with Metal3 plugin, I'd suggest using a kubevirt- prefix in the chunk name, e.g. kubevirt-virtual-machines or kubevirt-virtualmachines.

It will give us better traceability when analyzing webpack-generated chunks that might come from different Console plugins.

vojtechszocs · 2019-05-30T18:09:56Z

frontend/packages/kubevirt-plugin/src/yaml-templates.ts

@@ -0,0 +1,37 @@
+import { VirtualMachineModel } from './models';
+
+export const vmYamlTemplate = `


Check out #1613 - a more robust (but also more complex) alternative is to use a map with tuple key [model, templateName]:

import { Map as ImmutableMap } from 'immutable'; import { FooBarModel } from './models'; export const yamlTemplates = ImmutableMap() .setIn([FooBarModel, 'default'], ` apiVersion: ${FooBarModel.apiVersion} kind: ${FooBarModel.kind} metadata: name: example namespace: default `);

I'm OK with a simple const vmYamlTemplate too.

In any case, please remove the leading whitespace (they aren't stripped automatically):

export const vmYamlTemplate = ` apiVersion: ${VirtualMachineModel.apiGroup}/${VirtualMachineModel.apiVersion} kind: VirtualMachine metadata: name: example .. `;

vojtechszocs · 2019-05-30T18:12:56Z

@spadgett @christianvogt Please check my inline comment regarding snapshot testing.

mareklibra · 2019-06-06T07:12:51Z

/retest

So far without any functionlity.

An extension point needs to be implemented first.

And small code-style improvements.

As recently updated within @console/plugin-sdk: ResourcePage/List => Page/Resource/List ResourcePage/Detail => Page/Resource/Details

mareklibra · 2019-06-06T10:05:57Z

/test e2e-aws

vojtechszocs · 2019-06-06T16:07:15Z

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

+};
+
+/* TODO(mlibra): migrate templates
+export const VmTemplateModel: K8sKind = {


This relates to #1658 - we really shouldn't redefine existing base k8s models.

I'm working on a PR that will integration-test actual Console plugins and detect any conflicts among their extensions.

(It's perfectly fine to do this in our Console fork, which is meant to support KubeVirt specifically.)

vojtechszocs · 2019-06-06T16:15:43Z

Thanks @mareklibra for updating the PR.

As far as I can see, the only remaining issue is deciding whether KubeVirt related models will be declared as CRDs or not.

Other than that, LGTM.

jelkosz · 2019-06-06T19:37:55Z

We had this discussion back in the days when implementing it in the fork and the decision was to go as it is in this PR. If the PR is otherwise ready, I would really love to see it merged. If it will prove to be a problem, we can change it in a followup, but lets not block on it.

@spadgett WDYT?

vojtechszocs · 2019-06-07T13:01:19Z

If it will prove to be a problem, we can change it in a followup, but lets not block on it.

Agreed. I'm fine with merging this and improve/change as needed.

spadgett · 2019-06-07T13:20:11Z

frontend/yarn.lock

+    lodash "4.x"
+    react-dnd "2.6.x"
+    react-dnd-html5-backend "5.0.x"
+    react-measure "2.x"


We should look at removing this dependency from kubevirt-web-ui-components if we can. See discussion in #1591

spadgett · 2019-06-07T13:20:39Z

frontend/yarn.lock

@@ -8023,6 +8087,21 @@ kind-of@^6.0.0, kind-of@^6.0.2:
  version "6.0.2"
  resolved "https://registry.yarnpkg.com/kind-of/-/kind-of-6.0.2.tgz#01146b36a6218e64e58f3a8d66de5d7fc6f6d051"

+kubevirt-web-ui-components@^0.1.34-4:


Can we consider removing this in this PR? We're only using getName and getNamespace currently. Those are trivial to implement.

Having this dependency defeats the purpose of the monorepo. Is it feasible to simply bring the components in to console directly as we add the capabilities? (I don't know the code well enough to say.)

Sam, this is just initial PR with bare minimum functionality to enable transfer of the rest in follow-ups. I guess 80% of the planed functionality is recently in kubevirt-web-ui.

As mentioned before: we plan to deprecate kubevirt-web-ui asap and so remove it even from dependencies here.

But doing it now would cause us a lot of troubles in case we do not achieve full feature parity in 4.2 timeframe (considering recent progress, this is valid concern).

So if possible, it will be huge relieve for us to keep it and go safely feature-by-feature.

The whole kubevirt-web-ui-components will be moved into this monorepo, but only as a last step. I would like to avoid duplicating the code as part of this effort and having the same stuff on two places. We still need to maintain the fork and its releases so it is important to have it on one place only and if we make fix there, not to have to create a new fix also here.

Until the transition from our fork to Console plugins is complete (in terms of feature parity), it's more practical to use kubevirt-web-ui-components as an external library. As @mareklibra mentioned, there's always the chance of not achieving full parity in 4.2 timeframe.

Once our fork is discontinued, we can

either move kubevirt-web-ui-components as-is under e.g. packages/kubevirt-components (which isn't ideal, since it's plain JS and its code & deps might need revisiting, like react-measure mentioned above)

or we can create an empty e.g. packages/kubevirt-components and gradually bring in the necessary bits (revisiting & migrating them to TS in the process) and eventually remove the kubevirt-web-ui-components dependency

Personally, I favor the second approach.

spadgett · 2019-06-07T13:30:56Z

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

@@ -0,0 +1,136 @@
+import { K8sKind } from '@console/internal/module/k8s';
+
+export const VirtualMachineModel: K8sKind = {


I would suggest crd: true for now, although I think we should get rid of the crd property in the model object if we can. It's a bit misleading since not all CRDs have it, and some resources have it that aren't CRDs. We used to key on it for other behavior changes around watches, but I think we've removed that.

Honestly, I don't think most users will notice or care if the URL doesn't use the short name. The biggest problem is that we include the API version in the URL, not just the group. This could break links if versions change (for instance, v1alpha1 -> v1). But that's outside the scope of this PR. We need to look at this problem generally, not just for KubeVirt.

spadgett

/lgtm
/approve

openshift-ci-robot · 2019-06-07T19:29:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mareklibra, spadgett

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

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 added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 21, 2019

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 21, 2019

openshift-ci-robot requested review from dtaylor113 and rhamilto May 21, 2019 12:13

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 22, 2019

mareklibra changed the title ~~[WIP] Introduce kubevirt2~~ [WIP] Introduce kubevirt May 22, 2019

spadgett added this to the v4.2 milestone May 22, 2019

mareklibra commented May 23, 2019

View reviewed changes

frontend/package.json Outdated Show resolved Hide resolved

mareklibra commented May 23, 2019

View reviewed changes

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 23, 2019

yaacov mentioned this pull request May 30, 2019

[WIP] Kubvirt plugin vmtemplate #1637

Closed

3 tasks

vojtechszocs suggested changes May 30, 2019

View reviewed changes

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

mareklibra added 14 commits June 6, 2019 09:29

Introduce @console/kubevirt-plugin

e82c073

So far without any functionlity.

Initial VirtualMachinesPage

0d8d34e

Use constants

a714e07

Disable vm-status filter

5064d66

An extension point needs to be implemented first.

Add template for Create VM from YAML

c8e45cd

Put "Virtual Machines" after "Pods" in left-side menu

f945aec

Inital OWNERS file for kubevirt-plugin

059b7b0

Register VirtualMachinesModel

4a175aa

Transform VirtualMachinesPage to React functional component

94eab7d

Rebase on changed YAMLTemplate

4cc403b

Change webpackChunkName to kubevirt-virtual-machines

055473d

Types added and models pruned

85e7591

And small code-style improvements.

Change resource extension types

8cb16e5

As recently updated within @console/plugin-sdk: ResourcePage/List => Page/Resource/List ResourcePage/Detail => Page/Resource/Details

kubevirt-plugin: yarn.lock

f950a20

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 6, 2019

vojtechszocs reviewed Jun 6, 2019

View reviewed changes

mareklibra mentioned this pull request Jun 7, 2019

Kubevirt VmDetails #1682

Merged

4 tasks

spadgett reviewed Jun 7, 2019

View reviewed changes

spadgett approved these changes Jun 7, 2019

View reviewed changes

openshift-ci-robot assigned spadgett Jun 7, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 7, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 7, 2019

openshift-merge-robot merged commit e966550 into openshift:master Jun 7, 2019

		@@ -0,0 +1,60 @@
		import * as _ from 'lodash-es';
		import { Plugin, ResourceNSNavItem, ResourceListPage, ResourceDetailPage, ModelFeatureFlag, YamlTemplate, ModelDefinition } from '@console/plugin-sdk';

		@@ -0,0 +1,37 @@
		import { VirtualMachineModel } from './models';

		export const vmYamlTemplate = `

		@@ -0,0 +1,136 @@
		import { K8sKind } from '@console/internal/module/k8s';

		export const VirtualMachineModel: K8sKind = {

Introduce kubevirt #1592

Introduce kubevirt #1592

Conversation

mareklibra commented May 21, 2019 • edited Loading

openshift-ci-robot commented May 21, 2019

vojtechszocs commented May 21, 2019

Choose a reason for hiding this comment

vojtechszocs commented May 23, 2019

vojtechszocs commented May 24, 2019

mareklibra commented May 30, 2019

vojtechszocs commented May 30, 2019

vojtechszocs commented May 30, 2019

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

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

vojtechszocs commented May 30, 2019

mareklibra commented Jun 6, 2019

mareklibra commented Jun 6, 2019

Choose a reason for hiding this comment

vojtechszocs commented Jun 6, 2019

jelkosz commented Jun 6, 2019

vojtechszocs commented Jun 7, 2019

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 left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jun 7, 2019

mareklibra commented May 21, 2019 •

edited

Loading