MAAS

Merge ~ack/maas:virsh-create-vol-rework into maas:master

  1. Git
  2. lp:~ack/maas
  3. virsh-create-vol-rework
  4. Merge into master
Proposed by Alberto Donato
Status: Merged
Approved by: Alberto Donato
Approved revision: e7f1ae2f088475af62ff9e9c418ceb055cdf95a8
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~ack/maas:virsh-create-vol-rework
Merge into: maas:master
Diff against target: 335 lines (+89/-72)
2 files modified
src/provisioningserver/drivers/pod/tests/test_virsh.py (+55/-31)
src/provisioningserver/drivers/pod/virsh.py (+34/-41)
Reviewer Review Type Date Requested Status
Lee Trager (community) Approve
MAAS Lander Approve
Review via email: mp+403335@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
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b virsh-create-vol-rework lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10141/console
COMMIT: 7e90feafae26cdbef36644d68db4750e817259f9

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

UNIT TESTS
-b virsh-create-vol-rework lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

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

review: Needs Fixing
Revision history for this message
Alberto Donato (ack) wrote :

jenkins: !test

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

UNIT TESTS
-b virsh-create-vol-rework lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

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

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

UNIT TESTS
-b virsh-create-vol-rework lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10148/console
COMMIT: 4450ef6af95876957ef22edccdd1b34394451abc

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

UNIT TESTS
-b virsh-create-vol-rework lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: e7f1ae2f088475af62ff9e9c418ceb055cdf95a8

review: Approve
Revision history for this message
Lee Trager (ltrager) wrote :

LGTM!

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

LANDING
-b virsh-create-vol-rework lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

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

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

