Merge ~juliank/ubuntu-release-upgrader:improve-obsoletes into ubuntu-release-upgrader:ubuntu/main

[Julian Andres Klode]

[Julian Andres Klode (juliank)]

I think we likely want to ship this _after_ the current fixes in ubuntu/main have landed in -updates, as this is a bigger change for performance, but ymmv.

Unmerged commits

ae0c35f... by Julian Andres Klode on 2024-04-29

Reimplement obsolete removal with resolve_by_keep

Basically the idea is that we pass auto_fix=False to the cache
and then use resolve_by_keep() to revert any removals that cause
additional removals.

This is easier than it sounds however, as we need to pay special
attention to forced_obsoletes. As can be seen in the code, forced
obsoletes also remove additional dependencies (they do not check
the changes made by mark_delete() and revert it).

So we solve this with a two stage approach:

1. We mark the forced obsoletes for removal with auto_fix turned
   on. This will keep the behavior for them the same. If one of
   them can't be removed, we end up reverting it.

   For this, we need to keep the code to create and restore the
   snapshot.

2. We mark the other packages for removal without auto_fix and
   then call ProblemResolver.resolve_by_keep() to fix any broken
   packages by keeping them back.

This brings runtime from multiple seconds down to half a second
for a bare lxd container:

2024-04-29 10:34:35,549 DEBUG Start checking for obsolete pkgs
2024-04-29 10:34:35,912 DEBUG Finish checking for obsolete pkgs

It used to be over 11s before:

2024-04-29 10:28:59,382 DEBUG Start checking for obsolete pkgs
2024-04-29 10:29:11,855 DEBUG Finish checking for obsolete pkgs

LP: #1874272

11047fa... by Julian Andres Klode on 2024-04-29

Move the actiongroup out of the obsoletes loop

This improves performance as we do not need to check for autoremovable
packages after each removal.

diff --git a/DistUpgrade/DistUpgradeCache.py b/DistUpgrade/DistUpgradeCache.py
index 0692ef9..77ffb64 100644
--- a/DistUpgrade/DistUpgradeCache.py
+++ b/DistUpgrade/DistUpgradeCache.py
@@ -940,7 +940,7 @@ class MyCache(apt.Cache):
         return False
 
     @withResolverLog
-    def tryMarkObsoleteForRemoval(self, pkgname, remove_candidates, forced_obsoletes, foreign_pkgs):
+    def tryMarkObsoleteForRemoval(self, pkgname, remove_candidates, forced_obsoletes, foreign_pkgs, auto_fix):
         #logging.debug("tryMarkObsoleteForRemoval(): %s" % pkgname)
         # coherence check, first see if it looks like a running kernel pkg
         if pkgname.endswith(self.uname):
@@ -972,28 +972,27 @@ class MyCache(apt.Cache):
         # if this package has not been forced obsolete, only
         # delete it if it doesn't remove other dependents
         # that are not obsolete as well
-        actiongroup = apt_pkg.ActionGroup(self._depcache)
-        # just make pyflakes shut up, later we should use
-        # with self.actiongroup():
-        actiongroup
-        self.create_snapshot()
+        if auto_fix:
+            self.create_snapshot()
         try:
-            self[pkgname].mark_delete(purge=purge)
+            self[pkgname].mark_delete(purge=purge, auto_fix=auto_fix)
             self.view.processEvents()
-            if pkgname in forced_obsoletes:
-                return True
-            #logging.debug("marking '%s' for removal" % pkgname)
-            for pkg in self.get_changes():
-                if (pkg.name not in remove_candidates or
-                      pkg.name in foreign_pkgs or
-                      self._inRemovalDenylist(pkg.name) or
-                      pkg.name == self.linux_metapackage):
-                    logging.debug("package '%s' produces an unwanted removal '%s', skipping" % (pkgname, pkg.name))
-                    self.restore_snapshot()
-                    return False
+            if auto_fix:
+                if pkgname in forced_obsoletes:
+                    return True
+                #logging.debug("marking '%s' for removal" % pkgname)
+                for pkg in self.get_changes():
+                    if (pkg.name not in remove_candidates or
+                          pkg.name in foreign_pkgs or
+                          self._inRemovalDenylist(pkg.name) or
+                          pkg.name == self.linux_metapackage):
+                        logging.debug("package '%s' produces an unwanted removal '%s', skipping" % (pkgname, pkg.name))
+                        self.restore_snapshot()
+                        return False
         except (SystemError, KeyError) as e:
             logging.warning("_tryMarkObsoleteForRemoval failed for '%s' (%s: %s)" % (pkgname, repr(e), e))
-            self.restore_snapshot()
+            if auto_fix:
+                self.restore_snapshot()
             return False
         return True
 
diff --git a/DistUpgrade/DistUpgradeController.py b/DistUpgrade/DistUpgradeController.py
index eebe281..f7853ae 100644
--- a/DistUpgrade/DistUpgradeController.py
+++ b/DistUpgrade/DistUpgradeController.py
@@ -1995,12 +1995,41 @@ class DistUpgradeController(object):
         logging.debug("remove_candidates: '%s'" % remove_candidates)
         logging.debug("Start checking for obsolete pkgs")
         progress = self._view.getOpCacheProgress()
-        for (i, pkgname) in enumerate(remove_candidates):
-            progress.update((i/float(len(remove_candidates)))*100.0)
-            if pkgname not in self.foreign_pkgs:
-                self._view.processEvents()
-                if not self.cache.tryMarkObsoleteForRemoval(pkgname, remove_candidates, self.forced_obsoletes, self.foreign_pkgs):
-                    logging.debug("'%s' scheduled for remove but not safe to remove, skipping", pkgname)
+        scheduled_remove = set()
+
+        with self.cache.actiongroup():
+            # Forced obsoletes we remove, removing any of their dependencies, hence do a first loop with auto_fix=True
+            for (i, pkgname) in enumerate(remove_candidates):
+                progress.update((i/float(len(remove_candidates)))*100.0 / 2)
+                if pkgname in self.foreign_pkgs and pkgname in self.force_obsoletes:
+                    self._view.processEvents()
+                    if not self.cache.tryMarkObsoleteForRemoval(pkgname, remove_candidates, self.forced_obsoletes, self.foreign_pkgs, auto_fix=True):
+                        logging.debug("'%s' scheduled for remove but not safe to remove, skipping", pkgname)
+                    else:
+                        scheduled_remove.add(pkgname)
+
+            # Now let's try to remove other packages
+            for (i, pkgname) in enumerate(remove_candidates):
+                progress.update((i/float(len(remove_candidates)))*100.0 / 2 + 50)
+                if pkgname not in self.foreign_pkgs:
+                    self._view.processEvents()
+                    if not self.cache.tryMarkObsoleteForRemoval(pkgname, remove_candidates, self.forced_obsoletes, self.foreign_pkgs, auto_fix=False):
+                        logging.debug("'%s' scheduled for remove but not safe to remove, skipping", pkgname)
+                    else:
+                        scheduled_remove.add(pkgname)
+
+            # We have scheduled their removals, but not any reverse-dependencies. If anything is broken now,
+            # resolve them by keeping back the obsolete packages.
+            self.cache._startAptResolverLog()
+            pr = apt.ProblemResolver(self.cache)
+            pr.resolve_by_keep()
+            self.cache._stopAptResolverLog()
+
+            # resolve_by_keep() will revert any unsafe removals, so we need to list them here again.
+            for pkgname in scheduled_remove:
+                if not self.cache[pkgname].marked_delete:
+                    logging.debug("obsolete package '%s' could not be removed", pkgname)
+
         logging.debug("Finish checking for obsolete pkgs")
         progress.done()