Merge lp:~doanac/uci-engine/lander-code-layout into lp:uci-engine

[Andy Doan]

lander charm: reorganize code layout and improve install logic

This does a couple of things that were hard to do piece by piece. The
change is a bit big but:

The main goal was getting our code to be in its own directory so it
can start to be treated like read-only. We have an idea in the future
of code rollbacks, so this fits in with that model. This now deploys
code to:

 /srv/<service>/code/r<revno>_<sha1>

 and we maintain a symlink to the latest code with:

  /srv/<service>/code/current

While doing this I got rid of all the bzr-branch logic we no longer
use. This reduces complexity.

I'll also converted our tarball logic to use charmhelpers.fetch.

While changing so much, I also decided to improve our code-extraction
logic by moving it to our config-changed logic. This means that
we now have the ability to update code by just changing the payload
value of the service.

I originally worked on this for the restish-charm. However, thomi is landing an MP that would be broken by this:

 https://code.launchpad.net/~doanac/uci-engine/restish-code-layout/+merge/224520

While waiting, I copied the logic from that MP to give us the same ability in the lander.

This gives us the ability to update/change code for a service with simple "juju set" commands.

ev - has created an asana task that would expand upon this allowing specific units to be at specific code levels. In the interest of a somewhat readable MP, I'd like to do that as follow-up work on this.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Andy Doan (doanac)]

gonna - make a couple of minor changes after discussion with cprov and fginther

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Celso Providelo (cprov)]

Andy,

I am worried about the lack of abstraction to access disk-resources in lander, but that's not enough to block this change to land and experiment "upgrading" with the new code layout.

Hopefully accessing lander juju-relations and lander-logs will get clearer later.

review: Approve

[Ubuntu CI Bot (uci-bot)]

The attempt to merge lp:~doanac/uci-engine/lander-code-layout into lp:uci-engine failed. Below is the output from the failed tests.

Running cm...
Updating source dependencies...
Updating source dependencies...
Updating source dependencies...
Updating source dependencies...
Updating source dependencies...
Updating source dependencies...
Updating source dependencies...
uploading webui-content.tgz to swift
Checking juju status
Preparing local branch upload...
Uploading local branch, fingerprint 096f5a75d81880fb5db22111f1b6b8872cff7209
Private PPAs only is DISABLED (CI_PRIVATE_PPAS_ONLY: 0)
Building charm: chroot-builder
Building charm: rabbitmq-worker
Building charm: lander
Building charm: system-image-server
Building charm: python-django
Building charm: restish
Building charm: webui
Running juju-deployer -v -c /tmp/tmpGx9DVq/deployer/branch-source-builder.yaml -c /tmp/tmpGx9DVq/deployer/gatekeeper.yaml -c /tmp/tmpGx9DVq/deployer/image-builder.yaml -c /tmp/tmpGx9DVq/deployer/lander.yaml -c /tmp/tmpGx9DVq/deployer/nf-stats-service.yaml -c /tmp/tmpGx9DVq/deployer/ppa-assigner.yaml -c /tmp/tmpGx9DVq/deployer/production-only.yaml -c /tmp/tmpGx9DVq/deployer/publisher.yaml -c /tmp/tmpGx9DVq/deployer/relations.yaml -c /tmp/tmpGx9DVq/deployer/test-runner.yaml -c /tmp/tmpGx9DVq/deployer/ticket-system.yaml -c /tmp/tmpGx9DVq/deployer/webui.yaml ci-airline
Tests running...
ci-utils.ci_utils.tests.test_amqp.TestAMQP.testConnectFailed ... OK (0.001 secs)
ci-utils.ci_utils.tests.test_amqp.TestAMQP.testProcessQueue ... OK (0.002 secs)
ci-utils.ci_utils.tests.test_amqp.TestAMQP.testRunForever ... OK (0.102 secs)
ci-utils.ci_utils.tests.test_amqp.TestAMQP.testSent ... OK (0.002 secs)
ci-utils.ci_utils.tests.test_amqp.TestProgressTrigger.testProgress ... OK (0.001 secs)
ci-utils.ci_utils.tests.test_amqp_worker.TestAMQPWorker.testCancel ... OK (0.108 secs)
ci-utils.ci_utils.tests.test_amqp_worker.TestAMQPWorker.testNoQueue ... OK (0.002 secs)
ci-utils.ci_utils.tests.test_amqp_worker.TestAMQPWorker.testNoTicket ... OK (0.003 secs)
ci-utils.ci_utils.tests.test_amqp_worker.TestAMQPWorker.testOnMessageCalledProcessError ... OK (0.003 secs)
ci-utils.ci_utils.tests.test_amqp_worker.TestAMQPWorker.testOnMessageFail ... OK (0.002 secs)
ci-utils.ci_utils.tests.test_amqp_worker.TestAMQPWorker.testOnMessageKilled ... OK (0.003 secs)
ci-utils.ci_utils.tests.test_amqp_worker.TestAMQPWorker.testOnMessageSimple ... OK (0.002 secs)
ci-utils.ci_utils.tests.test_amqp_worker.TestAMQPWorker.testOnMessageUnexpected ... OK (0.003 secs)
ci-utils.ci_utils.tests.test_amqp_worker.TestTimer.testCBRuns ... OK (0.021 secs)
ci-utils.ci_utils.tests.test_amqp_worker.TestTimer.testCanCancel ... OK (0.000 secs)
ci-utils.ci_utils.tests.test_data_store.TestDataStoreConfig.test_invalid_auth_config ... OK (0.000 secs)
ci-utils.ci_utils.tests.test_data_store.TestDataStoreConfig.test_valid_auth_config ... OK (0.000 secs)
ci-utils.ci_utils.tests.test_data_store.TestDataStoreFileName.test_get_file_name ... OK (0.000 secs)
ci-utils.ci_utils.tests.test_data_store.TestDataStoreFileName.test_get_file_name_same_directory ... OK (0.000 secs)
ci-utils.ci_utils.tests.test_data_store.TestDataStoreFileName.test_get...

[Evan (ev)]

======================================================================
FAIL: testing.test_style.TestPep8.test_pep8_conformance
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/dev/shm/tmpH5DVmw/local/lib/python2.7/site-packages/ucitests-0.1.3-py2.7.egg/ucitests/styles.py", line 84, in test_pep8_conformance
    '\n'.join(self.report._msgs))
  File "/usr/lib/python2.7/unittest/case.py", line 515, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/lib/python2.7/unittest/case.py", line 744, in assertListEqual
    self.assertSequenceEqual(list1, list2, msg, seq_type=list)
  File "/usr/lib/python2.7/unittest/case.py", line 726, in assertSequenceEqual
    self.fail(msg)
  File "/usr/lib/python2.7/unittest/case.py", line 412, in fail
    raise self.failureException(msg)
AssertionError: bin/../testing/run_tests.py:418:1: W293 blank line contains whitespace

[Francis Ginther (fginther)]

I had the same comment as Celso, these ../../../../ paths are a hard to keep straight. But I agree this isn't the place to address that.

Also, a good chunk of the code in lander/hooks/hooks.py looks like it would be used in other charms as well. Does this eventually belong in our own charm helpers? I really have no good suggestions for addressing this given how charms work other then copy/paste.

review: Approve

[Andy Doan (doanac)]

On 07/09/2014 11:48 AM, Francis Ginther wrote:
> Also, a good chunk of the code in lander/hooks/hooks.py looks like it would be used in other charms as well. Does this eventually belong in our own charm helpers? I really have no good suggestions for addressing this given how charms work other then copy/paste.

yeah- this sucks. we could consolidate a lot of charm code. if we had
our own charm helper.

[lp:~doanac/uci-engine/lander-code-layout]

635. By Andy Doan on 2014-07-09

merge with trunk

636. By Andy Doan on 2014-07-09

fix pep8 issue to get tarmac working again

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

=== modified file 'charms/precise/lander/charm-helpers.yaml'
--- charms/precise/lander/charm-helpers.yaml	2014-01-30 11:47:15 +0000
+++ charms/precise/lander/charm-helpers.yaml	2014-07-09 16:56:25 +0000
@@ -9,3 +9,4 @@
 include:
   - core
   - fetch
+  - payload.archive

=== modified file 'charms/precise/lander/config.yaml'
--- charms/precise/lander/config.yaml	2014-05-01 08:34:49 +0000
+++ charms/precise/lander/config.yaml	2014-07-09 16:56:25 +0000
@@ -1,17 +1,12 @@
 options:
-  branch:
-    type: string
-    description: "BZR branch the service lives in"
-  revno:
-    type: string
-    description: "Revision or tag to branch from"
-  tarball:
+  current_code:
     type: string
     description: A URL to a tarball of the code
-  vcs:
-    default: "branch"
+  available_code:
     type: string
-    description: "Grab code from a bzr 'branch' or a URL to a 'tarball'"
+    description: |
+      A list of code deployment URLs to keep around. Any versions found locally
+      not in this list will be removed.
   install_root:
     type: string
     default: "/srv/"

=== modified file 'charms/precise/lander/hooks/hooks.py'
--- charms/precise/lander/hooks/hooks.py	2014-06-30 12:07:54 +0000
+++ charms/precise/lander/hooks/hooks.py	2014-07-09 16:56:25 +0000
@@ -16,8 +16,8 @@
 
 import base64
 import os
+import re
 import shutil
-import subprocess
 import sys
 import textwrap
 
@@ -47,6 +47,10 @@
     return os.path.join(core.hookenv.config()['install_root'], unit)
 
 
+def _code_dir(revno='current'):
+    return os.path.join(_service_dir(), 'code/%s' % revno)
+
+
 def _upstart_log_dir():
     return os.path.join(_service_dir(), 'logs')
 
@@ -71,56 +75,71 @@
         fetch.apt_install(package, fatal=True)
 
 
-def _install_from_tarball(config, upgrade):
-    juju_info('grabbing service from tarball...')
-    sdir = _service_dir()
-    if not upgrade:
-        if os.path.exists(sdir):
-            juju_info('deleting pre-existing service directory: %s' % sdir)
-            shutil.rmtree(sdir)
-        os.mkdir(sdir)
-    cmd = 'curl %s | tar -xzC %s' % (config['tarball'], sdir)
-    subprocess.check_call(cmd, shell=True)
-
-
-def _install_from_bzr(config, upgrade):
-    juju_info('grabbing service from bzr...')
-
-    sdir = _service_dir()
-    rev = config.get('revno', '')
-
-    if upgrade:
-        args = ['bzr', 'pull']
-        if rev:
-            args.extend(['-r', rev])
-        subprocess.check_call(args, cwd=sdir)
-        return
-
-    if os.path.exists(sdir):
-        juju_info('deleting pre-existing service directory: %s' % sdir)
-        shutil.rmtree(sdir)
-
-    args = ['bzr', 'branch']
-    if rev:
-        args.extend(['-r', rev])
-    args.append(config['branch'])
-    args.append(sdir)
-    subprocess.check_call(args)
-
-
[e-mail]()
-def install(upgrade=False):
+def _get_revno(src):
+    revno = os.path.basename(src).split('.tgz')[0]
+    revfile = os.path.join(src, '.uploaded_version_info')
+    if os.path.exists(revfile):
+        # revline will be: branch@617 - 2014-06-25 11:50:44 -0500
+        revline = open(revfile).read()
+        match = re.match(r'.*@(\d+)', revline)
+        if match:
+            revno = 'r' + match.group(1) + '_' + revno
+    return revno
+
+
+def _payload_local(payload):
+    '''Figure out if the payload is already available locally'''
+    name = payload.rsplit('/', 1)[1]
+    name = name.split('.tgz')[0]
+    for dirname in os.listdir(_code_dir('')):
+        if dirname == 'current':
+            continue
+        if name == dirname.split('_', 1)[1]:
+            return dirname
+
+
+def _payload_extract(payload):
+    juju_info('grabbing %s' % payload)
+    src = fetch.install_remote(payload)
+
+    revno = _get_revno(src)
+
+    core.host.mkdir(_code_dir('..'))
+
+    # charmhelpers puts the payload in an odd place, move it to our code_dir
+    dst = _code_dir(revno)
+    shutil.move(src, dst)
+
+
+def _payloads_not_configured(avail):
+    '''returns a list of payloads no longer configured for the charm'''
+    configured_dirs = [_payload_local(x) for x in avail]
+    all_dirs = [x for x in os.listdir(_code_dir('')) if x != 'current']
+    return set(all_dirs) - set(configured_dirs)
+
+
+def handle_code():
+    '''deploy all new payloads configured for the charm. return list of
+       payloads to cleanup.'''
     config = core.hookenv.config()
-    if config.get('vcs') == 'tarball':
-        _install_from_tarball(config, upgrade)
-    else:
-        _install_from_bzr(config, upgrade)
-
-
[e-mail]()
-def upgrade_charm():
-    install(True)
-    restart_upstart()
+    avail = [x for x in config['available_code'].split(' ') if x.strip()]
+    new_current = config['current_code']
+    if new_current not in avail:
+        raise ValueError(
+            '"current_code" not configured to a value in "available_code"')
+
+    for payload in avail:
+        if not _payload_local(payload):
+            _payload_extract(payload)
+
+    current = _code_dir()
+    if os.path.exists(current):
+        os.unlink(current)
+    dst = _payload_local(new_current)
+    juju_info('linking %s as current' % dst)
+    os.symlink(dst, current)
+
+    return _payloads_not_configured(avail)
 
 
 def create_unit_config():
