Merge ~blake-rouse/maas:fix-1843493 into maas:master

[Blake Rouse]

Fixes LP: #1843493 - Handles serialization issues with saving composed machine information into the database.

[Newell Jensen (newell-jensen)]

LGTM

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b fix-1843493 lp:~blake-rouse/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6389/console
COMMIT: 80dae65319be4d28bd48fb2b326ea1259e8d57d2

review: Needs Fixing

[MAAS Lander (maas-lander)]

LANDING
-b fix-1843493 lp:~blake-rouse/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6391/consoleText

[MAAS Lander (maas-lander)]

LANDING
-b fix-1843493 lp:~blake-rouse/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6409/consoleText

[MAAS Lander (maas-lander)]

LANDING
-b fix-1843493 lp:~blake-rouse/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6414/consoleText

[MAAS Lander (maas-lander)]

UNIT TESTS
-b fix-1843493 lp:~blake-rouse/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6415/console
COMMIT: 850984c31a86fc99ce0595fe4862c3576dbcbbbe

review: Needs Fixing

[Pedro Guimarães (pguimaraes)]

The compose method in src/maasserver/forms/pods.py has a "if isInIOThread" contiditional. I've ran MP multiple times but I never seen MAAS going into that conditional:
https://github.com/maas/maas/blob/master/src/maasserver/forms/pods.py#L639

I've tried to force it, just to check what would happen: remove the if isInIOThread all together and left its conditional code to be ran (the else conditional + code was removed entirely)
So, I was not running within an IOThread, but used its code within if-conditional to give it a shot.

Eventually, MAAS fails with: https://pastebin.canonical.com/p/gBTPbMfBR8/

Indeed, end of "if isInIOThread" returns a Deferred object which we pass directly back to api/machines.py. I am not sure if the fact of running within an IOThread would change anything and Deferred would be processed on the spot. I guess not since Deferred processing demands reactor to be run to produce a result.

I imagine that, if that code ever runs, we're missing a decorator @asynchronous that would queue that Deferred for processing just like:
https://github.com/maas/maas/blob/master/src/provisioningserver/utils/twisted.py#L156

Unmerged commits

850984c... by Blake Rouse on 2019-09-13

Fix lint.

80dae65... by Blake Rouse on 2019-09-11

Update comment.

9022f83... by Blake Rouse on 2019-09-11

Fixes LP: #1843493 - Handles serialization issues with saving composed machine information into the database.

diff --git a/src/maasserver/forms/pods.py b/src/maasserver/forms/pods.py
index 1a68329..85c1da2 100644
--- a/src/maasserver/forms/pods.py
+++ b/src/maasserver/forms/pods.py
@@ -25,6 +25,7 @@ from maasserver.clusterrpc.driver_parameters import (
 )
 from maasserver.clusterrpc.pods import (
     compose_machine,
+    decompose_machine,
     discover_pod,
     get_best_discovered_result,
 )
@@ -59,7 +60,10 @@ from maasserver.node_constraint_filter_forms import (
 from maasserver.rpc import getClientFromIdentifiers
 from maasserver.utils.dns import validate_hostname
 from maasserver.utils.forms import set_form_error
-from maasserver.utils.orm import transactional
+from maasserver.utils.orm import (
+    post_commit_do,
+    transactional,
+)
 from maasserver.utils.threads import deferToDatabase
 import petname
 from provisioningserver.drivers import SETTING_SCOPE
@@ -76,6 +80,7 @@ from provisioningserver.enum import (
     MACVLAN_MODE_CHOICES,
 )
 from provisioningserver.logger import LegacyLogger
+from provisioningserver.rpc.exceptions import PodInvalidResources
 from provisioningserver.utils.network import get_ifname_for_label
 from provisioningserver.utils.twisted import asynchronous
 from twisted.internet.defer import inlineCallbacks
@@ -607,7 +612,7 @@ class ComposeMachineForm(forms.Form):
 
             return client, interfaces
 
-        def create_and_sync(result):
+        def create_and_sync(result, post_pod_sync=False):
             requested_machine, result = result
             discovered_machine, pod_hints = result
             created_machine = self.pod.create_machine(
@@ -620,7 +625,18 @@ class ComposeMachineForm(forms.Form):
                 pool=self.get_value_for('pool'),
                 zone=self.get_value_for('zone'),
             )
-            self.pod.sync_hints(pod_hints)
+            if post_pod_sync:
+                # Do not perform the hints syncing in this transaction as it
+                # can cause a transactional issue and we want to ensure that
+                # the machine is created and committed. Post commit update the
+                # pod hints as the current running transaction doesn't
+                # need that data for the resulting output (only for future
+                # requests).
+                post_commit_do(
+                    deferToDatabase,
+                    transactional(self.pod.sync_hints), pod_hints)
+            else:
+                self.pod.sync_hints(pod_hints)
             return created_machine
 
         @inlineCallbacks
@@ -663,11 +679,22 @@ class ComposeMachineForm(forms.Form):
                     pod_id=pod_id, name=name)
                 return d
 
