From 1f00cc641ddab3ee5016bde364af44256bc49d3d Mon Sep 17 00:00:00 2001 From: Alex Schultz Date: Thu, 16 Jul 2020 09:52:33 -0600 Subject: [PATCH] Throw warning if --limit used with a skip list in Heat We have recently switched the --limit option to take precedence over the defined skip list via a Heat parameter. In order to prevent confusion between these two, we should throw a warning indicating that the parameter in Heat will be ignored when running ansible. Depends-On: https://review.opendev.org/#/c/741271/ Change-Id: I7c43563381b694573801e6a54e973fdbd36e0798 Related-Bug: #1857298 (cherry picked from commit 47dc9aae0e04856d8ffccd8c8c9b50d02c554aea) --- .../overcloud_deploy/test_overcloud_deploy.py | 39 ++++++++++++++++++ .../v1/overcloud_node/test_overcloud_node.py | 41 +++++++++++++++++++ tripleoclient/v1/overcloud_deploy.py | 11 +++++ tripleoclient/v1/overcloud_node.py | 12 ++++++ 4 files changed, 103 insertions(+) diff --git a/tripleoclient/tests/v1/overcloud_deploy/test_overcloud_deploy.py b/tripleoclient/tests/v1/overcloud_deploy/test_overcloud_deploy.py index d8af0d39c..4cd624d7d 100644 --- a/tripleoclient/tests/v1/overcloud_deploy/test_overcloud_deploy.py +++ b/tripleoclient/tests/v1/overcloud_deploy/test_overcloud_deploy.py @@ -1715,6 +1715,45 @@ class TestDeployOvercloud(fakes.TestDeployOvercloud): expected = ('fd12::1 uc.ctlplane.localdomain uc.ctlplane') self.assertEqual(expected, self.cmd._get_undercloud_host_entry()) + def test_check_limit_warning(self): + mock_warning = mock.MagicMock() + mock_log = mock.MagicMock() + mock_log.warning = mock_warning + env = {'parameter_defaults': {}} + + old_logger = self.cmd.log + self.cmd.log = mock_log + self.cmd._check_limit_skiplist_warning(env) + self.cmd.log = old_logger + mock_warning.assert_not_called() + + def test_check_limit_warning_empty(self): + mock_warning = mock.MagicMock() + mock_log = mock.MagicMock() + mock_log.warning = mock_warning + env = {'parameter_defaults': {'DeploymentServerBlacklist': []}} + + old_logger = self.cmd.log + self.cmd.log = mock_log + self.cmd._check_limit_skiplist_warning(env) + self.cmd.log = old_logger + mock_warning.assert_not_called() + + def test_check_limit_warning_warns(self): + mock_warning = mock.MagicMock() + mock_log = mock.MagicMock() + mock_log.warning = mock_warning + env = {'parameter_defaults': {'DeploymentServerBlacklist': ['a']}} + + old_logger = self.cmd.log + self.cmd.log = mock_log + self.cmd._check_limit_skiplist_warning(env) + self.cmd.log = old_logger + expected_message = ('[WARNING] DeploymentServerBlacklist is defined ' + 'and will be ignored because --limit has been ' + 'specified.') + mock_warning.assert_called_once_with(expected_message) + class TestArgumentValidation(fakes.TestDeployOvercloud): diff --git a/tripleoclient/tests/v1/overcloud_node/test_overcloud_node.py b/tripleoclient/tests/v1/overcloud_node/test_overcloud_node.py index b9e8a215d..ee0fc9fe6 100644 --- a/tripleoclient/tests/v1/overcloud_node/test_overcloud_node.py +++ b/tripleoclient/tests/v1/overcloud_node/test_overcloud_node.py @@ -343,6 +343,47 @@ class TestDeleteNode(fakes.TestDeleteNode): self.assertEqual(expected, nodes_text) self.assertEqual(['compute-0', 'controller-0'], nodes) + def test_check_skiplist_exists(self): + mock_warning = mock.MagicMock() + mock_log = mock.MagicMock() + mock_log.warning = mock_warning + env = {'parameter_defaults': {}} + + old_logger = self.cmd.log + self.cmd.log = mock_log + self.cmd._check_skiplist_exists(env) + self.cmd.log = old_logger + mock_warning.assert_not_called() + + def test_check_skiplist_exists_empty(self): + mock_warning = mock.MagicMock() + mock_log = mock.MagicMock() + mock_log.warning = mock_warning + env = {'parameter_defaults': {'DeploymentServerBlacklist': []}} + + old_logger = self.cmd.log + self.cmd.log = mock_log + self.cmd._check_skiplist_exists(env) + self.cmd.log = old_logger + mock_warning.assert_not_called() + + def test_check_skiplist_exists_warns(self): + mock_warning = mock.MagicMock() + mock_log = mock.MagicMock() + mock_log.warning = mock_warning + env = {'parameter_defaults': {'DeploymentServerBlacklist': ['a']}} + + old_logger = self.cmd.log + self.cmd.log = mock_log + self.cmd._check_skiplist_exists(env) + self.cmd.log = old_logger + expected_message = ('[WARNING] DeploymentServerBlacklist is ignored ' + 'when executing scale down actions. If the ' + 'node(s) being removed should *NOT* have any ' + 'actions executed on them, please shut them off ' + 'prior to their removal.') + mock_warning.assert_called_once_with(expected_message) + class TestProvideNode(fakes.TestOvercloudNode): diff --git a/tripleoclient/v1/overcloud_deploy.py b/tripleoclient/v1/overcloud_deploy.py index 58fd00025..e3447326d 100644 --- a/tripleoclient/v1/overcloud_deploy.py +++ b/tripleoclient/v1/overcloud_deploy.py @@ -137,6 +137,12 @@ class DeployOvercloud(command.Command): container_name) return parameter_defaults + def _check_limit_skiplist_warning(self, env): + if env.get('parameter_defaults').get('DeploymentServerBlacklist'): + msg = _('[WARNING] DeploymentServerBlacklist is defined and will ' + 'be ignored because --limit has been specified.') + self.log.warning(msg) + def _user_env_path(self, abs_env_path, tht_root): env_dirname = os.path.dirname(abs_env_path) user_env_dir = os.path.join( @@ -452,6 +458,11 @@ class DeployOvercloud(command.Command): cleanup=(not parsed_args.no_cleanup)) template_utils.deep_update(env, localenv) + if parsed_args.limit: + # check if skip list is defined while using --limit and throw a + # warning if necessary + self._check_limit_skiplist_warning(env) + if stack: if not parsed_args.disable_validations: # note(aschultz): network validation goes here before we deploy diff --git a/tripleoclient/v1/overcloud_node.py b/tripleoclient/v1/overcloud_node.py index 848884660..afe498756 100644 --- a/tripleoclient/v1/overcloud_node.py +++ b/tripleoclient/v1/overcloud_node.py @@ -137,6 +137,16 @@ class DeleteNode(command.Command): ) return output.getvalue(), node_hostnames + def _check_skiplist_exists(self, env): + skiplist = env.get('parameter_defaults', + {}).get('DeploymentServerBlacklist') + if skiplist: + self.log.warning(_('[WARNING] DeploymentServerBlacklist is ' + 'ignored when executing scale down actions. If ' + 'the node(s) being removed should *NOT* have ' + 'any actions executed on them, please shut ' + 'them off prior to their removal.')) + def take_action(self, parsed_args): self.log.debug("take_action(%s)" % parsed_args) clients = self.app.client_manager @@ -172,6 +182,8 @@ class DeleteNode(command.Command): print("Deleting the following nodes from stack {stack}:\n{nodes}" .format(stack=stack.stack_name, nodes=nodes_text)) + self._check_skiplist_exists(stack.environment()) + scale.scale_down( log=self.log, clients=clients,