Improve use of temporary_file and temporary_dir

cinder/image/image_utils.py contains the functions temporary_file and
temporary_dir for use with handling temporary data.

temporary_file automatically creates files in CONF.image_conversion_dir
and will create CONF.image_conversion_dir if required. This commit
removes duplicate code that checks for/creates the dir before calling
temporary_file. This commit also replaces code that duplicates the
functionality of temporary_file with temporary_file.

temporary_dir requires that CONF.image_conversion_dir exists. This
commit adds a check to the function that creates it if it is missing.

Change-Id: Idecba85e19a0fe16dfc9f840913857137bfeee1b

diff --git a/cinder/image/image_utils.py b/cinder/image/image_utils.py
index 9a56b18ff19bfd73cd6f806f4eafb36b9cdabd15..7aa6a4da6778a850341075d2da97a6c2ad5e6257 100644 (file)
--- a/cinder/image/image_utils.py
+++ b/cinder/image/image_utils.py
@@ -192,10 +192,6 @@ def fetch_to_volume_format(context, image_service,
                            image_id, dest, volume_format, blocksize,
                            user_id=None, project_id=None, size=None,
                            run_as_root=True):
-    if (CONF.image_conversion_dir and not
-            os.path.exists(CONF.image_conversion_dir)):
-        os.makedirs(CONF.image_conversion_dir)
-
     qemu_img = True
     image_meta = image_service.show(context, image_id)
 
