diff --git a/nova/db/api.py b/nova/db/api.py index 11372bf2109a..28d937eeda6c 100644 --- a/nova/db/api.py +++ b/nova/db/api.py @@ -748,9 +748,19 @@ def instance_create(context, values): return IMPL.instance_create(context, values) -def instance_destroy(context, instance_uuid, constraint=None): - """Destroy the instance or raise if it does not exist.""" - return IMPL.instance_destroy(context, instance_uuid, constraint) +def instance_destroy(context, instance_uuid, constraint=None, + hard_delete=False): + """Destroy the instance or raise if it does not exist. + + :param context: request context object + :param instance_uuid: uuid of the instance to delete + :param constraint: a constraint object + :param hard_delete: when set to True, removes all records related to the + instance + """ + return IMPL.instance_destroy(context, instance_uuid, + constraint=constraint, + hard_delete=hard_delete) def instance_get_by_uuid(context, uuid, columns_to_join=None): diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index 5f81e32a33bd..0fcc6194df62 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -1751,7 +1751,8 @@ def instance_create(context, values): @require_context @oslo_db_api.wrap_db_retry(max_retries=5, retry_on_deadlock=True) @pick_context_manager_writer -def instance_destroy(context, instance_uuid, constraint=None): +def instance_destroy(context, instance_uuid, constraint=None, + hard_delete=False): if uuidutils.is_uuid_like(instance_uuid): instance_ref = _instance_get_by_uuid(context, instance_uuid) else: @@ -1761,37 +1762,31 @@ def instance_destroy(context, instance_uuid, constraint=None): filter_by(uuid=instance_uuid) if constraint is not None: query = constraint.apply(models.Instance, query) + # Either in hard or soft delete, we soft delete the instance first + # to make sure that that the constraints were met. count = query.soft_delete() if count == 0: raise exception.ConstraintNotMet() - model_query(context, models.SecurityGroupInstanceAssociation).\ - filter_by(instance_uuid=instance_uuid).\ - soft_delete() - model_query(context, models.InstanceInfoCache).\ - filter_by(instance_uuid=instance_uuid).\ - soft_delete() - model_query(context, models.InstanceMetadata).\ - filter_by(instance_uuid=instance_uuid).\ - soft_delete() - model_query(context, models.InstanceFault).\ - filter_by(instance_uuid=instance_uuid).\ - soft_delete() - model_query(context, models.InstanceExtra).\ - filter_by(instance_uuid=instance_uuid).\ - soft_delete() - model_query(context, models.InstanceSystemMetadata).\ - filter_by(instance_uuid=instance_uuid).\ - soft_delete() - model_query(context, models.BlockDeviceMapping).\ - filter_by(instance_uuid=instance_uuid).\ - soft_delete() - model_query(context, models.Migration).\ - filter_by(instance_uuid=instance_uuid).\ - soft_delete() - model_query(context, models.VirtualInterface).filter_by( - instance_uuid=instance_uuid).soft_delete() - model_query(context, models.InstanceIdMapping).filter_by( - uuid=instance_uuid).soft_delete() + + models_to_delete = [ + models.SecurityGroupInstanceAssociation, models.InstanceInfoCache, + models.InstanceMetadata, models.InstanceFault, models.InstanceExtra, + models.InstanceSystemMetadata, models.BlockDeviceMapping, + models.Migration, models.VirtualInterface + ] + + # For most referenced models we filter by the instance_uuid column, but for + # these models we filter by the uuid column. + filtered_by_uuid = [models.InstanceIdMapping] + + for model in models_to_delete + filtered_by_uuid: + key = 'instance_uuid' if model not in filtered_by_uuid else 'uuid' + filter_ = {key: instance_uuid} + if hard_delete: + model_query(context, model).filter_by(**filter_).delete() + else: + model_query(context, model).filter_by(**filter_).soft_delete() + # NOTE(snikitin): We can't use model_query here, because there is no # column 'deleted' in 'tags' or 'console_auth_tokens' tables. context.session.query(models.Tag).filter_by( @@ -1803,6 +1798,21 @@ def instance_destroy(context, instance_uuid, constraint=None): # can be used by operators to find out what actions were performed on a # deleted instance. Both of these tables are special-cased in # _archive_deleted_rows_for_table(). + if hard_delete: + # NOTE(ttsiousts): In case of hard delete, we need to remove the + # instance actions too since instance_uuid is a foreign key and + # for this we need to delete the corresponding InstanceActionEvents + actions = context.session.query(models.InstanceAction).filter_by( + instance_uuid=instance_uuid).all() + for action in actions: + context.session.query(models.InstanceActionEvent).filter_by( + action_id=action.id).delete() + context.session.query(models.InstanceAction).filter_by( + instance_uuid=instance_uuid).delete() + # NOTE(ttsiouts): The instance is the last thing to be deleted in + # order to respect all constraints + context.session.query(models.Instance).filter_by( + uuid=instance_uuid).delete() return instance_ref diff --git a/nova/objects/instance.py b/nova/objects/instance.py index 55ae2170be6e..542037d262d9 100644 --- a/nova/objects/instance.py +++ b/nova/objects/instance.py @@ -114,7 +114,8 @@ class Instance(base.NovaPersistentObject, base.NovaObject, # Version 2.2: Added keypairs # Version 2.3: Added device_metadata # Version 2.4: Added trusted_certs - VERSION = '2.4' + # Version 2.5: Added hard_delete kwarg in destroy + VERSION = '2.5' fields = { 'id': fields.IntegerField(), @@ -608,7 +609,7 @@ class Instance(base.NovaPersistentObject, base.NovaObject, self.obj_reset_changes(['ec2_ids']) @base.remotable - def destroy(self): + def destroy(self, hard_delete=False): if not self.obj_attr_is_set('id'): raise exception.ObjectActionError(action='destroy', reason='already destroyed') @@ -627,7 +628,8 @@ class Instance(base.NovaPersistentObject, base.NovaObject, try: db_inst = db.instance_destroy(self._context, self.uuid, - constraint=constraint) + constraint=constraint, + hard_delete=hard_delete) self._from_db_object(self._context, self, db_inst) except exception.ConstraintNotMet: raise exception.ObjectActionError(action='destroy', diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index 083aedbd43c2..36ba0ddcfea5 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -1189,7 +1189,8 @@ class _ComputeAPIUnitTestMixIn(object): inst, '%s.end' % delete_type)]) mock_deallocate.assert_called_once_with(self.context, inst) mock_inst_destroy.assert_called_once_with( - self.context, instance_uuid, constraint=None) + self.context, instance_uuid, constraint=None, + hard_delete=False) mock_get_inst.assert_called_with(self.context, instance_uuid) self.assertEqual(2, mock_get_inst.call_count) self.assertTrue(mock_get_inst.return_value.queued_for_delete) @@ -1440,7 +1441,8 @@ class _ComputeAPIUnitTestMixIn(object): mock_cons.assert_called_once_with(host=mock.ANY) mock_inst_destroy.assert_called_once_with( - self.context, instance_uuid, constraint='constraint') + self.context, instance_uuid, constraint='constraint', + hard_delete=False) def _fake_do_delete(context, instance, bdms, rservations=None, local=False): diff --git a/nova/tests/unit/db/test_db_api.py b/nova/tests/unit/db/test_db_api.py index eb6e494811b9..1d22932f9407 100644 --- a/nova/tests/unit/db/test_db_api.py +++ b/nova/tests/unit/db/test_db_api.py @@ -137,6 +137,24 @@ def _quota_create(context, project_id, user_id): user_id=user_id).hard_limit +@sqlalchemy_api.pick_context_manager_reader +def _assert_instance_id_mapping(_ctxt, tc, inst_uuid, expected_existing=False): + # NOTE(mriedem): We can't use ec2_instance_get_by_uuid to assert + # the instance_id_mappings record is gone because it hard-codes + # read_deleted='yes' and will read the soft-deleted record. So we + # do the model_query directly here. See bug 1061166. + inst_id_mapping = sqlalchemy_api.model_query( + _ctxt, models.InstanceIdMapping).filter_by(uuid=inst_uuid).first() + if not expected_existing: + tc.assertFalse(inst_id_mapping, + 'instance_id_mapping not deleted for ' + 'instance: %s' % inst_uuid) + else: + tc.assertTrue(inst_id_mapping, + 'instance_id_mapping not found for ' + 'instance: %s' % inst_uuid) + + class DbTestCase(test.TestCase): def setUp(self): super(DbTestCase, self).setUp() @@ -2948,19 +2966,7 @@ class InstanceTestCase(test.TestCase, ModelsObjectComparatorMixin): self.assertEqual([], db.instance_tag_get_by_instance_uuid( ctxt, inst_uuid)) - @sqlalchemy_api.pick_context_manager_reader - def _assert_instance_id_mapping(_ctxt): - # NOTE(mriedem): We can't use ec2_instance_get_by_uuid to assert - # the instance_id_mappings record is gone because it hard-codes - # read_deleted='yes' and will read the soft-deleted record. So we - # do the model_query directly here. See bug 1061166. - inst_id_mapping = sqlalchemy_api.model_query( - _ctxt, models.InstanceIdMapping).filter_by( - uuid=inst_uuid).first() - self.assertFalse(inst_id_mapping, - 'instance_id_mapping not deleted for ' - 'instance: %s' % inst_uuid) - _assert_instance_id_mapping(ctxt) + _assert_instance_id_mapping(ctxt, self, inst_uuid) ctxt.read_deleted = 'yes' self.assertEqual(values['system_metadata'], db.instance_system_metadata_get(ctxt, inst_uuid)) @@ -2972,6 +2978,125 @@ class InstanceTestCase(test.TestCase, ModelsObjectComparatorMixin): self.assertRaises(exception.InstanceNotFound, db.instance_destroy, ctxt, instance['uuid']) + def test_instance_destroy_hard(self): + ctxt = context.get_admin_context() + instance = self.create_instance_with_args() + uuid = instance['uuid'] + utc_now = timeutils.utcnow() + + action_values = { + 'action': 'run_instance', + 'instance_uuid': uuid, + 'request_id': ctxt.request_id, + 'user_id': ctxt.user_id, + 'project_id': ctxt.project_id, + 'start_time': utc_now, + 'updated_at': utc_now, + 'message': 'action-message' + } + action = db.action_start(ctxt, action_values) + + action_event_values = { + 'event': 'schedule', + 'action_id': action['id'], + 'instance_uuid': uuid, + 'start_time': utc_now, + 'request_id': ctxt.request_id, + 'host': 'fake-host', + } + db.action_event_start(ctxt, action_event_values) + + security_group_values = { + 'name': 'fake_sec_group', + 'user_id': ctxt.user_id, + 'project_id': ctxt.project_id, + 'instances': [] + } + security_group = db.security_group_create(ctxt, security_group_values) + db.instance_add_security_group(ctxt, uuid, security_group['id']) + + instance_fault_values = { + 'message': 'message', + 'details': 'detail', + 'instance_uuid': uuid, + 'code': 404, + 'host': 'localhost' + } + db.instance_fault_create(ctxt, instance_fault_values) + + bdm_values = { + 'instance_uuid': uuid, + 'device_name': 'fake_device', + 'source_type': 'volume', + 'destination_type': 'volume', + } + block_dev = block_device.BlockDeviceDict(bdm_values) + db.block_device_mapping_create(self.ctxt, block_dev, legacy=False) + + migration_values = { + "status": "finished", + "instance_uuid": uuid, + "dest_compute": "fake_host2" + } + db.migration_create(self.ctxt, migration_values) + + db.virtual_interface_create(ctxt, {'instance_uuid': uuid}) + + pool_values = { + 'address': '192.168.10.10', + 'username': 'user1', + 'password': 'passwd1', + 'console_type': 'type1', + 'public_hostname': 'public_host1', + 'host': 'host1', + 'compute_host': 'compute_host1', + } + console_pool = db.console_pool_create(ctxt, pool_values) + console_values = { + 'instance_name': instance['name'], + 'instance_uuid': uuid, + 'password': 'pass', + 'port': 7878, + 'pool_id': console_pool['id'] + } + db.console_create(self.ctxt, console_values) + + # Hard delete the instance + db.instance_destroy(ctxt, uuid, hard_delete=True) + + # Check that related records are deleted + with utils.temporary_mutation(ctxt, read_deleted="yes"): + # Assert that all information related to the instance is not found + # even using a context that can read soft deleted records. + self.assertEqual(0, len(db.actions_get(ctxt, uuid))) + self.assertEqual(0, len(db.action_events_get(ctxt, action['id']))) + db_sg = db.security_group_get_by_name( + ctxt, ctxt.project_id, security_group_values['name']) + self.assertEqual(0, len(db_sg['instances'])) + instance_faults = db.instance_fault_get_by_instance_uuids( + ctxt, [uuid]) + self.assertEqual(0, len(instance_faults[uuid])) + inst_bdms = db.block_device_mapping_get_all_by_instance(ctxt, uuid) + self.assertEqual(0, len(inst_bdms)) + filters = {"isntance_uuid": uuid} + inst_migrations = db.migration_get_all_by_filters(ctxt, filters) + self.assertEqual(0, len(inst_migrations)) + vifs = db.virtual_interface_get_by_instance(ctxt, uuid) + self.assertEqual(0, len(vifs)) + self.assertIsNone(db.instance_info_cache_get(ctxt, uuid)) + self.assertEqual({}, db.instance_metadata_get(ctxt, uuid)) + self.assertIsNone(db.instance_extra_get_by_instance_uuid( + ctxt, uuid)) + system_meta = db.instance_system_metadata_get(ctxt, uuid) + self.assertEqual({}, system_meta) + _assert_instance_id_mapping(ctxt, self, uuid) + self.assertRaises(exception.InstanceNotFound, + db.instance_destroy, ctxt, uuid) + # NOTE(ttsiouts): Should these also be valid? + # instance_consoles = db.console_get_all_by_instance(ctxt, uuid) + # self.assertEqual(0, len(instance_consoles)) + # Also FixedIp has the instance_uuid as a foreign key + def test_check_instance_exists(self): instance = self.create_instance_with_args() diff --git a/nova/tests/unit/objects/test_instance.py b/nova/tests/unit/objects/test_instance.py index 81a0db93a239..d11f6eaca41c 100644 --- a/nova/tests/unit/objects/test_instance.py +++ b/nova/tests/unit/objects/test_instance.py @@ -13,6 +13,7 @@ # under the License. import datetime +import six import mock import netaddr @@ -1234,7 +1235,8 @@ class _TestInstanceObject(object): timeutils.normalize_time(inst.deleted_at)) self.assertTrue(inst.deleted) mock_destroy.assert_called_once_with(self.context, uuids.instance, - constraint=None) + constraint=None, + hard_delete=False) def test_destroy(self): values = {'user_id': self.context.user_id, @@ -1281,6 +1283,29 @@ class _TestInstanceObject(object): inst.destroy() self.assertFalse(mock_destroy_at_top.called) + def test_destroy_hard(self): + values = {'user_id': self.context.user_id, + 'project_id': self.context.project_id} + db_inst = db.instance_create(self.context, values) + inst = objects.Instance(context=self.context, id=db_inst['id'], + uuid=db_inst['uuid']) + inst.destroy(hard_delete=True) + elevated = self.context.elevated(read_deleted="yes") + self.assertRaises(exception.InstanceNotFound, + objects.Instance.get_by_uuid, elevated, + db_inst['uuid']) + + def test_destroy_hard_host_constraint(self): + values = {'user_id': self.context.user_id, + 'project_id': self.context.project_id, + 'host': 'foo'} + db_inst = db.instance_create(self.context, values) + inst = objects.Instance.get_by_uuid(self.context, db_inst['uuid']) + inst.host = None + ex = self.assertRaises(exception.ObjectActionError, + inst.destroy, hard_delete=True) + self.assertIn('host changed', six.text_type(ex)) + def test_name_does_not_trigger_lazy_loads(self): values = {'user_id': self.context.user_id, 'project_id': self.context.project_id, diff --git a/nova/tests/unit/objects/test_objects.py b/nova/tests/unit/objects/test_objects.py index ba1a1e569160..e10d014650a7 100644 --- a/nova/tests/unit/objects/test_objects.py +++ b/nova/tests/unit/objects/test_objects.py @@ -1072,7 +1072,7 @@ object_data = { 'IDEDeviceBus': '1.0-29d4c9f27ac44197f01b6ac1b7e16502', 'ImageMeta': '1.8-642d1b2eb3e880a367f37d72dd76162d', 'ImageMetaProps': '1.21-f3721d8f744a9507a1966c81c386b532', - 'Instance': '2.4-4437eb8b2737c3054ea579b8efe31dc5', + 'Instance': '2.5-94e3881f0b9a06c2c3640e44816b51de', 'InstanceAction': '1.1-f9f293e526b66fca0d05c3b3a2d13914', 'InstanceActionEvent': '1.2-b2f368b8a29d8d872b1f6ea841e820a0', 'InstanceActionEventList': '1.1-13d92fb953030cdbfee56481756e02be',