+            @asynchronous
+            def wrap_decompose_machine(
+                    client_idents, pod_type, parameters, pod_id, name):
+                """Wrapper to get the client."""
+                d = getClientFromIdentifiers(client_idents)
+                d.addCallback(
+                    decompose_machine, pod_type, parameters,
+                    pod_id=pod_id, name=name)
+                return d
+
             _, result = db_work(None)
+            client_ids = self.pod.get_client_identifiers()
             try:
                 requested_machine = self.get_requested_machine(result)
                 result = wrap_compose_machine(
-                    self.pod.get_client_identifiers(),
+                    client_ids,
                     self.pod.power_type,
                     power_parameters,
                     requested_machine,
@@ -678,7 +705,19 @@ class ComposeMachineForm(forms.Form):
                     "Unable to compose a machine because '%s' driver "
                     "timed out after %d seconds." % (
                         self.pod.power_type, timeout))
-            return create_and_sync((requested_machine, result))
+            try:
+                return create_and_sync(
+                    (requested_machine, result), post_pod_sync=True)
+            except Exception:
+                # Issue saving the machine into the database. The composed
+                # machine must be decomposed or the MAAS information will be
+                # out of sync of the actual Pod.
+                discovered_machine, _ = result
+                power_parameters.update(discovered_machine.power_parameters)
+                wrap_decompose_machine(
+                    client_ids, self.pod.power_type, power_parameters,
+                    self.pod.id, self.pod.name).wait(timeout)
+                raise
 
 
 class ComposeMachineForPodsForm(forms.Form):
@@ -720,7 +759,7 @@ class ComposeMachineForPodsForm(forms.Form):
                 return form.compose(
                     skip_commissioning=True,
                     creation_type=NODE_CREATION_TYPE.DYNAMIC)
-            except Exception:
+            except (PodInvalidResources, PodProblem, ValidationError):
                 continue
         # Second, try to compose a machine from commitable pods
         for form in commit_forms:
@@ -728,7 +767,7 @@ class ComposeMachineForPodsForm(forms.Form):
                 return form.compose(
                     skip_commissioning=True,
                     creation_type=NODE_CREATION_TYPE.DYNAMIC)
-            except Exception:
+            except (PodInvalidResources, PodProblem, ValidationError):
                 continue
         # No machine found.
         return None
diff --git a/src/maasserver/forms/tests/test_pods.py b/src/maasserver/forms/tests/test_pods.py
index a49cc5d..5c3af2c 100644
--- a/src/maasserver/forms/tests/test_pods.py
+++ b/src/maasserver/forms/tests/test_pods.py
@@ -45,7 +45,10 @@ from maasserver.testing.testcase import (
     MAASServerTestCase,
     MAASTransactionServerTestCase,
 )
-from maasserver.utils.orm import reload_object
+from maasserver.utils.orm import (
+    post_commit_hooks,
+    reload_object,
+)
 from maasserver.utils.threads import deferToDatabase
 from maastesting.matchers import (
     MockCalledOnce,
@@ -70,6 +73,7 @@ from provisioningserver.enum import (
     MACVLAN_MODE,
     MACVLAN_MODE_CHOICES,
 )
+from provisioningserver.rpc.exceptions import PodInvalidResources
 from testtools import ExpectedException
 from testtools.matchers import (
     Equals,
@@ -1507,7 +1511,8 @@ class TestComposeMachineForm(MAASTransactionServerTestCase):
             'interfaces': interfaces,
         }, request=request, pod=pod)
         self.assertTrue(form.is_valid(), form.errors)
-        machine = form.compose()
+        with post_commit_hooks:
+            machine = form.compose()
         ip = StaticIPAddress.objects.filter(ip=expected_ip).first()
         self.assertThat(ip.get_interface().node, Equals(machine))
 
@@ -1531,7 +1536,8 @@ class TestComposeMachineForm(MAASTransactionServerTestCase):
 
         form = ComposeMachineForm(data={}, request=request, pod=pod)
         self.assertTrue(form.is_valid())
