launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #15312
[Merge] lp:~cjwatson/launchpad/copy-delete-overrides-race into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/copy-delete-overrides-race into lp:launchpad.
Commit message:
Respect overrides from deleted publications when copying.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1152149 in Launchpad itself: "Combining async copying and deletion loses overrides"
https://bugs.launchpad.net/launchpad/+bug/1152149
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/copy-delete-overrides-race/+merge/152243
== Summary ==
Bug 1152149 describes a race between asynchronous copies and deletions which sometimes causes overrides applied in raring-proposed to be lost upon copy to raring. As one of the most frequent causes of Ubuntu image build failures at the moment, we'd love to fix this.
== Proposed fix ==
Add DELETED publications to the list of those examined for overrides, so that if a DELETED publication is the newest present then we'll use its overrides.
== Implementation details ==
There are two users of FromExistingOverridePolicy. UbuntuOverridePolicy (and all its callers) should be fine; that's the very thing we're trying to change. However, PlainPackageCopyJob._checkPolicies actually cares about whether a package is NEW or not, and if it's been deleted then it does still need to go through NEW again (it might not even share any history with the previous package). I therefore put this behind an include_deleted=False check, which UbuntuOverridePolicy sets to True, and added a test for appropriate NEW handling.
== Tests ==
bin/test -vvct TestDoDirectCopy -t PlainPackageCopyJobTests
== Demo and Q/A ==
On dogfood, upload a package to raring-proposed and apply non-default overrides to it (this can be a copy from somewhere else, to avoid having to dig up builders). Use copyPackage to request an asynchronous copy to raring. Delete the publication in raring-proposed. Run the PCJ queue processor. Check that the overrides in raring match those previously set manually.
--
https://code.launchpad.net/~cjwatson/launchpad/copy-delete-overrides-race/+merge/152243
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/copy-delete-overrides-race into lp:launchpad.
=== modified file 'lib/lp/soyuz/adapters/overrides.py'
--- lib/lp/soyuz/adapters/overrides.py 2012-10-30 01:46:10 +0000
+++ lib/lp/soyuz/adapters/overrides.py 2013-03-07 17:50:51 +0000
@@ -1,8 +1,7 @@
-# Copyright 2011 Canonical Ltd. This software is licensed under the
+# Copyright 2011-2013 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
-"""Generic Override Policy classes.
-"""
+"""Generic Override Policy classes."""
__metaclass__ = type
@@ -33,8 +32,8 @@
from lp.services.database import bulk
from lp.services.database.decoratedresultset import DecoratedResultSet
from lp.services.database.lpstorm import IStore
+from lp.soyuz.enums import PackagePublishingStatus
from lp.soyuz.interfaces.component import IComponentSet
-from lp.soyuz.interfaces.publishing import active_publishing_status
from lp.soyuz.model.binarypackagename import BinaryPackageName
from lp.soyuz.model.component import Component
from lp.soyuz.model.distroarchseries import DistroArchSeries
@@ -192,9 +191,17 @@
for the latest published binary publication.
"""
+ def existing_publishing_status(self, include_deleted):
+ status = [
+ PackagePublishingStatus.PENDING,
+ PackagePublishingStatus.PUBLISHED,
+ ]
+ if include_deleted:
+ status.append(PackagePublishingStatus.DELETED)
+ return status
+
def calculateSourceOverrides(self, archive, distroseries, pocket, spns,
- source_component=None):
-
+ source_component=None, include_deleted=False):
def eager_load(rows):
bulk.load(Component, (row[1] for row in rows))
bulk.load(Section, (row[2] for row in rows))
@@ -209,7 +216,7 @@
SourcePackagePublishingHistory.distroseriesID ==
distroseries.id,
SourcePackagePublishingHistory.status.is_in(
- active_publishing_status),
+ self.existing_publishing_status(include_deleted)),
SourcePackagePublishingHistory.sourcepackagenameID.is_in(
spn.id for spn in spns)).order_by(
SourcePackagePublishingHistory.sourcepackagenameID,
@@ -225,7 +232,7 @@
for (name, component, section) in already_published]
def calculateBinaryOverrides(self, archive, distroseries, pocket,
- binaries):
+ binaries, include_deleted=False):
def eager_load(rows):
bulk.load(Component, (row[2] for row in rows))
bulk.load(Section, (row[3] for row in rows))
@@ -246,7 +253,7 @@
BinaryPackagePublishingHistory.sectionID,
BinaryPackagePublishingHistory.priority),
BinaryPackagePublishingHistory.status.is_in(
- active_publishing_status),
+ self.existing_publishing_status(include_deleted)),
Or(*candidates)).order_by(
BinaryPackagePublishingHistory.distroarchseriesID,
BinaryPackagePublishingHistory.binarypackagenameID,
@@ -333,13 +340,13 @@
sources, source_component=None):
total = set(sources)
overrides = FromExistingOverridePolicy.calculateSourceOverrides(
- self, archive, distroseries, pocket, sources, source_component)
+ self, archive, distroseries, pocket, sources, source_component,
+ include_deleted=True)
existing = set(override.source_package_name for override in overrides)
missing = total.difference(existing)
if missing:
unknown = UnknownOverridePolicy.calculateSourceOverrides(
- self, archive, distroseries, pocket, missing,
- source_component)
+ self, archive, distroseries, pocket, missing, source_component)
overrides.extend(unknown)
return overrides
@@ -347,7 +354,8 @@
binaries):
total = set(binaries)
overrides = FromExistingOverridePolicy.calculateBinaryOverrides(
- self, archive, distroseries, pocket, binaries)
+ self, archive, distroseries, pocket, binaries,
+ include_deleted=True)
existing = set(
(
override.binary_package_name,
=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py 2012-11-13 09:45:19 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py 2013-03-07 17:50:51 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2013 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
=== modified file 'lib/lp/soyuz/scripts/tests/test_copypackage.py'
--- lib/lp/soyuz/scripts/tests/test_copypackage.py 2013-01-22 02:06:59 +0000
+++ lib/lp/soyuz/scripts/tests/test_copypackage.py 2013-03-07 17:50:51 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
@@ -143,12 +143,7 @@
# Create a brand new PPA.
archive = self.factory.makeArchive(
distribution=self.test_publisher.ubuntutest,
- purpose=ArchivePurpose.PPA)
-
- # Make it private if necessary.
- if private:
- archive.buildd_secret = 'x'
- archive.private = True
+ purpose=ArchivePurpose.PPA, private=private)
# Create a testing source publication with binaries
source = self.test_publisher.getPubSource(archive=archive)
@@ -183,7 +178,7 @@
self.assertChangedFiles([], changed_files)
self.assertSourceFilesArePrivate(private_source, 3)
- # Copy The original source to a public PPA, at this point all
+ # Copy the original source to a public PPA. At this point all
# files related to it will remain private.
public_archive = self.factory.makeArchive(
distribution=self.test_publisher.ubuntutest,
@@ -258,7 +253,7 @@
self.assertChangedFiles([], changed_files)
self.assertBinaryFilesArePrivate(private_binary, 3)
- # Copy The original binary to a public PPA, at this point all
+ # Copy the original binary to a public PPA. At this point all
# files related to it will remain private.
public_archive = self.factory.makeArchive(
distribution=self.test_publisher.ubuntutest,
@@ -295,12 +290,10 @@
public_binary = public_source.getPublishedBinaries()[0]
self.layer.commit()
- # Copy The original source and binaries to a private PPA.
+ # Copy the original source and binaries to a private PPA.
private_archive = self.factory.makeArchive(
distribution=self.test_publisher.ubuntutest,
- purpose=ArchivePurpose.PPA)
- private_archive.buildd_secret = 'x'
- private_archive.private = True
+ purpose=ArchivePurpose.PPA, private=True)
copied_source = public_source.copyTo(
public_source.distroseries, public_source.pocket,
@@ -407,8 +400,7 @@
def test_cannot_copy_binaries_from_building(self):
[build] = self.source.createMissingBuilds()
- self.assertCannotCopyBinaries(
- 'source has no binaries to be copied')
+ self.assertCannotCopyBinaries('source has no binaries to be copied')
def test_cannot_copy_check_perm_no_person(self):
# If check_permissions=True and person=None is passed to
@@ -441,8 +433,7 @@
def test_can_copy_binaries_from_fullybuilt_and_published(self):
self.test_publisher.getPubBinaries(
- pub_source=self.source,
- status=PackagePublishingStatus.PUBLISHED)
+ pub_source=self.source, status=PackagePublishingStatus.PUBLISHED)
self.assertCanCopyBinaries()
@@ -576,8 +567,7 @@
def test_cannot_copy_only_source_from_fullybuilt_and_published(self):
self.test_publisher.getPubBinaries(
- pub_source=self.source,
- status=PackagePublishingStatus.PUBLISHED)
+ pub_source=self.source, status=PackagePublishingStatus.PUBLISHED)
self.assertCannotCopySourceOnly(
'same version already has published binaries in the '
'destination archive')
@@ -648,20 +638,16 @@
def test_can_copy_only_source_from_fullybuilt_and_published(self):
self.test_publisher.getPubBinaries(
- pub_source=self.source,
- status=PackagePublishingStatus.PUBLISHED)
+ pub_source=self.source, status=PackagePublishingStatus.PUBLISHED)
self.assertCanCopySourceOnly()
def switchToAPrivateSource(self):
"""Override the probing source with a private one."""
private_archive = self.factory.makeArchive(
distribution=self.test_publisher.ubuntutest,
- purpose=ArchivePurpose.PPA)
- private_archive.buildd_secret = 'x'
- private_archive.private = True
+ purpose=ArchivePurpose.PPA, private=True)
- self.source = self.test_publisher.getPubSource(
- archive=private_archive)
+ self.source = self.test_publisher.getPubSource(archive=private_archive)
def test_can_copy_only_source_from_private_archives(self):
# Source-only copies from private archives to public ones
@@ -693,8 +679,7 @@
self.source = self.test_publisher.getPubSource(archive=ppa)
self.test_publisher.getPubBinaries(
pub_source=self.source, with_debug=True)
- self.assertCannotCopyBinaries(
- 'Cannot copy DDEBs to a primary archive')
+ self.assertCannotCopyBinaries('Cannot copy DDEBs to a primary archive')
def test_cannot_copy_source_twice(self):
# checkCopy refuses to copy the same source twice. Duplicates are
@@ -831,8 +816,7 @@
spr.addFile(new_tar)
# Set previous source tarball to be expired.
with dbuser('librarian'):
- naked_prev_tar = removeSecurityProxy(prev_tar)
- naked_prev_tar.content = None
+ removeSecurityProxy(prev_tar).content = None
self.assertCanCopySourceOnly()
@@ -998,10 +982,8 @@
# the copy batch.
# Create a source with binaries in ubuntutest/breezy-autotest.
- source = self.test_publisher.getPubSource(
- architecturehintlist='i386')
- binary = self.test_publisher.getPubBinaries(
- pub_source=source)[0]
+ source = self.test_publisher.getPubSource(architecturehintlist='i386')
+ binary = self.test_publisher.getPubBinaries(pub_source=source)[0]
# Copy it with binaries to ubuntutest/hoary-test.
hoary = self.test_publisher.ubuntutest.getSeries('hoary-test')
@@ -1048,8 +1030,7 @@
for arch in archs:
pf = self.factory.makeProcessorFamily(name='my_%s' % arch)
self.factory.makeDistroArchSeries(
- distroseries=nobby, architecturetag=arch,
- processorfamily=pf)
+ distroseries=nobby, architecturetag=arch, processorfamily=pf)
nobby.nominatedarchindep = nobby[archs[0]]
self.test_publisher.addFakeChroots(nobby)
return nobby
@@ -1221,8 +1202,7 @@
# The copy succeeds, and no i386 publication is created.
self.assertCopied(copies, nobby, ('hppa',))
- def assertComponentSectionAndPriority(self, component, source,
- destination):
+ def assertOverrides(self, component, source, destination):
self.assertEqual(component, destination.component)
self.assertEqual(source.section, destination.section)
self.assertEqual(source.priority, destination.priority)
@@ -1253,10 +1233,8 @@
source, target_archive, nobby, source.pocket, True)
universe = getUtility(IComponentSet)['universe']
self.assertEqual(universe, copied_source.component)
- self.assertComponentSectionAndPriority(
- universe, bin_i386, copied_bin_i386)
- self.assertComponentSectionAndPriority(
- universe, bin_hppa, copied_bin_hppa)
+ self.assertOverrides(universe, bin_i386, copied_bin_i386)
+ self.assertOverrides(universe, bin_hppa, copied_bin_hppa)
def test_existing_publication_overrides(self):
# When source/binaries are copied to a destination primary archive,
@@ -1289,16 +1267,15 @@
[copied_source, copied_bin_i386, copied_bin_hppa] = self.doCopy(
source, target_archive, nobby, source.pocket, True)
self.assertEqual(copied_source.component, existing_source.component)
- self.assertComponentSectionAndPriority(
- ebin_i386.component, ebin_i386, copied_bin_i386)
- self.assertComponentSectionAndPriority(
- ebin_hppa.component, ebin_hppa, copied_bin_hppa)
+ self.assertOverrides(ebin_i386.component, ebin_i386, copied_bin_i386)
+ self.assertOverrides(ebin_hppa.component, ebin_hppa, copied_bin_hppa)
- def _setup_archive(self):
+ def _setup_archive(self, version="1.0-2", use_nobby=False, **kwargs):
archive = self.test_publisher.ubuntutest.main_archive
+ nobby = self.createNobby(('i386', 'hppa'))
source = self.test_publisher.getPubSource(
- archive=archive, version='1.0-2', architecturehintlist='any')
- nobby = self.createNobby(('i386', 'hppa'))
+ archive=archive, version=version, architecturehintlist='any',
+ distroseries=(nobby if use_nobby else None), **kwargs)
getUtility(ISourcePackageFormatSelectionSet).add(
nobby, SourcePackageFormat.FORMAT_1_0)
return nobby, archive, source
@@ -1306,10 +1283,8 @@
def test_existing_publication_overrides_pockets(self):
# When we copy source/binaries from one pocket to another, the
# overrides are unchanged from the source publication overrides.
- nobby, archive, _ = self._setup_archive()
- source = self.test_publisher.getPubSource(
- archive=archive, version='1.0-1', architecturehintlist='any',
- distroseries=nobby, pocket=PackagePublishingPocket.PROPOSED)
+ nobby, archive, source = self._setup_archive(
+ use_nobby=True, pocket=PackagePublishingPocket.PROPOSED)
[bin_i386, bin_hppa] = self.test_publisher.getPubBinaries(
pub_source=source, distroseries=nobby,
pocket=PackagePublishingPocket.PROPOSED)
@@ -1321,10 +1296,8 @@
[copied_source, copied_bin_i386, copied_bin_hppa] = self.doCopy(
source, archive, nobby, PackagePublishingPocket.UPDATES, True)
self.assertEqual(copied_source.component, source.component)
- self.assertComponentSectionAndPriority(
- bin_i386.component, bin_i386, copied_bin_i386)
- self.assertComponentSectionAndPriority(
- bin_hppa.component, bin_hppa, copied_bin_hppa)
+ self.assertOverrides(bin_i386.component, bin_i386, copied_bin_i386)
+ self.assertOverrides(bin_hppa.component, bin_hppa, copied_bin_hppa)
def test_existing_publication_no_overrides(self):
# When we copy source/binaries into a PPA, we don't respect their
@@ -1343,16 +1316,31 @@
source, target_archive, nobby, source.pocket, True)
main = getUtility(IComponentSet)['main']
self.assertEqual(main, copied_source.component)
- self.assertComponentSectionAndPriority(
- main, bin_i386, copied_bin_i386)
- self.assertComponentSectionAndPriority(
- main, bin_hppa, copied_bin_hppa)
+ self.assertOverrides(main, bin_i386, copied_bin_i386)
+ self.assertOverrides(main, bin_hppa, copied_bin_hppa)
+
+ def test_deleted_publication_overrides(self):
+ # Copying a deleted publication still respects its overrides.
+ nobby, archive, source = self._setup_archive(
+ use_nobby=True, pocket=PackagePublishingPocket.PROPOSED)
+ [bin_i386, bin_hppa] = self.test_publisher.getPubBinaries(
+ pub_source=source, distroseries=nobby,
+ pocket=PackagePublishingPocket.PROPOSED)
+ component = self.factory.makeComponent()
+ for kind in (source, bin_i386, bin_hppa):
+ kind.component = component
+ getUtility(IPublishingSet).requestDeletion([source], archive.owner)
+ transaction.commit()
+
+ [copied_source, copied_bin_i386, copied_bin_hppa] = self.doCopy(
+ source, archive, nobby, PackagePublishingPocket.UPDATES, True)
+ self.assertEqual(source.component, copied_source.component)
+ self.assertOverrides(bin_i386.component, bin_i386, copied_bin_i386)
+ self.assertOverrides(bin_hppa.component, bin_hppa, copied_bin_hppa)
def test_copy_into_derived_series(self):
# We are able to successfully copy into a derived series.
- archive = self.test_publisher.ubuntutest.main_archive
- source = self.test_publisher.getPubSource(
- archive=archive, version='1.0-2', architecturehintlist='any')
+ _, archive, source = self._setup_archive()
dsp = self.factory.makeDistroSeriesParent()
target_archive = dsp.derived_series.main_archive
switch_dbuser('archivepublisher')
@@ -1363,9 +1351,7 @@
def test_copy_with_override(self):
# Test the override parameter for do_copy and by extension
# _do_direct_copy.
- archive = self.test_publisher.ubuntutest.main_archive
- source = self.test_publisher.getPubSource(
- archive=archive, version='1.0-2', architecturehintlist='any')
+ _, archive, source = self._setup_archive()
dsp = self.factory.makeDistroSeriesParent()
target_archive = dsp.derived_series.main_archive
override = SourceOverride(
@@ -1424,16 +1410,11 @@
def test_copy_generates_notification(self):
# When a copy into a primary archive is performed, a notification is
# sent.
- archive = self.test_publisher.ubuntutest.main_archive
- source = self.test_publisher.getPubSource(
- archive=archive, version='1.0-2', architecturehintlist='any')
+ nobby, archive, source = self._setup_archive()
changelog = self.factory.makeChangelog(spn="foo", versions=["1.0-2"])
source.sourcepackagerelease.changelog = changelog
# Copying to a primary archive reads the changes to close bugs.
transaction.commit()
- nobby = self.createNobby(('i386', 'hppa'))
- getUtility(ISourcePackageFormatSelectionSet).add(
- nobby, SourcePackageFormat.FORMAT_1_0)
nobby.changeslist = 'nobby-changes@xxxxxxxxxxx'
[copied_source] = do_copy(
[source], archive, nobby, source.pocket, False,
@@ -1506,19 +1487,14 @@
# the changelog should contain all of the changelog_entry texts for
# all the sourcepackagereleases between the last published version
# and the new version.
- archive = self.test_publisher.ubuntutest.main_archive
- source3 = self.test_publisher.getPubSource(
- sourcename="foo", archive=archive, version='1.2',
- architecturehintlist='any')
+ nobby, archive, source3 = self._setup_archive(
+ sourcename="foo", version="1.2")
changelog = self.factory.makeChangelog(
spn="foo", versions=["1.2", "1.1", "1.0"])
source3.sourcepackagerelease.changelog = changelog
transaction.commit()
- # Now make a new series, nobby, and publish foo 1.0 in it.
- nobby = self.createNobby(('i386', 'hppa'))
- getUtility(ISourcePackageFormatSelectionSet).add(
- nobby, SourcePackageFormat.FORMAT_1_0)
+ # Now publish foo 1.0 in nobby.
nobby.changeslist = 'nobby-changes@xxxxxxxxxxx'
source1 = self.factory.makeSourcePackageRelease(
sourcepackagename="foo", version="1.0")
=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py 2013-02-14 01:10:48 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py 2013-03-07 17:50:51 +0000
@@ -978,6 +978,22 @@
published_sources = archive.getPublishedSources(name=u"copyme")
self.assertIsNotNone(published_sources.any())
+ def test_copying_resurrects_deleted_package_primary_new(self):
+ # Resurrecting a previously-deleted package in a PRIMARY archive
+ # (which has an archive admin workflow) requires NEW queue approval.
+ archive = self.factory.makeArchive(
+ self.distroseries.distribution, purpose=ArchivePurpose.PRIMARY)
+ spph = self.publisher.getPubSource(
+ distroseries=self.distroseries, sourcename="copyme",
+ status=PackagePublishingStatus.DELETED, archive=archive)
+ job = self.createCopyJobForSPPH(
+ spph, archive, archive, requester=archive.owner)
+ self.runJob(job)
+ self.assertEqual(JobStatus.SUSPENDED, job.status)
+ pu = getUtility(IPackageUploadSet).getByPackageCopyJobIDs(
+ [removeSecurityProxy(job).context.id]).one()
+ self.assertEqual(PackageUploadStatus.NEW, pu.status)
+
def test_copying_to_main_archive_unapproved(self):
# Uploading to a series that is in a state that precludes auto
# approval will cause the job to suspend and a packageupload
