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

Proposed by Alberto Donato
Status: Merged
Approved by: Alberto Donato
Approved revision: c75e353dffa2e1def3dc838952b32674f501747c
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~ack/maas:1887797-remove-machine-pod-error
Merge into: maas:master
Diff against target: 82 lines (+24/-12)
2 files modified
src/maasserver/models/node.py (+13/-4)
src/maasserver/models/tests/test_node.py (+11/-8)
Reviewer Review Type Date Requested Status
Adam Collard Approve
MAAS Lander Needs Fixing
Review via email: mp+397432@code.launchpad.net

Commit message

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

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

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
Revision history for this message
Adam Collard (adam-collard) :
review: Approve

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
2index 98a2635..105ac72 100644
3--- a/src/maasserver/models/node.py
4+++ b/src/maasserver/models/node.py
5@@ -2942,7 +2942,7 @@ class Node(CleanSave, TimestampedModel):
6 client_idents = pod.get_client_identifiers()
7
8 @transactional
9- def _save(machine_id, pod_id, hints):
10+ def _save(machine_id, pod_id, result):
11 from maasserver.models.bmc import Pod
12
13 machine = Machine.objects.filter(id=machine_id).first()
14@@ -2958,9 +2958,16 @@ class Node(CleanSave, TimestampedModel):
15 machine_id=machine_id
16 ).delete()
17 super(Node, machine).delete()
18+
19+ if isinstance(result, Failure):
20+ maaslog.warning(
21+ f"{self.hostname}: Failure decomposing machine: {result.value}"
22+ )
23+ return
24+
25 pod = Pod.objects.filter(id=pod_id).first()
26 if pod is not None:
27- pod.sync_hints(hints)
28+ pod.sync_hints(result)
29
30 maaslog.info("%s: Decomposing machine", self.hostname)
31
32@@ -2973,8 +2980,10 @@ class Node(CleanSave, TimestampedModel):
33 pod_id=pod.id,
34 name=pod.name,
35 )
36- d.addCallback(
37- lambda hints: (deferToDatabase(_save, self.id, pod.id, hints))
38+ d.addBoth(
39+ lambda result: (
40+ deferToDatabase(_save, self.id, pod.id, result)
41+ )
42 )
43 d.addCallback(lambda _: request_commissioning_results(pod))
44 else:
45diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
46index bd6d54d..e63ffa5 100644
47--- a/src/maasserver/models/tests/test_node.py
48+++ b/src/maasserver/models/tests/test_node.py
49@@ -73,7 +73,6 @@ from maasserver.exceptions import (
50 IPAddressCheckFailed,
51 NetworkingResetProblem,
52 NodeStateViolation,
53- PodProblem,
54 PowerProblem,
55 StaticIPAddressExhaustion,
56 )
57@@ -6055,14 +6054,18 @@ class TestDecomposeMachineTransactional(
58 interface = transactional(reload_object)(interface)
59 self.assertIsNone(interface)
60
61- def test_errors_raised_up(self):
62+ def test_delete_doesnt_fail_removal(self):
63+ mock_log_warning = self.patch(node_module.maaslog, "warning")
64 pod, machine, hints, client = self.create_pod_machine_and_hints()
65- client.return_value = defer.fail(PodActionFail())
66- with ExpectedException(PodProblem):
67- with post_commit_hooks:
68- machine.delete()
69- machine = transactional(reload_object)(machine)
70- self.assertIsNotNone(machine)
71+ client.return_value = defer.fail(PodActionFail("bang!"))
72+ with post_commit_hooks:
73+ machine.delete()
74+ mock_log_warning.assert_called_with(
75+ f"{machine.hostname}: Failure decomposing machine: "
76+ "Unable to decompose machine because: bang!"
77+ )
78+ # the machine is still deleted
79+ self.assertIsNone(transactional(reload_object)(machine))
80
81 def test_release_deletes_dynamic_machine(self):
82 owner = transactional(factory.make_User)()

Subscribers

People subscribed via source and target branches