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

Proposed by Newell Jensen on 2018-08-24
Status: Merged
Approved by: Newell Jensen on 2018-08-25
Approved revision: 19d0b693523255c2702859ba62c24269acfdd933
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~newell-jensen/maas:lp1788756
Merge into: maas:master
Diff against target: 236 lines (+54/-26)
2 files modified
src/provisioningserver/drivers/pod/tests/test_virsh.py (+40/-15)
src/provisioningserver/drivers/pod/virsh.py (+14/-11)
Reviewer Review Type Date Requested Status
Mike Pontillo (community) 2018-08-24 Approve on 2018-08-25
MAAS Lander Approve on 2018-08-24
Review via email: mp+353741@code.launchpad.net

Commit message

LP: #1788756 -- Only raise no network error for network attached or non-specified interfaces.

To post a comment you must log in.
MAAS Lander (maas-lander) wrote :

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

STATUS: SUCCESS
COMMIT: 19d0b693523255c2702859ba62c24269acfdd933

review: Approve
Mike Pontillo (mpontillo) wrote :

Looks good; one question inline below that I won't block you on. (I think the same issue happens in the block devices loop, too.)

review: Approve
Newell Jensen (newell-jensen) wrote :

> Looks good; one question inline below that I won't block you on. (I think the
> same issue happens in the block devices loop, too.)

