From dc2350247c0dd52d4d263c0493568b2882ce4d93 Mon Sep 17 00:00:00 2001 From: Glenn Matthews Date: Fri, 24 Mar 2017 09:43:57 -0400 Subject: [PATCH 1/8] Fix UnboundLocalError in VMDK conversion error recovery. Fixes #67. --- CHANGELOG.rst | 10 +++++++ COT/disks/tests/test_vmdk.py | 58 ++++++++++++++++++++++++++++++++---- COT/disks/vmdk.py | 5 +++- 3 files changed, 66 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index d6abff8..0259770 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -3,6 +3,15 @@ Change Log All notable changes to the COT project will be documented in this file. This project adheres to `Semantic Versioning`_. +`Unreleased`_ +------------- + +**Fixed** + +- Fixed issue where UnboundLocalError would be raised during COT's + attempt to clean up after a qemu-img error occurring while trying to + convert a disk to VMDK (`#67`_). + `2.0.2`_ - 2017-03-20 --------------------- @@ -763,6 +772,7 @@ Initial public release. .. _#64: https://github.com/glennmatthews/cot/issues/64 .. _#65: https://github.com/glennmatthews/cot/issues/65 .. _#66: https://github.com/glennmatthews/cot/issues/66 +.. _#67: https://github.com/glennmatthews/cot/issues/67 .. _Semantic Versioning: http://semver.org/ .. _`PEP 8`: https://www.python.org/dev/peps/pep-0008/ diff --git a/COT/disks/tests/test_vmdk.py b/COT/disks/tests/test_vmdk.py index 3e94a05..0be44c0 100644 --- a/COT/disks/tests/test_vmdk.py +++ b/COT/disks/tests/test_vmdk.py @@ -100,8 +100,10 @@ def setUp(self): super(TestVMDKConversion, self).setUp() self.input_image_paths = {} self.input_disks = {} + input_dir = os.path.join(self.temp_dir, "disks") + os.makedirs(input_dir) for disk_format in ["raw", "qcow2", "vmdk"]: - temp_disk = os.path.join(self.temp_dir, + temp_disk = os.path.join(input_dir, "foo.{0}".format(disk_format)) helpers['qemu-img'].call(['create', '-f', disk_format, temp_disk, "16M"]) @@ -117,6 +119,12 @@ def other_format_to_vmdk_test(self, disk_format, self.assertEqual(vmdk.disk_format, 'vmdk') self.assertEqual(vmdk.disk_subformat, output_subformat) + # With older versions of QEMU, COT may convert the input image to a RAW + # file as an intermediate step. Make sure it's cleaned up afterwards! + temp_image = os.path.join(self.temp_dir, 'foo.img') + self.assertFalse(os.path.exists(temp_image), + "Temporary image {0} not deleted".format(temp_image)) + @mock.patch('COT.helpers.qemu_img.QEMUImg.version', new_callable=mock.PropertyMock, return_value=StrictVersion("1.2.0")) @@ -134,13 +142,46 @@ def test_disk_conversion_old_qemu(self, for disk_format in ["raw", "qcow2", "vmdk"]: self.other_format_to_vmdk_test(disk_format) - for call_args in mock_qemu_call.call_args_list: - self.assertNotIn('convert', call_args) + if disk_format != "raw": + # use qemu-img to convert to raw + mock_qemu_call.assert_called_once_with( + ['convert', '-O', 'raw', + self.input_disks[disk_format].path, mock.ANY]) + else: + mock_qemu_call.assert_not_called() + mock_vmdktool_call.assert_called_once() mock_qemu_call.reset_mock() mock_vmdktool_call.reset_mock() + @mock.patch('COT.helpers.qemu_img.QEMUImg.version', + new_callable=mock.PropertyMock, + return_value=StrictVersion("1.2.0")) + def test_disk_conversion_old_qemu_error(self, *_): + """Error recovery/cleanup during multi-step conversion with vmdktool. + + https://github.com/glennmatthews/cot/issues/67 + """ + # Error in conversion from qcow2 to raw + with mock.patch('COT.helpers.qemu_img.QEMUImg.call', + side_effect=HelperError): + self.assertRaises(HelperError, + VMDK.from_other_image, + self.input_disks['qcow2'], self.temp_dir) + + # Error in conversion from raw to vmdk + with mock.patch('COT.helpers.vmdktool.VMDKTool.call', + side_effect=HelperError): + self.assertRaises(HelperError, + VMDK.from_other_image, + self.input_disks['qcow2'], self.temp_dir) + + # Make sure we didn't leave the temporary image behind + temp_image = os.path.join(self.temp_dir, 'foo.img') + self.assertFalse(os.path.exists(temp_image), + "Temporary image {0} not deleted".format(temp_image)) + @mock.patch('COT.helpers.qemu_img.QEMUImg.version', new_callable=mock.PropertyMock, return_value=StrictVersion("2.1.0")) @@ -157,12 +198,17 @@ def test_disk_conversion_med_qemu(self, we still prefer vmdktool, but fall back to qemu-img with a warning if vmdktool is not available. """ - # First, with vmdktool, same as previous test case + # First, with vmdktool, same as test_disk_conversion_old_qemu for disk_format in ["raw", "qcow2", "vmdk"]: self.other_format_to_vmdk_test(disk_format) - for call_args in mock_qemu_call.call_args_list: - self.assertNotIn('convert', call_args) + if disk_format != "raw": + # use qemu-img to convert to raw + mock_qemu_call.assert_called_once_with( + ['convert', '-O', 'raw', + self.input_disks[disk_format].path, mock.ANY]) + else: + mock_qemu_call.assert_not_called() mock_vmdktool_call.assert_called_once() mock_qemu_call.reset_mock() diff --git a/COT/disks/vmdk.py b/COT/disks/vmdk.py index 4c1e814..25511b2 100644 --- a/COT/disks/vmdk.py +++ b/COT/disks/vmdk.py @@ -102,6 +102,7 @@ def from_other_image(cls, input_image, output_dir, if input_image.disk_format != 'raw': # vmdktool needs a raw image as input from COT.disks import RAW + temp_image = None try: temp_image = RAW.from_other_image(input_image, output_dir) @@ -109,7 +110,9 @@ def from_other_image(cls, input_image, output_dir, output_dir, output_subformat) finally: - os.remove(temp_image.path) + if temp_image is not None: + os.remove(temp_image.path) + temp_image = None # Note that vmdktool takes its arguments in unusual order - # output file comes before input file From 6c2f1f7117b40dd314a7ec583196de3230b1fda0 Mon Sep 17 00:00:00 2001 From: Glenn Matthews Date: Mon, 27 Mar 2017 13:54:01 -0400 Subject: [PATCH 2/8] Uprev coverage 4.1 --> 4.3.4 --- .coveragerc | 10 +++++++++- tox.ini | 3 +-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/.coveragerc b/.coveragerc index 5ae19f6..3f00432 100644 --- a/.coveragerc +++ b/.coveragerc @@ -1,14 +1,22 @@ # .coveragerc to control coverage.py [run] branch = True +source = + COT omit = */tests/* COT/_version.py setup.py versioneer.py - .tox/* + demo_logging.py .eggs/* *easy_install* */pypy/* */lib_pypy/* */distutils/* + +[paths] +source = + COT + .tox/*/lib/python*/site-packages/COT + .tox/pypy/site-packages/COT diff --git a/tox.ini b/tox.ini index 9f2d10f..c47316b 100644 --- a/tox.ini +++ b/tox.ini @@ -31,7 +31,7 @@ PyPy = setup, pypy, stats passenv = PREFIX deps = -rrequirements.txt - coverage==4.1 + coverage==4.3.4 mock sphinx>=1.3 sphinx_rtd_theme @@ -47,7 +47,6 @@ commands = [testenv:stats] commands = - coverage combine coverage report -i coverage html -i From 137a353a20858800f38ea6768a1a6c8318b2b192 Mon Sep 17 00:00:00 2001 From: Glenn Matthews Date: Mon, 27 Mar 2017 15:38:35 -0400 Subject: [PATCH 3/8] Add exclusion for demo_logging test command. --- .coveragerc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.coveragerc b/.coveragerc index 3f00432..523c54b 100644 --- a/.coveragerc +++ b/.coveragerc @@ -6,9 +6,9 @@ source = omit = */tests/* COT/_version.py + COT/commands/demo_logging.py setup.py versioneer.py - demo_logging.py .eggs/* *easy_install* */pypy/* From a3bff685b30dfa4c33740a85a9b5090727e2fce7 Mon Sep 17 00:00:00 2001 From: Glenn Matthews Date: Sun, 2 Apr 2017 20:54:50 -0400 Subject: [PATCH 4/8] Handle Travis sudo ENOEXEC case? --- COT/helpers/helper.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/COT/helpers/helper.py b/COT/helpers/helper.py index 0721dc2..8750f2a 100644 --- a/COT/helpers/helper.py +++ b/COT/helpers/helper.py @@ -582,6 +582,14 @@ def check_call(args, require_success=True, retry_with_sudo=False, **kwargs): retry_with_sudo=False, **kwargs) return + # In Travis CI container environment, 'sudo' is disallowed. + # For some reason, recently (4/2017) it's changed from failing with + # EPERM "sudo: must be setuid root" + # to: + # ENOEXEC "Exec format error" + # We shouldn't see ENOEXEC otherwise, so we special case this. + if exc.errno == errno.ENOEXEC and args[0] == 'sudo': # pragma: no cover + raise HelperError(exc.errno, "The 'sudo' command is unavailable") if exc.errno != errno.ENOENT: raise raise HelperNotFoundError(exc.errno, From 3db703c1297464edb1f052c5b6d68dcdd2422d8a Mon Sep 17 00:00:00 2001 From: Glenn Matthews Date: Sun, 2 Apr 2017 20:55:09 -0400 Subject: [PATCH 5/8] Briefer usage example --- COT/commands/install_helpers.py | 29 +---------------------------- 1 file changed, 1 insertion(+), 28 deletions(-) diff --git a/COT/commands/install_helpers.py b/COT/commands/install_helpers.py index 6b87d4c..c29f431 100644 --- a/COT/commands/install_helpers.py +++ b/COT/commands/install_helpers.py @@ -271,34 +271,7 @@ def create_subparser(self): "``--ignore-errors``.", """ > cot install-helpers - INFO: Installing 'fatdisk'... - INFO: Compiling 'fatdisk' - INFO: Calling './RUNME'... (...) - INFO: ...done - INFO: Compilation complete, installing to /usr/local/bin - INFO: Successfully installed 'fatdisk' - INFO: Calling 'fatdisk --version' and capturing its output... - INFO: ...done - INFO: Installing 'vmdktool'... - INFO: vmdktool requires 'zlib'... installing 'zlib' - INFO: Calling 'dpkg -s zlib1g-dev' and capturing its output... - INFO: ...done - INFO: Compiling 'vmdktool' - INFO: Calling 'make CFLAGS="-D_GNU_SOURCE -g -O -pipe"'... -(...) - INFO: ...done - INFO: Compilation complete, installing to /usr/local - INFO: Calling 'make install'... -install -s vmdktool /usr/local/bin/ -install vmdktool.8 /usr/local/man/man8/ - INFO: ...done - INFO: Successfully installed 'vmdktool' - INFO: Calling 'vmdktool -V' and capturing its output... - INFO: ...done - INFO: Copying cot-add-disk.1 to /usr/share/man/man1/cot-add-disk.1 -(...) - INFO: Copying cot.1 to /usr/share/man/man1/cot.1 Results: ------------- COT manpages: successfully installed to /usr/share/man @@ -311,7 +284,7 @@ def create_subparser(self): qemu-img: present at /usr/bin/qemu-img vmdktool: successfully installed to /usr/local/bin/vmdktool -Unable to install some helpers""".strip())]), +[Errno 1] Unable to install some helpers""".strip())]), formatter_class=argparse.RawDescriptionHelpFormatter) group = parser.add_mutually_exclusive_group() From f70b9123f95191354fef96b3154cc3ad760303de Mon Sep 17 00:00:00 2001 From: Glenn Matthews Date: Sun, 2 Apr 2017 23:28:58 -0400 Subject: [PATCH 6/8] Fix mkdir params to work on OS X as well as Linux --- CHANGELOG.rst | 1 + COT/helpers/helper.py | 6 ++++-- COT/helpers/tests/test_fatdisk.py | 6 +++--- COT/helpers/tests/test_helper.py | 4 ++-- COT/helpers/tests/test_vmdktool.py | 12 ++++++------ 5 files changed, 16 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 0259770..2994ad9 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -11,6 +11,7 @@ This project adheres to `Semantic Versioning`_. - Fixed issue where UnboundLocalError would be raised during COT's attempt to clean up after a qemu-img error occurring while trying to convert a disk to VMDK (`#67`_). +- Fixed incorrect invocation of 'sudo mkdir' on Mac OS X. `2.0.2`_ - 2017-03-20 --------------------- diff --git a/COT/helpers/helper.py b/COT/helpers/helper.py index 8750f2a..57a9d78 100644 --- a/COT/helpers/helper.py +++ b/COT/helpers/helper.py @@ -466,7 +466,8 @@ def mkdir(directory, permissions=493): # 493 == 0o755 directory) try: check_call(['sudo', 'mkdir', '-p', - '--mode=%o' % permissions, + # We previously used '--mode' but OS X lacks it. + '-m', '%o' % permissions, directory]) except HelperError: # That failed too - re-raise the original exception @@ -588,7 +589,8 @@ def check_call(args, require_success=True, retry_with_sudo=False, **kwargs): # to: # ENOEXEC "Exec format error" # We shouldn't see ENOEXEC otherwise, so we special case this. - if exc.errno == errno.ENOEXEC and args[0] == 'sudo': # pragma: no cover + if (exc.errno == errno.ENOEXEC and # pragma: no cover + args[0] == 'sudo'): raise HelperError(exc.errno, "The 'sudo' command is unavailable") if exc.errno != errno.ENOENT: raise diff --git a/COT/helpers/tests/test_fatdisk.py b/COT/helpers/tests/test_fatdisk.py index 5914636..fd94407 100644 --- a/COT/helpers/tests/test_fatdisk.py +++ b/COT/helpers/tests/test_fatdisk.py @@ -92,7 +92,7 @@ def test_install_apt_get(self, ['apt-get', '-q', 'install', 'make'], ['apt-get', '-q', 'install', 'gcc'], ['./RUNME'], - ['sudo', 'mkdir', '-p', '--mode=755', '/usr/local/bin'], + ['sudo', 'mkdir', '-p', '-m', '755', '/usr/local/bin'], ]) self.assertTrue(re.search("/fatdisk$", mock_copy.call_args[0][0])) self.assertEqual('/usr/local/bin', mock_copy.call_args[0][1]) @@ -119,7 +119,7 @@ def test_install_apt_get(self, mock_check_call, [ ['./RUNME'], - ['sudo', 'mkdir', '-p', '--mode=755', + ['sudo', 'mkdir', '-p', '-m', '755', '/home/cot/opt/local/bin'], ]) self.assertTrue(re.search("/fatdisk$", mock_copy.call_args[0][0])) @@ -156,7 +156,7 @@ def test_install_yum(self, ['yum', '--quiet', 'install', 'make'], ['yum', '--quiet', 'install', 'gcc'], ['./RUNME'], - ['sudo', 'mkdir', '-p', '--mode=755', '/usr/local/bin'], + ['sudo', 'mkdir', '-p', '-m', '755', '/usr/local/bin'], ]) self.assertTrue(re.search("/fatdisk$", mock_copy.call_args[0][0])) self.assertEqual('/usr/local/bin', mock_copy.call_args[0][1]) diff --git a/COT/helpers/tests/test_helper.py b/COT/helpers/tests/test_helper.py index 4d7f047..ec889a9 100644 --- a/COT/helpers/tests/test_helper.py +++ b/COT/helpers/tests/test_helper.py @@ -500,7 +500,7 @@ def test_need_sudo(self, mock_isdir, mock_exists, mock_exists.assert_called_with('/foo/bar') mock_makedirs.assert_called_with('/foo/bar', 493) # 493 == 0o755 mock_check_call.assert_called_with( - ['sudo', 'mkdir', '-p', '--mode=755', '/foo/bar']) + ['sudo', 'mkdir', '-p', '-m', '755', '/foo/bar']) def test_nondefault_permissions(self, mock_isdir, mock_exists, mock_makedirs, mock_check_call): @@ -518,7 +518,7 @@ def test_nondefault_permissions(self, mock_isdir, mock_exists, self.assertTrue(Helper.mkdir('/foo/bar', 511)) # 511 == 0o777 mock_makedirs.assert_called_with('/foo/bar', 511) mock_check_call.assert_called_with( - ['sudo', 'mkdir', '-p', '--mode=777', '/foo/bar']) + ['sudo', 'mkdir', '-p', '-m', '777', '/foo/bar']) @mock.patch('COT.helpers.helper.check_call') diff --git a/COT/helpers/tests/test_vmdktool.py b/COT/helpers/tests/test_vmdktool.py index 9e04aab..357a1e4 100644 --- a/COT/helpers/tests/test_vmdktool.py +++ b/COT/helpers/tests/test_vmdktool.py @@ -84,8 +84,8 @@ def test_install_helper_apt_get(self, ['apt-get', '-q', 'install', 'make'], ['apt-get', '-q', 'install', 'zlib1g-dev'], ['make', 'CFLAGS="-D_GNU_SOURCE -g -O -pipe"'], - ['sudo', 'mkdir', '-p', '--mode=755', '/usr/local/man/man8'], - ['sudo', 'mkdir', '-p', '--mode=755', '/usr/local/bin'], + ['sudo', 'mkdir', '-p', '-m', '755', '/usr/local/man/man8'], + ['sudo', 'mkdir', '-p', '-m', '755', '/usr/local/bin'], ['make', 'install', 'PREFIX=/usr/local'], ]) self.assertAptUpdated() @@ -111,9 +111,9 @@ def test_install_helper_apt_get(self, mock_check_call, [ ['make', 'CFLAGS="-D_GNU_SOURCE -g -O -pipe"'], - ['sudo', 'mkdir', '-p', '--mode=755', + ['sudo', 'mkdir', '-p', '-m', '755', '/home/cot/opt/local/man/man8'], - ['sudo', 'mkdir', '-p', '--mode=755', + ['sudo', 'mkdir', '-p', '-m', '755', '/home/cot/opt/local/bin'], ['make', 'install', 'PREFIX=/opt/local', 'DESTDIR=/home/cot'], ]) @@ -146,8 +146,8 @@ def test_install_helper_yum(self, ['yum', '--quiet', 'install', 'make'], ['yum', '--quiet', 'install', 'zlib-devel'], ['make', 'CFLAGS="-D_GNU_SOURCE -g -O -pipe"'], - ['sudo', 'mkdir', '-p', '--mode=755', '/usr/local/man/man8'], - ['sudo', 'mkdir', '-p', '--mode=755', '/usr/local/bin'], + ['sudo', 'mkdir', '-p', '-m', '755', '/usr/local/man/man8'], + ['sudo', 'mkdir', '-p', '-m', '755', '/usr/local/bin'], ['make', 'install', 'PREFIX=/usr/local'], ]) From 4ed15a619f5e34863f4480ed39a2477f6c4c1008 Mon Sep 17 00:00:00 2001 From: Glenn Matthews Date: Mon, 3 Apr 2017 12:44:31 -0400 Subject: [PATCH 7/8] Fix placement of coverage pragma --- COT/helpers/helper.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/COT/helpers/helper.py b/COT/helpers/helper.py index 57a9d78..6672baf 100644 --- a/COT/helpers/helper.py +++ b/COT/helpers/helper.py @@ -589,8 +589,8 @@ def check_call(args, require_success=True, retry_with_sudo=False, **kwargs): # to: # ENOEXEC "Exec format error" # We shouldn't see ENOEXEC otherwise, so we special case this. - if (exc.errno == errno.ENOEXEC and # pragma: no cover - args[0] == 'sudo'): + if (exc.errno == errno.ENOEXEC and + args[0] == 'sudo'): # pragma: no cover raise HelperError(exc.errno, "The 'sudo' command is unavailable") if exc.errno != errno.ENOENT: raise From 685b69a5a5ed901c5b06853926b403e4078b48f7 Mon Sep 17 00:00:00 2001 From: Glenn Matthews Date: Mon, 3 Apr 2017 14:27:06 -0400 Subject: [PATCH 8/8] Bump version --- CHANGELOG.rst | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 2994ad9..9676084 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -3,8 +3,8 @@ Change Log All notable changes to the COT project will be documented in this file. This project adheres to `Semantic Versioning`_. -`Unreleased`_ -------------- +`2.0.3`_ - 2017-04-03 +--------------------- **Fixed** @@ -806,6 +806,7 @@ Initial public release. .. _verboselogs: https://verboselogs.readthedocs.io/en/latest/ .. _Unreleased: https://github.com/glennmatthews/cot/compare/master...develop +.. _2.0.3: https://github.com/glennmatthews/cot/compare/v2.0.2...v2.0.3 .. _2.0.2: https://github.com/glennmatthews/cot/compare/v2.0.1...v2.0.2 .. _2.0.1: https://github.com/glennmatthews/cot/compare/v2.0.0...v2.0.1 .. _2.0.0: https://github.com/glennmatthews/cot/compare/v1.9.1...v2.0.0