Merge lp:~jtv/launchpad/bug-844550 into lp:launchpad

[Jeroen T. Vermeulen]

Replace transitional Gina domination with its permanent form.

= Summary =

We're rolling out “transitional Gina domination.” This means that Gina will perform source domination on the imported Debian packages: publications of package versions that are no longer in Debian's Sources lists will become Superseded by their newer versions. Where the very newest versions are deleted from the Sources lists for whatever reason, the publication records in our database will be marked Deleted. (Also, Debian source publications are no longer nonsensically kept Pending; they will be Published immediately upon import.)

But what happens when a package no longer occurs in the Debian Sources list at all? We have tons of these still lying around for old packages. For the historical cases, we'll assume that the last known release for a package superseded all others. So the last known release should end up Deleted and all its older ones Superseded. In the future, when a package disappears from Debian's Sources list, its remaining active publications should be considered Deleted.

To make a smooth transition, transitional domination does the one-off jobs: mark any remaining Pending publications for Debian as Published (the bulk of the work was done in custom SQL), and for any package that isn't in the Sources lists, pick the latest version as “live.” That last version stays Published; the older ones become Deleted.

And then there's this branch. Once transitional domination has run, disposing of all the historical data, we can start marking publications for packages that aren't in the Sources at all as Deleted. That includes, for the historical data, that one last release that transitional domination had kept Published.

== Pre-implementation notes ==

Discussed in great detail with wgrant. Unfortunately the transition to making Debian source package publications Published (rather than Pending) broke some scripts the Ubuntu people were running, which explicitly looked for the doubtful Pending status. No multi-status filtering is available, so the easiest fix was to steam ahead to the more desirable situation and assume that all new publications are Published instead.

== Implementation details ==

Mostly this branch consists of removals of clearly marked transitional code. Things get a bit simpler, as planned. But I extended a test for the gina code, to cover a more complete scenario. There is already an extensive scenario test for the underlying functionality, but the new test code shows in one place how permanent domination behaves over a realistic "chain" of package publications.

== Tests ==

{{{
./bin/test -vvc lp.soyuz.scripts.tests.test_gina
}}}

== Demo and Q/A ==

We've found we can run Gina on dogfood's copy of sid to compare:

    ./scripts/gina.py -v sid

This closely matches what will happen in production, since we also ran transitional Gina domination there. The only slightly unusual thing there is that the Sources list we have on dogfood is a bit older than the publications we have in the database, so some publications are marked Deleted because they're newer than any listed ones.

A useful thing to do before running domination is to copy all (id, status) from SourcePackagePublishingHistory into a table of its own. We currently have such a snapshot from before we ran transitional domination on dogfood.

Then, to get an overview of all changes made by gina:

    http://paste.ubuntu.com/687616/

Results after setting all SPPHs for archive 3 (Debian primary) to Published, for a really thorough domination run:

    http://paste.ubuntu.com/687584/

With this new code, a portion of those 18668 Published records should become Deleted. These should be one per SourcePackageName, and all for source packages that are no longer in the Sources list we have in /src/launchpad.net/gina-mirror/dists/sid/main/source/Sources.gz, or possibly that are in there but only with versions that we don't have.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/scripts/gina/dominate.py
  lib/lp/soyuz/scripts/tests/test_gina.py

[Graham Binns (gmb)]

Hi Jeroen,

Excellent branch this; I don't have any questions other than:

112 + self.assertEqual([
113 + PackagePublishingStatus.SUPERSEDED,
114 + PackagePublishingStatus.SUPERSEDED,
115 + PackagePublishingStatus.PUBLISHED,
116 + PackagePublishingStatus.DELETED,
117 + PackagePublishingStatus.PENDING,
118 + ],
119 + [pub.status for pub in spphs])

This might be more comprehensible if it were written as single statements (though I can appreciate that that might be a bit less elegant), purely from a "don't make me think about it" point of view. Also, if it's a series of single asserts, it's likely to be simpler to unwind when something goes wrong.

review: Approve (code)

[Jeroen T. Vermeulen (jtv)]

(Marking this Work In Progress to keep it off the review list for now, and to stop me and others from landing it by accident. Transitional Gina domination needs to run on production before this can land.)

[Jeroen T. Vermeulen (jtv)]

