MAAS

Merge ~ltrager/maas:proxmox_papercuts_2.9 into maas:2.9

  1. Git
  2. lp:~ltrager/maas
  3. proxmox_papercuts_2.9
  4. Merge into 2.9
Proposed by Lee Trager
Status: Merged
Approved by: Lee Trager
Approved revision: f66b4a8bf2b92fc7178188dd471d0759c698b8b3
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~ltrager/maas:proxmox_papercuts_2.9
Merge into: maas:2.9
Diff against target: 313 lines (+120/-68)
4 files modified
src/provisioningserver/drivers/power/proxmox.py (+13/-5)
src/provisioningserver/drivers/power/tests/test_proxmox.py (+100/-54)
src/provisioningserver/rpc/clusterservice.py (+1/-2)
src/provisioningserver/rpc/tests/test_clusterservice.py (+6/-7)
Reviewer Review Type Date Requested Status
Lee Trager (community) Approve
Review via email: mp+399318@code.launchpad.net

Commit message

Fix various Proxmox papercuts

* power_vm_name is required
* Fix warnings that a synchronous thread was returning a Deferred() object
* Add log message when no VMs are returned. Proxmox forces you to define
  permissions for a token, if you don't nothing gets returned. Users
  thought this was a MAAS bug.

Backport of c19697f

To post a comment you must log in.
Revision history for this message
Lee Trager (ltrager) wrote :

