From e95168c3eec5f770fdb3a335c71e813cf531f4a0 Mon Sep 17 00:00:00 2001
From: Jan Kaluza <jkaluza@redhat.com>
Date: May 22 2019 11:48:08 +0000
Subject: Continue removing expired compose despite the error in `shutil.rmtree`.


The current code simply throws an exception on any error in `shutil.rmtree`
and stops removing expired composes completely. The error can for example
be "Permission denied" when some files in compose directory are owned
by root:root as a result of issue with runroot tasks execution.

In this commit, such errors are logged, but the rmtree execution continues
and removes as much composes/files as it can.

---

diff --git a/server/odcs/server/backend.py b/server/odcs/server/backend.py
index b681288..f9bdb75 100644
--- a/server/odcs/server/backend.py
+++ b/server/odcs/server/backend.py
@@ -134,6 +134,16 @@ class RemoveExpiredComposesThread(BackendThread):
         """
         super(RemoveExpiredComposesThread, self).__init__(10)
 
+    def _on_rmtree_error(self, function, path, excinf):
+        """
+        Helper method passed to `shutil.rmtree` as `onerror` kwarg which logs
+        the rmtree error, but allows the rmtree to continue removing other
+        files.
+        """
+        exception_value = excinf[1]
+        log.warning("The %r function for path %s failed: %r" % (
+            function, path, exception_value))
+
     def _remove_compose_dir(self, toplevel_dir):
         """
         Removes the compose toplevel_dir symlink together with the real
@@ -153,9 +163,9 @@ class RemoveExpiredComposesThread(BackendThread):
             targetpath = os.path.realpath(toplevel_dir)
             os.unlink(toplevel_dir)
             if os.path.exists(targetpath):
-                shutil.rmtree(targetpath)
+                shutil.rmtree(targetpath, onerror=self._on_rmtree_error)
         else:
-            shutil.rmtree(toplevel_dir)
+            shutil.rmtree(toplevel_dir, onerror=self._on_rmtree_error)
 
     def _get_compose_id_from_path(self, path):
         """
diff --git a/server/tests/test_remove_expired_composes_thread.py b/server/tests/test_remove_expired_composes_thread.py
index ee908c4..be45217 100644
--- a/server/tests/test_remove_expired_composes_thread.py
+++ b/server/tests/test_remove_expired_composes_thread.py
@@ -228,3 +228,17 @@ class TestRemoveExpiredComposesThread(ModelsBaseTest):
         self.thread._remove_compose_dir(toplevel_dir)
         unlink.assert_not_called()
         rmtree.assert_called_once()
+
+    @patch("os.unlink")
+    @patch("os.path.realpath")
+    @patch("os.path.exists")
+    @patch("odcs.server.backend.log.warning")
+    def test_remove_compose_rmtree_error(
+            self, log_warning, exists, realpath, unlink):
+        exists.return_value = True
+        toplevel_dir = "/odcs"
+        realpath.return_value = "/odcs-real"
+
+        # This must not raise an exception.
+        self.thread._remove_compose_dir(toplevel_dir)
+        log_warning.assert_called()