Introduce a "Transaction" for changes to testing

This isolates the undo handling in the new transaction object and in
doop_source, which currently generates the undo items.  This commit
will be a stepping stone to rewriting the undo handling.

Signed-off-by: Niels Thykier <niels@thykier.net>

britney.py

@@ -202,7 +202,8 @@ from britney2.migrationitem import MigrationItem
 from britney2.policies import PolicyVerdict
 from britney2.policies.policy import AgePolicy, RCBugPolicy, PiupartsPolicy, BuildDependsPolicy
 from britney2.policies.autopkgtest import AutopkgtestPolicy
-from britney2.utils import (log_and_format_old_libraries, undo_changes,
+from britney2.transaction import start_transaction
+from britney2.utils import (log_and_format_old_libraries,
                             compute_reverse_tree, get_dependency_solvers,
                             read_nuninst, write_nuninst, write_heidi,
                             format_and_log_uninst, newly_uninst, make_migrationitem,
@@ -1632,11 +1633,11 @@ class Britney(object):
 
         return (adds, rms, smoothbins, skip)
 
-    def doop_source(self, item, hint_undo=None, removals=frozenset()):
+    def doop_source(self, item, transaction, removals=frozenset()):
         """Apply a change to the target suite as requested by `item`
 
-        An optional list of undo actions related to packages processed earlier
+        A transaction in which all changes will be recorded.  Can be None (e.g.
-        in a hint may be passed in `hint_undo`.
+        during a "force-hint"), when the changes will not be rolled back.
 
         An optional set of binaries may be passed in "removals". Binaries listed
         in this set will be assumed to be removed at the same time as the "item"
@@ -1764,8 +1765,8 @@ class Britney(object):
                         # all the reverse conflicts
                         affected_direct.update(pkg_universe.reverse_dependencies_of(old_pkg_id))
                     target_suite.remove_binary(old_pkg_id)
-                elif hint_undo:
+                elif transaction and transaction.parent_transaction:
-                    # the binary isn't in testing, but it may have been at
+                    # the binary isn't in the target suite, but it may have been at
                     # the start of the current hint and have been removed
                     # by an earlier migration. if that's the case then we
                     # will have a record of the older instance of the binary
@@ -1775,7 +1776,7 @@ class Britney(object):
                     # reverse dependencies built from this source can be
                     # ignored as their reverse trees are already handled
                     # by this function
-                    for (tundo, tpkg) in hint_undo:
+                    for (tundo, tpkg) in transaction.parent_transaction.undo_items:
                         if key in tundo['binaries']:
                             tpkg_id = tundo['binaries'][key]
                             affected_direct.update(pkg_universe.reverse_dependencies_of(tpkg_id))
@@ -1801,10 +1802,12 @@ class Britney(object):
         # Also include the transitive rdeps of the packages found so far
         affected_all = affected_direct.copy()
         compute_reverse_tree(pkg_universe, affected_all)
-        # return the package name, the suite, the list of affected packages and the undo dictionary
+        if transaction:
-        return (affected_direct, affected_all, undo)
+            transaction.add_undo_item(undo, item)
+        # return the affected packages (direct and than all)
+        return (affected_direct, affected_all)
 
-    def try_migration(self, actions, nuninst_now, lundo=None, automatic_revert=True):
+    def try_migration(self, actions, nuninst_now, transaction, automatic_revert=True):
         is_accepted = True
         affected_architectures = set()
         item = actions
@@ -1819,14 +1822,12 @@ class Britney(object):
         if len(actions) == 1:
             item = actions[0]
             # apply the changes
-            affected_direct, affected_all, undo = self.doop_source(item, hint_undo=lundo)
+            affected_direct, affected_all = self.doop_source(item, transaction)
-            undo_list = [(undo, item)]
             if item.architecture == 'source':
                 affected_architectures = set(self.options.architectures)
             else:
                 affected_architectures.add(item.architecture)
         else:
-            undo_list = []
             removals = set()
             affected_direct = set()
             affected_all = set()
@@ -1842,12 +1843,11 @@ class Britney(object):
                 affected_architectures = set(self.options.architectures)
 
             for item in actions:
-                item_affected_direct, item_affected_all, undo = self.doop_source(item,
+                item_affected_direct, item_affected_all = self.doop_source(item,
-                                                                                 hint_undo=lundo,
+                                                                           transaction,
-                                                                                 removals=removals)
+                                                                           removals=removals)
                 affected_direct.update(item_affected_direct)
                 affected_all.update(item_affected_all)
-                undo_list.append((undo, item))
 
         # Optimise the test if we may revert directly.
         # - The automatic-revert is needed since some callers (notably via hints) may
@@ -1892,12 +1892,11 @@ class Britney(object):
 
         # check if the action improved the uninstallability counters
         if not is_accepted and automatic_revert:
-            undo_copy = list(reversed(undo_list))
+            transaction.rollback()
-            undo_changes(undo_copy, self.suite_info, self.all_binaries)
 
-        return (is_accepted, nuninst_after, undo_list, arch)
+        return (is_accepted, nuninst_after, arch)
 
-    def iter_packages(self, packages, selected, nuninst=None, lundo=None):
+    def iter_packages(self, packages, selected, nuninst=None, parent_transaction=None):
         """Iter on the list of actions and apply them one-by-one
 
         This method applies the changes from `packages` to testing, checking the uninstallability
@@ -1909,6 +1908,8 @@ class Britney(object):
         rescheduled_packages = packages
         maybe_rescheduled_packages = []
         output_logger = self.output_logger
+        suite_info = self.suite_info
+        all_binaries = self.all_binaries
         solver = InstallabilitySolver(self.pkg_universe, self._inst_tester)
 
         for y in sorted((y for y in packages), key=attrgetter('uvname')):
@@ -1935,45 +1936,47 @@ class Britney(object):
                 comp = worklist.pop()
                 comp_name = ' '.join(item.uvname for item in comp)
                 output_logger.info("trying: %s" % comp_name)
-                accepted, nuninst_after, comp_undo, failed_arch = self.try_migration(comp, nuninst_last_accepted, lundo)
+                with start_transaction(suite_info, all_binaries, parent_transaction) as transaction:
-                if accepted:
+                    accepted, nuninst_after, failed_arch = self.try_migration(comp,
-                    selected.extend(comp)
+                                                                              nuninst_last_accepted,
-                    if lundo is not None:
+                                                                              transaction)
-                        lundo.extend(comp_undo)
+                    if accepted:
-                    output_logger.info("accepted: %s", comp_name)
+                        selected.extend(comp)
-                    output_logger.info("   ori: %s", self.eval_nuninst(nuninst_orig))
+                        transaction.commit()
-                    output_logger.info("   pre: %s", self.eval_nuninst(nuninst_last_accepted))
+                        output_logger.info("accepted: %s", comp_name)
-                    output_logger.info("   now: %s", self.eval_nuninst(nuninst_after))
+                        output_logger.info("   ori: %s", self.eval_nuninst(nuninst_orig))
-                    if len(selected) <= 20:
+                        output_logger.info("   pre: %s", self.eval_nuninst(nuninst_last_accepted))
-                        output_logger.info("   all: %s", " ".join(x.uvname for x in selected))
+                        output_logger.info("   now: %s", self.eval_nuninst(nuninst_after))
+                        if len(selected) <= 20:
+                            output_logger.info("   all: %s", " ".join(x.uvname for x in selected))
+                        else:
+                            output_logger.info("  most: (%d) .. %s",
+                                               len(selected),
+                                               " ".join(x.uvname for x in selected[-20:]))
+                        nuninst_last_accepted = nuninst_after
+                        rescheduled_packages.extend(maybe_rescheduled_packages)
+                        maybe_rescheduled_packages.clear()
                     else:
-                        output_logger.info("  most: (%d) .. %s",
+                        broken = sorted(b for b in nuninst_after[failed_arch]
-                                           len(selected),
+                                        if b not in nuninst_last_accepted[failed_arch])
-                                           " ".join(x.uvname for x in selected[-20:]))
+                        compare_nuninst = None
-                    nuninst_last_accepted = nuninst_after
+                        if any(item for item in comp if item.architecture != 'source'):
-                    rescheduled_packages.extend(maybe_rescheduled_packages)
+                            compare_nuninst = nuninst_last_accepted
-                    maybe_rescheduled_packages.clear()
+                        # NB: try_migration already reverted this for us, so just print the results and move on
-                else:
+                        output_logger.info("skipped: %s (%d, %d, %d)",
-                    broken = sorted(b for b in nuninst_after[failed_arch]
+                                           comp_name,
-                                    if b not in nuninst_last_accepted[failed_arch])
+                                           len(rescheduled_packages),
-                    compare_nuninst = None
+                                           len(maybe_rescheduled_packages),
-                    if any(item for item in comp if item.architecture != 'source'):
+                                           len(worklist)
-                        compare_nuninst = nuninst_last_accepted
+                                           )
-                    # NB: try_migration already reverted this for us, so just print the results and move on
+                        output_logger.info("    got: %s", self.eval_nuninst(nuninst_after, compare_nuninst))
-                    output_logger.info("skipped: %s (%d, %d, %d)",
+                        output_logger.info("    * %s: %s", failed_arch, ", ".join(broken))
-                                       comp_name,
-                                       len(rescheduled_packages),
-                                       len(maybe_rescheduled_packages),
-                                       len(worklist)
-                                       )
-                    output_logger.info("    got: %s", self.eval_nuninst(nuninst_after, compare_nuninst))
-                    output_logger.info("    * %s: %s", failed_arch, ", ".join(broken))
 
-                    if len(comp) > 1:
+                        if len(comp) > 1:
-                        output_logger.info("    - splitting the component into single items and retrying them")
+                            output_logger.info("    - splitting the component into single items and retrying them")
-                        worklist.extend([item] for item in comp)
+                            worklist.extend([item] for item in comp)
-                    else:
+                        else:
-                        maybe_rescheduled_packages.append(comp[0])
+                            maybe_rescheduled_packages.append(comp[0])
 
         output_logger.info(" finish: [%s]", ",".join(x.uvname for x in selected))
         output_logger.info("endloop: %s", self.eval_nuninst(self.nuninst_orig))
@@ -1986,7 +1989,6 @@ class Britney(object):
 
         return (nuninst_last_accepted, maybe_rescheduled_packages)
 
-
     def do_all(self, hinttype=None, init=None, actions=None):
         """Testing update runner
 
@@ -2005,7 +2007,6 @@ class Britney(object):
         # these are special parameters for hints processing
         force = False
         recurse = True
-        lundo = None
         nuninst_end = None
         extra = []
 
@@ -2015,8 +2016,6 @@ class Britney(object):
 
         # if we have a list of initial packages, check them
         if init:
-            if not force:
-                lundo = []
             for x in init:
                 if x not in upgrade_me:
                     output_logger.warning("failed: %s is not a valid candidate (or it already migrated)", x.uvname)
@@ -2027,85 +2026,91 @@ class Britney(object):
         output_logger.info("start: %s", self.eval_nuninst(nuninst_start))
         output_logger.info("orig: %s", self.eval_nuninst(nuninst_start))
 
-        if init:
+        with start_transaction(self.suite_info, self.all_binaries) as transaction:
-            # init => a hint (e.g. "easy") - so do the hint run
+            if not init or force:
-            (_, nuninst_end, undo_list, _) = self.try_migration(selected,
+                # Throw away the (outer) transaction as we will not be using it
-                                                                self.nuninst_orig,
+                transaction.rollback()
-                                                                lundo=lundo,
+                transaction = None
-                                                                automatic_revert=False)
 
-            if lundo is not None:
+            if init:
-                lundo.extend(undo_list)
+                # init => a hint (e.g. "easy") - so do the hint run
+                (_, nuninst_end, undo_list,) = self.try_migration(selected,
+                                                                  self.nuninst_orig,
+                                                                  transaction,
+                                                                  automatic_revert=False)
+
+                if recurse:
+                    # Ensure upgrade_me and selected do not overlap, if we
+                    # follow-up with a recurse ("hint"-hint).
+                    upgrade_me = [x for x in upgrade_me if x not in set(selected)]
 
             if recurse:
-                # Ensure upgrade_me and selected do not overlap, if we
+                # Either the main run or the recursive run of a "hint"-hint.
-                # follow-up with a recurse ("hint"-hint).
+                (nuninst_end, extra) = self.iter_packages(upgrade_me,
-                upgrade_me = [x for x in upgrade_me if x not in set(selected)]
+                                                          selected,
+                                                          nuninst=nuninst_end,
+                                                          parent_transaction=transaction)
 
-        if recurse:
+            nuninst_end_str = self.eval_nuninst(nuninst_end)
-            # Either the main run or the recursive run of a "hint"-hint.
-            (nuninst_end, extra) = self.iter_packages(upgrade_me, selected, nuninst=nuninst_end, lundo=lundo)
 
-        nuninst_end_str = self.eval_nuninst(nuninst_end)
+            if not recurse:
+                # easy or force-hint
+                output_logger.info("easy: %s", nuninst_end_str)
 
-        if not recurse:
+                if not force:
-            # easy or force-hint
-            output_logger.info("easy: %s", nuninst_end_str)
-
-            if not force:
-                format_and_log_uninst(self.output_logger,
-                                      self.options.architectures,
-                                      newly_uninst(nuninst_start, nuninst_end)
-                                      )
-
-        if force:
-            # Force implies "unconditionally better"
-            better = True
-        else:
-            break_arches = set(self.options.break_arches)
-            if all(x.architecture in break_arches for x in selected):
-                # If we only migrated items from break-arches, then we
-                # do not allow any regressions on these architectures.
-                # This usually only happens with hints
-                break_arches = set()
-            better = is_nuninst_asgood_generous(self.constraints,
-                                                self.options.architectures,
-                                                self.nuninst_orig,
-                                                nuninst_end,
-                                                break_arches)
-
-        if better:
-            # Result accepted either by force or by being better than the original result.
-            output_logger.info("final: %s", ",".join(sorted(x.uvname for x in selected)))
-            output_logger.info("start: %s", self.eval_nuninst(nuninst_start))
-            output_logger.info(" orig: %s", self.eval_nuninst(self.nuninst_orig))
-            output_logger.info("  end: %s", nuninst_end_str)
-            if force:
-                broken = newly_uninst(nuninst_start, nuninst_end)
-                if broken:
-                    output_logger.warning("force breaks:")
                     format_and_log_uninst(self.output_logger,
                                           self.options.architectures,
-                                          broken,
+                                          newly_uninst(nuninst_start, nuninst_end)
-                                          loglevel=logging.WARNING,
                                           )
-                else:
-                    output_logger.info("force did not break any packages")
-            output_logger.info("SUCCESS (%d/%d)", len(actions or self.upgrade_me), len(extra))
-            self.nuninst_orig = nuninst_end
-            self.all_selected += selected
-            if not actions:
-                if recurse:
-                    self.upgrade_me = extra
-                else:
-                    self.upgrade_me = [x for x in self.upgrade_me if x not in set(selected)]
-        else:
-            output_logger.info("FAILED\n")
-            if not lundo:
-                return
-            lundo.reverse()
 
-            undo_changes(lundo, self.suite_info, self.all_binaries)
+            if force:
+                # Force implies "unconditionally better"
+                better = True
+            else:
+                break_arches = set(self.options.break_arches)
+                if all(x.architecture in break_arches for x in selected):
+                    # If we only migrated items from break-arches, then we
+                    # do not allow any regressions on these architectures.
+                    # This usually only happens with hints
+                    break_arches = set()
+                better = is_nuninst_asgood_generous(self.constraints,
+                                                    self.options.architectures,
+                                                    self.nuninst_orig,
+                                                    nuninst_end,
+                                                    break_arches)
+
+            if better:
+                # Result accepted either by force or by being better than the original result.
+                output_logger.info("final: %s", ",".join(sorted(x.uvname for x in selected)))
+                output_logger.info("start: %s", self.eval_nuninst(nuninst_start))
+                output_logger.info(" orig: %s", self.eval_nuninst(self.nuninst_orig))
+                output_logger.info("  end: %s", nuninst_end_str)
+                if force:
+                    broken = newly_uninst(nuninst_start, nuninst_end)
+                    if broken:
+                        output_logger.warning("force breaks:")
+                        format_and_log_uninst(self.output_logger,
+                                              self.options.architectures,
+                                              broken,
+                                              loglevel=logging.WARNING,
+                                              )
+                    else:
+                        output_logger.info("force did not break any packages")
+                output_logger.info("SUCCESS (%d/%d)", len(actions or self.upgrade_me), len(extra))
+                self.nuninst_orig = nuninst_end
+                self.all_selected += selected
+                if transaction:
+                    transaction.commit()
+                if not actions:
+                    if recurse:
+                        self.upgrade_me = extra
+                    else:
+                        self.upgrade_me = [x for x in self.upgrade_me if x not in set(selected)]
+            else:
+                output_logger.info("FAILED\n")
+                if not transaction:
+                    return
+                transaction.rollback()
 
         output_logger.info("")
 

britney2/transaction.py

@@ -0,0 +1,131 @@
+import contextlib
+
+
+@contextlib.contextmanager
+def start_transaction(suite_info, all_binaries, parent_transaction=None):
+    tmts = MigrationTransactionState(suite_info, all_binaries, parent_transaction)
+    try:
+        yield tmts
+    except Exception:
+        if not tmts.is_committed and not tmts.is_rolled_back:
+            tmts.rollback()
+        raise
+    assert tmts.is_rolled_back or tmts.is_committed
+
+
+class MigrationTransactionState(object):
+
+    def __init__(self, suite_info, all_binaries, parent=None):
+        self._suite_info = suite_info
+        self._all_binaries = all_binaries
+        self.parent_transaction = parent
+        self._is_rolled_back = False
+        self._is_committed = False
+        self._undo_items = []
+
+    def add_undo_item(self, undo, item):
+        self._assert_open_transaction()
+        self._undo_items.append((undo, item))
+
+    def _assert_open_transaction(self):
+        assert not self._is_rolled_back and not self._is_committed
+        p = self.parent_transaction
+        if p:
+            p._assert_open_transaction()
+
+    @property
+    def undo_items(self):
+        """Only needed by a doop_source for the "hint"-hint case"""
+        yield from self._undo_items
+
+    def commit(self):
+        """Commit the transaction
+
+        After this call, it is not possible to roll these changes
+        back (except if there is a parent transaction, which can
+        still be rolled back).
+        """
+        self._assert_open_transaction()
+        self._is_committed = True
+        if self.parent_transaction:
+            for undo_item in self._undo_items:
+                self.parent_transaction.add_undo_item(*undo_item)
+
+    def rollback(self):
+        """Rollback all recorded changes by this transaction
+
+        The parent transaction (if any) will remain unchanged
+        """
+
+        self._assert_open_transaction()
+
+        self._is_rolled_back = True
+        lundo = self._undo_items
+        lundo.reverse()
+
+        # We do the undo process in "4 steps" and each step must be
+        # fully completed for each undo-item before starting on the
+        # next.
+        #
+        # see commit:ef71f0e33a7c3d8ef223ec9ad5e9843777e68133 and
+        # #624716 for the issues we had when we did not do this.
+
+        all_binary_packages = self._all_binaries
+        target_suite = self._suite_info.target_suite
+        sources_t = target_suite.sources
+        binaries_t = target_suite.binaries
+        provides_t = target_suite.provides_table
+
+        # STEP 1
+        # undo all the changes for sources
+        for (undo, item) in lundo:
+            for k in undo['sources']:
+                if k[0] == '-':
+                    del sources_t[k[1:]]
+                else:
+                    sources_t[k] = undo['sources'][k]
+
+        # STEP 2
+        # undo all new binaries (consequence of the above)
+        for (undo, item) in lundo:
+            if not item.is_removal and item.package in item.suite.sources:
+                source_data = item.suite.sources[item.package]
+                for pkg_id in source_data.binaries:
+                    binary, _, arch = pkg_id
+                    if item.architecture in ['source', arch]:
+                        try:
+                            del binaries_t[arch][binary]
+                        except KeyError:
+                            # If this happens, pkg_id must be a cruft item that
+                            # was *not* migrated.
+                            assert source_data.version != all_binary_packages[pkg_id].version
+                            assert not target_suite.is_pkg_in_the_suite(pkg_id)
+                        target_suite.remove_binary(pkg_id)
+
+        # STEP 3
+        # undo all other binary package changes (except virtual packages)
+        for (undo, item) in lundo:
+            for p in undo['binaries']:
+                binary, arch = p
+                binaries_t_a = binaries_t[arch]
+                assert binary not in binaries_t_a
+                pkgdata = all_binary_packages[undo['binaries'][p]]
+                binaries_t_a[binary] = pkgdata
+                target_suite.add_binary(pkgdata.pkg_id)
+
+        # STEP 4
+        # undo all changes to virtual packages
+        for (undo, item) in lundo:
+            for provided_pkg, arch in undo['nvirtual']:
+                del provides_t[arch][provided_pkg]
+            for p in undo['virtual']:
+                provided_pkg, arch = p
+                provides_t[arch][provided_pkg] = undo['virtual'][p]
+
+    @property
+    def is_rolled_back(self):
+        return self._is_rolled_back
+
+    @property
+    def is_committed(self):
+        return self._is_committed

britney2/utils.py

@@ -95,74 +95,6 @@ def iter_except(func, exception, first=None):
         pass
 
 
-def undo_changes(lundo, suite_info, all_binary_packages):
-    """Undoes one or more changes to the target suite
-
-    * lundo is a list of (undo, item)-tuples
-    * suite_info is the Suites object
-    * all_binary_packages is the table of all binary packages for
-      all suites and architectures
-    """
-
-    # We do the undo process in "4 steps" and each step must be
-    # fully completed for each undo-item before starting on the
-    # next.
-    #
-    # see commit:ef71f0e33a7c3d8ef223ec9ad5e9843777e68133 and
-    # #624716 for the issues we had when we did not do this.
-
-    target_suite = suite_info.target_suite
-    sources_t = target_suite.sources
-    binaries_t = target_suite.binaries
-    provides_t = target_suite.provides_table
-
-    # STEP 1
-    # undo all the changes for sources
-    for (undo, item) in lundo:
-        for k in undo['sources']:
-            if k[0] == '-':
-                del sources_t[k[1:]]
-            else:
-                sources_t[k] = undo['sources'][k]
-
-    # STEP 2
-    # undo all new binaries (consequence of the above)
-    for (undo, item) in lundo:
-        if not item.is_removal and item.package in item.suite.sources:
-            source_data = item.suite.sources[item.package]
-            for pkg_id in source_data.binaries:
-                binary, _, arch = pkg_id
-                if item.architecture in ['source', arch]:
-                    try:
-                        del binaries_t[arch][binary]
-                    except KeyError:
-                        # If this happens, pkg_id must be a cruft item that
-                        # was *not* migrated.
-                        assert source_data.version != all_binary_packages[pkg_id].version
-                        assert not target_suite.is_pkg_in_the_suite(pkg_id)
-                    target_suite.remove_binary(pkg_id)
-
-    # STEP 3
-    # undo all other binary package changes (except virtual packages)
-    for (undo, item) in lundo:
-        for p in undo['binaries']:
-            binary, arch = p
-            binaries_t_a = binaries_t[arch]
-            assert binary not in binaries_t_a
-            pkgdata = all_binary_packages[undo['binaries'][p]]
-            binaries_t_a[binary] = pkgdata
-            target_suite.add_binary(pkgdata.pkg_id)
-
-    # STEP 4
-    # undo all changes to virtual packages
-    for (undo, item) in lundo:
-        for provided_pkg, arch in undo['nvirtual']:
-            del provides_t[arch][provided_pkg]
-        for p in undo['virtual']:
-            provided_pkg, arch = p
-            provides_t[arch][provided_pkg] = undo['virtual'][p]
-
-
 def log_and_format_old_libraries(logger, libs):
     """Format and log old libraries in a table (no header)"""
     libraries = {}