Fix duplicate zone when creating ptr records

This fixes a race-condition when creating multiple PTR records
under the same zone. There is a brief window when creating two
identical zones can cause an error. This adds a fallback that
should prevent the error.

I also added a threaded test that caught multiple additional
bugs in this code.
- Wrong find_recordset used caused the wrong exception to be thrown.
- The transaction workflow would break error handling.

Change-Id: Ia1194ab838c52d5d91cb1d26c4556c73b4f3a745
(cherry picked from commit 0c7d218ba103e8260322e40f76a49a8c92556bfe)

3 files changed, 56 insertions, 18 deletions

diff --git a/designate/central/service.py b/designate/central/service.py
index 135bb341..e280f97e 100644
--- a/designate/central/service.py
+++ b/designate/central/service.py
@@ -2248,19 +2248,7 @@ class Service(service.RPCService):
                     'zonename': zone_name
                 })
 
-            email = cfg.CONF['service:central'].managed_resource_email
-            tenant_id = cfg.CONF['service:central'].managed_resource_tenant_id
-
-            zone_values = {
-                'type': 'PRIMARY',
-                'name': zone_name,
-                'email': email,
-                'tenant_id': tenant_id
-            }
-
-            zone = self.create_zone(
-                elevated_context, objects.Zone(**zone_values)
-            )
+            zone = self._create_ptr_zone(elevated_context, zone_name)
 
         record_name = self.network_api.address_name(fip['address'])
         recordset_values = {
@@ -2288,6 +2276,27 @@ class Service(service.RPCService):
             context, fip, record, zone=zone, recordset=recordset
         )
 
+    def _create_ptr_zone(self, elevated_context, zone_name):
+        zone_values = {
+            'type': 'PRIMARY',
+            'name': zone_name,
+            'email': cfg.CONF['service:central'].managed_resource_email,
+            'tenant_id': cfg.CONF['service:central'].managed_resource_tenant_id
+        }
+        try:
+            zone = self.create_zone(
+                elevated_context, objects.Zone(**zone_values)
+            )
+        except exceptions.DuplicateZone:
+            # NOTE(eandersson): This code is prone to race conditions, and
+            #                   it does not hurt to try to handle this if it
+            #                   fails.
+            zone = self.storage.find_zone(
+                elevated_context, {'name': zone_name}
+            )
+
+        return zone
+
     def _unset_floatingip_reverse(self, context, region, floatingip_id):
         """
         Unset the FloatingIP PTR record based on the
@@ -2368,6 +2377,7 @@ class Service(service.RPCService):
             fips.append(fip_ptr)
         return fips
 
+    @transaction
     def _delete_ptr_record(self, context, record):
         try:
             recordset = self.get_recordset(
@@ -2394,10 +2404,11 @@ class Service(service.RPCService):
         except exceptions.RecordSetNotFound:
             pass
 
+    @transaction
     def _replace_or_create_ptr_recordset(self, context, record, zone_id,
                                          name, type, ttl=None):
         try:
-            recordset = self.find_recordset(context, {
+            recordset = self.storage.find_recordset(context, {
                 'zone_id': zone_id,
                 'name': name,
                 'type': type,
@@ -2423,7 +2434,6 @@ class Service(service.RPCService):
         return recordset
 
     @rpc.expected_exceptions()
-    @transaction
     def update_floatingip(self, context, region, floatingip_id, values):
         """
         We strictly see if values['ptrdname'] is str or None and set / unset
diff --git a/designate/tests/__init__.py b/designate/tests/__init__.py
index 07bf510d..c71236dc 100644
--- a/designate/tests/__init__.py
+++ b/designate/tests/__init__.py
@@ -212,7 +212,11 @@ class TestCase(base.BaseTestCase):
 
     ptr_fixtures = [
         {'ptrdname': 'srv1.example.com.'},
-        {'ptrdname': 'srv1.example.net.'}
+        {'ptrdname': 'srv1.example.net.'},
+        {'ptrdname': 'srv2.example.com.'},
+        {'ptrdname': 'srv3.example.com.'},
+        {'ptrdname': 'srv4.example.com.'},
+        {'ptrdname': 'srv5.example.com.'},
     ]
 
     blacklist_fixtures = [{
diff --git a/designate/tests/test_central/test_service.py b/designate/tests/test_central/test_service.py
index 6961e3dd..4674242e 100644
--- a/designate/tests/test_central/test_service.py
+++ b/designate/tests/test_central/test_service.py
@@ -15,10 +15,12 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
-import datetime
+from collections import namedtuple
+from concurrent import futures
 import copy
+import datetime
+import futurist
 import random
-from collections import namedtuple
 from unittest import mock
 
 import testtools
@@ -2586,6 +2588,28 @@ class CentralServiceTest(CentralTestCase):
         self.assertIsNone(fip_ptr['description'])
         self.assertIsNotNone(fip_ptr['ttl'])
 
+    def test_set_floatingip_multiple_requests(self):
+        context = self.get_context()
+
+        def update_floatingip(fixture):
+            fip = self.network_api.fake.allocate_floatingip(context.project_id)
+            return self.central_service.update_floatingip(
+                context, fip['region'], fip['id'], fixture
+            )
+
+        with futurist.GreenThreadPoolExecutor() as executor:
+            results = []
+            for fixture in [0, 2, 3, 4, 5]:
+                results.append(executor.submit(
+                    update_floatingip, fixture=self.get_ptr_fixture(fixture)
+                ))
+            for future in futures.as_completed(results):
+                self.assertTrue(future.result())
+
+        fips = self.central_service.list_floatingips(context)
+
+        self.assertEqual(5, len(fips))
+
     def test_set_floatingip_no_managed_resource_tenant_id(self):
         context = self.get_context(project_id='a')