Merge lp:~bac/charms/oneiric/buildbot-master/history-s3 into lp:~yellow/charms/oneiric/buildbot-master/trunk

Proposed by Brad Crittenden
Status: Merged
Approved by: Gary Poster
Approved revision: 34
Merged at revision: 33
Proposed branch: lp:~bac/charms/oneiric/buildbot-master/history-s3
Merge into: lp:~yellow/charms/oneiric/buildbot-master/trunk
Diff against target: 590 lines (+372/-15)
9 files modified
HACKING.txt (+3/-0)
config.yaml (+22/-0)
examples/pyflakes.yaml (+1/-1)
hooks/config-changed (+22/-7)
hooks/install (+1/-1)
hooks/local.py (+121/-0)
hooks/start (+0/-1)
hooks/stop (+5/-2)
hooks/tests.py (+197/-3)
To merge this branch: bzr merge lp:~bac/charms/oneiric/buildbot-master/history-s3
Reviewer Review Type Date Requested Status
Gary Poster (community) Approve
Yellow Squad code Pending
Review via email: mp+94262@code.launchpad.net

Description of the change

Add ability to store buildbot data files to S3 and retrieve them the next time the master is started.

The configuration settings for access-key and secret-key need to be passed, like so:

juju set buildbot-master access-key='<your AWS key>' secret-key='<your AWS secret key>'

Unfortunately the 'stop' hook is not being called (see bug 872264) so automatically saving the history when bringing down the juju service does not work. As a work-around a new config value is provided which causes the files to get pushed. Setting that config variable to a different value will cause it to save again, e.g.

juju set buildbot-master save-history-now='`date`'

To post a comment you must log in.
Revision history for this message
Gary Poster (gary) wrote :

Brad, this looks great! Nicely done, and nice to have helpers that can do this for us with future charms.

As I mentioned on IRC, please clean up the log messages in handle_config_changes one way or another, so that we no longer have a colon and two messages when one would do.

Making a chdir context manager would be nice. We have one or two hanging around. You mentioned you had found one in setuplxc.

I asked whether tar xvf will have the expanded file or the existing file win: you said the expanded one, which is good.

You pointed out there are no tests. If there's a sane way to write them then that would be great.

Thank you!

Gary

review: Approve
34. By Brad Crittenden

Added unit tests of history management functions

Revision history for this message
Gary Poster (gary) wrote :

Hi Brad. That looks good.

You asked about the amount of code needed to stub boto. I am very happy we have it, and I wonder if it ought to be put somewhere else to make it reusable--later, perhaps.

For now, though, the only idea I have is that it might be nice to put the boto setup/teardown stuff in a separate file, just to make the test file easier to read. Just an idea.

