MAAS

Merge ~ack/maas:1869300-3.0 into maas:3.0

  1. Git
  2. lp:~ack/maas
  3. 1869300-3.0
  4. Merge into 3.0
Proposed by Alberto Donato
Status: Merged
Approved by: Alberto Donato
Approved revision: 0ab0c4921cff1ba4b5b12fdfc352bde78807c314
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~ack/maas:1869300-3.0
Merge into: maas:3.0
Diff against target: 401 lines (+120/-113)
2 files modified
src/provisioningserver/drivers/pod/tests/test_virsh.py (+85/-70)
src/provisioningserver/drivers/pod/virsh.py (+35/-43)
Reviewer Review Type Date Requested Status
MAAS Lander Approve
Alberto Donato (community) Approve
Review via email: mp+404881@code.launchpad.net

Commit message

refactor VirshSSH.create_local_volume to reduce duplication in command line

This also remove useless passing of --format raw where it's ignored

To post a comment you must log in.
Revision history for this message
Alberto Donato (ack) :
review: Approve
Revision history for this message
MAAS Lander (maas-lander) wrote :

LANDING
-b 1869300-3.0 lp:~ack/maas/+git/maas into -b 3.0 lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10345/consoleText

Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b 1869300-3.0 lp:~ack/maas/+git/maas into -b 3.0 lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10346/console
COMMIT: 9d417c5f81c31536f2a6dc9003fe5458da230c1c

review: Needs Fixing
Revision history for this message
MAAS Lander (maas-lander) wrote :

LANDING
-b 1869300-3.0 lp:~ack/maas/+git/maas into -b 3.0 lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10348/consoleText

Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b 1869300-3.0 lp:~ack/maas/+git/maas into -b 3.0 lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 0ab0c4921cff1ba4b5b12fdfc352bde78807c314

