Skip to content
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

vms: VM management - initial commit #4434

Merged
merged 5 commits into from Sep 29, 2016

Conversation

Projects
None yet
9 participants
@mareklibra
Copy link
Contributor

commented May 18, 2016

VM Management and Monitoring for Cockpit is introduced.

Functionality in this commit:
  - list of defined VMs (in all states)
  - Vm detail view
    - overview
    - usage charts
  - basic VM operations (start, stop, restart)

Features:
  - React/Redux based
  - libvirt as data provider
  - base architecture for ongoing enhancements

This is an initial commit to get feedback before ongoing work.

Remaining work:

  • Design Work and Wireframes
  • Integration tests
  • Work for non-root wheel group users
  • Measure impact on packaging (with babel 5.x is a good compromise)
  • Refreshing and Polling

@mareklibra mareklibra force-pushed the mareklibra:vms branch 2 times, most recently from 87117c0 to 76b44e8 May 18, 2016

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented May 24, 2016

@andreasn This needs design.

@mareklibra I can't get this to work properly. Why is there a refresh button on the main page?

The name of the package needs a bit more thought. Maybe 'vms' is okay, but likely we'll need to consider how listing of machined data also shows up here.

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented May 24, 2016

I'll add a list of checkboxes above.

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented May 24, 2016

FWIW, I think this should likely show up in the main section, and not under "Tools".

bower.json Outdated
"term.js-cockpit": "#0.0.7"
}
"term.js-cockpit": "#0.0.7",
"jquery-amend": "cockpit-project/jquery-amend",

This comment has been minimized.

Copy link
@stefwalter

stefwalter May 24, 2016

Contributor

We no longer need jquery-amend.

bower.json Outdated
}
"term.js-cockpit": "#0.0.7",
"jquery-amend": "cockpit-project/jquery-amend",
"redux": "https://cdnjs.cloudflare.com/ajax/libs/redux/3.5.1/redux.js"

This comment has been minimized.

Copy link
@stefwalter

stefwalter May 24, 2016

Contributor

Any reason not to use a normal bower version here?

This comment has been minimized.

Copy link
@mareklibra

mareklibra May 25, 2016

Author Contributor

There's no redux bower package, please see reduxjs/redux#579

@@ -16,6 +16,8 @@ nodist_base_DATA = \
pkg/base1/jquery.min.js.gz \
pkg/base1/cockpit.min.js.gz \
pkg/base1/bundle.min.js.gz \
pkg/base1/react.min.js.gz \
pkg/base1/redux.min.js.gz \

This comment has been minimized.

Copy link
@stefwalter

stefwalter May 24, 2016

Contributor

Lets put this redux.js file the vms package for now. We're actively moving stuff out of the base1 package until we can reliably commit to it being stable long term.

@@ -78,6 +80,8 @@ pkg/base1/term.min.js: pkg/base1/term.js
$(MIN_JS_RULE)
pkg/base1/react.min.js: pkg/base1/react.js
$(MIN_JS_RULE)
pkg/base1/redux.min.js: pkg/base1/redux.js
$(MIN_JS_RULE)

This comment has been minimized.

Copy link
@stefwalter

stefwalter May 24, 2016

Contributor

Again, this should go in the vms package.

@@ -133,6 +137,7 @@ CLEANFILES += \
pkg/base1/moment.min.js \
pkg/base1/mustache.min.js \
pkg/base1/react.min.js \
pkg/base1/redux.min.js \

This comment has been minimized.

Copy link
@stefwalter

stefwalter May 24, 2016

Contributor

Ditto.

@@ -151,6 +156,7 @@ EXTRA_DIST += \
pkg/base1/mustache.min.js \
pkg/base1/moment.min.js \
pkg/base1/react.min.js \
pkg/base1/redux.min.js \

This comment has been minimized.

Copy link
@stefwalter

stefwalter May 24, 2016

Contributor

Ditto.

@@ -0,0 +1,90 @@
SRCDIR=pkg/vms

This comment has been minimized.

Copy link
@stefwalter

stefwalter May 24, 2016

Contributor

Using the name $(SRCDIR) is confusing as it reads like $(srcdir). Another name would be better here.