Approved in https://code.launchpad.net/~ltrager/maas/+git/maas/+merge/399245

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/src/provisioningserver/drivers/power/proxmox.py b/src/provisioningserver/drivers/power/proxmox.py
index cf7a8db..cc0b720 100644
--- a/src/provisioningserver/drivers/power/proxmox.py
+++ b/src/provisioningserver/drivers/power/proxmox.py
@@ -54,7 +54,7 @@ class ProxmoxPowerDriver(WebhookPowerDriver):
54 field_type="password",54 field_type="password",
55 ),55 ),
56 make_setting_field(56 make_setting_field(
57 "power_vm_name", "Node ID", scope=SETTING_SCOPE.NODE57 "power_vm_name", "Node ID", scope=SETTING_SCOPE.NODE, required=True
58 ),58 ),
59 make_setting_field(59 make_setting_field(
60 "power_verify_ssl",60 "power_verify_ssl",
@@ -157,7 +157,12 @@ class ProxmoxPowerDriver(WebhookPowerDriver):
157157
158 def cb(response_data):158 def cb(response_data):
159 parsed_data = json.loads(response_data)159 parsed_data = json.loads(response_data)
160 for vm in parsed_data["data"]:160 vms = parsed_data["data"]
161 if not vms:
162 raise PowerActionError(
163 "No VMs returned! Are permissions set correctly?"
164 )
165 for vm in vms:
161 if power_vm_name in (str(vm.get("vmid")), vm.get("name")):166 if power_vm_name in (str(vm.get("vmid")), vm.get("name")):
162 return vm167 return vm
163 raise PowerActionError("Unable to find virtual machine")168 raise PowerActionError("Unable to find virtual machine")
@@ -252,7 +257,6 @@ def probe_proxmox_and_enlist(
252257
253 d = proxmox._login("", context)258 d = proxmox._login("", context)
254259
255 @asynchronous
256 @inlineCallbacks260 @inlineCallbacks
257 def get_vms(extra_headers):261 def get_vms(extra_headers):
258 vms = yield proxmox._webhook_request(262 vms = yield proxmox._webhook_request(
@@ -265,11 +269,15 @@ def probe_proxmox_and_enlist(
265269
266 d.addCallback(get_vms)270 d.addCallback(get_vms)
267271
268 @asynchronous
269 @inlineCallbacks272 @inlineCallbacks
270 def process_vms(data):273 def process_vms(data):
271 extra_headers, response_data = data274 extra_headers, response_data = data
272 for vm in json.loads(response_data)["data"]:275 vms = json.loads(response_data)["data"]
276 if not vms:
277 raise PowerActionError(
278 "No VMs returned! Are permissions set correctly?"
279 )
280 for vm in vms:
273 if prefix_filter and not vm["name"].startswith(prefix_filter):281 if prefix_filter and not vm["name"].startswith(prefix_filter):
274 continue282 continue
275 # Proxmox doesn't have an easy way to get the MAC address, it283 # Proxmox doesn't have an easy way to get the MAC address, it
diff --git a/src/provisioningserver/drivers/power/tests/test_proxmox.py b/src/provisioningserver/drivers/power/tests/test_proxmox.py
index 6292eae..2903e8c 100644
--- a/src/provisioningserver/drivers/power/tests/test_proxmox.py
+++ b/src/provisioningserver/drivers/power/tests/test_proxmox.py
@@ -108,20 +108,17 @@ class TestProxmoxPowerDriver(MAASTestCase):
108 },108 },
109 extra_headers,109 extra_headers,
110 )110 )
111 self.assertThat(111 self.mock_webhook_request.assert_called_once_with(
112 self.mock_webhook_request,112 b"POST",
113 MockCalledOnceWith(113 self.proxmox._get_url(context, "access/ticket"),
114 b"POST",114 self.proxmox._make_auth_headers(
115 self.proxmox._get_url(context, "access/ticket"),115 system_id,
116 self.proxmox._make_auth_headers(116 {},
117 system_id,117 {b"Content-Type": [b"application/json; charset=utf-8"]},
118 {},
119 {b"Content-Type": [b"application/json; charset=utf-8"]},
120 ),
121 False,
122 # unittest doesn't know how to compare FileBodyProducer
123 ANY,
124 ),118 ),
119 False,
120 # unittest doesn't know how to compare FileBodyProducer
121 ANY,
125 )122 )
126123
127 @inlineCallbacks124 @inlineCallbacks
@@ -197,16 +194,43 @@ class TestProxmoxPowerDriver(MAASTestCase):
197 )194 )
198195
199 self.assertEqual(vm, found_vm)196 self.assertEqual(vm, found_vm)
200 self.assertThat(197 self.mock_webhook_request.assert_called_once_with(
201 self.mock_webhook_request,198 b"GET",
202 MockCalledOnceWith(199 self.proxmox._get_url(
203 b"GET",200 context, "cluster/resources", {"type": "vm"}
204 self.proxmox._get_url(
205 context, "cluster/resources", {"type": "vm"}
206 ),
207 self.proxmox._make_auth_headers(system_id, {}, extra_headers),
208 False,
209 ),201 ),
202 self.proxmox._make_auth_headers(system_id, {}, extra_headers),
203 False,
204 )
205
206 @inlineCallbacks
207 def test_find_vm_doesnt_find_any_vms(self):
208 system_id = factory.make_name("system_id")
209 context = {
210 "power_address": factory.make_name("power_address"),
211 "power_vm_name": factory.make_name("power_vm_name"),
212 }
213 extra_headers = {
214 factory.make_name("key").encode(): [
215 factory.make_name("value").encode()
216 ]
217 for _ in range(3)
218 }
219 self.mock_webhook_request.return_value = succeed(
220 json.dumps({"data": []})
221 )
222
223 with ExpectedException(
224 PowerActionError, "No VMs returned! Are permissions set correctly?"
225 ):
226 yield self.proxmox._find_vm(system_id, context, extra_headers)
227 self.mock_webhook_request.assert_called_once_with(
228 b"GET",
229 self.proxmox._get_url(
230 context, "cluster/resources", {"type": "vm"}
231 ),
232 self.proxmox._make_auth_headers(system_id, {}, extra_headers),
233 False,
210 )234 )
211235
212 @inlineCallbacks236 @inlineCallbacks
@@ -231,18 +255,17 @@ class TestProxmoxPowerDriver(MAASTestCase):
231 json.dumps({"data": [vm]})255 json.dumps({"data": [vm]})
232 )256 )
233257
234 with ExpectedException(PowerActionError):258 with ExpectedException(
259 PowerActionError, "Unable to find virtual machine"
260 ):
235 yield self.proxmox._find_vm(system_id, context, extra_headers)261 yield self.proxmox._find_vm(system_id, context, extra_headers)
236 self.assertThat(262 self.mock_webhook_request.assert_called_once_with(
237 self.mock_webhook_request,263 b"GET",
238 MockCalledOnceWith(264 self.proxmox._get_url(
239 b"GET",265 context, "cluster/resources", {"type": "vm"}
240 self.proxmox._get_url(
241 context, "cluster/resources", {"type": "vm"}
242 ),
243 self.proxmox._make_auth_headers(system_id, {}, extra_headers),
244 False,
245 ),266 ),
267 self.proxmox._make_auth_headers(system_id, {}, extra_headers),
268 False,
246 )269 )
247270
248 @inlineCallbacks271 @inlineCallbacks
@@ -268,18 +291,15 @@ class TestProxmoxPowerDriver(MAASTestCase):
268291
269 yield self.proxmox.power_on(system_id, context)292 yield self.proxmox.power_on(system_id, context)
270293
271 self.assertThat(294 self.mock_webhook_request.assert_called_once_with(
272 self.mock_webhook_request,295 b"POST",
273 MockCalledOnceWith(296 self.proxmox._get_url(
274 b"POST",297 context,
275 self.proxmox._get_url(298 f"nodes/{vm['node']}/{vm['type']}/{vm['vmid']}/"
276 context,299 "status/start",
277 f"nodes/{vm['node']}/{vm['type']}/{vm['vmid']}/"
278 "status/start",
279 ),
280 self.proxmox._make_auth_headers(system_id, {}, extra_headers),
281 False,
282 ),300 ),
301 self.proxmox._make_auth_headers(system_id, {}, extra_headers),
302 False,
283 )303 )
284304
285 @inlineCallbacks305 @inlineCallbacks
@@ -330,18 +350,14 @@ class TestProxmoxPowerDriver(MAASTestCase):
330350
331 yield self.proxmox.power_off(system_id, context)351 yield self.proxmox.power_off(system_id, context)
332352
333 self.assertThat(353 self.mock_webhook_request.assert_called_once_with(
334 self.mock_webhook_request,354 b"POST",
335 MockCalledOnceWith(355 self.proxmox._get_url(
336 b"POST",356 context,
337 self.proxmox._get_url(357 f"nodes/{vm['node']}/{vm['type']}/{vm['vmid']}/" "status/stop",
338 context,
339 f"nodes/{vm['node']}/{vm['type']}/{vm['vmid']}/"
340 "status/stop",
341 ),
342 self.proxmox._make_auth_headers(system_id, {}, extra_headers),
343 False,
344 ),358 ),
359 self.proxmox._make_auth_headers(system_id, {}, extra_headers),
360 False,
345 )361 )
346362
347 @inlineCallbacks363 @inlineCallbacks
@@ -573,6 +589,36 @@ class TestProxmoxProbeAndEnlist(MAASTestCase):
573 self.assertThat(self.mock_commission_node, MockNotCalled())589 self.assertThat(self.mock_commission_node, MockNotCalled())
574590
575 @inlineCallbacks591 @inlineCallbacks
592 def test_probe_and_enlist_doesnt_find_any_vms(self):
593 user = factory.make_name("user")
594 hostname = factory.make_ipv4_address()
595 username = factory.make_name("username")
596 password = factory.make_name("password")
597 token_name = factory.make_name("token_name")
598 token_secret = factory.make_name("token_secret")
599 domain = factory.make_name("domain")
600 mock_webhook_request = self.patch(
601 proxmox_module.ProxmoxPowerDriver, "_webhook_request"
602 )
603 mock_webhook_request.return_value = succeed(json.dumps({"data": []}))
604
605 with ExpectedException(
606 PowerActionError, "No VMs returned! Are permissions set correctly?"
607 ):
608 yield proxmox_module.probe_proxmox_and_enlist(
609 user,
610 hostname,
611 username,
612 password,
613 token_name,
614 token_secret,
615 False,
616 False,
617 domain,
618 None,
619 )
620
621 @inlineCallbacks
576 def test_probe_and_enlist_filters(self):622 def test_probe_and_enlist_filters(self):
577 user = factory.make_name("user")623 user = factory.make_name("user")
578 hostname = factory.make_ipv4_address()624 hostname = factory.make_ipv4_address()
diff --git a/src/provisioningserver/rpc/clusterservice.py b/src/provisioningserver/rpc/clusterservice.py
index d8b269d..279a804 100644
--- a/src/provisioningserver/rpc/clusterservice.py
+++ b/src/provisioningserver/rpc/clusterservice.py
@@ -807,8 +807,7 @@ class Cluster(RPCProtocol):
807 )807 )
808 d.addErrback(partial(catch_probe_and_enlist_error, "virsh"))808 d.addErrback(partial(catch_probe_and_enlist_error, "virsh"))
809 elif chassis_type == "proxmox":809 elif chassis_type == "proxmox":
810 d = deferToThread(810 d = probe_proxmox_and_enlist(
811 probe_proxmox_and_enlist,
812 user,811 user,
813 hostname,812 hostname,
814 username,813 username,
diff --git a/src/provisioningserver/rpc/tests/test_clusterservice.py b/src/provisioningserver/rpc/tests/test_clusterservice.py
index 4485515..3512523 100644
--- a/src/provisioningserver/rpc/tests/test_clusterservice.py
+++ b/src/provisioningserver/rpc/tests/test_clusterservice.py
@@ -3487,8 +3487,8 @@ class TestClusterProtocol_AddChassis(MAASTestCase):
3487 )3487 )
34883488
3489 def test_chassis_type_proxmox_calls_probe_proxmoxand_enlist(self):3489 def test_chassis_type_proxmox_calls_probe_proxmoxand_enlist(self):
3490 mock_deferToThread = self.patch_autospec(3490 mock_proxmox = self.patch_autospec(
3491 clusterservice, "deferToThread"3491 clusterservice, "probe_proxmox_and_enlist"
3492 )3492 )
3493 user = factory.make_name("user")3493 user = factory.make_name("user")
3494 hostname = factory.make_hostname()3494 hostname = factory.make_hostname()
@@ -3518,9 +3518,8 @@ class TestClusterProtocol_AddChassis(MAASTestCase):
3518 },3518 },
3519 )3519 )
3520 self.assertThat(3520 self.assertThat(
3521 mock_deferToThread,3521 mock_proxmox,
3522 MockCalledOnceWith(3522 MockCalledOnceWith(
3523 clusterservice.probe_proxmox_and_enlist,
3524 user,3523 user,
3525 hostname,3524 hostname,
3526 username,3525 username,
@@ -3537,10 +3536,10 @@ class TestClusterProtocol_AddChassis(MAASTestCase):
3537 def test_chassis_type_proxmox_logs_error_to_maaslog(self):3536 def test_chassis_type_proxmox_logs_error_to_maaslog(self):
3538 fake_error = factory.make_name("error")3537 fake_error = factory.make_name("error")
3539 self.patch(clusterservice, "maaslog")3538 self.patch(clusterservice, "maaslog")
3540 mock_deferToThread = self.patch_autospec(3539 mock_proxmox = self.patch_autospec(
3541 clusterservice, "deferToThread"3540 clusterservice, "probe_proxmox_and_enlist"
3542 )3541 )
3543 mock_deferToThread.return_value = fail(Exception(fake_error))3542 mock_proxmox.return_value = fail(Exception(fake_error))
3544 user = factory.make_name("user")3543 user = factory.make_name("user")
3545 hostname = factory.make_hostname()3544 hostname = factory.make_hostname()
3546 username = factory.make_name("username")3545 username = factory.make_name("username")

Subscribers

People subscribed via source and target branches