review: Approve

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 8ef80de..9fb3274 100644
3--- a/src/provisioningserver/drivers/pod/tests/test_virsh.py
4+++ b/src/provisioningserver/drivers/pod/tests/test_virsh.py
5@@ -424,6 +424,7 @@ class VirshRunFake:
6 def add_pool(
7 self,
8 name,
9+ pool_type="dir",
10 active=True,
11 autostart=True,
12 pool_uuid=None,
13@@ -442,25 +443,27 @@ class VirshRunFake:
14 available = capacity - allocation
15 if path is None:
16 path = "var/lib/virtlib/" + factory.make_name("images")
17- self.pools.append(
18- {
19- "name": name,
20- "active": active,
21- "autostart": autostart,
22- "uuid": pool_uuid,
23- "capacity": capacity,
24- "allocation": allocation,
25- "available": available,
26- "path": path,
27- }
28- )
29+ pool = {
30+ "name": name,
31+ "type": pool_type,
32+ "active": active,
33+ "autostart": autostart,
34+ "uuid": pool_uuid,
35+ "capacity": capacity,
36+ "allocation": allocation,
37+ "available": available,
38+ "path": path,
39+ }
40+ self.pools.append(pool)
41+ return pool
42
43 def __call__(self, args):
44 command = args.pop(0)
45 func = getattr(self, "cmd_" + command.replace("-", "_"))
46 return func(*args)
47
48- def cmd_pool_list(self):
49+ def cmd_pool_list(self, _, pool_types):
50+ filter_types = pool_types.split(",")
51 template = " {name: <21}{state: <11}{autostart}"
52 lines = [
53 template.format(name="Name", state="State", autostart="Autostart")
54@@ -473,6 +476,7 @@ class VirshRunFake:
55 autostart="yes" if pool["autostart"] else "no",
56 )
57 for pool in self.pools
58+ if pool["type"] in filter_types
59 )
60 return "\n".join(lines)
61
62@@ -834,6 +838,29 @@ class TestVirshSSH(MAASTestCase):
63 ]
64 self.assertEqual(expected, conn.get_pod_storage_pools())
65
66+ def test_get_pod_storage_pools_filters_supported(self):
67+ conn = virsh.VirshSSH()
68+ fake_runner = VirshRunFake()
69+ valid_pools = [
70+ fake_runner.add_pool(factory.make_name("pool")) for _ in range(3)
71+ ]
72+ # extra pool of unsupported type is not returned
73+ fake_runner.add_pool(factory.make_name("pool"), pool_type="disk")
74+ self.patch(virsh.VirshSSH, "run").side_effect = fake_runner
75+ self.assertEqual(
76+ conn.get_pod_storage_pools(),
77+ [
78+ DiscoveredPodStoragePool(
79+ id=pool["uuid"],
80+ type="dir",
81+ name=pool["name"],
82+ storage=pool["capacity"],
83+ path=pool["path"],
84+ )
85+ for pool in valid_pools
86+ ],
87+ )
88+
89 def test_get_pod_storage_pools_no_pool(self):
90 conn = self.configure_virshssh(SAMPLE_POOLINFO)
91 pools_mock = self.patch(virsh.VirshSSH, "list_pools")
92@@ -1531,7 +1558,9 @@ class TestVirshSSH(MAASTestCase):
93 None,
94 None,
95 )
96- self.assertIsNone(conn.create_local_volume(random.randint(1000, 2000)))
97+ self.assertIsNone(
98+ conn._create_local_volume(random.randint(1000, 2000))
99+ )
100
101 def test_create_local_volume_returns_tagged_pool_and_volume(self):
102 conn = self.configure_virshssh("")
103@@ -1540,36 +1569,32 @@ class TestVirshSSH(MAASTestCase):
104 mock_run = self.patch(virsh.VirshSSH, "run")
105 mock_run.side_effect = (SAMPLE_POOLINFO_FULL, SAMPLE_POOLINFO, None)
106 disk = RequestedMachineBlockDevice(size=4096, tags=tagged_pools)
107- used_pool, _ = conn.create_local_volume(disk)
108+ used_pool, _ = conn._create_local_volume(disk)
109 self.assertEqual(tagged_pools[1], used_pool)
110
111- def test_create_local_volume_makes_call_returns_pool_and_volume(self):
112+ def test_create_local_volume_makes_call_returns_pool_and_volume_dir(self):
113 conn = self.configure_virshssh("")
114 pool = factory.make_name("pool")
115- pool_type = factory.make_name("pool_type")
116 self.patch(virsh.VirshSSH, "get_usable_pool").return_value = (
117- pool_type,
118+ "dir",
119 pool,
120 )
121- mock_run = self.patch(virsh.VirshSSH, "run")
122+ self.patch(virsh.VirshSSH, "run")
123 disk = RequestedMachineBlockDevice(
124 size=random.randint(1000, 2000), tags=[]
125 )
126- used_pool, volume_name = conn.create_local_volume(disk)
127- self.assertThat(
128- mock_run,
129- MockCalledOnceWith(
130- [
131- "vol-create-as",
132- used_pool,
133- volume_name,
134- str(disk.size),
135- "--allocation",
136- "0",
137- "--format",
138- "raw",
139- ]
140- ),
141+ used_pool, volume_name = conn._create_local_volume(disk)
142+ conn.run.assert_called_once_with(
143+ [
144+ "vol-create-as",
145+ used_pool,
146+ volume_name,
147+ str(disk.size),
148+ "--allocation",
149+ "0",
150+ "--format",
151+ "raw",
152+ ]
153 )
154 self.assertEqual(pool, used_pool)
155 self.assertIsNotNone(volume_name)
156@@ -1581,23 +1606,18 @@ class TestVirshSSH(MAASTestCase):
157 "logical",
158 pool,
159 )
160- mock_run = self.patch(virsh.VirshSSH, "run")
161+ self.patch(virsh.VirshSSH, "run")
162 disk = RequestedMachineBlockDevice(
163 size=random.randint(1000, 2000), tags=[]
164 )
165- used_pool, volume_name = conn.create_local_volume(disk)
166- self.assertThat(
167- mock_run,
168- MockCalledOnceWith(
169- [
170- "vol-create-as",
171- used_pool,
172- volume_name,
173- str(disk.size),
174- "--format",
175- "raw",
176- ]
177- ),
178+ used_pool, volume_name = conn._create_local_volume(disk)
179+ conn.run.assert_called_once_with(
180+ [
181+ "vol-create-as",
182+ used_pool,
183+ volume_name,
184+ str(disk.size),
185+ ]
186 )
187 self.assertEqual(pool, used_pool)
188 self.assertIsNotNone(volume_name)
189@@ -1609,26 +1629,21 @@ class TestVirshSSH(MAASTestCase):
190 "zfs",
191 pool,
192 )
193- mock_run = self.patch(virsh.VirshSSH, "run")
194+ self.patch(virsh.VirshSSH, "run")
195 disk = RequestedMachineBlockDevice(
196 size=random.randint(1000, 2000), tags=[]
197 )
198- used_pool, volume_name = conn.create_local_volume(disk)
199+ used_pool, volume_name = conn._create_local_volume(disk)
200 size = int(floor(disk.size / 2 ** 20)) * 2 ** 20
201- self.assertThat(
202- mock_run,
203- MockCalledOnceWith(
204- [
205- "vol-create-as",
206- used_pool,
207- volume_name,
208- str(size),
209- "--allocation",
210- "0",
211- "--format",
212- "raw",
213- ]
214- ),
215+ conn.run.assert_called_once_with(
216+ [
217+ "vol-create-as",
218+ used_pool,
219+ volume_name,
220+ str(size),
221+ "--allocation",
222+ "0",
223+ ]
224 )
225 self.assertEqual(pool, used_pool)
226 self.assertIsNotNone(volume_name)
227@@ -2088,7 +2103,7 @@ class TestVirshSSH(MAASTestCase):
228 factory.make_name("pool"),
229 factory.make_name("vol"),
230 )
231- self.patch(virsh.VirshSSH, "create_local_volume").side_effect = [
232+ self.patch(virsh.VirshSSH, "_create_local_volume").side_effect = [
233 (first_pool, first_vol),
234 exception,
235 ]
236@@ -2102,7 +2117,7 @@ class TestVirshSSH(MAASTestCase):
237 def test_create_domain_handles_no_space(self):
238 conn = self.configure_virshssh("")
239 request = make_requested_machine()
240- self.patch(virsh.VirshSSH, "create_local_volume").return_value = None
241+ self.patch(virsh.VirshSSH, "_create_local_volume").return_value = None
242 error = self.assertRaises(
243 PodInvalidResources, conn.create_domain, request
244 )
245@@ -2119,7 +2134,7 @@ class TestVirshSSH(MAASTestCase):
246 "emulator": "/usr/bin/qemu-system-x86_64",
247 }
248 self.patch(
249- virsh.VirshSSH, "create_local_volume"
250+ virsh.VirshSSH, "_create_local_volume"
251 ).return_value = disk_info
252 self.patch(
253 virsh.VirshSSH, "get_domain_capabilities"
254@@ -2197,7 +2212,7 @@ class TestVirshSSH(MAASTestCase):
255 "emulator": "/usr/bin/qemu-system-x86_64",
256 }
257 self.patch(
258- virsh.VirshSSH, "create_local_volume"
259+ virsh.VirshSSH, "_create_local_volume"
260 ).return_value = disk_info
261 self.patch(
262 virsh.VirshSSH, "get_domain_capabilities"
263@@ -2275,7 +2290,7 @@ class TestVirshSSH(MAASTestCase):
264 "emulator": "/usr/bin/qemu-system-x86_64",
265 }
266 self.patch(
267- virsh.VirshSSH, "create_local_volume"
268+ virsh.VirshSSH, "_create_local_volume"
269 ).return_value = disk_info
270 self.patch(
271 virsh.VirshSSH, "get_domain_capabilities"
272@@ -2353,7 +2368,7 @@ class TestVirshSSH(MAASTestCase):
273 "emulator": "/usr/bin/qemu-system-x86_64",
274 }
275 self.patch(
276- virsh.VirshSSH, "create_local_volume"
277+ virsh.VirshSSH, "_create_local_volume"
278 ).return_value = disk_info
279 self.patch(
280 virsh.VirshSSH, "get_domain_capabilities"
281diff --git a/src/provisioningserver/drivers/pod/virsh.py b/src/provisioningserver/drivers/pod/virsh.py
282index 09dbba3..f26d4a5 100644
283--- a/src/provisioningserver/drivers/pod/virsh.py
284+++ b/src/provisioningserver/drivers/pod/virsh.py
285@@ -247,6 +247,9 @@ REQUIRED_PACKAGES = [
286 ]
287
288
289+SUPPORTED_STORAGE_TYPES = ("dir", "logical", "zfs")
290+
291+
292 class VirshVMState:
293 OFF = "shut off"
294 ON = "running"
295@@ -465,10 +468,11 @@ class VirshSSH(pexpect.spawn):
296 return [m for m in machines if m.startswith(self.dom_prefix)]
297
298 def list_pools(self):
299- """Lists all pools in the pod."""
300- keys = ["Name"]
301- output = self.run(["pool-list"])
302- pools = self.get_column_values(output, keys)
303+ """Lists supported pools in the host."""
304+ output = self.run(
305+ ["pool-list", "--type", ",".join(SUPPORTED_STORAGE_TYPES)]
306+ )
307+ pools = self.get_column_values(output, ["Name"])
308 return [p[0] for p in pools]
309
310 def list_machine_block_devices(self, machine):
311@@ -897,7 +901,7 @@ class VirshSSH(pexpect.spawn):
312 % (", ".join([pool.name for pool in pools]))
313 )
314
315- def create_local_volume(self, disk, default_pool=None):
316+ def _create_local_volume(self, disk, default_pool=None):
317 """Create a local volume with `disk.size`."""
318 usable_pool_type, usable_pool = self.get_usable_pool(
319 disk, default_pool
320@@ -905,47 +909,35 @@ class VirshSSH(pexpect.spawn):
321 if usable_pool is None:
322 return None
323 volume = str(uuid4())
324- if usable_pool_type == "logical":
325- self.run(
326- [
327- "vol-create-as",
328- usable_pool,
329- volume,
330- str(disk.size),
331- "--format",
332- "raw",
333- ]
334- )
335+
336+ disk_size = disk.size
337+ extra_args = []
338+ if usable_pool_type == "dir":
339+ extra_args = [
340+ "--allocation",
341+ "0",
342+ "--format",
343+ "raw",
344+ ]
345 elif usable_pool_type == "zfs":
346 # LP: #1858201
347 # Round down to the nearest MiB for zfs.
348 # See bug comments for more details.
349- size = int(floor(disk.size / 2 ** 20)) * 2 ** 20
350- self.run(
351- [
352- "vol-create-as",
353- usable_pool,
354- volume,
355- str(size),
356- "--allocation",
357- "0",
358- "--format",
359- "raw",
360- ]
361- )
362- else:
363- self.run(
364- [
365- "vol-create-as",
366- usable_pool,
367- volume,
368- str(disk.size),
369- "--allocation",
370- "0",
371- "--format",
372- "raw",
373- ]
374- )
375+ disk_size = int(floor(disk.size / 2 ** 20)) * 2 ** 20
376+ extra_args = [
377+ "--allocation",
378+ "0",
379+ ]
380+
381+ self.run(
382+ [
383+ "vol-create-as",
384+ usable_pool,
385+ volume,
386+ str(disk_size),
387+ ]
388+ + extra_args,
389+ )
390 return usable_pool, volume
391
392 def delete_local_volume(self, pool, volume):
393@@ -1199,7 +1191,7 @@ class VirshSSH(pexpect.spawn):
394 created_disks = []
395 for idx, disk in enumerate(request.block_devices):
396 try:
397- disk_info = self.create_local_volume(disk, default_pool)
398+ disk_info = self._create_local_volume(disk, default_pool)
399 except Exception:
400 self.cleanup_disks(created_disks)
401 raise

Subscribers

People subscribed via source and target branches