From e352506699f8f52f1ce8cf05862f9ba9c2520a0c Mon Sep 17 00:00:00 2001 From: Rabi Mishra Date: Wed, 29 Jul 2020 18:11:57 +0530 Subject: [PATCH] Simplify error handling in tripleo_deploy Don't use return code from ansible_runner and manage with a flag. This also removes the fail_on_rc flag from run_ansible_playbook() and makes it consistent to raise RuntimeError if rc !=0 everywhere including _standalone_deploy(). Conflicts: tripleoclient/tests/v1/tripleo/test_tripleo_deploy.py tripleoclient/utils.py tripleoclient/v1/tripleo_deploy.py Change-Id: Ia5971af601d1d9500f33045768b38ac7937117f5 Closes-Bug: #1889394 (cherry picked from commit a29dd8ef79dcf9fbde5dae1b98f0e88694b14b7a) --- tripleoclient/tests/test_utils.py | 17 +- .../tests/v1/tripleo/test_tripleo_deploy.py | 186 +++++++++++++++++- tripleoclient/utils.py | 16 +- tripleoclient/v1/tripleo_deploy.py | 31 +-- 4 files changed, 203 insertions(+), 47 deletions(-) diff --git a/tripleoclient/tests/test_utils.py b/tripleoclient/tests/test_utils.py index 4e219e068..4085af8af 100644 --- a/tripleoclient/tests/test_utils.py +++ b/tripleoclient/tests/test_utils.py @@ -114,12 +114,11 @@ class TestRunAnsiblePlaybook(TestCase): return_value="/foo/inventory.yaml") def test_run_success_default(self, mock_dump_artifact, mock_run, mock_mkdirs, mock_exists, mock_mkstemp): - retcode, output = utils.run_ansible_playbook( + utils.run_ansible_playbook( playbook='existing.yaml', inventory='localhost,', workdir='/tmp' ) - self.assertEqual(retcode, 0) @mock.patch('os.path.exists', return_value=True) @mock.patch('os.makedirs') @@ -132,12 +131,11 @@ class TestRunAnsiblePlaybook(TestCase): return_value="/foo/inventory.yaml") def test_run_success_ansible_cfg(self, mock_dump_artifact, mock_run, mock_mkdirs, mock_exists): - retcode, output = utils.run_ansible_playbook( + utils.run_ansible_playbook( playbook='existing.yaml', inventory='localhost,', workdir='/tmp' ) - self.assertEqual(retcode, 0) @mock.patch('tempfile.mkstemp', return_value=('foo', '/tmp/fooBar.cfg')) @mock.patch('os.path.exists', return_value=True) @@ -152,13 +150,12 @@ class TestRunAnsiblePlaybook(TestCase): def test_run_success_connection_local(self, mock_dump_artifact, mock_run, mock_mkdirs, mock_exists, mock_mkstemp): - retcode, output = utils.run_ansible_playbook( + utils.run_ansible_playbook( playbook='existing.yaml', inventory='localhost,', workdir='/tmp', connection='local' ) - self.assertEqual(retcode, 0) @mock.patch('os.makedirs', return_value=None) @mock.patch('tempfile.mkstemp', return_value=('foo', '/tmp/fooBar.cfg')) @@ -173,14 +170,13 @@ class TestRunAnsiblePlaybook(TestCase): def test_run_success_gathering_policy(self, mock_dump_artifact, mock_run, mock_exists, mock_mkstemp, mock_makedirs): - retcode, output = utils.run_ansible_playbook( + utils.run_ansible_playbook( playbook='existing.yaml', inventory='localhost,', workdir='/tmp', connection='local', gathering_policy='smart' ) - self.assertEqual(retcode, 0) @mock.patch('os.makedirs', return_value=None) @mock.patch('tempfile.mkstemp', return_value=('foo', '/tmp/fooBar.cfg')) @@ -197,7 +193,7 @@ class TestRunAnsiblePlaybook(TestCase): arglist = { 'var_one': 'val_one', } - retcode, output = utils.run_ansible_playbook( + utils.run_ansible_playbook( playbook='existing.yaml', inventory='localhost,', workdir='/tmp', @@ -205,7 +201,6 @@ class TestRunAnsiblePlaybook(TestCase): gathering_policy='smart', extra_vars=arglist ) - self.assertEqual(retcode, 0) @mock.patch('six.moves.builtins.open') @mock.patch('tripleoclient.utils.makedirs') @@ -214,7 +209,7 @@ class TestRunAnsiblePlaybook(TestCase): ansible_runner.ArtifactLoader = mock.MagicMock() ansible_runner.Runner.run = mock.MagicMock(return_value=('', 0)) ansible_runner.runner_config = mock.MagicMock() - retcode, output = utils.run_ansible_playbook( + utils.run_ansible_playbook( playbook='existing.yaml', inventory='localhost,', workdir='/tmp', diff --git a/tripleoclient/tests/v1/tripleo/test_tripleo_deploy.py b/tripleoclient/tests/v1/tripleo/test_tripleo_deploy.py index f4c7cc33b..b50092205 100644 --- a/tripleoclient/tests/v1/tripleo/test_tripleo_deploy.py +++ b/tripleoclient/tests/v1/tripleo/test_tripleo_deploy.py @@ -956,6 +956,187 @@ class TestDeployUndercloud(TestPluginV1): mock_cleanupdirs.assert_called_once() self.assertEqual(mock_killheat.call_count, 2) + @mock.patch.object( + ansible_runner.runner_config, + 'RunnerConfig', + return_value=fakes.FakeRunnerConfig() + ) + @mock.patch.object( + ansible_runner.Runner, + 'run', + return_value=fakes.fake_ansible_runner_run_return(1) + ) + @mock.patch('os.path.exists') + @mock.patch('os.chdir') + @mock.patch('tripleoclient.utils.reset_cmdline') + @mock.patch('tripleoclient.utils.copy_clouds_yaml') + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_download_stack_outputs') + @mock.patch('tripleo_common.actions.ansible.' + 'write_default_ansible_cfg') + # TODO(cjeanner) drop once we have proper oslo.privsep + @mock.patch('os.chmod') + # TODO(cjeanner) drop once we have proper oslo.privsep + @mock.patch('subprocess.check_call', autospec=True) + # TODO(cjeanner) drop once we have proper oslo.privsep + @mock.patch('getpass.getuser', return_value='stack') + @mock.patch('os.mkdir') + @mock.patch('six.moves.builtins.open') + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_populate_templates_dir') + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_create_install_artifact', return_value='/tmp/foo.tar.bzip2') + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_cleanup_working_dirs') + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_create_working_dirs') + @mock.patch('tripleoclient.utils.wait_api_port_ready', + autospec=True) + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_deploy_tripleo_heat_templates', autospec=True, + return_value='undercloud, 0') + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_download_ansible_playbooks', autospec=True, + return_value='/foo') + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_launch_heat') + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_kill_heat') + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_configure_puppet') + @mock.patch('os.geteuid', return_value=0) + @mock.patch('os.environ', return_value='CREATE_COMPLETE') + @mock.patch('tripleoclient.utils.wait_for_stack_ready', return_value=True) + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_set_default_plan') + @mock.patch('ansible_runner.utils.dump_artifact', autospec=True, + return_value="/foo/inventory.yaml") + def test_take_action_ansible_err(self, mock_dump_artifact, + mock_def_plan, mock_poll, + mock_environ, mock_geteuid, mock_puppet, + mock_killheat, mock_launchheat, + mock_download, mock_tht, + mock_wait_for_port, mock_createdirs, + mock_cleanupdirs, mock_tarball, + mock_templates_dir, mock_open, mock_os, + mock_user, mock_cc, mock_chmod, mock_ac, + mock_outputs, mock_copy, mock_cmdline, + mock_chdir, mock_file_exists, mock_run, + mock_run_prepare): + parsed_args = self.check_parser(self.cmd, + ['--local-ip', '127.0.0.1', + '--templates', '/tmp/thtroot', + '--stack', 'undercloud', + '--output-dir', '/my', + '--standalone', + '--standalone-role', 'Undercloud', + # TODO(cjeanner) drop once we have + # proper oslo.privsep + '--deployment-user', 'stack', + '-e', '/tmp/thtroot/puppet/foo.yaml', + '-e', '/tmp/thtroot//docker/bar.yaml', + '-e', '/tmp/thtroot42/notouch.yaml', + '-e', '~/custom.yaml', + '-e', 'something.yaml', + '-e', '../../../outside.yaml'], []) + + mock_file_exists.return_value = True + fake_orchestration = mock_launchheat(parsed_args) + self.assertRaises(exceptions.DeploymentError, + self.cmd.take_action, parsed_args) + mock_createdirs.assert_called_once() + mock_puppet.assert_called_once() + mock_launchheat.assert_called_with(parsed_args) + mock_tht.assert_called_once_with(self.cmd, fake_orchestration, + parsed_args) + mock_download.assert_called_with(self.cmd, fake_orchestration, + 'undercloud', 'Undercloud', + sys.executable) + mock_tarball.assert_called_once() + mock_cleanupdirs.assert_called_once() + self.assertEqual(mock_killheat.call_count, 2) + + @mock.patch('os.chdir') + @mock.patch('tripleoclient.utils.reset_cmdline') + @mock.patch('tripleoclient.utils.copy_clouds_yaml') + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_download_stack_outputs') + @mock.patch('tripleo_common.actions.ansible.' + 'write_default_ansible_cfg') + # TODO(cjeanner) drop once we have proper oslo.privsep + @mock.patch('os.chmod') + # TODO(cjeanner) drop once we have proper oslo.privsep + @mock.patch('subprocess.check_call', autospec=True) + # TODO(cjeanner) drop once we have proper oslo.privsep + @mock.patch('getpass.getuser', return_value='stack') + @mock.patch('os.mkdir') + @mock.patch('six.moves.builtins.open') + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_populate_templates_dir') + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_create_install_artifact', return_value='/tmp/foo.tar.bzip2') + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_cleanup_working_dirs') + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_create_working_dirs') + @mock.patch('tripleoclient.utils.wait_api_port_ready') + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_deploy_tripleo_heat_templates', autospec=True, + return_value='undercloud, 0') + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_download_ansible_playbooks', autospec=True, + return_value='/foo') + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_launch_heat') + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_kill_heat') + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_configure_puppet') + @mock.patch('os.geteuid', return_value=0) + @mock.patch('os.environ', return_value='CREATE_COMPLETE') + @mock.patch('tripleoclient.utils.wait_for_stack_ready', return_value=True) + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy.' + '_set_default_plan') + def test_take_action_other_err(self, + mock_def_plan, mock_poll, + mock_environ, mock_geteuid, mock_puppet, + mock_killheat, mock_launchheat, + mock_download, mock_tht, + mock_wait_for_port, mock_createdirs, + mock_cleanupdirs, mock_tarball, + mock_templates_dir, mock_open, mock_os, + mock_user, mock_cc, mock_chmod, mock_ac, + mock_outputs, mock_copy, mock_cmdline, + mock_chdir): + parsed_args = self.check_parser(self.cmd, + ['--local-ip', '127.0.0.1', + '--templates', '/tmp/thtroot', + '--stack', 'undercloud', + '--output-dir', '/my', + '--standalone', + '--standalone-role', 'Undercloud', + # TODO(cjeanner) drop once we have + # proper oslo.privsep + '--deployment-user', 'stack', + '-e', '/tmp/thtroot/puppet/foo.yaml', + '-e', '/tmp/thtroot//docker/bar.yaml', + '-e', '/tmp/thtroot42/notouch.yaml', + '-e', '~/custom.yaml', + '-e', 'something.yaml', + '-e', '../../../outside.yaml'], []) + + mock_wait_for_port.side_effect = exceptions.DeploymentError + self.assertRaises(exceptions.DeploymentError, + self.cmd.take_action, parsed_args) + mock_createdirs.assert_called_once() + mock_puppet.assert_called_once() + mock_launchheat.assert_called_with(parsed_args) + mock_tht.assert_not_called() + mock_download.assert_not_called() + mock_tarball.assert_called_once() + mock_cleanupdirs.assert_called_once() + self.assertEqual(mock_killheat.call_count, 1) + @mock.patch('tripleoclient.utils.reset_cmdline') @mock.patch('tripleoclient.utils.copy_clouds_yaml') def test_take_action(self, mock_copy, mock_cmdline): @@ -963,6 +1144,7 @@ class TestDeployUndercloud(TestPluginV1): ['--local-ip', '127.0.0.1', '--templates', '/tmp/thtroot', '--stack', 'undercloud', + '--standalone', '--output-dir', '/my'], []) self.assertRaises(exceptions.DeploymentError, self.cmd.take_action, parsed_args) @@ -970,8 +1152,7 @@ class TestDeployUndercloud(TestPluginV1): @mock.patch('tripleoclient.utils.reset_cmdline') @mock.patch('tripleoclient.utils.copy_clouds_yaml') - @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy._standalone_deploy', - return_value=1) + @mock.patch('tripleoclient.v1.tripleo_deploy.Deploy._standalone_deploy') def test_take_action_failure(self, mock_deploy, mock_copy, mock_cmdline): parsed_args = self.check_parser(self.cmd, ['--local-ip', '127.0.0.1', @@ -979,6 +1160,7 @@ class TestDeployUndercloud(TestPluginV1): '--stack', 'undercloud', '--output-dir', '/my', '--standalone'], []) + mock_deploy.side_effect = exceptions.DeploymentError self.assertRaises(exceptions.DeploymentError, self.cmd.take_action, parsed_args) mock_copy.assert_called_once() diff --git a/tripleoclient/utils.py b/tripleoclient/utils.py index 801e770d8..8aee80e4d 100644 --- a/tripleoclient/utils.py +++ b/tripleoclient/utils.py @@ -251,7 +251,7 @@ def run_ansible_playbook(playbook, inventory, workdir, playbook_dir=None, parallel_run=False, callback_whitelist=constants.ANSIBLE_CWL, ansible_cfg=None, ansible_timeout=30, - reproduce_command=False, fail_on_rc=True, + reproduce_command=False, timeout=None): """Simple wrapper for ansible-playbook. @@ -342,11 +342,6 @@ def run_ansible_playbook(playbook, inventory, workdir, playbook_dir=None, and retry purposes. :type reproduce_command: Boolean - :param fail_on_rc: Enable or disable raising an exception whenever the - return code from the playbook execution results in a - non 0 exit code. The default is True. - :type fail_on_rc: Boolean - :param timeout: Timeout for ansible to finish playbook execution (minutes). :type timeout: int """ @@ -703,16 +698,11 @@ def run_ansible_playbook(playbook, inventory, workdir, playbook_dir=None, if not quiet: LOG.error(err_msg) - - if fail_on_rc: - raise RuntimeError(err_msg) + raise RuntimeError(err_msg) LOG.info( 'Ansible execution success. playbook: {}'.format( - playbook - ) - ) - return rc, status + playbook)) def convert(data): diff --git a/tripleoclient/v1/tripleo_deploy.py b/tripleoclient/v1/tripleo_deploy.py index 5272cfc26..c421b3af1 100644 --- a/tripleoclient/v1/tripleo_deploy.py +++ b/tripleoclient/v1/tripleo_deploy.py @@ -1221,7 +1221,7 @@ class Deploy(command.Command): # Set default plan if not specified by user self._set_default_plan() - rc = 1 + is_complete = False try: # NOTE(bogdando): Look for the unique virtual update mark matching # the heat stack name we are going to create below. If found the @@ -1313,7 +1313,7 @@ class Deploy(command.Command): operation[k] = ','.join([operation[k], v]) else: operation[k] = v - rc = utils.run_ansible_playbook( + utils.run_ansible_playbook( inventory=os.path.join( self.ansible_dir, 'inventory.yaml' @@ -1321,15 +1321,8 @@ class Deploy(command.Command): workdir=self.ansible_dir, verbosity=utils.playbook_verbosity(self=self), extra_env_variables=extra_env_var, - fail_on_rc=False, - **operation - )[0] - if rc != 0: - break - except Exception as e: - self.log.error("Exception: %s" % six.text_type(e)) - self.log.error(traceback.print_exc()) - raise exceptions.DeploymentError(six.text_type(e)) + **operation) + is_complete = True finally: if not parsed_args.keep_running: self._kill_heat(parsed_args) @@ -1348,7 +1341,7 @@ class Deploy(command.Command): if tar_filename: self.log.warning('Install artifact is located at %s' % tar_filename) - if not parsed_args.output_only and rc != 0: + if not is_complete: # We only get here on error. # Alter the stack virtual state for failed deployments if (self.stack_update_mark and @@ -1366,7 +1359,6 @@ class Deploy(command.Command): self.log.error(DEPLOY_FAILURE_MESSAGE.format( self.heat_launch.install_tmp )) - raise exceptions.DeploymentError('Deployment failed.') else: # We only get here if no errors if parsed_args.output_only: @@ -1402,21 +1394,18 @@ class Deploy(command.Command): _('Not creating the stack %s virtual update mark ' 'file') % parsed_args.stack) - return rc - def take_action(self, parsed_args): self.log.debug("take_action(%s)" % parsed_args) - try: if parsed_args.standalone: - if self._standalone_deploy(parsed_args) != 0: - msg = _('Deployment failed.') - self.log.error(msg) - raise exceptions.DeploymentError(msg) + self._standalone_deploy(parsed_args) else: msg = _('Non-standalone is currently not supported') self.log.error(msg) - raise exceptions.DeploymentError(msg) + except Exception as ex: + self.log.error("Exception: %s" % six.text_type(ex)) + self.log.error(traceback.print_exc()) + raise exceptions.DeploymentError(six.text_type(ex)) finally: # Copy clouds.yaml from /etc/openstack so credentials can be # read by the deployment user and not only root.