Merge lp:~bjornt/landscape-client/systemerror-no-retry into lp:~landscape/landscape-client/trunk

[Björn Tillenius]

Don't retry package operations if a SystemError is raised, since a
SystemError is most likely a permanent error and retrying won't help.

The original patch to retry the operation was to handle lock errors,
which are still properly retried with this branch.

Don't retry package operations if a SystemError is raised, since a
SystemError is most likely a permanent error and retrying won't help.

The original patch to retry the operation was to handle lock errors,
which are properly retried with this branch.

I tested that retrying lock errors still works in both trusty and lucid.

[Alberto Donato (ack)]

nice fix, +1

One nitpick inline

review: Approve

[Chad Smith (chad.smith)]

+1 on this change. I think at that time of lp:1177419 we had an imperfect understanding about where apt package errors could come from and just added a blanket retry to catch those cases.

review: Approve

=== modified file 'landscape/package/facade.py'
--- landscape/package/facade.py	2014-09-26 08:21:06 +0000
+++ landscape/package/facade.py	2015-04-15 15:06:54 +0000
@@ -675,12 +675,16 @@
                         install_progress=install_progress)
                     if not install_progress.dpkg_exited:
                         raise SystemError("dpkg didn't exit cleanly.")
-                except (apt.cache.LockFailedException, SystemError), error:
+                except (apt.cache.LockFailedException, SystemError), exception:
                     result_text = (fetch_output.getvalue()
                                    + read_file(install_output_path))
-                    error = TransactionError(error.args[0] +
+                    error = TransactionError(exception.args[0] +
                                              "\n\nPackage operation log:\n" +
                                              result_text)
+                    if isinstance(exception, SystemError):
+                        # No need to retry SystemError, since it's most
+                        # likely a permanent error.
+                        break
                 else:
                     result_text = (fetch_output.getvalue()
                                    + read_file(install_output_path))

=== modified file 'landscape/package/tests/test_facade.py'
--- landscape/package/tests/test_facade.py	2014-12-09 07:54:03 +0000
+++ landscape/package/tests/test_facade.py	2015-04-15 15:06:54 +0000
@@ -1065,7 +1065,7 @@
              "Stderr output", "Stdout output again"],
             output)
 
-    def _test_retry_changes(self, error_type):
+    def test_retry_changes_lock_failed(self):
         """
         Test that changes are retried with the given exception type.
         """
@@ -1080,7 +1080,7 @@
         def commit1(fetch_progress, install_progress):
             self.facade._cache.commit = commit2
             os.write(2, "bad stuff!\n")
-            raise error_type("Oops")
+            raise LockFailedException("Oops")
 
         def commit2(fetch_progress, install_progress):
             install_progress.dpkg_exited = True
@@ -1095,15 +1095,28 @@
 
     def test_retry_changes_system_error(self):
         """
-        Changes are retried in the event of a SystemError.
-        """
-        self._test_retry_changes(SystemError)
-
-    def test_retry_changes_lock_failed(self):
-        """
-        Changes are retried in the event of a L{LockFailedException}.
-        """
-        self._test_retry_changes(LockFailedException)
+        Changes are not retried in the event of a SystemError, since
+        it's most likely a permanent error.
+        """
+        self.facade.max_dpkg_retries = 1
+        deb_dir = self.makeDir()
+        self._add_package_to_deb_dir(deb_dir, "foo")
+        self.facade.add_channel_apt_deb("file://%s" % deb_dir, "./")
+        self.facade.reload_channels()
+        [foo] = self.facade.get_packages_by_name("foo")
+        self.facade.mark_install(foo)
+
+        def commit1(fetch_progress, install_progress):
+            self.facade._cache.commit = commit2
+            os.write(2, "bad stuff!\n")
+            raise SystemError("Oops")
+
+        def commit2(fetch_progress, install_progress):
+            install_progress.dpkg_exited = True
+            os.write(1, "good stuff!")
+
+        self.facade._cache.commit = commit1
+        self.assertRaises(TransactionError, self.facade.perform_changes)
 
     def test_perform_changes_dpkg_error_real(self):
         """