Merge lp:~adeuring/charms/precise/juju-reports/juju-reports-new-revno-in-new-dir into lp:~juju-qa/charms/precise/juju-reports/trunk

[Abel Deuring]

A (partial) fix for bug 1357390 and 1357391. The assumption is that these bugs occur when a a new juju-reports revision is installed: Some libraries needed by cron script are not available during the upgrade.

The main idea of this change: When a new revision is set, this revision is stored in a new directory, named juju-reports-REVNO. When "make prodinstall" finishes, a symlink named juju-reports is created that points to juju-reports-REVNO.

This works fine for revno upgrades, but it does not address another issue: When the charm config variable "dist-clean" is true, setting any config value will still cause cronscript failures until "make prodinstall" finishes.

This could be fixed in a follow-up branch, which could probably also move the stop() call in update_from_config() closer to the start() call.

[Aaron Bentley (abentley)]

Why use a symlink? Why not just rename the new project dir into place?

review: Needs Fixing

[Abel Deuring (adeuring)]

If we rename the directoy, many scripts will break. As an example from a test run, the script bin/cbtest:

#!/home/ubuntu/juju-reports-170/bin/python
# EASY-INSTALL-ENTRY-SCRIPT: 'jujureports==0.0','console_scripts','cbtest'
__requires__ = 'jujureports==0.0'
import sys
from pkg_resources import load_entry_point

if __name__ == '__main__':
    sys.exit(
        load_entry_point('jujureports==0.0', 'console_scripts', 'cbtest')()
    )

[lp:~adeuring/charms/precise/juju-reports/juju-reports-new-revno-in-new-dir]

51. By Abel Deuring on 2014-11-20

Use a new source directory in each call of update_source(); exception-based logic to distiguish between PROJECT_DIR being a directory or a symlink or not existing.

52. By Abel Deuring on 2014-11-20

reduce downtime during a config-changed hook: Call stop() immediaetly before make_project_link()

[Abel Deuring (adeuring)]

Another small change that became possible because update_soruce() now always uses a new directory: stop() can be called much later: Just before make_project_link(). This reduces the downtime when the config-changed hook is running.

[Aaron Bentley (abentley)]

Thanks for explaining why we need to use symlinks here.

Thanks for switching to EAFP. I disagree that Look Before You Leap vs Easier to Ask Forgiveness than permission is simply a matter of taste; EAFP is more correct because it does not have race conditions, and typically more efficient. In the symlink case, the symlink could be moved or deleted between the os.path.lexists, os.path.islink and os.readlink calls, so EAFP is less correct. It also requires three times as many system calls, so it is less efficient.

Now, it's true that two extra system calls don't really matter. But using three times as many system calls in order to be less correct does not make sense to me.

It also looks like you're pulling the branch from scratch, instead of taking advantage of the data in the pre-existing branch. That's going to make updates slower. Instead, you could use a shared repository, or you could branch from the old branch instead of using "bzr init".

I am also concerned that update_source is now going to do work for every config change. It was intended to only do work when the branch revision changed. Do we need to start by comparing revision-ids at the top of update_source and exiting early if there's no change?

review: Needs Fixing

[lp:~adeuring/charms/precise/juju-reports/juju-reports-new-revno-in-new-dir]

53. By Abel Deuring on 2014-11-20

make_project_link(): use OSError.errno to figure out the exact error condition.

54. By Abel Deuring on 2014-11-21

Run install_production() only when the revno or the branch URL changes.

55. By Abel Deuring on 2014-11-21

Use a shared repository for the juju-reports branches.

56. By Abel Deuring on 2014-11-21

Do not remove an existing directory /juju-reports in the install hook.

[Abel Deuring (adeuring)]

The charm now uses a shared repository. I've also added some logic to check if the revno has changed between two calls of the config-changed hook: The current revno and parent branch URL are stored a file "last-source-revision"; update_source() and install_production() are now called only when the current config has other branch URL/revo values than before.

