Merge lp:~wgrant/launchpad/archive-defer-deletion into lp:launchpad

[William Grant]

Defer PPA publication deletion to the publisher, remove NBS binaries, and allow files to expire.

This branch fixes three archive deletion bugs.

Firstly, it was marking all publications deleted in the webapp request, which is obviously inappropriate as it's an unbounded number of writes. It's also racy, as the archive is disabled in the same transaction so packages may end up successfully uploading slightly after the other publications are deleted. I moved the publication deletion phase to the publisher. Since we don't have the deletion requester at that point, they're now attributed to the janitor.

Secondly, PPA deletion left BPPHs around if there was no corresponding active SPPH (usually an NBS case). I fixed this by also deleting any remaining binaries.

Thirdly, while the files were removed from disk, dateremoved wasn't being set. This prevents any package published in a deleted archive from ever being expired. I adjusted deletion to also set dateremoved in this case, since we don't need to wait for p-d-r to remove the files.

[Steve Kowalik (stevenk)]

review: Approve (code)

=== modified file 'lib/lp/archivepublisher/publishing.py'
--- lib/lp/archivepublisher/publishing.py	2012-09-07 05:13:57 +0000
+++ lib/lp/archivepublisher/publishing.py	2013-02-20 02:37:30 +0000
@@ -20,7 +20,9 @@
     _multivalued,
     Release,
     )
+from zope.component import getUtility
 
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.archivepublisher import HARDCODED_COMPONENT_ORDER
 from lp.archivepublisher.config import getPubConfig
 from lp.archivepublisher.diskpool import DiskPool
@@ -40,6 +42,7 @@
     )
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
+from lp.services.database.constants import UTC_NOW
 from lp.services.database.sqlbase import sqlvalues
 from lp.services.librarian.client import LibrarianClient
 from lp.services.utils import file_exists
@@ -49,6 +52,10 @@
     BinaryPackageFormat,
     PackagePublishingStatus,
     )
+from lp.soyuz.interfaces.publishing import (
+    active_publishing_status,
+    IPublishingSet,
+    )
 from lp.soyuz.model.publishing import (
     BinaryPackagePublishingHistory,
     SourcePackagePublishingHistory,
@@ -747,6 +754,28 @@
             "Attempting to delete archive '%s/%s' at '%s'." % (
                 self.archive.owner.name, self.archive.name, root_dir))
 
+        # Set all the publications to DELETED.
+        sources = self.archive.getPublishedSources(
+            status=active_publishing_status)
+        getUtility(IPublishingSet).requestDeletion(
+            sources, removed_by=getUtility(ILaunchpadCelebrities).janitor,
+            removal_comment="Removed when deleting archive")
+
+        # Deleting the sources will have killed the corresponding
+        # binaries too, but there may be orphaned leftovers (eg. NBS).
+        binaries = self.archive.getAllPublishedBinaries(
+            status=active_publishing_status)
+        getUtility(IPublishingSet).requestDeletion(
+            binaries, removed_by=getUtility(ILaunchpadCelebrities).janitor,
+            removal_comment="Removed when deleting archive")
+
+        # Now set dateremoved on any publication that doesn't already
+        # have it set, so things can expire from the librarian.
+        for pub in self.archive.getPublishedSources(include_removed=False):
+            pub.dateremoved = UTC_NOW
+        for pub in self.archive.getAllPublishedBinaries(include_removed=False):
+            pub.dateremoved = UTC_NOW
+
         for directory in (root_dir, self._config.metaroot):
             if not os.path.exists(directory):
                 continue

=== modified file 'lib/lp/archivepublisher/tests/test_publisher.py'
--- lib/lp/archivepublisher/tests/test_publisher.py	2012-08-17 11:12:37 +0000
+++ lib/lp/archivepublisher/tests/test_publisher.py	2013-02-20 02:37:30 +0000
@@ -148,6 +148,24 @@
         test_archive = getUtility(IArchiveSet).new(
             distribution=self.ubuntutest, owner=ubuntu_team,
             purpose=ArchivePurpose.PPA)
+
+        # Create some source and binary publications, including an
+        # orphaned NBS binary.
+        spph = self.factory.makeSourcePackagePublishingHistory(
+            archive=test_archive)
+        bpph = self.factory.makeBinaryPackagePublishingHistory(
+            archive=test_archive)
+        orphaned_bpph = self.factory.makeBinaryPackagePublishingHistory(
+            archive=test_archive)
+        bpb = orphaned_bpph.binarypackagerelease.build
+        bpb.current_source_publication.supersede()
+        dead_spph = self.factory.makeSourcePackagePublishingHistory(
+            archive=test_archive)
+        dead_spph.supersede()
+        dead_bpph = self.factory.makeBinaryPackagePublishingHistory(
+            archive=test_archive)
+        dead_bpph.supersede()
+
         publisher = getPublisher(test_archive, None, self.logger)
 
         self.assertTrue(os.path.exists(publisher._config.archiveroot))
@@ -168,6 +186,20 @@
         self.assertEqual(ArchiveStatus.DELETED, test_archive.status)
         self.assertEqual(False, test_archive.publish)
 