Graham, you raise a difficult point. I went with the entire-list-at-once assertion for exactly the considerations you give. I'll elaborate.

When an early assertion in a series of them fails, the first thing I always want to know is “yeah but what did it do in the other cases, that are all similar except in one important respect?” For instance, did all publications just get the same status for some reason, or are they put in the wrong order, or is it just one case that's off, or was nothing updated at all, or is something else going on? Often I find myself re-running tests in the debugger, or with the failing assertion commented out, or with reordered assertions just to find out. Hence the choice of an assertion that shows the full pattern I expected and the full pattern that was observed. The alternative would be a series of similar tests, but in this case I particularly wanted to give a single full panorama view on what happens to a typical string of publications.

Ideally, I'd like to annotate the items in the assertion with what's unique about how each of them went through the process, and why I expect them to end up in the given way. In most cases that can be done with a bit of case-by-case creativity.

In this case, I can think of two ways of doing that:

1. Assert the “before action” state in a similar way.

2. Compare not lists, but dicts that map versions to statuses. So it'll read as “1.0 is Superseded,” etc.

I could do either one of these, or both. For brevity, one component of clarity, I'd prefer to limit it to one.

=== modified file 'lib/lp/soyuz/scripts/gina/dominate.py'
--- lib/lp/soyuz/scripts/gina/dominate.py	2011-09-16 11:41:14 +0000
+++ lib/lp/soyuz/scripts/gina/dominate.py	2011-09-21 08:14:25 +0000
@@ -20,86 +20,14 @@
     series = getUtility(IDistributionSet)[distro_name].getSeries(series_name)
     dominator = Dominator(logger, series.main_archive)
 
-    # XXX JeroenVermeulen 2011-09-08, bug=844550: This is a transitional
-    # hack.  Gina used to create SPPHs in Pending state.  We cleaned up
-    # the bulk of them, and changed the code to create Published ones, but
-    # some new ones will have been created since.
-    # Update those to match what the new Gina does.
-    from canonical.launchpad.interfaces.lpstorm import IStore
-    from lp.soyuz.enums import PackagePublishingStatus
-    from lp.soyuz.model.publishing import SourcePackagePublishingHistory
-    SPPH = SourcePackagePublishingHistory
-    store = IStore(SPPH)
-    spphs = store.find(
-        SPPH,
-        SPPH.archive == series.main_archive,
-        SPPH.distroseries == series,
-        SPPH.pocket == pocket,
-        SPPH.status == PackagePublishingStatus.PENDING)
-    spphs.set(status=PackagePublishingStatus.PUBLISHED)
-
-    # Dominate packages found in the Sources list we're importing.
+    # Dominate all packages published in the series.  This includes all
+    # packages listed in the Sources file we imported, but also packages
+    # that have been recently deleted.
     package_names = dominator.findPublishedSourcePackageNames(series, pocket)
     for package_name in package_names:
-        entries = packages_map.src_map.get(package_name)
-
-        if entries is None:
-            # XXX JeroenVermeulen 2011-09-08, bug=844550: This is a
-            # transitional hack.  The database is full of "Published"
-            # Debian SPPHs whose packages have actually been deleted.
-            # In the future such publications should simply be marked
-            # Deleted, but for the legacy baggage we currently carry
-            # around we'll just do traditional domination first: pick
-            # the latest Published version, and mark the rest of the
-            # SPPHs as superseded by that version.  The latest version
-            # will then, finally, be marked appropriately Deleted once
-            # we remove this transitional hack.
-            # To remove the transitional hack, just let live_versions
-            # default to the empty list instead of doing this:
-            import apt_pkg
-            from lp.services.database.bulk import load_related
-            from lp.soyuz.model.sourcepackagerelease import (
-                SourcePackageRelease,
-                )
-            pubs = list(
-                dominator.findPublishedSPPHs(series, pocket, package_name))
-            if len(pubs) <= 1:
-                # Without at least two published SPPHs, the transitional
-                # code will make no changes.  Skip, and leave the
-                # algorithm free to assume there's a pubs[0].
-                continue
-            load_related(
-                SourcePackageRelease, pubs, ['sourcepackagereleaseID'])
-
-            # Close off the transaction to avoid being idle-killed.
-            # Nothing else is going to supersede or delete these
-            # publications in the meantime, so our data stays valid; and
-            # if new ones become published, they still won't be
-            # considered anyway.
-            txn.commit()
-
-            # Find the "latest" publication.  A purely in-memory
-            # operation; won't open a new transaction.
-            def is_newer(candidate, reference):
-                comparison = apt_pkg.VersionCompare(
-                    candidate.sourcepackagerelease.version,
-                    reference.sourcepackagerelease.version)
-                if comparison > 0:
-                    return True
-                elif comparison < 0:
-                    return False
-                else:
-                    return candidate.datecreated > reference.datecreated
-
-            latest_pub = pubs[0]
-            for pub in pubs[1:]:
-                if is_newer(pub, latest_pub):
-                    latest_pub = pub
-            live_versions = [latest_pub.sourcepackagerelease.version]
-        else:
-            live_versions = [
-                entry['Version']
-                for entry in entries if 'Version' in entry]
+        entries = packages_map.src_map.get(package_name, [])
+        live_versions = [
+            entry['Version'] for entry in entries if 'Version' in entry]
 
         dominator.dominateRemovedSourceVersions(
             series, pocket, package_name, live_versions)

