kubernetes: Virtual machines side tab added #7830

jniederm · 2017-10-06T14:58:58Z

It lists VMs of Kubevirt. The tab is only shown if connected cluster
has Kubevirt installed.

Navigation vertical bar extended to be able to show "Virtual Machines"
label.

petervo

Nice start, have a couple questions and comments.

petervo · 2017-10-06T15:12:25Z

pkg/kubernetes/scripts/main.js

@@ -44,6 +44,7 @@
    require('./nodes');
    require('./topology');
    require('./volumes');
+    require('./virtual-machines.es6');


Can we stick to standard js in the module? Or is there a reason we need to use es6?

petervo · 2017-10-06T15:12:55Z

pkg/kubernetes/scripts/virtual-machines.es6

+        '$routeProvider',
+        function($routeProvider) {
+            $routeProvider
+                // .when('/vms/:vm_namespace/:vm_name', {


Let's not commit commented out code.

petervo · 2017-10-06T15:13:34Z

pkg/kubernetes/scripts/virtual-machines.es6

+        'kubeLoader',
+        'kubeSelect',
+        function($scope, loader, select) {
+            console.log('controller arguments', arguments);


debug log statements here.

petervo · 2017-10-06T15:16:37Z

pkg/kubernetes/scripts/kube-client.js

@@ -38,6 +38,7 @@

    var KUBE = "/api/v1";
    var OPENSHIFT = "/oapi/v1";
+    var KUBEVIRT = "/apis/kubevirt.io/v1alpha1";


Is there a way we can discover this instead having to hard code it?

I guess prefix/apis/kubevirt.io is ok. v1alpha1 could be in theory extracted from response to GET /apis/kubevirt.io as value of preferredVersion field.

But that's probably not what we want. All the kubevirt related code relies on v1alpha1 and is not guarantied to work with other versions (since there is currently no other version). I case of api update (and related api prefix update) we would need most probably update the code as well.

I've seen a lot of use cases where users use a new version of this (usually the container) with an older kubernetes version. So if we hard code this then in the future when we bump to v1 this code will no longer be usable with older versions.

But it's also possible that the API will change so much that we'll never be able to support any other version in the same js code base. So I guess it makes sense to leave it like this for now and we can make the decision about continuing to support v1alpha1 once we know what the differences are.

jniederm · 2017-10-06T15:29:37Z

Widening of vertical navigation bar got broken, so this is still WIP.

jniederm · 2017-10-06T20:12:32Z

screenshots:

jniederm · 2017-10-06T20:30:25Z

ready for next review

petervo · 2017-10-06T20:42:40Z

pkg/kubernetes/views/virtual-machines-listing.html

+    </thead>
+
+    <tbody ng-repeat="vm in vms track by vm.metadata.uid">
+        <tr class="listing-ct-item">


Can we provide an initial expanded version. Even if it's just the same info for now, just for completeness?

petervo · 2017-10-06T20:48:16Z

This is looking good. Before we can merge we need to figure out testing. So we can test that the nav item comes and goes correctly. I can help with that.

How are you going about installing now? I'm guessing this is not packaged for any distro yet. Are there containers we can use? Or how would you recommend we set kubevirt up on our test machines?

mareklibra · 2017-10-11T09:38:12Z

AFAIK, there's recently no packaging for any distro. There's Vagrant image wrapping all together but this might be complicated to use in the test environment. Deployment of development environment on openshift is in progress, we might be able to reuse that once/if finished [1].

Or we can explore how to install kubevirt [2] on top of existing cocpit's k8s test image.

Local configuration [3] will be probably needed to set master node name, kubevirt expects master by default. See configuration under [4]. Or:

kubectl label --overwrite node rhel-kube-1 kubernetes.io/hostname=master

To build and deploy docker images:

kubectl taint node rhel-kube-1 node-role.kubernetes.io/master:NoSchedule-

cd ~/go/src/kubevirt.io/kubevirt
make && make docker
for m in manifests; do kubectl apply -f $m; done

[1] https://github.com/petrkotas/openshift-env/
[2] kubevirt: https://github.com/kubevirt/kubevirt
[3] kubevirt/docs/dev-env-local.md
[4] kubevirt/hack/*

petervo · 2017-10-17T15:00:30Z

So I looked into adding kubevirt to one of our test images. While i did manage to get it working its' not super reproducable right now. My concern is that it will break every time we rebuild the image. I talked to @fabiand on IRC and he was going to see if he can refactor the scripts a little so that they can be use with both minikube and standard kube so we don't have to invent and support our manifests.

kubevirt/demo#24

mareklibra · 2017-10-25T14:16:44Z

The user shall be able to see additional details of the virtual machine and modify it.

It would be great to reuse existing pkg/machines for that to benefit from its farther development and common look&feel.

As the datasource, k8s/kubevirt API should be used instead of Libvirt.

The view is cluster-level and not host- (node-) level (compared to pkg/machines or pkg/ovirt).

For discussion/decision is the way how to achieve that.

In all options bellow, new pkg/kubevirt will be created. But its scope and relation to pkg/kubernetess varies.

To use #7830 list of virtual machines list as it is (so written in angular):

click-through to the Virtual Machines page
- how:
  - pkg/kubevirt extends pkg/machines more-or-less same way as pkg/ovirt does
  - pkg/kubevirt renders list of VMs
  - pkg/kubevirt is registers to /machines context-root
  - user is navigated from pkg/kubernetess to pkg/kubevirt and vice versa
- pros:
  - kubevirt stuff is kept out of pkg/kubernetess
  - already known way how to reuse pkg/machines (same as for oVirt)
- cons:
  - switching between Host and Cluster tabs might be confusing for the user since it makes no sense (we have cluster-level view only)
integrate single-VM pkg/kubevirt into pkg/kubernetess view
- how:
  - the pkg/kubevirt will not list virtual machines but allows view/edit functionality just for a single VM
  - maybe as multiple iframes or React rendering into multiple placeholders
- pros:
  - user is not confused by "jumping" throughout Cockpit
  - kubevirt stuff is kept out of pkg/kubernetess
- cons:
  - from design perspective, the integration must be smooth - unrecognizable for the user - to be resolved
  - pkg/kubevirt should not be loaded more than once even if multiple virtual machines are "expanded" - to be resolved
  - list of virtual machines would be implemented twice - within kubernetes: Virtual machines side tab added #7830 and pkg/machines

Or still keep the list of virtual machines under Cluster dashboard but re-implement the page using React

how:
- more-or-less similar to the first option but rendered either into single <div> placeholder or new iframe (both maintained by pkg/kubernetess angular code)
- in fact the Virtual Machines list in pkg/kubernetess page will be single React tree comming from pkg/kubevirt
- React component wrapped by Angular code - the code itself "lives" in two different pkg/ subdirectories
- in this option, reason for creating pkg/kubevirt is only to keep React/Angular sources unmixed
pros:
- seamless reuse of pkg/machines
- seamlessly share connection to kubernetess and most probably events (if needed)
- easiest way to control React/Redux and other from pkg/machines logic
- smooth view for the user
- everything VM-related comes from pkg/machines or its derivates
cons:
- at runtime, higher-level angular code wraps React

In all cases, code shared from pkg/machines by pkg/ovirt or pkg/kubevirt at build-time (webpack) shall be refactored to better isolate it's boundaries (means react, redux and whathever else) - probably nothing complicated.

My prefference is on the last option with integrating react tree into existing angular code.
Code under pkg/kubernetess will stay React-free, will just maintain the placeholder or single iframe if combining these two is not possible.

Any other idea how to provide pkg/machines functionality for kubevirt virtual machines?

petervo · 2017-10-25T14:35:20Z

We don't need to override the machines package. It's easy to have this link refer to a different page.

The angular / react thing is a little more complicated. All the code that interacts with the kubernetes API is in angular. So I'm not sure we can get completely away from that.

I also imagine there might also be quite a few differences in how you would configure machines using kubevirt. You'll probably leave networking to kubernetes, use PVs for storage etc... So i think we will only want to reuse certain parts of the machines code and not the entire package the way ovirt does.

fabiand · 2017-11-09T08:39:24Z

Just head's up that this is not forgotten.
In kubevirt/kubevirt#560 we are getting closer to having simple manifests which will allow you to deploy KubevIrt - if all goes well.
I'll provide more updates now and then.

jniederm · 2017-11-10T02:55:36Z

The angular / react thing is a little more complicated. All the code that interacts with the kubernetes API is in angular. So I'm not sure we can get completely away from that.

Yes, it's not exactly straightforward to use react components in angular environment. Let's give it a try. I'll create a POC utilizing angular kubernetes bindings.

I also imagine there might also be quite a few differences in how you would configure machines using kubevirt. You'll probably leave networking to kubernetes, use PVs for storage etc... So i think we will only want to reuse certain parts of the machines code and not the entire package the way ovirt does.

Exactly. The Kubevirt extension is supposed to be similar to machines plugin from the user perspective so we'd like to reuse mainly UI components to avoid code duplication and easily achieve similar look and feel. And at the same time the Kubevirt api is pretty different from libvirt/oVirt apis so this area will require new code.

jniederm · 2017-12-06T20:43:43Z

React version pushed, tests not (yet) included

martinpitt · 2017-12-06T23:02:22Z

This fails on semaphore: FAIL: tools/test-static-code 3 js-translatable-strings; please run tools/test-static-code locally and push a fix, then I'll trigger the verify tests. Thanks!

petervo · 2017-12-06T23:09:41Z

@martinpitt no point in triggering yet, tests still need to be written.

michalskrivanek · 2017-12-07T15:14:10Z

as for kubevirt deployment, with kubevirt/kubevirt#602 we're getting even more close to a reasonable state.

fabiand · 2017-12-20T16:52:57Z

I've just updated the kubevirt demo, which now uses the improved manifests.

Cockpit can use the demo approach to deploy kubevirt in minikube: See https://github.com/kubevirt/demo/blob/64357903f6a83fb687d05c6f47b687e98b196f3d/README.md for details.

petervo · 2017-12-20T17:06:28Z

@fabiand awesome. To uninstall is deleting everything in kubevirt-infra good enough? Or are there other steps we would need to take?

fabiand · 2017-12-20T19:39:09Z

@petervo In an ideal world, yes. I'd recommed to run a kubectl delete -f kubevirt.yaml in order to cleanup. but this is not really reliable, there are a few bugs about deleting deployments etc.

However, try this, and if it doesn't work the way we expect then we can fix further bugs.

michalskrivanek · 2018-01-09T15:06:03Z

@jniederm anything missing?

jniederm · 2018-01-09T20:17:25Z

@michalskrivanek We need to make tests passing and then reviews.

mareklibra · 2018-01-12T09:34:22Z

There's new Kubevirt 0.2.0 in the meantime: #8414

mareklibra · 2018-01-12T09:37:15Z

Removing blocked since #8247 and #8236 (for Kubevirt 0.1.0) are already in.
There's no need to block this on 0.2.0. I think its fine to adjust accordingly later.

mareklibra · 2018-01-16T13:21:57Z

LGTM, @petervo , @martinpitt , can you please review?

martinpitt

Aside from a few nitpicks and questions this looks good to me; it's rather clear code - although a little over-abstracted for my taste, it could be much denser; but I know you guys like redux and you will maintain it, so no complaints.

ATM this is informational, and I suppose this will grow some more features in the future for actually interacting with the VMs? Similar to the containers page, where you can delete them, get a console, etc.?

Can you please also post some up to date screenshots with a "General" details pane opened, for the release notes? If you rather want to have a quick video demo, that's much appreciated too, of course!

Thanks!

martinpitt · 2018-01-17T20:21:25Z

pkg/kubernetes/scripts/virtual-machines.js

+/*
+ * This file is part of Cockpit.
+ *
+ * Copyright (C) 2015 Red Hat, Inc.


This should be bumped :-)

martinpitt · 2018-01-17T20:29:04Z

pkg/kubernetes/scripts/virtual-machines/components/VmsListing.jsx

+    return (
+        <Listing title={_("Virtual Machines")}
+                 emptyCaption={_("No virtual machines")}
+                 columnTitles={[_("Name"), namespaceLabel, _("Node"), _("Phase")]}>


"phase" might be the kubevirt-internal term for this, but the docker and libvirt pages (Containers and Machines) call them "State". Can we use that same label for consistency?

martinpitt · 2018-01-17T20:37:00Z

pkg/kubernetes/scripts/virtual-machines/index.jsx

+ */
+function init($scope, kubeLoader, kubeSelect) {
+    initReduxStore()
+    sagaMiddleware.run(rootSaga)


Are you actually using sagas anywhere? At least to my ignorant eye it just looks like you have the skeleton here, but it doesn't do anything (yet?). I may sound like an old man, but I find it a bit hard to follow all that boilerplate in reducers, action-types, and action-creators (particularly as this page does not actually have any user-visible actions, it's just displaying state). It's mostly "your" code, so I don't object, but I'm at least trying to understand what's happening :-)

Your sharp eye correctly recognized that there are no sagas yet. If it's a problem it can be removed from this pull. Commit 'kubernetes: Virtual Machines view - infra' is meant to set up complete react-redux-saga environment. Sagas will be used in future patches performing "write" operations.

I agree it's not exactly concise. However AFAIK it's the best of structuring state management for React.

Action is unfortunately overloaded expression here. In context of redux it's used to describe an event changing the state.

It's okay if you are going to use them soon, I just wanted to confirm that it's not just some leftover boilerplate. Thanks! (Going to review the rest of the updated PR on Monday, thanks for the cleanups!)

martinpitt · 2018-01-17T20:42:18Z

pkg/kubernetes/scripts/virtual-machines/utils.jsx

+        result.push(propertyName)
+    }
+    return result
+}


