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

[Julian Edwards]

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.

With reference to the attached bug, it looks like we need a lock around the section of code that acquires static IP addresses. At least, I can't see any other reason for errors like this in the PG log:

2014-10-28 15:40:35 UTC ERROR: duplicate key value violates unique constraint "maasserver_staticipaddress_ip_key"
2014-10-28 15:40:35 UTC DETAIL: Key (ip)=(10.245.0.201) already exists.
2014-10-28 15:40:35 UTC STATEMENT: INSERT INTO "maasserver_staticipaddress" ("created", "updated", "ip", "alloc_type", "user_id") VALUES ('2014-10-28 15:40:32.431728', '2014-10-28 15:40:32.431728', '10.245.0.201', 0, NULL) RETURNING "maasserver_staticipaddress"."id"

However the error that bubbles up to the appserver is a little odd (see the bug), you'd think it would be an IntegrityError. Maybe it's a consequence of the atomic context manager used in Node.claim_static_ip_addresses() ?

[Newell Jensen (newell-jensen)]

Approve. One question inline.

review: Approve

[Julian Edwards (julian-edwards)]

On Wednesday 05 Nov 2014 04:22:23 you wrote:
> I assume you are using two calls to assertThat instead of expectThat because
> if the lock was not acquired then you know it wouldn't have been released,
> correct? I.e., you can't get the second test to error unless the first one
> passes. This stood out to me and I had to ask because I know you love
> expectThat!

Correct :)

[Graham Binns (gmb)]

LGTM. I wondered for a second whether we needed a lock in _attempt_allocation(), too, but I see upon looking at the code that we already handle IntegrityErrors there.

Nice work that man.

review: Approve

=== modified file 'src/maasserver/locks.py'
--- src/maasserver/locks.py	2014-10-30 00:02:48 +0000
+++ src/maasserver/locks.py	2014-11-05 03:59:43 +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-10-21 11:35:32 +0000
+++ src/maasserver/models/staticipaddress.py	2014-11-05 03:59:43 +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,
@@ -146,6 +149,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
@@ -172,19 +191,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-05 03:59:43 +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] = [