Merge ~ack/maas:1887797-remove-machine-pod-error into maas:master

[Alberto Donato]

LP: #1887797 - don't fail machine removal if Pod action fails

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1887797-remove-machine-pod-error 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/9161/console
COMMIT: e740923ed84001aa5aabe1c4b4eb957854f9ceaf

review: Needs Fixing

[Adam Collard (adam-collard)]

review: Approve

diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 98a2635..105ac72 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -2942,7 +2942,7 @@ class Node(CleanSave, TimestampedModel):
             client_idents = pod.get_client_identifiers()
 
             @transactional
-            def _save(machine_id, pod_id, hints):
+            def _save(machine_id, pod_id, result):
                 from maasserver.models.bmc import Pod
 
                 machine = Machine.objects.filter(id=machine_id).first()
@@ -2958,9 +2958,16 @@ class Node(CleanSave, TimestampedModel):
                         machine_id=machine_id
                     ).delete()
                     super(Node, machine).delete()
+
+                if isinstance(result, Failure):
+                    maaslog.warning(
+                        f"{self.hostname}: Failure decomposing machine: {result.value}"
+                    )
+                    return
+
                 pod = Pod.objects.filter(id=pod_id).first()
                 if pod is not None:
-                    pod.sync_hints(hints)
+                    pod.sync_hints(result)
 
             maaslog.info("%s: Decomposing machine", self.hostname)
 
@@ -2973,8 +2980,10 @@ class Node(CleanSave, TimestampedModel):
                 pod_id=pod.id,
                 name=pod.name,
             )
-            d.addCallback(
-                lambda hints: (deferToDatabase(_save, self.id, pod.id, hints))
+            d.addBoth(
+                lambda result: (
+                    deferToDatabase(_save, self.id, pod.id, result)
+                )
             )
             d.addCallback(lambda _: request_commissioning_results(pod))
         else:
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index bd6d54d..e63ffa5 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -73,7 +73,6 @@ from maasserver.exceptions import (
     IPAddressCheckFailed,
     NetworkingResetProblem,
     NodeStateViolation,
-    PodProblem,
     PowerProblem,
     StaticIPAddressExhaustion,
 )
@@ -6055,14 +6054,18 @@ class TestDecomposeMachineTransactional(
         interface = transactional(reload_object)(interface)
         self.assertIsNone(interface)
 
-    def test_errors_raised_up(self):
+    def test_delete_doesnt_fail_removal(self):
+        mock_log_warning = self.patch(node_module.maaslog, "warning")
         pod, machine, hints, client = self.create_pod_machine_and_hints()
-        client.return_value = defer.fail(PodActionFail())
-        with ExpectedException(PodProblem):
-            with post_commit_hooks:
-                machine.delete()
-        machine = transactional(reload_object)(machine)
-        self.assertIsNotNone(machine)
+        client.return_value = defer.fail(PodActionFail("bang!"))
+        with post_commit_hooks:
+            machine.delete()
+        mock_log_warning.assert_called_with(
+            f"{machine.hostname}: Failure decomposing machine: "
+            "Unable to decompose machine because: bang!"
+        )
+        # the machine is still deleted
+        self.assertIsNone(transactional(reload_object)(machine))
 
     def test_release_deletes_dynamic_machine(self):
         owner = transactional(factory.make_User)()