From 699bd410c7346a7fbe63616222eba94503ec6641 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Wed, 6 Feb 2019 18:57:38 +0100 Subject: [PATCH] [Refactor] Make caching BIOS settings explicit Currently we cache BIOS setting after calling apply_configuration or factory_reset implicitly via a wrapper added in BIOSInterface.__new__. It's confusing, and we did forget about this aspect when writing unit tests for the new iLO BIOS interface. This results in unsufficient mocking that breaks unit tests with proliantutils installed. This patch moves caching BIOS settings to an explicit decorator and fixes the mocking problem. Change-Id: I704eccea484b36cb5056fdb64d3702738c22c678 Story: #2004953 Task: #29375 --- ironic/drivers/base.py | 36 +++++++------------ ironic/drivers/modules/ilo/bios.py | 2 ++ ironic/drivers/modules/irmc/bios.py | 2 ++ ironic/drivers/modules/redfish/bios.py | 2 ++ .../unit/drivers/modules/ilo/test_bios.py | 12 +++++-- ironic/tests/unit/drivers/test_base.py | 2 ++ 6 files changed, 31 insertions(+), 25 deletions(-) diff --git a/ironic/drivers/base.py b/ironic/drivers/base.py index 980ea2c67a..963727c754 100644 --- a/ironic/drivers/base.py +++ b/ironic/drivers/base.py @@ -1004,32 +1004,22 @@ class InspectInterface(BaseInterface): driver=task.node.driver, extension='abort') +def cache_bios_settings(func): + """A decorator to cache bios settings after running the function. + + :param func: Function or method to wrap. + """ + @six.wraps(func) + def wrapped(self, task, *args, **kwargs): + result = func(self, task, *args, **kwargs) + self.cache_bios_settings(task) + return result + return wrapped + + class BIOSInterface(BaseInterface): interface_type = 'bios' - def __new__(cls, *args, **kwargs): - # Wrap the apply_configuration and factory_reset into a decorator - # which call cache_bios_settings() to update the node's BIOS setting - # table after apply_configuration and factory_reset have finished. - - super_new = super(BIOSInterface, cls).__new__ - instance = super_new(cls, *args, **kwargs) - - def wrapper(func): - @six.wraps(func) - def wrapped(task, *args, **kwargs): - result = func(task, *args, **kwargs) - instance.cache_bios_settings(task) - return result - return wrapped - - for n, method in inspect.getmembers(instance, inspect.ismethod): - if n == "apply_configuration": - instance.apply_configuration = wrapper(method) - elif n == "factory_reset": - instance.factory_reset = wrapper(method) - return instance - @abc.abstractmethod def apply_configuration(self, task, settings): """Validate & apply BIOS settings on the given node. diff --git a/ironic/drivers/modules/ilo/bios.py b/ironic/drivers/modules/ilo/bios.py index 74ccb503d5..d934684a84 100644 --- a/ironic/drivers/modules/ilo/bios.py +++ b/ironic/drivers/modules/ilo/bios.py @@ -154,6 +154,7 @@ class IloBIOS(base.BIOSInterface): 'required': True } }) + @base.cache_bios_settings def apply_configuration(self, task, settings): """Applies the provided configuration on the node. @@ -177,6 +178,7 @@ class IloBIOS(base.BIOSInterface): @METRICS.timer('IloBIOS.factory_reset') @base.clean_step(priority=0, abortable=False) + @base.cache_bios_settings def factory_reset(self, task): """Reset the BIOS settings to factory configuration. diff --git a/ironic/drivers/modules/irmc/bios.py b/ironic/drivers/modules/irmc/bios.py index 4300fc415f..55201b5d1a 100644 --- a/ironic/drivers/modules/irmc/bios.py +++ b/ironic/drivers/modules/irmc/bios.py @@ -61,6 +61,7 @@ class IRMCBIOS(base.BIOSInterface): 'required': True } }) + @base.cache_bios_settings def apply_configuration(self, task, settings): """Applies BIOS configuration on the given node. @@ -98,6 +99,7 @@ class IRMCBIOS(base.BIOSInterface): operation='Apply BIOS configuration', error=e) @METRICS.timer('IRMCBIOS.factory_reset') + @base.cache_bios_settings def factory_reset(self, task): """Reset BIOS configuration to factory default on the given node. diff --git a/ironic/drivers/modules/redfish/bios.py b/ironic/drivers/modules/redfish/bios.py index 53b5b35723..1a0eed9518 100644 --- a/ironic/drivers/modules/redfish/bios.py +++ b/ironic/drivers/modules/redfish/bios.py @@ -79,6 +79,7 @@ class RedfishBIOS(base.BIOSInterface): task.context, node_id, delete_names) @base.clean_step(priority=0) + @base.cache_bios_settings def factory_reset(self, task): """Reset the BIOS settings of the node to the factory default. @@ -110,6 +111,7 @@ class RedfishBIOS(base.BIOSInterface): 'required': True } }) + @base.cache_bios_settings def apply_configuration(self, task, settings): """Apply the BIOS settings to the node. diff --git a/ironic/tests/unit/drivers/modules/ilo/test_bios.py b/ironic/tests/unit/drivers/modules/ilo/test_bios.py index 0d0c12b665..026d035f0e 100644 --- a/ironic/tests/unit/drivers/modules/ilo/test_bios.py +++ b/ironic/tests/unit/drivers/modules/ilo/test_bios.py @@ -68,12 +68,15 @@ class IloBiosTestCase(test_common.BaseIloTest): called_method["name"].assert_called_once_with( *called_method["args"]) + @mock.patch.object(ilo_bios.IloBIOS, 'cache_bios_settings', + autospec=True) @mock.patch.object(ilo_bios.IloBIOS, '_execute_post_boot_bios_step', autospec=True) @mock.patch.object(ilo_bios.IloBIOS, '_execute_pre_boot_bios_step', autospec=True) def test_apply_configuration_pre_boot(self, exe_pre_boot_mock, - exe_post_boot_mock): + exe_post_boot_mock, + cache_settings_mock): settings = [ { "name": "SET_A", "value": "VAL_A", @@ -101,13 +104,17 @@ class IloBiosTestCase(test_common.BaseIloTest): exe_pre_boot_mock.assert_called_once_with( task.driver.bios, task, 'apply_configuration', actual_settings) self.assertFalse(exe_post_boot_mock.called) + cache_settings_mock.assert_called_once_with(task.driver.bios, task) + @mock.patch.object(ilo_bios.IloBIOS, 'cache_bios_settings', + autospec=True) @mock.patch.object(ilo_bios.IloBIOS, '_execute_post_boot_bios_step', autospec=True) @mock.patch.object(ilo_bios.IloBIOS, '_execute_pre_boot_bios_step', autospec=True) def test_apply_configuration_post_boot(self, exe_pre_boot_mock, - exe_post_boot_mock): + exe_post_boot_mock, + cache_settings_mock): settings = [ { "name": "SET_A", "value": "VAL_A", @@ -133,6 +140,7 @@ class IloBiosTestCase(test_common.BaseIloTest): exe_post_boot_mock.assert_called_once_with( task.driver.bios, task, 'apply_configuration') self.assertFalse(exe_pre_boot_mock.called) + cache_settings_mock.assert_called_once_with(task.driver.bios, task) @mock.patch.object(ilo_boot.IloVirtualMediaBoot, 'prepare_ramdisk', spec_set=True, autospec=True) diff --git a/ironic/tests/unit/drivers/test_base.py b/ironic/tests/unit/drivers/test_base.py index dca68ef8f4..398e032b4b 100644 --- a/ironic/tests/unit/drivers/test_base.py +++ b/ironic/tests/unit/drivers/test_base.py @@ -539,9 +539,11 @@ class MyBIOSInterface(driver_base.BIOSInterface): def validate(self, task): pass + @driver_base.cache_bios_settings def apply_configuration(self, task, settings): return "return_value_apply_configuration" + @driver_base.cache_bios_settings def factory_reset(self, task): return "return_value_factory_reset"