The config parameter "dist-clean" is has not real usage in the strict sense since juju-reports is now always built in a new directory; I've added logic so that update_source() and install_production() are always called when dist-clean is true. Similary, the revno value -1 cause a new build in each config-chaned run. (This should be improved: We can check if the parent branch has a new revision.)

Finally I noticed that the install hook still removed the directory $home/juju-reports. This is done in make_project_link(), so I removed it.

[lp:~adeuring/charms/precise/juju-reports/juju-reports-new-revno-in-new-dir]

57. By Abel Deuring on 2014-11-21

do not automatically re-install juju-reports when the revno is set to -1

[Aaron Bentley (abentley)]

I think it is simpler and more accurate to check revision-id, as I suggested in my review, rather than revno and branch.

It is simpler because Bazaar tracks revision-id itself, so you don't need to add new config files. It is more accurate because changes to revno and branch do not necessarily imply that revision-id has changed. source can change to a different location with the same branch contents. revno can change from -1 to a positive value and still refer to the same revision-id. And when branch and revno are unchanged, this does not guarantee that revision-id is unchanged; if revno is -1, it will change as the branch is updated.

So I don't think this is the right way to handle this, but I also don't think this will cause practical problems because
1. we don't use -1 in production
2. we don't change source in production, and even if we did, it would just mean one extra period of downtime.

review: Approve

=== modified file 'hooks/common.py'
--- hooks/common.py	2014-11-10 14:31:29 +0000
+++ hooks/common.py	2014-11-21 16:47:34 +0000
@@ -4,10 +4,10 @@
 import errno
 import os
 from pwd import getpwnam
-import re
-from StringIO import StringIO
+import shutil
 import subprocess
-import sys
+from tempfile import mkdtemp
+import yaml
 
 from Cheetah.Template import Template
 
@@ -18,28 +18,29 @@
 HOME = '/home/ubuntu'
 PROJECT_DIR = os.path.join(HOME, 'juju-reports')
 INI = 'production.ini'
-
-
-def get_ini_path():
-    return os.path.join(PROJECT_DIR, INI)
-
-
-def get_ini():
+REVISION_INFO_FILE = os.path.join(HOME, 'last-source-revision')
+
+
+def get_ini_path(source_path):
+    return os.path.join(source_path, INI)
+
+
+def get_ini(source_path):
     config = ConfigParser()
-    config.read(get_ini_path())
+    config.read(get_ini_path(source_path))
     return config
 
 
-def get_template_path(name):
-    return os.path.join(PROJECT_DIR, 'resources/{}.template'.format(name))
+def get_template_path(name, source_path):
+    return os.path.join(source_path, 'resources/{}.template'.format(name))
 
 
 def install_ini(mongo_url, gh_token, jujuci_url, token, error_email,
-                error_email_from, cloud_health_failure_threshold):
+                error_email_from, cloud_health_failure_threshold, source_path):
     config = ConfigParser()
-    path = get_template_path(INI)
+    path = get_template_path(INI, source_path)
     if not os.path.exists(path):
-        path = get_ini_path()
+        path = get_ini_path(source_path)
     config.read(path)
     config.set('app:main', 'mongo.url', mongo_url)
     config.set('app:main', 'gh_token', gh_token)
@@ -51,12 +52,12 @@
     config.set(
         'app:main', 'cloud_health_failure_threshold',
         cloud_health_failure_threshold)
-    config.write(open(get_ini_path(), 'w'))
+    config.write(open(get_ini_path(source_path), 'w'))
     return config
 
 
-def configure_s3(config):
-    template_path = get_template_path('juju-qa.s3cfg')
+def configure_s3(config, source_path):
+    template_path = get_template_path('juju-qa.s3cfg', source_path)
     try:
         template_cm = open(template_path)
     except IOError as e:
@@ -65,7 +66,7 @@
             return
     with template_cm as template_file:
         template = template_file.read()