Meanwhile though, I approve it again!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'HACKING.txt'
2--- HACKING.txt 2012-02-07 17:20:47 +0000
3+++ HACKING.txt 2012-02-23 16:49:43 +0000
4@@ -21,6 +21,9 @@
5 4) Run the tests: RESOLVE_TEST_CHARMS=1 tests/buildbot-master.test
6
7
8+XXX bac 2012-02-23: The tests do not run locally using precise. Set
9+default-series: oneiric to get them to pass.
10+
11 Running the charm helper tests
12 ==============================
13
14
15=== modified file 'config.yaml'
16--- config.yaml 2012-02-10 21:19:58 +0000
17+++ config.yaml 2012-02-23 16:49:43 +0000
18@@ -52,3 +52,25 @@
19 install the newly specified packages while leaving the previous
20 ones installed.
21 type: string
22+ access-key:
23+ description: |
24+ Access key for EC2.
25+ type: string
26+ secret-key:
27+ description: |
28+ Secret key for EC2.
29+ type: string
30+ bucket-name:
31+ description: |
32+ The bucket used to store buildbot history. If not provided a
33+ default based on the access-key will be used.
34+ type: string
35+ save-history-now:
36+ description: |
37+ Configuration hack to fire off the saving of the buildbot master
38+ history. Normally this would be done in the stop hook but due to
39+ Bug 872264 that hook is not firing properly. The value of the
40+ setting is not important but it must change between invocations
41+ or the event will not be recogized. Monotonically increasing
42+ integer values would be a good choice. Or a time string.
43+ type: string
44
45=== modified file 'examples/pyflakes.yaml'
46--- examples/pyflakes.yaml 2012-02-10 21:19:58 +0000
47+++ examples/pyflakes.yaml 2012-02-23 16:49:43 +0000
48@@ -1,5 +1,5 @@
49 buildbot-master:
50- extra-packages: git
51+ extra-packages: git python-sqlalchemy python-migrate
52 installdir: /tmp/buildbot
53 config-file: |
54 # -*- python -*-
55
56=== modified file 'hooks/config-changed'
57--- hooks/config-changed 2012-02-14 15:51:49 +0000
58+++ hooks/config-changed 2012-02-23 16:49:43 +0000
59@@ -3,14 +3,17 @@
60 # Copyright 2012 Canonical Ltd. This software is licensed under the
61 # GNU Affero General Public License version 3 (see the file LICENSE).
62
63+import base64
64 import json
65 import os
66 import os.path
67 import shutil
68+import subprocess
69 import sys
70
71 from helpers import (
72 apt_get_install,
73+ cd,
74 command,
75 DictDiffer,
76 get_config,
77@@ -26,7 +29,11 @@
78 buildbot_create,
79 buildbot_reconfig,
80 config_json,
81+ fetch_history,
82 generate_string,
83+ get_bucket,
84+ get_key,
85+ put_history,
86 slave_json,
87 )
88
89@@ -36,7 +43,6 @@
90 ]
91 SUPPORTED_TRANSPORTS = ['bzr']
92
93-
94 bzr = command('bzr')
95
96
97@@ -57,8 +63,6 @@
98
99 def handle_config_changes(config, diff):
100 log('Updating buildbot configuration.')
101- log('Configuration changes seen:')
102- log(str(diff))
103
104 buildbot_pkg = config.get('buildbot-pkg')
105 extra_repo = config.get('extra-repository')
106@@ -91,7 +95,6 @@
107
108 # Write the buildbot config to disk (fetching it if necessary).
109 added_or_changed = diff.added_or_changed
110- log("CONFIG FILE: {}".format(config_file))
111 log("ADDED OR CHANGED: {}".format(added_or_changed))
112 if config_file and 'config-file' in added_or_changed:
113 buildbot_create(buildbot_dir)
114@@ -108,7 +111,7 @@
115 # gpg-agent needs to send the key over and the bzr launchpad-login
116 # needs to be set.
117 with su('buildbot'):
118- lp_id = config.get('config-usr')
119+ lp_id = config.get('config-user')
120 if lp_id:
121 bzr('launchpad-login', lp_id)
122 private_key = config.get('config-private-key')
123@@ -153,12 +156,21 @@
124 if not os.path.exists(placeholder_path):
125 with open(placeholder_path, 'w') as f:
126 json.dump(generate_string("temporary-placeholder-"), f)
127- buildbot_reconfig()
128+ try:
129+ buildbot_reconfig()
130+ except subprocess.CalledProcessError as e:
131+ print e
132+ print e.output
133+ raise
134+
135+
136+def conditionally_save_history(config, diff):
137+ if 'save-history-now' in diff.added_or_changed:
138+ put_history(config)
139
140
141 def main():
142 config = get_config()
143- log(str(config))
144 if not check_config(config):
145 log("Configuration not valid.")
146 sys.exit(1)
147@@ -178,6 +190,7 @@
148 os.makedirs(buildbot_dir)
149
150 restart_required |= configure_buildbot(config, diff)
151+ restart_required |= fetch_history(config, diff)
152
153 master_cfg_path = os.path.join(buildbot_dir, 'master.cfg')
154 if restart_required and os.path.exists(master_cfg_path):
155@@ -185,6 +198,8 @@
156 else:
157 log("Configuration changed but didn't require restarting.")
158
159+ conditionally_save_history(config, diff)
160+
161 config_json.set(config)
162
163
164
165=== modified file 'hooks/install'
166--- hooks/install 2012-02-14 16:54:21 +0000
167+++ hooks/install 2012-02-23 16:49:43 +0000
168@@ -22,7 +22,7 @@
169
170
171 def cleanup(buildbot_dir):
172- apt_get_install('bzr')
173+ apt_get_install('bzr', 'python-boto')
174 # Since we may be installing into a pre-existing service, ensure the
175 # buildbot directory is removed.
176 if os.path.exists(buildbot_dir):
177
178=== modified file 'hooks/local.py'
179--- hooks/local.py 2012-02-10 01:05:09 +0000
180+++ hooks/local.py 2012-02-23 16:49:43 +0000
181@@ -10,16 +10,22 @@
182 'buildslave_stop',
183 'config_json',
184 'create_slave',
185+ 'fetch_history',
186 'generate_string',
187+ 'get_bucket',
188+ 'get_key',
189 'HTTP_PORT_PROTOCOL',
190+ 'put_history',
191 'slave_json',
192 ]
193
194 import os
195+import re
196 import subprocess
197 import uuid
198
199 from helpers import (
200+ cd,
201 get_config,
202 log,
203 run,
204@@ -157,3 +163,118 @@
205
206 slave_json = Serializer('/tmp/slave_info.json')
207 config_json = Serializer('/tmp/config.json')
208+
209+
210+def get_bucket(config, bucket_name=None):
211+ """Return an S3 bucket or None."""
212+ # Late import to ensure python-boto package has been installed.
213+ import boto
214+ access_key = config.get('access-key')
215+ secret_key = config.get('secret-key')
216+ bucket = None
217+ if access_key and secret_key:
218+ if bucket_name is None:
219+ bucket_name = str(access_key + '-buildbot-history').lower()
220+ conn = boto.connect_s3(access_key, secret_key)
221+ bucket = conn.create_bucket(bucket_name)
222+ log("Using bucket: " + bucket.name)
223+ return bucket
224+
225+
226+def get_key(bucket):
227+ """Return an S3 key for the bucket."""
228+ # Late import to ensure python-boto package has been installed.
229+ from boto.s3.key import Key
230+ key = Key(bucket)
231+ value = os.environ['JUJU_UNIT_NAME']
232+ key.key = value
233+ log("Using key: " + key.key)
234+ return key
235+
236+
237+VERSION_TO_STORE = {
238+ '0.7': "*/builder",
239+ '0.8': "state.sqlite",
240+ }
241+
242+
243+def get_buildbot_version():
244+ """Get the major version (x.y) of buildbot and return as a string.
245+
246+ Return None if the output from buildbot cannot be parsed.
247+ """
248+ version = None
249+ output = run('buildbot', '--version')
250+ match = re.search('Buildbot version: (\d+\.\d+)(\.\d+)*\n', output)
251+ if match and len(match.groups()) > 0:
252+ version = match.group(1)
253+ return version
254+
255+
256+def put_history(config):
257+ """Put the buildbot history to an external store, if set up."""
258+ log("put_history called")
259+ bucket_name = config.get('bucket-name')
260+ bucket = get_bucket(config, bucket_name)
261+ success = False
262+ if bucket:
263+ key = get_key(bucket)
264+ target = '/tmp/history-put.tgz'
265+ version = get_buildbot_version()
266+ store_pattern = VERSION_TO_STORE.get(version)
267+ assert store_pattern is not None, (
268+ "Buildbot version not supported: {}".format(version))
269+ with cd(config['installdir']):
270+ with su('buildbot'):
271+ try:
272+ run('tar', 'czf', target, store_pattern)
273+ key.set_contents_from_filename(target)
274+ # If would be natural to just log the success here, but we
275+ # are su-ed to the buildbot user and that causes permission
276+ # problems, so instead set a flag.
277+ success = True
278+ except subprocess.CalledProcessError as e:
279+ print e
280+ print e.output
281+ raise
282+ os.unlink(target)
283+ else:
284+ log("Bucket not found: " + bucket_name)
285+ if success:
286+ log("History stored to S3.")
287+
288+
289+def fetch_history(config, diff):
290+ """Fetch the buildbot history from an external store."""
291+ # Currently only S3 is supported.
292+ restart_required = False
293+
294+ log("fetching history")
295+ if 'secret-key' not in diff.added_or_changed:
296+ log("skipping fetch of history")
297+ return restart_required
298+
299+ bucket_name = config.get('bucket-name')
300+ bucket = get_bucket(config, bucket_name)
301+
302+ if bucket:
303+ key = get_key(bucket)
304+ if key.exists():
305+ target = '/tmp/history-fetched.tgz'
306+ key.get_contents_to_filename(target)
307+ with cd(config['installdir']):
308+ with su('buildbot'):
309+ try:
310+ run('tar', 'xzf', target)
311+ except subprocess.CalledProcessError as e:
312+ print e
313+ print e.output
314+ raise
315+ os.unlink(target)
316+ log("History fetched from S3.")
317+ restart_required = True
318+ else:
319+ log("Key does not exist: " + key.key)
320+ else:
321+ log("Bucket not found: " + bucket_name)
322+ return restart_required
323
324=== modified file 'hooks/start'
325--- hooks/start 2012-02-08 14:26:58 +0000
326+++ hooks/start 2012-02-23 16:49:43 +0000
327@@ -4,7 +4,6 @@
328 # GNU Affero General Public License version 3 (see the file LICENSE).
329
330 from helpers import (
331- log,
332 log_entry,
333 log_exit,
334 run,
335
336=== modified file 'hooks/stop'
337--- hooks/stop 2012-02-08 14:26:58 +0000
338+++ hooks/stop 2012-02-23 16:49:43 +0000
339@@ -10,14 +10,17 @@
340 log_exit,
341 run,
342 )
343-from local import HTTP_PORT_PROTOCOL
344+from local import (
345+ HTTP_PORT_PROTOCOL,
346+ put_history,
347+ )
348
349
350 def main():
351 config = get_config()
352 log('Stopping buildbot in {}'.format(config['installdir']))
353+ put_history(config)
354 run('close-port', HTTP_PORT_PROTOCOL)
355-
356 log('Finished stopping buildbot')
357
358
359
360=== modified file 'hooks/tests.py'
361--- hooks/tests.py 2012-02-10 19:43:46 +0000
362+++ hooks/tests.py 2012-02-23 16:49:43 +0000
363@@ -1,16 +1,32 @@
364+# Copyright 2012 Canonical Ltd. This software is licensed under the
365+# GNU Affero General Public License version 3 (see the file LICENSE).
366+
367+"""Helper functions for writing hooks in python."""
368+
369+__metaclass__ = type
370+
371+
372+from contextlib import contextmanager
373 import os
374 from subprocess import CalledProcessError
375+import tempfile
376 import unittest
377
378 from helpers import (
379+ cd,
380+ command,
381 DictDiffer,
382- command,
383 run,
384 su,
385 unit_info,
386 )
387+from local import (
388+ get_bucket,
389+ get_key,
390+ fetch_history,
391+ put_history,
392+ )
393
394-from subprocess import CalledProcessError
395
396 class TestRun(unittest.TestCase):
397
398@@ -24,7 +40,7 @@
399 # produces a string.
400 self.assertIn('Usage:', run('/bin/ls', '--help'))
401
402- def testSimpleCommand(self):
403+ def testCalledProcessErrorRaised(self):
404 # If an error occurs a CalledProcessError is raised with the return
405 # code, command executed, and the output of the command.
406 with self.assertRaises(CalledProcessError) as info:
407@@ -205,5 +221,183 @@
408 self.assertEqual(current_home, os.environ['HOME'])
409
410
411+class TestCdContextManager(unittest.TestCase):
412+ def test_cd(self):
413+ curdir = os.getcwd()
414+ self.assertNotEqual('/var', curdir)
415+ with cd('/var'):
416+ self.assertEqual('/var', os.getcwd())
417+ self.assertEqual(curdir, os.getcwd())
418+
419+
420+class StubBucket:
421+ """Stub S3 Bucket."""
422+ def __init__(self, name):
423+ self.name = name
424+ self.keys = {}
425+
426+
427+class StubKey:
428+ """Stub S3 Key."""
429+ def __init__(self, bucket):
430+ self.bucket = bucket
431+ self._name = None
432+
433+ def _get_key(self):
434+ return self._name
435+ def _set_key(self, name):
436+ if name not in self.bucket.keys:
437+ self.bucket.keys[name] = None
438+ self._name = name
439+ key = property(_get_key, _set_key)
440+
441+ def _get_value(self):
442+ return self.bucket.keys.get(self._name)
443+ def _set_value(self, v):
444+ self.bucket.keys[self._name] = v
445+ value = property(_get_value, _set_value)
446+
447+ def exists(self):
448+ return self.value is not None
449+
450+ def get_contents_to_filename(self, fn):
451+ with open(fn, 'w') as f:
452+ f.write(self.value)
453+
454+ def set_contents_from_filename(self, fn):
455+ with open(fn, 'r') as f:
456+ self.value = f.read()
457+
458+
459+class StubConnection:
460+ """A stub S3 connection."""
461+ def __init__(self, access_key, secret_key):
462+ self.buckets = {}
463+
464+ def create_bucket(self, name):
465+ if name in self.buckets:
466+ bucket = self.buckets[name]
467+ else:
468+ bucket = StubBucket(name)
469+ self.buckets[name] = bucket
470+ return bucket
471+
472+
473+connection = None
474+
475+
476+def stub_connect_s3(access_key, secret_key):
477+ # A stub boto connection factory.
478+ global connection
479+ if connection is None:
480+ connection = StubConnection(access_key, secret_key)
481+ return connection
482+
483+
484+def stub_log(msg):
485+ pass
486+
487+
488+@contextmanager
489+def stub_su(user):
490+ yield
491+
492+
493+class TestHistoryManagement(unittest.TestCase):
494+
495+ def setUp(self):
496+ self.config = {'access-key': 'fake-access-key',
497+ 'secret-key': 'fake-secret-key',
498+ 'bucket-name': 'fake-bucket-name',
499+ 'installdir': '/tmp',
500+ }
501+ import local
502+ import boto
503+ import boto.s3.key
504+ self.connect_s3 = boto.connect_s3
505+ self.Key = boto.s3.key.Key
506+ self.log = local.log
507+ self.get_buildbot_version = local.get_buildbot_version
508+ self.su = local.su
509+ boto.connect_s3 = stub_connect_s3
510+ boto.s3.key.Key = StubKey
511+ local.log = stub_log
512+ local.get_buildbot_version = lambda: '0.8'
513+ local.su = stub_su
514+ self.tempfile = tempfile.NamedTemporaryFile(delete=False)
515+ self.tempfilename = self.tempfile.name
516+ self.tempfile.close()
517+ os.environ['JUJU_UNIT_NAME'] = 'fake-unit-name'
518+
519+ def tearDown(self):
520+ import local
521+ import boto
522+ import boto.s3.key
523+ boto.connect_s3 = self.connect_s3
524+ boto.s3.key.Key = self.Key
525+ local.log = self.log
526+ local.get_buildbot_version = self.get_buildbot_version
527+ local.su = self.su
528+ os.unlink(self.tempfilename)
529+
530+ def test_get_bucket(self):
531+ bucket = get_bucket(self.config, 'fake-bucket')
532+ self.assertEqual('fake-bucket', bucket.name)
533+
534+ def test_get_key(self):
535+ bucket = get_bucket(self.config, 'fake-bucket')
536+ key = get_key(bucket)
537+ self.assertEqual('fake-bucket', key.bucket.name)
538+ self.assertEqual('fake-unit-name', key.key)
539+ self.assertFalse(key.exists())
540+
541+ def test_key_content_file(self):
542+ contents = "Fake file contents, yo."
543+ with open(self.tempfilename, 'w') as f:
544+ f.write(contents)
545+ bucket = get_bucket(self.config, 'fake-bucket')
546+ key = get_key(bucket)
547+ key.set_contents_from_filename(self.tempfilename)
548+ self.assertTrue(key.exists())
549+ self.assertEqual(contents, key.value)
550+ os.unlink(self.tempfilename)
551+ key.get_contents_to_filename(self.tempfilename)
552+ with open(self.tempfilename, 'r') as f:
553+ value = f.read()
554+ self.assertEqual(contents, value)
555+
556+ def test_put_fetch_history(self):
557+ # Tests putting the history file into the fake bucket and then
558+ # retrieving it.
559+ fake_contents = "Fake state.sqlite"
560+ history_fn = os.path.join(self.config['installdir'], 'state.sqlite')
561+ with open(history_fn, 'w') as f:
562+ f.write(fake_contents)
563+ put_history(self.config)
564+ os.unlink(history_fn)
565+ diff = DictDiffer(self.config, {})
566+ restart = fetch_history(self.config, diff)
567+ self.assertTrue(restart)
568+ with open(history_fn, 'r') as f:
569+ contents = f.read()
570+ self.assertEqual(fake_contents, contents)
571+
572+ def test_fetch_history_no_restart(self):
573+ diff = DictDiffer(self.config, self.config)
574+ restart = fetch_history(self.config, diff)
575+ self.assertFalse(restart)
576+
577+ def test_fetch_history_no_credentials(self):
578+ self.config['access-key'] = None
579+ diff = DictDiffer(self.config, {})
580+ restart = fetch_history(self.config, diff)
581+ self.assertFalse(restart)
582+
583+ def test_fetch_history_no_key(self):
584+ diff = DictDiffer(self.config, {})
585+ restart = fetch_history(self.config, diff)
586+ self.assertFalse(restart)
587+
588+
589 if __name__ == '__main__':
590 unittest.main()

Subscribers

People subscribed via source and target branches

to all changes: