Use ids when removing sink managed records

If the record status changes during the removal process,
the current implementation will fail.

Closes-Bug: 2015762
Change-Id: Iebe609e5f365d03e99f2a4580671175b4642763c

designate/notification_handler/base.py

@@ -99,26 +99,32 @@ class NotificationHandler(ExtensionPlugin):
         return recordset
 
     def _update_or_delete_recordset(self, context, zone_id, recordset_id,
-                                    record_to_delete):
+                                    record_to_delete_id):
         LOG.debug(
             'Deleting record in %s / %s',
-            zone_id, record_to_delete['id']
+            zone_id, record_to_delete_id
         )
-        try:
-            recordset = self.central_api.get_recordset(
-                context, zone_id, recordset_id
-            )
 
-            # Record not longer in recordset. Lets abort.
-            if record_to_delete not in recordset.records:
+        try:
+            recordset = self.central_api.find_recordset(
+                context, {'id': recordset_id, 'zone_id': zone_id}
+            )
+            record_ids = [record['id'] for record in recordset.records]
+
+            # Record no longer in recordset. Let's abort.
+            if record_to_delete_id not in record_ids:
                 LOG.debug(
                     'Record %s not found in recordset %s',
-                    record_to_delete['id'], recordset_id
+                    record_to_delete_id, recordset_id
                 )
                 return
 
             # Remove the record from the recordset.
-            recordset.records.remove(record_to_delete)
+            for record in list(recordset.records):
+                if record['id'] != record_to_delete_id:
+                    continue
+                recordset.records.remove(record)
+                break
 
             if not recordset.records:
                 # Recordset is now empty. Remove it.
@@ -133,7 +139,7 @@ class NotificationHandler(ExtensionPlugin):
         except exceptions.RecordSetNotFound:
             LOG.info(
                 'Recordset %s for record %s was already removed',
-                recordset_id, record_to_delete['id']
+                recordset_id, record_to_delete_id
             )
 
 
@@ -252,5 +258,5 @@ class BaseAddressHandler(NotificationHandler):
         records = self.central_api.find_records(context, criterion)
         for record in records:
             self._update_or_delete_recordset(
-                context, zone_id, record['recordset_id'], record
+                context, zone_id, record['recordset_id'], record['id']
             )

designate/tests/test_notification_handler/test_nova.py

@@ -18,7 +18,9 @@ from unittest import mock
 
 from oslo_log import log as logging
 
+from designate import exceptions
 from designate.notification_handler.nova import NovaFixedHandler
+from designate import objects
 from designate.tests.test_notification_handler import NotificationHandlerMixin
 from designate.tests import TestCase
 
@@ -147,6 +149,52 @@ class NovaFixedHandlerTest(TestCase, NotificationHandlerMixin):
             self.assertEqual('DELETE', record.action)
             self.assertEqual('172.16.0.14', record.data)
 
+    def test_instance_delete_start_record_status_changed(self):
+        start_event_type = 'compute.instance.create.end'
+        start_fixture = self.get_notification_fixture('nova', start_event_type)
+
+        self.plugin.process_notification(self.admin_context.to_dict(),
+                                         start_event_type,
+                                         start_fixture['payload'])
+
+        event_type = 'compute.instance.delete.start'
+        fixture = self.get_notification_fixture('nova', event_type)
+
+        self.assertIn(event_type, self.plugin.get_event_types())
+
+        criterion = {
+            'zone_id': self.zone_id,
+            'managed': True,
+            'managed_resource_type': 'instance',
+        }
+
+        records = self.central_service.find_records(self.admin_context,
+                                                    criterion)
+
+        self.assertEqual(2, len(records))
+
+        org_find_recordset = self.central_service.find_recordset
+
+        def mock_find_recordset(context, criterion):
+            results = org_find_recordset(context, criterion)
+            for r in results.records:
+                r.status = 'PENDING'
+            return results
+
+        with mock.patch.object(self.central_service, 'find_recordset',
+                               side_effect=mock_find_recordset):
+            self.plugin.process_notification(
+                self.admin_context.to_dict(), event_type, fixture['payload'])
+
+        records = self.central_service.find_records(self.admin_context,
+                                                    criterion)
+
+        self.assertEqual(2, len(records), records)
+
+        for record in records:
+            self.assertEqual('DELETE', record.action)
+            self.assertEqual('172.16.0.14', record.data)
+
     def test_instance_delete_one_with_multiple_records_with_same_name(self):
         # Prepare for the test
         for start_event_type in ['compute.instance.create.end',
@@ -221,6 +269,44 @@ class NovaFixedHandlerTest(TestCase, NotificationHandlerMixin):
 
         self.assertFalse(records)
 
+    def test_instance_delete_with_no_recordset(self):
+        start_event_type = 'compute.instance.create.end'
+        start_fixture = self.get_notification_fixture('nova', start_event_type)
+
+        self.plugin.process_notification(self.admin_context.to_dict(),
+                                         start_event_type,
+                                         start_fixture['payload'])
+
+        event_type = 'compute.instance.delete.start'
+        fixture = self.get_notification_fixture('nova', event_type)
+
+        # Make sure we don't fail here, even though there is nothing to
+        # do, since the recordset we are trying to delete does not actually
+        # exist.
+        with mock.patch.object(self.central_service, 'find_recordset',
+                               side_effect=exceptions.RecordSetNotFound):
+            self.plugin.process_notification(
+                self.admin_context.to_dict(), event_type, fixture['payload'])
+
+    def test_instance_delete_with_no_records_in_recordset(self):
+        start_event_type = 'compute.instance.create.end'
+        start_fixture = self.get_notification_fixture('nova', start_event_type)
+
+        self.plugin.process_notification(self.admin_context.to_dict(),
+                                         start_event_type,
+                                         start_fixture['payload'])
+
+        event_type = 'compute.instance.delete.start'
+        fixture = self.get_notification_fixture('nova', event_type)
+
+        # Make sure we don't fail here, even though there is nothing to
+        # do, since the recordset we are trying to delete contains no records.
+        with mock.patch.object(
+                self.central_service, 'find_recordset',
+                return_value=objects.RecordSet(records=objects.RecordList())):
+            self.plugin.process_notification(
+                self.admin_context.to_dict(), event_type, fixture['payload'])
+
     def test_label_in_format_v4_v6(self):
         event_type = 'compute.instance.create.end'
         self.config(formatv4=['%(label)s.example.com.'],

releasenotes/notes/bug-2015762-sink-delete-fails-intermittently-53168cf5cd830b59.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Fixed `bug 2015762`_ which could cause managed records to
+    occasionally fail to delete due to a race condition.
+
+    .. _Bug 2015762: https://bugs.launchpad.net/designate/+bug/2015762