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 bada388997..78853c5e52 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 acceed987d..c7faa5188b 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 @@ -243,7 +244,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) @@ -287,7 +291,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 4bc5d85488..5c20a7584b 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',