@@ -147,7 +166,7 @@
 
     env JUJU_UNIT_IDX={idx}
 
-    chdir {sdir}
+    chdir {cdir}
 
     # setuid/setgid don't work because we need to do mkdir as root
     # this is the easiest work-around:
@@ -161,7 +180,7 @@
     ''')
     params = {
         'main': config['main'],
-        'sdir': _service_dir(),
+        'cdir': _code_dir(),
         'uid': config.get('uid', 'nobody'),
         'gid': config.get('gid', 'nogroup'),
         'idx': core.hookenv.local_unit().split('/')[1],
@@ -184,11 +203,18 @@
 
 @hooks.hook()
 def config_changed():
+    core.host.mkdir(_code_dir(''))
+
     add_dependencies()
     create_unit_config()
     create_upstart()
+    todelete = handle_code()
+
     restart_upstart()
 
+    for x in todelete:
+        shutil.rmtree(_code_dir(x))
+
 
 @hooks.hook()
 @hooks.hook('lander-relation-changed')

=== removed symlink 'charms/precise/lander/hooks/install'
=== target was u'hooks.py'
=== removed symlink 'charms/precise/lander/hooks/upgrade-charm'
=== target was u'hooks.py'
=== modified file 'charms/precise/lander/unit_tests/test_hooks.py'
--- charms/precise/lander/unit_tests/test_hooks.py	2014-06-26 20:02:54 +0000
+++ charms/precise/lander/unit_tests/test_hooks.py	2014-07-09 16:56:25 +0000
@@ -12,7 +12,6 @@
 
 # You should have received a copy of the GNU Affero General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
-import json
 import mock
 import os
 import unittest
@@ -47,6 +46,7 @@
     'charmhelpers.core.host.mkdir',
     'charmhelpers.fetch.apt_install',
     'charmhelpers.fetch.configure_sources',
+    'charmhelpers.fetch.install_remote',
 ]
 
 
@@ -83,6 +83,53 @@
         self.config.return_value['install_root'] = self.tmpdir
 
 
+class TestInstallHook(JujuTestCase):
+    def setUp(self):
+        super(TestInstallHook, self).setUp()
+        m = mock.patch('hooks._service_dir')
+        m.start().return_value = self.tmpdir
+        self.addCleanup(m.stop)
+        os.mkdir(os.path.join(self.tmpdir, 'code'))
+
+    def test_payload_local(self):
+        self.assertIsNone(hooks._payload_local('http://no_such_sha1.tgz'))
+        name = 'r123_sha1hash'
+        os.mkdir(hooks._code_dir(name))
+        self.assertEqual(name, hooks._payload_local('http://sha1hash.tgz'))
+
+    def test_payload_not_configured(self):
+        local = ['r1_aa', 'r2_bb', 'r3_cc', 'r4_dd']
+        configured = ['http://aa.tgz', 'http://cc.tgz', 'http://dd.tgz']
+        os.mkdir(hooks._code_dir())  # create a directory for "current"
+        for x in local:
+            os.mkdir(hooks._code_dir(x))
+        not_configured = hooks._payloads_not_configured(configured)
+        self.assertEquals(not_configured, set(['r2_bb']))
+
+    @mock.patch('hooks._payload_extract')
+    def test_handle_code_noop(self, payload_extract):
+        local = ['r1_aa', 'r2_bb']
+        self.config.return_value['current_code'] = 'http://bb.tgz'
+        self.config.return_value['available_code'] = '''
+            http://aa.tgz http://bb.tgz'''
+        for x in local:
+            os.mkdir(hooks._code_dir(x))
+        hooks.handle_code()
+        self.assertEqual(0, payload_extract.call_count)
+        self.assertEqual('r2_bb', os.readlink(hooks._code_dir()))
+
+    @mock.patch('hooks._payload_extract')
+    def test_handle_code(self, payload_extract):
+        self.config.return_value['current_code'] = 'http://bb.tgz'
+        self.config.return_value['available_code'] = 'http://bb.tgz'
+
+        payload_extract.side_effect = lambda x: os.mkdir(
+            hooks._code_dir('r2_bb'))
+        self.assertEqual(set([]), hooks.handle_code())
+        self.assertEqual(1, payload_extract.call_count)
+        self.assertEqual('r2_bb', os.readlink(hooks._code_dir()))
+
+
 class TestLanderHook(JujuTestCase):
     def setUp(self):
         super(TestLanderHook, self).setUp()

