New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

oVirt Machines #7139

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@mareklibra
Contributor

mareklibra commented Jun 29, 2017

With this PR, cockpit-machines-ovirt-provider is migrated into Cockpit code base under pkg/ovirt.

The newly introduced pkg/ovirt is an extension of pkg/machines for oVirt functionality, namely:

  • active actions (i.e. SHUT DOWN) are routed to oVirt instead of direct libvirt call
  • extended list of actions
  • enriched list of rendered VM properties
  • list of cluster VMs - those defined on oVirt engine with basic actions like 'Run'
  • list of VM templates along with CREATE VM action
  • configuration of local VDSM (vdsm.conf)

Depends on:

Requires:

  • Use cockpit's modal dialog for action confirmation
  • refresh oVirt login when SSO token expires
  • spinner when login-in-progress
  • installation - setup for oVirt URL, generate override.json
  • noVNC
  • migrate cluster-view
  • migrate edit of vdsm.conf
  • README
  • adapt noVnc to bundled version
  • Integration tests - ovirt functionality
  • An oVirt image
  • rebase & squash
  • Documentation in doc/guide/feature-ovirt.xml
  • Brief Youtube video recorded showing this feature

Walk-through video: https://youtu.be/5i-kshT6c5A

Show outdated Hide outdated pkg/ovirt/config.es6
clusterId: host.cluster ? host.cluster.id : undefined,
status: host.status,
memory: host.memory,
cpu: host.cpu ? {

This comment has been minimized.

@mykaul

mykaul Jul 2, 2017

Why would we refresh host cpu name, speed, etc.? Is it ever going to change after the 1st refresh?

@mykaul

mykaul Jul 2, 2017

Why would we refresh host cpu name, speed, etc.? Is it ever going to change after the 1st refresh?

This comment has been minimized.

@mareklibra

mareklibra Jul 12, 2017

Contributor

Right, but the data are retrieved fir no additional cost anyway.
Same parsing is shared for both the initial and the subsequent load.

@mareklibra

mareklibra Jul 12, 2017

Contributor

Right, but the data are retrieved fir no additional cost anyway.
Same parsing is shared for both the initial and the subsequent load.

Show outdated Hide outdated pkg/ovirt/ovirtApiAccess.es6
@mareklibra

This comment has been minimized.

Show comment
Hide comment
@mareklibra

mareklibra Jul 26, 2017

Contributor

Rebased.
So far functionality of former cockpit-machines-ovirt-provider is migrated to cockpit's code base.

Remains:

  • 7133 merge
  • update of README file
  • review CSP in override.json
  • integration tests
  • examination of semi-event driven data refresh via oVirt's AuditLog messages
Contributor

mareklibra commented Jul 26, 2017

Rebased.
So far functionality of former cockpit-machines-ovirt-provider is migrated to cockpit's code base.

Remains:

  • 7133 merge
  • update of README file
  • review CSP in override.json
  • integration tests
  • examination of semi-event driven data refresh via oVirt's AuditLog messages
@mareklibra

This comment has been minimized.

Show comment
Hide comment
@mareklibra

mareklibra Aug 17, 2017

Contributor

Rebased.
Connection to the new ovirt image from check-ovirt works.

Contributor

mareklibra commented Aug 17, 2017

Rebased.
Connection to the new ovirt image from check-ovirt works.

@mareklibra

This comment has been minimized.

Show comment
Hide comment
@mareklibra

mareklibra Sep 1, 2017

Contributor

Rebased, basic integration tests are implemented

Contributor

mareklibra commented Sep 1, 2017

Rebased, basic integration tests are implemented

@mareklibra mareklibra changed the title from WIP: oVirt Machines to oVirt Machines Sep 5, 2017

@mareklibra

This comment has been minimized.

Show comment
Hide comment
@mareklibra

mareklibra Sep 7, 2017

Contributor

This is ready for review

Contributor

mareklibra commented Sep 7, 2017

This is ready for review

@mareklibra

This comment has been minimized.

Show comment
Hide comment
@mareklibra

mareklibra Sep 8, 2017

Contributor

Rebased after F24 EOL

Contributor

mareklibra commented Sep 8, 2017

Rebased after F24 EOL

@stefwalter

I'm marking this 'Request changes' because it needs broader discussion ... either on the mailing list or in a breakout/hangout ... with a summary on the mailing list.

Show outdated Hide outdated package.json
@mareklibra

This comment has been minimized.

Show comment
Hide comment
@mareklibra

mareklibra Sep 12, 2017

Contributor

Minor UI change: rephrase true|false to yes|no

Contributor

mareklibra commented Sep 12, 2017

Minor UI change: rephrase true|false to yes|no

@mareklibra

This comment has been minimized.

Show comment
Hide comment
@mareklibra

mareklibra Sep 12, 2017

Contributor

cockpit-machines enriched for oVirt details:
01_host

List of virtual machines defined in the oVirt cluster
02_cluster

List of templates to create new VMs from
03_templates

First-time Installation dialog:
04_installationdialog

Contributor

mareklibra commented Sep 12, 2017

cockpit-machines enriched for oVirt details:
01_host

List of virtual machines defined in the oVirt cluster
02_cluster

List of templates to create new VMs from
03_templates

First-time Installation dialog:
04_installationdialog

@mareklibra

This comment has been minimized.

Show comment
Hide comment
@mareklibra

mareklibra Sep 13, 2017

Contributor

@petervo , I just pushed 4e96dc6 to revert the micro-dependencies.
make-rpms is still working for me locally while having

 node v6.11.1 (npm v3.10.10)
Contributor

mareklibra commented Sep 13, 2017

@petervo , I just pushed 4e96dc6 to revert the micro-dependencies.
make-rpms is still working for me locally while having

 node v6.11.1 (npm v3.10.10)
@stefwalter

Thanks for fixing the package.json. Some more review here.

Show outdated Hide outdated doc/guide/feature-ovirt.xml
Show outdated Hide outdated doc/guide/feature-ovirt.xml
Show outdated Hide outdated doc/guide/feature-virtualmachines.xml
Show outdated Hide outdated doc/guide/feature-virtualmachines.xml
Show outdated Hide outdated pkg/ovirt/manifest.json.in
Show outdated Hide outdated test/common/phantom-command
def setUp(self):
MachineCase.setUp(self)
# enforce use of cockpit-machines instead of cockpit-ovirt

This comment has been minimized.

@stefwalter

stefwalter Sep 13, 2017

Contributor

Do RPMs of the two normally conflict for real users? Or is it the case that normally when cockpit-ovirt is installed it overrides cockpit-machines. If so this should be documented in doc features page.

@stefwalter

stefwalter Sep 13, 2017

Contributor

Do RPMs of the two normally conflict for real users? Or is it the case that normally when cockpit-ovirt is installed it overrides cockpit-machines. If so this should be documented in doc features page.

This comment has been minimized.

@mareklibra

mareklibra Sep 14, 2017

Contributor

Packages are not mutually exclusive but cockpit-ovirt overrides cockpit-machines (manifest.json:priority). Documentation is updated.

@mareklibra

mareklibra Sep 14, 2017

Contributor

Packages are not mutually exclusive but cockpit-ovirt overrides cockpit-machines (manifest.json:priority). Documentation is updated.

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Sep 13, 2017

Contributor

@mareklibra

So applying the following diff gets semaphore to use the latest node package that it knows about and the build succeeds.

diff --git a/tools/semaphore-run b/tools/semaphore-run
index 3aa7199..953f70a 100755
--- a/tools/semaphore-run
+++ b/tools/semaphore-run
@@ -1,5 +1,9 @@
 #!/bin/bash
 
+# Use the latest lts node version
+. ~/.nvm/nvm.sh
+nvm use lts/*
+
 set -o pipefail
 set -eux

But doing that just changes the way the npm install works. The deps in question still end up in the
node_modules directory. So I don't really see a difference between doing this or adding the package deps manually.

However we should probably discuss further. I'm sure others have opinions on this too. We were going to bring this up as a topic on our monday #cockpit meeting on IRC. Are you ok waiting on resolving this until then?

Contributor

petervo commented Sep 13, 2017

@mareklibra

So applying the following diff gets semaphore to use the latest node package that it knows about and the build succeeds.

diff --git a/tools/semaphore-run b/tools/semaphore-run
index 3aa7199..953f70a 100755
--- a/tools/semaphore-run
+++ b/tools/semaphore-run
@@ -1,5 +1,9 @@
 #!/bin/bash
 
+# Use the latest lts node version
+. ~/.nvm/nvm.sh
+nvm use lts/*
+
 set -o pipefail
 set -eux

But doing that just changes the way the npm install works. The deps in question still end up in the
node_modules directory. So I don't really see a difference between doing this or adding the package deps manually.

However we should probably discuss further. I'm sure others have opinions on this too. We were going to bring this up as a topic on our monday #cockpit meeting on IRC. Are you ok waiting on resolving this until then?

@mareklibra

This comment has been minimized.

Show comment
Hide comment
@mareklibra

mareklibra Sep 14, 2017

Contributor

@petervo , thanks for the effort. Sure it can wait for others, I can address remaining comments in the meantime or continue with other features.

Contributor

mareklibra commented Sep 14, 2017

@petervo , thanks for the effort. Sure it can wait for others, I can address remaining comments in the meantime or continue with other features.

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Sep 18, 2017

Contributor

So we talked about this on IRC a little more today. I think our preference would be

  1. Use something lighter weight than react-router-dom. But I'm not sure if there are any better alternatives. Also in the past i've seen cockpit not play nice with other routers that try to manage history since cockpit shell tries to manage that as well. But maybe you've already sorted that out.
  2. Use something already that already bundles together react-router-dom and it's deps. So as stef put it, instead of us having to distribute A+B+C+D+E we can just distribute M where someone else did M = A+B+C+D+E.
  3. If none of the above are viable then at the very least we need to make sure that .tools/build-copying and ./tools/build-debian-copyright work properly with all the dependencies here.

@stefwalter or @mvollmer please weigh in if I got anything wrong or if you have anything to add.

Contributor

petervo commented Sep 18, 2017

So we talked about this on IRC a little more today. I think our preference would be

  1. Use something lighter weight than react-router-dom. But I'm not sure if there are any better alternatives. Also in the past i've seen cockpit not play nice with other routers that try to manage history since cockpit shell tries to manage that as well. But maybe you've already sorted that out.
  2. Use something already that already bundles together react-router-dom and it's deps. So as stef put it, instead of us having to distribute A+B+C+D+E we can just distribute M where someone else did M = A+B+C+D+E.
  3. If none of the above are viable then at the very least we need to make sure that .tools/build-copying and ./tools/build-debian-copyright work properly with all the dependencies here.

@stefwalter or @mvollmer please weigh in if I got anything wrong or if you have anything to add.

@mareklibra

This comment has been minimized.

Show comment
Hide comment
@mareklibra

mareklibra Sep 19, 2017

Contributor

@petervo, thanks for summarizing.

The use of react-router within pkg/ovirt is recently nothing complex and there is no expectation of growing into a complex, hard-to-manage, multi-subpage solution where a generic tool like react-router would become 'a must'.

List of recent subpages is more-or-less final, maybe a bare minimum of others might be added in the future.

Considering the issue it caused, I think there's no harm in implementing a simple router within pkg/ovirt on my own. Back button support and navigation to particular subpage via URL will not be supported within first implementation but can be added later - the routing-related code will be kept isolated.

Contributor

mareklibra commented Sep 19, 2017

@petervo, thanks for summarizing.

The use of react-router within pkg/ovirt is recently nothing complex and there is no expectation of growing into a complex, hard-to-manage, multi-subpage solution where a generic tool like react-router would become 'a must'.

List of recent subpages is more-or-less final, maybe a bare minimum of others might be added in the future.

Considering the issue it caused, I think there's no harm in implementing a simple router within pkg/ovirt on my own. Back button support and navigation to particular subpage via URL will not be supported within first implementation but can be added later - the routing-related code will be kept isolated.

@stefwalter

This comment has been minimized.

Show comment
Hide comment
@stefwalter

stefwalter Sep 19, 2017

Contributor

@stefwalter or @mvollmer please weigh in if I got anything wrong or if you have anything to add.

This seems right. Our current javascript dependency story is already on shaky ground.

Over time our javascript dependency, building and distribution story will become clearer with regard to each of the myriad distros that we release in ... at which point we may be able to dive full bore into bundling node micro-dependencies.

Contributor

stefwalter commented Sep 19, 2017

@stefwalter or @mvollmer please weigh in if I got anything wrong or if you have anything to add.

This seems right. Our current javascript dependency story is already on shaky ground.

Over time our javascript dependency, building and distribution story will become clearer with regard to each of the myriad distros that we release in ... at which point we may be able to dive full bore into bundling node micro-dependencies.

@mareklibra

This comment has been minimized.

Show comment
Hide comment
@mareklibra

mareklibra Sep 19, 2017

Contributor

Recent changes:

  • react-router and related changes removed from this PR
  • react-router replaced by simpler but working routing based on redux
Contributor

mareklibra commented Sep 19, 2017

Recent changes:

  • react-router and related changes removed from this PR
  • react-router replaced by simpler but working routing based on redux
Show outdated Hide outdated pkg/ovirt/install.sh
} \
} \
}" >> ${CONFIG_FILE}
fi

This comment has been minimized.

@stefwalter

stefwalter Sep 20, 2017

Contributor

What local command line tooling is used to interact with oVirt. What commands are used to set it up? Can we just share configuration with the local tooling?

@stefwalter

stefwalter Sep 20, 2017

Contributor

What local command line tooling is used to interact with oVirt. What commands are used to set it up? Can we just share configuration with the local tooling?

This comment has been minimized.

@mareklibra

mareklibra Sep 20, 2017

Contributor

To run pkg/ovirt, tooling used by pkg/machines is sufficient (recently means virsh).

This configuration is for hacking only, like within integration tests where VDSM/Libvirt is running on a different machine than the one with cockpit. So for the test purposes, virsh connects to the ovirt-host via qemu+tcp:///... URI while leveraging the fact that an oVirt host has Libvirt port open by default due to VM migrations.

It's not a recent use case, but I can imagine weird deployments where the user might want manage VMs running on a different host than the one with cockpit. Its would be feasible by providing this configuration.

Within #7484, such a configuration will be available even for pkg/machines.

@mareklibra

mareklibra Sep 20, 2017

Contributor

To run pkg/ovirt, tooling used by pkg/machines is sufficient (recently means virsh).

This configuration is for hacking only, like within integration tests where VDSM/Libvirt is running on a different machine than the one with cockpit. So for the test purposes, virsh connects to the ovirt-host via qemu+tcp:///... URI while leveraging the fact that an oVirt host has Libvirt port open by default due to VM migrations.

It's not a recent use case, but I can imagine weird deployments where the user might want manage VMs running on a different host than the one with cockpit. Its would be feasible by providing this configuration.

Within #7484, such a configuration will be available even for pkg/machines.

@stefwalter

This comment has been minimized.

Show comment
Hide comment
@stefwalter

stefwalter Sep 20, 2017

Contributor

This also needs a brief Youtube video recorded for this feature, for the release notes.

Contributor

stefwalter commented Sep 20, 2017

This also needs a brief Youtube video recorded for this feature, for the release notes.

@stefwalter stefwalter added the question label Sep 20, 2017

@mareklibra

This comment has been minimized.

Show comment
Hide comment
@mareklibra

mareklibra Sep 21, 2017

Contributor

Recent changes:

  • direct ajax API call is replaced by proxy (cockpit.http() and cockpit-bridge)
  • install.sh does not generate override.json anymore (proxy eases the CSP pain)
  • manifest.json contains content-security-policy to allow dynamic images (icons) and .vv file download
  • to register oVirt, the user enters just FQDN and port (not full API URL)

Within a follow-up, the user will be able to change the registered oVirt FQDN

Contributor

mareklibra commented Sep 21, 2017

Recent changes:

  • direct ajax API call is replaced by proxy (cockpit.http() and cockpit-bridge)
  • install.sh does not generate override.json anymore (proxy eases the CSP pain)
  • manifest.json contains content-security-policy to allow dynamic images (icons) and .vv file download
  • to register oVirt, the user enters just FQDN and port (not full API URL)

Within a follow-up, the user will be able to change the registered oVirt FQDN

Pull request has been significantly updated.

@larskarlitski

This comment has been minimized.

Show comment
Hide comment
@larskarlitski

larskarlitski Oct 11, 2017

Contributor

I've looked at this and am fine merging it and addressing remaining points via issues, like we discussed.

I do have two questions before merging though:

  1. Does passing --web-security=false to phantomjs affect the other tests? We probably don't want to allow accessing other hosts in general, do we? Is there a way to only enable this for the ovirt tests? @petervo - do you have an opinion on that?

  2. I'm not entirely sure I like the way it overrides the machines package. It's fine for users, because they can just not install cockpit-ovirt. Developers however will always have to mask it when working on the machines package itself. Is there a way to only enable the ovirt package when the machine is part of an ovirt cluster? (I know you've said that that might be hard to figure out - just making sure again).

Thanks for all the work you've put into this.

Contributor

larskarlitski commented Oct 11, 2017

I've looked at this and am fine merging it and addressing remaining points via issues, like we discussed.

I do have two questions before merging though:

  1. Does passing --web-security=false to phantomjs affect the other tests? We probably don't want to allow accessing other hosts in general, do we? Is there a way to only enable this for the ovirt tests? @petervo - do you have an opinion on that?

  2. I'm not entirely sure I like the way it overrides the machines package. It's fine for users, because they can just not install cockpit-ovirt. Developers however will always have to mask it when working on the machines package itself. Is there a way to only enable the ovirt package when the machine is part of an ovirt cluster? (I know you've said that that might be hard to figure out - just making sure again).

Thanks for all the work you've put into this.

@mareklibra

This comment has been minimized.

Show comment
Hide comment
@mareklibra

mareklibra Oct 11, 2017

Contributor

@larskarlitski , thanks for your feedback.

  1. The --web-security=false might be leftover now since within the course of this pull request I changed the direct AJAX call (involving CORS) to proxy behind cockpit.http(). I need to run tests, but I believe this might be removed from this PR.

  2. Tests will (hopefully) ensure integrity. Anyway, it's up to deployment procedure whethe cockpit-machines.rpm or cockpit-ovirt.rpm are installed. If both, cockpit-ovirt wins (see manifest.json:priority).

It means:

  • for a non-oVirt host, there's probably no reason to install cockpit-ovirt unless hacking.
  • for an oVirt host, the cockpit-ovirt package is meant to be installed automatically within standard oVirt host installation procedure we do once the host is added to a cluster. There's probably no need to install cockpit-machines manually/along but still possible. Anyway, such cockpit-machines will have limited access to Libvirt due to permissions/password set by oVirt. The user would need to modify Libvirt connection URI but it's hacky and not supported by oVirt. I can't see use case for it once the host is part of an oVirt cluster.
Contributor

mareklibra commented Oct 11, 2017

@larskarlitski , thanks for your feedback.

  1. The --web-security=false might be leftover now since within the course of this pull request I changed the direct AJAX call (involving CORS) to proxy behind cockpit.http(). I need to run tests, but I believe this might be removed from this PR.

  2. Tests will (hopefully) ensure integrity. Anyway, it's up to deployment procedure whethe cockpit-machines.rpm or cockpit-ovirt.rpm are installed. If both, cockpit-ovirt wins (see manifest.json:priority).

It means:

  • for a non-oVirt host, there's probably no reason to install cockpit-ovirt unless hacking.
  • for an oVirt host, the cockpit-ovirt package is meant to be installed automatically within standard oVirt host installation procedure we do once the host is added to a cluster. There's probably no need to install cockpit-machines manually/along but still possible. Anyway, such cockpit-machines will have limited access to Libvirt due to permissions/password set by oVirt. The user would need to modify Libvirt connection URI but it's hacky and not supported by oVirt. I can't see use case for it once the host is part of an oVirt cluster.

mareklibra added some commits Aug 7, 2017

ovirt: Bring pkg/ovirt into codebase
Extension of pkg/machines for oVirt [1] host specifics.

Based on cockpit-machines-ovirt-provider project [2].

Working:
    - login into oVirt
    - oVirt provider's verbs
    - data retrieval from oVirt
    - machine's Overview subtab extended for oVirt specifics
    - new oVirt subtab added
    - top-level navigation with Host | Cluster | VDSM views
    - configuration retrieval
    - dialog to set oVirt API URL
    - noVNC

[1] www.ovirt.org
[2] github.com/oVirt/cockpit-machines-ovirt-provider
ovirt: integration tests
Integration test for pkg/ovirt is implemented.
test-static-code: blacklist dist/machines/base.css from font-path checks
The "tools/test-static-code 5 patternfly-font-paths" is related
to patternfly only and the noVnc's base.css distributed along
'machines' or 'ovirt' packages shall be excluded from this test.
@mareklibra

This comment has been minimized.

Show comment
Hide comment
@mareklibra

mareklibra Oct 12, 2017

Contributor

So the phantom's --web-security=false is no more needed. Removed.

Contributor

mareklibra commented Oct 12, 2017

So the phantom's --web-security=false is no more needed. Removed.

@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Oct 13, 2017

Member

I had a very brief look at the structure, and it appears the changes to pkg/machines are mostly related to some cleanup from the old "external providers" API. So this by and large only affects the new cockpit-ovirt package, thus this is safe to release. Thanks @mareklibra!

Member

martinpitt commented Oct 13, 2017

I had a very brief look at the structure, and it appears the changes to pkg/machines are mostly related to some cleanup from the old "external providers" API. So this by and large only affects the new cockpit-ovirt package, thus this is safe to release. Thanks @mareklibra!

@larskarlitski

This comment has been minimized.

Show comment
Hide comment
@larskarlitski

larskarlitski Oct 13, 2017

Contributor

Merged. Thanks @mareklibra!

Contributor

larskarlitski commented Oct 13, 2017

Merged. Thanks @mareklibra!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment