Merge lp:~rvb/maas/transaction-1.7-bug-1409852 into lp:maas/1.7

[Raphaël Badin]

Don't use transaction.atomic: 1.7 don't use manually configured atomic transactions.

The related bug is the result of a semantic conflict introduced by backporting r3391 (revision 3316 in 1.7): trunk uses transaction.atomic along with commit_within_atomic_block but commit_within_atomic_block doesn't exist in 1.7.

This was not detected in the tests because (mostly for performance reasons) the tests don't use a fully functional transaction environment unless you use a TransactionTestCase test.

[Blake Rouse (blake-rouse)]

Looks good.

Something the CI could have caught if it was testing static ip allocation!

review: Approve

[Raphaël Badin (rvb)]

> Something the CI could have caught if it was testing static ip allocation!

Indeed :)

Thanks for the review!

[Raphaël Badin (rvb)]

> > Something the CI could have caught if it was testing static ip allocation!
>
> Indeed :)

Basic QA would have caught the bug.

=== modified file 'src/maasserver/api/ip_addresses.py'
--- src/maasserver/api/ip_addresses.py	2014-12-16 14:55:35 +0000
+++ src/maasserver/api/ip_addresses.py	2015-01-13 09:54:55 +0000
@@ -62,7 +62,6 @@
     def resource_uri(cls, *args, **kwargs):
         return ('ipaddresses_handler', [])
 
-    @transaction.atomic
     def claim_ip(self, user, interface, requested_address, mac=None):
         """Attempt to get a USER_RESERVED StaticIPAddress for `user` on
         `interface`.
 
=== modified file 'src/maasserver/api/tests/test_ipaddresses.py'
--- src/maasserver/api/tests/test_ipaddresses.py	2014-12-04 12:33:26 +0000
+++ src/maasserver/api/tests/test_ipaddresses.py	2015-01-13 09:54:55 +0000
@@ -18,6 +18,7 @@
 import json
 
 from django.core.urlresolvers import reverse
+from django.db import transaction
 from maasserver.api import ip_addresses as ip_addresses_module
 from maasserver.enum import (
     IPADDRESS_TYPE,
@@ -27,11 +28,14 @@
 from maasserver.models.macaddress import MACAddress
 from maasserver.testing.api import APITestCase
 from maasserver.testing.factory import factory
+from maasserver.testing.oauthclient import OAuthAuthenticatedClient
 from maasserver.testing.orm import reload_object
+from maastesting.djangotestcase import TransactionTestCase
 from maastesting.matchers import MockCalledOnceWith
 from netaddr import IPAddress
 from provisioningserver.rpc.exceptions import NoConnectionsAvailable
 from testtools.matchers import (
+    Contains,
     Equals,
     HasLength,
     Is,
@@ -40,6 +44,35 @@
 from twisted.python.failure import Failure
 
 
+class TestNetworksAPITransaction(TransactionTestCase):
+
+    def test_transactional_reserve_creates_ipaddress(self):
+        # Reserving an address through the API method 'reserve' works
+        # with transaction management enabled (see bug 1409852 for details).
+        with transaction.atomic():
+            self.logged_in_user = factory.make_User(
+                username='test', password='test')
+            self.logged_in_user.is_superuser = True
+            self.logged_in_user.save()
+            self.client = OAuthAuthenticatedClient(self.logged_in_user)
+
+            interface = factory.make_NodeGroupInterface(
+                factory.make_NodeGroup(status=NODEGROUP_STATUS.ACCEPTED))
+            net = interface.network
+        params = {
+            'op': 'reserve',
+            'network': unicode(net),
+        }
+        response = self.client.post(reverse('ipaddresses_handler'), params)
+
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        [staticipaddress] = StaticIPAddress.objects.all()
+        self.assertThat(net, Contains(IPAddress(staticipaddress.ip)))
+        self.assertEqual(
+            IPADDRESS_TYPE.USER_RESERVED, staticipaddress.alloc_type)
+        self.assertEqual(self.logged_in_user, staticipaddress.user)
+
+
 class TestNetworksAPI(APITestCase):
 
     def make_interface(self, status=NODEGROUP_STATUS.ACCEPTED, **kwargs):