From 190ba2acd0fb5b99cbd3561c2196c0cc1f6ccfeb Mon Sep 17 00:00:00 2001 From: Jiri Podivin Date: Mon, 3 May 2021 11:36:29 +0200 Subject: [PATCH] Improved log path handling Validations logging now has several documented fallbacks. constants.py now includes constant default path, in order to improve readability and provide reference. Tests are included. Complementary patch for validations_commons callbacks is optional, as the behavior should stay largely the same. Co-Authored-By: mbu Change-Id: Iab5f246f22fa6998ea6cec578eec6ebb5de13ad4 --- validations_libs/constants.py | 4 +- validations_libs/tests/test_utils.py | 12 --- .../tests/test_validation_actions.py | 90 +++++++++++-------- validations_libs/utils.py | 42 +++++++-- validations_libs/validation_actions.py | 10 ++- 5 files changed, 93 insertions(+), 65 deletions(-) diff --git a/validations_libs/constants.py b/validations_libs/constants.py index 19a61d31..da5f9f4e 100644 --- a/validations_libs/constants.py +++ b/validations_libs/constants.py @@ -20,9 +20,7 @@ ANSIBLE_VALIDATION_DIR = '/usr/share/ansible/validation-playbooks' VALIDATION_GROUPS_INFO = '%s/groups.yaml' % DEFAULT_VALIDATIONS_BASEDIR -VALIDATIONS_LOG_BASEDIR = ('/var/log/validations' - if os.path.exists('/var/log/validations') else - os.getcwd()) +VALIDATIONS_LOG_BASEDIR = os.path.join(os.environ.get('HOME'), 'validations') VALIDATION_ANSIBLE_ARTIFACT_PATH = '{}/artifacts/'.format( VALIDATIONS_LOG_BASEDIR) diff --git a/validations_libs/tests/test_utils.py b/validations_libs/tests/test_utils.py index 655c602f..45b3eff7 100644 --- a/validations_libs/tests/test_utils.py +++ b/validations_libs/tests/test_utils.py @@ -47,18 +47,6 @@ class TestUtils(TestCase): utils.get_validations_data, validation) - @mock.patch('validations_libs.utils.current_time', - return_value='2020-04-02T06:58:20.352272Z') - @mock.patch('os.makedirs') - @mock.patch('uuid.uuid4', return_value='1234') - def test_create_artifacts_dir(self, mock_uuid, mock_makedirs, - mock_datetime): - uuid, dir_path = utils.create_artifacts_dir(dir_path='/tmp/foo', - prefix='ntp') - self.assertEqual(uuid, '1234') - self.assertEqual(dir_path, - '/tmp/foo/1234_ntp_2020-04-02T06:58:20.352272Z') - @mock.patch('yaml.safe_load', return_value=fakes.FAKE_PLAYBOOK) @mock.patch('six.moves.builtins.open') @mock.patch('glob.glob') diff --git a/validations_libs/tests/test_validation_actions.py b/validations_libs/tests/test_validation_actions.py index e2a6dbaf..4f2783e7 100644 --- a/validations_libs/tests/test_validation_actions.py +++ b/validations_libs/tests/test_validation_actions.py @@ -45,11 +45,9 @@ class TestValidationActions(TestCase): 'My Validation Two Name', ['prep', 'pre-introspection'])])) - @mock.patch('validations_libs.utils.create_artifacts_dir', - return_value=('1234', '/tmp/')) @mock.patch('validations_libs.utils.get_validations_playbook', return_value=['/tmp/foo/fake.yaml']) - def test_validation_skip_validation(self, mock_validation_play, mock_tmp): + def test_validation_skip_validation(self, mock_validation_play): playbook = ['fake.yaml'] inventory = 'tmp/inventory.yaml' @@ -66,16 +64,26 @@ class TestValidationActions(TestCase): limit_hosts=None) self.assertEqual(run_return, []) + @mock.patch('validations_libs.utils.current_time', + return_value='time') + @mock.patch('validations_libs.utils.uuid.uuid4', + return_value='123') + @mock.patch('validations_libs.utils.os.makedirs') + @mock.patch('validations_libs.utils.os.path.exists', + return_value=True) @mock.patch('validations_libs.utils.get_validations_playbook', return_value=['/tmp/foo/fake.yaml']) @mock.patch('validations_libs.ansible.Ansible.run') - @mock.patch('validations_libs.utils.create_artifacts_dir', - return_value=('1234', '/tmp/')) - def test_validation_skip_on_specific_host(self, mock_tmp, mock_ansible_run, - mock_validation_play): + def test_validation_skip_on_specific_host(self, mock_ansible_run, + mock_validation_play, + mock_exists, + mock_makedirs, + mock_uuid, + mock_time): + mock_ansible_run.return_value = ('fake.yaml', 0, 'successful') run_called_args = { - 'workdir': '/tmp/', + 'workdir': '/var/log/validations/artifacts/123_fake.yaml_time', 'playbook': '/tmp/foo/fake.yaml', 'base_dir': '/usr/share/ansible/', 'playbook_dir': '/tmp/foo', @@ -86,11 +94,11 @@ class TestValidationActions(TestCase): 'quiet': True, 'extra_vars': None, 'limit_hosts': '!cloud1', - 'ansible_artifact_path': '/tmp/', 'extra_env_variables': None, 'ansible_cfg': None, 'gathering_policy': 'explicit', - 'log_path': None, + 'ansible_artifact_path': '/var/log/validations/artifacts/123_fake.yaml_time', + 'log_path': '/var/log/validations', 'run_async': False, 'python_interpreter': None, 'ssh_user': None @@ -106,22 +114,33 @@ class TestValidationActions(TestCase): run = ValidationActions() run_return = run.run_validations(playbook, inventory, + log_path='/var/log/validations', validations_dir='/tmp/foo', skip_list=skip_list, limit_hosts='!cloud1', ) mock_ansible_run.assert_called_with(**run_called_args) + @mock.patch('validations_libs.utils.current_time', + return_value='time') + @mock.patch('validations_libs.utils.uuid.uuid4', + return_value='123') + @mock.patch('validations_libs.utils.os.makedirs') + @mock.patch('validations_libs.utils.os.path.exists', + return_value=True) @mock.patch('validations_libs.utils.get_validations_playbook', return_value=['/tmp/foo/fake.yaml']) @mock.patch('validations_libs.ansible.Ansible.run') - @mock.patch('validations_libs.utils.create_artifacts_dir', - return_value=('1234', '/tmp/')) - def test_validation_skip_with_limit_host(self, mock_tmp, mock_ansible_run, - mock_validation_play): + def test_validation_skip_with_limit_host(self, mock_ansible_run, + mock_validation_play, + mock_exists, + mock_makedirs, + mock_uuid, + mock_time): + mock_ansible_run.return_value = ('fake.yaml', 0, 'successful') run_called_args = { - 'workdir': '/tmp/', + 'workdir': '/var/log/validations/artifacts/123_fake.yaml_time', 'playbook': '/tmp/foo/fake.yaml', 'base_dir': '/usr/share/ansible/', 'playbook_dir': '/tmp/foo', @@ -135,8 +154,8 @@ class TestValidationActions(TestCase): 'extra_env_variables': None, 'ansible_cfg': None, 'gathering_policy': 'explicit', - 'ansible_artifact_path': '/tmp/', - 'log_path': None, + 'ansible_artifact_path': '/var/log/validations/artifacts/123_fake.yaml_time', + 'log_path': '/var/log/validations', 'run_async': False, 'python_interpreter': None, 'ssh_user': None @@ -152,40 +171,31 @@ class TestValidationActions(TestCase): run = ValidationActions() run_return = run.run_validations(playbook, inventory, + log_path='/var/log/validations', validations_dir='/tmp/foo', skip_list=skip_list, limit_hosts='cloud,cloud1,!cloud2') + mock_ansible_run.assert_called_with(**run_called_args) - @mock.patch('validations_libs.validation_logs.ValidationLogs.get_results') + @mock.patch('validations_libs.validation_actions.ValidationLogs.get_results', + side_effect=fakes.FAKE_SUCCESS_RUN) @mock.patch('validations_libs.utils.parse_all_validations_on_disk') @mock.patch('validations_libs.ansible.Ansible.run') - @mock.patch('validations_libs.utils.create_artifacts_dir', - return_value=('1234', '/tmp/')) - def test_validation_run_success(self, mock_tmp, mock_ansible_run, - mock_validation_dir, mock_results): + def test_validation_run_success(self, mock_ansible_run, + mock_validation_dir, + mock_results): + mock_validation_dir.return_value = [{ 'description': 'My Validation One Description', 'groups': ['prep', 'pre-deployment'], 'id': 'foo', 'name': 'My Validition One Name', 'parameters': {}}] + mock_ansible_run.return_value = ('foo.yaml', 0, 'successful') - mock_results.return_value = [{'Duration': '0:00:01.761', - 'Host_Group': 'overcloud', - 'Status': 'PASSED', - 'Status_by_Host': 'subnode-1,PASSED', - 'UUID': 'foo', - 'Unreachable_Hosts': '', - 'Validations': 'ntp'}] - expected_run_return = [{'Duration': '0:00:01.761', - 'Host_Group': 'overcloud', - 'Status': 'PASSED', - 'Status_by_Host': 'subnode-1,PASSED', - 'UUID': 'foo', - 'Unreachable_Hosts': '', - 'Validations': 'ntp'}] + expected_run_return = fakes.FAKE_SUCCESS_RUN[0] playbook = ['fake.yaml'] inventory = 'tmp/inventory.yaml' @@ -209,17 +219,18 @@ class TestValidationActions(TestCase): @mock.patch('validations_libs.validation_logs.ValidationLogs.get_results') @mock.patch('validations_libs.utils.parse_all_validations_on_disk') @mock.patch('validations_libs.ansible.Ansible.run') - @mock.patch('validations_libs.utils.create_artifacts_dir', - return_value=('1234', '/tmp/')) - def test_validation_run_failed(self, mock_tmp, mock_ansible_run, + def test_validation_run_failed(self, mock_ansible_run, mock_validation_dir, mock_results): + mock_validation_dir.return_value = [{ 'description': 'My Validation One Description', 'groups': ['prep', 'pre-deployment'], 'id': 'foo', 'name': 'My Validition One Name', 'parameters': {}}] + mock_ansible_run.return_value = ('foo.yaml', 0, 'failed') + mock_results.return_value = [{'Duration': '0:00:01.761', 'Host_Group': 'overcloud', 'Status': 'PASSED', @@ -227,6 +238,7 @@ class TestValidationActions(TestCase): 'UUID': 'foo', 'Unreachable_Hosts': '', 'Validations': 'ntp'}] + expected_run_return = [{'Duration': '0:00:01.761', 'Host_Group': 'overcloud', 'Status': 'PASSED', diff --git a/validations_libs/utils.py b/validations_libs/utils.py index 7f8ac791..0f7c2de2 100644 --- a/validations_libs/utils.py +++ b/validations_libs/utils.py @@ -32,25 +32,51 @@ def current_time(): return '%sZ' % datetime.datetime.utcnow().isoformat() -def create_artifacts_dir(dir_path=None, prefix=None): - """Create Ansible artifacts directory +def create_log_dir(log_path=constants.VALIDATIONS_LOG_BASEDIR): + """Create default validations log dir. + Log the failure if encountering OSError or PermissionError. + :raises: RuntimeError + """ + try: + if os.path.exists(log_path): + if os.access(log_path, os.W_OK): + return log_path + else: + create_log_dir(constants.VALIDATIONS_LOG_BASEDIR) + else: + os.makedirs(log_path) + return log_path + except (OSError, PermissionError): + LOG.error( + ( + "Error while creating the log directory. " + "Please check the access rights for: '{}'" + ).format(log_path) + ) + # Fallback in default path if log_path != from constants path + if log_path != constants.VALIDATIONS_LOG_BASEDIR: + create_log_dir(constants.VALIDATIONS_LOG_BASEDIR) + raise RuntimeError() - :param dir_path: Directory asbolute path - :type dir_path: `string` + +def create_artifacts_dir(log_path=constants.VALIDATIONS_LOG_BASEDIR, + prefix=None): + """Create Ansible artifacts directory + :param log_path: Directory asbolute path + :type log_path: `string` :param prefix: Playbook name :type prefix: `string` :return: The UUID of the validation and the absolute Path of the log file :rtype: `string`, `string` """ - dir_path = (dir_path if dir_path else - constants.VALIDATION_ANSIBLE_ARTIFACT_PATH) + artifact_dir = os.path.join(log_path, 'artifacts') validation_uuid = str(uuid.uuid4()) - log_dir = "{}/{}_{}_{}".format(dir_path, validation_uuid, + log_dir = "{}/{}_{}_{}".format(artifact_dir, validation_uuid, (prefix if prefix else ''), current_time()) try: os.makedirs(log_dir) return validation_uuid, log_dir - except OSError: + except (OSError, PermissionError): LOG.exception( ( "Error while creating Ansible artifacts log file." diff --git a/validations_libs/validation_actions.py b/validations_libs/validation_actions.py index c2e32e01..38152d20 100644 --- a/validations_libs/validation_actions.py +++ b/validations_libs/validation_actions.py @@ -228,7 +228,8 @@ class ValidationActions(object): extra_env_vars=None, ansible_cfg=None, quiet=True, workdir=None, limit_hosts=None, run_async=False, base_dir=constants.DEFAULT_VALIDATIONS_BASEDIR, - log_path=None, python_interpreter=None, + log_path=constants.VALIDATIONS_LOG_BASEDIR, + python_interpreter=None, skip_list=None, callback_whitelist=None, output_callback='validation_stdout', @@ -268,6 +269,8 @@ class ValidationActions(object): ``constants.DEFAULT_VALIDATIONS_BASEDIR``) :type base_dir: ``string`` :param log_path: The absolute path of the validations logs directory + (Defaults to + ``constants.VALIDATIONS_LOG_BASEDIR``) :type log_path: ``string`` :param python_interpreter: Path to the Python interpreter to be used for module execution on remote targets, @@ -353,6 +356,7 @@ class ValidationActions(object): else: raise RuntimeError("No validations found") + log_path = v_utils.create_log_dir(log_path) self.log.debug('Running the validations with Ansible') results = [] for playbook in playbooks: @@ -363,7 +367,7 @@ class ValidationActions(object): limit_hosts) if _play: validation_uuid, artifacts_dir = v_utils.create_artifacts_dir( - dir_path=log_path, prefix=os.path.basename(playbook)) + log_path=log_path, prefix=os.path.basename(playbook)) run_ansible = v_ansible(validation_uuid) _playbook, _rc, _status = run_ansible.run( workdir=artifacts_dir, @@ -398,7 +402,7 @@ class ValidationActions(object): return results # Return log results uuid = [id['UUID'] for id in results] - vlog = ValidationLogs() + vlog = ValidationLogs(log_path) return vlog.get_results(uuid) def group_information(self, groups):