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

[Brad Crittenden]

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`'

[Gary Poster (gary)]

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

[lp:~bac/charms/oneiric/buildbot-master/history-s3]

34. By Brad Crittenden on 2012-02-23

Added unit tests of history management functions

[Gary Poster (gary)]

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

=== modified file 'HACKING.txt'
--- HACKING.txt	2012-02-07 17:20:47 +0000
+++ HACKING.txt	2012-02-23 16:49:43 +0000
@@ -21,6 +21,9 @@
 4) Run the tests: RESOLVE_TEST_CHARMS=1 tests/buildbot-master.test
 
 
+XXX bac 2012-02-23: The tests do not run locally using precise.  Set
+default-series: oneiric to get them to pass.
+
 Running the charm helper tests
 ==============================
 
 
=== modified file 'config.yaml'
--- config.yaml	2012-02-10 21:19:58 +0000
+++ config.yaml	2012-02-23 16:49:43 +0000
@@ -52,3 +52,25 @@
       install the newly specified packages while leaving the previous
       ones installed.
     type: string
+  access-key:
+    description: |
+      Access key for EC2.
+    type: string
+  secret-key:
+    description: |
+      Secret key for EC2.
+    type: string
+  bucket-name:
+    description: |
+      The bucket used to store buildbot history.  If not provided a
+      default based on the access-key will be used.
+    type: string
+  save-history-now:
+    description: |
+      Configuration hack to fire off the saving of the buildbot master
+      history.  Normally this would be done in the stop hook but due to
+      Bug 872264 that hook is not firing properly.  The value of the
+      setting is not important but it must change between invocations
+      or the event will not be recogized.  Monotonically increasing
+      integer values would be a good choice.  Or a time string.
+    type: string
 
=== modified file 'examples/pyflakes.yaml'
--- examples/pyflakes.yaml	2012-02-10 21:19:58 +0000
+++ examples/pyflakes.yaml	2012-02-23 16:49:43 +0000
@@ -1,5 +1,5 @@
 buildbot-master:
-  extra-packages: git
+  extra-packages: git python-sqlalchemy python-migrate
   installdir: /tmp/buildbot
   config-file: |
     # -*- python -*-
 
=== modified file 'hooks/config-changed'
--- hooks/config-changed	2012-02-14 15:51:49 +0000
+++ hooks/config-changed	2012-02-23 16:49:43 +0000
@@ -3,14 +3,17 @@
 # Copyright 2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+import base64
 import json
 import os
 import os.path
 import shutil
+import subprocess
 import sys
 
 from helpers import (
     apt_get_install,
+    cd,
     command,
     DictDiffer,
     get_config,
@@ -26,7 +29,11 @@
     buildbot_create,
     buildbot_reconfig,
     config_json,
+    fetch_history,
     generate_string,
+    get_bucket,
+    get_key,
+    put_history,
     slave_json,
     )
 
@@ -36,7 +43,6 @@
     ]
 SUPPORTED_TRANSPORTS = ['bzr']
 
-
 bzr = command('bzr')
 
 
@@ -57,8 +63,6 @@
 
 def handle_config_changes(config, diff):
     log('Updating buildbot configuration.')
-    log('Configuration changes seen:')
-    log(str(diff))
 
     buildbot_pkg = config.get('buildbot-pkg')
     extra_repo = config.get('extra-repository')
@@ -91,7 +95,6 @@
 
     # Write the buildbot config to disk (fetching it if necessary).
     added_or_changed = diff.added_or_changed
-    log("CONFIG FILE: {}".format(config_file))
     log("ADDED OR CHANGED: {}".format(added_or_changed))
     if config_file and 'config-file' in added_or_changed:
         buildbot_create(buildbot_dir)
@@ -108,7 +111,7 @@
         # gpg-agent needs to send the key over and the bzr launchpad-login
         # needs to be set.
         with su('buildbot'):
-            lp_id = config.get('config-usr')
+            lp_id = config.get('config-user')
             if lp_id:
                 bzr('launchpad-login', lp_id)
             private_key = config.get('config-private-key')
@@ -153,12 +156,21 @@
         if not os.path.exists(placeholder_path):
             with open(placeholder_path, 'w') as f:
                 json.dump(generate_string("temporary-placeholder-"), f)
-    buildbot_reconfig()
+    try:
+        buildbot_reconfig()
+    except subprocess.CalledProcessError as e:
+        print e
+        print e.output
+        raise
+
+
+def conditionally_save_history(config, diff):
+    if 'save-history-now' in diff.added_or_changed:
+        put_history(config)
 
 
 def main():
     config = get_config()
-    log(str(config))
     if not check_config(config):
         log("Configuration not valid.")
         sys.exit(1)
@@ -178,6 +190,7 @@
             os.makedirs(buildbot_dir)
 
     restart_required |= configure_buildbot(config, diff)
+    restart_required |= fetch_history(config, diff)
 
     master_cfg_path = os.path.join(buildbot_dir, 'master.cfg')
     if restart_required and os.path.exists(master_cfg_path):
@@ -185,6 +198,8 @@
     else:
         log("Configuration changed but didn't require restarting.")
 
+    conditionally_save_history(config, diff)
+
     config_json.set(config)
 
 
 
=== modified file 'hooks/install'
--- hooks/install	2012-02-14 16:54:21 +0000
+++ hooks/install	2012-02-23 16:49:43 +0000
@@ -22,7 +22,7 @@
 
 
 def cleanup(buildbot_dir):
-    apt_get_install('bzr')
+    apt_get_install('bzr', 'python-boto')
     # Since we may be installing into a pre-existing service, ensure the
     # buildbot directory is removed.
     if os.path.exists(buildbot_dir):
 
=== modified file 'hooks/local.py'
--- hooks/local.py	2012-02-10 01:05:09 +0000
+++ hooks/local.py	2012-02-23 16:49:43 +0000
@@ -10,16 +10,22 @@
     'buildslave_stop',
     'config_json',
     'create_slave',
+    'fetch_history',
     'generate_string',
+    'get_bucket',
+    'get_key',
     'HTTP_PORT_PROTOCOL',
+    'put_history',
     'slave_json',
     ]
 
 import os
+import re
 import subprocess
 import uuid
 
 from helpers import (
+    cd,
     get_config,
     log,
     run,
@@ -157,3 +163,118 @@
 
 slave_json = Serializer('/tmp/slave_info.json')
 config_json = Serializer('/tmp/config.json')
+
+
+def get_bucket(config, bucket_name=None):
+    """Return an S3 bucket or None."""
+    # Late import to ensure python-boto package has been installed.
+    import boto
+    access_key = config.get('access-key')
+    secret_key = config.get('secret-key')
+    bucket = None
+    if access_key and secret_key:
+        if bucket_name is None:
+            bucket_name = str(access_key + '-buildbot-history').lower()
+        conn = boto.connect_s3(access_key, secret_key)
+        bucket = conn.create_bucket(bucket_name)
+        log("Using bucket: " + bucket.name)
+    return bucket
+
+
+def get_key(bucket):
+    """Return an S3 key for the bucket."""
+    # Late import to ensure python-boto package has been installed.
+    from boto.s3.key import Key
+    key = Key(bucket)
+    value = os.environ['JUJU_UNIT_NAME']
+    key.key = value
+    log("Using key: " + key.key)
+    return key
+
+
+VERSION_TO_STORE = {
+    '0.7': "*/builder",
+    '0.8': "state.sqlite",
+    }
+
+
+def get_buildbot_version():
+    """Get the major version (x.y) of buildbot and return as a string.
+
+    Return None if the output from buildbot cannot be parsed.
+    """
+    version = None
+    output = run('buildbot', '--version')
+    match = re.search('Buildbot version: (\d+\.\d+)(\.\d+)*\n', output)
+    if match and len(match.groups()) > 0:
+        version = match.group(1)
+    return version
+
+
+def put_history(config):
+    """Put the buildbot history to an external store, if set up."""
+    log("put_history called")
+    bucket_name = config.get('bucket-name')
+    bucket = get_bucket(config, bucket_name)
+    success = False
+    if bucket:
+        key = get_key(bucket)
+        target = '/tmp/history-put.tgz'
+        version = get_buildbot_version()
+        store_pattern = VERSION_TO_STORE.get(version)
+        assert store_pattern is not None, (
+            "Buildbot version not supported: {}".format(version))
+        with cd(config['installdir']):
+            with su('buildbot'):
+                try:
+                    run('tar', 'czf', target, store_pattern)
+                    key.set_contents_from_filename(target)
+                    # If would be natural to just log the success here, but we
+                    # are su-ed to the buildbot user and that causes permission
+                    # problems, so instead set a flag.
+                    success = True
+                except subprocess.CalledProcessError as e:
+                    print e
+                    print e.output
+                    raise
+                os.unlink(target)
+    else:
+        log("Bucket not found: " + bucket_name)
+    if success:
+        log("History stored to S3.")
+
+
+def fetch_history(config, diff):
+    """Fetch the buildbot history from an external store."""
+    # Currently only S3 is supported.
+    restart_required = False
+
+    log("fetching history")
+    if 'secret-key' not in diff.added_or_changed:
+        log("skipping fetch of history")
+        return restart_required
+
+    bucket_name = config.get('bucket-name')
+    bucket = get_bucket(config, bucket_name)
+
+    if bucket:
+        key = get_key(bucket)
+        if key.exists():
+            target = '/tmp/history-fetched.tgz'
+            key.get_contents_to_filename(target)
+            with cd(config['installdir']):
+                with su('buildbot'):
+                    try:
+                        run('tar', 'xzf', target)
+                    except subprocess.CalledProcessError as e:
+                        print e
+                        print e.output
+                        raise
+            os.unlink(target)
+            log("History fetched from S3.")
+            restart_required = True
+        else:
+            log("Key does not exist: " + key.key)
+    else:
+        log("Bucket not found: " + bucket_name)
+    return restart_required
 
=== modified file 'hooks/start'
--- hooks/start	2012-02-08 14:26:58 +0000
+++ hooks/start	2012-02-23 16:49:43 +0000
@@ -4,7 +4,6 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from helpers import (
-    log,
     log_entry,
     log_exit,
     run,
 
=== modified file 'hooks/stop'
--- hooks/stop	2012-02-08 14:26:58 +0000
+++ hooks/stop	2012-02-23 16:49:43 +0000
@@ -10,14 +10,17 @@
     log_exit,
     run,
     )
-from local import HTTP_PORT_PROTOCOL
+from local import (
+    HTTP_PORT_PROTOCOL,
+    put_history,
+    )
 
 
 def main():
     config = get_config()
     log('Stopping buildbot in {}'.format(config['installdir']))
+    put_history(config)
     run('close-port', HTTP_PORT_PROTOCOL)
-
     log('Finished stopping buildbot')
 
 
 
=== modified file 'hooks/tests.py'
--- hooks/tests.py	2012-02-10 19:43:46 +0000
+++ hooks/tests.py	2012-02-23 16:49:43 +0000
@@ -1,16 +1,32 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Helper functions for writing hooks in python."""
+
+__metaclass__ = type
+
+
+from contextlib import contextmanager
 import os
 from subprocess import CalledProcessError
+import tempfile
 import unittest
 
 from helpers import (
+    cd,
+    command,
     DictDiffer,
-    command,
     run,
     su,
     unit_info,
     )
+from local import (
+    get_bucket,
+    get_key,
+    fetch_history,
+    put_history,
+    )
 
-from subprocess import CalledProcessError
 
 class TestRun(unittest.TestCase):
 
@@ -24,7 +40,7 @@
         # produces a string.
         self.assertIn('Usage:', run('/bin/ls', '--help'))
 
-    def testSimpleCommand(self):
+    def testCalledProcessErrorRaised(self):
         # If an error occurs a CalledProcessError is raised with the return
         # code, command executed, and the output of the command.
         with self.assertRaises(CalledProcessError) as info:
@@ -205,5 +221,183 @@
             self.assertEqual(current_home, os.environ['HOME'])
 
 
+class TestCdContextManager(unittest.TestCase):
+    def test_cd(self):
+        curdir = os.getcwd()
+        self.assertNotEqual('/var', curdir)
+        with cd('/var'):
+            self.assertEqual('/var', os.getcwd())
+        self.assertEqual(curdir, os.getcwd())
+
+
+class StubBucket:
+    """Stub S3 Bucket."""
+    def __init__(self, name):
+        self.name = name
+        self.keys = {}
+
+
+class StubKey:
+    """Stub S3 Key."""
+    def __init__(self, bucket):
+        self.bucket = bucket
+        self._name = None
+
+    def _get_key(self):
+        return self._name
+    def _set_key(self, name):
+        if name not in self.bucket.keys:
+            self.bucket.keys[name] = None
+        self._name = name
+    key = property(_get_key, _set_key)
+
+    def _get_value(self):
+        return self.bucket.keys.get(self._name)
+    def _set_value(self, v):
+        self.bucket.keys[self._name] = v
+    value = property(_get_value, _set_value)
+
+    def exists(self):
+        return self.value is not None
+
+    def get_contents_to_filename(self, fn):
+        with open(fn, 'w') as f:
+            f.write(self.value)
+
+    def set_contents_from_filename(self, fn):
+        with open(fn, 'r') as f:
+            self.value = f.read()
+
+
+class StubConnection:
+    """A stub S3 connection."""
+    def __init__(self, access_key, secret_key):
+        self.buckets = {}
+
+    def create_bucket(self, name):
+        if name in self.buckets:
+            bucket = self.buckets[name]
+        else:
+            bucket = StubBucket(name)
+            self.buckets[name] = bucket
+        return bucket
+
+
+connection = None
+
+
+def stub_connect_s3(access_key, secret_key):
+    # A stub boto connection factory.
+    global connection
+    if connection is None:
+        connection = StubConnection(access_key, secret_key)
+    return connection
+
+
+def stub_log(msg):
+    pass
+
+
+@contextmanager
+def stub_su(user):
+    yield
+
+
+class TestHistoryManagement(unittest.TestCase):
+
+    def setUp(self):
+        self.config = {'access-key': 'fake-access-key',
+                       'secret-key': 'fake-secret-key',
+                       'bucket-name': 'fake-bucket-name',
+                       'installdir': '/tmp',
+                       }
+        import local
+        import boto
+        import boto.s3.key
+        self.connect_s3 = boto.connect_s3
+        self.Key = boto.s3.key.Key
+        self.log = local.log
+        self.get_buildbot_version = local.get_buildbot_version
+        self.su = local.su
+        boto.connect_s3 = stub_connect_s3
+        boto.s3.key.Key = StubKey
+        local.log = stub_log
+        local.get_buildbot_version = lambda: '0.8'
+        local.su = stub_su
+        self.tempfile = tempfile.NamedTemporaryFile(delete=False)
+        self.tempfilename = self.tempfile.name
+        self.tempfile.close()
+        os.environ['JUJU_UNIT_NAME'] = 'fake-unit-name'
+
+    def tearDown(self):
+        import local
+        import boto
+        import boto.s3.key
+        boto.connect_s3 = self.connect_s3
+        boto.s3.key.Key = self.Key
+        local.log = self.log
+        local.get_buildbot_version = self.get_buildbot_version
+        local.su = self.su
+        os.unlink(self.tempfilename)
+
+    def test_get_bucket(self):
+        bucket = get_bucket(self.config, 'fake-bucket')
+        self.assertEqual('fake-bucket', bucket.name)
+
+    def test_get_key(self):
+        bucket = get_bucket(self.config, 'fake-bucket')
+        key = get_key(bucket)
+        self.assertEqual('fake-bucket', key.bucket.name)
+        self.assertEqual('fake-unit-name', key.key)
+        self.assertFalse(key.exists())
+
+    def test_key_content_file(self):
+        contents = "Fake file contents, yo."
+        with open(self.tempfilename, 'w') as f:
+            f.write(contents)
+        bucket = get_bucket(self.config, 'fake-bucket')
+        key = get_key(bucket)
+        key.set_contents_from_filename(self.tempfilename)
+        self.assertTrue(key.exists())
+        self.assertEqual(contents, key.value)
+        os.unlink(self.tempfilename)
+        key.get_contents_to_filename(self.tempfilename)
+        with open(self.tempfilename, 'r') as f:
+            value = f.read()
+        self.assertEqual(contents, value)
+
+    def test_put_fetch_history(self):
+        # Tests putting the history file into the fake bucket and then
+        # retrieving it.
+        fake_contents = "Fake state.sqlite"
+        history_fn = os.path.join(self.config['installdir'], 'state.sqlite')
+        with open(history_fn, 'w') as f:
+            f.write(fake_contents)
+        put_history(self.config)
+        os.unlink(history_fn)
+        diff = DictDiffer(self.config, {})
+        restart = fetch_history(self.config, diff)
+        self.assertTrue(restart)
+        with open(history_fn, 'r') as f:
+            contents = f.read()
+        self.assertEqual(fake_contents, contents)
+
+    def test_fetch_history_no_restart(self):
+        diff = DictDiffer(self.config, self.config)
+        restart = fetch_history(self.config, diff)
+        self.assertFalse(restart)
+
+    def test_fetch_history_no_credentials(self):
+        self.config['access-key'] = None
+        diff = DictDiffer(self.config, {})
+        restart = fetch_history(self.config, diff)
+        self.assertFalse(restart)
+
+    def test_fetch_history_no_key(self):
+        diff = DictDiffer(self.config, {})
+        restart = fetch_history(self.config, diff)
+        self.assertFalse(restart)
+
+
 if __name__ == '__main__':
     unittest.main()