launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #15186
[Merge] lp:~wgrant/launchpad/archive-defer-deletion into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/archive-defer-deletion into lp:launchpad.
Commit message:
Defer PPA publication deletion to the publisher, remove NBS binaries, and allow files to expire.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #685076 in Launchpad itself: "PPA deletion leaves unremoved publications"
https://bugs.launchpad.net/launchpad/+bug/685076
Bug #733803 in Launchpad itself: "Archive:+delete timeout deleting PPA"
https://bugs.launchpad.net/launchpad/+bug/733803
Bug #879208 in Launchpad itself: "deleted PPA has binary build records"
https://bugs.launchpad.net/launchpad/+bug/879208
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/archive-defer-deletion/+merge/148988
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.
--
https://code.launchpad.net/~wgrant/launchpad/archive-defer-deletion/+merge/148988
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/archive-defer-deletion into lp:launchpad.
=== 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-18 06:34:25 +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
@@ -49,6 +51,10 @@
BinaryPackageFormat,
PackagePublishingStatus,
)
+from lp.soyuz.interfaces.publishing import (
+ active_publishing_status,
+ IPublishingSet,
+ )
from lp.soyuz.model.publishing import (
BinaryPackagePublishingHistory,
SourcePackagePublishingHistory,
@@ -747,6 +753,23 @@
"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",
+ already_removed=True)
+
+ # 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",
+ already_removed=True)
+
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-18 06:34:25 +0000
@@ -148,6 +148,18 @@
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()
+
publisher = getPublisher(test_archive, None, self.logger)
self.assertTrue(os.path.exists(publisher._config.archiveroot))
@@ -168,6 +180,14 @@
self.assertEqual(ArchiveStatus.DELETED, test_archive.status)
self.assertEqual(False, test_archive.publish)
+ # All of the archive's 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)
+
# 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-18 06:34:25 +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("admin@xxxxxxxxxxxxx")
>>> 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-18 06:34:25 +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-18 06:34:25 +0000
@@ -1971,12 +1971,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-18 06:34:25 +0000
@@ -1967,7 +1967,7 @@
source_publication.archive)[source_id]
def setMultipleDeleted(self, publication_class, ids, removed_by,
- removal_comment=None):
+ removal_comment=None, already_removed=False):
"""Mark multiple publication records as deleted."""
ids = list(ids)
if len(ids) == 0:
@@ -1991,29 +1991,41 @@
datesuperseded=UTC_NOW,
removed_byID=removed_by_id,
removal_comment=removal_comment)
+ if already_removed:
+ affected_pubs.set(dateremoved=UTC_NOW)
- def requestDeletion(self, sources, removed_by, removal_comment=None):
+ def requestDeletion(self, pubs, removed_by, removal_comment=None,
+ already_removed=False):
"""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(
SourcePackagePublishingHistory, spph_ids, removed_by,
- removal_comment=removal_comment)
+ removal_comment=removal_comment, already_removed=already_removed)
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,
- removal_comment=removal_comment)
+ removal_comment=removal_comment,
+ already_removed=already_removed)
def getNearestAncestor(
self, package_name, archive, distroseries, pocket=None,
=== 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-18 06:34:25 +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)
@@ -1204,6 +1210,16 @@
# The test is that this does not fail.
Store.of(person).flush()
+ def test_requestDeletion_already_removed(self):
+ person = self.factory.makePerson()
+ spph = self.factory.makeSourcePackagePublishingHistory()
+ getUtility(IPublishingSet).requestDeletion([spph], person)
+ self.assertEqual(PackagePublishingStatus.DELETED, spph.status)
+ self.assertIs(None, spph.dateremoved)
+ getUtility(IPublishingSet).requestDeletion(
+ [spph], person, already_removed=True)
+ self.assertIsNot(None, spph.dateremoved)
+
def test_requestDeletion_creates_DistroSeriesDifferenceJobs(self):
dsp = self.factory.makeDistroSeriesParent()
series = dsp.derived_series
