Merge lp:~doanac/ubuntu-ci-services-itself/publisher-ppa-sync into lp:ubuntu-ci-services-itself

Proposed by Andy Doan on 2014-03-16
Status: Merged
Approved by: Andy Doan on 2014-03-17
Approved revision: 404
Merged at revision: 407
Proposed branch: lp:~doanac/ubuntu-ci-services-itself/publisher-ppa-sync
Merge into: lp:ubuntu-ci-services-itself
Diff against target: 217 lines (+61/-60)
4 files modified
lander/bin/lander_service_wrapper.py (+44/-16)
lander/bin/ppa_sync.py (+17/-0)
ppa-assigner/ppa_assigner/api.py (+0/-24)
ppa-assigner/ppa_assigner/tests.py (+0/-20)
To merge this branch: bzr merge lp:~doanac/ubuntu-ci-services-itself/publisher-ppa-sync
Reviewer Review Type Date Requested Status
Francis Ginther 2014-03-16 Approve on 2014-03-17
PS Jenkins bot (community) continuous-integration Approve on 2014-03-17
Review via email: mp+211189@code.launchpad.net

Commit message

publisher: move ppa_sync logic into lander

We are seeing some flakiness in the publishing phase. This simplifies
things a little in a way that might make it "just work", and at a
minimum, it should get us improved logging.

Description of the change

This is an idea I had to help improve the chances of the publisher working. It solves the issue where publisher logs weren't getting connected to Joe's new webui workflow stuff.

To post a comment you must log in.
Francis Ginther (fginther) wrote :

ppa-assigner/ppa_assigner/api.py still has:
  from ppa_assigner import ppa_sync

This breaks startup of ppa_django. Still reviewing.

review: Needs Fixing
Francis Ginther (fginther) wrote :

68-95: the try block in lander/bin/lander_service_wrapper.py:

There are two actions in this try block, the ppa_copy and the artifact creation. If either fails, the publiser step is marked as failed. I believe this is the right outcome and is consistent with other components. Another thing I considered if either of these actions could be retried if it fails and I don't see a use case where it would help.

So I thinks is right, but please give it a second thought if I overlooked something.

I'm finished reviewing this.

Andy Doan (doanac) wrote :

On 03/16/2014 01:51 PM, Francis Ginther wrote:
> 68-95: the try block in lander/bin/lander_service_wrapper.py:
>
> There are two actions in this try block, the ppa_copy and the
> artifact creation. If either fails, the publiser step is marked as
> failed. I believe this is the right outcome and is consistent with
> other components. Another thing I considered if either of these
> actions could be retried if it fails and I don't see a use case where
> it would help.

Hmm. I think you've noticed something here. Really once we are through
that first block, the ticket has been marked COMPLETE. The 2nd block is
merely uploading/attaching artifacts. I don't think its worth failing an
otherwise successfull ticket over that. That should be easy to fix.

Andy Doan (doanac) wrote :

I'm testing revno 404 right now with:

 http://15.125.119.88/ticket.html?ticket_id=4

Not sure I totally trust myself patching this on the fly like I am

Andy Doan (doanac) wrote :

ticket: http://15.125.119.88/ticket.html?ticket_id=4 looks good. moving to "needs review"

PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:404
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/444/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/444/rebuild

review: Approve (continuous-integration)
Francis Ginther (fginther) wrote :

90 + with open(output) as f:
91 + ticket_id = config['master']['request_id']
92 + auth = unit_config.get_auth_config()
93 + ds = data_store.create_for_ticket(ticket_id, auth)
94 + url = ds.put_file(output, f.read(), 'text/plain')
95 + result['artifacts'] = [{
96 + 'name': output,
97 + 'reference': url,
98 + 'type': 'LOGS',
99 + }]
100 + ticket.process_artifacts(result)

This is now outside the try block. Is that what you want? If it fails, the exception will now be caught by the try block in main(). This looks right to me as it's still a failure that needs to be surfaced and dealt with (in this case it would behave in a similar way as if the API call had failed). I'll approve, but just want to mention this in case you had a different outcome in mind.

review: Approve
Andy Doan (doanac) wrote :

