diff options
| -rw-r--r-- | designate/central/service.py | 26 | ||||
| -rw-r--r-- | designate/notification_handler/base.py | 26 | ||||
| -rw-r--r-- | designate/tests/test_notification_handler/test_nova.py | 91 | ||||
| -rw-r--r-- | releasenotes/notes/bug-2015762-sink-delete-fails-intermittently-53168cf5cd830b59.yaml | 7 | ||||
| -rw-r--r-- | tox.ini | 2 |
5 files changed, 129 insertions, 23 deletions
diff --git a/designate/central/service.py b/designate/central/service.py index e8afa7c6..9df81db1 100644 --- a/designate/central/service.py +++ b/designate/central/service.py @@ -2147,7 +2147,8 @@ class Service(service.RPCService): LOG.debug('Deleting record %s for FIP %s', record['id'], record['managed_resource_id']) self._delete_ptr_record( - elevated_context, record + elevated_context, record.zone_id, record.recordset_id, + record['id'] ) def _list_floatingips(self, context, region=None): @@ -2321,7 +2322,8 @@ class Service(service.RPCService): raise exceptions.NotFound(msg) self._delete_ptr_record( - elevated_context, record + elevated_context, record.zone_id, record.recordset_id, + record['id'] ) def _create_floating_ip(self, context, fip, record, @@ -2381,24 +2383,30 @@ class Service(service.RPCService): return fips @transaction - def _delete_ptr_record(self, context, record): + def _delete_ptr_record(self, context, zone_id, recordset_id, + record_to_delete_id): try: - recordset = self.get_recordset( - context, record.zone_id, record.recordset_id + recordset = self.storage.find_recordset( + context, {'id': recordset_id, 'zone_id': zone_id} ) + record_ids = [record['id'] for record in recordset.records] - if record not in recordset.records: + if record_to_delete_id not in record_ids: LOG.debug( 'PTR Record %s not found in recordset %s', - record.id, record.recordset_id + record_to_delete_id, recordset_id ) return - recordset.records.remove(record) + for record in list(recordset.records): + if record['id'] != record_to_delete_id: + continue + recordset.records.remove(record) + break if not recordset.records: self.delete_recordset( - context, record.zone_id, record.recordset_id + context, zone_id, recordset_id ) return diff --git a/designate/notification_handler/base.py b/designate/notification_handler/base.py index a95c0145..8a02b651 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 4d973987..245a36da 100644 --- a/designate/tests/test_notification_handler/test_nova.py +++ b/designate/tests/test_notification_handler/test_nova.py @@ -18,10 +18,11 @@ from unittest import mock from oslo_log import log as logging -from designate.tests import TestCase +from designate import exceptions from designate.notification_handler.nova import NovaFixedHandler -from designate.tests.test_notification_handler import \ - NotificationHandlerMixin +from designate import objects +from designate.tests.test_notification_handler import NotificationHandlerMixin +from designate.tests import TestCase LOG = logging.getLogger(__name__) @@ -148,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', @@ -222,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 @@ -74,7 +74,7 @@ commands = deps = -r{toxinidir}/test-requirements.txt commands = bandit -r designate -n5 -x 'designate/tests/*' -t \ B505,B504,B503,B502,B501,B604,B605,B001,B601,B602,B701,B609,B702,\ - B608,B506,B312,B310,B411,B108,B103,B102,B309,B308,B302,B307,B306 + B608,B506,B312,B310,B411,B108,B103,B102,B308,B302,B307,B306 [testenv:debug] commands = oslo_debug_helper -t designate/tests {posargs} |