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

Proposed by Raphaël Badin on 2015-01-13
Status: Merged
Approved by: Raphaël Badin on 2015-01-13
Approved revision: 3335
Merged at revision: 3335
Proposed branch: lp:~rvb/maas/transaction-1.7-bug-1409852
Merge into: lp:maas/1.7
Diff against target: 74 lines (+33/-1)
2 files modified
src/maasserver/api/ip_addresses.py (+0/-1)
src/maasserver/api/tests/test_ipaddresses.py (+33/-0)
To merge this branch: bzr merge lp:~rvb/maas/transaction-1.7-bug-1409852
Reviewer Review Type Date Requested Status
Blake Rouse 2015-01-13 Approve on 2015-01-13
Review via email: mp+246268@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Blake Rouse (blake-rouse) wrote :

Looks good.

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

review: Approve
Raphaël Badin (rvb) wrote :

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

Indeed :)

Thanks for the review!

Raphaël Badin (rvb) wrote :

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

Basic QA would have caught the bug.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
62 def resource_uri(cls, *args, **kwargs):62 def resource_uri(cls, *args, **kwargs):
63 return ('ipaddresses_handler', [])63 return ('ipaddresses_handler', [])
6464
65 @transaction.atomic
66 def claim_ip(self, user, interface, requested_address, mac=None):65 def claim_ip(self, user, interface, requested_address, mac=None):
67 """Attempt to get a USER_RESERVED StaticIPAddress for `user` on66 """Attempt to get a USER_RESERVED StaticIPAddress for `user` on
68 `interface`.67 `interface`.
6968
=== 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 @@
18import json18import json
1919
20from django.core.urlresolvers import reverse20from django.core.urlresolvers import reverse
21from django.db import transaction
21from maasserver.api import ip_addresses as ip_addresses_module22from maasserver.api import ip_addresses as ip_addresses_module
22from maasserver.enum import (23from maasserver.enum import (
23 IPADDRESS_TYPE,24 IPADDRESS_TYPE,
@@ -27,11 +28,14 @@
27from maasserver.models.macaddress import MACAddress28from maasserver.models.macaddress import MACAddress
28from maasserver.testing.api import APITestCase29from maasserver.testing.api import APITestCase
29from maasserver.testing.factory import factory30from maasserver.testing.factory import factory
31from maasserver.testing.oauthclient import OAuthAuthenticatedClient
30from maasserver.testing.orm import reload_object32from maasserver.testing.orm import reload_object
33from maastesting.djangotestcase import TransactionTestCase
31from maastesting.matchers import MockCalledOnceWith34from maastesting.matchers import MockCalledOnceWith
32from netaddr import IPAddress35from netaddr import IPAddress
33from provisioningserver.rpc.exceptions import NoConnectionsAvailable36from provisioningserver.rpc.exceptions import NoConnectionsAvailable
34from testtools.matchers import (37from testtools.matchers import (
38 Contains,
35 Equals,39 Equals,
36 HasLength,40 HasLength,
37 Is,41 Is,
@@ -40,6 +44,35 @@
40from twisted.python.failure import Failure44from twisted.python.failure import Failure
4145
4246
47class TestNetworksAPITransaction(TransactionTestCase):
48
49 def test_transactional_reserve_creates_ipaddress(self):
50 # Reserving an address through the API method 'reserve' works
51 # with transaction management enabled (see bug 1409852 for details).
52 with transaction.atomic():
53 self.logged_in_user = factory.make_User(
54 username='test', password='test')
55 self.logged_in_user.is_superuser = True
56 self.logged_in_user.save()
57 self.client = OAuthAuthenticatedClient(self.logged_in_user)
58
59 interface = factory.make_NodeGroupInterface(
60 factory.make_NodeGroup(status=NODEGROUP_STATUS.ACCEPTED))
61 net = interface.network
62 params = {
63 'op': 'reserve',
64 'network': unicode(net),
65 }
66 response = self.client.post(reverse('ipaddresses_handler'), params)
67
68 self.assertEqual(httplib.OK, response.status_code, response.content)
69 [staticipaddress] = StaticIPAddress.objects.all()
70 self.assertThat(net, Contains(IPAddress(staticipaddress.ip)))
71 self.assertEqual(
72 IPADDRESS_TYPE.USER_RESERVED, staticipaddress.alloc_type)
73 self.assertEqual(self.logged_in_user, staticipaddress.user)
74
75
43class TestNetworksAPI(APITestCase):76class TestNetworksAPI(APITestCase):
4477
45 def make_interface(self, status=NODEGROUP_STATUS.ACCEPTED, **kwargs):78 def make_interface(self, status=NODEGROUP_STATUS.ACCEPTED, **kwargs):

Subscribers

People subscribed via source and target branches

to all changes: