From 46ea0743a98e7dd6379256fce78820d605998511 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lubom=C3=ADr=20Sedl=C3=A1=C5=99?= Date: Wed, 29 Jan 2020 15:15:47 +0100 Subject: [PATCH] iso: Clean up cache for guestmount MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When partial cleanup messes up the guestfs cache, the call to guestmount will fail. To fix that, let's check if there is a problem first and clean up everything if needed. Relates: https://bugzilla.redhat.com/show_bug.cgi?id=1771976 JIRA: COMPOSE-3932 Signed-off-by: Lubomír Sedlář --- pungi/wrappers/iso.py | 22 ++++++++++++++ tests/test_iso_wrapper.py | 62 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 82 insertions(+), 2 deletions(-) diff --git a/pungi/wrappers/iso.py b/pungi/wrappers/iso.py index 7f527a09..dd18ef2c 100644 --- a/pungi/wrappers/iso.py +++ b/pungi/wrappers/iso.py @@ -417,6 +417,28 @@ def mount(image, logger=None, use_guestmount=True): # LIBGUESTFS_BACKEND=direct: running qemu directly without libvirt env = {'LIBGUESTFS_BACKEND': 'direct', 'LIBGUESTFS_DEBUG': '1', 'LIBGUESTFS_TRACE': '1'} cmd = ["guestmount", "-a", image, "-m", "/dev/sda", mount_dir] + # guestmount caches files for faster mounting. However, + # systemd-tmpfiles is cleaning it up if the files have not been + # used in 30 days. It seems to be leaving an empty appliance.d + # directory in place, which is causing guestmount to fail because + # the files are missing and it only checks the directory. + # + # Thus we check if the directory is empty, and remove it in such + # case. There is still a possible race condition that the cleanup + # will happen between our check and libguestfs using the directory. + # The performance penalty for never reusing the cached files is too + # high given how unlikely the race condition is. + # + # https://bugzilla.redhat.com/show_bug.cgi?id=1771976 + guestfs_tmp_dir = "/var/tmp/.guestfs-%d" % os.getuid() + try: + if not os.listdir(os.path.join(guestfs_tmp_dir, "appliance.d")): + if logger: + logger.info("Cleaning up %s", guestfs_tmp_dir) + util.rmtree(guestfs_tmp_dir) + except OSError: + # The directory is missing. That's fine for us too. + pass else: env = {} cmd = ["mount", "-o", "loop", image, mount_dir] diff --git a/tests/test_iso_wrapper.py b/tests/test_iso_wrapper.py index fd995f2f..e30b3164 100644 --- a/tests/test_iso_wrapper.py +++ b/tests/test_iso_wrapper.py @@ -3,7 +3,7 @@ import itertools import mock import os -import sys +import six try: import unittest2 as unittest except ImportError: @@ -19,6 +19,24 @@ Supported ISO: no INCORRECT_OUTPUT = '''This should never happen: File not found''' +# Cached to use in tests that mock os.listdir +orig_listdir = os.listdir + + +def fake_listdir(pattern, result=None, exc=None): + """Create a function that mocks os.listdir. If the path contains pattern, + result will be returned or exc raised. Otherwise it's normal os.listdir + """ + # The point of this is to avoid issues on Python 2, where apparently + # isdir() is using listdir(), so the mocking is breaking it. + def worker(path): + if isinstance(path, six.string_types) and pattern in path: + if exc: + raise exc + return result + return orig_listdir(path) + return worker + class TestIsoUtils(unittest.TestCase): @@ -60,9 +78,49 @@ class TestIsoUtils(unittest.TestCase): self.assertTrue(unmount_call_str.startswith("call(['umount'")) self.assertFalse(os.path.isdir(temp_dir)) + @mock.patch("pungi.util.rmtree") + @mock.patch("os.listdir", new=fake_listdir("guestfs", ["root"])) @mock.patch('pungi.util.run_unmount_cmd') @mock.patch('pungi.wrappers.iso.run') - def test_guestmount(self, mock_run, mock_unmount): + def test_guestmount(self, mock_run, mock_unmount, mock_rmtree): + # first tuple is return value for command 'which guestmount' + # value determines type of the mount/unmount command ('0' - guestmount is available) + # for approach as a non-root, pair commands guestmount-fusermount are used + mock_run.side_effect = [(0, ''), (0, '')] + with iso.mount('dummy') as temp_dir: + self.assertTrue(os.path.isdir(temp_dir)) + self.assertEqual(len(mock_run.call_args_list), 2) + mount_call_str = str(mock_run.call_args_list[1]) + self.assertTrue(mount_call_str.startswith("call(['guestmount'")) + self.assertEqual(len(mock_unmount.call_args_list), 1) + unmount_call_str = str(mock_unmount.call_args_list[0]) + self.assertTrue(unmount_call_str.startswith("call(['fusermount'")) + self.assertFalse(os.path.isdir(temp_dir)) + + @mock.patch("pungi.util.rmtree") + @mock.patch("os.listdir", new=fake_listdir("guestfs", [])) + @mock.patch('pungi.util.run_unmount_cmd') + @mock.patch('pungi.wrappers.iso.run') + def test_guestmount_cleans_up_cache(self, mock_run, mock_unmount, mock_rmtree): + # first tuple is return value for command 'which guestmount' + # value determines type of the mount/unmount command ('0' - guestmount is available) + # for approach as a non-root, pair commands guestmount-fusermount are used + mock_run.side_effect = [(0, ''), (0, '')] + with iso.mount('dummy') as temp_dir: + self.assertTrue(os.path.isdir(temp_dir)) + self.assertEqual(len(mock_run.call_args_list), 2) + mount_call_str = str(mock_run.call_args_list[1]) + self.assertTrue(mount_call_str.startswith("call(['guestmount'")) + self.assertEqual(len(mock_unmount.call_args_list), 1) + unmount_call_str = str(mock_unmount.call_args_list[0]) + self.assertTrue(unmount_call_str.startswith("call(['fusermount'")) + self.assertFalse(os.path.isdir(temp_dir)) + + @mock.patch("pungi.util.rmtree") + @mock.patch("os.listdir", new=fake_listdir("guestfs", OSError("No such file"))) + @mock.patch('pungi.util.run_unmount_cmd') + @mock.patch('pungi.wrappers.iso.run') + def test_guestmount_handles_missing_cache(self, mock_run, mock_unmount, mock_rmtree): # first tuple is return value for command 'which guestmount' # value determines type of the mount/unmount command ('0' - guestmount is available) # for approach as a non-root, pair commands guestmount-fusermount are used