Merge ~mrfreezeex/maas:fix-pool-no-path into maas:master

Proposed by Arthur Outhenin-Chalandre
Status: Needs review
Proposed branch: ~mrfreezeex/maas:fix-pool-no-path
Merge into: maas:master
Diff against target: 157 lines (+71/-23)
2 files modified
src/provisioningserver/drivers/pod/tests/test_virsh.py (+65/-22)
src/provisioningserver/drivers/pod/virsh.py (+6/-1)
Reviewer Review Type Date Requested Status
MAAS Lander Approve
Adam Collard Needs Fixing
Review via email: mp+390022@code.launchpad.net

Description of the change

On some storage pool (i.e. RBD) there is no pool_path, hence the
get_pod_storage_pools function called when a user refresh a KVM info is
crashing.

This commit fix this issue by checking if the pool_path does exist and
if not assigns a dummy/default text to it.

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

UNIT TESTS
-b fix-pool-no-path lp:~mrfreezeex/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 71de0a47b2a0b9ecd90e6482f26ab606a3ee75f4

review: Approve
Revision history for this message
Adam Collard (adam-collard) wrote :

Thanks for the contribution! We'll need tests for this, and having an example xml document which shows the errant behaviour would be really handy, do you have something you can share?

review: Needs Fixing
~mrfreezeex/maas:fix-pool-no-path updated
50aaf77... by Arthur Outhenin-Chalandre

Add test for a basic RBD for get_pod_storage_pools

Signed-off-by: Arthur Outhenin-Chalandre <email address hidden>

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

UNIT TESTS
-b fix-pool-no-path lp:~mrfreezeex/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8231/console
COMMIT: 415f02a13a7f5e6bcce656c5e55e67b090abc469

review: Needs Fixing
Revision history for this message
Arthur Outhenin-Chalandre (mrfreezeex) wrote :

> Thanks for the contribution! We'll need tests for this, and having an example
> xml document which shows the errant behaviour would be really handy, do you
> have something you can share?

I added a test with a basic RBD template. I had to change a bit of logic for defaults in add_pool to make this possible and add a special case for RBD. I don't think that's a great design, but I didn't had any idea to make this better. Let me know if you have a better idea for this test.

Revision history for this message
Arthur Outhenin-Chalandre (mrfreezeex) wrote :

I had issues to run the full testsuite so I am no sure of the failing test in the CI. The test I added did pass on my end.

Revision history for this message
Adam Collard (adam-collard) wrote :

+ sudo -u ubuntu -E -H make lint
would reformat /run/build/maas/src/provisioningserver/drivers/pod/tests/test_virsh.py
Oh no! 💥 💔 💥
1 file would be reformatted, 1565 files would be left unchanged.
make: *** [Makefile:245: lint-py] Error 1

The tests that failed were lint tests, please use `make format-py` to automatically fix it (or run black manually)

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

UNIT TESTS
-b fix-pool-no-path lp:~mrfreezeex/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 50aaf77cb418ef36963470840dbcf2b2a23bd1ab

review: Approve
Revision history for this message
Arthur Outhenin-Chalandre (mrfreezeex) wrote :

Oh BTW this MR is ready to be reviewed, just to be sure that you are not waiting an input from my side before reviewing It.

And thanks again for the hint on black ! :)

P.S.: You probably have many other things to do than looking at my crappy code, I won't bump again my MR.

Unmerged commits

50aaf77... by Arthur Outhenin-Chalandre

Add test for a basic RBD for get_pod_storage_pools

Signed-off-by: Arthur Outhenin-Chalandre <email address hidden>

71de0a4... by Arthur Outhenin-Chalandre

Fix resfresh KVM with storage pool without path

On some storage pool (i.e. RBD) there is no pool_path, hence the
get_pod_storage_pools function called when a user refresh a KVM info is
crashing.

This commit fix this issue by checking if the pool_path does exist and
if not assigns a dummy/default text to it.

