From ee6119e7744b98b9d00d4a7e50adfa4b2a18c117 Mon Sep 17 00:00:00 2001 From: Feruzjon Muyassarov Date: Wed, 11 Nov 2020 15:13:25 +0200 Subject: [PATCH] Allow disabling automated_clean per node This allows users to disable automated cleaning on Node level. Story: #2008113 Task: #40829 Change-Id: If583bae4108b9bfa99cc460509af84696c7003c5 --- ironic/api/controllers/v1/node.py | 4 ++++ ironic/common/policy.py | 8 ++++++- ironic/conductor/utils.py | 10 +++++++- .../unit/api/controllers/v1/test_node.py | 21 ++++++++++++++++ ironic/tests/unit/conductor/test_cleaning.py | 24 +++++++++++++++++++ ...able_automated_clean-a3ccb1e19940a7a4.yaml | 7 ++++++ 6 files changed, 72 insertions(+), 2 deletions(-) create mode 100644 releasenotes/notes/allow_to_disable_automated_clean-a3ccb1e19940a7a4.yaml diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 46c02521d4..f63e810970 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -2351,6 +2351,10 @@ class NodesController(rest.RestController): policy_checks.append('baremetal:node:update_instance_info') elif p['path'].startswith('/extra'): policy_checks.append('baremetal:node:update_extra') + elif (p['path'].startswith('/automated_clean') + and strutils.bool_from_string(p['value'], default=None) + is False): + policy_checks.append('baremetal:node:disable_cleaning') else: generic_update = True diff --git a/ironic/common/policy.py b/ironic/common/policy.py index 2407977311..811198206d 100644 --- a/ironic/common/policy.py +++ b/ironic/common/policy.py @@ -256,7 +256,13 @@ node_policies = [ 'rule:is_admin or rule:is_observer', 'Retrieve Node BIOS information', [{'path': '/nodes/{node_ident}/bios', 'method': 'GET'}, - {'path': '/nodes/{node_ident}/bios/{setting}', 'method': 'GET'}]) + {'path': '/nodes/{node_ident}/bios/{setting}', 'method': 'GET'}]), + + policy.DocumentedRuleDefault( + 'baremetal:node:disable_cleaning', + 'rule:baremetal:node:update', + 'Disable Node disk cleaning', + [{'path': '/nodes/{node_ident}', 'method': 'PATCH'}]), ] port_policies = [ diff --git a/ironic/conductor/utils.py b/ironic/conductor/utils.py index 5830011fa0..6ba6204940 100644 --- a/ironic/conductor/utils.py +++ b/ironic/conductor/utils.py @@ -861,7 +861,15 @@ def skip_automated_cleaning(node): :param node: the node to consider """ - return not CONF.conductor.automated_clean and not node.automated_clean + if node.automated_clean: + return False + elif node.automated_clean is None: + return not CONF.conductor.automated_clean + else: + LOG.info("Automated cleaning is disabled via the API for " + "node %(node)s", + {'node': node.uuid}) + return True def power_on_node_if_needed(task): diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py index 2d979fb42d..78628ba2e6 100644 --- a/ironic/tests/unit/api/controllers/v1/test_node.py +++ b/ironic/tests/unit/api/controllers/v1/test_node.py @@ -3359,6 +3359,27 @@ class TestPatch(test_api_base.BaseApiTest): self.assertEqual('application/json', response.content_type) self.assertEqual(http_client.OK, response.status_code) + @mock.patch.object(policy, 'authorize', spec=True) + def test_update_automated_clean_with_false(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:node:disable_cleaning': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + + node = obj_utils.create_test_node(self.context, + uuid=uuidutils.generate_uuid()) + self.mock_update_node.return_value = node + headers = {api_base.Version.string: '1.47'} + response = self.patch_json('/nodes/%s' % node.uuid, + [{'path': '/automated_clean', + 'value': False, + 'op': 'replace'}], + headers=headers, + expect_errors=True) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.FORBIDDEN, response.status_code) + def test_update_automated_clean_old_api(self): node = obj_utils.create_test_node(self.context, uuid=uuidutils.generate_uuid()) diff --git a/ironic/tests/unit/conductor/test_cleaning.py b/ironic/tests/unit/conductor/test_cleaning.py index 8398b38e94..8ea42970ae 100644 --- a/ironic/tests/unit/conductor/test_cleaning.py +++ b/ironic/tests/unit/conductor/test_cleaning.py @@ -193,6 +193,30 @@ class DoNodeCleanTestCase(db_base.DbTestCase): 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_enabled_individual_disabled( + self, mock_validate): + self.config(automated_clean=True, group='conductor') + + 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: + cleaning.do_node_clean(task) + 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) def test__do_node_clean_automated_disabled_individual_disabled( diff --git a/releasenotes/notes/allow_to_disable_automated_clean-a3ccb1e19940a7a4.yaml b/releasenotes/notes/allow_to_disable_automated_clean-a3ccb1e19940a7a4.yaml new file mode 100644 index 0000000000..aa9f937d58 --- /dev/null +++ b/releasenotes/notes/allow_to_disable_automated_clean-a3ccb1e19940a7a4.yaml @@ -0,0 +1,7 @@ +--- +features: + - | + Allows disabling automated cleaning per node if it is enabled globally. + An existing ``automated_clean`` field will allow disabling of automated + cleaning on the node object. A new ``baremetal:node:disable_cleaning`` + policy is added which defaults to ``baremetal:node:update``. \ No newline at end of file