From 60c0b9c0af8c056b952c782dc00ccfa84a8b3e05 Mon Sep 17 00:00:00 2001 From: smartu3 Date: Thu, 17 May 2018 11:29:59 +0800 Subject: [PATCH] Replace os.makedirs to avoid process race Use oslo_utils.fileutils.ensure_tree(path, mode=_DEFAULT_MODE) to replace os.makedir. Co-Authored-By: ycx Change-Id: Ia5de63768171aa715da6318e768cce1420fef302 Closes-Bug: #1757521 --- cinder/image/image_utils.py | 8 ++--- cinder/tests/unit/test_image_utils.py | 32 ++++++++------------ cinder/volume/flows/manager/create_volume.py | 7 ++--- 3 files changed, 18 insertions(+), 29 deletions(-) diff --git a/cinder/image/image_utils.py b/cinder/image/image_utils.py index 428cbdf6e41..40688a83022 100644 --- a/cinder/image/image_utils.py +++ b/cinder/image/image_utils.py @@ -620,9 +620,7 @@ def coalesce_vhd(vhd_path): def create_temporary_file(*args, **kwargs): - if (CONF.image_conversion_dir and not - os.path.exists(CONF.image_conversion_dir)): - os.makedirs(CONF.image_conversion_dir) + fileutils.ensure_tree(CONF.image_conversion_dir) fd, tmp = tempfile.mkstemp(dir=CONF.image_conversion_dir, *args, **kwargs) os.close(fd) @@ -664,9 +662,7 @@ def temporary_file(*args, **kwargs): def temporary_dir(): - if (CONF.image_conversion_dir and not - os.path.exists(CONF.image_conversion_dir)): - os.makedirs(CONF.image_conversion_dir) + fileutils.ensure_tree(CONF.image_conversion_dir) return utils.tempdir(dir=CONF.image_conversion_dir) diff --git a/cinder/tests/unit/test_image_utils.py b/cinder/tests/unit/test_image_utils.py index e08c3f62043..e65a9ddb8f2 100644 --- a/cinder/tests/unit/test_image_utils.py +++ b/cinder/tests/unit/test_image_utils.py @@ -456,24 +456,22 @@ class TestVerifyImage(test.TestCase): class TestTemporaryDir(test.TestCase): @mock.patch('cinder.image.image_utils.CONF') - @mock.patch('os.makedirs') - @mock.patch('os.path.exists', return_value=True) + @mock.patch('oslo_utils.fileutils.ensure_tree') @mock.patch('cinder.image.image_utils.utils.tempdir') - def test_conv_dir_exists(self, mock_tempdir, mock_exists, mock_make, + def test_conv_dir_exists(self, mock_tempdir, mock_make, mock_conf): mock_conf.image_conversion_dir = mock.sentinel.conv_dir output = image_utils.temporary_dir() - self.assertFalse(mock_make.called) + self.assertTrue(mock_make.called) mock_tempdir.assert_called_once_with(dir=mock.sentinel.conv_dir) self.assertEqual(output, mock_tempdir.return_value) @mock.patch('cinder.image.image_utils.CONF') - @mock.patch('os.makedirs') - @mock.patch('os.path.exists', return_value=False) + @mock.patch('oslo_utils.fileutils.ensure_tree') @mock.patch('cinder.image.image_utils.utils.tempdir') - def test_create_conv_dir(self, mock_tempdir, mock_exists, mock_make, + def test_create_conv_dir(self, mock_tempdir, mock_make, mock_conf): mock_conf.image_conversion_dir = mock.sentinel.conv_dir @@ -484,16 +482,15 @@ class TestTemporaryDir(test.TestCase): self.assertEqual(output, mock_tempdir.return_value) @mock.patch('cinder.image.image_utils.CONF') - @mock.patch('os.makedirs') - @mock.patch('os.path.exists', return_value=False) + @mock.patch('oslo_utils.fileutils.ensure_tree') @mock.patch('cinder.image.image_utils.utils.tempdir') - def test_no_conv_dir(self, mock_tempdir, mock_exists, mock_make, + def test_no_conv_dir(self, mock_tempdir, mock_make, mock_conf): mock_conf.image_conversion_dir = None output = image_utils.temporary_dir() - self.assertFalse(mock_make.called) + self.assertTrue(mock_make.called) mock_tempdir.assert_called_once_with(dir=None) self.assertEqual(output, mock_tempdir.return_value) @@ -1554,11 +1551,10 @@ class TestVhdUtils(test.TestCase): class TestCreateTemporaryFile(test.TestCase): @mock.patch('cinder.image.image_utils.os.close') @mock.patch('cinder.image.image_utils.CONF') - @mock.patch('cinder.image.image_utils.os.path.exists') @mock.patch('cinder.image.image_utils.os.makedirs') @mock.patch('cinder.image.image_utils.tempfile.mkstemp') def test_create_temporary_file_no_dir(self, mock_mkstemp, mock_dirs, - mock_path, mock_conf, mock_close): + mock_conf, mock_close): mock_conf.image_conversion_dir = None fd = mock.sentinel.file_descriptor path = mock.sentinel.absolute_pathname @@ -1572,11 +1568,10 @@ class TestCreateTemporaryFile(test.TestCase): @mock.patch('cinder.image.image_utils.os.close') @mock.patch('cinder.image.image_utils.CONF') - @mock.patch('cinder.image.image_utils.os.path.exists', return_value=True) @mock.patch('cinder.image.image_utils.os.makedirs') @mock.patch('cinder.image.image_utils.tempfile.mkstemp') def test_create_temporary_file_with_dir(self, mock_mkstemp, mock_dirs, - mock_path, mock_conf, mock_close): + mock_conf, mock_close): conv_dir = mock.sentinel.image_conversion_dir mock_conf.image_conversion_dir = conv_dir fd = mock.sentinel.file_descriptor @@ -1586,17 +1581,16 @@ class TestCreateTemporaryFile(test.TestCase): output = image_utils.create_temporary_file() self.assertEqual(path, output) - self.assertFalse(mock_dirs.called) + self.assertTrue(mock_dirs.called) mock_mkstemp.assert_called_once_with(dir=conv_dir) mock_close.assert_called_once_with(fd) @mock.patch('cinder.image.image_utils.os.close') @mock.patch('cinder.image.image_utils.CONF') - @mock.patch('cinder.image.image_utils.os.path.exists', return_value=False) - @mock.patch('cinder.image.image_utils.os.makedirs') + @mock.patch('cinder.image.image_utils.fileutils.ensure_tree') @mock.patch('cinder.image.image_utils.tempfile.mkstemp') def test_create_temporary_file_and_dir(self, mock_mkstemp, mock_dirs, - mock_path, mock_conf, mock_close): + mock_conf, mock_close): conv_dir = mock.sentinel.image_conversion_dir mock_conf.image_conversion_dir = conv_dir fd = mock.sentinel.file_descriptor diff --git a/cinder/volume/flows/manager/create_volume.py b/cinder/volume/flows/manager/create_volume.py index 03068704227..f7bbb771f75 100644 --- a/cinder/volume/flows/manager/create_volume.py +++ b/cinder/volume/flows/manager/create_volume.py @@ -10,13 +10,13 @@ # License for the specific language governing permissions and limitations # under the License. -import os import traceback from oslo_concurrency import processutils from oslo_config import cfg from oslo_log import log as logging from oslo_utils import excutils +from oslo_utils import fileutils from oslo_utils import timeutils import taskflow.engines from taskflow.patterns import linear_flow @@ -762,9 +762,8 @@ class CreateVolumeFromSpecTask(flow_utils.CinderTask): # NOTE(mnaser): This check *only* happens if the backend is not able # to clone volumes and we have to resort to downloading # the image from Glance and uploading it. - if (CONF.image_conversion_dir and not - os.path.exists(CONF.image_conversion_dir)): - os.makedirs(CONF.image_conversion_dir) + if CONF.image_conversion_dir: + fileutils.ensure_tree(CONF.image_conversion_dir) try: image_utils.check_available_space( CONF.image_conversion_dir,