launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/multiple-override-race into lp:launchpad.

Commit message:
Fix multiple-override race for architecture-independent binaries.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #180218 in Launchpad itself: "override mismatch race needs to be fixed"
  https://bugs.launchpad.net/launchpad/+bug/180218

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/multiple-override-race/+merge/368023

If an architecture-independent binary had multiple identical overrides created in the same publisher run (for example by multiple archive administrators racing to fulfil the same request, or by the phased-updates script), then BinaryPackagePublishingHistory.supersede would supersede them all without ensuring that the newest publication remained live.

The architecture-independent sibling publications don't have a formal relation to each other in the data model, which is why this has always been hard to fix.  However, the dominator sorts publications with equal versions by their creation dates, so it will always consider the most recent override to be live.  We can therefore fix this by deferring the superseding of architecture-independent siblings until we've positively confirmed which publications are live using the dominator's normal logic, and taking care not to supersede the confirmed-live publications.

An additional complication is that the dominator processes one architecture at a time, so in general it won't have confirmed the full set of live publications until it has processed all architectures.  To cope with this, I've lifted the handling of architecture-independent siblings out of BinaryPackagePublishingHistory.supersede into a new BinaryPackagePublishingHistory.supersedeAssociated method.  The supersede method calls supersedeAssociated by default to minimise chaos in the test suite, but the dominator passes a flag to disable that and calls supersedeAssociated separately.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/multiple-override-race into lp:launchpad.

=== modified file 'lib/lp/archivepublisher/domination.py'
--- lib/lp/archivepublisher/domination.py	2014-10-31 10:34:51 +0000
+++ lib/lp/archivepublisher/domination.py	2019-05-28 23:48:46 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Archive Domination class.
@@ -369,7 +369,7 @@
         self.archive = archive
 
     def dominatePackage(self, sorted_pubs, live_versions, generalization,
-                        immutable_check=True):
+                        immutable_check=True, superseded=None, keep=None):
         """Dominate publications for a single package.
 
         The latest publication for any version in `live_versions` stays
@@ -397,6 +397,17 @@
             are listed in `live_versions` are marked as Deleted.
         :param generalization: A `GeneralizedPublication` helper representing
             the kind of publications these are: source or binary.
+        :param immutable_check: If True, fail if a deletion would modify an
+            immutable suite (e.g. the RELEASE pocket of a CURRENT series).
+        :param superseded: If not None, append (superseded publication,
+            dominant publication) pairs to this list when marking a
+            publication as superseded.  Binary domination uses this to
+            supersede other publications associated with the superseded
+            ones.
+        :param keep: If not None, add publications that have been confirmed
+            as live to this set.  Binary domination uses this to ensure that
+            these live publications are not superseded when superseding
+            associated publications.
         """
         live_versions = frozenset(live_versions)
 
@@ -423,7 +434,11 @@
                 # This publication is for a live version, but has been
                 # superseded by a newer publication of the same version.
                 # Supersede it.
-                pub.supersede(current_dominant, logger=self.logger)
+                pub.supersede(
+                    current_dominant, supersede_associated=False,
+                    logger=self.logger)
+                if superseded is not None:
+                    superseded.append((pub, current_dominant))
                 self.logger.debug2(
                     "Superseding older publication for version %s.", version)
             elif version in live_versions:
@@ -432,6 +447,8 @@
                 # this is the release that they are superseded by.
                 current_dominant = pub
                 dominant_version = version
+                if keep is not None:
+                    keep.add(pub)
                 self.logger.debug2("Keeping version %s.", version)
             elif current_dominant is None:
                 # This publication is no longer live, but there is no
@@ -442,7 +459,11 @@
             else:
                 # This publication is superseded.  This is what we're
                 # here to do.
-                pub.supersede(current_dominant, logger=self.logger)
+                pub.supersede(
+                    current_dominant, supersede_associated=False,
+                    logger=self.logger)
+                if superseded is not None:
+                    superseded.append((pub, current_dominant))
                 self.logger.debug2("Superseding version %s.", version)
 
     def _sortPackages(self, publications, generalization):
@@ -615,6 +636,8 @@
         # published, architecture-independent publications; anything
         # else will have completed domination in the first pass.
         packages_w_arch_indep = set()
+        superseded = []
+        keep = set()
 
         for distroarchseries in distroseries.architectures:
             self.logger.info(
@@ -630,11 +653,19 @@
                 self.logger.debug("Dominating %s" % name)
                 assert len(pubs) > 0, "Dominating zero binaries!"
                 live_versions = find_live_binary_versions_pass_1(pubs)
-                self.dominatePackage(pubs, live_versions, generalization)
+                self.dominatePackage(
+                    pubs, live_versions, generalization,
+                    superseded=superseded, keep=keep)
                 if contains_arch_indep(pubs):
                     packages_w_arch_indep.add(name)
 
+        self.logger.info("Dominating associated binaries...")
+        for pub, dominant in superseded:
+            pub.supersedeAssociated(dominant, keep=keep, logger=self.logger)
+
         packages_w_arch_indep = frozenset(packages_w_arch_indep)
+        superseded = []
+        keep = set()
 
         # The second pass attempts to supersede arch-all publications of
         # older versions, from source package releases that no longer
@@ -655,7 +686,13 @@
                 assert len(pubs) > 0, "Dominating zero binaries in 2nd pass!"
                 live_versions = find_live_binary_versions_pass_2(
                     pubs, reprieve_cache)
-                self.dominatePackage(pubs, live_versions, generalization)
+                self.dominatePackage(
+                    pubs, live_versions, generalization,
+                    superseded=superseded, keep=keep)
+
+        self.logger.info("Dominating associated binaries...(2nd pass)")
+        for pub, dominant in superseded:
+            pub.supersedeAssociated(dominant, keep=keep, logger=self.logger)
 
     def _composeActiveSourcePubsCondition(self, distroseries, pocket):
         """Compose ORM condition for restricting relevant source pubs."""

=== modified file 'lib/lp/archivepublisher/tests/test_dominator.py'
--- lib/lp/archivepublisher/tests/test_dominator.py	2018-03-02 18:35:42 +0000
+++ lib/lp/archivepublisher/tests/test_dominator.py	2019-05-28 23:48:46 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for domination.py."""
@@ -11,7 +11,11 @@
 from operator import attrgetter
 
 import apt_pkg
-from testtools.matchers import LessThan
+from testtools.matchers import (
+    GreaterThan,
+    LessThan,
+    )
+import transaction
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -326,6 +330,32 @@
         self.checkPublications(foo_10_all_bins + [foo_10_i386_bin],
                                PackagePublishingStatus.SUPERSEDED)
 
+    def test_dominatePackage_handles_double_arch_indep_override(self):
+        # If there are multiple identical publications of an
+        # architecture-independent binary, dominatePackage leaves the most
+        # recent one live.
+        originals = self.getPubBinaries(
+            status=PackagePublishingStatus.PUBLISHED,
+            architecturespecific=False)
+        self.assertThat(len(originals), GreaterThan(1))
+        self.assertNotEqual("universe", originals[0].component.name)
+        overrides_1 = [
+            pub.changeOverride(new_component="universe") for pub in originals]
+        transaction.commit()
+        overrides_2 = [
+            pub.changeOverride(new_component="universe") for pub in originals]
+        for pub in overrides_1 + overrides_2:
+            pub.setPublished()
+        dominator = Dominator(self.logger, originals[0].archive)
+        dominator.judgeAndDominate(
+            originals[0].distroseries, originals[0].pocket)
+        for pub in originals:
+            self.assertEqual(PackagePublishingStatus.SUPERSEDED, pub.status)
+        for pub in overrides_1:
+            self.assertEqual(PackagePublishingStatus.SUPERSEDED, pub.status)
+        for pub in overrides_2:
+            self.assertEqual(PackagePublishingStatus.PUBLISHED, pub.status)
+
 
 class TestDomination(TestNativePublishingBase):
     """Test overall domination procedure."""

=== modified file 'lib/lp/soyuz/interfaces/publishing.py'
--- lib/lp/soyuz/interfaces/publishing.py	2018-05-23 13:24:03 +0000
+++ lib/lp/soyuz/interfaces/publishing.py	2019-05-28 23:48:46 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Publishing interfaces."""
@@ -480,11 +480,13 @@
         :return: a list of `ILibraryFileAlias`.
         """
 
-    def supersede(dominant=None, logger=None):
+    def supersede(dominant=None, supersede_associated=True, logger=None):
         """Supersede this publication.
 
         :param dominant: optional `ISourcePackagePublishingHistory` which is
             triggering the domination.
+        :param supersede_associated: no-op here, for compatibility with
+            `IBinaryPackagePublishingHistory.supersede`.
         :param logger: optional object to which debug information will be
             logged.
         """
@@ -751,11 +753,26 @@
             required=False, readonly=True),
         as_of="devel")
 
-    def supersede(dominant=None, logger=None):
+    def supersede(dominant=None, supersede_associated=True, logger=None):
         """Supersede this publication.
 
         :param dominant: optional `IBinaryPackagePublishingHistory` which is
             triggering the domination.
+        :param supersede_associated: if True, also supersede other
+            publications associated closely with this one.  Callers may set
+            this to False and call `supersedeAssociated` themselves if they
+            need more control over which publications remain live.
+        :param logger: optional object to which debug information will be
+            logged.
+        """
+
+    def supersedeAssociated(dominant=None, keep=None, logger=None):
+        """Supersede other publications associated closely with this one.
+
+        :param dominant: optional `IBinaryPackagePublishingHistory` which is
+            triggering the domination.
+        :param keep: optional set of other associated publications that have
+            been positively determined to be live and should be skipped.
         :param logger: optional object to which debug information will be
             logged.
         """

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2019-04-16 11:13:34 +0000
+++ lib/lp/soyuz/model/publishing.py	2019-05-28 23:48:46 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -452,7 +452,7 @@
         name = release.sourcepackagename.name
         return "%s %s in %s" % (name, release.version, self.distroseries.name)
 
-    def supersede(self, dominant=None, logger=None):
+    def supersede(self, dominant=None, supersede_associated=True, logger=None):
         """See `ISourcePackagePublishingHistory`."""
         assert self.status in active_publishing_status, (
             "Should not dominate unpublished source %s" %
@@ -754,7 +754,7 @@
                 priority=self.priority,
                 phased_update_percentage=self.phased_update_percentage)
 
-    def supersede(self, dominant=None, logger=None):
+    def supersede(self, dominant=None, supersede_associated=True, logger=None):
         """See `IBinaryPackagePublishingHistory`."""
         # At this point only PUBLISHED (ancient versions) or PENDING (
         # multiple overrides/copies) publications should be given. We
@@ -802,11 +802,17 @@
         for dominated in debug:
             dominated.supersede(dominant, logger)
 
+        if supersede_associated:
+            self.supersedeAssociated(dominant, logger=logger)
+
+    def supersedeAssociated(self, dominant=None, keep=None, logger=None):
+        """See `IBinaryPackagePublishingHistory`."""
         # If this is architecture-independent, all publications with the same
         # context and overrides should be dominated simultaneously.
         if not self.binarypackagerelease.architecturespecific:
             for dominated in self._getOtherPublications():
-                dominated.supersede(dominant, logger)
+                if keep is None or dominated not in keep:
+                    dominated.supersede(dominant, logger)
 
     def changeOverride(self, new_component=None, new_section=None,
                        new_priority=None, new_phased_update_percentage=None):

=== modified file 'lib/lp/soyuz/tests/test_publishing.py'
--- lib/lp/soyuz/tests/test_publishing.py	2018-05-06 08:52:34 +0000
+++ lib/lp/soyuz/tests/test_publishing.py	2019-05-28 23:48:46 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test native publication workflow for Soyuz. """
@@ -15,7 +15,10 @@
 import pytz
 import scandir
 from storm.store import Store
-from testtools.matchers import Equals
+from testtools.matchers import (
+    Equals,
+    GreaterThan,
+    )
 import transaction
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
@@ -635,7 +638,7 @@
     def checkPublication(self, pub, status):
         """Assert the publication has the given status."""
         self.assertEqual(
-            pub.status, status, "%s is not %s (%s)" % (
+            status, pub.status, "%s is not %s (%s)" % (
             pub.displayname, status.name, pub.status.name))
 
     def checkPublications(self, pubs, status):
@@ -1157,6 +1160,31 @@
         self.checkSuperseded([bin], super_bin)
         self.assertEqual(super_date, bin.datesuperseded)
 
+    def testSkipsDominantArchIndependentBinaries(self):
+        """Check that supersedeAssociated skips dominant arch-indep binaries.
+
+        It's possible for there to be multiple pending publications with
+        identical overrides, for instance if two archive admins call
+        changeOverride with the same parameters at nearly the same time.  In
+        this situation, we make sure not to supersede the dominant one.
+        """
+        originals = self.getPubBinaries(architecturespecific=False)
+        self.assertThat(len(originals), GreaterThan(1))
+        self.assertNotEqual("universe", originals[0].component.name)
+        overrides_1 = [
+            pub.changeOverride(new_component="universe") for pub in originals]
+        transaction.commit()
+        overrides_2 = [
+            pub.changeOverride(new_component="universe") for pub in originals]
+        for pub, dominant in zip(overrides_1 + originals, overrides_2 * 2):
+            pub.supersede(dominant, supersede_associated=False)
+        for pub, dominant in zip(overrides_1 + originals, overrides_2 * 2):
+            pub.supersedeAssociated(dominant, keep=overrides_2)
+        for original, override_1, override_2 in zip(
+                originals, overrides_1, overrides_2):
+            self.checkSuperseded([original, override_1], override_2)
+        self.checkPublications(overrides_2, PackagePublishingStatus.PENDING)
+
     def testSupersedesCorrespondingDDEB(self):
         """Check that supersede() takes with it any corresponding DDEB.