All the block devices are created first (see top part of the create_domain method in provisioningserver.drivers.pod.virsh.py) and then attached after the domain has been defined within virsh. Good question though.

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 ff616c7..f6137c8 100644
3--- a/src/provisioningserver/drivers/pod/tests/test_virsh.py
4+++ b/src/provisioningserver/drivers/pod/tests/test_virsh.py
5@@ -1256,11 +1256,12 @@ class TestVirshSSH(MAASTestCase):
6 conn = self.configure_virshssh('')
7 domain = factory.make_name('domain')
8 network = factory.make_name('network')
9+ self.patch(virsh.VirshSSH, "get_best_network").return_value = network
10 mock_run = self.patch(virsh.VirshSSH, "run")
11 fake_mac = factory.make_mac_address()
12 interface = RequestedMachineInterface()
13 self.patch(virsh, 'generate_mac_address').return_value = fake_mac
14- conn.attach_interface(interface, domain, network)
15+ conn.attach_interface(interface, domain)
16 self.assertThat(mock_run, MockCalledOnceWith([
17 'attach-interface', domain, 'network', network,
18 '--mac', fake_mac,
19@@ -1270,12 +1271,13 @@ class TestVirshSSH(MAASTestCase):
20 conn = self.configure_virshssh('')
21 domain = factory.make_name('domain')
22 network = factory.make_name('network')
23+ self.patch(virsh.VirshSSH, "get_best_network").return_value = network
24 mock_run = self.patch(virsh.VirshSSH, "run")
25 fake_mac = factory.make_mac_address()
26 interface = RequestedMachineInterface(
27 attach_name=network, attach_type='network')
28 self.patch(virsh, 'generate_mac_address').return_value = fake_mac
29- conn.attach_interface(interface, domain, network)
30+ conn.attach_interface(interface, domain)
31 self.assertThat(mock_run, MockCalledOnceWith([
32 'attach-interface', domain, 'network', network,
33 '--mac', fake_mac,
34@@ -1284,7 +1286,6 @@ class TestVirshSSH(MAASTestCase):
35 def test_attach_interface_attaches_macvlan(self):
36 conn = self.configure_virshssh('')
37 domain = factory.make_name('domain')
38- network = factory.make_name('network')
39 mock_run = self.patch(virsh.VirshSSH, "run")
40 fake_mac = factory.make_mac_address()
41 interface = RequestedMachineInterface(
42@@ -1296,7 +1297,7 @@ class TestVirshSSH(MAASTestCase):
43 tmpfile = NamedTemporaryFile.return_value
44 tmpfile.__enter__.return_value = tmpfile
45 tmpfile.name = factory.make_name("filename")
46- conn.attach_interface(interface, domain, network)
47+ conn.attach_interface(interface, domain)
48
49 device_params = {
50 'mac_address': fake_mac,
51@@ -1318,7 +1319,6 @@ class TestVirshSSH(MAASTestCase):
52 def test_attach_interface_attaches_bridge(self):
53 conn = self.configure_virshssh('')
54 domain = factory.make_name('domain')
55- network = factory.make_name('network')
56 mock_run = self.patch(virsh.VirshSSH, "run")
57 fake_mac = factory.make_mac_address()
58 interface = RequestedMachineInterface(
59@@ -1330,7 +1330,7 @@ class TestVirshSSH(MAASTestCase):
60 tmpfile = NamedTemporaryFile.return_value
61 tmpfile.__enter__.return_value = tmpfile
62 tmpfile.name = factory.make_name("filename")
63- conn.attach_interface(interface, domain, network)
64+ conn.attach_interface(interface, domain)
65
66 device_params = {
67 'mac_address': fake_mac,
68@@ -1434,9 +1434,38 @@ class TestVirshSSH(MAASTestCase):
69 request = make_requested_machine()
70 self.patch(virsh.VirshSSH, "create_local_volume").return_value = None
71 error = self.assertRaises(
72- PodInvalidResources, conn.create_domain, request, )
73+ PodInvalidResources, conn.create_domain, request)
74 self.assertEqual("not enough space for disk 0.", str(error))
75
76+ def test_create_domain_handles_no_network_for_requested_interface(self):
77+ conn = self.configure_virshssh('')
78+ request = make_requested_machine()
79+ request.block_devices = request.block_devices[:1]
80+ request.interfaces = request.interfaces[:1]
81+ disk_info = (factory.make_name('pool'), factory.make_name('vol'))
82+ domain_params = {
83+ "type": "kvm",
84+ "emulator": "/usr/bin/qemu-system-x86_64",
85+ }
86+ self.patch(
87+ virsh.VirshSSH, "create_local_volume").return_value = disk_info
88+ self.patch(virsh.VirshSSH, "get_domain_capabilities").return_value = (
89+ domain_params)
90+ mock_uuid = self.patch(virsh_module, "uuid4")
91+ mock_uuid.return_value = str(uuid4())
92+ domain_params['name'] = request.hostname
93+ domain_params['uuid'] = mock_uuid.return_value
94+ domain_params['arch'] = ARCH_FIX_REVERSE[request.architecture]
95+ domain_params['cores'] = str(request.cores)
96+ domain_params['memory'] = str(request.memory)
97+ NamedTemporaryFile = self.patch(virsh_module, "NamedTemporaryFile")
98+ tmpfile = NamedTemporaryFile.return_value
99+ tmpfile.__enter__.return_value = tmpfile
100+ tmpfile.name = factory.make_name("filename")
101+ self.patch(virsh.VirshSSH, "run")
102+ self.patch(virsh.VirshSSH, "get_network_list").return_value = []
103+ self.assertRaises(PodInvalidResources, conn.create_domain, request)
104+
105 def test_create_domain_calls_correct_methods_with_amd64_arch(self):
106 conn = self.configure_virshssh('')
107 request = make_requested_machine()
108@@ -1462,7 +1491,6 @@ class TestVirshSSH(MAASTestCase):
109 tmpfile = NamedTemporaryFile.return_value
110 tmpfile.__enter__.return_value = tmpfile
111 tmpfile.name = factory.make_name("filename")
112- self.patch(virsh.VirshSSH, "get_best_network").return_value = "maas"
113 mock_run = self.patch(virsh.VirshSSH, "run")
114 mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")
115 mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")
116@@ -1486,7 +1514,7 @@ class TestVirshSSH(MAASTestCase):
117 mock_attach_disk,
118 MockCalledOnceWith(ANY, disk_info[0], disk_info[1], 'vda'))
119 self.assertThat(
120- mock_attach_nic, MockCalledOnceWith(ANY, ANY, 'maas'))
121+ mock_attach_nic, MockCalledOnceWith(ANY, ANY))
122 self.assertThat(
123 mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
124 self.assertThat(
125@@ -1521,7 +1549,6 @@ class TestVirshSSH(MAASTestCase):
126 tmpfile = NamedTemporaryFile.return_value
127 tmpfile.__enter__.return_value = tmpfile
128 tmpfile.name = factory.make_name("filename")
129- self.patch(virsh.VirshSSH, "get_best_network").return_value = "maas"
130 mock_run = self.patch(virsh.VirshSSH, "run")
131 mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")
132 mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")
133@@ -1545,7 +1572,7 @@ class TestVirshSSH(MAASTestCase):
134 mock_attach_disk,
135 MockCalledOnceWith(ANY, disk_info[0], disk_info[1], 'vda'))
136 self.assertThat(
137- mock_attach_nic, MockCalledOnceWith(ANY, ANY, 'maas'))
138+ mock_attach_nic, MockCalledOnceWith(ANY, ANY))
139 self.assertThat(
140 mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
141 self.assertThat(
142@@ -1580,7 +1607,6 @@ class TestVirshSSH(MAASTestCase):
143 tmpfile = NamedTemporaryFile.return_value
144 tmpfile.__enter__.return_value = tmpfile
145 tmpfile.name = factory.make_name("filename")
146- self.patch(virsh.VirshSSH, "get_best_network").return_value = "maas"
147 mock_run = self.patch(virsh.VirshSSH, "run")
148 mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")
149 mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")
150@@ -1604,7 +1630,7 @@ class TestVirshSSH(MAASTestCase):
151 mock_attach_disk,
152 MockCalledOnceWith(ANY, disk_info[0], disk_info[1], 'vda'))
153 self.assertThat(
154- mock_attach_nic, MockCalledOnceWith(ANY, ANY, 'maas'))
155+ mock_attach_nic, MockCalledOnceWith(ANY, ANY))
156 self.assertThat(
157 mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
158 self.assertThat(
159@@ -1639,7 +1665,6 @@ class TestVirshSSH(MAASTestCase):
160 tmpfile = NamedTemporaryFile.return_value
161 tmpfile.__enter__.return_value = tmpfile
162 tmpfile.name = factory.make_name("filename")
163- self.patch(virsh.VirshSSH, "get_best_network").return_value = "maas"
164 mock_run = self.patch(virsh.VirshSSH, "run")
165 mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")
166 mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")
167@@ -1663,7 +1688,7 @@ class TestVirshSSH(MAASTestCase):
168 mock_attach_disk,
169 MockCalledOnceWith(ANY, disk_info[0], disk_info[1], 'vda'))
170 self.assertThat(
171- mock_attach_nic, MockCalledOnceWith(ANY, ANY, 'maas'))
172+ mock_attach_nic, MockCalledOnceWith(ANY, ANY))
173 self.assertThat(
174 mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
175 self.assertThat(
176diff --git a/src/provisioningserver/drivers/pod/virsh.py b/src/provisioningserver/drivers/pod/virsh.py
177index f04fb9c..629c2d8 100644
178--- a/src/provisioningserver/drivers/pod/virsh.py
179+++ b/src/provisioningserver/drivers/pod/virsh.py
180@@ -871,13 +871,14 @@ class VirshSSH(pexpect.spawn):
181
182 return networks[0]
183
184- def attach_interface(self, interface, domain, network):
185+ def attach_interface(self, interface, domain):
186 """Attach new network interface on `domain` to `network`."""
187 mac = generate_mac_address()
188 # If attachment type is not specified, default to network.
189 if interface.attach_type in (None, InterfaceAttachType.NETWORK):
190 # Set the network if we are explicity attaching a network
191 # specified by the user.
192+ network = self.get_best_network()
193 if interface.attach_type is not None:
194 network = interface.attach_name
195 return self.run([
196@@ -977,11 +978,7 @@ class VirshSSH(pexpect.spawn):
197 return "vd" + name
198
199 def create_domain(self, request, default_pool=None):
200- """Create a domain based on the `request` with hostname.
201-
202- For now this just uses `get_best_network` to connect the interfaces
203- of the domain to the network.
204- """
205+ """Create a domain based on the `request` with hostname."""
206 # Create all the block devices first. If cannot complete successfully
207 # then fail early. The driver currently doesn't do any tag matching
208 # for block devices, and is not really required for Virsh.
209@@ -1030,17 +1027,23 @@ class VirshSSH(pexpect.spawn):
210 f.flush()
211 self.run(['define', f.name])
212
213+ # Attach the requested interfaces.
214+ for interface in request.interfaces:
215+ try:
216+ self.attach_interface(interface, request.hostname)
217+ except PodInvalidResources as error:
218+ # Delete the defined domain in virsh.
219+ self.run(['destroy', request.hostname])
220+ self.run(['undefine', request.hostname])
221+ # Re-raise the exception.
222+ raise error
223+
224 # Attach the created disks in order.
225 for idx, (pool, volume) in enumerate(created_disks):
226 block_name = self.get_block_name_from_idx(idx)
227 self.attach_local_volume(
228 request.hostname, pool, volume, block_name)
229
230- # Attach new interfaces to the best possible network.
231- best_network = self.get_best_network()
232- for interface in request.interfaces:
233- self.attach_interface(interface, request.hostname, best_network)
234-
235 # Set machine to autostart.
236 self.set_machine_autostart(request.hostname)
237

Subscribers

People subscribed via source and target branches