=== modified file 'ci-utils/ci_utils/amqp_utils.py'
--- ci-utils/ci_utils/amqp_utils.py	2014-06-16 13:18:53 +0000
+++ ci-utils/ci_utils/amqp_utils.py	2014-07-09 16:56:25 +0000
@@ -45,8 +45,10 @@
     '''Load the rabbit config created by the charm'''
     config = None
     try:
-        # try and find the config file, should be in the root of our bzr branch
-        f = os.path.join(HERE, '../../amqp_config.py')
+        f = os.path.join(HERE, '../../../../amqp_config.py')
+        # TODO remove once all charms have converted to new code layout
+        if not os.path.exists(f):
+            f = os.path.join(HERE, '../../amqp_config.py')
         if os.path.exists(f):
             import imp
             config = imp.load_source('amqp_config', f)

=== modified file 'juju-deployer/configs/lander_http_vhost'
--- juju-deployer/configs/lander_http_vhost	2014-06-23 19:14:23 +0000
+++ juju-deployer/configs/lander_http_vhost	2014-07-09 16:56:25 +0000
@@ -10,7 +10,7 @@
      Allow from all
   </Directory>
 
-  ScriptAlias /json_status /srv/ci-airline-lander/lander/bin/json_status_cgi.py
+  ScriptAlias /json_status /srv/ci-airline-lander/code/current/lander/bin/json_status_cgi.py
   <Location /json_status>
      Options +ExecCGI
      Allow from all

=== modified file 'juju-deployer/lander.yaml.tmpl'
--- juju-deployer/lander.yaml.tmpl	2014-06-16 13:18:53 +0000
+++ juju-deployer/lander.yaml.tmpl	2014-07-09 16:56:25 +0000
@@ -16,9 +16,8 @@
         ci-airline-lander:
             charm: lander
             options:
-                vcs: ${CI_CODE_SOURCE}
-                branch: ${CI_BRANCH}
-                tarball: ${CI_PAYLOAD_URL}
+                current_code: ${CI_PAYLOAD_URL}
+                available_code: ${CI_PAYLOAD_URL}
                 sources: ${CI_PPA}
                 packages: "python-amqplib python-swiftclient lazr.enum"
                 unit-config: include-base64://configs/unit_config.yaml

=== modified file 'lander/bin/json_status_cgi.py'
--- lander/bin/json_status_cgi.py	2014-06-30 12:07:54 +0000
+++ lander/bin/json_status_cgi.py	2014-07-09 16:56:25 +0000
@@ -33,7 +33,7 @@
     # Service names sometimes change, and this is hard to find and know to
     # fix otherwise
     service = os.path.abspath(
-        os.path.join(os.path.dirname(__file__), '../../'))
+        os.path.join(os.path.dirname(__file__), '../../../../'))
     service = os.path.basename(service)
     state = subprocess.check_output(
         ['/sbin/status', service], stderr=subprocess.STDOUT)
@@ -47,7 +47,7 @@
 status.add_true_false('lander worker', state, 'start/running' in state)
 status.add_deployed_revision()
 
-relations_dir = '../../juju-relations'
+relations_dir = '../../../../juju-relations'
 exist = os.path.exists(relations_dir)
 if exist:
     ppa = ts = False

=== modified file 'lander/lander/run_worker.py'
--- lander/lander/run_worker.py	2014-06-30 12:07:54 +0000
+++ lander/lander/run_worker.py	2014-07-09 16:56:25 +0000
@@ -29,14 +29,14 @@
 from ci_utils import amqp_utils, data_store, dump_stack, unit_config
 
 _here = os.path.abspath(os.path.dirname(__file__))
-SERVICE_PATH = os.path.join(_here, '../../juju-relations')
+SERVICE_PATH = os.path.join(_here, '../../../../juju-relations')
 PROCESS_TICKET = os.path.join(_here, '../bin/lander_process_ticket.py')
 
 logger = lander.get_logger('lander_next_ticket')
 
 
 def _get_parser():
-    logdir = os.path.join(_here, '../../lander-logs')
+    logdir = os.path.join(_here, '../../../../lander-logs')
     parser = argparse.ArgumentParser(
         description='Wraps the REST API calls with a progress queue monitor.')
     parser.add_argument('--service-name',

=== modified file 'testing/run_tests.py'
--- testing/run_tests.py	2014-07-09 12:49:37 +0000
+++ testing/run_tests.py	2014-07-09 16:56:25 +0000
@@ -415,7 +415,7 @@
             '--no-skip', action='store_true',
             dest='no_skip',
             help='Errors if at lest one test is skipped.')
-        
+
 
 def main(args, stdout, stderr):
     # Interpret user wishes