Merge lp:~jtv/launchpad/bug-845326 into lp:launchpad

Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Julian Edwards
Approved revision: no longer in the source branch.
Merged at revision: 13911
Proposed branch: lp:~jtv/launchpad/bug-845326
Merge into: lp:launchpad
Prerequisite: lp:~jtv/launchpad/transitional-published
Diff against target: 288 lines (+177/-22)
3 files modified
lib/lp/archivepublisher/domination.py (+19/-4)
lib/lp/archivepublisher/tests/test_dominator.py (+152/-17)
lib/lp/soyuz/model/publishing.py (+6/-1)
To merge this branch: bzr merge lp:~jtv/launchpad/bug-845326
Reviewer Review Type Date Requested Status
Julian Edwards (community) Approve
Review via email: mp+74741@code.launchpad.net

This proposal supersedes a proposal from 2011-09-09.

Commit message

[r=julian-edwards][bug=845326] Make newer [BS]PPHs supersede older ones for same version.

Description of the change

= Summary =

I'm re-doing the domination process so that one Dominator can serve the needs of traditional, latest-publication-record-wins domination; versions-we-see-in-Sources-lists-live domination as needed in Gina; and the as-yet unspecified multiple-versions-can-live-side-by-side domination we are supposed to get in the future.

My code for this was not a full replacement for traditional domination. A single package release can have multiple active publications in the same archive, distroseries, and pocket (e.g. when the package is being re-published into a different component) and traditional domination would mark the newer publication as superseding the older one. The challenge for this branch was to fix that, in a way that was still general enough to serve Gina's needs. All that Gina really knows is what version numbers should survive.

== Proposed fix ==

Extend the new-style domination algorithm: when domination finds multiple publication records for the same package (and archive etc.), for a version that should stay published, then have the newest one supersede the older ones.

(My previous generalized code would keep all publications for the version published. The classic dominator code would only keep the newest publication of the very latest version published and supersede all older ones.)

== Pre-implementation notes ==

William seems to think the solution is sane. As far as we're aware right now, this is the last step before we can land and deploy transitional Gina domination.

== Implementation details ==

The fix itself is fairly straightforward. It's in the first file in the diff. It involves one redundant variable and an extra case in an if/elif cadence, but overall I think it works out pretty cleanly.

== Tests ==

Besides a unit test for the new behaviour, for good measure I also added a massive test for complex combined data. This is not meant to replace proper unit tests; it's too complex for that. But it may reveal any breakage that the unit tests might miss.

{{{
./bin/test -vvc lp.archivepublisher.tests.test_dominator
}}}

== Demo and Q/A ==

A bunch of branches are going to land together. We'll have to make sure that domination still works; that Gina still works; and that Gina now does proper domination.

= Launchpad lint =

There's some pre-existing lint in the dependent branches that either can't be fixed, or would increase the risk of conflicts too much. I did not create any lint of my own, however, and left less than I found.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/archivepublisher/domination.py
  lib/lp/soyuz/scripts/gina/dominate.py
  lib/lp/soyuz/model/publishing.py
  lib/lp/soyuz/doc/gina.txt
  lib/lp/soyuz/interfaces/publishing.py
  scripts/gina.py
  lib/lp/archivepublisher/tests/test_dominator.py
  lib/lp/soyuz/scripts/tests/test_gina.py
  lib/lp/soyuz/scripts/gina/handlers.py

./lib/lp/soyuz/doc/gina.txt
     113: narrative exceeds 78 characters.
     162: want exceeds 78 characters.
     179: want exceeds 78 characters.
     189: narrative uses a moin header.
     221: want exceeds 78 characters.
     234: want exceeds 78 characters.
     240: want exceeds 78 characters.
     295: source exceeds 78 characters.
     324: narrative uses a moin header.
     342: narrative exceeds 78 characters.
     354: narrative uses a moin header.
     360: narrative exceeds 78 characters.
     361: narrative exceeds 78 characters.
     459: narrative uses a moin header.
     461: narrative exceeds 78 characters.
     462: narrative exceeds 78 characters.
     477: narrative uses a moin header.
     563: narrative exceeds 78 characters.
     600: narrative uses a moin header.
     657: narrative uses a moin header.
     746: narrative uses a moin header.
     767: narrative uses a moin header.
     780: narrative uses a moin header.