+        # All of the archive's active publications have been marked
+        # DELETED, and dateremoved has been set early because they've
+        # already been removed from disk.
+        for pub in (spph, bpph, orphaned_bpph):
+            self.assertEqual(PackagePublishingStatus.DELETED, pub.status)
+            self.assertEqual(u'janitor', pub.removed_by.name)
+            self.assertIsNot(None, pub.dateremoved)
+
+        # The SUPERSEDED publications now have dateremoved set, even
+        # though p-d-r hasn't run over them.
+        for pub in (dead_spph, dead_bpph):
+            self.assertIs(None, pub.scheduleddeletiondate)
+            self.assertIsNot(None, pub.dateremoved)
+
         # Trying to delete it again won't fail, in the corner case where
         # some admin manually deleted the repo.
         publisher.deleteArchive()

=== modified file 'lib/lp/soyuz/doc/archive-deletion.txt'
--- lib/lp/soyuz/doc/archive-deletion.txt	2012-04-10 14:01:17 +0000
+++ lib/lp/soyuz/doc/archive-deletion.txt	2013-02-20 02:37:30 +0000
@@ -1,17 +1,13 @@
 = Deleting an archive =
 
 When deleting an archive, the user calls IArchive.delete(), passing in
-the IPerson who is requesting the deletion.
-
-All of the publishing records will be marked as DELETED, the archive is
-disabled and the status is set to DELETING.
-
-This status tells the publisher to then delete the repository area.  Once
-it completes that task it will set the status to DELETED.
-
-    >>> from lp.soyuz.enums import ArchiveStatus
-    >>> from lp.soyuz.interfaces.archive import (
-    ...     IArchiveSet, IArchive)
+the IPerson who is requesting the deletion. The archive is disabled and
+the status set to DELETING.
+
+This status tells the publisher to then set the publications to DELETED
+and delete the repository area. Once it completes that task the status
+of the archive itself is set to DELETED.
+
     >>> from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
     >>> login("[e-mail]")
     >>> stp = SoyuzTestPublisher()
@@ -55,26 +51,8 @@
     >>> ignored = login_person(archive.owner)
     >>> archive.delete(archive.owner)
 
-The deletion code uses a store.execute() command to speed up the operation
-where many records need to be updated.  Therefore we need to invalidate
-the cache to make Storm re-read the database objects.
-
-    >>> Store.of(archive).invalidate()
-
-Now, all the publications are DELETED, the archive is disabled and the
-status is DELETING to tell the publisher to remove the repository:
-
-    >>> publications = list(archive.getPublishedSources())
-    >>> publications.extend(list(archive.getAllPublishedBinaries()))
-    >>> for pub in publications:
-    ...     print "%s, %s by %s" % (
-    ...         pub.displayname, pub.status.name, pub.removed_by.name)
-    foo 666 in breezy-autotest, DELETED by archive-owner
-    foo 666 in breezy-autotest, DELETED by archive-owner
-    foo-bin1 666 in breezy-autotest i386, DELETED by archive-owner
-    foo-bin1 666 in breezy-autotest hppa, DELETED by archive-owner
-    foo-bin2 666 in breezy-autotest i386, DELETED by archive-owner
-    foo-bin2 666 in breezy-autotest hppa, DELETED by archive-owner
+Now the archive is disabled and the status is DELETING to tell the
+publisher to remove the publications and the repository:
 
     >>> print archive.enabled
     False

=== modified file 'lib/lp/soyuz/interfaces/publishing.py'
--- lib/lp/soyuz/interfaces/publishing.py	2013-01-31 02:02:37 +0000
+++ lib/lp/soyuz/interfaces/publishing.py	2013-02-20 02:37:30 +0000
@@ -1268,13 +1268,15 @@
         This is a supporting operation for a deletion request.
         """
 
-    def requestDeletion(sources, removed_by, removal_comment=None):
+    def requestDeletion(pub, removed_by, removal_comment=None):
         """Delete the source and binary publications specified.
 
         This method deletes the source publications passed via the first
-        parameter as well as their associated binary publications.
+        parameter as well as their associated binary publications, and any
+        binary publications passed in.
 
-        :param sources: list of `SourcePackagePublishingHistory` objects.
+        :param pubs: list of `SourcePackagePublishingHistory` and
+            `BinaryPackagePublishingHistory` objects.
         :param removed_by: `IPerson` responsible for the removal.
         :param removal_comment: optional text describing the removal reason.
 

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2013-02-06 10:40:16 +0000
+++ lib/lp/soyuz/model/archive.py	2013-02-20 02:37:30 +0000
@@ -493,7 +493,8 @@
     def getPublishedSources(self, name=None, version=None, status=None,
                             distroseries=None, pocket=None,
                             exact_match=False, created_since_date=None,
-                            eager_load=False, component_name=None):
+                            eager_load=False, component_name=None,
+                            include_removed=True):
         """See `IArchive`."""
         # clauses contains literal sql expressions for things that don't work
         # easily in storm : this method was migrated from sqlobject but some
@@ -560,6 +561,9 @@
                 SourcePackagePublishingHistory.datecreated >=
                     created_since_date)
 
