Merge lp:~julian-edwards/maas/1.7-race-for-staticip-bug-1387262 into lp:maas/1.7

[Julian Edwards]

Backport r3337: Use a lock in the critical section of the code that finds and records a new static IP address in the database. Some users have reported errors in the PG log file complaining about inserting duplicates, so there must be a race.

[Julian Edwards (julian-edwards)]

Approved for a backport by Andres earlier in the bug.

review: Approve

[Christian Reis (kiko)]

review: Needs Information

=== modified file 'src/maasserver/locks.py'
--- src/maasserver/locks.py	2014-10-20 22:01:33 +0000
+++ src/maasserver/locks.py	2014-11-06 01:25:00 +0000
@@ -43,3 +43,6 @@
 
 # Lock to prevent concurrent acquisition of nodes.
 node_acquire = DatabaseXactLock(7)
+
+# Lock to prevent concurrent allocation of StaticIPAddress
+staticip_acquire = DatabaseXactLock(8)

=== modified file 'src/maasserver/models/staticipaddress.py'
--- src/maasserver/models/staticipaddress.py	2014-09-08 19:35:59 +0000
+++ src/maasserver/models/staticipaddress.py	2014-11-06 01:25:00 +0000
@@ -35,7 +35,10 @@
     IntegerField,
     Manager,
     )
-from maasserver import DefaultMeta
+from maasserver import (
+    DefaultMeta,
+    locks,
+    )
 from maasserver.enum import IPADDRESS_TYPE
 from maasserver.exceptions import (
     StaticIPAddressExhaustion,
@@ -144,6 +147,22 @@
         static_range = IPRange(range_low, range_high)
 
         if requested_address is None:
+            return self._find_free_ip(
+                range_low, range_high, static_range, alloc_type, user)
+        else:
+            requested_address = IPAddress(requested_address)
+            if requested_address not in static_range:
+                raise StaticIPAddressOutOfRange(
+                    "%s is not inside the range %s to %s" % (
+                        requested_address.format(), range_low.format(),
+                        range_high.format()))
+            return self._attempt_allocation(
+                requested_address, alloc_type, user)
+
+    def _find_free_ip(self, range_low, range_high, static_range, alloc_type,
+                      user):
+        """Helper function that finds a free IP address using a lock."""
+        with locks.staticip_acquire:
             # The set of _allocated_ addresses in the range is going to be
             # smaller or at least no bigger than the set of addresses in the
             # whole range, so we materialise a Python set of only allocated
@@ -170,19 +189,11 @@
                     except StaticIPAddressUnavailable:
                         # That address has been taken since we obtained the
                         # list of existing addresses from the database. This
-                        # is a race!
+                        # is a race!  It also ought to be impossible as
+                        # this critical section is in a lock...
                         continue
             else:
                 raise StaticIPAddressExhaustion()
-        else:
-            requested_address = IPAddress(requested_address)
-            if requested_address not in static_range:
-                raise StaticIPAddressOutOfRange(
-                    "%s is not inside the range %s to %s" % (
-                        requested_address.format(), range_low.format(),
-                        range_high.format()))
-            return self._attempt_allocation(
-                requested_address, alloc_type, user)
 
     def _deallocate(self, filter):
         """Helper func to deallocate the records in the supplied queryset

=== modified file 'src/maasserver/models/tests/test_staticipaddress.py'
--- src/maasserver/models/tests/test_staticipaddress.py	2014-09-19 03:12:47 +0000
+++ src/maasserver/models/tests/test_staticipaddress.py	2014-11-06 01:25:00 +0000
@@ -16,6 +16,7 @@
 
 
 from django.core.exceptions import ValidationError
+from maasserver import locks
 from maasserver.enum import IPADDRESS_TYPE
 from maasserver.exceptions import (
     StaticIPAddressExhaustion,
@@ -25,6 +26,10 @@
 from maasserver.models.staticipaddress import StaticIPAddress
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
+from maastesting.matchers import (
+    MockCalledOnceWith,
+    MockNotCalled,
+    )
 from mock import sentinel
 from netaddr import (
     IPAddress,
@@ -105,6 +110,17 @@
             StaticIPAddressUnavailable, StaticIPAddress.objects.allocate_new,
             low, high, requested_address=requested_address)
 
+    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.
+        lock = self.patch(locks, 'staticip_acquire')
+        low, high = factory.make_ip_range()
+        requested_address = low + 1
+        ipaddress = StaticIPAddress.objects.allocate_new(
+            low, high, requested_address=requested_address)
+        self.assertIsInstance(ipaddress, StaticIPAddress)
+        self.assertThat(lock.__enter__, MockNotCalled())
+
     def test_allocate_new_raises_when_requested_IP_out_of_range(self):
         low, high = factory.make_ip_range()
         requested_address = low - 1
@@ -134,6 +150,15 @@
             "IP address type 12345 is not a member of IPADDRESS_TYPE.",
             unicode(error))
 
+    def test_allocate_new_uses_staticip_acquire_lock(self):
+        lock = self.patch(locks, 'staticip_acquire')
+        low, high = factory.make_ip_range()
+        ipaddress = StaticIPAddress.objects.allocate_new(low, high)
+        self.assertIsInstance(ipaddress, StaticIPAddress)
+        self.assertThat(lock.__enter__, MockCalledOnceWith())
+        self.assertThat(
+            lock.__exit__, MockCalledOnceWith(None, None, None))
+
     def test_deallocate_by_node_removes_addresses(self):
         node = factory.make_Node()
         [mac1, mac2] = [