How is that different from Object.keys(object)?

Removed. It was similar to Object.keys(object) but also went down the prototype chain. Verified by experiment that kubeSelect() returns results only in own properties. If it's not always true please let me know.

martinpitt · 2018-01-17T20:42:49Z

pkg/kubernetes/scripts/virtual-machines/utils.jsx

+export function getValues(object) {
+    return getKeys(object).map(key => object[key])
+}
+


Likewise, what's different from Object.values(object)?

Likewise, removed.

martinpitt · 2018-01-17T20:47:29Z

test/verify/check-openshift

+        b.wait_present("#vms-menu-link")
+        b.click("#vms-menu-link")
+        b.wait_present("tr[data-row-id='vm-testvm']")
+        self.assertTrue(b.is_present("tr[data-row-id='vm-testvm']"))


Not important, but this is redundant: The wait_present before already asserts the presence.

Could this also check some actual content of the VM, like that it has a reasonable name and node? Clicking on it should reveal the details pane, which should have some contents asserted.

Are kubevirt nodes the same as "normal" container nodes in openshift? There is no particular implementation of a "nodes" view here, so I assume the nodes link will just lead to the existing pages?

Kubevirt is just an extension to k8s, nodes are same with or without kubevirt. The idea is to implement click-through in a follow-up patch.

martinpitt · 2018-01-17T20:54:55Z

Oh, and I suppose the commits should be squashed together when we merge this, as they are rather incremental. This makes incremental reviews easier, so it's a good thing; just want to confirm that you are okay with this.

Infrastructure change supporting adding Virtual Machines (KubeVirt) view to kubernetes package. Changed dependencies: * react-redux, redux-saga - React state management dependencies * babel-plugin-transform-regenerator - support for JS generator functions * eslint-plugin-flowtype, flow-bin, flow-webpack-plugin - JS type annotation * htmlparser - alphabetical order fixed

It lists VMs of Kubevirt. The tab is only shown if connected cluster has Kubevirt installed. Navigation vertical bar extended to be able to show "Virtual Machines" label. It uses standard React + Redux + Saga approach. Nested in angular environment. Redux store is garbage collected when Virtual Machines view is left.

React event handlers were receiving click events on local links (<a href="#...">) with `defaultPrevented` flag set. Navigation was performed despite of that flag. Fixed by disabling Angular links rewriting for Virtual Machines angular module.

... since it is breaking GitHub CI, error message: Unhandled exception: Unix.Unix_error(Unix.EACCES, "ftruncate", "")

jniederm · 2018-01-20T17:28:22Z

Oh, and I suppose the commits should be squashed together when we merge this, as they are rather incremental. This makes incremental reviews easier, so it's a good thing; just want to confirm that you are okay with this.

I wasn't meant that way originally. I find detailed git history useful. But yes, I confirm I'm ok with that.

jniederm · 2018-01-20T17:48:59Z

Can you please also post some up to date screenshots with a "General" details pane opened, for the release notes?

screenshot of latest version

ATM this is informational, and I suppose this will grow some more features in the future for actually interacting with the VMs? Similar to the containers page, where you can delete them, get a console, etc.?

Yes, this patch just lists VMs. We'd like Cockpit to be the main low level GUI for Kubevirt so there is plan to provide full set of CRUD operations, support for Offline VMs, console and other. There's already pull #8434 adding new VM dialog (WIP).

martinpitt

Thanks for the fixups!

martinpitt · 2018-01-22T06:55:18Z

test/verify/check-openshift

+        node_name = b.text("tr[data-row-id='vm-testvm'] + tr.listing-ct-panel .vm-node").strip()
+        if node_name != node_not_assigned:
+            b.click("tr[data-row-id='vm-testvm'] + tr.listing-ct-panel .vm-node a")
+            b.wait_js_cond('window.location.hash === "#/nodes/%s"' % (node_name,))


The setup cost of these tests is very high, so I'd like to merge these into one test. But this can happen in a follow-up.

Done in #8465.

petervo reviewed Oct 6, 2017

View reviewed changes

jniederm force-pushed the kubevirt-first branch 2 times, most recently from 1e4595c to 33e1d6b Compare October 6, 2017 15:24

jniederm force-pushed the kubevirt-first branch from 33e1d6b to 92f67b5 Compare October 6, 2017 20:08

jniederm force-pushed the kubevirt-first branch from 92f67b5 to c9a11eb Compare October 6, 2017 20:25

petervo reviewed Oct 6, 2017

View reviewed changes

petervo added the blocked Don't land until something else happens first (see task list) label Oct 17, 2017

stefwalter mentioned this pull request Oct 18, 2017

kubernetes: Fix regression in sidebar layout #7900

Closed

jniederm force-pushed the kubevirt-first branch from c9a11eb to 7bc923e Compare December 6, 2017 20:36

martinpitt added the needswork label Dec 6, 2017

jniederm force-pushed the kubevirt-first branch from 7bc923e to 43e1159 Compare December 7, 2017 14:02

jniederm force-pushed the kubevirt-first branch from 43e1159 to 787bd44 Compare December 11, 2017 13:20

mareklibra mentioned this pull request Dec 11, 2017

image: Add Kubevirt to Openshift image #8236

Merged

1 task

jniederm force-pushed the kubevirt-first branch from f188fdb to 042ea4c Compare January 9, 2018 12:38

jniederm force-pushed the kubevirt-first branch from 042ea4c to d3b5f8b Compare January 9, 2018 20:15

jniederm force-pushed the kubevirt-first branch from d3b5f8b to 22744a5 Compare January 10, 2018 11:46

mareklibra removed the blocked Don't land until something else happens first (see task list) label Jan 12, 2018

mareklibra approved these changes Jan 12, 2018

View reviewed changes

martinpitt removed the needswork label Jan 15, 2018

atiratree mentioned this pull request Jan 16, 2018

kubernetes: add create vm dialog to VirtualMachines #8434

Closed

8 tasks

martinpitt requested changes Jan 17, 2018

View reviewed changes

jniederm added 6 commits January 19, 2018 14:53

VS Code settings git ignored

bb6902b

kubernetes: VMs: Node link added

4024b1a

kubernetes: VMs: labels listed

ddf341f

kubernetes: VMs: <a> click events fixed

08ba4db

React event handlers were receiving click events on local links (<a href="#...">) with `defaultPrevented` flag set. Navigation was performed despite of that flag. Fixed by disabling Angular links rewriting for Virtual Machines angular module.

jniederm force-pushed the kubevirt-first branch from 22744a5 to 4eae2fa Compare January 20, 2018 15:35

jniederm added 2 commits January 20, 2018 18:26

kubernetes: Kubevirt - tests

26cf68c

kubernetes: Flow validation removed

2526ff4

... since it is breaking GitHub CI, error message: Unhandled exception: Unix.Unix_error(Unix.EACCES, "ftruncate", "")

jniederm force-pushed the kubevirt-first branch from 4eae2fa to 2526ff4 Compare January 20, 2018 17:27

martinpitt approved these changes Jan 22, 2018

View reviewed changes

martinpitt merged commit 2a7622d into cockpit-project:master Jan 22, 2018

This was referenced Jan 22, 2018

test: Merge and clean up kubevirt tests #8465

Merged

packagekit: Render Markdown changelogs #8430

Merged

kubernetes: Virtual machines side tab added #7830

kubernetes: Virtual machines side tab added #7830

Conversation

jniederm commented Oct 6, 2017

petervo 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

jniederm commented Oct 6, 2017

jniederm commented Oct 6, 2017 • edited Loading

jniederm commented Oct 6, 2017

Choose a reason for hiding this comment

petervo commented Oct 6, 2017

mareklibra commented Oct 11, 2017

petervo commented Oct 17, 2017

mareklibra commented Oct 25, 2017

petervo commented Oct 25, 2017

fabiand commented Nov 9, 2017

jniederm commented Nov 10, 2017

jniederm commented Dec 6, 2017

martinpitt commented Dec 6, 2017

petervo commented Dec 6, 2017

michalskrivanek commented Dec 7, 2017

fabiand commented Dec 20, 2017

petervo commented Dec 20, 2017

fabiand commented Dec 20, 2017

michalskrivanek commented Jan 9, 2018

jniederm commented Jan 9, 2018

mareklibra commented Jan 12, 2018

mareklibra commented Jan 12, 2018

mareklibra commented Jan 16, 2018

martinpitt 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

martinpitt commented Jan 17, 2018

jniederm commented Jan 20, 2018

jniederm commented Jan 20, 2018

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jniederm commented Oct 6, 2017 •

edited

Loading