LANDING
-b virsh-create-vol-rework lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

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

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 e1a04b4..79ebded 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@@ -827,6 +831,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@@ -1517,7 +1544,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 tagged_pools = ["pool1", "pool2"]
103@@ -1526,21 +1555,20 @@ class TestVirshSSH(MAASTestCase):
104 )
105 self.patch(conn, "list_pools").return_value = tagged_pools
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 disk = RequestedMachineBlockDevice(
122 size=random.randint(1000, 2000), tags=[]
123 )
124- used_pool, volume_name = conn.create_local_volume(disk)
125+ used_pool, volume_name = conn._create_local_volume(disk)
126 conn.run.assert_called_once_with(
127 [
128 "vol-create-as",
129@@ -1566,15 +1594,13 @@ class TestVirshSSH(MAASTestCase):
130 disk = RequestedMachineBlockDevice(
131 size=random.randint(1000, 2000), tags=[]
132 )
133- used_pool, volume_name = conn.create_local_volume(disk)
134+ used_pool, volume_name = conn._create_local_volume(disk)
135 conn.run.assert_called_once_with(
136 [
137 "vol-create-as",
138 used_pool,
139 volume_name,
140 str(disk.size),
141- "--format",
142- "raw",
143 ]
144 )
145 self.assertEqual(pool, used_pool)
146@@ -1590,7 +1616,7 @@ class TestVirshSSH(MAASTestCase):
147 disk = RequestedMachineBlockDevice(
148 size=random.randint(1000, 2000), tags=[]
149 )
150- used_pool, volume_name = conn.create_local_volume(disk)
151+ used_pool, volume_name = conn._create_local_volume(disk)
152 size = int(floor(disk.size / 2 ** 20)) * 2 ** 20
153 conn.run.assert_called_once_with(
154 [
155@@ -1600,8 +1626,6 @@ class TestVirshSSH(MAASTestCase):
156 str(size),
157 "--allocation",
158 "0",
159- "--format",
160- "raw",
161 ]
162 )
163 self.assertEqual(pool, used_pool)
164@@ -2032,7 +2056,7 @@ class TestVirshSSH(MAASTestCase):
165 factory.make_name("pool"),
166 factory.make_name("vol"),
167 )
168- self.patch(virsh.VirshSSH, "create_local_volume").side_effect = [
169+ self.patch(virsh.VirshSSH, "_create_local_volume").side_effect = [
170 (first_pool, first_vol),
171 exception,
172 ]
173@@ -2046,7 +2070,7 @@ class TestVirshSSH(MAASTestCase):
174 def test_create_domain_handles_no_space(self):
175 conn = self.configure_virshssh("")
176 request = make_requested_machine()
177- self.patch(virsh.VirshSSH, "create_local_volume").return_value = None
178+ self.patch(virsh.VirshSSH, "_create_local_volume").return_value = None
179 error = self.assertRaises(
180 PodInvalidResources, conn.create_domain, request
181 )
182@@ -2063,7 +2087,7 @@ class TestVirshSSH(MAASTestCase):
183 "emulator": "/usr/bin/qemu-system-x86_64",
184 }
185 self.patch(
186- virsh.VirshSSH, "create_local_volume"
187+ virsh.VirshSSH, "_create_local_volume"
188 ).return_value = disk_info
189 self.patch(
190 virsh.VirshSSH, "get_domain_capabilities"
191@@ -2135,7 +2159,7 @@ class TestVirshSSH(MAASTestCase):
192 "emulator": "/usr/bin/qemu-system-x86_64",
193 }
194 self.patch(
195- virsh.VirshSSH, "create_local_volume"
196+ virsh.VirshSSH, "_create_local_volume"
197 ).return_value = disk_info
198 self.patch(
199 virsh.VirshSSH, "get_domain_capabilities"
200@@ -2199,7 +2223,7 @@ class TestVirshSSH(MAASTestCase):
201 "emulator": "/usr/bin/qemu-system-x86_64",
202 }
203 self.patch(
204- virsh.VirshSSH, "create_local_volume"
205+ virsh.VirshSSH, "_create_local_volume"
206 ).return_value = disk_info
207 self.patch(
208 virsh.VirshSSH, "get_domain_capabilities"
209@@ -2263,7 +2287,7 @@ class TestVirshSSH(MAASTestCase):
210 "emulator": "/usr/bin/qemu-system-x86_64",
211 }
212 self.patch(
213- virsh.VirshSSH, "create_local_volume"
214+ virsh.VirshSSH, "_create_local_volume"
215 ).return_value = disk_info
216 self.patch(
217 virsh.VirshSSH, "get_domain_capabilities"
218diff --git a/src/provisioningserver/drivers/pod/virsh.py b/src/provisioningserver/drivers/pod/virsh.py
219index 2845e50..40903d4 100644
220--- a/src/provisioningserver/drivers/pod/virsh.py
221+++ b/src/provisioningserver/drivers/pod/virsh.py
222@@ -253,6 +253,9 @@ REQUIRED_PACKAGES = [
223 ]
224
225
226+SUPPORTED_STORAGE_TYPES = ("dir", "logical", "zfs")
227+
228+
229 class VirshVMState:
230 OFF = "shut off"
231 ON = "running"
232@@ -465,8 +468,10 @@ class VirshSSH(pexpect.spawn):
233 return [m for m in machines if m.startswith(self.dom_prefix)]
234
235 def list_pools(self):
236- """Lists all pools in the pod."""
237- output = self.run(["pool-list"])
238+ """Lists supported pools in the host."""
239+ output = self.run(
240+ ["pool-list", "--type", ",".join(SUPPORTED_STORAGE_TYPES)]
241+ )
242 pools = self._get_column_values(output, ["Name"])
243 return [p[0] for p in pools]
244
245@@ -892,7 +897,7 @@ class VirshSSH(pexpect.spawn):
246 % (", ".join([pool.name for pool in pools]))
247 )
248
249- def create_local_volume(self, disk, default_pool=None):
250+ def _create_local_volume(self, disk, default_pool=None):
251 """Create a local volume with `disk.size`."""
252 usable_pool_type, usable_pool = self.get_usable_pool(
253 disk, default_pool
254@@ -900,47 +905,35 @@ class VirshSSH(pexpect.spawn):
255 if usable_pool is None:
256 return None
257 volume = str(uuid4())
258- if usable_pool_type == "logical":
259- self.run(
260- [
261- "vol-create-as",
262- usable_pool,
263- volume,
264- str(disk.size),
265- "--format",
266- "raw",
267- ]
268- )
269+
270+ disk_size = disk.size
271+ extra_args = []
272+ if usable_pool_type == "dir":
273+ extra_args = [
274+ "--allocation",
275+ "0",
276+ "--format",
277+ "raw",
278+ ]
279 elif usable_pool_type == "zfs":
280 # LP: #1858201
281 # Round down to the nearest MiB for zfs.
282 # See bug comments for more details.
283- size = int(floor(disk.size / 2 ** 20)) * 2 ** 20
284- self.run(
285- [
286- "vol-create-as",
287- usable_pool,
288- volume,
289- str(size),
290- "--allocation",
291- "0",
292- "--format",
293- "raw",
294- ]
295- )
296- else:
297- self.run(
298- [
299- "vol-create-as",
300- usable_pool,
301- volume,
302- str(disk.size),
303- "--allocation",
304- "0",
305- "--format",
306- "raw",
307- ]
308- )
309+ disk_size = int(floor(disk.size / 2 ** 20)) * 2 ** 20
310+ extra_args = [
311+ "--allocation",
312+ "0",
313+ ]
314+
315+ self.run(
316+ [
317+ "vol-create-as",
318+ usable_pool,
319+ volume,
320+ str(disk_size),
321+ ]
322+ + extra_args,
323+ )
324 return usable_pool, volume
325
326 def delete_local_volume(self, pool, volume):
327@@ -1175,7 +1168,7 @@ class VirshSSH(pexpect.spawn):
328 created_disks = []
329 for idx, disk in enumerate(request.block_devices):
330 try:
331- disk_info = self.create_local_volume(disk, default_pool)
332+ disk_info = self._create_local_volume(disk, default_pool)
333 except Exception:
334 self.cleanup_disks(created_disks)
335 raise

Subscribers

People subscribed via source and target branches