@@ -308,13 +304,7 @@ def upload_volume(context, image_service, image_meta, volume_path,
                     image_service.update(context, image_id, {}, image_file)
         return
 
-    if (CONF.image_conversion_dir and not
-            os.path.exists(CONF.image_conversion_dir)):
-        os.makedirs(CONF.image_conversion_dir)
-
-    fd, tmp = tempfile.mkstemp(dir=CONF.image_conversion_dir)
-    os.close(fd)
-    with fileutils.remove_path_on_error(tmp):
+    with temporary_file() as tmp:
         LOG.debug("%s was %s, converting to %s" %
                   (image_id, volume_format, image_meta['disk_format']))
         convert_image(volume_path, tmp, image_meta['disk_format'],
@@ -330,7 +320,6 @@ def upload_volume(context, image_service, image_meta, volume_path,
 
         with fileutils.file_open(tmp, 'rb') as image_file:
             image_service.update(context, image_id, {}, image_file)
-        fileutils.delete_if_exists(tmp)
 
 
 def is_xenserver_image(context, image_service, image_id):
@@ -403,6 +392,10 @@ 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)
+
     return utils.tempdir(dir=CONF.image_conversion_dir)
 
 

diff --git a/cinder/tests/test_image_utils.py b/cinder/tests/test_image_utils.py
index f26afd9389e47298cb8b6efcdeb8cc48dd4f2bf7..31ba6cdfdba1e5dd6424e1254029e37988998a6d 100644 (file)
--- a/cinder/tests/test_image_utils.py
+++ b/cinder/tests/test_image_utils.py
@@ -16,7 +16,6 @@
 """Unit tests for image utils."""
 
 import contextlib
-import tempfile
 
 import mock
 import mox
@@ -439,118 +438,6 @@ class TestUtils(test.TestCase):
 
         self._test_fetch_verify_image(TEST_RETURN)
 
-    @mock.patch('os.stat')
-    def test_upload_volume(self, mock_stat, bps_limit=0):
-        image_meta = {'id': 1, 'disk_format': 'qcow2'}
-        TEST_RET = "image: qemu.qcow2\n"\
-                   "file_format: qcow2 \n"\
-                   "virtual_size: 50M (52428800 bytes)\n"\
-                   "cluster_size: 65536\n"\
-                   "disk_size: 196K (200704 bytes)"
-
-        if bps_limit:
-            self.override_config('volume_copy_bps_limit', bps_limit)
-            prefix = ('cgexec', '-g', 'blkio:test')
-        else:
-            prefix = ()
-
-        cmd = prefix + ('qemu-img', 'convert', '-O', 'qcow2',
-                        mox.IgnoreArg(), mox.IgnoreArg())
-
-        m = self._mox
-        m.StubOutWithMock(utils, 'execute')
-        m.StubOutWithMock(volume_utils, 'setup_blkio_cgroup')
-
-        volume_utils.setup_blkio_cgroup(mox.IgnoreArg(), mox.IgnoreArg(),
-                                        bps_limit).AndReturn(prefix)
-
-        utils.execute(*cmd, run_as_root=True)
-        utils.execute(
-            'env', 'LC_ALL=C', 'qemu-img', 'info',
-            mox.IgnoreArg(), run_as_root=True).AndReturn(
-                (TEST_RET, 'ignored'))
-
-        m.ReplayAll()
-
-        image_utils.upload_volume(context, FakeImageService(),
-                                  image_meta, '/dev/loop1')
-        m.VerifyAll()
-
-    @mock.patch('os.stat')
-    def test_upload_volume_with_bps_limit(self, mock_stat):
-        bps_limit = 1048576
-        image_meta = {'id': 1, 'disk_format': 'qcow2'}
-        TEST_RET = "image: qemu.qcow2\n"\
-                   "file_format: qcow2 \n"\
-                   "virtual_size: 50M (52428800 bytes)\n"\
-                   "cluster_size: 65536\n"\
-                   "disk_size: 196K (200704 bytes)"
-
-        self.override_config('volume_copy_bps_limit', bps_limit)
-        prefix = ('cgexec', '-g', 'blkio:test')
-
-        cmd = prefix + ('qemu-img', 'convert', '-O', 'qcow2',
-                        mox.IgnoreArg(), mox.IgnoreArg())
-
-        m = self._mox
-        m.StubOutWithMock(utils, 'execute')
-        m.StubOutWithMock(volume_utils, 'setup_blkio_cgroup')
-        m.StubOutWithMock(volume_utils, 'check_for_odirect_support')
-
-        volume_utils.setup_blkio_cgroup(mox.IgnoreArg(), mox.IgnoreArg(),
-                                        bps_limit).AndReturn(prefix)
-        utils.execute(*cmd, run_as_root=True)
-        utils.execute(
-            'env', 'LC_ALL=C', 'qemu-img', 'info',
-            mox.IgnoreArg(), run_as_root=True).AndReturn(
-                (TEST_RET, 'ignored'))
-
-        m.ReplayAll()
-        image_utils.upload_volume(context, FakeImageService(),
-                                  image_meta, '/dev/loop1')
-        m.VerifyAll()
-
-    def test_upload_volume_with_raw_image(self):
-        image_meta = {'id': 1, 'disk_format': 'raw'}
-        mox = self._mox
-
-        mox.StubOutWithMock(image_utils, 'convert_image')
-
-        mox.ReplayAll()
-
-        with tempfile.NamedTemporaryFile() as f:
-            image_utils.upload_volume(context, FakeImageService(),
-                                      image_meta, f.name)
-        mox.VerifyAll()
-
-    @mock.patch('os.stat')
-    def test_upload_volume_on_error(self, mock_stat):
-        image_meta = {'id': 1, 'disk_format': 'qcow2'}
-        TEST_RET = "image: qemu.vhd\n"\
-                   "file_format: vhd \n"\
-                   "virtual_size: 50M (52428800 bytes)\n"\
-                   "cluster_size: 65536\n"\
-                   "disk_size: 196K (200704 bytes)"
-
-        m = self._mox
-        m.StubOutWithMock(utils, 'execute')
-        m.StubOutWithMock(volume_utils, 'check_for_odirect_support')
-
-        utils.execute('qemu-img', 'convert', '-O', 'qcow2',
-                      mox.IgnoreArg(), mox.IgnoreArg(), run_as_root=True)
-        utils.execute(
-            'env', 'LC_ALL=C', 'qemu-img', 'info',
-            mox.IgnoreArg(), run_as_root=True).AndReturn(
-                (TEST_RET, 'ignored'))
-
-        m.ReplayAll()
-
-        self.assertRaises(exception.ImageUnacceptable,
-                          image_utils.upload_volume,
-                          context, FakeImageService(),
-                          image_meta, '/dev/loop1')
-        m.VerifyAll()
-
 
 class TestExtractTo(test.TestCase):
     def test_extract_to_calls_tar(self):
@@ -764,3 +651,170 @@ class TestXenServerImageToCoalescedVhd(test.TestCase):
         mox.ReplayAll()
         image_utils.replace_xenserver_image_with_coalesced_vhd('image')
         mox.VerifyAll()
+
+
+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('cinder.image.image_utils.utils.tempdir')
+    def test_conv_dir_exists(self, mock_tempdir, mock_exists, mock_make,
+                             mock_conf):
+        mock_conf.image_conversion_dir = mock.sentinel.conv_dir
+
+        output = image_utils.temporary_dir()
+
+        self.assertFalse(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('cinder.image.image_utils.utils.tempdir')
+    def test_create_conv_dir(self, mock_tempdir, mock_exists, mock_make,
+                             mock_conf):
+        mock_conf.image_conversion_dir = mock.sentinel.conv_dir
+
+        output = image_utils.temporary_dir()
+
+        mock_make.assert_called_once_with(mock.sentinel.conv_dir)
+        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('cinder.image.image_utils.utils.tempdir')
+    def test_no_conv_dir(self, mock_tempdir, mock_exists, mock_make,
+                         mock_conf):
+        mock_conf.image_conversion_dir = None
+
+        output = image_utils.temporary_dir()
+
+        self.assertFalse(mock_make.called)
+        mock_tempdir.assert_called_once_with(dir=None)
+        self.assertEqual(output, mock_tempdir.return_value)
+
+
+class TestUploadVolume(test.TestCase):
+    @mock.patch('cinder.image.image_utils.CONF')
+    @mock.patch('cinder.image.image_utils.fileutils.file_open')
+    @mock.patch('cinder.image.image_utils.qemu_img_info')
+    @mock.patch('cinder.image.image_utils.convert_image')
+    @mock.patch('cinder.image.image_utils.temporary_file')
+    @mock.patch('cinder.image.image_utils.os')
+    def test_diff_format(self, mock_os, mock_temp, mock_convert, mock_info,
+                         mock_open, mock_conf):
+        context = mock.sentinel.context
+        image_service = mock.Mock()
+        image_meta = {'id': 'test_id',
+                      'disk_format': mock.sentinel.disk_format}
+        volume_path = mock.sentinel.volume_path
+        mock_os.name = 'posix'
+        mock_conf.volume_copy_bps_limit = mock.sentinel.bps_limit
+        data = mock_info.return_value
+        data.file_format = mock.sentinel.disk_format
+        temp_file = mock_temp.return_value.__enter__.return_value
+
+        output = image_utils.upload_volume(context, image_service, image_meta,
+                                           volume_path)
+
+        self.assertIsNone(output)
+        mock_convert.assert_called_once_with(volume_path,
+                                             temp_file,
+                                             mock.sentinel.disk_format,
+                                             bps_limit=mock.sentinel.bps_limit,
+                                             run_as_root=True)
+        mock_info.assert_called_once_with(temp_file, run_as_root=True)
+        mock_open.assert_called_once_with(temp_file, 'rb')
+        image_service.update.assert_called_once_with(
+            context, image_meta['id'], {},
+            mock_open.return_value.__enter__.return_value)
+
+    @mock.patch('cinder.image.image_utils.utils.temporary_chown')
+    @mock.patch('cinder.image.image_utils.CONF')
+    @mock.patch('cinder.image.image_utils.fileutils.file_open')
+    @mock.patch('cinder.image.image_utils.qemu_img_info')
+    @mock.patch('cinder.image.image_utils.convert_image')
+    @mock.patch('cinder.image.image_utils.temporary_file')
+    @mock.patch('cinder.image.image_utils.os')
+    def test_same_format(self, mock_os, mock_temp, mock_convert, mock_info,
+                         mock_open, mock_conf, mock_chown):
+        context = mock.sentinel.context
+        image_service = mock.Mock()
+        image_meta = {'id': 'test_id',
+                      'disk_format': 'raw'}
+        volume_path = mock.sentinel.volume_path
+        mock_os.name = 'posix'
+        mock_os.access.return_value = False
+
+        output = image_utils.upload_volume(context, image_service, image_meta,
+                                           volume_path)
+
+        self.assertIsNone(output)
+        self.assertFalse(mock_convert.called)
+        self.assertFalse(mock_info.called)
+        mock_chown.assert_called_once_with(volume_path)
+        mock_open.assert_called_once_with(volume_path)
+        image_service.update.assert_called_once_with(
+            context, image_meta['id'], {},
+            mock_open.return_value.__enter__.return_value)
+
+    @mock.patch('cinder.image.image_utils.utils.temporary_chown')
+    @mock.patch('cinder.image.image_utils.CONF')
+    @mock.patch('cinder.image.image_utils.fileutils.file_open')
+    @mock.patch('cinder.image.image_utils.qemu_img_info')
+    @mock.patch('cinder.image.image_utils.convert_image')
+    @mock.patch('cinder.image.image_utils.temporary_file')
+    @mock.patch('cinder.image.image_utils.os')
+    def test_same_format_on_nt(self, mock_os, mock_temp, mock_convert,
+                               mock_info, mock_open, mock_conf, mock_chown):
+        context = mock.sentinel.context
+        image_service = mock.Mock()
+        image_meta = {'id': 'test_id',
+                      'disk_format': 'raw'}
+        volume_path = mock.sentinel.volume_path
+        mock_os.name = 'nt'
+        mock_os.access.return_value = False
+
+        output = image_utils.upload_volume(context, image_service, image_meta,
+                                           volume_path)
+
+        self.assertIsNone(output)
+        self.assertFalse(mock_convert.called)
+        self.assertFalse(mock_info.called)
+        mock_open.assert_called_once_with(volume_path, 'rb')
+        image_service.update.assert_called_once_with(
+            context, image_meta['id'], {},
+            mock_open.return_value.__enter__.return_value)
+
+    @mock.patch('cinder.image.image_utils.CONF')
+    @mock.patch('cinder.image.image_utils.fileutils.file_open')
+    @mock.patch('cinder.image.image_utils.qemu_img_info')
+    @mock.patch('cinder.image.image_utils.convert_image')
+    @mock.patch('cinder.image.image_utils.temporary_file')
+    @mock.patch('cinder.image.image_utils.os')
+    def test_convert_error(self, mock_os, mock_temp, mock_convert, mock_info,
+                           mock_open, mock_conf):
+        context = mock.sentinel.context
+        image_service = mock.Mock()
+        image_meta = {'id': 'test_id',
+                      'disk_format': mock.sentinel.disk_format}
+        volume_path = mock.sentinel.volume_path
+        mock_os.name = 'posix'
+        mock_conf.volume_copy_bps_limit = mock.sentinel.bps_limit
+        data = mock_info.return_value
+        data.file_format = mock.sentinel.other_disk_format
+        temp_file = mock_temp.return_value.__enter__.return_value
+
+        self.assertRaises(exception.ImageUnacceptable,
+                          image_utils.upload_volume,
+                          context, image_service, image_meta, volume_path)
+        mock_convert.assert_called_once_with(volume_path,
+                                             temp_file,
+                                             mock.sentinel.disk_format,
+                                             bps_limit=mock.sentinel.bps_limit,
+                                             run_as_root=True)
+        mock_info.assert_called_once_with(temp_file, run_as_root=True)
+        self.assertFalse(image_service.update.called)

diff --git a/cinder/tests/test_sheepdog.py b/cinder/tests/test_sheepdog.py
index 4bf1f379c18e6f472a6af0c9a0bf3d1237f579b0..ccce1c09366224b95f1c9e94c657716a8206c876 100644 (file)
--- a/cinder/tests/test_sheepdog.py
+++ b/cinder/tests/test_sheepdog.py
@@ -16,8 +16,6 @@
 
 
 import contextlib
-import os
-import tempfile
 
 from oslo.concurrency import processutils
 from oslo.utils import units
@@ -107,17 +105,16 @@ class SheepdogTestCase(test.TestCase):
 
     def test_copy_image_to_volume(self):
         @contextlib.contextmanager
-        def fake_temp_file(dir):
+        def fake_temp_file():
             class FakeTmp:
                 def __init__(self, name):
                     self.name = name
-            yield FakeTmp('test')
+            yield FakeTmp('test').name
 
         def fake_try_execute(obj, *command, **kwargs):
             return True
 
-        self.stubs.Set(tempfile, 'NamedTemporaryFile', fake_temp_file)
-        self.stubs.Set(os.path, 'exists', lambda x: True)
+        self.stubs.Set(image_utils, 'temporary_file', fake_temp_file)
         self.stubs.Set(image_utils, 'fetch_verify_image',
                        lambda w, x, y, z: None)
         self.stubs.Set(image_utils, 'convert_image',

diff --git a/cinder/tests/windows/test_windows.py b/cinder/tests/windows/test_windows.py
index 43c1d3f0fe0b1f8aa7f4f01e39d7bb4f55aef61f..5ec0a2e2a313ac5086fd5c51bd884e9ceca458da 100644 (file)
--- a/cinder/tests/windows/test_windows.py
+++ b/cinder/tests/windows/test_windows.py
@@ -261,7 +261,6 @@ class TestWindowsDriver(test.TestCase):
         self.stubs.Set(windows_utils.WindowsUtils, 'get_supported_vhd_type',
                        fake_get_supported_type)
 
-        self.mox.StubOutWithMock(os, 'makedirs')
         self.mox.StubOutWithMock(os, 'unlink')
         self.mox.StubOutWithMock(image_utils, 'create_temporary_file')
         self.mox.StubOutWithMock(image_utils, 'fetch_to_vhd')

diff --git a/cinder/volume/drivers/sheepdog.py b/cinder/volume/drivers/sheepdog.py
index 75951852380eb968c5d9c13f502ad1f6f21abcbe..1943f6134efef3008367d167f6fdb0067622bbab 100644 (file)
--- a/cinder/volume/drivers/sheepdog.py
+++ b/cinder/volume/drivers/sheepdog.py
@@ -18,9 +18,7 @@
 SheepDog Volume Driver.
 
 """
-import os
 import re
-import tempfile
 
 from oslo.concurrency import processutils
 from oslo.config import cfg
@@ -85,10 +83,6 @@ class SheepdogDriver(driver.VolumeDriver):
         """Delete a logical volume."""
         self._delete(volume)
 
-    def _ensure_dir_exists(self, tmp_dir):
-        if tmp_dir and not os.path.exists(tmp_dir):
-            os.makedirs(tmp_dir)
-
     def _resize(self, volume, size=None):
         if not size:
             size = int(volume['size']) * units.Gi
@@ -101,19 +95,16 @@ class SheepdogDriver(driver.VolumeDriver):
                           volume['name'])
 
     def copy_image_to_volume(self, context, volume, image_service, image_id):
-        # use the image_conversion_dir as a temporary place to save the image
-        conversion_dir = CONF.image_conversion_dir
-        self._ensure_dir_exists(conversion_dir)
-        with tempfile.NamedTemporaryFile(dir=conversion_dir) as tmp:
+        with image_utils.temporary_file() as tmp:
             # (wenhao): we don't need to convert to raw for sheepdog.
             image_utils.fetch_verify_image(context, image_service,
-                                           image_id, tmp.name)
+                                           image_id, tmp)
 
             # remove the image created by import before this function.
             # see volume/drivers/manager.py:_create_volume
             self._delete(volume)
             # convert and store into sheepdog
-            image_utils.convert_image(tmp.name, 'sheepdog:%s' % volume['name'],
+            image_utils.convert_image(tmp, 'sheepdog:%s' % volume['name'],
                                       'raw')
             self._resize(volume)
 

diff --git a/cinder/volume/drivers/windows/windows.py b/cinder/volume/drivers/windows/windows.py
index 522a76289ce208949b5220f101810c9d2abdb049..7aec54b896d33eaacb35024e77a9523479975ac7 100644 (file)
--- a/cinder/volume/drivers/windows/windows.py
+++ b/cinder/volume/drivers/windows/windows.py
@@ -167,9 +167,6 @@ class WindowsDriver(driver.ISCSIDriver):
         """Fetch the image from image_service and create a volume using it."""
         # Convert to VHD and file back to VHD
         vhd_type = self.utils.get_supported_vhd_type()
-        if (CONF.image_conversion_dir and not
-                os.path.exists(CONF.image_conversion_dir)):
-            os.makedirs(CONF.image_conversion_dir)
         with image_utils.temporary_file(suffix='.vhd') as tmp:
             volume_path = self.local_path(volume)
             image_utils.fetch_to_vhd(context, image_service, image_id, tmp,