Merge ~ilasc/launchpad:populate-store-upload-revision into launchpad:master
- Git
- lp:~ilasc/launchpad
- populate-store-upload-revision
- Merge into master
Status: | Merged |
---|---|
Approved by: | Ioana Lasc |
Approved revision: | 87014a2cd2a230179585b1d3db9b26aad4f21014 |
Merge reported by: | Otto Co-Pilot |
Merged at revision: | not available |
Proposed branch: | ~ilasc/launchpad:populate-store-upload-revision |
Merge into: | launchpad:master |
Diff against target: |
252 lines (+149/-5) 4 files modified
database/schema/security.cfg (+1/-0) lib/lp/scripts/garbo.py (+51/-2) lib/lp/scripts/tests/test_garbo.py (+83/-1) lib/lp/snappy/model/snapbuild.py (+14/-2) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Colin Watson (community) | Approve | ||
Review via email: mp+407679@code.launchpad.net |
Commit message
Populate store_upload_
Description of the change
This populates the new DB column store_upload_
We're renaming the existent "store_
This work will be followed by a second MP for LP-349 to start updating the new DB column "store_
Ioana Lasc (ilasc) : | # |
Colin Watson (cjwatson) : | # |
Ioana Lasc (ilasc) wrote : | # |
Colin Watson (cjwatson) wrote : | # |
There's certainly no need to rename the DB column; for this sort of thing it's always easier to handle any necessary renamings in Python, and there are various primitives available for that. The underlying DB column name isn't required to match the name of the Storm column (it's set using the 'name' parameter to the Storm variable constructor), and there are quite a few cases where we have something like this:
_foo = Int(name='foo')
@property
def foo(self):
if self._foo is not None:
return self._foo
else:
# compute and return something
Of course anything that sets the actual DB column needs to use the Storm variable name, unless we also use a setter property (which can sometimes be useful), and Storm queries typically need to be written to refer to the name of the Storm variable rather than the wrapping property. The important thing is that any names used in API-exported attributes need to be defined in a way that preserves compatibility.
Happy to help with getting the details of this right tomorrow if you need it.
Ioana Lasc (ilasc) wrote : | # |
Thanks Colin! Updates done, MP ready for review.
Colin Watson (cjwatson) wrote : | # |
It's a bit strange to land the backfill job before starting to populate the new column in the "ordinary" inline way (in this case having `SnapStoreUploa
When you do start populating the new column in `SnapStoreUploa
Colin Watson (cjwatson) : | # |
Preview Diff
1 | diff --git a/database/schema/security.cfg b/database/schema/security.cfg | |||
2 | index 7f39796..8567419 100644 | |||
3 | --- a/database/schema/security.cfg | |||
4 | +++ b/database/schema/security.cfg | |||
5 | @@ -2530,6 +2530,7 @@ public.revisionauthor = SELECT, UPDATE | |||
6 | 2530 | public.revisioncache = SELECT, DELETE | 2530 | public.revisioncache = SELECT, DELETE |
7 | 2531 | public.snap = SELECT, UPDATE | 2531 | public.snap = SELECT, UPDATE |
8 | 2532 | public.snapbase = SELECT | 2532 | public.snapbase = SELECT |
9 | 2533 | public.snapbuild = SELECT, UPDATE | ||
10 | 2533 | public.snapfile = SELECT, DELETE | 2534 | public.snapfile = SELECT, DELETE |
11 | 2534 | public.snappydistroseries = SELECT | 2535 | public.snappydistroseries = SELECT |
12 | 2535 | public.snappyseries = SELECT | 2536 | public.snappyseries = SELECT |
13 | diff --git a/lib/lp/scripts/garbo.py b/lib/lp/scripts/garbo.py | |||
14 | index fba6c29..f416b3f 100644 | |||
15 | --- a/lib/lp/scripts/garbo.py | |||
16 | +++ b/lib/lp/scripts/garbo.py | |||
17 | @@ -37,6 +37,7 @@ from storm.expr import ( | |||
18 | 37 | Except, | 37 | Except, |
19 | 38 | In, | 38 | In, |
20 | 39 | Join, | 39 | Join, |
21 | 40 | LeftJoin, | ||
22 | 40 | Max, | 41 | Max, |
23 | 41 | Min, | 42 | Min, |
24 | 42 | Or, | 43 | Or, |
25 | @@ -134,8 +135,14 @@ from lp.services.verification.model.logintoken import LoginToken | |||
26 | 134 | from lp.services.webapp.publisher import canonical_url | 135 | from lp.services.webapp.publisher import canonical_url |
27 | 135 | from lp.services.webhooks.interfaces import IWebhookJobSource | 136 | from lp.services.webhooks.interfaces import IWebhookJobSource |
28 | 136 | from lp.services.webhooks.model import WebhookJob | 137 | from lp.services.webhooks.model import WebhookJob |
31 | 137 | from lp.snappy.model.snapbuild import SnapFile | 138 | from lp.snappy.model.snapbuild import ( |
32 | 138 | from lp.snappy.model.snapbuildjob import SnapBuildJobType | 139 | SnapBuild, |
33 | 140 | SnapFile, | ||
34 | 141 | ) | ||
35 | 142 | from lp.snappy.model.snapbuildjob import ( | ||
36 | 143 | SnapBuildJob, | ||
37 | 144 | SnapBuildJobType, | ||
38 | 145 | ) | ||
39 | 139 | from lp.soyuz.enums import ArchiveSubscriberStatus | 146 | from lp.soyuz.enums import ArchiveSubscriberStatus |
40 | 140 | from lp.soyuz.interfaces.publishing import active_publishing_status | 147 | from lp.soyuz.interfaces.publishing import active_publishing_status |
41 | 141 | from lp.soyuz.model.archive import Archive | 148 | from lp.soyuz.model.archive import Archive |
42 | @@ -1717,6 +1724,47 @@ class ArchiveAuthTokenDeactivator(BulkPruner): | |||
43 | 1717 | transaction.commit() | 1724 | transaction.commit() |
44 | 1718 | 1725 | ||
45 | 1719 | 1726 | ||
46 | 1727 | class PopulateSnapBuildStoreRevision(TunableLoop): | ||
47 | 1728 | """Populates snapbuild.store_revision if not set.""" | ||
48 | 1729 | |||
49 | 1730 | maximum_chunk_size = 5000 | ||
50 | 1731 | |||
51 | 1732 | def __init__(self, log, abort_time=None): | ||
52 | 1733 | super(PopulateSnapBuildStoreRevision, self).__init__(log, abort_time) | ||
53 | 1734 | self.start_at = 1 | ||
54 | 1735 | self.store = IMasterStore(SnapBuild) | ||
55 | 1736 | |||
56 | 1737 | def findSnapBuilds(self): | ||
57 | 1738 | origin = [ | ||
58 | 1739 | SnapBuild, | ||
59 | 1740 | LeftJoin( | ||
60 | 1741 | SnapBuildJob, | ||
61 | 1742 | SnapBuildJob.snapbuild_id == SnapBuild.id), | ||
62 | 1743 | LeftJoin( | ||
63 | 1744 | Job, | ||
64 | 1745 | Job.id == SnapBuildJob.job_id) | ||
65 | 1746 | ] | ||
66 | 1747 | builds = self.store.using(*origin).find( | ||
67 | 1748 | SnapBuild, | ||
68 | 1749 | SnapBuild.id >= self.start_at, | ||
69 | 1750 | SnapBuild._store_upload_revision == None, | ||
70 | 1751 | SnapBuildJob.job_type == SnapBuildJobType.STORE_UPLOAD, | ||
71 | 1752 | Job._status == JobStatus.COMPLETED) | ||
72 | 1753 | |||
73 | 1754 | return builds.order_by(SnapBuild.id) | ||
74 | 1755 | |||
75 | 1756 | def isDone(self): | ||
76 | 1757 | return self.findSnapBuilds().is_empty() | ||
77 | 1758 | |||
78 | 1759 | def __call__(self, chunk_size): | ||
79 | 1760 | builds = list(self.findSnapBuilds()[:chunk_size]) | ||
80 | 1761 | for build in builds: | ||
81 | 1762 | build._store_upload_revision = build.store_upload_revision | ||
82 | 1763 | if len(builds): | ||
83 | 1764 | self.start_at = builds[-1].id + 1 | ||
84 | 1765 | transaction.commit() | ||
85 | 1766 | |||
86 | 1767 | |||
87 | 1720 | class BaseDatabaseGarbageCollector(LaunchpadCronScript): | 1768 | class BaseDatabaseGarbageCollector(LaunchpadCronScript): |
88 | 1721 | """Abstract base class to run a collection of TunableLoops.""" | 1769 | """Abstract base class to run a collection of TunableLoops.""" |
89 | 1722 | script_name = None # Script name for locking and database user. Override. | 1770 | script_name = None # Script name for locking and database user. Override. |
90 | @@ -2008,6 +2056,7 @@ class DailyDatabaseGarbageCollector(BaseDatabaseGarbageCollector): | |||
91 | 2008 | OCIFilePruner, | 2056 | OCIFilePruner, |
92 | 2009 | ObsoleteBugAttachmentPruner, | 2057 | ObsoleteBugAttachmentPruner, |
93 | 2010 | OldTimeLimitedTokenDeleter, | 2058 | OldTimeLimitedTokenDeleter, |
94 | 2059 | PopulateSnapBuildStoreRevision, | ||
95 | 2011 | POTranslationPruner, | 2060 | POTranslationPruner, |
96 | 2012 | PreviewDiffPruner, | 2061 | PreviewDiffPruner, |
97 | 2013 | ProductVCSPopulator, | 2062 | ProductVCSPopulator, |
98 | diff --git a/lib/lp/scripts/tests/test_garbo.py b/lib/lp/scripts/tests/test_garbo.py | |||
99 | index 0d34fce..3a43045 100644 | |||
100 | --- a/lib/lp/scripts/tests/test_garbo.py | |||
101 | +++ b/lib/lp/scripts/tests/test_garbo.py | |||
102 | @@ -97,6 +97,7 @@ from lp.scripts.garbo import ( | |||
103 | 97 | load_garbo_job_state, | 97 | load_garbo_job_state, |
104 | 98 | LoginTokenPruner, | 98 | LoginTokenPruner, |
105 | 99 | OpenIDConsumerAssociationPruner, | 99 | OpenIDConsumerAssociationPruner, |
106 | 100 | PopulateSnapBuildStoreRevision, | ||
107 | 100 | ProductVCSPopulator, | 101 | ProductVCSPopulator, |
108 | 101 | save_garbo_job_state, | 102 | save_garbo_job_state, |
109 | 102 | UnusedPOTMsgSetPruner, | 103 | UnusedPOTMsgSetPruner, |
110 | @@ -130,11 +131,17 @@ from lp.services.verification.interfaces.authtoken import LoginTokenType | |||
111 | 130 | from lp.services.verification.model.logintoken import LoginToken | 131 | from lp.services.verification.model.logintoken import LoginToken |
112 | 131 | from lp.services.worlddata.interfaces.language import ILanguageSet | 132 | from lp.services.worlddata.interfaces.language import ILanguageSet |
113 | 132 | from lp.snappy.interfaces.snap import SNAP_TESTING_FLAGS | 133 | from lp.snappy.interfaces.snap import SNAP_TESTING_FLAGS |
114 | 134 | from lp.snappy.interfaces.snapbuildjob import ISnapStoreUploadJobSource | ||
115 | 135 | from lp.snappy.interfaces.snapstoreclient import ISnapStoreClient | ||
116 | 133 | from lp.snappy.model.snapbuild import SnapFile | 136 | from lp.snappy.model.snapbuild import SnapFile |
117 | 134 | from lp.snappy.model.snapbuildjob import ( | 137 | from lp.snappy.model.snapbuildjob import ( |
118 | 135 | SnapBuildJob, | 138 | SnapBuildJob, |
119 | 136 | SnapStoreUploadJob, | 139 | SnapStoreUploadJob, |
120 | 137 | ) | 140 | ) |
121 | 141 | from lp.snappy.tests.test_snapbuildjob import ( | ||
122 | 142 | FakeSnapStoreClient, | ||
123 | 143 | run_isolated_jobs, | ||
124 | 144 | ) | ||
125 | 138 | from lp.soyuz.enums import ( | 145 | from lp.soyuz.enums import ( |
126 | 139 | ArchiveSubscriberStatus, | 146 | ArchiveSubscriberStatus, |
127 | 140 | PackagePublishingStatus, | 147 | PackagePublishingStatus, |
128 | @@ -153,7 +160,11 @@ from lp.testing import ( | |||
129 | 153 | TestCase, | 160 | TestCase, |
130 | 154 | TestCaseWithFactory, | 161 | TestCaseWithFactory, |
131 | 155 | ) | 162 | ) |
133 | 156 | from lp.testing.dbuser import switch_dbuser | 163 | from lp.testing.dbuser import ( |
134 | 164 | dbuser, | ||
135 | 165 | switch_dbuser, | ||
136 | 166 | ) | ||
137 | 167 | from lp.testing.fixture import ZopeUtilityFixture | ||
138 | 157 | from lp.testing.layers import ( | 168 | from lp.testing.layers import ( |
139 | 158 | DatabaseLayer, | 169 | DatabaseLayer, |
140 | 159 | LaunchpadScriptLayer, | 170 | LaunchpadScriptLayer, |
141 | @@ -1978,6 +1989,77 @@ class TestGarbo(FakeAdapterMixin, TestCaseWithFactory): | |||
142 | 1978 | self.assertIsNotNone(token.date_deactivated) | 1989 | self.assertIsNotNone(token.date_deactivated) |
143 | 1979 | self.assertEmailQueueLength(0) | 1990 | self.assertEmailQueueLength(0) |
144 | 1980 | 1991 | ||
145 | 1992 | def test_PopulateSnapBuildStoreRevision(self): | ||
146 | 1993 | switch_dbuser('testadmin') | ||
147 | 1994 | snap1 = self.factory.makeSnap() | ||
148 | 1995 | build1 = self.factory.makeSnapBuild( | ||
149 | 1996 | snap=snap1, | ||
150 | 1997 | status=BuildStatus.FULLYBUILT) | ||
151 | 1998 | |||
152 | 1999 | # test that build1 does not get picked up | ||
153 | 2000 | # as it is a build without a store upload | ||
154 | 2001 | populator = PopulateSnapBuildStoreRevision(None) | ||
155 | 2002 | rs = populator.findSnapBuilds() | ||
156 | 2003 | self.assertEqual(0, rs.count()) | ||
157 | 2004 | |||
158 | 2005 | # Upload build | ||
159 | 2006 | job = getUtility(ISnapStoreUploadJobSource).create(build1) | ||
160 | 2007 | client = FakeSnapStoreClient() | ||
161 | 2008 | client.upload.result = ( | ||
162 | 2009 | "http://sca.example/dev/api/snaps/1/builds/1/status") | ||
163 | 2010 | client.checkStatus.result = ( | ||
164 | 2011 | "http://sca.example/dev/click-apps/1/rev/1/", 1) | ||
165 | 2012 | self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient)) | ||
166 | 2013 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): | ||
167 | 2014 | run_isolated_jobs([job]) | ||
168 | 2015 | |||
169 | 2016 | # this mimics what we have in the DB right now: | ||
170 | 2017 | # uploaded snaps that do not have the new DB column | ||
171 | 2018 | # _store_upload_revision populated yet | ||
172 | 2019 | populator = PopulateSnapBuildStoreRevision(None) | ||
173 | 2020 | filter = populator.findSnapBuilds() | ||
174 | 2021 | build1 = removeSecurityProxy(build1) | ||
175 | 2022 | self.assertEqual(1, filter.count()) | ||
176 | 2023 | self.assertEqual(build1, filter.one()) | ||
177 | 2024 | self.assertEqual(build1._store_upload_revision, None) | ||
178 | 2025 | |||
179 | 2026 | # run the garbo job and verify _store_upload_revision | ||
180 | 2027 | # is not populated with the value assigned to the build during upload | ||
181 | 2028 | self.runDaily() | ||
182 | 2029 | switch_dbuser('testadmin') | ||
183 | 2030 | self.assertEqual(build1._store_upload_revision, 1) | ||
184 | 2031 | |||
185 | 2032 | # Tests that of all builds for the same snap only those that have | ||
186 | 2033 | # been uploaded to the store will get | ||
187 | 2034 | # their new _store_upload_revision DB column updated | ||
188 | 2035 | build2 = self.factory.makeSnapBuild( | ||
189 | 2036 | snap=snap1, | ||
190 | 2037 | status=BuildStatus.FULLYBUILT) | ||
191 | 2038 | build3 = self.factory.makeSnapBuild( | ||
192 | 2039 | snap=snap1, | ||
193 | 2040 | status=BuildStatus.FULLYBUILT) | ||
194 | 2041 | job = getUtility(ISnapStoreUploadJobSource).create(build2) | ||
195 | 2042 | client = FakeSnapStoreClient() | ||
196 | 2043 | client.upload.result = ( | ||
197 | 2044 | "http://sca.example/dev/api/snaps/1/builds/2/status") | ||
198 | 2045 | client.checkStatus.result = ( | ||
199 | 2046 | "http://sca.example/dev/click-apps/1/rev/2/", 1) | ||
200 | 2047 | self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient)) | ||
201 | 2048 | with dbuser(config.ISnapStoreUploadJobSource.dbuser): | ||
202 | 2049 | run_isolated_jobs([job]) | ||
203 | 2050 | |||
204 | 2051 | populator = PopulateSnapBuildStoreRevision(None) | ||
205 | 2052 | filter = populator.findSnapBuilds() | ||
206 | 2053 | self.assertEqual(1, filter.count()) | ||
207 | 2054 | self.assertEqual(build2, filter.one()) | ||
208 | 2055 | |||
209 | 2056 | self.runDaily() | ||
210 | 2057 | switch_dbuser('testadmin') | ||
211 | 2058 | build2 = removeSecurityProxy(build2) | ||
212 | 2059 | self.assertEqual(build2._store_upload_revision, 1) | ||
213 | 2060 | build3 = removeSecurityProxy(build3) | ||
214 | 2061 | self.assertIsNone(build3._store_upload_revision) | ||
215 | 2062 | |||
216 | 1981 | 2063 | ||
217 | 1982 | class TestGarboTasks(TestCaseWithFactory): | 2064 | class TestGarboTasks(TestCaseWithFactory): |
218 | 1983 | layer = LaunchpadZopelessLayer | 2065 | layer = LaunchpadZopelessLayer |
219 | diff --git a/lib/lp/snappy/model/snapbuild.py b/lib/lp/snappy/model/snapbuild.py | |||
220 | index ca5285a..23864c9 100644 | |||
221 | --- a/lib/lp/snappy/model/snapbuild.py | |||
222 | +++ b/lib/lp/snappy/model/snapbuild.py | |||
223 | @@ -191,6 +191,8 @@ class SnapBuild(PackageBuildMixin, Storm): | |||
224 | 191 | 191 | ||
225 | 192 | failure_count = Int(name='failure_count', allow_none=False) | 192 | failure_count = Int(name='failure_count', allow_none=False) |
226 | 193 | 193 | ||
227 | 194 | _store_upload_revision = Int(name='store_upload_revision', allow_none=True) | ||
228 | 195 | |||
229 | 194 | store_upload_metadata = JSON('store_upload_json_data', allow_none=True) | 196 | store_upload_metadata = JSON('store_upload_json_data', allow_none=True) |
230 | 195 | 197 | ||
231 | 196 | def __init__(self, build_farm_job, requester, snap, archive, | 198 | def __init__(self, build_farm_job, requester, snap, archive, |
232 | @@ -527,8 +529,18 @@ class SnapBuild(PackageBuildMixin, Storm): | |||
233 | 527 | 529 | ||
234 | 528 | @property | 530 | @property |
235 | 529 | def store_upload_revision(self): | 531 | def store_upload_revision(self): |
238 | 530 | job = self.last_store_upload_job | 532 | # We are now persisting the revision assigned by the store |
239 | 531 | return job and job.store_revision | 533 | # on package upload in the new DB column _store_upload_revision. |
240 | 534 | # We backfill _store_upload_revision with | ||
241 | 535 | # PopulateSnapBuildStoreRevision. | ||
242 | 536 | # If the persisted field (_store_upload_revision) | ||
243 | 537 | # is not populated yet we return the old way of computing this | ||
244 | 538 | # value so that we don't break existing API clients. | ||
245 | 539 | if self._store_upload_revision: | ||
246 | 540 | return self._store_upload_revision | ||
247 | 541 | else: | ||
248 | 542 | job = self.last_store_upload_job | ||
249 | 543 | return job and job.store_revision | ||
250 | 532 | 544 | ||
251 | 533 | @property | 545 | @property |
252 | 534 | def store_upload_error_message(self): | 546 | def store_upload_error_message(self): |
Thanks Colin! That helped debug interactively and get to the bottom of it. It turns out the script was in fact getting triggered but it was failing with a "hidden" SilentLaunchpad ScriptFailure caused by garbo_daily not having enough permissions to update SnapBuild.
In terms of breaking API clients that expect the `store_ upload_ revision` attribute of a snap build to contain useful data, indeed I believe we have a problem there:
I might be misunderstanding this but it looks like we now have a collision between the DB column name and the property that I didn't realize was exposed through the API before we landed the DB patch. I think we won't be able to define a getter with the same name and have it exposed through API same as before along the lines of:
def store_upload_ revision( self): upload_ revision is None: upload_ revision
if self.store_
return self._store_
because that will interfere with the query to populate the DB (the new getter will not return None when it should so we can backfill, it would return the value in the former property).
I could propose a patch to alter the name of the currently empty column from "store_ upload_ revision" to "store_revision".
It's entirely possible I misunderstood what's possible / what's required (for backward compatibility) here though...