kubernetes: virtual machines - add Vm Detail page #9466

atiratree · 2018-06-21T14:33:40Z

First stage of #8948

adds Vm Detail page and navigation between vms and vm
this PR still uses old terminology (vm/ovm), but it will be changed in the next stages

I also created DetailPage component for easier creation of pages like this (will be used at least for Vm/Vm Instance). Not sure if I should split the component to another PR?

Todo:

polish ui
fix tests (mainly ids)
add tests - basic use case + navigation

atiratree · 2018-06-21T14:40:42Z

Screenshots:

Navigation

Click on vm in vms list

Vm Detail

404 page

atiratree · 2018-06-21T14:51:32Z

Few notes about the design:

I am showing the icon of vm (will be probably the same for vm/vm instance) and "namespace:name". Not sure if I should put the namespace somewhere else
This is the link to go back to the vm list. It is consistent with kubernetes plugin. We could also show a path there
There will be Vm / Vm Instance depending on the type here (old terminology is Offline Vm / Vm)
I also put state there. Please let me know if I should put it somewhere else.

@garrett @andreasn @beanh66

atiratree · 2018-06-21T15:06:21Z

+ the vm details are accessible at /kubernetes#/vms/$VM_UID

atiratree · 2018-06-21T15:13:08Z

+ there are two React 16 TODOs in lib/cockpit-components-detail-page.jsx - #9263

mykaul · 2018-06-21T18:34:00Z

VM, not Vm.
VMs, not Vms.

I'm also unsure about the title 'Vm' instead of just 'Details' ?

beanh66 · 2018-06-21T20:30:06Z

@suomiy For #2 in your list, can we show that as breadcrumbs instead?

atiratree · 2018-06-22T13:30:48Z

VM, not Vm.
VMs, not Vms.

thanks

I'm also unsure about the title 'Vm' instead of just 'Details' ?

Could be 'Details' but we have to indicate the difference between 'VM' and 'VM Instance' somehow, because they will have similar pages.

For #2 in your list, can we show that as breadcrumbs instead?

I can add breadcrumbs. But all of the other pages in Kubernetes plugin are done this way. I am not sure if I should keep it for consistency simpler like this.

There is also another problem:

breadcrumbs for VM Instances > Vm Instance will not have VM Instances page because it will redirect directly into the vms.
but it will have it's own path /kubernetes#/vminstances/$VM_INSTANCE_UID

andreasn · 2018-06-22T22:47:47Z

I can add breadcrumbs. But all of the other pages in Kubernetes plugin are done this way. I am not sure if I should keep it for consistency simpler like this.

I agree with this. It's important to keep consistency across pages. We can revisit the current pattern later if it performs badly in usability tests, and if so, we can change all the navigation across all pages in the kubernetes plugin to work like that. But for now, I would prefer if the current behaviour is kept.

beanh66 · 2018-06-25T11:44:25Z

@suomiy @andreasn Got it, thanks for the explanation! That makes sense to me, keeping consistent behavior in the product. 👍

atiratree · 2018-06-26T12:49:57Z

fixed Vm -> 'VM'

mareklibra · 2018-06-29T08:12:58Z

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

+            <div>
+                <DetailPage>
+                    {header}
+                    <DetailPageRow title={cockpit.format(_("Vm with uid $0 does not exist."), vmUid)} idPrefix={'vm-not-found'} />


nit: uid should be in CAPITAL

fine for a follow-up

mareklibra · 2018-06-29T08:14:05Z

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

