From 423eac9e7fca9ea3df5c3c75ab2379c0d1ca66e6 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Oct 30 2017 16:09:08 +0000 Subject: PR#658: consolidate safe_rmtree, rmtree and shutil.rmtree Merges #658 https://pagure.io/koji/pull-request/658 Fixes: #648 https://pagure.io/koji/issue/648 Consolidate rmtree usage --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 69927c7..cbad388 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -4970,7 +4970,7 @@ def recycle_build(old, data): update.execute() builddir = koji.pathinfo.build(data) if os.path.exists(builddir): - shutil.rmtree(builddir) + koji.util.rmtree(builddir) koji.plugin.run_callbacks('postBuildStateChange', attribute='state', old=old['state'], new=data['state'], info=data) @@ -7119,19 +7119,10 @@ def _delete_build(binfo): update.execute() update = """UPDATE build SET state=%(st_deleted)i WHERE id=%(build_id)i""" _dml(update, locals()) - #now clear the build dirs - dirs_to_clear = [] + # now clear the build dir builddir = koji.pathinfo.build(binfo) if os.path.exists(builddir): - dirs_to_clear.append(builddir) - for filedir in dirs_to_clear: - rv = os.system(r"find '%s' -xdev \! -type d -print0 |xargs -0 rm -f" % filedir) - if rv != 0: - raise koji.GenericError('file removal failed (code %r) for %s' % (rv, filedir)) - #and clear out the emptied dirs - rv = os.system(r"find '%s' -xdev -depth -type d -print0 |xargs -0 rmdir" % filedir) - if rv != 0: - raise koji.GenericError('directory removal failed (code %r) for %s' % (rv, filedir)) + koji.util.rmtree(builddir) koji.plugin.run_callbacks('postBuildStateChange', attribute='state', old=binfo['state'], new=st_deleted, info=binfo) def reset_build(build): @@ -7195,19 +7186,10 @@ def reset_build(build): binfo['state'] = koji.BUILD_STATES['CANCELED'] update = """UPDATE build SET state=%(state)s, task_id=NULL, volume_id=0 WHERE id=%(id)s""" _dml(update, binfo) - #now clear the build dirs - dirs_to_clear = [] + # now clear the build dir builddir = koji.pathinfo.build(binfo) if os.path.exists(builddir): - dirs_to_clear.append(builddir) - for filedir in dirs_to_clear: - rv = os.system(r"find '%s' -xdev \! -type d -print0 |xargs -0 rm -f" % filedir) - if rv != 0: - raise koji.GenericError('file removal failed (code %r) for %s' % (rv, filedir)) - #and clear out the emptied dirs - rv = os.system(r"find '%s' -xdev -depth -type d -print0 |xargs -0 rmdir" % filedir) - if rv != 0: - raise koji.GenericError('directory removal failed (code %r) for %s' % (rv, filedir)) + koji.util.rmtree(builddir) koji.plugin.run_callbacks('postBuildStateChange', attribute='state', old=binfo['state'], new=koji.BUILD_STATES['CANCELED'], info=binfo) def cancel_build(build_id, cancel_task=True): diff --git a/koji/tasks.py b/koji/tasks.py index 917ff87..b8ad060 100644 --- a/koji/tasks.py +++ b/koji/tasks.py @@ -67,8 +67,6 @@ def umount_all(topdir): def safe_rmtree(path, unmount=False, strict=True): logger = logging.getLogger("koji.build") - #safe remove: with -xdev the find cmd will not cross filesystems - # (though it will cross bind mounts from the same filesystem) if unmount: umount_all(path) if os.path.isfile(path) or os.path.islink(path): @@ -80,30 +78,22 @@ def safe_rmtree(path, unmount=False, strict=True): raise else: logger.warn("Error removing: %s", exc_info=True) - return + return 1 + return 0 if not os.path.exists(path): logger.debug("No such path: %s" % path) - return - #first rm -f non-directories + return 0 + logger.debug('Scrubbing files in %s' % path) - rv = os.system("find '%s' -xdev \\! -type d -print0 |xargs -0 rm -f" % path) - msg = 'file removal failed (code %r) for %s' % (rv, path) - if rv != 0: - logger.warn(msg) - if strict: - raise koji.GenericError(msg) - else: - return rv - #them rmdir directories - #with -depth, we start at the bottom and work up - logger.debug('Scrubbing directories in %s' % path) - rv = os.system("find '%s' -xdev -depth -type d -print0 |xargs -0 rmdir" % path) - msg = 'dir removal failed (code %r) for %s' % (rv, path) - if rv != 0: - logger.warn(msg) + try: + koji.util.rmtree(path) + except Exception: + logger.warn('file removal failed for %s' % path) if strict: - raise koji.GenericError(msg) - return rv + raise + return 1 + return 0 + class ServerExit(Exception): """Raised to shutdown the server""" diff --git a/plugins/hub/rpm2maven.py b/plugins/hub/rpm2maven.py index b1310ea..c216fdc 100644 --- a/plugins/hub/rpm2maven.py +++ b/plugins/hub/rpm2maven.py @@ -8,6 +8,7 @@ import koji from koji.context import context from koji.plugin import callback +from koji.util import rmtree import ConfigParser import fnmatch import os @@ -42,13 +43,13 @@ def maven_import(cbtype, *args, **kws): tmpdir = os.path.join(koji.pathinfo.work(), 'rpm2maven', koji.buildLabel(buildinfo)) try: if os.path.exists(tmpdir): - shutil.rmtree(tmpdir) + rmtree(tmpdir) koji.ensuredir(tmpdir) expand_rpm(filepath, tmpdir) scan_and_import(buildinfo, rpminfo, tmpdir) finally: if os.path.exists(tmpdir): - shutil.rmtree(tmpdir) + rmtree(tmpdir) def expand_rpm(filepath, tmpdir): devnull = file('/dev/null', 'r+') diff --git a/tests/test_hub/test_recycle_build.py b/tests/test_hub/test_recycle_build.py index 2179b04..119814f 100644 --- a/tests/test_hub/test_recycle_build.py +++ b/tests/test_hub/test_recycle_build.py @@ -16,7 +16,7 @@ class TestRecycleBuild(): side_effect=self.getUpdate).start() self._dml = mock.patch('kojihub._dml').start() self.run_callbacks = mock.patch('koji.plugin.run_callbacks').start() - self.rmtree = mock.patch('shutil.rmtree').start() + self.rmtree = mock.patch('koji.util.rmtree').start() self.exists = mock.patch('os.path.exists').start() self.updates = [] diff --git a/tests/test_lib/test_utils.py b/tests/test_lib/test_utils.py index 1ff221c..f18269f 100644 --- a/tests/test_lib/test_utils.py +++ b/tests/test_lib/test_utils.py @@ -1,7 +1,7 @@ from __future__ import absolute_import import mock import unittest -from mock import call +from mock import call, patch import os import optparse @@ -488,6 +488,195 @@ class MavenUtilTestCase(unittest.TestCase): config.readfp(conf_file) return config +class TestRmtree(unittest.TestCase): + @patch('koji.util._rmtree') + @patch('os.rmdir') + @patch('os.chdir') + @patch('os.getcwd') + @patch('stat.S_ISDIR') + @patch('os.lstat') + def test_rmtree_file(self, lstat, isdir, getcwd, chdir, rmdir, _rmtree): + """ Tests that the koji.util.rmtree function raises error when the + path parameter is not a directory. + """ + stat = mock.MagicMock() + stat.st_dev = 'dev' + lstat.return_value = stat + isdir.return_value = False + getcwd.return_value = 'cwd' + + with self.assertRaises(koji.GenericError): + koji.util.rmtree('/mnt/folder/some_file') + _rmtree.assert_not_called() + rmdir.assert_not_called() + + @patch('koji.util._rmtree') + @patch('os.rmdir') + @patch('os.chdir') + @patch('os.getcwd') + @patch('stat.S_ISDIR') + @patch('os.lstat') + def test_rmtree_directory(self, lstat, isdir, getcwd, chdir, rmdir, _rmtree): + """ Tests that the koji.util.rmtree function returns nothing when the path is a directory. + """ + stat = mock.MagicMock() + stat.st_dev = 'dev' + lstat.return_value = stat + isdir.return_value = True + getcwd.return_value = 'cwd' + path = '/mnt/folder' + + self.assertEquals(koji.util.rmtree(path), None) + chdir.assert_called_with('cwd') + _rmtree.assert_called_once_with('dev') + rmdir.assert_called_once_with(path) + + @patch('koji.util._rmtree') + @patch('os.rmdir') + @patch('os.chdir') + @patch('os.getcwd') + @patch('stat.S_ISDIR') + @patch('os.lstat') + def test_rmtree_directory_scrub_failure(self, lstat, isdir, getcwd, chdir, rmdir, _rmtree): + """ Tests that the koji.util.rmtree function returns a GeneralException + when the scrub of the files in the directory fails. + """ + stat = mock.MagicMock() + stat.st_dev = 'dev' + lstat.return_value = stat + isdir.return_value = True + getcwd.return_value = 'cwd' + path = '/mnt/folder' + _rmtree.side_effect = OSError('xyz') + + with self.assertRaises(OSError): + koji.util.rmtree(path) + + @patch('os.chdir') + @patch('os.rmdir') + @patch('koji.util._stripcwd') + def test_rmtree_internal_empty(self, stripcwd, rmdir, chdir): + dev = 'dev' + stripcwd.return_value = [] + + koji.util._rmtree(dev) + + stripcwd.assert_called_once_with(dev) + rmdir.assert_not_called() + chdir.assert_not_called() + + @patch('os.chdir') + @patch('os.rmdir') + @patch('koji.util._stripcwd') + def test_rmtree_internal_dirs(self, stripcwd, rmdir, chdir): + dev = 'dev' + stripcwd.side_effect = (['a', 'b'], [], []) + + koji.util._rmtree(dev) + + stripcwd.assert_has_calls([call(dev), call(dev), call(dev)]) + rmdir.assert_has_calls([call('b'), call('a')]) + chdir.assert_has_calls([call('b'), call('..'), call('a'), call('..')]) + + @patch('os.chdir') + @patch('os.rmdir') + @patch('koji.util._stripcwd') + def test_rmtree_internal_fail(self, stripcwd, rmdir, chdir): + dev = 'dev' + stripcwd.side_effect = (['a', 'b'], [], []) + rmdir.side_effect = OSError() + + # don't fail on anything + koji.util._rmtree(dev) + + stripcwd.assert_has_calls([call(dev), call(dev), call(dev)]) + rmdir.assert_has_calls([call('b'), call('a')]) + chdir.assert_has_calls([call('b'), call('..'), call('a'), call('..')]) + + + @patch('os.listdir') + @patch('os.lstat') + @patch('stat.S_ISDIR') + @patch('os.unlink') + def test_stripcwd_empty(dev, unlink, isdir, lstat, listdir): + # simple empty directory + dev = 'dev' + listdir.return_value = [] + + koji.util._stripcwd(dev) + + listdir.assert_called_once_with('.') + unlink.assert_not_called() + isdir.assert_not_called() + lstat.assert_not_called() + + @patch('os.listdir') + @patch('os.lstat') + @patch('stat.S_ISDIR') + @patch('os.unlink') + def test_stripcwd_all(dev, unlink, isdir, lstat, listdir): + # test valid file + dir + dev = 'dev' + listdir.return_value = ['a', 'b'] + st = mock.MagicMock() + st.st_dev = dev + st.st_mode = 'mode' + lstat.return_value = st + isdir.side_effect = [True, False] + + koji.util._stripcwd(dev) + + listdir.assert_called_once_with('.') + unlink.assert_called_once_with('b') + isdir.assert_has_calls([call('mode'), call('mode')]) + lstat.assert_has_calls([call('a'), call('b')]) + + @patch('os.listdir') + @patch('os.lstat') + @patch('stat.S_ISDIR') + @patch('os.unlink') + def test_stripcwd_diffdev(dev, unlink, isdir, lstat, listdir): + # ignore files on different devices + dev = 'dev' + listdir.return_value = ['a', 'b'] + st1 = mock.MagicMock() + st1.st_dev = dev + st1.st_mode = 'mode' + st2 = mock.MagicMock() + st2.st_dev = 'other_dev' + st2.st_mode = 'mode' + lstat.side_effect = [st1, st2] + isdir.side_effect = [True, False] + + koji.util._stripcwd(dev) + + listdir.assert_called_once_with('.') + unlink.assert_not_called() + isdir.assert_called_once_with('mode') + lstat.assert_has_calls([call('a'), call('b')]) + + @patch('os.listdir') + @patch('os.lstat') + @patch('stat.S_ISDIR') + @patch('os.unlink') + def test_stripcwd_fails(dev, unlink, isdir, lstat, listdir): + # ignore all unlink errors + dev = 'dev' + listdir.return_value = ['a', 'b'] + st = mock.MagicMock() + st.st_dev = dev + st.st_mode = 'mode' + lstat.return_value = st + isdir.side_effect = [True, False] + unlink.side_effect = OSError() + + koji.util._stripcwd(dev) + + listdir.assert_called_once_with('.') + unlink.assert_called_once_with('b') + isdir.assert_has_calls([call('mode'), call('mode')]) + lstat.assert_has_calls([call('a'), call('b')]) + if __name__ == '__main__': unittest.main() diff --git a/tests/test_lib_py2only/test_tasks.py b/tests/test_lib_py2only/test_tasks.py index 6cbc298..7b2d0f2 100644 --- a/tests/test_lib_py2only/test_tasks.py +++ b/tests/test_lib_py2only/test_tasks.py @@ -1,7 +1,7 @@ from __future__ import absolute_import import random +import shutil from os import path, makedirs -from shutil import rmtree from tempfile import gettempdir from unittest import TestCase from mock import patch, MagicMock, Mock, call @@ -74,7 +74,7 @@ class TasksTestCase(TestCase): temp_dir_root = get_temp_dir_root() if path.isdir(temp_dir_root): - rmtree(get_temp_dir_root()) + shutil.rmtree(get_temp_dir_root()) def test_scan_mounts_results(self): """ Tests the scan_mounts function with a mocked /proc/mounts file. A list containing mount points @@ -128,66 +128,6 @@ class TasksTestCase(TestCase): except koji.GenericError as e: self.assertEquals(e.args[0], 'Unmounting incomplete: [\'/dev/shm\', \'/dev/mqueue\']') - @patch('os.path.isfile', return_value=True) - @patch('os.remove') - def test_safe_rmtree_file(self, mock_remove, mock_isfile): - """ Tests that the safe_rmtree function returns nothing when the path parameter is a file. - """ - self.assertEquals(safe_rmtree('/mnt/folder/some_file', False, True), None) - - @patch('os.path.isfile', return_value=False) - @patch('os.path.islink', return_value=True) - @patch('os.remove') - def test_safe_rmtree_link(self, mock_remove, mock_islink, mock_isfile): - """ Tests that the safe_rmtree function returns nothing when the path parameter is a link. - """ - self.assertEquals(safe_rmtree('/mnt/folder/some_link', False, True), None) - - @patch('os.path.isfile', return_value=False) - @patch('os.path.islink', return_value=False) - @patch('os.path.exists', return_value=False) - def test_safe_rmtree_does_not_exist(self, mock_exists, mock_islink, mock_isfile): - """ Tests that the safe_rmtree function returns nothing if the path does not exist. - """ - self.assertEquals(safe_rmtree('/mnt/folder/some_file', False, True), None) - - @patch('os.path.isfile', return_value=False) - @patch('os.path.islink', return_value=False) - @patch('os.path.exists', return_value=True) - @patch('os.system', side_effect=[0, 0]) - def test_safe_rmtree_directory(self, mock_os_system, mock_exists, mock_islink, mock_isfile): - """ Tests that the safe_rmtree function returns nothing when the path is a directory. - """ - self.assertEquals(safe_rmtree('/mnt/folder', False, True), 0) - - @patch('os.path.isfile', return_value=False) - @patch('os.path.islink', return_value=False) - @patch('os.path.exists', return_value=True) - @patch('os.system', side_effect=[1, 0]) - def test_safe_rmtree_directory_scrub_file_failure(self, mock_os_system, mock_exists, mock_islink, mock_isfile): - """ Tests that the safe_rmtree function returns a GeneralException when the path parameter is a directory - and the scrub of the files in the directory fails. - """ - try: - safe_rmtree('/mnt/folder', False, True) - raise Exception('A GenericError was not raised during the test') - except koji.GenericError as e: - self.assertEquals(e.args[0], 'file removal failed (code 1) for /mnt/folder') - - @patch('os.path.isfile', return_value=False) - @patch('os.path.islink', return_value=False) - @patch('os.path.exists', return_value=True) - @patch('os.system', side_effect=[0, 1]) - def test_safe_rmtree_directory_scrub_directory_failure(self, mock_os_system, mock_exists, mock_islink, mock_isfile): - """ Tests that the safe_rmtree function returns a GeneralException when the path parameter is a directory - and the scrub of the directories in the directory fails. - """ - try: - safe_rmtree('/mnt/folder', False, True) - raise Exception('A GenericError was not raised during the test') - except koji.GenericError as e: - self.assertEquals(e.args[0], 'dir removal failed (code 1) for /mnt/folder') - def test_BaseTaskHandler_handler_not_set(self): """ Tests that an exception is thrown when the handler function is not overwritten by the child class. """ @@ -230,7 +170,7 @@ class TasksTestCase(TestCase): obj = TestTask(123, 'some_method', ['random_arg'], None, None, temp_path) obj.createWorkdir() self.assertEquals(path.isdir(temp_path), True) - rmtree(get_temp_dir_root()) + shutil.rmtree(get_temp_dir_root()) def test_BaseTaskHandler_removeWorkdir(self): """ Tests that the removeWOrkdir function deletes a folder based on the path given to the @@ -759,3 +699,114 @@ class TasksTestCase(TestCase): # getTaskResult should be called in 2nd round only for task 3, as 4 # will be skipped as 'canfail' obj.session.getTaskResult.assert_has_calls([call(3)]) + +class TestSafeRmtree(TestCase): + @patch('os.path.exists', return_value=True) + @patch('os.path.isfile', return_value=True) + @patch('os.path.islink', return_value=False) + @patch('os.remove') + @patch('koji.util.rmtree') + def test_safe_rmtree_file(self, rmtree, remove, islink, isfile, exists): + """ Tests that the koji.util.rmtree function returns nothing when the path parameter is a file. + """ + path = '/mnt/folder/some_file' + self.assertEquals(safe_rmtree(path, False, True), 0) + isfile.assert_called_once_with(path) + islink.assert_not_called() + exists.assert_not_called() + remove.assert_called_once_with(path) + rmtree.assert_not_called() + + @patch('os.path.exists', return_value=True) + @patch('os.path.isfile', return_value=False) + @patch('os.path.islink', return_value=True) + @patch('os.remove') + @patch('koji.util.rmtree') + def test_rmtree_link(self, rmtree, remove, islink, isfile, exists): + """ Tests that the koji.util.rmtree function returns nothing when the path parameter is a link. + """ + path = '/mnt/folder/some_link' + self.assertEquals(safe_rmtree(path, False, True), 0) + isfile.assert_called_once_with(path) + islink.assert_called_once_with(path) + exists.assert_not_called() + remove.assert_called_once_with(path) + rmtree.assert_not_called() + + + @patch('os.path.exists', return_value=False) + @patch('os.path.isfile', return_value=False) + @patch('os.path.islink', return_value=False) + @patch('os.remove') + @patch('koji.util.rmtree') + def test_rmtree_does_not_exist(self, rmtree, remove, islink, isfile, exists): + """ Tests that the koji.util.rmtree function returns nothing if the path does not exist. + """ + path = '/mnt/folder/some_file' + self.assertEquals(safe_rmtree(path, False, True), 0) + isfile.assert_called_once_with(path) + islink.assert_called_once_with(path) + exists.assert_called_once_with(path) + remove.assert_not_called() + rmtree.assert_not_called() + + @patch('os.path.exists', return_value=True) + @patch('os.path.isfile', return_value=False) + @patch('os.path.islink', return_value=False) + @patch('os.remove') + @patch('koji.util.rmtree') + def test_rmtree_directory(self, rmtree, remove, islink, isfile, exists): + """ Tests that the koji.util.rmtree function returns nothing when the path is a directory. + """ + path = '/mnt/folder' + self.assertEquals(safe_rmtree(path, False, True), 0) + isfile.assert_called_once_with(path) + islink.assert_called_once_with(path) + exists.assert_called_once_with(path) + remove.assert_not_called() + rmtree.assert_called_once_with(path) + + @patch('os.path.exists', return_value=True) + @patch('os.path.isfile', return_value=False) + @patch('os.path.islink', return_value=False) + @patch('os.remove') + @patch('koji.util.rmtree') + def test_rmtree_directory_scrub_file_failure(self, rmtree, remove, islink, isfile, exists): + """ Tests that the koji.util.rmtree function returns a GeneralException when the path parameter is a directory + and the scrub of the files in the directory fails. + """ + rmtree.side_effect = koji.GenericError('xyz') + path = '/mnt/folder' + try: + safe_rmtree(path, False, 1) + raise Exception('A GenericError was not raised during the test') + except koji.GenericError as e: + self.assertEquals(e.args[0], 'xyz') + isfile.assert_called_once_with(path) + islink.assert_called_once_with(path) + exists.assert_called_once_with(path) + remove.assert_not_called() + rmtree.assert_called_once_with(path) + + @patch('os.path.exists', return_value=True) + @patch('os.path.isfile', return_value=False) + @patch('os.path.islink', return_value=False) + @patch('os.remove') + @patch('koji.util.rmtree') + def test_safe_rmtree_directory_scrub_directory_failure(self, rmtree, remove, islink, isfile, exists): + """ Tests that the koji.util.rmtree function returns a GeneralException when the path parameter is a directory + and the scrub of the directories in the directory fails. + """ + rmtree.side_effect = OSError('xyz') + path = '/mnt/folder' + try: + safe_rmtree(path, False, True) + raise Exception('An OSError was not raised during the test') + except OSError as e: + self.assertEquals(e.args[0], 'xyz') + + isfile.assert_called_once_with(path) + islink.assert_called_once_with(path) + exists.assert_called_once_with(path) + remove.assert_not_called() + rmtree.assert_called_once_with(path)