From 5cd1c22e2daa5d69c57e4b4aa1a755450593236a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lubom=C3=ADr=20Sedl=C3=A1=C5=99?= Date: Wed, 8 Mar 2017 13:16:34 +0100 Subject: [PATCH] buildinstall: Retry unmounting image MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the image can not be unmounted because the device is busy, we should retry. There will be increasing pauses between the attempts. At most 10 attempts will be done before giving up. Fixes: #559 Signed-off-by: Lubomír Sedlář --- pungi/phases/buildinstall.py | 5 ++-- pungi/util.py | 20 +++++++++++++ tests/test_util.py | 56 ++++++++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 3 deletions(-) diff --git a/pungi/phases/buildinstall.py b/pungi/phases/buildinstall.py index c75020af..b10384de 100644 --- a/pungi/phases/buildinstall.py +++ b/pungi/phases/buildinstall.py @@ -27,7 +27,7 @@ from productmd.images import Image from pungi.arch import get_valid_arches from pungi.util import get_buildroot_rpms, get_volid, get_arch_variant_data -from pungi.util import get_file_size, get_mtime, failable, makedirs +from pungi.util import get_file_size, get_mtime, failable, makedirs, run_unmount_cmd from pungi.wrappers.lorax import LoraxWrapper from pungi.wrappers.kojiwrapper import KojiWrapper from pungi.wrappers import iso @@ -273,8 +273,7 @@ def tweak_buildinstall(compose, src, dst, arch, variant, label, volid, kickstart cmd = ["cp", "-v", "--remove-destination", config_path, config_in_image] run(cmd) - cmd = ["fusermount", "-u", mount_tmp_dir] - run(cmd) + run_unmount_cmd(["fusermount", "-u", mount_tmp_dir]) shutil.rmtree(mount_tmp_dir) # HACK: make buildinstall files world readable diff --git a/pungi/util.py b/pungi/util.py index 01cab53c..f98893d4 100644 --- a/pungi/util.py +++ b/pungi/util.py @@ -26,6 +26,7 @@ import urlparse import contextlib import traceback import tempfile +import time from kobo.shortcuts import run, force_list from productmd.common import get_major_version @@ -592,3 +593,22 @@ def temp_dir(log=None, *args, **kwargs): # Okay, we failed to delete temporary dir. if log: log.warning('Error removing %s: %s', dir, exc.strerror) + + +def run_unmount_cmd(cmd, max_retries=10): + """Attempt to run the command to unmount an image. + + If the command fails and stderr complains about device being busy, try + again. We will do up to ``max_retries`` attemps with increasing pauses. + """ + for i in xrange(max_retries): + proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + out, err = proc.communicate() + if proc.returncode == 0: + # We were successful + return + if 'Device or resource busy' not in err: + raise RuntimeError('Unhandled error when running %r: %r' % (cmd, err)) + time.sleep(i) + # Still busy, there's something wrong. + raise RuntimeError('Failed to run %r: Device or resource busy.' % cmd) diff --git a/tests/test_util.py b/tests/test_util.py index 3052aab5..108b57cc 100644 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -10,6 +10,7 @@ except ImportError: import unittest import tempfile import shutil +import subprocess sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) @@ -428,5 +429,60 @@ class TestTempFiles(unittest.TestCase): self.assertFalse(os.path.exists(tmp)) +class TestUnmountCmd(unittest.TestCase): + + def _fakeProc(self, ret, err): + proc = mock.Mock(returncode=ret) + proc.communicate.return_value = ('', err) + return proc + + @mock.patch('subprocess.Popen') + def test_unmount_cmd_success(self, mockPopen): + cmd = 'unmount' + mockPopen.side_effect = [self._fakeProc(0, '')] + util.run_unmount_cmd(cmd) + self.assertEqual(mockPopen.call_args_list, + [mock.call(cmd, stderr=subprocess.PIPE, stdout=subprocess.PIPE)]) + + @mock.patch('subprocess.Popen') + def test_unmount_cmd_fail_other_reason(self, mockPopen): + cmd = 'unmount' + mockPopen.side_effect = [self._fakeProc(1, 'It is broken')] + with self.assertRaises(RuntimeError) as ctx: + util.run_unmount_cmd(cmd) + self.assertEqual(str(ctx.exception), + "Unhandled error when running 'unmount': 'It is broken'") + self.assertEqual(mockPopen.call_args_list, + [mock.call(cmd, stderr=subprocess.PIPE, stdout=subprocess.PIPE)]) + + @mock.patch('time.sleep') + @mock.patch('subprocess.Popen') + def test_unmount_cmd_fail_then_retry(self, mockPopen, mock_sleep): + cmd = 'unmount' + mockPopen.side_effect = [self._fakeProc(1, 'Device or resource busy'), + self._fakeProc(1, 'Device or resource busy'), + self._fakeProc(0, '')] + util.run_unmount_cmd(cmd) + self.assertEqual(mockPopen.call_args_list, + [mock.call(cmd, stderr=subprocess.PIPE, stdout=subprocess.PIPE)] * 3) + self.assertEqual(mock_sleep.call_args_list, + [mock.call(0), mock.call(1)]) + + @mock.patch('time.sleep') + @mock.patch('subprocess.Popen') + def test_unmount_cmd_fail_then_retry_and_fail(self, mockPopen, mock_sleep): + cmd = 'unmount' + mockPopen.side_effect = [self._fakeProc(1, 'Device or resource busy'), + self._fakeProc(1, 'Device or resource busy'), + self._fakeProc(1, 'Device or resource busy')] + with self.assertRaises(RuntimeError) as ctx: + util.run_unmount_cmd(cmd, max_retries=3) + self.assertEqual(mockPopen.call_args_list, + [mock.call(cmd, stderr=subprocess.PIPE, stdout=subprocess.PIPE)] * 3) + self.assertEqual(mock_sleep.call_args_list, + [mock.call(0), mock.call(1), mock.call(2)]) + self.assertEqual(str(ctx.exception), "Failed to run 'unmount': Device or resource busy.") + + if __name__ == "__main__": unittest.main()