launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #13835
[Merge] lp:~cjwatson/launchpad/always-copy-packages-asynchronously into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/always-copy-packages-asynchronously into lp:launchpad.
Commit message:
Remove the old synchronous package copy path in favour of PackageCopyJobs.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #575450 in Launchpad itself: "Archive:+copy-packages nearly unusable due to timeouts"
https://bugs.launchpad.net/launchpad/+bug/575450
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/always-copy-packages-asynchronously/+merge/131837
== Summary ==
The synchronous package copy path has always been full of timeouts, and is now completely obsoleted by the asynchronous path. It has been feature-flagged off for all users for a couple of weeks, and the initial Ubuntu auto-sync was uneventful in this regard. We can and should now remove the old code.
== Proposed fix ==
Remove the dead code.
== Tests ==
bin/test -vvct lib/lp/soyuz/browser/tests/archive-views.txt -t lp.soyuz.browser.tests.test_package_copying_mixin
== Demo and Q/A ==
There should be no user-visible effects since this just amounts to removing code that has been disabled by a feature flag, but we should verify that copies between PPAs using the web UI still work.
--
https://code.launchpad.net/~cjwatson/launchpad/always-copy-packages-asynchronously/+merge/131837
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/always-copy-packages-asynchronously into lp:launchpad.
=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py 2012-10-25 14:18:36 +0000
+++ lib/lp/services/features/flags.py 2012-10-29 09:40:33 +0000
@@ -154,12 +154,6 @@
'',
'',
''),
- ('soyuz.derived_series.max_synchronous_syncs',
- 'int',
- "How many package syncs may be done directly in a web request.",
- '100',
- '',
- ''),
('soyuz.derived_series_sync.enabled',
'boolean',
'Enables syncing of packages on derivative distributions pages.',
=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py 2012-09-21 06:52:06 +0000
+++ lib/lp/soyuz/browser/archive.py 2012-10-29 09:40:33 +0000
@@ -29,7 +29,6 @@
]
-from cgi import escape
from datetime import (
datetime,
timedelta,
@@ -92,11 +91,7 @@
get_plural_text,
get_user_agent_distroseries,
)
-from lp.services.database.bulk import (
- load,
- load_related,
- )
-from lp.services.features import getFeatureFlag
+from lp.services.database.bulk import load_related
from lp.services.helpers import english_list
from lp.services.job.model.job import Job
from lp.services.librarian.browser import FileNavigationMixin
@@ -166,21 +161,8 @@
Archive,
validate_ppa,
)
-from lp.soyuz.model.binarypackagename import BinaryPackageName
-from lp.soyuz.model.publishing import (
- BinaryPackagePublishingHistory,
- SourcePackagePublishingHistory,
- )
-from lp.soyuz.scripts.packagecopier import (
- check_copy_permissions,
- do_copy,
- )
-
-# Feature flag: up to how many package sync requests (inclusive) are to be
-# processed synchronously within the web request?
-# Set to -1 to disable synchronous syncs.
-FEATURE_FLAG_MAX_SYNCHRONOUS_SYNCS = (
- 'soyuz.derived_series.max_synchronous_syncs')
+from lp.soyuz.model.publishing import SourcePackagePublishingHistory
+from lp.soyuz.scripts.packagecopier import check_copy_permissions
class ArchiveBadges(HasBadgeBase):
@@ -1256,63 +1238,6 @@
_messageNoValue = _("vocabulary-copy-to-same-series", "The same series")
-def preload_binary_package_names(copies):
- """Preload `BinaryPackageName`s to speed up display-name construction."""
- bpn_ids = [
- copy.binarypackagerelease.binarypackagenameID for copy in copies
- if isinstance(copy, BinaryPackagePublishingHistory)]
- load(BinaryPackageName, bpn_ids)
-
-
-def compose_synchronous_copy_feedback(copies, dest_archive, dest_url=None,
- dest_display_name=None):
- """Compose human-readable feedback after a synchronous copy."""
- if dest_url is None:
- dest_url = escape(
- canonical_url(dest_archive) + '/+packages', quote=True)
-
- if dest_display_name is None:
- dest_display_name = escape(dest_archive.displayname)
-
- if len(copies) == 0:
- return structured(
- '<p>All packages already copied to <a href="%s">%s</a>.</p>'
- % (dest_url, dest_display_name))
- else:
- messages = []
- messages.append(
- '<p>Packages copied to <a href="%s">%s</a>:</p>'
- % (dest_url, dest_display_name))
- messages.append('<ul>')
- messages.append("\n".join([
- '<li>%s</li>' % escape(copy) for copy in copies]))
- messages.append('</ul>')
- return structured("\n".join(messages))
-
-
-def copy_synchronously(source_pubs, dest_archive, dest_series, dest_pocket,
- include_binaries, dest_url=None,
- dest_display_name=None, person=None,
- check_permissions=True):
- """Copy packages right now.
-
- :return: A `structured` with human-readable feedback about the
- operation.
- :raises CannotCopy: If `check_permissions` is True and the copy is
- not permitted.
- """
- copies = do_copy(
- source_pubs, dest_archive, dest_series, dest_pocket, include_binaries,
- allow_delayed_copies=True, person=person,
- check_permissions=check_permissions)
-
- preload_binary_package_names(copies)
-
- return compose_synchronous_copy_feedback(
- [copy.displayname for copy in copies], dest_archive, dest_url,
- dest_display_name)
-
-
def copy_asynchronously(source_pubs, dest_archive, dest_series, dest_pocket,
include_binaries, dest_url=None,
dest_display_name=None, person=None,
@@ -1385,28 +1310,14 @@
class PackageCopyingMixin:
"""A mixin class that adds helpers for package copying."""
- def canCopySynchronously(self, source_pubs):
- """Can we afford to copy `source_pubs` synchronously?"""
- # Fixed estimate: up to 100 packages can be copied in acceptable
- # time. Anything more than that and we go async.
- limit = getFeatureFlag(FEATURE_FLAG_MAX_SYNCHRONOUS_SYNCS)
- try:
- limit = int(limit)
- except:
- limit = 100
-
- return len(source_pubs) <= limit
-
def do_copy(self, sources_field_name, source_pubs, dest_archive,
dest_series, dest_pocket, include_binaries,
dest_url=None, dest_display_name=None, person=None,
- check_permissions=True, force_async=False,
- sponsored_person=None):
+ check_permissions=True, sponsored_person=None):
"""Copy packages and add appropriate feedback to the browser page.
- This may either copy synchronously, if there are few enough
- requests to process right now; or asynchronously in which case
- it will schedule jobs that will be processed by a script.
+ This will copy asynchronously, scheduling jobs that will be
+ processed by a script.
:param sources_field_name: The name of the form field to set errors
on when the copy fails
@@ -1425,30 +1336,18 @@
:param person: The person requesting the copy.
:param: check_permissions: boolean indicating whether or not the
requester's permissions to copy should be checked.
- :param force_async: Force the copy to create package copy jobs and
- perform the copy asynchronously.
:param sponsored_person: An IPerson representing the person being
- sponsored (for asynchronous copies only).
+ sponsored.
:return: True if the copying worked, False otherwise.
"""
- assert force_async or not sponsored_person, (
- "sponsored must be None for sync copies")
try:
- if (force_async == False and
- self.canCopySynchronously(source_pubs)):
- notification = copy_synchronously(
- source_pubs, dest_archive, dest_series, dest_pocket,
- include_binaries, dest_url=dest_url,
- dest_display_name=dest_display_name, person=person,
- check_permissions=check_permissions)
- else:
- notification = copy_asynchronously(
- source_pubs, dest_archive, dest_series, dest_pocket,
- include_binaries, dest_url=dest_url,
- dest_display_name=dest_display_name, person=person,
- check_permissions=check_permissions,
- sponsored=sponsored_person)
+ notification = copy_asynchronously(
+ source_pubs, dest_archive, dest_series, dest_pocket,
+ include_binaries, dest_url=dest_url,
+ dest_display_name=dest_display_name, person=person,
+ check_permissions=check_permissions,
+ sponsored=sponsored_person)
except CannotCopy as error:
self.setFieldError(
sources_field_name, render_cannotcopy_as_html(error))
=== modified file 'lib/lp/soyuz/browser/tests/archive-views.txt'
--- lib/lp/soyuz/browser/tests/archive-views.txt 2012-09-13 04:11:09 +0000
+++ lib/lp/soyuz/browser/tests/archive-views.txt 2012-10-29 09:40:33 +0000
@@ -1342,9 +1342,8 @@
Copy private files to public archives
-------------------------------------
-Users are allowed to copy private sources into private PPAs, however
-it happens via 'delayed-copies' not the usual direct copying method.
-See more information in scripts/packagecopier.py
+Users are allowed to copy private sources into public PPAs.
+See more information in scripts/packagecopier.py.
First we will enable Celso's private PPA.
@@ -1386,60 +1385,12 @@
>>> len(view.errors)
0
-The action is performed as a delayed-copy, and the user is informed of
+The action is performed as an asynchronous copy, and the user is informed of
it via a page notification.
>>> from lp.testing.pages import extract_text
>>> for notification in view.request.response.notifications:
... print extract_text(notification.message)
- Packages copied to PPA for Ubuntu Team:
- Delayed copy of foocomm - 2.0-1 (source)
-
-The delayed-copy request is waiting to be processed in the ACCEPTED
-upload queue.
-
- >>> from lp.soyuz.interfaces.queue import IPackageUploadSet
- >>> copy = getUtility(IPackageUploadSet).findSourceUpload(
- ... 'foocomm', '2.0-1', ubuntu_team_ppa, ubuntu)
-
- >>> print copy.status.name
- ACCEPTED
-
-The action may also be performed asynchronously.
-
- >>> from lp.services.features.testing import FeatureFixture
- >>> from lp.soyuz.browser.archive import (
- ... FEATURE_FLAG_MAX_SYNCHRONOUS_SYNCS,
- ... )
- >>> fixture = FeatureFixture({
- ... FEATURE_FLAG_MAX_SYNCHRONOUS_SYNCS: '0',
- ... })
- >>> fixture.setUp()
-
- >>> login('foo.bar@xxxxxxxxxxxxx')
- >>> async_private_source = test_publisher.createSource(
- ... cprov_private_ppa, 'foocomm', '1.0-1', new_version='3.0-1')
- >>> transaction.commit()
-
- >>> print async_private_source.displayname
- foocomm 3.0-1 in hoary
-
- >>> login('celso.providelo@xxxxxxxxxxxxx')
- >>> view = create_initialized_view(
- ... cprov_private_ppa, name="+copy-packages",
- ... form={
- ... 'field.selected_sources': [str(async_private_source.id)],
- ... 'field.destination_archive': 'ubuntu-team/ppa',
- ... 'field.destination_series': '',
- ... 'field.include_binaries': 'REBUILD_SOURCES',
- ... 'field.actions.copy': 'Copy',
- ... })
-
- >>> len(view.errors)
- 0
-
- >>> for notification in view.request.response.notifications:
- ... print extract_text(notification.message)
Requested sync of 1 package.
Please allow some time for this to be processed.
@@ -1463,14 +1414,14 @@
>>> [copied_source] = ubuntu_team_ppa.getPublishedSources(
... name="foocomm", exact_match=True)
>>> print copied_source.displayname
- foocomm 3.0-1 in hoary
+ foocomm 2.0-1 in hoary
If we run the same copy again, it will fail.
>>> view = create_initialized_view(
... cprov_private_ppa, name="+copy-packages",
... form={
- ... 'field.selected_sources': [str(async_private_source.id)],
+ ... 'field.selected_sources': [str(private_source.id)],
... 'field.destination_archive': 'ubuntu-team/ppa',
... 'field.destination_series': '',
... 'field.include_binaries': 'REBUILD_SOURCES',
@@ -1495,12 +1446,10 @@
>>> for job in ubuntu_team_ppa_view.package_copy_jobs:
... print job.status.title, job.package_name, job.package_version
... print job.error_message
- Failed foocomm 3.0-1
- foocomm 3.0-1 in hoary (same version already building in the destination
+ Failed foocomm 2.0-1
+ foocomm 2.0-1 in hoary (same version already building in the destination
archive for Hoary)
- >>> fixture.cleanUp()
-
External dependencies validation
================================
=== modified file 'lib/lp/soyuz/browser/tests/test_package_copying_mixin.py'
--- lib/lp/soyuz/browser/tests/test_package_copying_mixin.py 2012-06-28 17:50:41 +0000
+++ lib/lp/soyuz/browser/tests/test_package_copying_mixin.py 2012-10-29 09:40:33 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011 Canonical Ltd. This software is licensed under the
+# Copyright 2011-2012 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for `PackageCopyingMixin`."""
@@ -9,14 +9,9 @@
from zope.security.proxy import removeSecurityProxy
from lp.registry.interfaces.pocket import PackagePublishingPocket
-from lp.services.features.testing import FeatureFixture
from lp.services.propertycache import cachedproperty
from lp.soyuz.browser.archive import (
- compose_synchronous_copy_feedback,
copy_asynchronously,
- copy_synchronously,
- FEATURE_FLAG_MAX_SYNCHRONOUS_SYNCS,
- PackageCopyingMixin,
render_cannotcopy_as_html,
)
from lp.soyuz.enums import SourcePackageFormat
@@ -29,9 +24,7 @@
TestCase,
TestCaseWithFactory,
)
-from lp.testing.fakemethod import FakeMethod
from lp.testing.layers import LaunchpadFunctionalLayer
-from lp.testing.views import create_initialized_view
def find_spph_copy(archive, spph):
@@ -41,40 +34,6 @@
name=spr.sourcepackagename.name, version=spr.version).one()
-class FakeDistribution:
- def __init__(self):
- self.archive = FakeArchive()
-
-
-class FakeDistroSeries:
- def __init__(self):
- self.distribution = FakeDistribution()
-
-
-class FakeSPN:
- name = "spn-name"
-
-
-class FakeSPR:
- def __init__(self):
- self.sourcepackagename = FakeSPN()
- self.version = "1.0"
-
-
-class FakeArchive:
- def __init__(self, displayname="archive-name"):
- self.displayname = displayname
-
-
-class FakeSPPH:
- def __init__(self, archive=None):
- if archive is None:
- archive = FakeArchive()
- self.sourcepackagerelease = FakeSPR()
- self.displayname = "spph-displayname"
- self.archive = archive
-
-
class TestPackageCopyingMixinLight(TestCase):
"""Test lightweight functions and methods.
@@ -83,25 +42,11 @@
unique_number = 1
- def getPocket(self):
- """Return an arbitrary `PackagePublishingPocket`."""
- return PackagePublishingPocket.SECURITY
-
def getUniqueString(self):
"""Return an arbitrary string."""
self.unique_number += 1
return "string_%d_" % self.unique_number
- def test_canCopySynchronously_allows_small_synchronous_copies(self):
- # Small numbers of packages can be copied synchronously.
- packages = [self.getUniqueString() for counter in range(3)]
- self.assertTrue(PackageCopyingMixin().canCopySynchronously(packages))
-
- def test_canCopySynchronously_disallows_large_synchronous_copies(self):
- # Large numbers of packages must be copied asynchronously.
- packages = [self.getUniqueString() for counter in range(300)]
- self.assertFalse(PackageCopyingMixin().canCopySynchronously(packages))
-
def test_render_cannotcopy_as_html_lists_errors(self):
# render_cannotcopy_as_html includes a CannotCopy error message
# into its HTML notice.
@@ -116,24 +61,6 @@
self.assertNotIn(message, html_text)
self.assertIn("x<>y", html_text)
- def test_compose_synchronous_copy_feedback_escapes_archive_name(self):
- # compose_synchronous_copy_feedback escapes archive displaynames.
- archive = FakeArchive(displayname="a&b")
- notice = compose_synchronous_copy_feedback(
- ["hi"], archive, dest_url="/")
- html_text = notice.escapedtext
- self.assertNotIn("a&b", html_text)
- self.assertIn("a&b", html_text)
-
- def test_compose_synchronous_copy_feedback_escapes_package_names(self):
- # compose_synchronous_copy_feedback escapes package names.
- archive = FakeArchive()
- notice = compose_synchronous_copy_feedback(
- ["x<y"], archive, dest_url="/")
- html_text = notice.escapedtext
- self.assertNotIn("x<y", html_text)
- self.assertIn("x<y", html_text)
-
class TestPackageCopyingMixinIntegration(TestCaseWithFactory):
"""Integration tests for `PackageCopyingMixin`."""
@@ -180,36 +107,12 @@
derived_series, SourcePackageFormat.FORMAT_1_0)
return derived_series
- def makeView(self):
- """Create a `PackageCopyingMixin`-based view."""
- return create_initialized_view(
- self.makeDerivedSeries(), "+localpackagediffs")
-
def getUploader(self, archive, spn):
"""Get person with upload rights for the given package and archive."""
uploader = archive.owner
removeSecurityProxy(archive).newPackageUploader(uploader, spn)
return uploader
- def test_canCopySynchronously_obeys_feature_flag(self):
- packages = [self.getUniqueString() for counter in range(3)]
- mixin = PackageCopyingMixin()
- with FeatureFixture({FEATURE_FLAG_MAX_SYNCHRONOUS_SYNCS: 2}):
- can_copy_synchronously = mixin.canCopySynchronously(packages)
- self.assertFalse(can_copy_synchronously)
-
- def test_copy_synchronously_copies_packages(self):
- # copy_synchronously copies packages into the destination
- # archive.
- spph = self.makeSPPH()
- dest_series = self.makeDerivedSeries()
- archive = dest_series.distribution.main_archive
- pocket = self.factory.getAnyPocket()
- copy_synchronously(
- [spph], archive, dest_series, pocket, include_binaries=False,
- check_permissions=False)
- self.assertNotEqual(None, find_spph_copy(archive, spph))
-
def test_copy_asynchronously_does_not_copy_packages(self):
# copy_asynchronously does not copy packages into the destination
# archive; that happens later, asynchronously.
@@ -222,19 +125,6 @@
check_permissions=False, person=self.factory.makePerson())
self.assertEqual(None, find_spph_copy(archive, spph))
- def test_copy_synchronously_lists_packages(self):
- # copy_synchronously returns feedback that includes the names of
- # packages it copied.
- spph = self.makeSPPH()
- dest_series = self.makeDerivedSeries()
- pocket = self.factory.getAnyPocket()
- notice = copy_synchronously(
- [spph], dest_series.distribution.main_archive, dest_series,
- pocket, include_binaries=False,
- check_permissions=False).escapedtext
- self.assertIn(
- spph.sourcepackagerelease.sourcepackagename.name, notice)
-
def test_copy_asynchronously_creates_copy_jobs(self):
# copy_asynchronously creates PackageCopyJobs.
spph = self.makeSPPH()
@@ -281,46 +171,6 @@
[("one", spph_one.distroseries), ("two", spph_two.distroseries)],
[(job.package_name, job.target_distroseries) for job in jobs])
- def test_do_copy_goes_async_if_canCopySynchronously_says_so(self):
- # The view opts for asynchronous copying if canCopySynchronously
- # returns False. This creates PackageCopyJobs.
- spph = self.makeSPPH()
- pocket = self.factory.getAnyPocket()
- view = self.makeView()
- dest_series = view.context
- archive = dest_series.distribution.main_archive
- view.canCopySynchronously = FakeMethod(result=False)
- view.do_copy(
- 'selected_differences', [spph], archive, dest_series, pocket,
- False, check_permissions=False, person=self.factory.makePerson())
- jobs = list(getUtility(IPlainPackageCopyJobSource).getActiveJobs(
- archive))
- self.assertNotEqual([], jobs)
-
- def test_copy_synchronously_may_allow_copy(self):
- # In a normal working situation, copy_synchronously allows a
- # copy.
- spph = self.makeSPPH()
- pocket = PackagePublishingPocket.RELEASE
- dest_series = self.makeDerivedSeries()
- dest_archive = dest_series.main_archive
- spn = spph.sourcepackagerelease.sourcepackagename
- notification = copy_synchronously(
- [spph], dest_archive, dest_series, pocket, False,
- person=self.getUploader(dest_archive, spn))
- self.assertIn("Packages copied", notification.escapedtext)
-
- def test_copy_synchronously_checks_permissions(self):
- # Unless told not to, copy_synchronously does a permissions
- # check.
- spph = self.makeSPPH()
- pocket = self.factory.getAnyPocket()
- dest_series = self.makeDistroSeries()
- self.assertRaises(
- CannotCopy,
- copy_synchronously,
- [spph], dest_series.main_archive, dest_series, pocket, False)
-
def test_copy_asynchronously_may_allow_copy(self):
# In a normal working situation, copy_asynchronously allows a
# copy.
Follow ups
