Merge lp:~newell-jensen/maas/fix-1690781 into lp:~maas-committers/maas/trunk

[Newell Jensen]

Raises VirshError if the `virsh domcapabilities --virttype kvm|qemu` command returns empty XML with a suggestion on how to potentially fix the situation.

[Newell Jensen (newell-jensen)]

I was unable to reproduce this bug following the steps outlined in the bug report. Regardless of this fact, it is a good idea to test to see whether or not the XML is empty from running `virsh domcapabilities --virttype kvm|qemu` as a precaution since the bug reporters has run into this.

[Данило Шеган (danilo)]

It's really weird that you couldn't reproduce this.

If qemu-system-x86 package is not installed, there should be no /usr/bin/qemu-system-x86_64 emulator binary, and that's what domcapabilities uses to check for domain capabilities (or whatever architecture you've got if it's not x86_64).

I'll confirm this fixes it, though I fully suspect it does.

I'd also prefer a fix were self.run() raises an exception when the return code is not 0, but that would probably be a much bigger change that's not suitable for upcoming 2.2.

(I've added another review slot for myself since Andres mentioned that he wants others to confirm our reviews too)

[Данило Шеган (danilo)]

Unfortunately, this doesn't fix the problem: some parts of my analysis were incorrect.

Virsh driver is actually running virsh and then running individual commands in there, so the output is always the full error output and not just a newline (iow, it does "virsh qemu:..." and then runs individual commands like "domcapabilities" there). So it still fails with the same error.

This means that we can't rely on virsh exit code either, so it seems the best way forward is to catch the XML parsing exception, and then fail with the error you added.

As for reproducing it, note that the error I get is:

  error: failed to get emulator capabilities
  error: invalid argument: unable to find any emulator to serve 'x86_64' architecture

Thus, when an emulator binary is missing (eg. qemu-system-x86 package not installed which provides /usr/bin/qemu-system-x86_64), I don't see how you could not get this error message. Make sure to restart libvirt-bin service after you remove the package, though, because libvirtd seems to cache the result.

Perhaps we can match on "error:" lines to find the error output.

  ubuntu@skrinja:~$ LC_ALL=en_US.UTF-8 virsh
  Welcome to virsh, the virtualization interactive terminal.

  Type: 'help' for help with commands
         'quit' to quit

  virsh # domcapabilities
  error: failed to get emulator capabilities
  error: invalid argument: unable to find any emulator to serve 'x86_64' architecture

  virsh # not-a-command
  error: unknown command: 'not-a-command'
  virsh #

(note how all the errors are preceded with "error" in an en_US.UTF-8 locale, so maybe it's best to make run() raise a VirshError exception if it sees lines starting with "error:" in the output)

review: Needs Fixing

[Newell Jensen (newell-jensen)]

I was able to reproduce the bug this time and have updated the branch to test that the output starts with 'error'.

[Данило Шеган (danilo)]

I would still prefer if we catch the etree.XMLSyntaxError in etree.XML(xml) call, and if we include the full virsh call output in the error (for easier debugging), but this is definitely good enough.

Thanks for bearing with me.

review: Approve

=== modified file 'src/provisioningserver/drivers/pod/tests/test_virsh.py'
--- src/provisioningserver/drivers/pod/tests/test_virsh.py	2017-05-17 12:08:54 +0000
+++ src/provisioningserver/drivers/pod/tests/test_virsh.py	2017-05-19 03:57:28 +0000
@@ -890,6 +890,10 @@
             'emulator': '/usr/bin/qemu-system-x86_64',
         }, conn.get_domain_capabilities())
 
+    def test_get_domain_capabilities_raises_error(self):
+        conn = self.configure_virshssh('error: some error')
+        self.assertRaises(virsh.VirshError, conn.get_domain_capabilities)
+
     def test_cleanup_disks_deletes_all(self):
         conn = self.configure_virshssh('')
         volumes = [

=== modified file 'src/provisioningserver/drivers/pod/virsh.py'
--- src/provisioningserver/drivers/pod/virsh.py	2017-05-17 12:08:54 +0000
+++ src/provisioningserver/drivers/pod/virsh.py	2017-05-19 03:57:28 +0000
@@ -645,6 +645,16 @@
             # Fallback to qemu support. Fail if qemu not supported.
             xml = self.run(['domcapabilities', '--virttype', 'qemu'])
             emulator_type = 'qemu'
+
+        # XXX newell 2017-05-18 bug=1690781
+        # Check to see if the XML output was an error.
+        # See bug for details about why and how this can occur.
+        if xml.startswith('error'):
+            raise VirshError(
+                "`virsh domcapabilities --virttype %s` errored.  Please "
+                "verify that package qemu-kvm is installed and restart "
+                "libvirt-bin service." % emulator_type)
+
         doc = etree.XML(xml)
         evaluator = etree.XPathEvaluator(doc)
         emulator = evaluator('/domainCapabilities/path')[0].text