-        created_machine = form.compose()
+        with post_commit_hooks:
+            created_machine = form.compose()
         self.assertThat(created_machine, MatchesAll(
             IsInstance(Machine),
             MatchesStructure(
@@ -1560,7 +1566,8 @@ class TestComposeMachineForm(MAASTransactionServerTestCase):
 
         form = ComposeMachineForm(data={}, request=request, pod=pod)
         self.assertTrue(form.is_valid())
-        form.compose()
+        with post_commit_hooks:
+            form.compose()
         self.assertThat(mock_compose_machine, MockCalledOnceWith(
             ANY, pod.power_type, {
                 'power_address': ANY,
@@ -1570,6 +1577,45 @@ class TestComposeMachineForm(MAASTransactionServerTestCase):
                 make_known_host_interfaces(pod)),
             pod_id=pod.id, name=pod.name))
 
+    def test__compose_decomposes_on_failure(self):
+        request = MagicMock()
+        pod = make_pod_with_hints()
+
+        # Mock the RPC client.
+        client = MagicMock()
+        mock_getClient = self.patch(pods_module, "getClientFromIdentifiers")
+        mock_getClient.return_value = succeed(client)
+
+        # Mock the result of the composed machine.
+        composed_machine, pod_hints = self.make_compose_machine_result(pod)
+        mock_compose_machine = self.patch(pods_module, "compose_machine")
+        mock_compose_machine.return_value = succeed(
+            (composed_machine, pod_hints))
+
+        # Mock start_commissioning so it doesn't use post commit hooks.
+        self.patch(Machine, "start_commissioning")
+
+        # Mock Pod class causing the create_machine to raise an exception.
+        self.patch(Pod, 'create_machine').side_effect = (
+            factory.make_exception())
+
+        # Mock decompose to verify its called.
+        mock_decompose_machine = self.patch(pods_module, "decompose_machine")
+        mock_decompose_machine.return_value = succeed(None)
+
+        form = ComposeMachineForm(data={}, request=request, pod=pod)
+        self.assertTrue(form.is_valid())
+        self.assertRaises(Exception, form.compose)
+
+        power_params = pod.power_parameters
+        power_params['default_storage_pool_id'] = (
+            pod.default_storage_pool.pool_id)
+        self.assertThat(
+            mock_decompose_machine,
+            MockCalledOnceWith(
+                None, pod.power_type, power_params,
+                pod_id=pod.id, name=pod.name))
+
     def test__compose_duplicated_hostname(self):
         factory.make_Node(hostname='test')
 
@@ -1619,7 +1665,8 @@ class TestComposeMachineForm(MAASTransactionServerTestCase):
         form = ComposeMachineForm(
             data={"skip_commissioning": 'true'}, request=request, pod=pod)
         self.assertTrue(form.is_valid())
-        created_machine = form.compose()
+        with post_commit_hooks:
+            created_machine = form.compose()
         self.assertThat(created_machine, MatchesAll(
             IsInstance(Machine),
             MatchesStructure(
@@ -1648,7 +1695,8 @@ class TestComposeMachineForm(MAASTransactionServerTestCase):
 
         form = ComposeMachineForm(data={}, request=request, pod=pod)
         self.assertTrue(form.is_valid())
-        created_machine = form.compose(skip_commissioning=True)
+        with post_commit_hooks:
+            created_machine = form.compose(skip_commissioning=True)
         self.assertThat(created_machine, MatchesAll(
             IsInstance(Machine),
             MatchesStructure(
@@ -1680,7 +1728,8 @@ class TestComposeMachineForm(MAASTransactionServerTestCase):
             "skip_commissioning": 'true',
         }, request=request, pod=pod)
         self.assertTrue(form.is_valid())
-        created_machine = form.compose()
+        with post_commit_hooks:
+            created_machine = form.compose()
         self.assertThat(created_machine, MatchesAll(
             IsInstance(Machine),
             MatchesStructure(
@@ -1708,7 +1757,8 @@ class TestComposeMachineForm(MAASTransactionServerTestCase):
             "skip_commissioning": 'true',
         }, request=request, pod=pod)
         self.assertTrue(form.is_valid())
-        created_machine = form.compose()
+        with post_commit_hooks:
+            created_machine = form.compose()
         self.assertEqual(pool, created_machine.pool)
 
     def test__compose_uses_pod_pool(self):
@@ -1732,7 +1782,8 @@ class TestComposeMachineForm(MAASTransactionServerTestCase):
             "skip_commissioning": 'true',
         }, request=request, pod=pod)
         self.assertTrue(form.is_valid())
-        created_machine = form.compose()
+        with post_commit_hooks:
+            created_machine = form.compose()
         self.assertEqual(pod.pool, created_machine.pool)
 
     def test__compose_check_over_commit_ratios_raises_error_for_cores(self):
@@ -1909,7 +1960,11 @@ class TestComposeMachineForPodsForm(MAASServerTestCase):
         data = self.make_data(pods)
         form = ComposeMachineForPodsForm(request=request, data=data, pods=pods)
         mock_form_compose = self.patch(ComposeMachineForm, 'compose')
-        mock_form_compose.side_effect = [factory.make_exception(), None]
+        mock_form_compose.side_effect = [
+            factory.make_exception(
+                message=factory.make_name('error'),
+                bases=(PodInvalidResources, PodProblem, ValidationError)),
+            None]
         self.assertTrue(form.is_valid())
 
         form.compose()
@@ -1932,8 +1987,16 @@ class TestComposeMachineForPodsForm(MAASServerTestCase):
         form = ComposeMachineForPodsForm(request=request, data=data, pods=pods)
         mock_form_compose = self.patch(ComposeMachineForm, 'compose')
         mock_form_compose.side_effect = [
-            factory.make_exception(), factory.make_exception(),
-            factory.make_exception(), None]
+            factory.make_exception(
+                message=factory.make_name('error'),
+                bases=(PodInvalidResources, PodProblem, ValidationError)),
+            factory.make_exception(
+                message=factory.make_name('error'),
+                bases=(PodInvalidResources, PodProblem, ValidationError)),
+            factory.make_exception(
+                message=factory.make_name('error'),
+                bases=(PodInvalidResources, PodProblem, ValidationError)),
+            None]
         self.assertTrue(form.is_valid())
 
         form.compose()