Merge lp:~rvb/maas/maas-ip-assign-bug-1438218 into lp:~maas-committers/maas/trunk

[Raphaël Badin]

When acquiring a static IP address before deployment, leverage the retry mechanism to grab another IP address if it's been snatched from under us by another transaction.

Without this change, the whole transaction would fail to grab a second IP address because it would have been broken by the first error encountered (the IntegrityError).

[Gavin Panella (allenap)]

Damn it... the whack-a-mole begins.

[Blake Rouse (blake-rouse)]

Looks good.

review: Approve

[Raphaël Badin (rvb)]

Thanks for the review Blake!

review: Approve

=== modified file 'src/maasserver/models/staticipaddress.py'
--- src/maasserver/models/staticipaddress.py	2015-03-26 00:12:12 +0000
+++ src/maasserver/models/staticipaddress.py	2015-03-31 15:49:59 +0000
@@ -54,6 +54,7 @@
     get_ip_based_hostname,
     validate_hostname,
 )
+from maasserver.utils.orm import make_serialization_failure
 from netaddr import (
     IPAddress,
     IPRange,
@@ -208,11 +209,10 @@
                             requested_address, alloc_type, user,
                             hostname=hostname)
                     except StaticIPAddressUnavailable:
-                        # That address has been taken since we obtained the
-                        # list of existing addresses from the database. This
-                        # is a race!  It also ought to be impossible as
-                        # this critical section is in a lock...
-                        continue
+                        # This is phantom read: another transaction has
+                        # taken this IP.  Raise a serialization failure to
+                        # let the retry mechanism do its thing.
+                        raise make_serialization_failure()
             else:
                 raise StaticIPAddressExhaustion(
                     "No more IPs available in range %s-%s" % (

=== modified file 'src/maasserver/models/tests/test_staticipaddress.py'
--- src/maasserver/models/tests/test_staticipaddress.py	2015-03-26 00:12:12 +0000
+++ src/maasserver/models/tests/test_staticipaddress.py	2015-03-31 15:49:59 +0000
@@ -32,6 +32,7 @@
     MockCalledOnceWith,
     MockNotCalled,
 )
+from maasserver.utils.orm import is_serialization_failure
 from mock import sentinel
 from netaddr import (
     IPAddress,
@@ -116,6 +117,17 @@
             StaticIPAddressUnavailable, StaticIPAddress.objects.allocate_new,
             low, high, requested_address=requested_address)
 
+    def test_allocate_new_raises_serialization_error_if_ip_taken(self):
+        low, high = factory.make_ip_range()
+        # Simulate a "IP already taken" error.
+        mock_attempt_allocation = self.patch(
+            StaticIPAddress.objects, '_attempt_allocation')
+        mock_attempt_allocation.side_effect = StaticIPAddressUnavailable()
+
+        error = self.assertRaises(
+            Exception, StaticIPAddress.objects.allocate_new, low, high)
+        self.assertTrue(is_serialization_failure(error))
+
     def test_allocate_new_does_not_use_lock_for_requested_ip(self):
         # When requesting a specific IP address, there's no need to
         # acquire the lock.

=== modified file 'src/maasserver/utils/orm.py'
--- src/maasserver/utils/orm.py	2015-03-26 00:12:12 +0000
+++ src/maasserver/utils/orm.py	2015-03-31 15:49:59 +0000
@@ -217,7 +217,6 @@
     set to `SERIALIZATION_FAILURE` without subclassing. I suspect only the C
     interface can do that.
     """
-
     pgcode = SERIALIZATION_FAILURE