From 869b86e0c3edc0d285502d1a9c2cb4c76d4c7e2c Mon Sep 17 00:00:00 2001 From: Yolanda Robla Date: Thu, 26 Jul 2018 17:47:15 +0200 Subject: [PATCH] Add functionality for individual cleaning on nodes Create the object for automated clean and add the logic in the conductor to be able to enable clean for specific nodes, when general automated clean is disabled. Story: #2002161 Task: #24579 Change-Id: If0130082e16d1205fdf65d083854ef9849754f8b --- ironic/common/release_mappings.py | 2 +- ironic/conductor/manager.py | 2 +- ironic/conductor/utils.py | 8 ++ ironic/drivers/modules/oneview/deploy.py | 10 +- ironic/objects/node.py | 73 +++++------- ironic/tests/unit/api/utils.py | 2 + ironic/tests/unit/conductor/test_manager.py | 122 ++++++++++++++++++++ ironic/tests/unit/db/utils.py | 1 + ironic/tests/unit/objects/test_node.py | 63 ++++++++++ ironic/tests/unit/objects/test_objects.py | 2 +- 10 files changed, 238 insertions(+), 47 deletions(-) diff --git a/ironic/common/release_mappings.py b/ironic/common/release_mappings.py index 52396bb833..477e33b73e 100644 --- a/ironic/common/release_mappings.py +++ b/ironic/common/release_mappings.py @@ -134,7 +134,7 @@ RELEASE_MAPPING = { 'api': '1.46', 'rpc': '1.47', 'objects': { - 'Node': ['1.27'], + 'Node': ['1.28'], 'Conductor': ['1.3'], 'Chassis': ['1.3'], 'Port': ['1.8'], diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 9bcf838b82..0741aafa0f 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -1200,7 +1200,7 @@ class ConductorManager(base_manager.BaseConductorManager): LOG.debug('Starting %(type)s cleaning for node %(node)s', {'type': clean_type, 'node': node.uuid}) - if not manual_clean and not CONF.conductor.automated_clean: + if not manual_clean and utils.skip_automated_cleaning(node): # Skip cleaning, move to AVAILABLE. node.clean_step = None node.save() diff --git a/ironic/conductor/utils.py b/ironic/conductor/utils.py index 780f676cd1..7583e71414 100644 --- a/ironic/conductor/utils.py +++ b/ironic/conductor/utils.py @@ -990,3 +990,11 @@ def notify_conductor_resume_clean(task): def notify_conductor_resume_deploy(task): _notify_conductor_resume_operation(task, 'deploying', 'continue_node_deploy') + + +def skip_automated_cleaning(node): + """Checks if node cleaning needs to be skipped for an specific node. + + :param node: the node to consider + """ + return not CONF.conductor.automated_clean and not node.automated_clean diff --git a/ironic/drivers/modules/oneview/deploy.py b/ironic/drivers/modules/oneview/deploy.py index 71a3cb8a7d..38c1ed4329 100644 --- a/ironic/drivers/modules/oneview/deploy.py +++ b/ironic/drivers/modules/oneview/deploy.py @@ -22,6 +22,7 @@ import six from ironic.common import exception from ironic.common import states +from ironic.conductor import utils from ironic.conf import CONF from ironic.drivers.modules import agent from ironic.drivers.modules import iscsi_deploy @@ -241,7 +242,10 @@ class OneViewIscsiDeploy(iscsi_deploy.ISCSIDeploy, OneViewPeriodicTasks): @METRICS.timer('OneViewIscsiDeploy.tear_down') def tear_down(self, task): - if not CONF.conductor.automated_clean: + # teardown if automated clean is disabled on the node + # or if general automated clean is not enabled generally + # and not on the node as well + if utils.skip_automated_cleaning(task.node): deploy_utils.tear_down(task) return super(OneViewIscsiDeploy, self).tear_down(task) @@ -285,7 +289,9 @@ class OneViewAgentDeploy(agent.AgentDeploy, OneViewPeriodicTasks): @METRICS.timer('OneViewAgentDeploy.tear_down') def tear_down(self, task): - if not CONF.conductor.automated_clean: + # if node specifically has cleanup disabled, or general cleanup + # is disabled and node has not it enabled + if utils.skip_automated_cleaning(task.node): deploy_utils.tear_down(task) return super(OneViewAgentDeploy, self).tear_down(task) diff --git a/ironic/objects/node.py b/ironic/objects/node.py index 047ac15c04..39b3cb68c4 100644 --- a/ironic/objects/node.py +++ b/ironic/objects/node.py @@ -64,7 +64,8 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): # Version 1.25: Add fault field # Version 1.26: Add deploy_step field # Version 1.27: Add conductor_group field - VERSION = '1.27' + # Version 1.28: Add automated_clean field + VERSION = '1.28' dbapi = db_api.get_instance() @@ -130,7 +131,7 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): 'inspection_started_at': object_fields.DateTimeField(nullable=True), 'extra': object_fields.FlexibleDictField(nullable=True), - + 'automated_clean': objects.fields.BooleanField(nullable=True), 'bios_interface': object_fields.StringField(nullable=True), 'boot_interface': object_fields.StringField(nullable=True), 'console_interface': object_fields.StringField(nullable=True), @@ -529,6 +530,26 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): elif self.conductor_group: self.conductor_group = '' + # NOTE (yolanda): new method created to avoid repeating code in + # _convert_to_version, and to avoid pep8 too complex error + def _adjust_field_to_version(self, field_name, field_default_value, + target_version, major, minor, + remove_unavailable_fields): + field_is_set = self.obj_attr_is_set(field_name) + if target_version >= (major, minor): + # target version supports the major/minor specified + if not field_is_set: + # set it to its default value if it is not set + setattr(self, field_name, field_default_value) + elif field_is_set: + # target version does not support the field, and it is set + if remove_unavailable_fields: + # (De)serialising: remove unavailable fields + delattr(self, field_name) + elif getattr(self, field_name) is not field_default_value: + # DB: set unavailable field to the default value + setattr(self, field_name, field_default_value) + def _convert_to_version(self, target_version, remove_unavailable_fields=True): """Convert to the target version. @@ -552,6 +573,8 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): this, it should be removed. Version 1.27: conductor_group field was added. For versions prior to this, it should be removed. + Version 1.28: automated_clean was added. For versions prior to this, it + should be set to None (or removed). :param target_version: the desired version of the object :param remove_unavailable_fields: True to remove fields that are @@ -560,47 +583,13 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): values; set this to False for DB interactions. """ target_version = versionutils.convert_version_to_tuple(target_version) - # Convert the rescue_interface field. - rescue_iface_is_set = self.obj_attr_is_set('rescue_interface') - if target_version >= (1, 22): - # Target version supports rescue_interface. - if not rescue_iface_is_set: - # Set it to its default value if it is not set. - self.rescue_interface = None - elif rescue_iface_is_set: - # Target version does not support rescue_interface, and it is set. - if remove_unavailable_fields: - # (De)serialising: remove unavailable fields. - delattr(self, 'rescue_interface') - elif self.rescue_interface is not None: - # DB: set unavailable field to the default of None. - self.rescue_interface = None - traits_is_set = self.obj_attr_is_set('traits') - if target_version >= (1, 23): - # Target version supports traits. - if not traits_is_set: - self.traits = None - elif traits_is_set: - if remove_unavailable_fields: - delattr(self, 'traits') - elif self.traits is not None: - self.traits = None - - bios_iface_is_set = self.obj_attr_is_set('bios_interface') - if target_version >= (1, 24): - # Target version supports bios_interface. - if not bios_iface_is_set: - # Set it to its default value if it is not set. - self.bios_interface = None - elif bios_iface_is_set: - # Target version does not support bios_interface, and it is set. - if remove_unavailable_fields: - # (De)serialising: remove unavailable fields. - delattr(self, 'bios_interface') - elif self.bios_interface is not None: - # DB: set unavailable field to the default of None. - self.bios_interface = None + # Convert the different fields depending on version + fields = [('rescue_interface', 22), ('traits', 23), + ('bios_interface', 24), ('automated_clean', 28)] + for name, minor in fields: + self._adjust_field_to_version(name, None, target_version, + 1, minor, remove_unavailable_fields) self._convert_fault_field(target_version, remove_unavailable_fields) self._convert_deploy_step_field(target_version, diff --git a/ironic/tests/unit/api/utils.py b/ironic/tests/unit/api/utils.py index 233bea1a5d..52afc394b1 100644 --- a/ironic/tests/unit/api/utils.py +++ b/ironic/tests/unit/api/utils.py @@ -112,6 +112,8 @@ def node_post_data(**kw): node.pop('resource_class') if 'fault' not in kw: node.pop('fault') + if 'automated_clean' not in kw: + node.pop('automated_clean') internal = node_controller.NodePatchType.internal_attrs() return remove_internal(node, internal) diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 9b7a39d8ef..97441570a9 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -3304,6 +3304,128 @@ class DoNodeCleanTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): self.assertNotIn('clean_steps', node.driver_internal_info) self.assertNotIn('clean_step_index', node.driver_internal_info) + @mock.patch('ironic.drivers.modules.fake.FakePower.validate', + autospec=True) + @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.validate', + autospec=True) + def test__do_node_clean_automated_disabled_individual_enabled( + self, mock_network, mock_validate): + self.config(automated_clean=False, group='conductor') + + self._start_service() + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + provision_state=states.CLEANING, + target_provision_state=states.AVAILABLE, + last_error=None, automated_clean=True) + with task_manager.acquire( + self.context, node.uuid, shared=False) as task: + self.service._do_node_clean(task) + self._stop_service() + node.refresh() + + # Assert that the node clean was called + self.assertTrue(mock_validate.called) + self.assertIn('clean_steps', node.driver_internal_info) + + @mock.patch('ironic.drivers.modules.fake.FakePower.validate', + autospec=True) + def test__do_node_clean_automated_disabled_individual_disabled( + self, mock_validate): + self.config(automated_clean=False, group='conductor') + + self._start_service() + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + provision_state=states.CLEANING, + target_provision_state=states.AVAILABLE, + last_error=None, automated_clean=False) + with task_manager.acquire( + self.context, node.uuid, shared=False) as task: + self.service._do_node_clean(task) + self._stop_service() + node.refresh() + + # Assert that the node was moved to available without cleaning + self.assertFalse(mock_validate.called) + self.assertEqual(states.AVAILABLE, node.provision_state) + self.assertEqual(states.NOSTATE, node.target_provision_state) + self.assertEqual({}, node.clean_step) + self.assertNotIn('clean_steps', node.driver_internal_info) + self.assertNotIn('clean_step_index', node.driver_internal_info) + + @mock.patch('ironic.drivers.modules.fake.FakePower.validate', + autospec=True) + @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.validate', + autospec=True) + def test__do_node_clean_automated_enabled(self, mock_validate, + mock_network): + self.config(automated_clean=True, group='conductor') + + self._start_service() + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + provision_state=states.CLEANING, + target_provision_state=states.AVAILABLE, + last_error=None) + with task_manager.acquire( + self.context, node.uuid, shared=False) as task: + self.service._do_node_clean(task) + self._stop_service() + node.refresh() + + # Assert that the node was cleaned + self.assertTrue(mock_validate.called) + self.assertIn('clean_steps', node.driver_internal_info) + + @mock.patch('ironic.drivers.modules.fake.FakePower.validate', + autospec=True) + @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.validate', + autospec=True) + def test__do_node_clean_automated_enabled_individual_enabled( + self, mock_network, mock_validate): + self.config(automated_clean=True, group='conductor') + + self._start_service() + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + provision_state=states.CLEANING, + target_provision_state=states.AVAILABLE, + last_error=None, automated_clean=True) + with task_manager.acquire( + self.context, node.uuid, shared=False) as task: + self.service._do_node_clean(task) + self._stop_service() + node.refresh() + + # Assert that the node was cleaned + self.assertTrue(mock_validate.called) + self.assertIn('clean_steps', node.driver_internal_info) + + @mock.patch('ironic.drivers.modules.fake.FakePower.validate', + autospec=True) + @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.validate', + autospec=True) + def test__do_node_clean_automated_enabled_individual_none( + self, mock_validate, mock_network): + self.config(automated_clean=True, group='conductor') + + self._start_service() + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + provision_state=states.CLEANING, + target_provision_state=states.AVAILABLE, + last_error=None, automated_clean=None) + with task_manager.acquire( + self.context, node.uuid, shared=False) as task: + self.service._do_node_clean(task) + self._stop_service() + node.refresh() + + # Assert that the node was cleaned + self.assertTrue(mock_validate.called) + self.assertIn('clean_steps', node.driver_internal_info) + @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.validate', autospec=True) @mock.patch('ironic.drivers.modules.fake.FakeDeploy.prepare_cleaning', diff --git a/ironic/tests/unit/db/utils.py b/ironic/tests/unit/db/utils.py index f981c07d8f..294d241078 100644 --- a/ironic/tests/unit/db/utils.py +++ b/ironic/tests/unit/db/utils.py @@ -214,6 +214,7 @@ def get_test_node(**kw): 'tags': kw.get('tags', []), 'resource_class': kw.get('resource_class'), 'traits': kw.get('traits', []), + 'automated_clean': kw.get('automated_clean', None), } for iface in drivers_base.ALL_INTERFACES: diff --git a/ironic/tests/unit/objects/test_node.py b/ironic/tests/unit/objects/test_node.py index c96d5e4b8c..cdd4b3fae1 100644 --- a/ironic/tests/unit/objects/test_node.py +++ b/ironic/tests/unit/objects/test_node.py @@ -706,6 +706,69 @@ class TestConvertToVersion(db_base.DbTestCase): self.assertEqual('', node.conductor_group) self.assertEqual({'conductor_group': ''}, node.obj_get_changes()) + def test_automated_clean_supported_missing(self): + # automated_clean_interface not set, should be set to default. + node = obj_utils.get_test_node(self.ctxt, **self.fake_node) + delattr(node, 'automated_clean') + node.obj_reset_changes() + + node._convert_to_version("1.28") + + self.assertIsNone(node.automated_clean) + self.assertEqual({'automated_clean': None}, + node.obj_get_changes()) + + def test_automated_clean_supported_set(self): + # automated_clean set, no change required. + node = obj_utils.get_test_node(self.ctxt, **self.fake_node) + + node.automated_clean = True + node.obj_reset_changes() + node._convert_to_version("1.28") + self.assertEqual(True, node.automated_clean) + self.assertEqual({}, node.obj_get_changes()) + + def test_automated_clean_unsupported_missing(self): + # automated_clean not set, no change required. + node = obj_utils.get_test_node(self.ctxt, **self.fake_node) + + delattr(node, 'automated_clean') + node.obj_reset_changes() + node._convert_to_version("1.27") + self.assertNotIn('automated_clean', node) + self.assertEqual({}, node.obj_get_changes()) + + def test_automated_clean_unsupported_set_remove(self): + # automated_clean set, should be removed. + node = obj_utils.get_test_node(self.ctxt, **self.fake_node) + + node.automated_clean = True + node.obj_reset_changes() + node._convert_to_version("1.27") + self.assertNotIn('automated_clean', node) + self.assertEqual({}, node.obj_get_changes()) + + def test_automated_clean_unsupported_set_no_remove_non_default(self): + # automated_clean set, should be set to default. + node = obj_utils.get_test_node(self.ctxt, **self.fake_node) + + node.automated_clean = True + node.obj_reset_changes() + node._convert_to_version("1.27", False) + self.assertIsNone(node.automated_clean) + self.assertEqual({'automated_clean': None}, + node.obj_get_changes()) + + def test_automated_clean_unsupported_set_no_remove_default(self): + # automated_clean set, no change required. + node = obj_utils.get_test_node(self.ctxt, **self.fake_node) + + node.automated_clean = None + node.obj_reset_changes() + node._convert_to_version("1.27", False) + self.assertIsNone(node.automated_clean) + self.assertEqual({}, node.obj_get_changes()) + class TestNodePayloads(db_base.DbTestCase): diff --git a/ironic/tests/unit/objects/test_objects.py b/ironic/tests/unit/objects/test_objects.py index 3c7d1782e4..c226820128 100644 --- a/ironic/tests/unit/objects/test_objects.py +++ b/ironic/tests/unit/objects/test_objects.py @@ -677,7 +677,7 @@ class TestObject(_LocalTest, _TestObject): # version bump. It is an MD5 hash of the object fields and remotable methods. # The fingerprint values should only be changed if there is a version bump. expected_object_fingerprints = { - 'Node': '1.27-129323d486c03a99de27053503b2cae3', + 'Node': '1.28-d4aba1f583774326903f7366fbaae752', 'MyObj': '1.5-9459d30d6954bffc7a9afd347a807ca6', 'Chassis': '1.3-d656e039fd8ae9f34efc232ab3980905', 'Port': '1.8-898a47921f4a1f53fcdddd4eeb179e0b',