./lib/lp/soyuz/interfaces/publishing.py
     381: E261 at least two spaces before inline comment
     478: E261 at least two spaces before inline comment
     511: E261 at least two spaces before inline comment
     681: E261 at least two spaces before inline comment
     767: E261 at least two spaces before inline comment
./scripts/gina.py
      26: '_pythonpath' imported but unused

To post a comment you must log in.
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/archivepublisher/domination.py'
2--- lib/lp/archivepublisher/domination.py 2011-09-09 18:57:14 +0000
3+++ lib/lp/archivepublisher/domination.py 2011-09-09 18:57:18 +0000
4@@ -193,9 +193,11 @@
5 def dominatePackage(self, publications, live_versions, generalization):
6 """Dominate publications for a single package.
7
8- Active publications for versions in `live_versions` stay active.
9- Any older versions are marked as superseded by the respective
10- oldest live versions that are newer than the superseded ones.
11+ The latest publication for any version in `live_versions` stays
12+ active. Any older publications (including older publications for
13+ live versions with multiple publications) are marked as superseded by
14+ the respective oldest live releases that are newer than the superseded
15+ ones.
16
17 Any versions that are newer than anything in `live_versions` are
18 marked as deleted. This should not be possible in Soyuz-native
19@@ -222,12 +224,25 @@
20 publications, cmp=generalization.compare, reverse=True)
21
22 current_dominant = None
23+ dominant_version = None
24+
25 for pub in publications:
26- if generalization.getPackageVersion(pub) in live_versions:
27+ version = generalization.getPackageVersion(pub)
28+ # There should never be two published releases with the same
29+ # version. So this comparison is really a string
30+ # comparison, not a version comparison: if the versions are
31+ # equal by either measure, they're from the same release.
32+ if dominant_version is not None and version == dominant_version:
33+ # This publication is for a live version, but has been
34+ # superseded by a newer publication of the same version.
35+ # Supersede it.
36+ pub.supersede(current_dominant, logger=self.logger)
37+ elif version in live_versions:
38 # This publication stays active; if any publications
39 # that follow right after this are to be superseded,
40 # this is the release that they are superseded by.
41 current_dominant = pub
42+ dominant_version = version
43 elif current_dominant is None:
44 # This publication is no longer live, but there is no
45 # newer version to supersede it either. Therefore it
46
47=== modified file 'lib/lp/archivepublisher/tests/test_dominator.py'
48--- lib/lp/archivepublisher/tests/test_dominator.py 2011-09-09 18:57:14 +0000
49+++ lib/lp/archivepublisher/tests/test_dominator.py 2011-09-09 18:57:18 +0000
50@@ -5,9 +5,9 @@
51
52 __metaclass__ = type
53
54+import apt_pkg
55 import datetime
56-
57-import apt_pkg
58+from operator import attrgetter
59 from zope.security.proxy import removeSecurityProxy
60
61 from canonical.database.sqlbase import flush_database_updates
62@@ -248,24 +248,25 @@
63 return [spph.sourcepackagerelease.version for spph in spphs]
64
65
66+def alter_creation_dates(spphs, ages):
67+ """Set `datecreated` on each of `spphs` according to `ages`.
68+
69+ :param spphs: Iterable of `SourcePackagePublishingHistory`. Their
70+ respective creation dates will be offset by the respective ages found
71+ in `ages` (with the two being matched up in the same order).
72+ :param ages: Iterable of ages. Must provide the same number of items as
73+ `spphs`. Ages are `timedelta` objects that will be subtracted from
74+ the creation dates on the respective records in `spph`.
75+ """
76+ for spph, age in zip(spphs, ages):
77+ spph.datecreated -= age
78+
79+
80 class TestGeneralizedPublication(TestCaseWithFactory):
81 """Test publication generalization helpers."""
82
83 layer = ZopelessDatabaseLayer
84
85- def alterCreationDates(self, spphs, ages):
86- """Set `datecreated` on each of `spphs` according to `ages`.
87-
88- :param spphs: Iterable of `SourcePackagePublishingHistory`. Their
89- respective creation dates will be offset by the respective ages
90- found in `ages` (with the two being matched up in the same order).
91- :param ages: Iterable of ages. Must provide the same number of items
92- as `spphs`. Ages are `timedelta` objects that will be subtracted
93- from the creation dates on the respective records in `spph`.
94- """
95- for spph, age in zip(spphs, ages):
96- spph.datecreated -= age
97-
98 def test_getPackageVersion_gets_source_version(self):
99 spph = self.factory.makeSourcePackagePublishingHistory()
100 self.assertEqual(
101@@ -327,7 +328,7 @@
102 sourcepackagerelease=spr, distroseries=distroseries,
103 pocket=pocket)
104 for counter in xrange(len(ages))]
105- self.alterCreationDates(spphs, ages)
106+ alter_creation_dates(spphs, ages)
107
108 self.assertEqual(
109 [spphs[2], spphs[0], spphs[1]],
110@@ -351,13 +352,27 @@
111 sourcepackagerelease=self.factory.makeSourcePackageRelease(
112 version=version))
113 for counter in xrange(len(ages))]
114- self.alterCreationDates(spphs, ages)
115+ alter_creation_dates(spphs, ages)
116
117 self.assertEqual(
118 [spphs[2], spphs[0], spphs[1]],
119 sorted(spphs, cmp=GeneralizedPublication().compare))
120
121
122+def jumble(ordered_list):
123+ """Jumble the elements of `ordered_list` into a weird order.
124+
125+ Ordering is very important in domination. We jumble some of our lists to
126+ insure against "lucky coincidences" that might give our tests the right
127+ answers for the wrong reasons.
128+ """
129+ even = [
130+ item for offset, item in enumerate(ordered_list) if offset % 2 == 0]
131+ odd = [
132+ item for offset, item in enumerate(ordered_list) if offset % 2 != 0]
133+ return list(reversed(odd)) + even
134+
135+
136 class TestDominatorMethods(TestCaseWithFactory):
137
138 layer = ZopelessDatabaseLayer
139@@ -435,6 +450,126 @@
140 self.assertEqual(None, pubs[1].supersededby)
141 self.assertEqual(PackagePublishingStatus.DELETED, pubs[1].status)
142
143+ def test_dominatePackage_supersedes_replaced_pub_for_live_version(self):
144+ # Even if a publication record is for a live version, a newer
145+ # one for the same version supersedes it.
146+ spr = self.factory.makeSourcePackageRelease()
147+ series = self.factory.makeDistroSeries()
148+ pocket = PackagePublishingPocket.RELEASE
149+ pubs = [
150+ self.factory.makeSourcePackagePublishingHistory(
151+ archive=series.main_archive, distroseries=series,
152+ pocket=pocket, status=PackagePublishingStatus.PUBLISHED,
153+ sourcepackagerelease=spr)
154+ for counter in xrange(3)]
155+ alter_creation_dates(pubs, [
156+ datetime.timedelta(3),
157+ datetime.timedelta(2),
158+ datetime.timedelta(1),
159+ ])
160+
161+ self.makeDominator(pubs).dominatePackage(
162+ pubs, [spr.version], GeneralizedPublication(True))
163+ self.assertEqual([
164+ PackagePublishingStatus.SUPERSEDED,
165+ PackagePublishingStatus.SUPERSEDED,
166+ PackagePublishingStatus.PUBLISHED,
167+ ],
168+ [pub.status for pub in pubs])
169+ self.assertEqual(
170+ [spr, spr, None], [pub.supersededby for pub in pubs])
171+
172+ def test_dominatePackage_advanced_scenario(self):
173+ # Put dominatePackage through its paces with complex combined
174+ # data.
175+ # This test should be redundant in theory (which in theory
176+ # equates practice but in practice does not). If this fails,
177+ # don't just patch up the code or this test. Create unit tests
178+ # that specifically cover the difference, then change the code
179+ # and/or adapt this test to return to harmony.
180+ series = self.factory.makeDistroSeries()
181+ package = self.factory.makeSourcePackageName()
182+ pocket = PackagePublishingPocket.RELEASE
183+
184+ versions = ["1.%d" % number for number in xrange(4)]
185+
186+ # We have one package releases for each version.
187+ relevant_releases = dict(
188+ (version, self.factory.makeSourcePackageRelease(
189+ sourcepackagename=package, version=version))
190+ for version in jumble(versions))
191+
192+ # Each of those releases is subsequently published in
193+ # different components.
194+ components = jumble(
195+ [self.factory.makeComponent() for version in versions])
196+
197+ # Map versions to lists of publications for that version, from
198+ # oldest to newest. Each re-publishing into a different
199+ # component is meant to supersede publication into the previous
200+ # component.
201+ pubs_by_version = dict(
202+ (version, [
203+ self.factory.makeSourcePackagePublishingHistory(
204+ archive=series.main_archive, distroseries=series,
205+ pocket=pocket, status=PackagePublishingStatus.PUBLISHED,
206+ sourcepackagerelease=relevant_releases[version],
207+ component=component)
208+ for component in components])
209+ for version in jumble(versions))
210+
211+ ages = jumble(
212+ [datetime.timedelta(age) for age in xrange(len(versions))])
213+
214+ # Actually the "oldest to newest" order on the publications only
215+ # applies to their creation dates. Their creation orders are
216+ # irrelevant.
217+ for pubs_list in pubs_by_version.itervalues():
218+ alter_creation_dates(pubs_list, ages)
219+ pubs_list.sort(key=attrgetter('datecreated'))
220+
221+ live_versions = ["1.1", "1.2"]
222+ last_version_alive = sorted(live_versions)[-1]
223+
224+ all_pubs = sum(pubs_by_version.itervalues(), [])
225+ Dominator(DevNullLogger(), series.main_archive).dominatePackage(
226+ all_pubs, live_versions, GeneralizedPublication(True))
227+
228+ for version in reversed(versions):
229+ pubs = pubs_by_version[version]
230+
231+ if version in live_versions:
232+ # Beware: loop-carried variable. Used locally as well,
233+ # but tells later iterations what the highest-versioned
234+ # release so far was. This is used in tracking
235+ # supersededby links.
236+ superseding_release = pubs[-1].sourcepackagerelease
237+
238+ if version in live_versions:
239+ # The live versions' latest publications are Published,
240+ # their older ones Superseded.
241+ expected_status = (
242+ [PackagePublishingStatus.SUPERSEDED] * (len(pubs) - 1) +
243+ [PackagePublishingStatus.PUBLISHED])
244+ expected_supersededby = (
245+ [superseding_release] * (len(pubs) - 1) + [None])
246+ elif version < last_version_alive:
247+ # The superseded versions older than the last live
248+ # version have all been superseded.
249+ expected_status = (
250+ [PackagePublishingStatus.SUPERSEDED] * len(pubs))
251+ expected_supersededby = [superseding_release] * len(pubs)
252+ else:
253+ # Versions that are newer than any live release have
254+ # been deleted.
255+ expected_status = (
256+ [PackagePublishingStatus.DELETED] * len(pubs))
257+ expected_supersededby = [None] * len(pubs)
258+
259+ self.assertEqual(expected_status, [pub.status for pub in pubs])
260+ self.assertEqual(
261+ expected_supersededby, [pub.supersededby for pub in pubs])
262+
263 def test_dominateRemovedSourceVersions_dominates_publications(self):
264 # dominateRemovedSourceVersions finds the publications for a
265 # package and calls dominatePackage on them.
266
267=== modified file 'lib/lp/soyuz/model/publishing.py'
268--- lib/lp/soyuz/model/publishing.py 2011-09-09 18:57:14 +0000
269+++ lib/lp/soyuz/model/publishing.py 2011-09-09 18:57:18 +0000
270@@ -1966,12 +1966,17 @@
271 ]
272 assert publication_class in permitted_classes, "Deleting wrong type."
273
274+ if removed_by is None:
275+ removed_by_id = None
276+ else:
277+ removed_by_id = removed_by.id
278+
279 affected_pubs = IMasterStore(publication_class).find(
280 publication_class, publication_class.id.is_in(ids))
281 affected_pubs.set(
282 status=PackagePublishingStatus.DELETED,
283 datesuperseded=UTC_NOW,
284- removed_byID=removed_by.id,
285+ removed_byID=removed_by_id,
286 removal_comment=removal_comment)
287
288 def requestDeletion(self, sources, removed_by, removal_comment=None):