-    config_path = os.path.join(PROJECT_DIR, 'juju-qa.s3cfg')
+    config_path = os.path.join(source_path, 'juju-qa.s3cfg')
     s3config = template.format(access_key=config['aws-access-key'],
                                secret_key=config['aws-secret-key'])
     update_file(config_path, s3config)
@@ -99,9 +100,9 @@
         hookenv.close_port(port)
 
 
-def call_script(script_path, root=False):
+def call_script(script_path, source_path, root=False):
     env = os.environ.copy()
-    ini = get_ini_path()
+    ini = get_ini_path(source_path)
     env['HOME'] = HOME
     env['INI'] = ini
     if root:
@@ -110,20 +111,20 @@
         args = ['su', 'ubuntu', '-p', '-c']
 
     args.append(script_path)
-    subprocess.check_call(args, env=env, cwd=PROJECT_DIR)
+    subprocess.check_call(args, env=env, cwd=source_path)
     return env
 
 
 def stop():
     try:
-        call_script('scripts/stop', root=True)
+        call_script('scripts/stop', PROJECT_DIR, root=True)
     except OSError as e:
         if e.errno != errno.ENOENT:
             raise
 
 
-def start():
-    call_script('scripts/run')
+def start(source_path):
+    call_script('scripts/run', source_path)
 
 
 def open_secret(path, uid=None, gid=None):
@@ -152,26 +153,39 @@
 
 
 def update_source(source_url, revno):
-    # Make it easier to develop on a deploy
-    call_bzr(['bind', '-d', PROJECT_DIR, source_url])
-
     # Grab the code
+    repository = os.path.join(HOME, 'juju-reports-repo')
+    pwd = getpwnam('ubuntu')
+    try:
+        os.mkdir(repository)
+    except OSError as e:
+        if e.errno != errno.EEXIST:
+            raise
+        pass
+    else:
+        call_bzr(['init-repo', repository])
+        os.chown(repository, pwd.pw_uid, pwd.pw_gid)
+    target_dir = mkdtemp(prefix='juju-reports-%s-' % revno, dir=repository)
     revno = 'revno:%s' % revno
+    os.chown(target_dir, pwd.pw_uid, pwd.pw_gid)
+    call_bzr(['init', target_dir])
+    os.chown(target_dir, pwd.pw_uid, pwd.pw_gid)
+
+    # Make it easier to develop on a deploy
+    call_bzr(['bind', '-d', target_dir, source_url])
 
     bzr_cmd = ['pull', '--overwrite', source_url, '-r', revno, '-d',
-               PROJECT_DIR, '--remember', '--local']
+               target_dir, '--remember', '--local']
     call_bzr(bzr_cmd)
-
-    # Restore the templates if they are missing/altered.
-    call_bzr(['revert', os.path.join(PROJECT_DIR, 'resources')])
-
-
-def install_production(dist_clean, develop):
-    subprocess.check_call(['make', 'deploydeps'], cwd=PROJECT_DIR)
+    return target_dir
+
+
+def install_production(dist_clean, develop, source_path):
+    subprocess.check_call(['make', 'deploydeps'], cwd=source_path)
     if dist_clean:
-        subprocess.check_call(['make', 'distclean'], cwd=PROJECT_DIR)
+        subprocess.check_call(['make', 'distclean'], cwd=source_path)
     target = 'install' if develop else 'prodinstall'
-    subprocess.check_call(['make', target], cwd=PROJECT_DIR)
+    subprocess.check_call(['make', target], cwd=source_path)
     subprocess.check_call(['apt-get', 'autoremove', '--yes'])
 
 
@@ -260,28 +274,73 @@
         fileobj.write(new_contents)
 
 
