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

Proposed by Newell Jensen on 2018-08-28
Status: Merged
Approved by: Newell Jensen on 2018-08-28
Approved revision: 71ad7c8995b6424af7ac581772b645ad5cc7f965
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~newell-jensen/maas:lp1788910
Merge into: maas:master
Diff against target: 148 lines (+64/-0)
2 files modified
src/provisioningserver/drivers/pod/tests/test_virsh.py (+43/-0)
src/provisioningserver/drivers/pod/virsh.py (+21/-0)
Reviewer Review Type Date Requested Status
MAAS Lander Approve on 2018-08-28
Mike Pontillo (community) 2018-08-28 Approve on 2018-08-28
Review via email: mp+353899@code.launchpad.net

Commit message

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.

To post a comment you must log in.
Mike Pontillo (mpontillo) wrote :

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) wrote :

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

STATUS: SUCCESS
COMMIT: 70a3974108db2c16df51404964b6246190c839b4

review: Approve
~newell-jensen/maas:lp1788910 updated on 2018-08-28
71ad7c8... by Newell Jensen on 2018-08-28

Fix docstring.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/provisioningserver/drivers/pod/tests/test_virsh.py b/src/provisioningserver/drivers/pod/tests/test_virsh.py
2index f6137c8..6651598 100644
3--- a/src/provisioningserver/drivers/pod/tests/test_virsh.py
4+++ b/src/provisioningserver/drivers/pod/tests/test_virsh.py
5@@ -982,6 +982,29 @@ class TestVirshSSH(MAASTestCase):
6 discovered_machine = conn.get_discovered_machine(hostname)
7 self.assertIsNone(discovered_machine)
8
9+ def test_check_machine_can_startup(self):
10+ machine = factory.make_name('machine')
11+ conn = self.configure_virshssh('')
12+ mock_run = self.patch(virsh.VirshSSH, "run")
13+ mock_run.side_effect = ("", "")
14+ conn.check_machine_can_startup(machine)
15+ self.assertThat(
16+ mock_run, MockCallsMatch(
17+ call(['start', '--paused', machine]),
18+ call(['destroy', machine])))
19+
20+ def test_check_machine_can_startup_raises_exception(self):
21+ machine = factory.make_name('machine')
22+ conn = self.configure_virshssh('')
23+ mock_run = self.patch(virsh.VirshSSH, "run")
24+ mock_run.side_effect = ("error: some error", "")
25+ mock_delete_domain = self.patch(virsh.VirshSSH, "delete_domain")
26+ self.assertRaises(
27+ virsh.VirshError, conn.check_machine_can_startup, machine)
28+ self.assertThat(mock_delete_domain, MockCalledOnceWith(machine))
29+ self.assertThat(
30+ mock_run, MockCalledOnceWith(['start', '--paused', machine]))
31+
32 def test_poweron(self):
33 conn = self.configure_virshssh('')
34 expected = conn.poweron(factory.make_name('machine'))
35@@ -1494,6 +1517,8 @@ class TestVirshSSH(MAASTestCase):
36 mock_run = self.patch(virsh.VirshSSH, "run")
37 mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")
38 mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")
39+ mock_check_machine_can_startup = self.patch(
40+ virsh.VirshSSH, "check_machine_can_startup")
41 mock_set_machine_autostart = self.patch(
42 virsh.VirshSSH, "set_machine_autostart")
43 mock_configure_pxe = self.patch(virsh.VirshSSH, "configure_pxe_boot")
44@@ -1516,6 +1541,9 @@ class TestVirshSSH(MAASTestCase):
45 self.assertThat(
46 mock_attach_nic, MockCalledOnceWith(ANY, ANY))
47 self.assertThat(
48+ mock_check_machine_can_startup,
49+ MockCalledOnceWith(request.hostname))
50+ self.assertThat(
51 mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
52 self.assertThat(
53 mock_configure_pxe, MockCalledOnceWith(request.hostname))
54@@ -1552,6 +1580,8 @@ class TestVirshSSH(MAASTestCase):
55 mock_run = self.patch(virsh.VirshSSH, "run")
56 mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")
57 mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")
58+ mock_check_machine_can_startup = self.patch(
59+ virsh.VirshSSH, "check_machine_can_startup")
60 mock_set_machine_autostart = self.patch(
61 virsh.VirshSSH, "set_machine_autostart")
62 mock_configure_pxe = self.patch(virsh.VirshSSH, "configure_pxe_boot")
63@@ -1574,6 +1604,9 @@ class TestVirshSSH(MAASTestCase):
64 self.assertThat(
65 mock_attach_nic, MockCalledOnceWith(ANY, ANY))
66 self.assertThat(
67+ mock_check_machine_can_startup,
68+ MockCalledOnceWith(request.hostname))
69+ self.assertThat(
70 mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
71 self.assertThat(
72 mock_configure_pxe, MockCalledOnceWith(request.hostname))
73@@ -1610,6 +1643,8 @@ class TestVirshSSH(MAASTestCase):
74 mock_run = self.patch(virsh.VirshSSH, "run")
75 mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")
76 mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")
77+ mock_check_machine_can_startup = self.patch(
78+ virsh.VirshSSH, "check_machine_can_startup")
79 mock_set_machine_autostart = self.patch(
80 virsh.VirshSSH, "set_machine_autostart")
81 mock_configure_pxe = self.patch(virsh.VirshSSH, "configure_pxe_boot")
82@@ -1632,6 +1667,9 @@ class TestVirshSSH(MAASTestCase):
83 self.assertThat(
84 mock_attach_nic, MockCalledOnceWith(ANY, ANY))
85 self.assertThat(
86+ mock_check_machine_can_startup,
87+ MockCalledOnceWith(request.hostname))
88+ self.assertThat(
89 mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
90 self.assertThat(
91 mock_configure_pxe, MockCalledOnceWith(request.hostname))
92@@ -1668,6 +1706,8 @@ class TestVirshSSH(MAASTestCase):
93 mock_run = self.patch(virsh.VirshSSH, "run")
94 mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")
95 mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")
96+ mock_check_machine_can_startup = self.patch(
97+ virsh.VirshSSH, "check_machine_can_startup")
98 mock_set_machine_autostart = self.patch(
99 virsh.VirshSSH, "set_machine_autostart")
100 mock_configure_pxe = self.patch(virsh.VirshSSH, "configure_pxe_boot")
101@@ -1690,6 +1730,9 @@ class TestVirshSSH(MAASTestCase):
102 self.assertThat(
103 mock_attach_nic, MockCalledOnceWith(ANY, ANY))
104 self.assertThat(
105+ mock_check_machine_can_startup,
106+ MockCalledOnceWith(request.hostname))
107+ self.assertThat(
108 mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
109 self.assertThat(
110 mock_configure_pxe, MockCalledOnceWith(request.hostname))
111diff --git a/src/provisioningserver/drivers/pod/virsh.py b/src/provisioningserver/drivers/pod/virsh.py
112index 629c2d8..451b38a 100644
113--- a/src/provisioningserver/drivers/pod/virsh.py
114+++ b/src/provisioningserver/drivers/pod/virsh.py
115@@ -717,6 +717,24 @@ class VirshSSH(pexpect.spawn):
116 discovered_machine.interfaces = interfaces
117 return discovered_machine
118
119+ def check_machine_can_startup(self, machine):
120+ """Check the machine for any startup errors
121+ after the domain is created in virsh.
122+
123+ If an error is reported, delete the domain and raise an exception.
124+ If no error is reported, destroy the domain to put it back into a
125+ 'shut off' state.
126+ """
127+ output = self.run(['start', '--paused', machine]).strip()
128+ if output.startswith("error:"):
129+ # Delete the domain.
130+ self.delete_domain(machine)
131+ # Raise the error.
132+ raise VirshError("Unable to compose %s: %s" % (machine, output))
133+ else:
134+ # No errors, so set machine back to 'shut off' state.
135+ self.run(['destroy', machine])
136+
137 def set_machine_autostart(self, machine):
138 """Set machine to autostart."""
139 output = self.run(['autostart', machine]).strip()
140@@ -1044,6 +1062,9 @@ class VirshSSH(pexpect.spawn):
141 self.attach_local_volume(
142 request.hostname, pool, volume, block_name)
143
144+ # Check machine for any startup errors.
145+ self.check_machine_can_startup(request.hostname)
146+
147 # Set machine to autostart.
148 self.set_machine_autostart(request.hostname)
149

Subscribers

People subscribed via source and target branches