From fdd988948879ec20ac3b4c06a3097e52d94b795b Mon Sep 17 00:00:00 2001 From: Jiri Podivin Date: Fri, 20 Aug 2021 16:55:48 +0200 Subject: [PATCH] Eliminating a race condition in the export_stack function The correct export of the stack parameters no longer depends on order of the tripleclient.constants.EXPORT_DATA, as the lack of expected json file with parameters to export raises exception, rather than filling the requested key with data obtained in the previous iteration. The I/O logic was moved to the tripleoclient.utils to keep things more consistent. No longer necessary json import was removed. Tests were adjusted to mock more selectively. Closes-Bug: #1940685 Signed-off-by: Jiri Podivin Change-Id: Ia7fb232261eb00c53d4e9bb352e572568704000f --- tripleoclient/export.py | 13 +----------- tripleoclient/tests/test_export.py | 33 +++++++++++++++++++----------- tripleoclient/utils.py | 23 +++++++++++++++++++++ 3 files changed, 45 insertions(+), 24 deletions(-) diff --git a/tripleoclient/export.py b/tripleoclient/export.py index 1ad53d242..f2ee66d84 100644 --- a/tripleoclient/export.py +++ b/tripleoclient/export.py @@ -14,7 +14,6 @@ # -import json import logging import os import re @@ -104,17 +103,7 @@ def export_stack(heat, stack, should_filter=False, file = os.path.join(config_download_dir, stack, export_param["file"]) - - if not os.path.exists(file): - LOG.warning('File %s was not found during export' % - file) - with open(file, 'r') as parameter_file: - try: - export_data = json.load(parameter_file) - except (TypeError, json.JSONDecodeError) as e: - LOG.error( - _('Could not read file %s') % file) - LOG.error(e) + export_data = oooutils.get_parameter_file(file) else: # get stack data export_data = oooutils.get_stack_output_item( diff --git a/tripleoclient/tests/test_export.py b/tripleoclient/tests/test_export.py index b34018e44..d65bba852 100644 --- a/tripleoclient/tests/test_export.py +++ b/tripleoclient/tests/test_export.py @@ -75,11 +75,13 @@ class TestExport(TestCase): } self.mock_open_ceph_all = mock.mock_open(read_data=str(ceph_all)) + @mock.patch('tripleoclient.utils.os.path.exists', + autospec=True, reutrn_value=True) @mock.patch('tripleoclient.utils.get_stack') - def test_export_stack(self, mock_get_stack): + def test_export_stack(self, mock_get_stack, mock_exists): heat = mock.Mock() mock_get_stack.return_value = self.mock_stack - with mock.patch('six.moves.builtins.open', self.mock_open): + with mock.patch('tripleoclient.utils.open', self.mock_open): data = export.export_stack(heat, "overcloud") expected = \ @@ -95,8 +97,10 @@ class TestExport(TestCase): 'config-download/overcloud/group_vars/overcloud.json'), 'r') + @mock.patch('tripleoclient.utils.os.path.exists', + autospec=True, reutrn_value=True) @mock.patch('tripleoclient.utils.get_stack') - def test_export_stack_should_filter(self, mock_get_stack): + def test_export_stack_should_filter(self, mock_get_stack, mock_exists): heat = mock.Mock() mock_get_stack.return_value = self.mock_stack self.mock_open = mock.mock_open( @@ -117,29 +121,33 @@ class TestExport(TestCase): 'config-download/overcloud/group_vars/overcloud.json'), 'r') + @mock.patch('tripleoclient.utils.os.path.exists', + autospec=True, reutrn_value=True) @mock.patch('tripleoclient.utils.get_stack') - def test_export_stack_cd_dir(self, mock_get_stack): + def test_export_stack_cd_dir(self, mock_get_stack, mock_exists): heat = mock.Mock() mock_get_stack.return_value = self.mock_stack - with mock.patch('six.moves.builtins.open', self.mock_open): + with mock.patch('tripleoclient.utils.open', self.mock_open): export.export_stack(heat, "overcloud", config_download_dir='/foo') self.mock_open.assert_called_once_with( '/foo/overcloud/group_vars/overcloud.json', 'r') + @mock.patch('tripleoclient.utils.os.path.exists', + autospec=True, reutrn_value=True) @mock.patch('tripleoclient.utils.get_stack') - def test_export_stack_stack_name(self, mock_get_stack): + def test_export_stack_stack_name(self, mock_get_stack, mock_exists): heat = mock.Mock() mock_get_stack.return_value = self.mock_stack - with mock.patch('six.moves.builtins.open', self.mock_open): + with mock.patch('tripleoclient.utils.open', self.mock_open): export.export_stack(heat, "control") mock_get_stack.assert_called_once_with(heat, 'control') - @mock.patch('tripleoclient.export.LOG.error', autospec=True) - @mock.patch('tripleoclient.export.json.load', autospec=True, + @mock.patch('tripleoclient.utils.LOG.error', autospec=True) + @mock.patch('tripleoclient.utils.json.load', autospec=True, side_effect=JSONDecodeError) - @mock.patch('tripleoclient.export.open') - @mock.patch('tripleoclient.export.os.path.exists', autospec=True, + @mock.patch('tripleoclient.utils.open') + @mock.patch('tripleoclient.utils.os.path.exists', autospec=True, return_value=True) @mock.patch('tripleoclient.utils.get_stack', autospec=True) def test_export_stack_decode_error(self, mock_get_stack, mock_exists, @@ -147,7 +155,8 @@ class TestExport(TestCase): heat = mock.MagicMock() mock_get_stack.return_value = self.mock_stack - export.export_stack(heat, "overcloud") + self.assertRaises( + RuntimeError, export.export_stack, heat, "overcloud") mock_open.assert_called_once_with( os.path.join( diff --git a/tripleoclient/utils.py b/tripleoclient/utils.py index 048ffcb26..9bf9c55ab 100644 --- a/tripleoclient/utils.py +++ b/tripleoclient/utils.py @@ -26,6 +26,7 @@ import errno import getpass import glob import hashlib +import json import logging import multiprocessing @@ -3052,3 +3053,25 @@ def parse_container_image_prepare(tht_key='ContainerImagePrepare', else: raise RuntimeError("Unsupported tht_key: %s" % tht_key) return image_map + + +def get_parameter_file(path): + """Retrieve parameter json file from the supplied path. + If the file doesn't exist, or if the decoding fails, log the failure + and return `None`. + :param path: path to the parameter file + :dtype path: `string` + """ + file_data = None + if os.path.exists(path): + with open(path, 'r') as parameter_file: + try: + file_data = json.load(parameter_file) + except (TypeError, json.JSONDecodeError) as e: + LOG.error( + _('Could not read file %s') % path) + LOG.error(e) + else: + LOG.warning('File %s was not found during export' % + path) + return file_data