From b7e9a64416ff239a4c1b8501f398796b02c46ce7 Mon Sep 17 00:00:00 2001 From: jichenjc Date: Sat, 19 Jul 2014 16:41:50 +0800 Subject: [PATCH] Let soft-deleted instance_system_metadata readable Currently, We can't delete instance system metadata after instance is deleted because of following reason: 1) We might have 'changed-since' request which will access the soft deleted system_metadata 2) We might has period audit function which also access the soft deleted system_metadata The root cause is the join function filtered the 'deleted' data of system metadata and it lead to ValueError later. Conceptually the system metadata is associated with instance, it will be used in instance's lifecycle, the data will only be updated so we won't have 2 copies or more system_metadata for the same instance. So allow to read it even if it's deleted is safe. This patch load the system_metadata from db when instance is in deleted state to avoid can't delete system_metadata issue. Co-Authored-By: Alex Xu Change-Id: Ifd8d2c91879834d3759aafdb036dac209a246d1b Partial-Bug: #1226049 --- nova/db/sqlalchemy/api.py | 16 +++++++++++++--- nova/db/sqlalchemy/models.py | 5 +---- nova/tests/unit/db/test_db_api.py | 3 +++ nova/utils.py | 12 +++++++----- 4 files changed, 24 insertions(+), 12 deletions(-) diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index be4ec4f6fd7b..d50be137532b 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -2469,8 +2469,17 @@ def _instance_metadata_update_in_place(context, instance, metadata_type, model, elif key not in metadata: to_delete.append(keyvalue) - for condemned in to_delete: - condemned.soft_delete(session=session) + # NOTE: we have to hard_delete here otherwise we will get more than one + # system_metadata record when we read deleted for an instance; + # regular metadata doesn't have the same problem because we don't + # allow reading deleted regular metadata anywhere. + if metadata_type == 'system_metadata': + for condemned in to_delete: + session.delete(condemned) + instance[metadata_type].remove(condemned) + else: + for condemned in to_delete: + condemned.soft_delete(session=session) for key, value in metadata.iteritems(): newitem = model() @@ -4992,7 +5001,8 @@ def _instance_system_metadata_get_multi(context, instance_uuids, if not instance_uuids: return [] return model_query(context, models.InstanceSystemMetadata, - session=session, use_slave=use_slave).\ + session=session, use_slave=use_slave, + read_deleted='yes').\ filter( models.InstanceSystemMetadata.instance_uuid.in_(instance_uuids)) diff --git a/nova/db/sqlalchemy/models.py b/nova/db/sqlalchemy/models.py index 2af86554cc61..253f5c6c76f7 100644 --- a/nova/db/sqlalchemy/models.py +++ b/nova/db/sqlalchemy/models.py @@ -978,11 +978,8 @@ class InstanceSystemMetadata(BASE, NovaBase): ForeignKey('instances.uuid'), nullable=False) - primary_join = ('and_(InstanceSystemMetadata.instance_uuid == ' - 'Instance.uuid, InstanceSystemMetadata.deleted == 0)') instance = orm.relationship(Instance, backref="system_metadata", - foreign_keys=instance_uuid, - primaryjoin=primary_join) + foreign_keys=instance_uuid) class InstanceTypeProjects(BASE, NovaBase): diff --git a/nova/tests/unit/db/test_db_api.py b/nova/tests/unit/db/test_db_api.py index 88319b5cacd5..6044d35427c7 100644 --- a/nova/tests/unit/db/test_db_api.py +++ b/nova/tests/unit/db/test_db_api.py @@ -2632,6 +2632,9 @@ class InstanceTestCase(test.TestCase, ModelsObjectComparatorMixin): set_and_check(meta) del meta['gigawatts'] set_and_check(meta) + self.ctxt.read_deleted = 'yes' + self.assertNotIn('gigawatts', + db.instance_system_metadata_get(self.ctxt, instance.uuid)) def test_security_group_in_use(self): db.instance_create(self.ctxt, dict(host='foo')) diff --git a/nova/utils.py b/nova/utils.py index af21f1572bd2..77900a57e2d1 100644 --- a/nova/utils.py +++ b/nova/utils.py @@ -828,11 +828,12 @@ def last_bytes(file_like_object, num): return (file_like_object.read(), remaining) -def metadata_to_dict(metadata): +def metadata_to_dict(metadata, filter_deleted=False): result = {} for item in metadata: - if not item.get('deleted'): - result[item['key']] = item['value'] + if not filter_deleted and item.get('deleted'): + continue + result[item['key']] = item['value'] return result @@ -856,7 +857,8 @@ def instance_sys_meta(instance): if isinstance(instance['system_metadata'], dict): return instance['system_metadata'] else: - return metadata_to_dict(instance['system_metadata']) + return metadata_to_dict(instance['system_metadata'], + filter_deleted=True) def get_wrapped_function(function): @@ -1103,7 +1105,7 @@ def get_image_from_system_metadata(system_meta): properties = {} if not isinstance(system_meta, dict): - system_meta = metadata_to_dict(system_meta) + system_meta = metadata_to_dict(system_meta, filter_deleted=True) for key, value in six.iteritems(system_meta): if value is None: