openstack/python-tripleoclient
Code Issues Proposed changes

Eliminating a race condition in the export_stack function

Browse Source 
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 <jpodivin@redhat.com>
Change-Id: Ia7fb232261eb00c53d4e9bb352e572568704000f
(cherry picked from commit fdd9889488)
This commit is contained in:
Jiri Podivin
2021-08-20 16:55:48 +02:00
parent f5f75c229b
commit ef151db52e
3 changed files with 45 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files

13
tripleoclient/export.py
View File

@@ -14,7 +14,6 @@
#
import json
import logging
import os
import re
@@ -93,17 +92,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(

33
tripleoclient/tests/test_export.py
View File

@@ -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(

23
tripleoclient/utils.py
View File

@@ -26,6 +26,7 @@ import errno
import getpass
import glob
import hashlib
import json
import logging
import multiprocessing
@@ -2870,3 +2871,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