On 03/17/2014 10:44 AM, Francis Ginther wrote:
> This is now outside the try block. Is that what you want? If it
> fails, the exception will now be caught by the try block in main().
> This looks right to me as it's still a failure that needs to be
> surfaced and dealt with (in this case it would behave in a similar
> way as if the API call had failed). I'll approve, but just want to
> mention this in case you had a different outcome in mind.

It fails the Jenkins job, but not the ticket. I figured this was a good
distinction. ie - there was a failure we might want to investigate. Its
just not a failure that should stop a ticket from being complete.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lander/bin/lander_service_wrapper.py'
2--- lander/bin/lander_service_wrapper.py 2014-03-14 18:26:46 +0000
3+++ lander/bin/lander_service_wrapper.py 2014-03-17 03:49:31 +0000
4@@ -19,6 +19,7 @@
5 import logging
6 import os
7 import signal
8+import subprocess
9 import sys
10 import time
11 import urllib2
12@@ -27,8 +28,12 @@
13 # the worker might not have installed this module, so determine the path
14 # and add it, so we can always safely import stuff
15 sys.path.append(os.path.join(os.path.dirname(__file__), '../../ci-utils'))
16-from ci_utils import amqp_utils
17-from ci_utils import dump_stack
18+from ci_utils import (
19+ amqp_utils,
20+ data_store,
21+ dump_stack,
22+ unit_config,
23+)
24
25 from ticket_api import TicketApi
26
27@@ -43,6 +48,10 @@
28 l.addHandler(stdout_handler)
29 l.setLevel(logging.DEBUG)
30
31+# the keystoneclient.httpclient debug logging leaks passwords. It enables
32+# this based on the setting of the "swiftclient" level
33+logging.getLogger('swiftclient').setLevel(logging.INFO)
34+
35 logger = logging.getLogger('lander_service_wrapper')
36
37
38@@ -283,8 +292,6 @@
39 config = _get_config(args)
40 ticket = _ticket_api(config)
41
42- url = '%s/api/v1/ppa_copy/' % config['master']['ppa_assigner_url']
43-
44 request_parameters = ticket.get_full_ticket()
45 packages = []
46 try:
47@@ -295,24 +302,45 @@
48 # Expected if there is no source package upload
49 pass
50
51- params = {
52- 'series': request_parameters['series'],
53- 'packages': packages,
54- 'source': config['bsbuilder']['ppa'],
55- 'target': request_parameters['master_ppa'],
56- }
57+ result = {'result': 'FAILED', 'packages': packages}
58+
59+ args = [
60+ os.path.join(os.path.dirname(__file__), 'ppa_sync.py'),
61+ '--source', config['bsbuilder']['ppa'],
62+ '--target', request_parameters['master_ppa'],
63+ '--series', request_parameters['series'],
64+ ]
65+ for p in packages:
66+ args.extend(['--package', p])
67+
68 try:
69 ticket.set_publishing_inprogress()
70- _post(url, params)
71- ticket.set_publishing_complete(True)
72+ output = 'package_publish.output.log'
73+ with open(output, 'w') as f:
74+ os.environ['PYTHONPATH'] = ':'.join(sys.path)
75+ rc = subprocess.call(args, stderr=subprocess.STDOUT, stdout=f)
76+ f.write('\nRC=%d\n' % rc)
77+ if rc == 0:
78+ result['result'] = 'PASSED'
79+ ticket.set_publishing_complete(True)
80+ else:
81+ ticket.set_publishing_complete(False)
82 except:
83 ticket.set_publishing_complete(False)
84 raise
85
86- result = {
87- 'result': 'PASSED',
88- 'packages': params['packages'],
89- }
90+ with open(output) as f:
91+ ticket_id = config['master']['request_id']
92+ auth = unit_config.get_auth_config()
93+ ds = data_store.create_for_ticket(ticket_id, auth)
94+ url = ds.put_file(output, f.read(), 'text/plain')
95+ result['artifacts'] = [{
96+ 'name': output,
97+ 'reference': url,
98+ 'type': 'LOGS',
99+ }]
100+ ticket.process_artifacts(result)
101+
102 return result
103
104
105
106=== renamed file 'ppa-assigner/ppa_assigner/ppa_sync.py' => 'lander/bin/ppa_sync.py'
107--- ppa-assigner/ppa_assigner/ppa_sync.py 2014-03-15 03:22:48 +0000
108+++ lander/bin/ppa_sync.py 2014-03-17 03:49:31 +0000
109@@ -1,5 +1,7 @@
110+#!/usr/bin/env python
111 # adapted from: http://code.launchpad.net/ppa-dev-tools
112
113+import argparse
114 import logging
115 import subprocess
116
117@@ -123,3 +125,18 @@
118 to_pocket='Release',
119 version=source_data['version'],
120 to_series=series.name)
121+
122+if __name__ == '__main__':
123+ parser = argparse.ArgumentParser(
124+ description='Copy packages from one ppa to another')
125+ parser.add_argument('--source', required=True,
126+ help='Source PPA')
127+ parser.add_argument('--target', required=True,
128+ help='Target PPA')
129+ parser.add_argument('--series', required=True,
130+ help='Ubuntu series')
131+ parser.add_argument('--package', action='append',
132+ help='Package (can be repeated)')
133+
134+ args = parser.parse_args()
135+ copy(args.source, args.target, args.package, args.series)
136
137=== modified file 'ppa-assigner/ppa_assigner/api.py'
138--- ppa-assigner/ppa_assigner/api.py 2014-03-14 23:53:04 +0000
139+++ ppa-assigner/ppa_assigner/api.py 2014-03-17 03:49:31 +0000
140@@ -22,13 +22,10 @@
141 from tastypie.http import (
142 HttpAccepted,
143 HttpApplicationError,
144- HttpBadRequest,
145- HttpNoContent,
146 HttpNotFound
147 )
148 from tastypie.resources import ALL
149
150-from ppa_assigner import ppa_sync
151 from ppa_assigner.models import PPA
152
153 from ci_utils.json_status import JSONStatus
154@@ -78,26 +75,6 @@
155 return bundle
156
157
158-class PPACopyResource(AuthorizedResource):
159- class Meta(AuthorizedResource.Meta):
160- resource_name = 'ppa_copy'
161- # just return nothing to work with tastypie
162- queryset = PPA.objects.none()
163-
164- def post_list(self, request, **kwargs):
165- fmt = request.META.get('CONTENT_TYPE', 'application/json')
166- data = self.deserialize(request, request.body, format=fmt)
167- required = set(('source', 'target', 'series', 'packages'))
168- missing = required - set(data.keys())
169- if len(missing):
170- return HttpBadRequest(
171- 'missing required parameters: %s\n' % ' '.join(required))
172-
173- ppa_sync.copy(
174- data['source'], data['target'], data['packages'], data['series'])
175- return HttpNoContent()
176-
177-
178 class PPAStatus(AuthorizedResource):
179 class Meta(AuthorizedResource.Meta):
180 resource_name = 'status'
181@@ -135,5 +112,4 @@
182
183 v1_api = Api(api_name='v1')
184 v1_api.register(PPAResource())
185-v1_api.register(PPACopyResource())
186 v1_api.register(PPAStatus())
187
188=== modified file 'ppa-assigner/ppa_assigner/tests.py'
189--- ppa-assigner/ppa_assigner/tests.py 2014-03-14 23:53:04 +0000
190+++ ppa-assigner/ppa_assigner/tests.py 2014-03-17 03:49:31 +0000
191@@ -240,26 +240,6 @@
192 self.assertEqual(PPA.objects.filter(private=False).count(), 2)
193 self.assertEqual(PPA.objects.all().count(), 3)
194
195- def testCopyBadParams(self):
196- resource = self._resource('ppa_copy/')
197- resp = self.client.post(
198- resource, data={}, authentication=self.auth)
199- self.assertHttpBadRequest(resp)
200-
201- @mock.patch('ppa_assigner.ppa_sync.copy')
202- def testCopy(self, copy):
203- resource = self._resource('ppa_copy/')
204- data = {
205- 'source': 'ppa:t1/ppa',
206- 'target': 'ppa:t2/ppa',
207- 'series': 'trusty',
208- 'packages': ['autopilot'],
209- }
210- resp = self.client.post(
211- resource, data=data, authentication=self.auth)
212- self.assertHttpAccepted(resp)
213- copy.assert_called_once()
214-
215 def testStatus(self):
216 status = self.get('status/')
217 total_ppas = PPA.objects.filter(private=settings.PRIVATE_PPAS_ONLY)

Subscribers

People subscribed via source and target branches