=== modified file 'lib/lp/soyuz/scripts/tests/test_gina.py'
--- lib/lp/soyuz/scripts/tests/test_gina.py	2011-09-16 10:48:59 +0000
+++ lib/lp/soyuz/scripts/tests/test_gina.py	2011-09-21 08:14:25 +0000
@@ -36,15 +36,51 @@
         # packages that Gina imports.
         logger = DevNullLogger()
         txn = FakeTransaction()
-        pub = self.factory.makeSourcePackagePublishingHistory(
-            status=PackagePublishingStatus.PUBLISHED)
-        series = pub.distroseries
-        spr = pub.sourcepackagerelease
-        package = spr.sourcepackagename
+        series = self.factory.makeDistroSeries()
+        pocket = PackagePublishingPocket.RELEASE
+        package = self.factory.makeSourcePackageName()
+
+        # Realistic situation: there's an older, superseded publication;
+        # a series of active ones; and a newer, pending publication
+        # that's not in the Sources lists yet.
+        # Gina dominates the Published ones and leaves the rest alone.
+        old_spph = self.factory.makeSourcePackagePublishingHistory(
+            distroseries=series, archive=series.main_archive,
+            pocket=pocket, status=PackagePublishingStatus.SUPERSEDED,
+            sourcepackagerelease=self.factory.makeSourcePackageRelease(
+                sourcepackagename=package, version='1.0'))
+
+        active_spphs = [
+            self.factory.makeSourcePackagePublishingHistory(
+                distroseries=series, archive=series.main_archive,
+                pocket=pocket, status=PackagePublishingStatus.PUBLISHED,
+                sourcepackagerelease=self.factory.makeSourcePackageRelease(
+                    sourcepackagename=package, version=version))
+            for version in ['1.1', '1.1.1', '1.1.1.1']]
+
+        new_spph = self.factory.makeSourcePackagePublishingHistory(
+            distroseries=series, archive=series.main_archive,
+            pocket=pocket, status=PackagePublishingStatus.PENDING,
+            sourcepackagerelease=self.factory.makeSourcePackageRelease(
+                sourcepackagename=package, version='1.2'))
+
+        spphs = [old_spph] + active_spphs + [new_spph]
+
+        # Of the active publications, in this scenario, only one version
+        # matches what Gina finds in the Sources list.  It stays
+        # published; older active publications are superseded, newer
+        # ones deleted.
         dominate_imported_source_packages(
-            txn, logger, series.distribution.name, series.name, pub.pocket,
-            FakePackagesMap({package.name: []}))
-        self.assertEqual(PackagePublishingStatus.DELETED, pub.status)
+            txn, logger, series.distribution.name, series.name, pocket,
+            FakePackagesMap({package.name: [{'Version': '1.1.1'}]}))
+        self.assertEqual([
+            PackagePublishingStatus.SUPERSEDED,
+            PackagePublishingStatus.SUPERSEDED,
+            PackagePublishingStatus.PUBLISHED,
+            PackagePublishingStatus.DELETED,
+            PackagePublishingStatus.PENDING,
+            ],
+            [pub.status for pub in spphs])
 
     def test_dominate_imported_source_packages_dominates_deletions(self):
         # dominate_imported_source_packages dominates the source
@@ -60,81 +96,25 @@
                 sourcepackagerelease=self.factory.makeSourcePackageRelease(
                     sourcepackagename=package, version=version))
             for version in ['1.0', '1.1', '1.1a']]
+
+        # In this scenario, 1.0 is a superseded release.
+        pubs[0].supersede()
         logger = DevNullLogger()
         txn = FakeTransaction()
         dominate_imported_source_packages(
             txn, logger, series.distribution.name, series.name, pocket,
             FakePackagesMap({}))
-        # XXX JeroenVermeulen 2011-09-08, bug=844550: This is
-        # "transitional" domination which supersedes older versions of
-        # deleted packages with the last known version.  Permanent
-        # domination will then mark the last known version as deleted.
-        # For permanent domination, the expected outcome is that all
-        # these publications will be Deleted (but any pre-existing
-        # Superseded publications for older versions will remain
-        # Superseded).
+
+        # The older, superseded release stays superseded; but the
+        # releases that dropped out of the imported Sources list without
+        # known successors are marked deleted.
         self.assertEqual([
             PackagePublishingStatus.SUPERSEDED,
-            PackagePublishingStatus.SUPERSEDED,
-            PackagePublishingStatus.PUBLISHED,
+            PackagePublishingStatus.DELETED,
+            PackagePublishingStatus.DELETED,
             ],
             [pub.status for pub in pubs])
 
-    def test_dominate_imported_source_packages_cleans_up_pending_spphs(self):
-        # XXX JeroenVermeulen 2011-09-08, bug=844550: For transition to
-        # Gina domination, dominate_imported_source_packages turns any
-        # remaining Pending SPPHS into Published ones.
-        series = self.factory.makeDistroSeries()
-        spph = self.factory.makeSourcePackagePublishingHistory(
-            distroseries=series, archive=series.main_archive,
-            status=PackagePublishingStatus.PENDING)
-        spr = spph.sourcepackagerelease
-        package_name = spr.sourcepackagename.name
-        logger = DevNullLogger()
-        txn = FakeTransaction()
-        dominate_imported_source_packages(
-            txn, logger, series.distribution.name, series.name, spph.pocket,
-            FakePackagesMap({package_name: [{"Version": spr.version}]}))
-        self.assertEqual(PackagePublishingStatus.PUBLISHED, spph.status)
-
-    def test_dominate_imported_source_packages_cleans_up_first(self):
-        # XXX JeroenVermeulen 2011-09-08, bug=844550: For transition to
-        # Gina domination, dominate_imported_source_packages turns any
-        # remaining Pending SPPHS into Published ones.  It does this
-        # *before* dominating, so no domination happens while some of
-        # the SPPHs are still mistakenly Pending (which would result in
-        # mistaken deletions).
-        series = self.factory.makeDistroSeries()
-        package = self.factory.makeSourcePackageName()
-        pocket = PackagePublishingPocket.RELEASE
-        versions = ['1.0', '1.1']
-        statuses_before = [
-            PackagePublishingStatus.PUBLISHED,
-            PackagePublishingStatus.PENDING,
-            ]
-        statuses_after = [
-            PackagePublishingStatus.SUPERSEDED,
-            PackagePublishingStatus.PUBLISHED,
-            ]
-        live_version = versions[-1]
-        sprs = [
-            self.factory.makeSourcePackageRelease(
-                sourcepackagename=package, version=version)
-            for version in versions]
-        spphs = [
-            self.factory.makeSourcePackagePublishingHistory(
-                archive=series.main_archive, distroseries=series,
-                sourcepackagerelease=spr, pocket=pocket, status=status)
-            for spr, status in zip(sprs, statuses_before)]
-
-        logger = DevNullLogger()
-        txn = FakeTransaction()
-        dominate_imported_source_packages(
-            txn, logger, series.distribution.name, series.name, pocket,
-            FakePackagesMap({package.name: [{"Version": live_version}]}))
-
-        self.assertEqual(statuses_after, [spph.status for spph in spphs])
-
 
 class TestSourcePackagePublisher(TestCaseWithFactory):