-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
machines: Add LibvirtDBus provider #9240
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this effort, moving away from virsh
is definately good way in the long-term!
Huray for the just released libvirt-dbus package.
Please correct me, but libvirt-dbus has still long way to pass to become a standard part of the supported distributions (aka debian or RHEL).
It's ok but just to be clear here: we will need to support both virsh
and dbus
connections in parallel for non-trivial time (even years?).
Adding Checklist
to the PR description.
pkg/machines/index.es6
Outdated
setVirtProvider(LibvirtDbus); | ||
} else { | ||
logDebug("index.es6: Setting virsh as virt provider."); | ||
setVirtProvider(Libvirt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ovirt/provider.es6
will need to use DBus as well, if available.
(ovirt/index.es6
, resp. inheritance in ovirt/provider.es6:40
)
pkg/machines/index.es6
Outdated
"ListActivatableNames") | ||
.done(services => { | ||
if (services[0].includes("org.libvirt")) { | ||
logDebug("index.es6: Setting libvirt-dbus as virt provider."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use console.info()
for this important decision
pkg/machines/index.es6
Outdated
logDebug("index.es6: Setting libvirt-dbus as virt provider."); | ||
setVirtProvider(LibvirtDbus); | ||
} else { | ||
logDebug("index.es6: Setting virsh as virt provider."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here
pkg/machines/libvirt-dbus.es6
Outdated
return true; | ||
}, | ||
|
||
canReset: (vmState) => vmState == 'running' || vmState == 'idle' || vmState == 'paused', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can following be shared?
pkg/machines/libvirt-dbus.es6
Outdated
serialConsoleCommand: ({ | ||
vm | ||
}) => vm.displays['pty'] ? [ 'virsh', ...VMS_CONFIG.Virsh.connections[vm.connectionName].params, 'console', vm.name ] : false, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... Up to here
pkg/machines/libvirt-dbus.es6
Outdated
const _ = cockpit.gettext; | ||
|
||
let clientLibvirt = { | ||
'session': null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to list them here. They will be undefined
by default, so same semantics. Their name might vary based on the configuration.
bab41b4
to
8fa488e
Compare
94f2ef6
to
8611756
Compare
@mareklibra @martinpitt @Antique I reposted the patches with some fixes, adjusted the tests to run with both providers and some added info about libvirt-dbus in plugin docs. The tests are all passing locally, though with the current images they will fail here, because the available libvirt-dbus release is missing some APIs that I am using in these commits. (See more here https://www.redhat.com/archives/libvir-list/2018-June/msg00898.html) So, although we shouldn't merge this patch series without the new release of libvirt-dbus to be included in the test images, can you please review so that I can get it to a state ready to merge? Regarding supporting the using the libvirt-dbus in oVirt package, I suggest to keep it in separate PR, since current is already very big. WDYT? |
Changes from previous push:
|
pkg/machines/libvirt-dbus.es6
Outdated
}, | ||
|
||
SET_VCPU_SETTINGS ({ name, connectionName, count, max, sockets, cores, threads, isRunning }) { | ||
logDebug(`${this.name}.SET_VCPU_SETTINGS(${name}):`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to implement this :|
I 'll repost.
6a2d70b
to
5a151cb
Compare
#9692 landed, can you please rebase? |
Ready for review. Only testAddDisk for libvirt-dbus provider is failing but I am not sure why at this point. I am afraid that refreshing storagePools here https://github.com/cockpit-project/cockpit/blob/master/pkg/machines/components/diskAdd.jsx#L425 is not actually happening, since the storagePools object it not fetched from this props every time it's updated. (Though I am total beginner with react, feedback would be appreciated) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looking good but there are some minor issues that needs to be addressed.
pkg/machines/index.es6
Outdated
let providerVal = libvirtDBusavailable ? LibvirtDbus : Libvirt; | ||
|
||
client.close(); | ||
return providerVal; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small nit, this can be simplified by removing providerVal and using this return:
return libvirtDBusavailable ? LibvirtDbus : Libvirt;
And you can change libvirtDBusavailable into libvirtDBusAvailable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pkg/machines/libvirt-dbus.es6
Outdated
return dispatch => { | ||
clientLibvirt[connectionName].call(objPath, 'org.libvirt.Domain', 'GetXMLDesc', [0], TIMEOUT) | ||
.done(domXml => { | ||
let updatedXml = updateNetworkIfaceState(domXml[0], networkMac, state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateNetworkIfaceState can return nothing if parsin domXml fails, we should check that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pkg/machines/libvirt-dbus.es6
Outdated
connectionName | ||
}) { | ||
return dispatch => { | ||
clientLibvirt[connectionName].call(objPath, 'org.libvirt.Domain', 'GetXMLDesc', [0], TIMEOUT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetXMLDesc can fail, shouldn't we check that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is it checked? I don't see a .catch
handler here?
pkg/machines/libvirt-dbus.es6
Outdated
clientLibvirt[connectionName].call(storageVolPath[0], 'org.libvirt.StorageVol', 'Delete', [0], TIMEOUT) | ||
.finally(undefine_synced.bind(null, i + 1)); | ||
}) | ||
else /* VIR_DOMAIN_UNDEFINE_MANAGED_SAVE */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably should use VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA and VIR_DOMAIN_UNDEFINE_NVRAM flags as well, otherwise cockpit would not be able to undefine such domains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
test/verify/check-machines-dbus
Outdated
@@ -0,0 +1,67 @@ | |||
#!/usr/bin/python2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather use python3 for new code, every other verify script uses python3.
test/verify/check-machines-dbus
Outdated
|
||
def detectLibvirtDbus(self): | ||
m = self.machine | ||
namesList = m.execute("dbus-send --system --dest=org.freedesktop.DBus " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot we use python dbus module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, this code is running on the machine you execute test tests from, m.execute executes the commands inside the VM the tests are going to run inside. If I was using dbus python module I would need to run it from a script that was running inside machine that contains the dbus I want to poll.. Or correct me if I am wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, I missed the fact the it is executed on a different machine.
test/verify/machineslib.py
Outdated
@@ -528,6 +524,8 @@ def testAddDisk(self): | |||
b.wait_present("#vm-subVmTest1-disks") # wait for the tab | |||
b.click("#vm-subVmTest1-disks") # open the "Disks" subtab | |||
|
|||
time.sleep(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need this change? There is nothing in commit message about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this already, I was debugging something.
access to Libvirtd is wrapped by the <ulink url="https://libvirt.org/virshcmdref.html">virsh</ulink> tool.</para> | ||
access to Libvirtd is wrapped either by the <ulink url="https://libvirt.org/virshcmdref.html">virsh</ulink> tool or | ||
<ulink url="https://libvirt.org/dbus.html">libvirt D-Bus API bindings</ulink>, depending if the latter is installed on | ||
the system. If libvirt-dbus package is not available in your distrubution's software repositories you can follow the instructions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/distrubution/distribution/
@@ -21,6 +25,15 @@ | |||
</para> | |||
</section> | |||
|
|||
<section id="feature-virtualmachines-sessionaccess"> | |||
<title>Managing VMs on session connection</title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Session access is allowed be default, I guess that you mean system access/connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
674214d
to
db27c9a
Compare
The libvirt-dbus API has `StorageVolLookupByPath` but not by target. Populate storage list with paths instead of targets to allow easy usage of the API. Current usage with `virsh undefine` is still going to work with storage paths passed to the `--storage` parameter.
A new provider is going to be introduced in followup patch and we need to clarify from the file name the provider used in each case.
The id parameter is going to be used by followup changes introducing new provider.
Replace parseXML from jquery with DOMParser.
7783c08
to
eb76890
Compare
Related test failures:
There are also some failures in the virsh tests, but these are (presumably) not new. But we shouldn't introduce more flakes, it's hard enough to get the tests to green as it is.. |
eb76890
to
63a3c7c
Compare
Looks like race condition when tests are executed very fast with state not being up to date.. I added some logs in react code and I can't reproduce locally any more.
Not reproducible locally. I tried 10 times with no error :/
This test can't be affected by this PR because the code for CREATE action is shared 1000% between the two providers.
|
Hmkay, retrying the tests a few times. By the way, if you want to add libvirt-dbus to the rhel-x image, now would be a good time for it. I just spent my entire morning yesterday to fix the current regressions, so we got a fresh build yesterday. There's a very high chance that a build today will also succeed :-) |
I can locally reproduce this failure on fedora-29 in about one out of three (parallel) runs. Just keeping the log pointer here for now. |
Reposting with the following diff. (forgot to change some naughy files)
|
Using libvirt D-Bus interface from https://github.com/libvirt/libvirt-dbus/ This commit is rewritting martinpitt@329e623 with the major difference that it's keeping both Libvirt (virsh) and LibvirtDBus providers. The new provider at this point has exactly the same behavior and no extra functionality compared to Libvirt provider. Note: libvirt-dbus should be used only with libvirt version > v4.3.0 or making sure that commit v4.2.0-104-g4d7384eb9d is included. All tests where moved to machineslib.py. Two new files with test classes were created for the two providers, both inheriting from the TestMachines class. Closes cockpit-project#9240
63a3c7c
to
935dedd
Compare
subVmTestCreate15 failure debugging notes:
|
I now added
Update: I got another failed test run without this crash, so it's unrelated. |
Not fully perfect yet, but this looks so much better, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and it only took 165 conversation comments and 59 force-pushes!
🎉
Using libvirt D-Bus interface from https://github.com/libvirt/libvirt-dbus/ This commit is rewritting martinpitt@329e623 with the major difference that it's keeping both Libvirt (virsh) and LibvirtDBus providers. The new provider at this point has exactly the same behavior and no extra functionality compared to Libvirt provider. Note: libvirt-dbus should be used only with libvirt version > v4.3.0 or making sure that commit v4.2.0-104-g4d7384eb9d is included. All tests where moved to machineslib.py. Two new files with test classes were created for the two providers, both inheriting from the TestMachines class. Closes #9240
Using libvirt D-Bus interface from https://github.com/libvirt/libvirt-dbus/ This commit is rewritting martinpitt@329e623 with the major difference that it's keeping both Libvirt (virsh) and LibvirtDBus providers. The new provider at this point has exactly the same behavior and no extra functionality compared to Libvirt provider. Note: libvirt-dbus should be used only with libvirt version > v4.3.0 or making sure that commit v4.2.0-104-g4d7384eb9d is included. All tests where moved to machineslib.py. Two new files with test classes were created for the two providers, both inheriting from the TestMachines class. Closes cockpit-project#9240
This patch series is rewriting martinpitt@329e623 and implements a second provider for Libvirt using Libvirt's D-Bus API (https://github.com/libvirt/libvirt-dbus/).
Since there is a lot of code that can be reused from libvirt.es6 file in the new provider, all reusable functions where moved to libvirt-common.es6 file to avoid code duplicity.
Checklist:
use DBus in(postponed to follow-up)ovirt/provider.es6
if available (so same story as forpkg/machines
)libvirt-dbus | libvirt-client
?