+        if not include_removed:
+            clauses.append(SourcePackagePublishingHistory.dateremoved == None)
+
         store = Store.of(self)
         resultset = store.find(
             SourcePackagePublishingHistory, *clauses).order_by(*orderBy)
@@ -703,7 +707,7 @@
     def _getBinaryPublishingBaseClauses(
         self, name=None, version=None, status=None, distroarchseries=None,
         pocket=None, exact_match=False, created_since_date=None,
-        ordered=True):
+        ordered=True, include_removed=True):
         """Base clauses and clauseTables for binary publishing queries.
 
         Returns a list of 'clauses' (to be joined in the callsite) and
@@ -781,17 +785,22 @@
                 "BinaryPackagePublishingHistory.datecreated >= %s"
                 % sqlvalues(created_since_date))
 
+        if not include_removed:
+            clauses.append(
+                "BinaryPackagePublishingHistory.dateremoved IS NULL")
+
         return clauses, clauseTables, orderBy
 
     def getAllPublishedBinaries(self, name=None, version=None, status=None,
                                 distroarchseries=None, pocket=None,
                                 exact_match=False, created_since_date=None,
-                                ordered=True):
+                                ordered=True, include_removed=True):
         """See `IArchive`."""
         clauses, clauseTables, orderBy = self._getBinaryPublishingBaseClauses(
             name=name, version=version, status=status, pocket=pocket,
             distroarchseries=distroarchseries, exact_match=exact_match,
-            created_since_date=created_since_date, ordered=ordered)
+            created_since_date=created_since_date, ordered=ordered,
+            include_removed=include_removed)
 
         all_binaries = BinaryPackagePublishingHistory.select(
             ' AND '.join(clauses), clauseTables=clauseTables,
@@ -1971,12 +1980,6 @@
             ArchiveStatus.DELETING, ArchiveStatus.DELETED,
             "This archive is already deleted.")
 
-        # Set all the publications to DELETED.
-        sources = self.getPublishedSources(status=active_publishing_status)
-        getUtility(IPublishingSet).requestDeletion(
-            sources, removed_by=deleted_by,
-            removal_comment="Removed when deleting archive")
-
         # Mark the archive's status as DELETING so the repository can be
         # removed by the publisher.
         self.status = ArchiveStatus.DELETING

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2013-02-06 07:25:12 +0000
+++ lib/lp/soyuz/model/publishing.py	2013-02-20 02:37:30 +0000
@@ -1992,11 +1992,18 @@
             removed_byID=removed_by_id,
             removal_comment=removal_comment)
 
-    def requestDeletion(self, sources, removed_by, removal_comment=None):
+    def requestDeletion(self, pubs, removed_by, removal_comment=None):
         """See `IPublishingSet`."""
-        sources = list(sources)
-        if len(sources) == 0:
+        pubs = list(pubs)
+        sources = [
+            pub for pub in pubs
+            if ISourcePackagePublishingHistory.providedBy(pub)]
+        binaries = [
+            pub for pub in pubs
+            if IBinaryPackagePublishingHistory.providedBy(pub)]
+        if not sources and not binaries:
             return
+        assert len(sources) + len(binaries) == len(pubs)
 
         spph_ids = [spph.id for spph in sources]
         self.setMultipleDeleted(
@@ -2005,11 +2012,12 @@
 
         getUtility(IDistroSeriesDifferenceJobSource).createForSPPHs(sources)
 
-        # Mark binary publications deleted.
-        bpph_ids = [
-            bpph.id
-            for source, bpph, bin, bin_name, arch
-                in self.getBinaryPublicationsForSources(sources)]
+        # Append the sources' related binaries to our condemned list,
+        # and mark them all deleted.
+        bpph_ids = [bpph.id for bpph in binaries]
+        bpph_ids.extend(
+            bpph.id for source, bpph, bin, bin_name, arch
+            in self.getBinaryPublicationsForSources(sources))
         if len(bpph_ids) > 0:
             self.setMultipleDeleted(
                 BinaryPackagePublishingHistory, bpph_ids, removed_by,

=== modified file 'lib/lp/soyuz/tests/test_publishing.py'
--- lib/lp/soyuz/tests/test_publishing.py	2013-02-14 01:10:48 +0000
+++ lib/lp/soyuz/tests/test_publishing.py	2013-02-20 02:37:30 +0000
@@ -1184,6 +1184,12 @@
             [other_spph], self.factory.makePerson())
         self.assertEqual(PackagePublishingStatus.PENDING, spph.status)
 
+    def test_requestDeletion_marks_BPPHs_deleted(self):
+        bpph = self.factory.makeBinaryPackagePublishingHistory()
+        getUtility(IPublishingSet).requestDeletion(
+            [bpph], self.factory.makePerson())
+        self.assertEqual(PackagePublishingStatus.DELETED, bpph.status)
+
     def test_requestDeletion_marks_attached_BPPHs_deleted(self):
         bpph = self.factory.makeBinaryPackagePublishingHistory()
         spph = self.factory.makeSPPHForBPPH(bpph)