From b17c5280e1b750ea5cdaf72ad81cf74675676d7c Mon Sep 17 00:00:00 2001 From: Zenghui Shi Date: Fri, 1 Jun 2018 16:49:09 +0800 Subject: [PATCH] BIOS Settings: add sync_node_setting sync_node_setting takes a list of bios settings as input and sorts out a tuple of lists of create, update, delete and nochange settings by comparing the given settings with node 'bios_settings' database table. This commit also modifies fake BIOS interface to use sync_node_setting for testing purpose. Change-Id: I831b3db8f4da24d88a81b4d85889f7fd6f83ffdb Story: #1712032 --- ironic/common/exception.py | 4 ++ ironic/db/api.py | 12 ++-- ironic/db/sqlalchemy/api.py | 16 ++++-- ironic/drivers/modules/fake.py | 36 ++++++++++-- ironic/objects/bios.py | 67 +++++++++++++++++++++- ironic/tests/unit/db/test_bios_settings.py | 45 +++++++++------ ironic/tests/unit/objects/test_bios.py | 53 ++++++++++++++++- 7 files changed, 193 insertions(+), 40 deletions(-) diff --git a/ironic/common/exception.py b/ironic/common/exception.py index 8ab107d9fe..ed6119a9da 100644 --- a/ironic/common/exception.py +++ b/ironic/common/exception.py @@ -785,3 +785,7 @@ class BIOSSettingAlreadyExists(Conflict): class BIOSSettingNotFound(NotFound): _msg_fmt = _("Node %(node)s doesn't have a BIOS setting '%(name)s'") + + +class BIOSSettingListNotFound(NotFound): + _msg_fmt = _("Node %(node)s doesn't have BIOS settings '%(names)s'") diff --git a/ironic/db/api.py b/ironic/db/api.py index 9d9b55d96a..aa6bf84909 100644 --- a/ironic/db/api.py +++ b/ironic/db/api.py @@ -1050,13 +1050,13 @@ class Connection(object): """ @abc.abstractmethod - def delete_bios_setting(self, node_id, name): - """Delete BIOS setting. + def delete_bios_setting_list(self, node_id, names): + """Delete a list of BIOS settings. :param node_id: The node id. - :param name: String containing name of bios setting to be deleted. + :param names: List of BIOS setting names to be deleted. :raises: NodeNotFound if the node is not found. - :raises: BIOSSettingNotFound if the bios setting is not found. + :raises: BIOSSettingNotFound if any of BIOS setting name is not found. """ @abc.abstractmethod @@ -1064,10 +1064,10 @@ class Connection(object): """Retrieve BIOS setting value. :param node_id: The node id. - :param name: String containing name of bios setting to be retrieved. + :param name: String containing name of BIOS setting to be retrieved. :returns: The BIOSSetting object. :raises: NodeNotFound if the node is not found. - :raises: BIOSSettingNotFound if the bios setting is not found. + :raises: BIOSSettingNotFound if the BIOS setting is not found. """ @abc.abstractmethod diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index 3677a4a6df..89fd03f0a5 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/ironic/db/sqlalchemy/api.py @@ -1431,14 +1431,18 @@ class Connection(api.Connection): return bios_settings @oslo_db_api.retry_on_deadlock - def delete_bios_setting(self, node_id, name): + def delete_bios_setting_list(self, node_id, names): self._check_node_exists(node_id) + missing_bios_settings = [] with _session_for_write(): - count = model_query(models.BIOSSetting).filter_by( - node_id=node_id, name=name).delete() - if count == 0: - raise exception.BIOSSettingNotFound( - node=node_id, name=name) + for name in names: + count = model_query(models.BIOSSetting).filter_by( + node_id=node_id, name=name).delete() + if count == 0: + missing_bios_settings.append(name) + if len(missing_bios_settings) > 0: + raise exception.BIOSSettingListNotFound( + node=node_id, names=','.join(missing_bios_settings)) def get_bios_setting(self, node_id, name): self._check_node_exists(node_id) diff --git a/ironic/drivers/modules/fake.py b/ironic/drivers/modules/fake.py index 0167845642..2391cba0a5 100644 --- a/ironic/drivers/modules/fake.py +++ b/ironic/drivers/modules/fake.py @@ -238,7 +238,7 @@ class FakeRAID(base.RAIDInterface): class FakeBIOS(base.BIOSInterface): - """Example implementation of simple BIOSInterface.""" + """Fake implementation of simple BIOSInterface.""" def get_properties(self): return {} @@ -247,13 +247,35 @@ class FakeBIOS(base.BIOSInterface): pass def apply_configuration(self, task, settings): + # Note: the implementation of apply_configuration in fake interface + # is just for testing purpose, for real driver implementation, please + # refer to develop doc at https://docs.openstack.org/ironic/latest/ + # contributor/bios_develop.html. node_id = task.node.id - try: - objects.BIOSSettingList.create(task.context, node_id, settings) - except exception.BIOSSettingAlreadyExists: - objects.BIOSSettingList.save(task.context, node_id, settings) + create_list, update_list, delete_list, nochange_list = ( + objects.BIOSSettingList.sync_node_setting(task.context, node_id, + settings)) + + if len(create_list) > 0: + objects.BIOSSettingList.create(task.context, node_id, create_list) + if len(update_list) > 0: + objects.BIOSSettingList.save(task.context, node_id, update_list) + if len(delete_list) > 0: + delete_names = [setting['name'] for setting in delete_list] + objects.BIOSSettingList.delete(task.context, node_id, + delete_names) + + # nochange_list is part of return of sync_node_setting and it might be + # useful to the drivers to give a message if no change is required + # during application of settings. + if len(nochange_list) > 0: + pass def factory_reset(self, task): + # Note: the implementation of factory_reset in fake interface is + # just for testing purpose, for real driver implementation, please + # refer to develop doc at https://docs.openstack.org/ironic/latest/ + # contributor/bios_develop.html. node_id = task.node.id setting_objs = objects.BIOSSettingList.get_by_node_id( task.context, node_id) @@ -261,6 +283,10 @@ class FakeBIOS(base.BIOSInterface): objects.BIOSSetting.delete(task.context, node_id, setting.name) def cache_bios_settings(self, task): + # Note: the implementation of cache_bios_settings in fake interface + # is just for testing purpose, for real driver implementation, please + # refer to develop doc at https://docs.openstack.org/ironic/latest/ + # contributor/bios_develop.html. pass diff --git a/ironic/objects/bios.py b/ironic/objects/bios.py index 1fd3d7e72f..c7f705ef7e 100644 --- a/ironic/objects/bios.py +++ b/ironic/objects/bios.py @@ -87,7 +87,7 @@ class BIOSSetting(base.IronicObject): :param context: Security context. :param node_id: The node id. - :param name: Bios setting name to be retrieved. + :param name: BIOS setting name to be retrieved. :raises: NodeNotFound if the node id is not found. :raises: BIOSSettingNotFound if the bios setting name is not found. :returns: A :class:'BIOSSetting' object. @@ -105,11 +105,11 @@ class BIOSSetting(base.IronicObject): :param context: Security context. :param node_id: The node id. - :param name: Bios setting name to be deleted. + :param name: BIOS setting name to be deleted. :raises: NodeNotFound if the node id is not found. :raises: BIOSSettingNotFound if the bios setting name is not found. """ - cls.dbapi.delete_bios_setting(node_id, name) + cls.dbapi.delete_bios_setting_list(node_id, [name]) @base.IronicObjectRegistry.register @@ -177,6 +177,22 @@ class BIOSSettingList(base.IronicObjectListBase, base.IronicObject): return object_base.obj_make_list( context, cls(), BIOSSetting, updated_setting_list) + # NOTE(xek): We don't want to enable RPC on this call just yet. Remotable + # methods can be used in the future to replace current explicit RPC calls. + # Implications of calling new remote procedures should be thought through. + # @object_base.remotable_classmethod + @classmethod + def delete(cls, context, node_id, names): + """Delete BIOS Settings based on node_id and names. + + :param context: Security context. + :param node_id: The node id. + :param names: List of BIOS setting names to be deleted. + :raises: NodeNotFound if the node id is not found. + :raises: BIOSSettingNotFound if any of BIOS setting fails to delete. + """ + cls.dbapi.delete_bios_setting_list(node_id, names) + # NOTE(xek): We don't want to enable RPC on this call just yet. Remotable # methods can be used in the future to replace current explicit RPC calls. # Implications of calling new remote procedures should be thought through. @@ -193,3 +209,48 @@ class BIOSSettingList(base.IronicObjectListBase, base.IronicObject): node_bios_setting = cls.dbapi.get_bios_setting_list(node_id) return object_base.obj_make_list( context, cls(), BIOSSetting, node_bios_setting) + + # NOTE(xek): We don't want to enable RPC on this call just yet. Remotable + # methods can be used in the future to replace current explicit RPC calls. + # Implications of calling new remote procedures should be thought through. + # @object_base.remotable_classmethod + @classmethod + def sync_node_setting(cls, context, node_id, settings): + """Returns lists of create/update/delete/unchanged settings. + + This method sync with 'bios_settings' database table and sorts + out four lists of create/update/delete/unchanged settings. + + :param context: Security context. + :param node_id: The node id. + :param settings: BIOS settings to be synced. + :returns: A 4-tuple of lists of BIOS settings to be created, + updated, deleted and unchanged. + """ + create_list = [] + update_list = [] + delete_list = [] + nochange_list = [] + current_settings_dict = {} + + given_setting_names = [setting['name'] for setting in settings] + current_settings = cls.get_by_node_id(context, node_id) + + for setting in current_settings: + current_settings_dict[setting.name] = setting.value + + for setting in settings: + if setting['name'] in current_settings_dict: + if setting['value'] != current_settings_dict[setting['name']]: + update_list.append(setting) + else: + nochange_list.append(setting) + else: + create_list.append(setting) + + for setting in current_settings: + if setting.name not in given_setting_names: + delete_list.append({'name': setting.name, + 'value': setting.value}) + + return (create_list, update_list, delete_list, nochange_list) diff --git a/ironic/tests/unit/db/test_bios_settings.py b/ironic/tests/unit/db/test_bios_settings.py index 5e68146599..684c307b27 100644 --- a/ironic/tests/unit/db/test_bios_settings.py +++ b/ironic/tests/unit/db/test_bios_settings.py @@ -58,24 +58,6 @@ class DbBIOSSettingTestCase(base.DbTestCase): self.dbapi.get_bios_setting_list, '456') - def test_delete_bios_setting(self): - db_utils.create_test_bios_setting(node_id=self.node.id) - self.dbapi.delete_bios_setting(self.node.id, 'virtualization') - self.assertRaises(exception.BIOSSettingNotFound, - self.dbapi.get_bios_setting, - self.node.id, 'virtualization') - - def test_delete_bios_setting_node_not_exist(self): - self.assertRaises(exception.NodeNotFound, - self.dbapi.delete_bios_setting, - '456', 'virtualization') - - def test_delete_bios_setting_setting_not_exist(self): - db_utils.create_test_bios_setting(node_id=self.node.id) - self.assertRaises(exception.BIOSSettingNotFound, - self.dbapi.delete_bios_setting, - self.node.id, 'hyperthread') - def test_create_bios_setting_list(self): settings = db_utils.get_test_bios_setting_setting_list() result = self.dbapi.create_bios_setting_list( @@ -121,3 +103,30 @@ class DbBIOSSettingTestCase(base.DbTestCase): self.assertRaises(exception.NodeNotFound, self.dbapi.update_bios_setting_list, '456', [], '1.0') + + def test_delete_bios_setting_list(self): + settings = db_utils.get_test_bios_setting_setting_list() + self.dbapi.create_bios_setting_list(self.node.id, settings, '1.0') + name_list = [setting['name'] for setting in settings] + self.dbapi.delete_bios_setting_list(self.node.id, name_list) + self.assertRaises(exception.BIOSSettingNotFound, + self.dbapi.get_bios_setting, + self.node.id, 'virtualization') + self.assertRaises(exception.BIOSSettingNotFound, + self.dbapi.get_bios_setting, + self.node.id, 'hyperthread') + self.assertRaises(exception.BIOSSettingNotFound, + self.dbapi.get_bios_setting, + self.node.id, 'numlock') + + def test_delete_bios_setting_list_node_not_exist(self): + self.assertRaises(exception.NodeNotFound, + self.dbapi.delete_bios_setting_list, + '456', ['virtualization']) + + def test_delete_bios_setting_list_setting_not_exist(self): + settings = db_utils.get_test_bios_setting_setting_list() + self.dbapi.create_bios_setting_list(self.node.id, settings, '1.0') + self.assertRaises(exception.BIOSSettingListNotFound, + self.dbapi.delete_bios_setting_list, + self.node.id, ['fake-bios-option']) diff --git a/ironic/tests/unit/objects/test_bios.py b/ironic/tests/unit/objects/test_bios.py index e1c601cc15..0d31ae4ec7 100644 --- a/ironic/tests/unit/objects/test_bios.py +++ b/ironic/tests/unit/objects/test_bios.py @@ -10,6 +10,8 @@ # License for the specific language governing permissions and limitations # under the License. +import types + import mock from ironic.common import context @@ -141,9 +143,56 @@ class TestBIOSSettingObject(db_base.DbTestCase, obj_utils.SchemasTestMixIn): self.assertEqual(bios_setting2['name'], bios_obj_list[1].name) self.assertEqual(bios_setting2['value'], bios_obj_list[1].value) - @mock.patch.object(dbapi.IMPL, 'delete_bios_setting', autospec=True) + @mock.patch.object(dbapi.IMPL, 'delete_bios_setting_list', autospec=True) def test_delete(self, mock_delete): objects.BIOSSetting.delete(self.context, self.node_id, self.bios_setting['name']) mock_delete.assert_called_once_with(self.node_id, - self.bios_setting['name']) + [self.bios_setting['name']]) + + @mock.patch.object(dbapi.IMPL, 'delete_bios_setting_list', autospec=True) + def test_list_delete(self, mock_delete): + bios_setting2 = db_utils.get_test_bios_setting(name='hyperthread') + name_list = [self.bios_setting['name'], bios_setting2['name']] + objects.BIOSSettingList.delete(self.context, self.node_id, name_list) + mock_delete.assert_called_once_with(self.node_id, name_list) + + @mock.patch('ironic.objects.bios.BIOSSettingList.get_by_node_id', + spec_set=types.FunctionType) + def test_sync_node_setting_create_and_update(self, mock_get): + node = obj_utils.create_test_node(self.ctxt) + bios_obj = [obj_utils.create_test_bios_setting( + self.ctxt, node_id=node.id)] + mock_get.return_value = bios_obj + settings = db_utils.get_test_bios_setting_setting_list() + settings[0]['value'] = 'off' + create, update, delete, nochange = ( + objects.BIOSSettingList.sync_node_setting(self.ctxt, node.id, + settings)) + + self.assertEqual(create, settings[1:]) + self.assertEqual(update, [settings[0]]) + self.assertEqual(delete, []) + self.assertEqual(nochange, []) + + @mock.patch('ironic.objects.bios.BIOSSettingList.get_by_node_id', + spec_set=types.FunctionType) + def test_sync_node_setting_delete_nochange(self, mock_get): + node = obj_utils.create_test_node(self.ctxt) + bios_obj_1 = obj_utils.create_test_bios_setting( + self.ctxt, node_id=node.id) + bios_obj_2 = obj_utils.create_test_bios_setting( + self.ctxt, node_id=node.id, name='numlock', value='off') + mock_get.return_value = [bios_obj_1, bios_obj_2] + settings = db_utils.get_test_bios_setting_setting_list() + settings[0]['name'] = 'fake-bios-option' + create, update, delete, nochange = ( + objects.BIOSSettingList.sync_node_setting(self.ctxt, node.id, + settings)) + + expected_delete = [{'name': bios_obj_1.name, + 'value': bios_obj_1.value}] + self.assertEqual(create, settings[:2]) + self.assertEqual(update, []) + self.assertEqual(delete, expected_delete) + self.assertEqual(nochange, [settings[2]])