#276 Continue removing expired compose despite the error in `shutil.rmtree`.
Merged 4 months ago by jkaluza. Opened 4 months ago by jkaluza.
jkaluza/odcs rmtree-exception  into  master

file modified
+12 -2

@@ -134,6 +134,16 @@ 

          """

          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 @@ 

              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)

I haven't used onerror, but the docs don't indicate that the exception is silenced by it.

          else:

-             shutil.rmtree(toplevel_dir)

+             shutil.rmtree(toplevel_dir, onerror=self._on_rmtree_error)

  

      def _get_compose_id_from_path(self, path):

          """

@@ -228,3 +228,17 @@ 

          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()

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.

I haven't used onerror, but the docs don't indicate that the exception is silenced by it.

The doc says:

If ignore_errors is true, errors resulting from failed removals will be ignored; if false or omitted, such errors are handled by calling a handler specified by onerror or, if that is omitted, they raise an exception.

So the exception are raised only if onerror kwarg is omitted, otherwise they are not raised.

:thumbsup:

Thanks for clarifying!

Pull-Request has been merged by jkaluza

4 months ago