Signed-off-by: Arthur Outhenin-Chalandre <email address hidden>

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 b9ab23e..33d8ada 100644
3--- a/src/provisioningserver/drivers/pod/tests/test_virsh.py
4+++ b/src/provisioningserver/drivers/pod/tests/test_virsh.py
5@@ -33,6 +33,7 @@ from provisioningserver.drivers.pod import (
6 virsh,
7 )
8 from provisioningserver.drivers.pod.virsh import (
9+ DEFAULT_POOL_PATH,
10 DOM_TEMPLATE_AMD64,
11 DOM_TEMPLATE_ARM64,
12 DOM_TEMPLATE_BRIDGE_INTERFACE,
13@@ -415,6 +416,22 @@ POOLINFO_TEMPLATE = dedent(
14 """
15 )
16
17+POOLINFO_RBD_TEMPLATE = dedent(
18+ """
19+ <pool type='rbd'>
20+ <name>{name}</name>
21+ <uuid>{uuid}</uuid>
22+ <capacity unit='bytes'>{capacity}</capacity>
23+ <allocation unit='bytes'>{allocation}</allocation>
24+ <available unit='bytes'>{available}</available>
25+ <source>
26+ </source>
27+ </pool>
28+ """
29+)
30+
31+DEFAULT = object()
32+
33
34 class VirshRunFake:
35 """Fake for running virtlib command.
36@@ -430,34 +447,38 @@ class VirshRunFake:
37 name,
38 active=True,
39 autostart=True,
40- pool_uuid=None,
41- capacity=None,
42- allocation=None,
43- available=None,
44- path=None,
45+ pool_uuid=DEFAULT,
46+ capacity=DEFAULT,
47+ allocation=DEFAULT,
48+ available=DEFAULT,
49+ path=DEFAULT,
50+ pool_type="dir",
51 ):
52- if pool_uuid is None:
53+ if pool_uuid is DEFAULT:
54 pool_uuid = str(uuid4())
55- if capacity is None:
56+ if capacity is DEFAULT:
57 capacity = random.randint(10000000000, 1000000000000)
58- if allocation is None:
59+ if allocation is DEFAULT:
60 allocation = random.randint(0, capacity)
61- if available is None:
62+ if available is DEFAULT:
63 available = capacity - allocation
64- if path is None:
65+ if path is DEFAULT:
66 path = "var/lib/virtlib/" + factory.make_name("images")
67- self.pools.append(
68- {
69- "name": name,
70- "active": active,
71- "autostart": autostart,
72- "uuid": pool_uuid,
73- "capacity": capacity,
74- "allocation": allocation,
75- "available": available,
76- "path": path,
77- }
78- )
79+
80+ pool = {
81+ "name": name,
82+ "type": pool_type,
83+ "active": active,
84+ "autostart": autostart,
85+ "uuid": pool_uuid,
86+ "capacity": capacity,
87+ "allocation": allocation,
88+ "available": available,
89+ }
90+
91+ if path:
92+ pool["path"] = path
93+ self.pools.append(pool)
94
95 def __call__(self, args):
96 command = args.pop(0)
97@@ -486,6 +507,8 @@ class VirshRunFake:
98 break
99 else:
100 raise RuntimeError("No pool named " + pool_name)
101+ if pool["type"] == "rbd":
102+ return POOLINFO_RBD_TEMPLATE.format(**pool)
103 return POOLINFO_TEMPLATE.format(**pool)
104
105
106@@ -838,6 +861,26 @@ class TestVirshSSH(MAASTestCase):
107 ]
108 self.assertEqual(expected, conn.get_pod_storage_pools())
109
110+ def test_get_pod_storage_pools_rbd_no_path(self):
111+ conn = virsh.VirshSSH()
112+ fake_runner = VirshRunFake()
113+ fake_runner.add_pool(
114+ factory.make_name("pool"), path=None, pool_type="rbd"
115+ )
116+ run_mock = self.patch(virsh.VirshSSH, "run")
117+ run_mock.side_effect = fake_runner
118+ expected = [
119+ DiscoveredPodStoragePool(
120+ id=pool["uuid"],
121+ type="rbd",
122+ name=pool["name"],
123+ storage=pool["capacity"],
124+ path=DEFAULT_POOL_PATH,
125+ )
126+ for pool in fake_runner.pools
127+ ]
128+ self.assertEqual(expected, conn.get_pod_storage_pools())
129+
130 def test_get_pod_storage_pools_no_pool(self):
131 conn = self.configure_virshssh(SAMPLE_POOLINFO)
132 pools_mock = self.patch(virsh.VirshSSH, "list_pools")
133diff --git a/src/provisioningserver/drivers/pod/virsh.py b/src/provisioningserver/drivers/pod/virsh.py
134index f7aa2f5..afa338c 100644
135--- a/src/provisioningserver/drivers/pod/virsh.py
136+++ b/src/provisioningserver/drivers/pod/virsh.py
137@@ -71,6 +71,8 @@ XPATH_POOL_CAPACITY = "/pool/capacity"
138 XPATH_POOL_PATH = "/pool/target/path"
139 XPATH_POOL_UUID = "/pool/uuid"
140
141+DEFAULT_POOL_PATH = "no_path"
142+
143 DOM_TEMPLATE_MACVLAN_INTERFACE = dedent(
144 """\
145 <interface type='direct'>
146@@ -557,7 +559,10 @@ class VirshSSH(pexpect.spawn):
147 doc = etree.XML(output)
148 evaluator = etree.XPathEvaluator(doc)
149 pool_capacity = int(evaluator(XPATH_POOL_CAPACITY)[0].text)
150- pool_path = evaluator(XPATH_POOL_PATH)[0].text
151+ pool_path = DEFAULT_POOL_PATH
152+ pool_path_xml = evaluator(XPATH_POOL_PATH)
153+ if len(pool_path_xml) > 0:
154+ pool_path = pool_path_xml[0].text
155 pool_type = evaluator(XPATH_POOL_TYPE)[0]
156 pool_uuid = evaluator(XPATH_POOL_UUID)[0].text
157 pool = DiscoveredPodStoragePool(

Subscribers

People subscribed via source and target branches