Merge lp:~rbalint/update-manager/retry-after-failed-transaction into lp:update-manager

[Balint Reczey]

[Balint Reczey (rbalint)]

Since update-manager does not hold a lock unattended-upgrades can change installed packages behind its back and u-m gets a failed transaction due to holding an outdated cache.
The user is now notified about the problem and can try again.

[Julian Andres Klode (juliank)]

bug 1702083 is

  File "/usr/lib/python3/dist-packages/UpdateManager/Core/UpdateList.py", line 471, in update
    self.distUpgradeWouldDelete = cache.saveDistUpgrade()
AttributeError: 'NoneType' object has no attribute 'saveDistUpgrade'

I don't see how that's relevant here.

[Balint Reczey (rbalint)]

> bug 1702083 is
>
> File "/usr/lib/python3/dist-packages/UpdateManager/Core/UpdateList.py", line
> 471, in update
> self.distUpgradeWouldDelete = cache.saveDistUpgrade()
> AttributeError: 'NoneType' object has no attribute 'saveDistUpgrade'
>
> I don't see how that's relevant here.

Ah, yes, sorry, fixed to LP: #1791931

[Julian Andres Klode (juliank)]

Needs some fixing

review: Needs Fixing

[lp:~rbalint/update-manager/retry-after-failed-transaction]

2837. By Balint Reczey on 2018-09-11

Don't ask backend to do package operations aready done

Aptdaemon cancels the transaction when asked to remove packages already
removed which results the failure being shown to the user. This
is unnecessary as update-manager can just filter the package operations to
be done using a fresh cache and decrease the likelyhood of hitting
a race condition where packages to be removed are already removed.

LP: #1791931

2838. By Balint Reczey on 2018-09-11

Update changelog

[Balint Reczey (rbalint)]

> Needs some fixing

Thanks, fixed those.

[Julian Andres Klode (juliank)]

lgtm

review: Approve

=== modified file 'UpdateManager/Core/MyCache.py'
--- UpdateManager/Core/MyCache.py	2018-03-18 21:43:58 +0000
+++ UpdateManager/Core/MyCache.py	2018-09-11 14:59:42 +0000
@@ -68,6 +68,8 @@
 
     def __init__(self, progress, rootdir=None):
         apt.Cache.__init__(self, progress, rootdir)
+        # save for later
+        self.rootdir = rootdir
         # raise if we have packages in reqreinst state
         # and let the caller deal with that (runs partial upgrade)
         assert len(self.req_reinstall_pkgs) == 0

=== modified file 'UpdateManager/UpdateManager.py'
--- UpdateManager/UpdateManager.py	2018-06-08 06:35:03 +0000
+++ UpdateManager/UpdateManager.py	2018-09-11 14:59:42 +0000
@@ -272,8 +272,8 @@
                 return HWEUpgradeDialog(self)
             return UpdatesAvailable(self, header, desc, need_reboot)
 
-    def start_error(self, is_update_error, header, desc):
-        if is_update_error:
+    def start_error(self, update_and_retry, header, desc):
+        if update_and_retry:
             self._start_pane(UpdateErrorDialog(self, header, desc))
         else:
             self._start_pane(ErrorDialog(self, header, desc))

=== modified file 'UpdateManager/backend/InstallBackendAptdaemon.py'
--- UpdateManager/backend/InstallBackendAptdaemon.py	2018-03-26 12:33:11 +0000
+++ UpdateManager/backend/InstallBackendAptdaemon.py	2018-09-11 14:59:42 +0000
@@ -46,6 +46,7 @@
         self.unity = UnitySupport()
         self._expanded_size = None
         self.button_cancel = None
+        self.trans_failed_msg = None
 
     def close(self):
         if self.button_cancel:
@@ -94,6 +95,8 @@
             self._action_done(self.ACTION_INSTALL,
                               authorized=False, success=False,
                               error_string=None, error_desc=None)
+        except errors.TransactionFailed as e:
+            self.trans_failed_msg = str(e)
         except dbus.DBusException as e:
             #print(e, e.get_dbus_name())
             if e.get_dbus_name() != "org.freedesktop.DBus.Error.NoReply":
@@ -223,15 +226,20 @@
     def _on_finished(self, trans, status, action):
         error_string = None
         error_desc = None
