From 8e7d795db5e614de6a3d8dd19ff26f869c8e9f9a Mon Sep 17 00:00:00 2001 From: Ruby Loo Date: Fri, 23 Jun 2017 01:00:42 -0400 Subject: [PATCH] Modifications for rolling upgrades This contains some changes to conversions of objects during a rolling upgrade. This changes objects.base.IronicObject.convert_to_version() to have a new parameter 'remove_unavail_fields': True (default) to remove fields that are unavailable in the target version -- for serialization/deserialization of objects. False to change unavailable fields as appropriate (for DB interactions). The reason for doing this is to make sure that during serialization (eg for RPC), that we don't include any object fields that are not supported. To make the code a bit more performant, we don't perform object conversions when the API service is serializing the objects for RPC to the conductor. This is because the conductor will always be running the same or a newer release as the API service. Change-Id: I6f77b24199412e3489dd6f3dcf0f51ed04c5c7c0 Partial-Bug: #1526283 --- ironic/common/rpc_service.py | 2 +- ironic/objects/base.py | 115 ++++++++++++++----- ironic/tests/unit/common/test_rpc_service.py | 2 +- ironic/tests/unit/objects/test_objects.py | 104 +++++++++++++---- 4 files changed, 172 insertions(+), 51 deletions(-) diff --git a/ironic/common/rpc_service.py b/ironic/common/rpc_service.py index 5a2aeacfe8..a2aebb56c4 100644 --- a/ironic/common/rpc_service.py +++ b/ironic/common/rpc_service.py @@ -46,7 +46,7 @@ class RPCService(service.Service): target = messaging.Target(topic=self.topic, server=self.host) endpoints = [self.manager] - serializer = objects_base.IronicObjectSerializer() + serializer = objects_base.IronicObjectSerializer(is_server=True) self.rpcserver = rpc.get_server(target, endpoints, serializer) self.rpcserver.start() diff --git a/ironic/objects/base.py b/ironic/objects/base.py index c7dd303c07..78dfedde98 100644 --- a/ironic/objects/base.py +++ b/ironic/objects/base.py @@ -80,26 +80,71 @@ class IronicObject(object_base.VersionedObject): self[field] != loaded_object[field]): self[field] = loaded_object[field] - def _convert_to_version(self, target_version): + def _convert_to_version(self, target_version, remove_unavail_fields=True): """Convert to the target version. - Subclasses should redefine this method, to do the conversion - of the object to the specified version. As a result of any - conversion, the object changes (self.obj_what_changed()) should - be retained. + Subclasses should redefine this method, to do the conversion of the + object to the target version. + + Convert the object to the target version. The target version may be + the same, older, or newer than the version of the object. This is + used for DB interactions as well as for serialization/deserialization. + + The remove_unavail_fields flag is used to distinguish these two cases: + + 1) For serialization/deserialization, we need to remove the unavailable + fields, because the service receiving the object may not know about + these fields. remove_unavail_fields is set to True in this case. + + 2) For DB interactions, we need to set the unavailable fields to their + appropriate values so that these fields are saved in the DB. (If + they are not set, the VersionedObject magic will not know to + save/update them to the DB.) remove_unavail_fields is set to False + in this case. :param target_version: the desired version of the object + :param remove_unavail_fields: True to remove fields that are + unavailable in the target version; set this to True when + (de)serializing. False to set the unavailable fields to appropriate + values; set this to False for DB interactions. """ pass - def convert_to_version(self, target_version): + def convert_to_version(self, target_version, remove_unavail_fields=True): """Convert this object to the target version. + Convert the object to the target version. The target version may be + the same, older, or newer than the version of the object. This is + used for DB interactions as well as for serialization/deserialization. + + The remove_unavail_fields flag is used to distinguish these two cases: + + 1) For serialization/deserialization, we need to remove the unavailable + fields, because the service receiving the object may not know about + these fields. remove_unavail_fields is set to True in this case. + + 2) For DB interactions, we need to set the unavailable fields to their + appropriate values so that these fields are saved in the DB. (If + they are not set, the VersionedObject magic will not know to + save/update them to the DB.) remove_unavail_fields is set to False + in this case. + _convert_to_version() does the actual work. :param target_version: the desired version of the object + :param remove_unavail_fields: True to remove fields that are + unavailable in the target version; set this to True when + (de)serializing. False to set the unavailable fields to appropriate + values; set this to False for DB interactions. """ - self._convert_to_version(target_version) + if self.VERSION != target_version: + self._convert_to_version( + target_version, remove_unavail_fields=remove_unavail_fields) + if remove_unavail_fields: + # NOTE(rloo): We changed the object, but don't keep track of + # any of these changes, since it is inaccurate anyway (because + # it doesn't keep track of any 'changed' unavailable fields). + self.obj_reset_changes() # NOTE(rloo): self.__class__.VERSION is the latest version that # is supported by this service. self.VERSION is the version of @@ -210,15 +255,9 @@ class IronicObject(object_base.VersionedObject): if db_version != obj.__class__.VERSION: # convert to the latest version - obj.convert_to_version(obj.__class__.VERSION) - if obj.get_target_version() == db_version: - # pinned, so no need to keep these changes (we'll end up - # converting back to db_version if obj is saved) - obj.obj_reset_changes() - else: - # keep these changes around because they are needed - # when/if saving to the DB in the latest version - pass + obj.VERSION = db_version + obj.convert_to_version(obj.__class__.VERSION, + remove_unavail_fields=False) return obj @@ -262,16 +301,14 @@ class IronicObject(object_base.VersionedObject): if target_version != self.VERSION: # Convert the object so we can save it in the target version. - self.convert_to_version(target_version) - db_version = target_version - else: - db_version = self.VERSION + self.convert_to_version(target_version, + remove_unavail_fields=False) changes = self.obj_get_changes() # NOTE(rloo): Since this object doesn't keep track of the version that # is saved in the DB and we don't want to make a DB call # just to find out, we always update 'version' in the DB. - changes['version'] = db_version + changes['version'] = self.VERSION return changes @@ -280,6 +317,16 @@ class IronicObjectSerializer(object_base.VersionedObjectSerializer): # Base class to use for object hydration OBJ_BASE_CLASS = IronicObject + def __init__(self, is_server=False): + """Initialization. + + :param is_server: True if the service using this Serializer is a + server (i.e. an ironic-conductor). Default is False for clients + (such as ironic-api). + """ + super(IronicObjectSerializer, self).__init__() + self.is_server = is_server + def _process_object(self, context, objprim): """Process the object. @@ -288,8 +335,8 @@ class IronicObjectSerializer(object_base.VersionedObjectSerializer): deserialization process converts them to Objects. This converts any IronicObjects to be in their latest versions, - so that the services (ironic-api and ironic-conductor) internally, - always deal objects in their latest versions. + so that internally, the services (ironic-api and ironic-conductor) + always deal with objects in their latest versions. :param objprim: a serialized entity that represents an object :returns: the deserialized Object @@ -299,7 +346,11 @@ class IronicObjectSerializer(object_base.VersionedObjectSerializer): context, objprim) if isinstance(obj, IronicObject): if obj.VERSION != obj.__class__.VERSION: - obj.convert_to_version(obj.__class__.VERSION) + # NOTE(rloo): if deserializing at API (client) side, + # we don't want any changes + obj.convert_to_version( + obj.__class__.VERSION, + remove_unavail_fields=not self.is_server) return obj def serialize_entity(self, context, entity): @@ -310,9 +361,14 @@ class IronicObjectSerializer(object_base.VersionedObjectSerializer): 'ironic_object.namespace', 'ironic_object.data', 'ironic_object.name', 'ironic_object.version', and 'ironic_object.changes'. - For IronicObjects, if the service (ironic-api or ironic-conductor) - is pinned, we want the object to be in the target/pinned version, - which is not necessarily the latest version of the object. + We assume that the client (ironic-API) is always talking to a + server (ironic-conductor) that is running the same or a newer + release than the client. The client doesn't need to downgrade + any IronicObjects when sending them over RPC. The server, on + the other hand, will need to do so if the server is pinned and + the target version of an IronicObject is older than the latest + version of that Object. + (Internally, the services deal with the latest versions of objects so we know that these objects are always in the latest versions.) @@ -322,13 +378,14 @@ class IronicObjectSerializer(object_base.VersionedObjectSerializer): :raises: ovo_exception.IncompatibleObjectVersion (via .get_target_version()) """ - if isinstance(entity, IronicObject): + if self.is_server and isinstance(entity, IronicObject): target_version = entity.get_target_version() if target_version != entity.VERSION: # NOTE(xek): If the version is pinned, target_version is an # older object version. We need to backport/convert to target # version before serialization. - entity.convert_to_version(target_version) + entity.convert_to_version(target_version, + remove_unavail_fields=True) return super(IronicObjectSerializer, self).serialize_entity( context, entity) diff --git a/ironic/tests/unit/common/test_rpc_service.py b/ironic/tests/unit/common/test_rpc_service.py index b1e0647306..755df4f81c 100644 --- a/ironic/tests/unit/common/test_rpc_service.py +++ b/ironic/tests/unit/common/test_rpc_service.py @@ -48,6 +48,6 @@ class TestRPCService(base.TestCase): mock_ctx.assert_called_once_with() mock_target.assert_called_once_with(topic=self.rpc_svc.topic, server="fake_host") - mock_ios.assert_called_once_with() + mock_ios.assert_called_once_with(is_server=True) mock_init_method.assert_called_once_with(self.rpc_svc.manager, mock_ctx.return_value) diff --git a/ironic/tests/unit/objects/test_objects.py b/ironic/tests/unit/objects/test_objects.py index 678c11ef0a..b4892b0163 100644 --- a/ironic/tests/unit/objects/test_objects.py +++ b/ironic/tests/unit/objects/test_objects.py @@ -90,11 +90,14 @@ class MyObj(base.IronicObject, object_base.VersionedObjectDictCompat): self.save() self.foo = 42 - def _convert_to_version(self, target_version): + def _convert_to_version(self, target_version, remove_unavail_fields=True): if target_version == '1.5': self.missing = 'foo' elif self.missing: - self.missing = '' + if remove_unavail_fields: + delattr(self, 'missing') + else: + self.missing = '' class MyObj2(object): @@ -381,18 +384,18 @@ class _TestObject(object): self._test_get_changes(target_version='1.4') def test_convert_to_version_same(self): - # missing is added + # no changes obj = MyObj(self.context) self.assertEqual('1.5', obj.VERSION) - obj.convert_to_version('1.5') + obj.convert_to_version('1.5', remove_unavail_fields=False) self.assertEqual('1.5', obj.VERSION) self.assertEqual(obj.__class__.VERSION, obj.VERSION) - self.assertEqual({'missing': 'foo'}, obj.obj_get_changes()) + self.assertEqual({}, obj.obj_get_changes()) def test_convert_to_version_new(self): obj = MyObj(self.context) obj.VERSION = '1.4' - obj.convert_to_version('1.5') + obj.convert_to_version('1.5', remove_unavail_fields=False) self.assertEqual('1.5', obj.VERSION) self.assertEqual(obj.__class__.VERSION, obj.VERSION) self.assertEqual({'missing': 'foo'}, obj.obj_get_changes()) @@ -401,7 +404,15 @@ class _TestObject(object): obj = MyObj(self.context) obj.missing = 'something' obj.obj_reset_changes() - obj.convert_to_version('1.4') + obj.convert_to_version('1.4', remove_unavail_fields=True) + self.assertEqual('1.4', obj.VERSION) + self.assertEqual({}, obj.obj_get_changes()) + + def test_convert_to_version_old_keep(self): + obj = MyObj(self.context) + obj.missing = 'something' + obj.obj_reset_changes() + obj.convert_to_version('1.4', remove_unavail_fields=False) self.assertEqual('1.4', obj.VERSION) self.assertEqual({'missing': ''}, obj.obj_get_changes()) @@ -809,10 +820,32 @@ class TestObjectSerializer(test_base.TestCase): @mock.patch.object(base.IronicObject, 'convert_to_version', autospec=True) @mock.patch.object(base.IronicObject, 'get_target_version', autospec=True) - def test_serialize_entity_unpinned(self, mock_version, mock_convert): + def test_serialize_entity_unpinned_api(self, mock_version, mock_convert): """Test single element serializer with no backport, unpinned.""" mock_version.return_value = MyObj.VERSION - serializer = base.IronicObjectSerializer() + serializer = base.IronicObjectSerializer(is_server=False) + obj = MyObj(self.context) + obj.foo = 1 + obj.bar = 'text' + obj.missing = 'textt' + primitive = serializer.serialize_entity(self.context, obj) + self.assertEqual('1.5', primitive['ironic_object.version']) + data = primitive['ironic_object.data'] + self.assertEqual(1, data['foo']) + self.assertEqual('text', data['bar']) + self.assertEqual('textt', data['missing']) + changes = primitive['ironic_object.changes'] + self.assertEqual(set(['foo', 'bar', 'missing']), set(changes)) + self.assertFalse(mock_version.called) + self.assertFalse(mock_convert.called) + + @mock.patch.object(base.IronicObject, 'convert_to_version', autospec=True) + @mock.patch.object(base.IronicObject, 'get_target_version', autospec=True) + def test_serialize_entity_unpinned_conductor(self, mock_version, + mock_convert): + """Test single element serializer with no backport, unpinned.""" + mock_version.return_value = MyObj.VERSION + serializer = base.IronicObjectSerializer(is_server=True) obj = MyObj(self.context) obj.foo = 1 obj.bar = 'text' @@ -829,11 +862,30 @@ class TestObjectSerializer(test_base.TestCase): self.assertFalse(mock_convert.called) @mock.patch.object(base.IronicObject, 'get_target_version', autospec=True) - def test_serialize_entity_pinned(self, mock_version): + def test_serialize_entity_pinned_api(self, mock_version): """Test single element serializer with backport to pinned version.""" mock_version.return_value = '1.4' - serializer = base.IronicObjectSerializer() + serializer = base.IronicObjectSerializer(is_server=False) + obj = MyObj(self.context) + obj.foo = 1 + obj.bar = 'text' + obj.missing = 'miss' + self.assertEqual('1.5', obj.VERSION) + primitive = serializer.serialize_entity(self.context, obj) + self.assertEqual('1.5', primitive['ironic_object.version']) + data = primitive['ironic_object.data'] + self.assertEqual(1, data['foo']) + self.assertEqual('text', data['bar']) + self.assertEqual('miss', data['missing']) + self.assertFalse(mock_version.called) + + @mock.patch.object(base.IronicObject, 'get_target_version', autospec=True) + def test_serialize_entity_pinned_conductor(self, mock_version): + """Test single element serializer with backport to pinned version.""" + mock_version.return_value = '1.4' + + serializer = base.IronicObjectSerializer(is_server=True) obj = MyObj(self.context) obj.foo = 1 obj.bar = 'text' @@ -844,9 +896,8 @@ class TestObjectSerializer(test_base.TestCase): data = primitive['ironic_object.data'] self.assertEqual(1, data['foo']) self.assertEqual('text', data['bar']) - self.assertEqual('', data['missing']) - changes = primitive['ironic_object.changes'] - self.assertEqual(set(['foo', 'bar', 'missing']), set(changes)) + self.assertNotIn('missing', data) + self.assertNotIn('ironic_object.changes', primitive) mock_version.assert_called_once_with(mock.ANY) @mock.patch.object(base.IronicObject, 'get_target_version', autospec=True) @@ -854,21 +905,21 @@ class TestObjectSerializer(test_base.TestCase): mock_version.side_effect = object_exception.InvalidTargetVersion( version='1.6') - serializer = base.IronicObjectSerializer() + serializer = base.IronicObjectSerializer(is_server=True) obj = MyObj(self.context) self.assertRaises(object_exception.InvalidTargetVersion, serializer.serialize_entity, self.context, obj) mock_version.assert_called_once_with(mock.ANY) @mock.patch.object(base.IronicObject, 'convert_to_version', autospec=True) - def test__process_object(self, mock_convert): + def _test__process_object(self, mock_convert, is_server=True): obj = MyObj(self.context) obj.foo = 1 obj.bar = 'text' obj.missing = 'miss' primitive = obj.obj_to_primitive() - serializer = base.IronicObjectSerializer() + serializer = base.IronicObjectSerializer(is_server=is_server) obj2 = serializer._process_object(self.context, primitive) self.assertEqual(obj.foo, obj2.foo) self.assertEqual(obj.bar, obj2.bar) @@ -876,8 +927,14 @@ class TestObjectSerializer(test_base.TestCase): self.assertEqual(obj.VERSION, obj2.VERSION) self.assertFalse(mock_convert.called) + def test__process_object_api(self): + self._test__process_object(is_server=False) + + def test__process_object_conductor(self): + self._test__process_object(is_server=True) + @mock.patch.object(base.IronicObject, 'convert_to_version', autospec=True) - def test__process_object_convert(self, mock_convert): + def _test__process_object_convert(self, is_server, mock_convert): obj = MyObj(self.context) obj.foo = 1 obj.bar = 'text' @@ -885,9 +942,16 @@ class TestObjectSerializer(test_base.TestCase): obj.VERSION = '1.4' primitive = obj.obj_to_primitive() - serializer = base.IronicObjectSerializer() + serializer = base.IronicObjectSerializer(is_server=is_server) serializer._process_object(self.context, primitive) - mock_convert.assert_called_once_with(mock.ANY, '1.5') + mock_convert.assert_called_once_with( + mock.ANY, '1.5', remove_unavail_fields=not is_server) + + def test__process_object_convert_api(self): + self._test__process_object_convert(False) + + def test__process_object_convert_conductor(self): + self._test__process_object_convert(True) class TestRegistry(test_base.TestCase):