@@ -66,7 +66,7 @@ const prepareDiskData = (disk, vm, pvs, idPrefix) => {
    if (pv) {
        bus = _("iSCSI");
        onNavigate = () => cockpit.jump(`/kubernetes#/volumes/${pv.metadata.name}`);
-    } else if (disk.disk.bus) {
+    } else if (disk.disk && disk.disk) {


Is this what was intended?

mareklibra · 2018-06-29T08:42:59Z

pkg/kubernetes/scripts/virtual-machines/entry-points/virtual-machine.jsx

+ * @param {kubeMethods} kubeMethods
+ * @param {KubeRequest} KubeRequest
+ */
+function init($scope, $routeParams, kubeLoader, kubeSelect, kubeMethods, KubeRequest) {


I think we can share can between these two entry points even more

What do you suggest? Unifying the id of the root element and passing the component to render? I do not see other improvements otherwise.

After offline discussion, this is nothing critical. I'm fine with this approach.

mareklibra · 2018-06-29T08:54:02Z

Important to note: this should work for kubevirt >= 0.6.0
Oncoming 0.7.0 (so far just pre-release alpha.3 is out) will change terminology in k8s objects, means code introduced by this PR will need to be adjusted.

I've tested this PR against kubevirt 0.6.0.

Might be my misunderstanding, but
oc get offlinevirtualmachine.kubevirt.io shows 2 OVMs but they are not listed within /kubernetes#/vms view . Just VM Instances are listed there.

I'm not quite sure if recent 2 entrypoints are good solution. If the user navigates between list and detail, redux store is reinitialized so loosing its state. There's so far no issue with that but I can imagine use-cases where this might be serious issue.

atiratree · 2018-07-10T14:16:19Z

changed navigation from "/kubernetes#/vms/$VM_UID" to "/kubernetes#/vms/$VM_NAMESPACE/$VM_NAME" to match other kubernetes pages' behaviour. Also it behaves better when restarting vm (vm can have different UID) . - using old terminology
added tests

@mareklibra please can you rereview?

atiratree · 2018-07-17T15:37:34Z

It seems I am not able to re-trigger the tests with bots/tests-trigger. Tests are triggered successfully when I push the changes.

./bots/tests-trigger 9466

verify/ubuntu-1604: triggering on pull request 9466
statuses/d76ee4e842ff4ca840f07c6c88273c5d68ded607
{"message":"Not Found","documentation_url":"https://developer.github.com/v3/repos/statuses/#create-a-status"}
Traceback (most recent call last):
  File "./bots/tests-trigger", line 74, in <module>
    sys.exit(main())
  File "./bots/tests-trigger", line 71, in main
    return trigger_pull(api, opts)
  File "./bots/tests-trigger", line 55, in trigger_pull
    api.post("statuses/" + revision, changes)
  File "/home/ansy/Projects/IdeaProjects/cockpit/bots/task/github.py", line 201, in post
    raise RuntimeError("GitHub API problem: {0}".format(response['reason'] or status))
RuntimeError: GitHub API problem: Not Found

Does somebody have a clue what might be a problem?

atiratree · 2018-07-17T17:24:57Z

i also have refreshed token in ~/.config/github-token

@martinpitt @stefwalter

martinpitt · 2018-07-17T18:12:51Z

@suomiy : You are in the contributors team, so I'm afraid I don't have an immediate idea what's wrong.

mareklibra · 2018-07-25T13:04:00Z

This should work against kubevirt 0.6.0 - so let's aim at merging this and change later once #9638 lands with >=0.7.1

mareklibra · 2018-07-25T13:29:13Z

One syntax issue found: rest of Cockpit uses semicolons. I'm wondering why linter is not enforcing this. I suggest fix eslint rules along code in a follow-up.

atiratree · 2018-07-25T15:13:25Z

@mareklibra fixed

- used primarily for the detail pages of listings

- add navigation between vms and vm - refactor initialization / redux store - add status to VmOverviewTabKubevirt when in VmDetail - refactor ids in VmMetricsTab

martinpitt · 2018-07-30T10:33:09Z

testKubevirtVmInstance fails, setting needswork. (Let's please not introduce even more flakes, the kubevirt tests are already bad enough for landing PRs.)

atiratree · 2018-07-31T12:25:53Z

testKubevirtVmInstance fails, setting needswork. (Let's please not introduce even more flakes, the kubevirt tests are already bad enough for landing PRs.)

@martinpitt this is actually the same flake as in testKubevirtMachinesList because the detail should show the same UI as the list, thus it is reusing the same code and we get the same flake. I can look at the tests and see if I can fix the flakes, but I am not sure if I will be successful, because the tests usually pass on my machine.

martinpitt · 2018-07-31T12:37:22Z

@suomiy : OK; let's retry the tests instead, the new test should at least be able to succeed.

For reproducing, try to run the tests with -j4 or so, to slow them down :-)

atiratree · 2018-07-31T15:15:25Z

I tried that (100% usage CPU and 90% RAM) but still no luck. Also got few of these: bcbcarl/react-c3js#22 ,which I hope are not a cause of some of these failures.

Current test runs seems to have passed fine for testKubevirtVmInstance

atiratree force-pushed the vm-detail branch from dacddf1 to d2319ae Compare June 21, 2018 14:56

atiratree mentioned this pull request Jun 21, 2018

WIP: kubernetes: add VMs and VM/VMInstance details #8948

Closed

8 tasks

atiratree force-pushed the vm-detail branch 2 times, most recently from 375f208 to edf4a30 Compare June 21, 2018 15:10

atiratree force-pushed the vm-detail branch 10 times, most recently from a599129 to b925116 Compare June 21, 2018 16:21

atiratree force-pushed the vm-detail branch from b925116 to 978c59e Compare June 26, 2018 12:42

atiratree requested a review from mareklibra June 26, 2018 12:50

atiratree force-pushed the vm-detail branch from 978c59e to c140c52 Compare June 26, 2018 14:14

mareklibra suggested changes Jun 29, 2018

View reviewed changes

atiratree force-pushed the vm-detail branch from c140c52 to 58a80dc Compare July 3, 2018 06:31

atiratree force-pushed the vm-detail branch 4 times, most recently from 20da1ed to d76ee4e Compare July 17, 2018 15:25

atiratree force-pushed the vm-detail branch from d76ee4e to 8a4c63d Compare July 19, 2018 12:43

mareklibra approved these changes Jul 25, 2018

View reviewed changes

atiratree mentioned this pull request Jul 25, 2018

eslint: Enable "semi" rule #9736

Merged

atiratree force-pushed the vm-detail branch 3 times, most recently from 1f7f44b to 6136f49 Compare July 26, 2018 15:48

suomiy added 5 commits July 26, 2018 22:35

lib: add DetailPage + subcomponents

ecaa770

- used primarily for the detail pages of listings

kubernetes: virtual machines - add Vm Detail page

dd86650

- add navigation between vms and vm - refactor initialization / redux store - add status to VmOverviewTabKubevirt when in VmDetail - refactor ids in VmMetricsTab

kubernetes: fix kubevirt tests

1565469

kubernetes: virtual machines - add Vm Detail page tests

8f68c23

kubernetes: virtual machines - fix semicolons

c5ea3bc

atiratree force-pushed the vm-detail branch from 6136f49 to c5ea3bc Compare July 26, 2018 20:46

martinpitt added the needswork label Jul 30, 2018

martinpitt removed the needswork label Jul 31, 2018

martinpitt closed this in bcd8490 Jul 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubernetes: virtual machines - add Vm Detail page #9466

kubernetes: virtual machines - add Vm Detail page #9466

atiratree commented Jun 21, 2018 •

edited

Loading

atiratree commented Jun 21, 2018

atiratree commented Jun 21, 2018 •

edited

Loading

atiratree commented Jun 21, 2018

atiratree commented Jun 21, 2018 •

edited

Loading

mykaul commented Jun 21, 2018

beanh66 commented Jun 21, 2018

atiratree commented Jun 22, 2018

andreasn commented Jun 22, 2018

beanh66 commented Jun 25, 2018

atiratree commented Jun 26, 2018

mareklibra Jun 29, 2018

mareklibra Jun 29, 2018

mareklibra Jun 29, 2018

mareklibra Jun 29, 2018

atiratree Jul 3, 2018

mareklibra Jul 25, 2018

mareklibra commented Jun 29, 2018

atiratree commented Jul 10, 2018

atiratree commented Jul 17, 2018

atiratree commented Jul 17, 2018

martinpitt commented Jul 17, 2018

mareklibra commented Jul 25, 2018

mareklibra commented Jul 25, 2018

atiratree commented Jul 25, 2018

martinpitt commented Jul 30, 2018

atiratree commented Jul 31, 2018

martinpitt commented Jul 31, 2018

atiratree commented Jul 31, 2018

kubernetes: virtual machines - add Vm Detail page #9466

kubernetes: virtual machines - add Vm Detail page #9466

Conversation

atiratree commented Jun 21, 2018 • edited Loading

atiratree commented Jun 21, 2018

Screenshots:

Navigation

Vm Detail

404 page

atiratree commented Jun 21, 2018 • edited Loading

atiratree commented Jun 21, 2018

atiratree commented Jun 21, 2018 • edited Loading

mykaul commented Jun 21, 2018

beanh66 commented Jun 21, 2018

atiratree commented Jun 22, 2018

andreasn commented Jun 22, 2018

beanh66 commented Jun 25, 2018

atiratree commented Jun 26, 2018

mareklibra Jun 29, 2018

Choose a reason for hiding this comment

mareklibra Jun 29, 2018

Choose a reason for hiding this comment

mareklibra Jun 29, 2018

Choose a reason for hiding this comment

mareklibra Jun 29, 2018

Choose a reason for hiding this comment

atiratree Jul 3, 2018

Choose a reason for hiding this comment

mareklibra Jul 25, 2018

Choose a reason for hiding this comment

mareklibra commented Jun 29, 2018

atiratree commented Jul 10, 2018

atiratree commented Jul 17, 2018

atiratree commented Jul 17, 2018

martinpitt commented Jul 17, 2018

mareklibra commented Jul 25, 2018

mareklibra commented Jul 25, 2018

atiratree commented Jul 25, 2018

martinpitt commented Jul 30, 2018

atiratree commented Jul 31, 2018

martinpitt commented Jul 31, 2018

atiratree commented Jul 31, 2018

atiratree commented Jun 21, 2018 •

edited

Loading

atiratree commented Jun 21, 2018 •

edited

Loading

atiratree commented Jun 21, 2018 •

edited

Loading