+        trans_failed = False
         if status == EXIT_FAILED:
             error_string = get_error_string_from_enum(trans.error.code)
             error_desc = get_error_description_from_enum(trans.error.code)
+            if self.trans_failed_msg:
+                trans_failed = True
+                error_desc = error_desc + "\n" + self.trans_failed_msg
         # tell unity to hide the progress again
         self.unity.set_progress(-1)
         is_success = (status == EXIT_SUCCESS)
         self._action_done(action,
                           authorized=True, success=is_success,
-                          error_string=error_string, error_desc=error_desc)
+                          error_string=error_string, error_desc=error_desc,
+                          trans_failed=trans_failed)
 
 
 if __name__ == "__main__":

=== modified file 'UpdateManager/backend/__init__.py'
--- UpdateManager/backend/__init__.py	2018-03-26 12:33:11 +0000
+++ UpdateManager/backend/__init__.py	2018-09-11 14:59:42 +0000
@@ -8,6 +8,7 @@
 
 from gi.repository import GLib
 
+from apt import Cache
 import os
 
 from UpdateManager.Core.utils import inhibit_sleep
@@ -34,16 +35,26 @@
             pkgs_install = []
             pkgs_upgrade = []
             pkgs_remove = []
+            # Get a fresh cache in case update-manager's is outdated to
+            # skip operations that already took place
+            fresh_cache = Cache(rootdir=self.window_main.cache.rootdir)
             for pkg in self.window_main.cache:
-                if pkg.marked_install:
-                    pkgname = pkg.name
-                    if pkg.is_auto_installed:
-                        pkgname += "#auto"
-                    pkgs_install.append(pkgname)
-                elif pkg.marked_upgrade:
-                    pkgs_upgrade.append(pkg.name)
-                elif pkg.marked_delete:
-                    pkgs_remove.append(pkg.name)
+                try:
+                    if pkg.marked_install and \
+                       not fresh_cache[pkg.name].is_installed:
+                        pkgname = pkg.name
+                        if pkg.is_auto_installed:
+                            pkgname += "#auto"
+                        pkgs_install.append(pkgname)
+                    elif (pkg.marked_upgrade and
+                          fresh_cache[pkg.name].is_upgradable):
+                        pkgs_upgrade.append(pkg.name)
+                    elif (pkg.marked_delete and
+                          fresh_cache[pkg.name].is_installed):
+                        pkgs_remove.append(pkg.name)
+                except KeyError:
+                    # pkg missing from fresh_cache can't be modified
+                    pass
             self.commit(pkgs_install, pkgs_upgrade, pkgs_remove)
         else:
             self.update()
@@ -57,7 +68,7 @@
         raise NotImplemented
 
     def _action_done(self, action, authorized, success, error_string,
-                     error_desc):
+                     error_desc, trans_failed=False):
 
         # If the progress dialog should be closed automatically afterwards
         #settings = Gio.Settings.new("com.ubuntu.update-manager")
@@ -70,7 +81,8 @@
             if success:
                 self.window_main.start_available()
             elif error_string:
-                self.window_main.start_error(False, error_string, error_desc)
+                self.window_main.start_error(trans_failed, error_string,
+                                             error_desc)
             else:
                 # exit gracefuly, we can't just exit as this will trigger
                 # a crash if system.exit() is called in a exception handler

=== modified file 'debian/changelog'
--- debian/changelog	2018-09-06 07:10:42 +0000
+++ debian/changelog	2018-09-11 14:59:42 +0000
@@ -1,3 +1,17 @@
+update-manager (1:18.10.8) UNRELEASED; urgency=medium
+
+  * Print transaction error and let the user try again applying updates
+    (LP: #1317164)
+  * Don't ask backend to do package operations aready done.
+    Aptdaemon cancels the transaction when asked to remove packages already
+    removed which results the failure being shown to the user. This
+    is unnecessary as update-manager can just filter the package operations to
+    be done using a fresh cache and decrease the likelyhood of hitting
+    a race condition where packages to be removed are already removed.
+    (LP: #1791931)
+
+ -- Balint Reczey <[e-mail]>  Tue, 11 Sep 2018 13:40:57 +0200
+
 update-manager (1:18.10.7) cosmic; urgency=medium
 
   * Fix PEP8 regression from 18.10.4 to allow autopkgtest to pass