From: git-harry Date: Tue, 9 Dec 2014 11:26:20 +0000 (+0000) Subject: Improve use of temporary_file and temporary_dir X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=297794bfbf3dc73b5667c0b34ab3a4f0e7bf43dd;p=openstack-build%2Fcinder-build.git 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 9a56b18ff..7aa6a4da6 100644 --- 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 f26afd938..31ba6cdfd 100644 --- 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 4bf1f379c..ccce1c093 100644 --- 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 43c1d3f0f..5ec0a2e2a 100644 --- 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 759518523..1943f6134 100644 --- 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 522a76289..7aec54b89 100644 --- 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,