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

[Lee Trager]

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

[Lee Trager (ltrager)]

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

review: Approve

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):
             field_type="password",
         ),
         make_setting_field(
-            "power_vm_name", "Node ID", scope=SETTING_SCOPE.NODE
+            "power_vm_name", "Node ID", scope=SETTING_SCOPE.NODE, required=True
         ),
         make_setting_field(
             "power_verify_ssl",
@@ -157,7 +157,12 @@ class ProxmoxPowerDriver(WebhookPowerDriver):
 
         def cb(response_data):
             parsed_data = json.loads(response_data)
-            for vm in parsed_data["data"]:
+            vms = parsed_data["data"]
+            if not vms:
+                raise PowerActionError(
+                    "No VMs returned! Are permissions set correctly?"
+                )
+            for vm in vms:
                 if power_vm_name in (str(vm.get("vmid")), vm.get("name")):
                     return vm
             raise PowerActionError("Unable to find virtual machine")
@@ -252,7 +257,6 @@ def probe_proxmox_and_enlist(
 
     d = proxmox._login("", context)
 
-    @asynchronous
     @inlineCallbacks
     def get_vms(extra_headers):
         vms = yield proxmox._webhook_request(
@@ -265,11 +269,15 @@ def probe_proxmox_and_enlist(
 
     d.addCallback(get_vms)
 
-    @asynchronous
     @inlineCallbacks
     def process_vms(data):
         extra_headers, response_data = data
-        for vm in json.loads(response_data)["data"]:
+        vms = json.loads(response_data)["data"]
+        if not vms:
+            raise PowerActionError(
+                "No VMs returned! Are permissions set correctly?"
+            )
+        for vm in vms:
             if prefix_filter and not vm["name"].startswith(prefix_filter):
                 continue
             # 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):
             },
             extra_headers,
         )
-        self.assertThat(
-            self.mock_webhook_request,
-            MockCalledOnceWith(
-                b"POST",
-                self.proxmox._get_url(context, "access/ticket"),
-                self.proxmox._make_auth_headers(
-                    system_id,
-                    {},
-                    {b"Content-Type": [b"application/json; charset=utf-8"]},
-                ),
-                False,
-                # unittest doesn't know how to compare FileBodyProducer
-                ANY,
+        self.mock_webhook_request.assert_called_once_with(
+            b"POST",
+            self.proxmox._get_url(context, "access/ticket"),
+            self.proxmox._make_auth_headers(
+                system_id,
+                {},
+                {b"Content-Type": [b"application/json; charset=utf-8"]},
             ),
+            False,
+            # unittest doesn't know how to compare FileBodyProducer
+            ANY,
         )
 
     @inlineCallbacks
@@ -197,16 +194,43 @@ class TestProxmoxPowerDriver(MAASTestCase):
         )
 
         self.assertEqual(vm, found_vm)
-        self.assertThat(
-            self.mock_webhook_request,
-            MockCalledOnceWith(
-                b"GET",
-                self.proxmox._get_url(
-                    context, "cluster/resources", {"type": "vm"}
-                ),
-                self.proxmox._make_auth_headers(system_id, {}, extra_headers),
-                False,
+        self.mock_webhook_request.assert_called_once_with(
+            b"GET",
+            self.proxmox._get_url(
+                context, "cluster/resources", {"type": "vm"}
             ),
+            self.proxmox._make_auth_headers(system_id, {}, extra_headers),
+            False,
+        )
+
+    @inlineCallbacks
+    def test_find_vm_doesnt_find_any_vms(self):
+        system_id = factory.make_name("system_id")
+        context = {
+            "power_address": factory.make_name("power_address"),
+            "power_vm_name": factory.make_name("power_vm_name"),
+        }
+        extra_headers = {
+            factory.make_name("key").encode(): [
+                factory.make_name("value").encode()
+            ]
+            for _ in range(3)
+        }
+        self.mock_webhook_request.return_value = succeed(
+            json.dumps({"data": []})
+        )
+
+        with ExpectedException(
+            PowerActionError, "No VMs returned! Are permissions set correctly?"
+        ):
+            yield self.proxmox._find_vm(system_id, context, extra_headers)
+        self.mock_webhook_request.assert_called_once_with(
+            b"GET",
+            self.proxmox._get_url(
+                context, "cluster/resources", {"type": "vm"}
+            ),
+            self.proxmox._make_auth_headers(system_id, {}, extra_headers),
+            False,
         )
 
     @inlineCallbacks
@@ -231,18 +255,17 @@ class TestProxmoxPowerDriver(MAASTestCase):
             json.dumps({"data": [vm]})
         )
 
-        with ExpectedException(PowerActionError):
+        with ExpectedException(
+            PowerActionError, "Unable to find virtual machine"
+        ):
             yield self.proxmox._find_vm(system_id, context, extra_headers)
-        self.assertThat(
-            self.mock_webhook_request,
-            MockCalledOnceWith(
-                b"GET",
-                self.proxmox._get_url(
-                    context, "cluster/resources", {"type": "vm"}
-                ),
-                self.proxmox._make_auth_headers(system_id, {}, extra_headers),
-                False,
+        self.mock_webhook_request.assert_called_once_with(
+            b"GET",
+            self.proxmox._get_url(
+                context, "cluster/resources", {"type": "vm"}
             ),
+            self.proxmox._make_auth_headers(system_id, {}, extra_headers),
+            False,
         )
 
     @inlineCallbacks
@@ -268,18 +291,15 @@ class TestProxmoxPowerDriver(MAASTestCase):
 
         yield self.proxmox.power_on(system_id, context)
 
-        self.assertThat(
-            self.mock_webhook_request,
-            MockCalledOnceWith(
-                b"POST",
-                self.proxmox._get_url(
-                    context,
-                    f"nodes/{vm['node']}/{vm['type']}/{vm['vmid']}/"
-                    "status/start",
-                ),
-                self.proxmox._make_auth_headers(system_id, {}, extra_headers),
-                False,
+        self.mock_webhook_request.assert_called_once_with(
+            b"POST",
+            self.proxmox._get_url(
+                context,
+                f"nodes/{vm['node']}/{vm['type']}/{vm['vmid']}/"
+                "status/start",
             ),
+            self.proxmox._make_auth_headers(system_id, {}, extra_headers),
+            False,
         )
 
     @inlineCallbacks
@@ -330,18 +350,14 @@ class TestProxmoxPowerDriver(MAASTestCase):
 
         yield self.proxmox.power_off(system_id, context)
 
-        self.assertThat(
-            self.mock_webhook_request,
-            MockCalledOnceWith(
-                b"POST",
-                self.proxmox._get_url(
-                    context,
-                    f"nodes/{vm['node']}/{vm['type']}/{vm['vmid']}/"
-                    "status/stop",
-                ),
-                self.proxmox._make_auth_headers(system_id, {}, extra_headers),
-                False,
+        self.mock_webhook_request.assert_called_once_with(
+            b"POST",
+            self.proxmox._get_url(
+                context,
+                f"nodes/{vm['node']}/{vm['type']}/{vm['vmid']}/" "status/stop",
             ),
+            self.proxmox._make_auth_headers(system_id, {}, extra_headers),
+            False,
         )
 
     @inlineCallbacks
@@ -573,6 +589,36 @@ class TestProxmoxProbeAndEnlist(MAASTestCase):
         self.assertThat(self.mock_commission_node, MockNotCalled())
 
     @inlineCallbacks
+    def test_probe_and_enlist_doesnt_find_any_vms(self):
+        user = factory.make_name("user")
+        hostname = factory.make_ipv4_address()
+        username = factory.make_name("username")
+        password = factory.make_name("password")
+        token_name = factory.make_name("token_name")
+        token_secret = factory.make_name("token_secret")
+        domain = factory.make_name("domain")
+        mock_webhook_request = self.patch(
+            proxmox_module.ProxmoxPowerDriver, "_webhook_request"
+        )
+        mock_webhook_request.return_value = succeed(json.dumps({"data": []}))
+
+        with ExpectedException(
+            PowerActionError, "No VMs returned! Are permissions set correctly?"
+        ):
+            yield proxmox_module.probe_proxmox_and_enlist(
+                user,
+                hostname,
+                username,
+                password,
+                token_name,
+                token_secret,
+                False,
+                False,
+                domain,
+                None,
+            )
+
+    @inlineCallbacks
     def test_probe_and_enlist_filters(self):
         user = factory.make_name("user")
         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):
             )
             d.addErrback(partial(catch_probe_and_enlist_error, "virsh"))
         elif chassis_type == "proxmox":
-            d = deferToThread(
-                probe_proxmox_and_enlist,
+            d = probe_proxmox_and_enlist(
                 user,
                 hostname,
                 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):
         )
 
     def test_chassis_type_proxmox_calls_probe_proxmoxand_enlist(self):
-        mock_deferToThread = self.patch_autospec(
-            clusterservice, "deferToThread"
+        mock_proxmox = self.patch_autospec(
+            clusterservice, "probe_proxmox_and_enlist"
         )
         user = factory.make_name("user")
         hostname = factory.make_hostname()
@@ -3518,9 +3518,8 @@ class TestClusterProtocol_AddChassis(MAASTestCase):
             },
         )
         self.assertThat(
-            mock_deferToThread,
+            mock_proxmox,
             MockCalledOnceWith(
-                clusterservice.probe_proxmox_and_enlist,
                 user,
                 hostname,
                 username,
@@ -3537,10 +3536,10 @@ class TestClusterProtocol_AddChassis(MAASTestCase):
     def test_chassis_type_proxmox_logs_error_to_maaslog(self):
         fake_error = factory.make_name("error")
         self.patch(clusterservice, "maaslog")
-        mock_deferToThread = self.patch_autospec(
-            clusterservice, "deferToThread"
+        mock_proxmox = self.patch_autospec(
+            clusterservice, "probe_proxmox_and_enlist"
         )
-        mock_deferToThread.return_value = fail(Exception(fake_error))
+        mock_proxmox.return_value = fail(Exception(fake_error))
         user = factory.make_name("user")
         hostname = factory.make_hostname()
         username = factory.make_name("username")