From 2cb42ac9f6e5273ad64e697919b62e029466078f Mon Sep 17 00:00:00 2001 From: Erik Olof Gunnar Andersson Date: Tue, 4 Apr 2023 22:04:56 -0700 Subject: 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 | 26 ++++--- .../tests/test_notification_handler/test_nova.py | 86 ++++++++++++++++++++++ ...lete-fails-intermittently-53168cf5cd830b59.yaml | 7 ++ 3 files changed, 109 insertions(+), 10 deletions(-) create mode 100644 releasenotes/notes/bug-2015762-sink-delete-fails-intermittently-53168cf5cd830b59.yaml diff --git a/designate/notification_handler/base.py b/designate/notification_handler/base.py index 50a5ef41..8a270eb3 100644 --- a/designate/notification_handler/base.py +++ b/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 + recordset = self.central_api.find_recordset( + context, {'id': recordset_id, 'zone_id': zone_id} ) + record_ids = [record['id'] for record in recordset.records] - # Record not longer in recordset. Lets abort. - if record_to_delete not 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'] ) diff --git a/designate/tests/test_notification_handler/test_nova.py b/designate/tests/test_notification_handler/test_nova.py index 5254cb77..271b3b57 100644 --- a/designate/tests/test_notification_handler/test_nova.py +++ b/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.'], diff --git a/releasenotes/notes/bug-2015762-sink-delete-fails-intermittently-53168cf5cd830b59.yaml b/releasenotes/notes/bug-2015762-sink-delete-fails-intermittently-53168cf5cd830b59.yaml new file mode 100644 index 00000000..6f1984ea --- /dev/null +++ b/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 -- cgit v1.2.1