+def make_project_link(source_path, branch_url, revno):
+    try:
+        old_sources = os.readlink(PROJECT_DIR)
+    except OSError as err:
+        if err.errno == errno.EINVAL:
+            shutil.rmtree(PROJECT_DIR)
+            old_sources = None
+        elif err.errno == errno.ENOENT:
+            old_sources = None
+        else:
+            raise
+    else:
+        if old_sources == source_path:
+            return
+        os.remove(PROJECT_DIR)
+    os.symlink(source_path, PROJECT_DIR)
+    if old_sources is not None:
+        shutil.rmtree(old_sources)
+    with open(REVISION_INFO_FILE, 'w') as f:
+        yaml.dump((branch_url, revno), f)
+
+
+def get_last_revision():
+    try:
+        file_context = open(REVISION_INFO_FILE, 'r')
+    except IOError as e:
+        if e.errno == errno.ENOENT:
+            return None, None
+        else:
+            raise
+    with file_context:
+        return yaml.load(file_context)
+
+
 def update_from_config():
-    ini = get_ini()
+    ini = get_ini(PROJECT_DIR)
     mongo_url = get_mongo_url()
     config = hookenv.config()
     set_port(ini, False)
-    stop()
     configure_lp(config['lp-key'])
     if config['source'] == '':
         hookenv.log('Incomplete config: source')
         return
-    update_source(config['source'], config['revno'])
-    oauth_path = os.path.join(PROJECT_DIR, 'jujureports/lp_credentials.txt')
+    last_source_url, last_revision = get_last_revision()
+    needs_install_update = (
+        (last_source_url, last_revision) != (config['source'], config['revno'])
+        or config['dist-clean'])
+    if needs_install_update:
+        source_path = update_source(
+            config['source'], config['revno'])
+    else:
+        source_path = PROJECT_DIR
+    oauth_path = os.path.join(source_path, 'jujureports/lp_credentials.txt')
     update_file(oauth_path, config['lp-oauth'] + '\n')
-    install_production(config['dist-clean'], config['develop-install'])
+    if needs_install_update:
+        install_production(
+            config['dist-clean'], config['develop-install'], source_path)
     if mongo_url == '':
         hookenv.log('No mongodb set up.')
+        if needs_install_update:
+            make_project_link(source_path, config['source'], config['revno'])
         return
     ini = install_ini(mongo_url, config['gh-token'], config['jujuci-url'],
                       config['charm-bundle-test-token'],
                       config['error-email'], config['error-email-from'],
-                      config['cloud-health-failure-threshold'])
-    configure_s3(config)
+                      config['cloud-health-failure-threshold'], source_path)
+    configure_s3(config, source_path)
     unset = [key for key in [
         'lp-key', 'lp-oauth', 'gh-token', 'charm-bundle-test-token',
         'error-email', 'error-email-from'
@@ -292,6 +351,9 @@
         update_cronjob(None)
     else:
         install_cronjob(config['cron-interval'], config['error-email'])
-    start()
+    stop()
+    if needs_install_update:
+        make_project_link(source_path, config['source'], config['revno'])
+    start(source_path)
     set_port(ini, True)
     update_website(ini)
 
=== modified file 'hooks/install'
--- hooks/install	2014-08-25 14:31:05 +0000
+++ hooks/install	2014-11-21 16:47:34 +0000
@@ -7,11 +7,6 @@
 home="/home/ubuntu"
 project_dir="$home/juju-reports"
 
-# Set up the project directory
-if [ -d $project_dir ]; then
-    rm -rf $project_dir
-fi
-mkdir -p $project_dir
 mkdir -p $home/logs
 chown ubuntu:ubuntu $home/logs
 
@@ -31,10 +26,6 @@
 cat templates/known_hosts >> /home/ubuntu/.ssh/known_hosts
 chown ubuntu:ubuntu /home/ubuntu/.ssh/known_hosts
 
-# Get the code.
-cd $project_dir
-bzr init
-chown ubuntu:ubuntu $project_dir
 if [ -f /tmp/app.log ]; then
     chown ubuntu:ubuntu /tmp/app.log
 fi