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
1=== modified file 'src/maasserver/api/ip_addresses.py'
2--- src/maasserver/api/ip_addresses.py 2014-12-16 14:55:35 +0000
3+++ src/maasserver/api/ip_addresses.py 2015-01-13 09:54:55 +0000
4@@ -62,7 +62,6 @@
5 def resource_uri(cls, *args, **kwargs):
6 return ('ipaddresses_handler', [])
7
8- @transaction.atomic
9 def claim_ip(self, user, interface, requested_address, mac=None):
10 """Attempt to get a USER_RESERVED StaticIPAddress for `user` on
11 `interface`.
12
13=== modified file 'src/maasserver/api/tests/test_ipaddresses.py'
14--- src/maasserver/api/tests/test_ipaddresses.py 2014-12-04 12:33:26 +0000
15+++ src/maasserver/api/tests/test_ipaddresses.py 2015-01-13 09:54:55 +0000
16@@ -18,6 +18,7 @@
17 import json
18
19 from django.core.urlresolvers import reverse
20+from django.db import transaction
21 from maasserver.api import ip_addresses as ip_addresses_module
22 from maasserver.enum import (
23 IPADDRESS_TYPE,
24@@ -27,11 +28,14 @@
25 from maasserver.models.macaddress import MACAddress
26 from maasserver.testing.api import APITestCase
27 from maasserver.testing.factory import factory
28+from maasserver.testing.oauthclient import OAuthAuthenticatedClient
29 from maasserver.testing.orm import reload_object
30+from maastesting.djangotestcase import TransactionTestCase
31 from maastesting.matchers import MockCalledOnceWith
32 from netaddr import IPAddress
33 from provisioningserver.rpc.exceptions import NoConnectionsAvailable
34 from testtools.matchers import (
35+ Contains,
36 Equals,
37 HasLength,
38 Is,
39@@ -40,6 +44,35 @@
40 from twisted.python.failure import Failure
41
42
43+class TestNetworksAPITransaction(TransactionTestCase):
44+
45+ def test_transactional_reserve_creates_ipaddress(self):
46+ # Reserving an address through the API method 'reserve' works
47+ # with transaction management enabled (see bug 1409852 for details).
48+ with transaction.atomic():
49+ self.logged_in_user = factory.make_User(
50+ username='test', password='test')
51+ self.logged_in_user.is_superuser = True
52+ self.logged_in_user.save()
53+ self.client = OAuthAuthenticatedClient(self.logged_in_user)
54+
55+ interface = factory.make_NodeGroupInterface(
56+ factory.make_NodeGroup(status=NODEGROUP_STATUS.ACCEPTED))
57+ net = interface.network
58+ params = {
59+ 'op': 'reserve',
60+ 'network': unicode(net),
61+ }
62+ response = self.client.post(reverse('ipaddresses_handler'), params)
63+
64+ self.assertEqual(httplib.OK, response.status_code, response.content)
65+ [staticipaddress] = StaticIPAddress.objects.all()
66+ self.assertThat(net, Contains(IPAddress(staticipaddress.ip)))
67+ self.assertEqual(
68+ IPADDRESS_TYPE.USER_RESERVED, staticipaddress.alloc_type)
69+ self.assertEqual(self.logged_in_user, staticipaddress.user)
70+
71+
72 class TestNetworksAPI(APITestCase):
73
74 def make_interface(self, status=NODEGROUP_STATUS.ACCEPTED, **kwargs):

Subscribers

People subscribed via source and target branches

to all changes: