Merge lp:~cjwatson/launchpad/always-copy-packages-asynchronously into lp:launchpad

Proposed by Colin Watson
Status: Merged
Approved by: Curtis Hovey
Approved revision: no longer in the source branch.
Merged at revision: 16210
Proposed branch: lp:~cjwatson/launchpad/always-copy-packages-asynchronously
Merge into: lp:launchpad
Diff against target: 540 lines (+21/-329)
4 files modified
lib/lp/services/features/flags.py (+0/-6)
lib/lp/soyuz/browser/archive.py (+13/-114)
lib/lp/soyuz/browser/tests/archive-views.txt (+7/-58)
lib/lp/soyuz/browser/tests/test_package_copying_mixin.py (+1/-151)
To merge this branch: bzr merge lp:~cjwatson/launchpad/always-copy-packages-asynchronously
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Review via email: mp+131837@code.launchpad.net

Commit message

Remove the old synchronous package copy path in favour of PackageCopyJobs.

Description of the change

== 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.

To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

Thank you.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/services/features/flags.py'
2--- lib/lp/services/features/flags.py 2012-10-25 14:18:36 +0000
3+++ lib/lp/services/features/flags.py 2012-10-29 09:40:33 +0000
4@@ -154,12 +154,6 @@
5 '',
6 '',
7 ''),
8- ('soyuz.derived_series.max_synchronous_syncs',
9- 'int',
10- "How many package syncs may be done directly in a web request.",
11- '100',
12- '',
13- ''),
14 ('soyuz.derived_series_sync.enabled',
15 'boolean',
16 'Enables syncing of packages on derivative distributions pages.',
17
18=== modified file 'lib/lp/soyuz/browser/archive.py'
19--- lib/lp/soyuz/browser/archive.py 2012-09-21 06:52:06 +0000
20+++ lib/lp/soyuz/browser/archive.py 2012-10-29 09:40:33 +0000
21@@ -29,7 +29,6 @@
22 ]
23
24
25-from cgi import escape
26 from datetime import (
27 datetime,
28 timedelta,
29@@ -92,11 +91,7 @@
30 get_plural_text,
31 get_user_agent_distroseries,
32 )
33-from lp.services.database.bulk import (
34- load,
35- load_related,
36- )
37-from lp.services.features import getFeatureFlag
38+from lp.services.database.bulk import load_related
39 from lp.services.helpers import english_list
40 from lp.services.job.model.job import Job
41 from lp.services.librarian.browser import FileNavigationMixin
42@@ -166,21 +161,8 @@
43 Archive,
44 validate_ppa,
45 )
46-from lp.soyuz.model.binarypackagename import BinaryPackageName
47-from lp.soyuz.model.publishing import (
48- BinaryPackagePublishingHistory,
49- SourcePackagePublishingHistory,
50- )
51-from lp.soyuz.scripts.packagecopier import (
52- check_copy_permissions,
53- do_copy,
54- )
55-
56-# Feature flag: up to how many package sync requests (inclusive) are to be
57-# processed synchronously within the web request?
58-# Set to -1 to disable synchronous syncs.
59-FEATURE_FLAG_MAX_SYNCHRONOUS_SYNCS = (
60- 'soyuz.derived_series.max_synchronous_syncs')
61+from lp.soyuz.model.publishing import SourcePackagePublishingHistory
62+from lp.soyuz.scripts.packagecopier import check_copy_permissions
63
64
65 class ArchiveBadges(HasBadgeBase):
66@@ -1256,63 +1238,6 @@
67 _messageNoValue = _("vocabulary-copy-to-same-series", "The same series")
68
69
70-def preload_binary_package_names(copies):
71- """Preload `BinaryPackageName`s to speed up display-name construction."""
72- bpn_ids = [
73- copy.binarypackagerelease.binarypackagenameID for copy in copies
74- if isinstance(copy, BinaryPackagePublishingHistory)]
75- load(BinaryPackageName, bpn_ids)
76-
77-
78-def compose_synchronous_copy_feedback(copies, dest_archive, dest_url=None,
79- dest_display_name=None):
80- """Compose human-readable feedback after a synchronous copy."""
81- if dest_url is None:
82- dest_url = escape(
83- canonical_url(dest_archive) + '/+packages', quote=True)
84-
85- if dest_display_name is None:
86- dest_display_name = escape(dest_archive.displayname)
87-
88- if len(copies) == 0:
89- return structured(
90- '<p>All packages already copied to <a href="%s">%s</a>.</p>'
91- % (dest_url, dest_display_name))
92- else:
93- messages = []
94- messages.append(
95- '<p>Packages copied to <a href="%s">%s</a>:</p>'
96- % (dest_url, dest_display_name))
97- messages.append('<ul>')
98- messages.append("\n".join([
99- '<li>%s</li>' % escape(copy) for copy in copies]))
100- messages.append('</ul>')
101- return structured("\n".join(messages))
102-
103-
104-def copy_synchronously(source_pubs, dest_archive, dest_series, dest_pocket,
105- include_binaries, dest_url=None,
106- dest_display_name=None, person=None,
107- check_permissions=True):
108- """Copy packages right now.
109-
110- :return: A `structured` with human-readable feedback about the
111- operation.
112- :raises CannotCopy: If `check_permissions` is True and the copy is
113- not permitted.
114- """
115- copies = do_copy(
116- source_pubs, dest_archive, dest_series, dest_pocket, include_binaries,
117- allow_delayed_copies=True, person=person,
118- check_permissions=check_permissions)
119-
120- preload_binary_package_names(copies)
121-
122- return compose_synchronous_copy_feedback(
123- [copy.displayname for copy in copies], dest_archive, dest_url,
124- dest_display_name)
125-
126-
127 def copy_asynchronously(source_pubs, dest_archive, dest_series, dest_pocket,
128 include_binaries, dest_url=None,
129 dest_display_name=None, person=None,
130@@ -1385,28 +1310,14 @@
131 class PackageCopyingMixin:
132 """A mixin class that adds helpers for package copying."""
133
134- def canCopySynchronously(self, source_pubs):
135- """Can we afford to copy `source_pubs` synchronously?"""
136- # Fixed estimate: up to 100 packages can be copied in acceptable
137- # time. Anything more than that and we go async.
138- limit = getFeatureFlag(FEATURE_FLAG_MAX_SYNCHRONOUS_SYNCS)
139- try:
140- limit = int(limit)
141- except:
142- limit = 100
143-
144- return len(source_pubs) <= limit
145-
146 def do_copy(self, sources_field_name, source_pubs, dest_archive,
147 dest_series, dest_pocket, include_binaries,
148 dest_url=None, dest_display_name=None, person=None,
149- check_permissions=True, force_async=False,
150- sponsored_person=None):
151+ check_permissions=True, sponsored_person=None):
152 """Copy packages and add appropriate feedback to the browser page.
153
154- This may either copy synchronously, if there are few enough
155- requests to process right now; or asynchronously in which case
156- it will schedule jobs that will be processed by a script.
157+ This will copy asynchronously, scheduling jobs that will be
158+ processed by a script.
159
160 :param sources_field_name: The name of the form field to set errors
161 on when the copy fails
162@@ -1425,30 +1336,18 @@
163 :param person: The person requesting the copy.
164 :param: check_permissions: boolean indicating whether or not the
165 requester's permissions to copy should be checked.
166- :param force_async: Force the copy to create package copy jobs and
167- perform the copy asynchronously.
168 :param sponsored_person: An IPerson representing the person being
169- sponsored (for asynchronous copies only).
170+ sponsored.
171
172 :return: True if the copying worked, False otherwise.
173 """
174- assert force_async or not sponsored_person, (
175- "sponsored must be None for sync copies")
176 try:
177- if (force_async == False and
178- self.canCopySynchronously(source_pubs)):
179- notification = copy_synchronously(
180- source_pubs, dest_archive, dest_series, dest_pocket,
181- include_binaries, dest_url=dest_url,
182- dest_display_name=dest_display_name, person=person,
183- check_permissions=check_permissions)
184- else:
185- notification = copy_asynchronously(
186- source_pubs, dest_archive, dest_series, dest_pocket,
187- include_binaries, dest_url=dest_url,
188- dest_display_name=dest_display_name, person=person,
189- check_permissions=check_permissions,
190- sponsored=sponsored_person)
191+ notification = copy_asynchronously(
192+ source_pubs, dest_archive, dest_series, dest_pocket,
193+ include_binaries, dest_url=dest_url,
194+ dest_display_name=dest_display_name, person=person,
195+ check_permissions=check_permissions,
196+ sponsored=sponsored_person)
197 except CannotCopy as error:
198 self.setFieldError(
199 sources_field_name, render_cannotcopy_as_html(error))
200
201=== modified file 'lib/lp/soyuz/browser/tests/archive-views.txt'
202--- lib/lp/soyuz/browser/tests/archive-views.txt 2012-09-13 04:11:09 +0000
203+++ lib/lp/soyuz/browser/tests/archive-views.txt 2012-10-29 09:40:33 +0000
204@@ -1342,9 +1342,8 @@
205 Copy private files to public archives
206 -------------------------------------
207
208-Users are allowed to copy private sources into private PPAs, however
209-it happens via 'delayed-copies' not the usual direct copying method.
210-See more information in scripts/packagecopier.py
211+Users are allowed to copy private sources into public PPAs.
212+See more information in scripts/packagecopier.py.
213
214 First we will enable Celso's private PPA.
215
216@@ -1386,60 +1385,12 @@
217 >>> len(view.errors)
218 0
219
220-The action is performed as a delayed-copy, and the user is informed of
221+The action is performed as an asynchronous copy, and the user is informed of
222 it via a page notification.
223
224 >>> from lp.testing.pages import extract_text
225 >>> for notification in view.request.response.notifications:
226 ... print extract_text(notification.message)
227- Packages copied to PPA for Ubuntu Team:
228- Delayed copy of foocomm - 2.0-1 (source)
229-
230-The delayed-copy request is waiting to be processed in the ACCEPTED
231-upload queue.
232-
233- >>> from lp.soyuz.interfaces.queue import IPackageUploadSet
234- >>> copy = getUtility(IPackageUploadSet).findSourceUpload(
235- ... 'foocomm', '2.0-1', ubuntu_team_ppa, ubuntu)
236-
237- >>> print copy.status.name
238- ACCEPTED
239-
240-The action may also be performed asynchronously.
241-
242- >>> from lp.services.features.testing import FeatureFixture
243- >>> from lp.soyuz.browser.archive import (
244- ... FEATURE_FLAG_MAX_SYNCHRONOUS_SYNCS,
245- ... )
246- >>> fixture = FeatureFixture({
247- ... FEATURE_FLAG_MAX_SYNCHRONOUS_SYNCS: '0',
248- ... })
249- >>> fixture.setUp()
250-
251- >>> login('foo.bar@canonical.com')
252- >>> async_private_source = test_publisher.createSource(
253- ... cprov_private_ppa, 'foocomm', '1.0-1', new_version='3.0-1')
254- >>> transaction.commit()
255-
256- >>> print async_private_source.displayname
257- foocomm 3.0-1 in hoary
258-
259- >>> login('celso.providelo@canonical.com')
260- >>> view = create_initialized_view(
261- ... cprov_private_ppa, name="+copy-packages",
262- ... form={
263- ... 'field.selected_sources': [str(async_private_source.id)],
264- ... 'field.destination_archive': 'ubuntu-team/ppa',
265- ... 'field.destination_series': '',
266- ... 'field.include_binaries': 'REBUILD_SOURCES',
267- ... 'field.actions.copy': 'Copy',
268- ... })
269-
270- >>> len(view.errors)
271- 0
272-
273- >>> for notification in view.request.response.notifications:
274- ... print extract_text(notification.message)
275 Requested sync of 1 package.
276 Please allow some time for this to be processed.
277
278@@ -1463,14 +1414,14 @@
279 >>> [copied_source] = ubuntu_team_ppa.getPublishedSources(
280 ... name="foocomm", exact_match=True)
281 >>> print copied_source.displayname
282- foocomm 3.0-1 in hoary
283+ foocomm 2.0-1 in hoary
284
285 If we run the same copy again, it will fail.
286
287 >>> view = create_initialized_view(
288 ... cprov_private_ppa, name="+copy-packages",
289 ... form={
290- ... 'field.selected_sources': [str(async_private_source.id)],
291+ ... 'field.selected_sources': [str(private_source.id)],
292 ... 'field.destination_archive': 'ubuntu-team/ppa',
293 ... 'field.destination_series': '',
294 ... 'field.include_binaries': 'REBUILD_SOURCES',
295@@ -1495,12 +1446,10 @@
296 >>> for job in ubuntu_team_ppa_view.package_copy_jobs:
297 ... print job.status.title, job.package_name, job.package_version
298 ... print job.error_message
299- Failed foocomm 3.0-1
300- foocomm 3.0-1 in hoary (same version already building in the destination
301+ Failed foocomm 2.0-1
302+ foocomm 2.0-1 in hoary (same version already building in the destination
303 archive for Hoary)
304
305- >>> fixture.cleanUp()
306-
307
308 External dependencies validation
309 ================================
310
311=== modified file 'lib/lp/soyuz/browser/tests/test_package_copying_mixin.py'
312--- lib/lp/soyuz/browser/tests/test_package_copying_mixin.py 2012-06-28 17:50:41 +0000
313+++ lib/lp/soyuz/browser/tests/test_package_copying_mixin.py 2012-10-29 09:40:33 +0000
314@@ -1,4 +1,4 @@
315-# Copyright 2011 Canonical Ltd. This software is licensed under the
316+# Copyright 2011-2012 Canonical Ltd. This software is licensed under the
317 # GNU Affero General Public License version 3 (see the file LICENSE).
318
319 """Tests for `PackageCopyingMixin`."""
320@@ -9,14 +9,9 @@
321 from zope.security.proxy import removeSecurityProxy
322
323 from lp.registry.interfaces.pocket import PackagePublishingPocket
324-from lp.services.features.testing import FeatureFixture
325 from lp.services.propertycache import cachedproperty
326 from lp.soyuz.browser.archive import (
327- compose_synchronous_copy_feedback,
328 copy_asynchronously,
329- copy_synchronously,
330- FEATURE_FLAG_MAX_SYNCHRONOUS_SYNCS,
331- PackageCopyingMixin,
332 render_cannotcopy_as_html,
333 )
334 from lp.soyuz.enums import SourcePackageFormat
335@@ -29,9 +24,7 @@
336 TestCase,
337 TestCaseWithFactory,
338 )
339-from lp.testing.fakemethod import FakeMethod
340 from lp.testing.layers import LaunchpadFunctionalLayer
341-from lp.testing.views import create_initialized_view
342
343
344 def find_spph_copy(archive, spph):
345@@ -41,40 +34,6 @@
346 name=spr.sourcepackagename.name, version=spr.version).one()
347
348
349-class FakeDistribution:
350- def __init__(self):
351- self.archive = FakeArchive()
352-
353-
354-class FakeDistroSeries:
355- def __init__(self):
356- self.distribution = FakeDistribution()
357-
358-
359-class FakeSPN:
360- name = "spn-name"
361-
362-
363-class FakeSPR:
364- def __init__(self):
365- self.sourcepackagename = FakeSPN()
366- self.version = "1.0"
367-
368-
369-class FakeArchive:
370- def __init__(self, displayname="archive-name"):
371- self.displayname = displayname
372-
373-
374-class FakeSPPH:
375- def __init__(self, archive=None):
376- if archive is None:
377- archive = FakeArchive()
378- self.sourcepackagerelease = FakeSPR()
379- self.displayname = "spph-displayname"
380- self.archive = archive
381-
382-
383 class TestPackageCopyingMixinLight(TestCase):
384 """Test lightweight functions and methods.
385
386@@ -83,25 +42,11 @@
387
388 unique_number = 1
389
390- def getPocket(self):
391- """Return an arbitrary `PackagePublishingPocket`."""
392- return PackagePublishingPocket.SECURITY
393-
394 def getUniqueString(self):
395 """Return an arbitrary string."""
396 self.unique_number += 1
397 return "string_%d_" % self.unique_number
398
399- def test_canCopySynchronously_allows_small_synchronous_copies(self):
400- # Small numbers of packages can be copied synchronously.
401- packages = [self.getUniqueString() for counter in range(3)]
402- self.assertTrue(PackageCopyingMixin().canCopySynchronously(packages))
403-
404- def test_canCopySynchronously_disallows_large_synchronous_copies(self):
405- # Large numbers of packages must be copied asynchronously.
406- packages = [self.getUniqueString() for counter in range(300)]
407- self.assertFalse(PackageCopyingMixin().canCopySynchronously(packages))
408-
409 def test_render_cannotcopy_as_html_lists_errors(self):
410 # render_cannotcopy_as_html includes a CannotCopy error message
411 # into its HTML notice.
412@@ -116,24 +61,6 @@
413 self.assertNotIn(message, html_text)
414 self.assertIn("x&lt;&gt;y", html_text)
415
416- def test_compose_synchronous_copy_feedback_escapes_archive_name(self):
417- # compose_synchronous_copy_feedback escapes archive displaynames.
418- archive = FakeArchive(displayname="a&b")
419- notice = compose_synchronous_copy_feedback(
420- ["hi"], archive, dest_url="/")
421- html_text = notice.escapedtext
422- self.assertNotIn("a&b", html_text)
423- self.assertIn("a&amp;b", html_text)
424-
425- def test_compose_synchronous_copy_feedback_escapes_package_names(self):
426- # compose_synchronous_copy_feedback escapes package names.
427- archive = FakeArchive()
428- notice = compose_synchronous_copy_feedback(
429- ["x<y"], archive, dest_url="/")
430- html_text = notice.escapedtext
431- self.assertNotIn("x<y", html_text)
432- self.assertIn("x&lt;y", html_text)
433-
434
435 class TestPackageCopyingMixinIntegration(TestCaseWithFactory):
436 """Integration tests for `PackageCopyingMixin`."""
437@@ -180,36 +107,12 @@
438 derived_series, SourcePackageFormat.FORMAT_1_0)
439 return derived_series
440
441- def makeView(self):
442- """Create a `PackageCopyingMixin`-based view."""
443- return create_initialized_view(
444- self.makeDerivedSeries(), "+localpackagediffs")
445-
446 def getUploader(self, archive, spn):
447 """Get person with upload rights for the given package and archive."""
448 uploader = archive.owner
449 removeSecurityProxy(archive).newPackageUploader(uploader, spn)
450 return uploader
451
452- def test_canCopySynchronously_obeys_feature_flag(self):
453- packages = [self.getUniqueString() for counter in range(3)]
454- mixin = PackageCopyingMixin()
455- with FeatureFixture({FEATURE_FLAG_MAX_SYNCHRONOUS_SYNCS: 2}):
456- can_copy_synchronously = mixin.canCopySynchronously(packages)
457- self.assertFalse(can_copy_synchronously)
458-
459- def test_copy_synchronously_copies_packages(self):
460- # copy_synchronously copies packages into the destination
461- # archive.
462- spph = self.makeSPPH()
463- dest_series = self.makeDerivedSeries()
464- archive = dest_series.distribution.main_archive
465- pocket = self.factory.getAnyPocket()
466- copy_synchronously(
467- [spph], archive, dest_series, pocket, include_binaries=False,
468- check_permissions=False)
469- self.assertNotEqual(None, find_spph_copy(archive, spph))
470-
471 def test_copy_asynchronously_does_not_copy_packages(self):
472 # copy_asynchronously does not copy packages into the destination
473 # archive; that happens later, asynchronously.
474@@ -222,19 +125,6 @@
475 check_permissions=False, person=self.factory.makePerson())
476 self.assertEqual(None, find_spph_copy(archive, spph))
477
478- def test_copy_synchronously_lists_packages(self):
479- # copy_synchronously returns feedback that includes the names of
480- # packages it copied.
481- spph = self.makeSPPH()
482- dest_series = self.makeDerivedSeries()
483- pocket = self.factory.getAnyPocket()
484- notice = copy_synchronously(
485- [spph], dest_series.distribution.main_archive, dest_series,
486- pocket, include_binaries=False,
487- check_permissions=False).escapedtext
488- self.assertIn(
489- spph.sourcepackagerelease.sourcepackagename.name, notice)
490-
491 def test_copy_asynchronously_creates_copy_jobs(self):
492 # copy_asynchronously creates PackageCopyJobs.
493 spph = self.makeSPPH()
494@@ -281,46 +171,6 @@
495 [("one", spph_one.distroseries), ("two", spph_two.distroseries)],
496 [(job.package_name, job.target_distroseries) for job in jobs])
497
498- def test_do_copy_goes_async_if_canCopySynchronously_says_so(self):
499- # The view opts for asynchronous copying if canCopySynchronously
500- # returns False. This creates PackageCopyJobs.
501- spph = self.makeSPPH()
502- pocket = self.factory.getAnyPocket()
503- view = self.makeView()
504- dest_series = view.context
505- archive = dest_series.distribution.main_archive
506- view.canCopySynchronously = FakeMethod(result=False)
507- view.do_copy(
508- 'selected_differences', [spph], archive, dest_series, pocket,
509- False, check_permissions=False, person=self.factory.makePerson())
510- jobs = list(getUtility(IPlainPackageCopyJobSource).getActiveJobs(
511- archive))
512- self.assertNotEqual([], jobs)
513-
514- def test_copy_synchronously_may_allow_copy(self):
515- # In a normal working situation, copy_synchronously allows a
516- # copy.
517- spph = self.makeSPPH()
518- pocket = PackagePublishingPocket.RELEASE
519- dest_series = self.makeDerivedSeries()
520- dest_archive = dest_series.main_archive
521- spn = spph.sourcepackagerelease.sourcepackagename
522- notification = copy_synchronously(
523- [spph], dest_archive, dest_series, pocket, False,
524- person=self.getUploader(dest_archive, spn))
525- self.assertIn("Packages copied", notification.escapedtext)
526-
527- def test_copy_synchronously_checks_permissions(self):
528- # Unless told not to, copy_synchronously does a permissions
529- # check.
530- spph = self.makeSPPH()
531- pocket = self.factory.getAnyPocket()
532- dest_series = self.makeDistroSeries()
533- self.assertRaises(
534- CannotCopy,
535- copy_synchronously,
536- [spph], dest_series.main_archive, dest_series, pocket, False)
537-
538 def test_copy_asynchronously_may_allow_copy(self):
539 # In a normal working situation, copy_asynchronously allows a
540 # copy.