From c66679f14b0b5f34462a896c3bff5a9f483a9a83 Mon Sep 17 00:00:00 2001 From: Mark Goddard Date: Mon, 19 Feb 2018 13:29:45 +0000 Subject: [PATCH] Support nested objects and object lists in as_dict The value returned by ironic.objects.IronicObject.as_dict() should be a plain object, in order for it to be serialised to JSON. Currently, nested object fields and object list fields are not converted to dict format. This caused problems during cleaning, when the node object's as_dict representation is JSON encoded and sent to IPA. This change adds support for calling as_dict() on nested objects and list objects, to ensure these are also returned in dict form. We also change the method used in as_dict() for checking whether an object has an attribute. The hasattr() function used previously has problems when used with properties in python 2 [1], in that any exceptions raised in the property getter result in hasattr() returning False. Instead we use obj_attr_is_set() to determine whether the object has a particular attribute. [1] https://hynek.me/articles/hasattr/ Change-Id: Ib2166040508827db28d6f6e2d9a3e655c16f2993 Closes-Bug: #1750027 --- ironic/api/controllers/v1/node.py | 2 +- ironic/objects/base.py | 26 ++++++++++++-- ironic/objects/trait.py | 2 +- ironic/tests/unit/objects/test_node.py | 14 ++++++++ ironic/tests/unit/objects/test_objects.py | 34 +++++++++++++++++++ ironic/tests/unit/objects/test_trait.py | 12 +++++++ ...cleaning-with-traits-3a54faa70d594fd0.yaml | 7 ++++ 7 files changed, 93 insertions(+), 4 deletions(-) create mode 100644 releasenotes/notes/fix-cleaning-with-traits-3a54faa70d594fd0.yaml diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index a7da93a6cb..c32f483fa8 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -1051,7 +1051,7 @@ class Node(base.APIBase): self.fields.append(k) # TODO(jroll) is there a less hacky way to do this? if k == 'traits' and kwargs.get('traits') is not None: - value = kwargs['traits'].get_trait_names() + value = [t['trait'] for t in kwargs['traits']['objects']] else: value = kwargs.get(k, wtypes.Unset) setattr(self, k, value) diff --git a/ironic/objects/base.py b/ironic/objects/base.py index 9dda3a0c50..a246565e0f 100644 --- a/ironic/objects/base.py +++ b/ironic/objects/base.py @@ -64,9 +64,21 @@ class IronicObject(object_base.VersionedObject): } def as_dict(self): - return dict((k, getattr(self, k)) + """Return the object represented as a dict. + + The returned object is JSON-serialisable. + """ + + def _attr_as_dict(field): + """Return an attribute as a dict, handling nested objects.""" + attr = getattr(self, field) + if isinstance(attr, IronicObject): + attr = attr.as_dict() + return attr + + return dict((k, _attr_as_dict(k)) for k in self.fields - if hasattr(self, k)) + if self.obj_attr_is_set(k)) def obj_refresh(self, loaded_object): """Applies updates for objects that inherit from base.IronicObject. @@ -331,6 +343,16 @@ class IronicObject(object_base.VersionedObject): return changes +class IronicObjectListBase(object_base.ObjectListBase): + + def as_dict(self): + """Return the object represented as a dict. + + The returned object is JSON-serialisable. + """ + return {'objects': [obj.as_dict() for obj in self.objects]} + + class IronicObjectSerializer(object_base.VersionedObjectSerializer): # Base class to use for object hydration OBJ_BASE_CLASS = IronicObject diff --git a/ironic/objects/trait.py b/ironic/objects/trait.py index 7eb319d1b0..d7e6c4b376 100644 --- a/ironic/objects/trait.py +++ b/ironic/objects/trait.py @@ -98,7 +98,7 @@ class Trait(base.IronicObject): @base.IronicObjectRegistry.register -class TraitList(object_base.ObjectListBase, base.IronicObject): +class TraitList(base.IronicObjectListBase, base.IronicObject): # Version 1.0: Initial version VERSION = '1.0' diff --git a/ironic/tests/unit/objects/test_node.py b/ironic/tests/unit/objects/test_node.py index 3d39ca9461..903716dce1 100644 --- a/ironic/tests/unit/objects/test_node.py +++ b/ironic/tests/unit/objects/test_node.py @@ -16,6 +16,7 @@ import datetime import mock +from oslo_serialization import jsonutils from oslo_utils import uuidutils from testtools import matchers @@ -41,6 +42,8 @@ class TestNodeObject(db_base.DbTestCase, obj_utils.SchemasTestMixIn): d = self.node.as_dict() self.assertEqual('fake', d['driver_info']['ipmi_password']) self.assertEqual('data', d['instance_info']['configdrive']) + # Ensure the node can be serialised. + jsonutils.dumps(d) def test_as_dict_secure(self): self.node.driver_info['ipmi_password'] = 'fake' @@ -48,6 +51,17 @@ class TestNodeObject(db_base.DbTestCase, obj_utils.SchemasTestMixIn): d = self.node.as_dict(secure=True) self.assertEqual('******', d['driver_info']['ipmi_password']) self.assertEqual('******', d['instance_info']['configdrive']) + # Ensure the node can be serialised. + jsonutils.dumps(d) + + def test_as_dict_with_traits(self): + self.fake_node['traits'] = ['CUSTOM_1'] + self.node = obj_utils.get_test_node(self.ctxt, **self.fake_node) + d = self.node.as_dict() + expected_traits = {'objects': [{'trait': 'CUSTOM_1'}]} + self.assertEqual(expected_traits, d['traits']) + # Ensure the node can be serialised. + jsonutils.dumps(d) def test_get_by_id(self): node_id = self.fake_node['id'] diff --git a/ironic/tests/unit/objects/test_objects.py b/ironic/tests/unit/objects/test_objects.py index 4da76c7ea0..67d7d4ffd2 100644 --- a/ironic/tests/unit/objects/test_objects.py +++ b/ironic/tests/unit/objects/test_objects.py @@ -675,6 +675,40 @@ class _TestObject(object): self.assertIn("'TestObj' object does not support item assignment", err_message) + def test_as_dict(self): + obj = MyObj(self.context) + obj.foo = 1 + result = obj.as_dict() + expected = {'foo': 1} + self.assertEqual(expected, result) + + def test_as_dict_with_nested_object(self): + @base.IronicObjectRegistry.register_if(False) + class TestObj(base.IronicObject, + object_base.VersionedObjectDictCompat): + fields = {'my_obj': fields.ObjectField('MyObj')} + + obj1 = MyObj(self.context) + obj1.foo = 1 + obj2 = TestObj(self.context) + obj2.my_obj = obj1 + result = obj2.as_dict() + expected = {'my_obj': {'foo': 1}} + self.assertEqual(expected, result) + + def test_as_dict_with_nested_object_list(self): + @base.IronicObjectRegistry.register_if(False) + class TestObj(base.IronicObjectListBase, base.IronicObject): + fields = {'objects': fields.ListOfObjectsField('MyObj')} + + obj1 = MyObj(self.context) + obj1.foo = 1 + obj2 = TestObj(self.context) + obj2.objects = [obj1] + result = obj2.as_dict() + expected = {'objects': [{'foo': 1}]} + self.assertEqual(expected, result) + class TestObject(_LocalTest, _TestObject): pass diff --git a/ironic/tests/unit/objects/test_trait.py b/ironic/tests/unit/objects/test_trait.py index 4c7b337533..b64248af63 100644 --- a/ironic/tests/unit/objects/test_trait.py +++ b/ironic/tests/unit/objects/test_trait.py @@ -101,3 +101,15 @@ class TestTraitObject(db_base.DbTestCase, obj_utils.SchemasTestMixIn): result = trait_list.get_trait_names() self.assertEqual([self.fake_trait['trait']], result) + + def test_as_dict(self): + trait = objects.Trait(context=self.context, + node_id=self.fake_trait['node_id'], + trait=self.fake_trait['trait']) + trait_list = objects.TraitList(context=self.context, objects=[trait]) + + result = trait_list.as_dict() + + expected = {'objects': [{'node_id': self.fake_trait['node_id'], + 'trait': self.fake_trait['trait']}]} + self.assertEqual(expected, result) diff --git a/releasenotes/notes/fix-cleaning-with-traits-3a54faa70d594fd0.yaml b/releasenotes/notes/fix-cleaning-with-traits-3a54faa70d594fd0.yaml new file mode 100644 index 0000000000..41bdc8d9e7 --- /dev/null +++ b/releasenotes/notes/fix-cleaning-with-traits-3a54faa70d594fd0.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Fixes an issue seen during cleaning when the node being cleaned has one or + more traits assigned. This issue caused cleaning to fail, and the node to + enter the ``clean failed`` state. See `bug 1750027 + `_ for details.