Merge ~bjornt/maas:bug-1845459-kvm-pod-creation-failure into maas:master

[Björn Tillenius]

LP #1845459: Failure creating KVM pod in CI

Re-raise DatabaseErrors if form_pod.save() raises an exception.
The old code tried to mark the node as failed deployment, which failed
since the transaction was broken. Further more, the transaction wouldn't
be retried, which would have make the deploy succeed.

This is a stop-gap fix that improves the situation slightly. It's made
small so tha it can be backported to 2.6. The real fix is to refactor
the processing of the messages, so that marking the node as failed or
succeeded happens in a separate transaction.

[Björn Tillenius (bjornt)]

The test isn't create, but it's how the existing tests look like. I'd like to write better tests when we refactor the message processing instead.

I've confirmed that this works in CI. I can see a conflict happen, but the transaction now gets retried so the deployment succeeds.

[Alberto Donato (ack)]

+1!

Looks good, just a typo fix needed

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b bug-1845459-kvm-pod-creation-failure lp:~bjornt/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/6537/consoleText

[MAAS Lander (maas-lander)]

UNIT TESTS
-b bug-1845459-kvm-pod-creation-failure lp:~bjornt/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6538/console
COMMIT: 42b5a7637ba74be7aa58590fff446e60e6bfd84f

review: Needs Fixing

4ae7045... by Björn Tillenius on 2019-09-30

make format.

diff --git a/src/metadataserver/api_twisted.py b/src/metadataserver/api_twisted.py
index 7549fd0..b727cc7 100644
--- a/src/metadataserver/api_twisted.py
+++ b/src/metadataserver/api_twisted.py
@@ -9,6 +9,7 @@ from collections import defaultdict
 from datetime import datetime
 import json
 
+from django.db.utils import DatabaseError
 from maasserver.api.utils import extract_oauth_key_from_auth_header
 from maasserver.enum import (
     NODE_STATUS,
@@ -211,6 +212,15 @@ def _create_pod_for_deployment(node):
         if pod_form.is_valid():
             try:
                 pod_form.save()
+            except DatabaseError:
+                # Re-raise database errors, since we want it to be
+                # retried if possible. If it's not retriable, we
+                # couldn't mark the node as failed anyway, since the
+                # transaction will be broken.
+                # XXX: We should refactor the processing of messages so
+                # that the node is marked failed/deployed in a seperate
+                # transaction than the one doing the processing.
+                raise
             except Exception:
                 node.mark_failed(comment=POD_CREATION_ERROR, commit=False)
                 log.err(None, "Exception while saving pod form.")
diff --git a/src/metadataserver/tests/test_api_twisted.py b/src/metadataserver/tests/test_api_twisted.py
index 4c2add2..dce81a3 100644
--- a/src/metadataserver/tests/test_api_twisted.py
+++ b/src/metadataserver/tests/test_api_twisted.py
@@ -21,6 +21,7 @@ from unittest.mock import (
 )
 
 from crochet import wait_for
+from django.db.utils import DatabaseError
 from maasserver.enum import NODE_STATUS
 from maasserver.models import (
     Event,
@@ -1125,3 +1126,19 @@ class TestCreatePodForDeployment(MAASServerTestCase):
         self.assertThat(node.status, Equals(NODE_STATUS.FAILED_DEPLOYMENT))
         self.assertThat(node.error_description, DocTestMatches(
             POD_CREATION_ERROR))
+
+    def test__raises_if_save_raises_database_error(self):
+        mock_pod_form = Mock()
+        self.mock_PodForm.return_value = mock_pod_form
+        mock_pod_form.errors = {}
+        mock_pod_form.is_valid = Mock()
+        mock_pod_form.is_valid.return_value = True
+        mock_pod_form.save = Mock()
+        mock_pod_form.save.side_effect = DatabaseError('broken transaction')
+        node = factory.make_Node_with_Interface_on_Subnet(
+            status=NODE_STATUS.DEPLOYING, agent_name="maas-kvm-pod",
+            install_kvm=True)
+        factory.make_StaticIPAddress(interface=node.boot_interface)
+        NodeMetadata.objects.create(
+            node=node, key="virsh_password", value="xyz123")
+        self.assertRaises(DatabaseError, _create_pod_for_deployment, node)