function virt (method, action) {
return (dispatch, getState) => getVirtProvider({ dispatch, getState }).then(provider => {
if (method in provider) {
console.log(`Calling ${provider.name}.${method}(${JSON.stringify(action)})`);

This comment has been minimized.

Copy link
@stefwalter

stefwalter May 24, 2016

Contributor

The debug lines should be hidden behind a toggle switch to turn them on. I'm sure you had plans for that, just figure I'd note it here as a merge req.

return cockpit.resolve(state.config.provider);
} else {
const deferred = cockpit.defer();
console.log('Discovering provider');

This comment has been minimized.

Copy link
@stefwalter

stefwalter May 24, 2016

Contributor

Does this display either libvirt or another provider? In which case what do you think about this being a part of build dependencies, resulting in separate conflicting cockpit-libvirt and cockpit-anotherprovider subpackages?

This comment has been minimized.

Copy link
@mareklibra

mareklibra May 25, 2016

Author Contributor

Good point. Recently it's considered to be exclusive either/or.

So far I'm not sure if merging data from different providers will be needed or not.
I think not, but I'm not sure.

If we keep all the providers together, we get possibility to switch them at runtime. It might be useful for testing of our code same as the providers itself. For sure there will be different dataset retrieved from different providers, the administrator might be interested in seeing the difference.

The overhead is small - one JS file per provider as the provider API is designed.

I need to think more about it.


export function updateOrAddVm ({ id, name, state, osType, fqdn, uptime, currentMemory, vcpus, autostart }) {
let vm = {};
if (id !== undefined) vm.id = id;

This comment has been minimized.

Copy link
@stefwalter

stefwalter May 24, 2016

Contributor

Shouldn't this use an Object.assign() or similar function? Or should callers just use Object.assign()?

@@ -0,0 +1,4 @@
.icon-2x-vms{font-size: 2em;}

This comment has been minimized.

Copy link
@stefwalter

stefwalter May 24, 2016

Contributor

The syntax should look more like:

.icon-2x-vms {
    font-size: 2em;
}
</div>
<span>
<h1>No VM is running on this host</h1>
<p>Let's keep hoping in a change or start a new one ...</p>

This comment has been minimized.

Copy link
@stefwalter

stefwalter May 24, 2016

Contributor

During design this text will need to change.

<div className="panel-heading">
Running Virtual Machines
<div className="actions pull-right">
<ReloadSwitch

This comment has been minimized.

Copy link
@stefwalter

stefwalter May 24, 2016

Contributor

We need to have a good way in Cockpit for a component to tell whether it's foreground focus or not. When this component is in the foreground, refresh could be on, when in the background refresh would be off.

This comment has been minimized.

Copy link
@mareklibra

mareklibra May 25, 2016

Author Contributor

That's what ract/redux does. If the redux store is updated, the subscribed listener (means react virtual dom) is refreshed. Then React compares with the actual DOM and makes changes.
With redux there is (or should not be) any reason to care about this kind of optimization.

For completeness, the Refresh switch turns on/off the polling. The polling needs to be on to read usage details (historical data will be collected by JavaScript) and get notified of changes.

Polling is not nice in general but I don't see any other option now.

The very first implementation was based on machined but I dropped that code.
Machined signals new/deleted VMs but the provided VM properties set is very limited. Since we still need to poll libvirt to get actual VM usage, these 2 signals are not needed.

It's design decision that this Refresh Switch component is rendered as part of the HostVmsList. There's no need for that, it just seems to be compact.

This comment has been minimized.

Copy link
@stefwalter

stefwalter May 31, 2016

Contributor

Right, but polling should stop when the VMs page is not visible. See #4492 for work that makes this possible.


vmsimagesdir = $(vmsdir)/images
vmsimages_DATA = \
$(SRCDIR)/images/vm_shutdown.png \

This comment has been minimized.

Copy link
@stefwalter

stefwalter May 24, 2016

Contributor

Is this file used? I would suggest not using an image here but an icon from the FontAwesome font + CSS colors .

http://fontawesome.io/icons/

This comment has been minimized.

Copy link
@mareklibra

mareklibra May 25, 2016

Author Contributor

Leftover, removed.

@@ -0,0 +1,9 @@
{
"version": 0,
"tools": {

This comment has been minimized.

Copy link
@stefwalter

stefwalter May 24, 2016

Contributor

I think this can change to "menu", to put this in the main menu.

<link rel="stylesheet" href="app.css">
<script type="text/javascript" src="../base1/bundle.js"></script>
<script type="text/javascript" src="../base1/jquery.js"></script>
<script type="text/javascript" src="../base1/cockpit.js"></script>

This comment has been minimized.

Copy link
@stefwalter

stefwalter May 24, 2016

Contributor

There's no need to load both jquery.js and cockpit.js if you're going to be loading base1/bundle.js ...

@andreasn

This comment has been minimized.

Copy link
Contributor

commented May 24, 2016

Sure, I'll give a shot at the design!

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented May 24, 2016

The changes to add babel, and es6 support in the build system should be a separate commit, IMO.

@petervo

This comment has been minimized.

Copy link
Contributor

commented May 24, 2016

IMO, until we support each package in pkg/ having it's own build setup (ei webpack) we shouldn't have ES6 javascript in the main repo cockpit. I think this is a good reason to prioritize that work, but i'm hesitant to introduce more transpiling to the current build setup.

Obviously it's not my call, just my 0.02.

@mareklibra

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2016

@petervo, do you have estimation when this change will happen? There's more to do before the merge, like the design review, so this probably doesn't seem to be blocker for now.

Do you think the vms package can be the first one with its separate (webpack) build?
Are there already ideas how this separation shall be implemented? How can I help?

@mareklibra mareklibra force-pushed the mareklibra:vms branch 2 times, most recently from 06dbc7f to 5df9ec3 May 25, 2016

mareklibra added some commits Jul 11, 2016

machines: VM management - initial commit
VM Management and Monitoring for Cockpit is introduced.

Functionality in this commit:
  - list of defined VMs (in all states)
  - Vm detail view
    - overview
    - usage charts
  - basic VM operations (start, stop, restart)

Features:
  - React/Redux based
  - libvirt as data provider
  - base architecture for ongoing enhancements

This is an initial commit to get feedback before ongoing work.
machines: Integration tests
Initial commit.

@mareklibra mareklibra force-pushed the mareklibra:vms branch from 6dd89cc to ca2f581 Sep 27, 2016

@mareklibra

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2016

Rebased.
Updated for the changes requested so far.

@dperpeet dperpeet removed the needswork label Sep 28, 2016

@dperpeet dperpeet assigned dperpeet and unassigned larskarlitski Sep 28, 2016

@dperpeet

This comment has been minimized.

Copy link
Member

commented Sep 28, 2016

Needs new rhel-7 and centos-7 images
blocked by #5095

@dperpeet dperpeet added blocked and removed blocked labels Sep 28, 2016

@dperpeet

This comment has been minimized.

Copy link
Member

commented Sep 28, 2016

If the tests pass, this is good to be merged.

@dperpeet

This comment has been minimized.

Copy link
Member

commented Sep 29, 2016

fedora-atomic test failure not relevant

@dperpeet dperpeet merged commit cec0f02 into cockpit-project:master Sep 29, 2016

9 of 10 checks passed

verify/fedora-atomic 2 tests failed
Details
avocado/fedora-24 Tests passed
Details
container/kubernetes Tests passed
Details
selenium/chrome Tests passed
Details
selenium/firefox Tests passed
Details
verify/centos-7 Tests passed
Details
verify/debian-unstable Tests passed
Details
verify/fedora-25 Tests passed
Details
verify/rhel-7 Tests passed
Details
verify/rhel-atomic Tests passed
Details

@xsgordon xsgordon referenced this pull request Dec 4, 2016

Closed

[RFE] Libvirt monitoring #2099

@igorSmirnovPlexi

This comment has been minimized.

Copy link

commented Dec 6, 2016

Hi, I'm trying to check the virtual machine package on my machine, but without a result. The virtual machine tab does not appear in the menu.

machine info:
Fedora 24
cockpit 125
libvirtd service is running

What I'm doing wrong?

@mareklibra

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2016

@igorSmirnovPlexi ,
Are 'machines' listed in the main cockpit menu (left-most column)?
Do you have 'cockpit-machines' package installed? (rpm -qa|grep cockpit-machines)

@igorSmirnovPlexi

This comment has been minimized.

Copy link

commented Dec 6, 2016

@mareklibra Thank you man,
The cockpit-machines package was missing, I thought it comes by default during installation.

@igorSmirnovPlexi

This comment has been minimized.

Copy link

commented Dec 12, 2016

Thank you for the new virtualization package.
Are you planning to add new features?

@mareklibra

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2016

Definitely yes. Recently I'm working on support for external providers, like oVirt or ManageIQ in addition to recent libvirt.
Some of additional less complex tasks include qemu://session, VM screenshots, console, VM backup/restore, etc.

Is there something you are looking for in particular?

@igorSmirnovPlexi

This comment has been minimized.

Copy link

commented Dec 13, 2016

Actually we have users looking for Kimchi functionality in Cockpit, so that they don't have to use both GUIs. So anything that can take the cockpit-machine package close to kimchi is more that appreciated.

@mareklibra mareklibra deleted the mareklibra:vms branch Feb 23, 2017

@BentHaase

This comment has been minimized.

Copy link

commented May 11, 2017

Is there any plan to advance this feature?
For example to implement the features provided by virt-manager which would make cockpit-machines a really handy tool.

@mareklibra

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2017

@ET-Bent , yes, there are already multiple follow-ups for this 4434.
The long term vision is to achieve the virt-manager functionality.

@BentHaase

This comment has been minimized.

Copy link

commented May 13, 2017

@mareklibra okay. Thank you for your answer. I will keep coming around to cockpit from time to time because this seems to be the first good solution for server monitoring / virt-manager in the web functionality without a ton of packages and special setups needed.

@GongT

This comment has been minimized.

Copy link

commented May 28, 2017

@mareklibra
I have a idea: auto generate config file for already exists "multiple servers" function, if there's cockpit instance running in vms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.