Merge ~newell-jensen/maas:lp1788910 into maas:master

[Newell Jensen]

LP: #1788910 -- check that a newly composed vm can startup properly. If it can't, it is deleted, otherwise, it is turned off and composition can proceed.

[Mike Pontillo (mpontillo)]

Thanks for the fix. This is a nice improvement that could save people a lot of time when composing machines. If an error occurs while Juju is attempting to allocate, this will also fail much faster, which is great.

I've got one comment below about the docstring, but I won't block you on it.

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1788910 lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 70a3974108db2c16df51404964b6246190c839b4

review: Approve

71ad7c8... by Newell Jensen on 2018-08-28

Fix docstring.

diff --git a/src/provisioningserver/drivers/pod/tests/test_virsh.py b/src/provisioningserver/drivers/pod/tests/test_virsh.py
index f6137c8..6651598 100644
--- a/src/provisioningserver/drivers/pod/tests/test_virsh.py
+++ b/src/provisioningserver/drivers/pod/tests/test_virsh.py
@@ -982,6 +982,29 @@ class TestVirshSSH(MAASTestCase):
         discovered_machine = conn.get_discovered_machine(hostname)
         self.assertIsNone(discovered_machine)
 
+    def test_check_machine_can_startup(self):
+        machine = factory.make_name('machine')
+        conn = self.configure_virshssh('')
+        mock_run = self.patch(virsh.VirshSSH, "run")
+        mock_run.side_effect = ("", "")
+        conn.check_machine_can_startup(machine)
+        self.assertThat(
+            mock_run, MockCallsMatch(
+                call(['start', '--paused', machine]),
+                call(['destroy', machine])))
+
+    def test_check_machine_can_startup_raises_exception(self):
+        machine = factory.make_name('machine')
+        conn = self.configure_virshssh('')
+        mock_run = self.patch(virsh.VirshSSH, "run")
+        mock_run.side_effect = ("error: some error", "")
+        mock_delete_domain = self.patch(virsh.VirshSSH, "delete_domain")
+        self.assertRaises(
+            virsh.VirshError, conn.check_machine_can_startup, machine)
+        self.assertThat(mock_delete_domain, MockCalledOnceWith(machine))
+        self.assertThat(
+            mock_run, MockCalledOnceWith(['start', '--paused', machine]))
+
     def test_poweron(self):
         conn = self.configure_virshssh('')
         expected = conn.poweron(factory.make_name('machine'))
@@ -1494,6 +1517,8 @@ class TestVirshSSH(MAASTestCase):
         mock_run = self.patch(virsh.VirshSSH, "run")
         mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")
         mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")
+        mock_check_machine_can_startup = self.patch(
+            virsh.VirshSSH, "check_machine_can_startup")
         mock_set_machine_autostart = self.patch(
             virsh.VirshSSH, "set_machine_autostart")
         mock_configure_pxe = self.patch(virsh.VirshSSH, "configure_pxe_boot")
@@ -1516,6 +1541,9 @@ class TestVirshSSH(MAASTestCase):
         self.assertThat(
             mock_attach_nic, MockCalledOnceWith(ANY, ANY))
         self.assertThat(
+            mock_check_machine_can_startup,
+            MockCalledOnceWith(request.hostname))
+        self.assertThat(
             mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
         self.assertThat(
             mock_configure_pxe, MockCalledOnceWith(request.hostname))
@@ -1552,6 +1580,8 @@ class TestVirshSSH(MAASTestCase):
         mock_run = self.patch(virsh.VirshSSH, "run")
         mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")
         mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")
+        mock_check_machine_can_startup = self.patch(
+            virsh.VirshSSH, "check_machine_can_startup")
         mock_set_machine_autostart = self.patch(
             virsh.VirshSSH, "set_machine_autostart")
         mock_configure_pxe = self.patch(virsh.VirshSSH, "configure_pxe_boot")
@@ -1574,6 +1604,9 @@ class TestVirshSSH(MAASTestCase):
         self.assertThat(
             mock_attach_nic, MockCalledOnceWith(ANY, ANY))
         self.assertThat(
+            mock_check_machine_can_startup,
+            MockCalledOnceWith(request.hostname))
+        self.assertThat(
             mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
         self.assertThat(
             mock_configure_pxe, MockCalledOnceWith(request.hostname))
@@ -1610,6 +1643,8 @@ class TestVirshSSH(MAASTestCase):
         mock_run = self.patch(virsh.VirshSSH, "run")
         mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")
         mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")
+        mock_check_machine_can_startup = self.patch(
+            virsh.VirshSSH, "check_machine_can_startup")
         mock_set_machine_autostart = self.patch(
             virsh.VirshSSH, "set_machine_autostart")
         mock_configure_pxe = self.patch(virsh.VirshSSH, "configure_pxe_boot")
@@ -1632,6 +1667,9 @@ class TestVirshSSH(MAASTestCase):
         self.assertThat(
             mock_attach_nic, MockCalledOnceWith(ANY, ANY))
         self.assertThat(
+            mock_check_machine_can_startup,
+            MockCalledOnceWith(request.hostname))
+        self.assertThat(
             mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
         self.assertThat(
             mock_configure_pxe, MockCalledOnceWith(request.hostname))
@@ -1668,6 +1706,8 @@ class TestVirshSSH(MAASTestCase):
         mock_run = self.patch(virsh.VirshSSH, "run")
         mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")
         mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")
+        mock_check_machine_can_startup = self.patch(
+            virsh.VirshSSH, "check_machine_can_startup")
         mock_set_machine_autostart = self.patch(
             virsh.VirshSSH, "set_machine_autostart")
         mock_configure_pxe = self.patch(virsh.VirshSSH, "configure_pxe_boot")
@@ -1690,6 +1730,9 @@ class TestVirshSSH(MAASTestCase):
         self.assertThat(
             mock_attach_nic, MockCalledOnceWith(ANY, ANY))
         self.assertThat(
+            mock_check_machine_can_startup,
+            MockCalledOnceWith(request.hostname))
+        self.assertThat(
             mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
         self.assertThat(
             mock_configure_pxe, MockCalledOnceWith(request.hostname))
diff --git a/src/provisioningserver/drivers/pod/virsh.py b/src/provisioningserver/drivers/pod/virsh.py
index 629c2d8..451b38a 100644
--- a/src/provisioningserver/drivers/pod/virsh.py
+++ b/src/provisioningserver/drivers/pod/virsh.py
@@ -717,6 +717,24 @@ class VirshSSH(pexpect.spawn):
         discovered_machine.interfaces = interfaces
         return discovered_machine
 
+    def check_machine_can_startup(self, machine):
+        """Check the machine for any startup errors
+        after the domain is created in virsh.
+
+        If an error is reported, delete the domain and raise an exception.
+        If no error is reported, destroy the domain to put it back into a
+        'shut off' state.
+        """
+        output = self.run(['start', '--paused', machine]).strip()
+        if output.startswith("error:"):
+            # Delete the domain.
+            self.delete_domain(machine)
+            # Raise the error.
+            raise VirshError("Unable to compose %s: %s" % (machine, output))
+        else:
+            # No errors, so set machine back to 'shut off' state.
+            self.run(['destroy', machine])
+
     def set_machine_autostart(self, machine):
         """Set machine to autostart."""
         output = self.run(['autostart', machine]).strip()
@@ -1044,6 +1062,9 @@ class VirshSSH(pexpect.spawn):
             self.attach_local_volume(
                 request.hostname, pool, volume, block_name)
 
+        # Check machine for any startup errors.
+        self.check_machine_can_startup(request.hostname)
+
         # Set machine to autostart.
         self.set_machine_autostart(request.hostname)