From dc5ced25c61fc742c513abc03b92e2d8f8042d8c Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Wed, 9 Sep 2015 20:29:09 -0700 Subject: [PATCH] Don't expect meta attributes in object_compat that aren't in the db obj The object_compat decorator expects to get the Instance object with 'metadata' and 'system_metadata' attributes but if those aren't in the db instance dict object, Instance._from_db_object will fail with a KeyError. In Kilo this happens per refresh_instance_security_rules because in the compute API code, the instance passed to refresh_instance_security_rules comes from the call to get the security group(s) which joins on the instances column, but that doesn't join on the metadata/system_metadata fields for the instances. So when the instances get to object_compat in the compute manager and the db instance dict is converted to the Instance object, it expects fields that aren't in the dict and we get the KeyError. The refresh_instance_security_rules case is fixed in Liberty per commit 12fbe6f082ef9b70b89302e15daa12e851e507a7 - in that case the compute API passes Instance objects to the compute manager so object_compat doesn't have anything to do, _load_instance just sees instance_or_dict isn't a dict and ignores it. We're making this change since (1) it's an obviously wrong assumption in object_compat and should be fixed and (2) we need to backport this fix to stable/kilo since it's an upgrade impact for users there. Closes-Bug: #1484738 Conflicts: nova/tests/unit/compute/test_compute.py NOTE(mriedem): The conflict is due to the unit tests being moved in kilo, otherwise this is unchanged. Change-Id: I36a954c095a9aa35879200784dc18e35edf689e6 (cherry picked from commit 9369aab04e37b7818d49b00e65857be8b3564e9e) (cherry picked from commit 08d1153d3be9f8d59aa0acc03eedd45a1697ed7b) --- nova/compute/manager.py | 6 +++++- nova/tests/compute/test_compute.py | 21 +++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index bdc53338bd5a..2f0b5e7453da 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -407,6 +407,11 @@ def object_compat(function): def decorated_function(self, context, *args, **kwargs): def _load_instance(instance_or_dict): if isinstance(instance_or_dict, dict): + # try to get metadata and system_metadata for most cases but + # only attempt to load those if the db instance already has + # those fields joined + metas = [meta for meta in ('metadata', 'system_metadata') + if meta in instance_or_dict] instance = objects.Instance._from_db_object( context, objects.Instance(), instance_or_dict, expected_attrs=metas) @@ -414,7 +419,6 @@ def object_compat(function): return instance return instance_or_dict - metas = ['metadata', 'system_metadata'] try: kwargs['instance'] = _load_instance(kwargs['instance']) except KeyError: diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py index 040ec6bc3276..8237718bdac3 100644 --- a/nova/tests/compute/test_compute.py +++ b/nova/tests/compute/test_compute.py @@ -1352,6 +1352,24 @@ class ComputeTestCase(BaseTestCase): def test_fn(_self, context, instance): self.assertIsInstance(instance, objects.Instance) self.assertEqual(instance.uuid, db_inst['uuid']) + self.assertEqual(instance.metadata, db_inst['metadata']) + self.assertEqual(instance.system_metadata, + db_inst['system_metadata']) + test_fn(None, self.context, instance=db_inst) + + def test_object_compat_no_metas(self): + # Tests that we don't try to set metadata/system_metadata on the + # instance object using fields that aren't in the db object. + db_inst = fake_instance.fake_db_instance() + db_inst.pop('metadata', None) + db_inst.pop('system_metadata', None) + + @compute_manager.object_compat + def test_fn(_self, context, instance): + self.assertIsInstance(instance, objects.Instance) + self.assertEqual(instance.uuid, db_inst['uuid']) + self.assertNotIn('metadata', instance) + self.assertNotIn('system_metadata', instance) test_fn(None, self.context, instance=db_inst) def test_object_compat_more_positional_args(self): @@ -1361,6 +1379,9 @@ class ComputeTestCase(BaseTestCase): def test_fn(_self, context, instance, pos_arg_1, pos_arg_2): self.assertIsInstance(instance, objects.Instance) self.assertEqual(instance.uuid, db_inst['uuid']) + self.assertEqual(instance.metadata, db_inst['metadata']) + self.assertEqual(instance.system_metadata, + db_inst['system_metadata']) self.assertEqual(pos_arg_1, 'fake_pos_arg1') self.assertEqual(pos_arg_2, 'fake_pos_arg2')