yellow team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Brad Crittenden has proposed merging lp:~bac/charms/precise/juju-gui/config-changed into lp:~juju-gui/charms/precise/juju-gui/trunk.

Requested reviews:
  Juju GUI Hackers (juju-gui)
Related bugs:
  Bug #1086507 in juju-gui: "Charm should have a config-changed hook"
  https://bugs.launchpad.net/juju-gui/+bug/1086507

For more details, see:
https://code.launchpad.net/~bac/charms/precise/juju-gui/config-changed/+merge/140316

Add support for config-changed

The config-changed hook has been added. In order to share code from the
install hook, common routines were moved to utils. Those routines have unit
tests now, except for 'build' which didn't look like much.

Many of the functions in utils were modified for testability where they needed
to write files.

There is an issue lurking. Using 'juju set juju-gui juju-gui-branch=lp:<some
branch> seems to leave nginx in an odd state. It is started but it cannot
find the document root so it may be a race condition. I'll continue to hunt
for the problem while this cut is in review.
-- 
https://code.launchpad.net/~bac/charms/precise/juju-gui/config-changed/+merge/140316
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~bac/charms/precise/juju-gui/config-changed into lp:~juju-gui/charms/precise/juju-gui/trunk.

=== added file '.bzrignore'
--- .bzrignore	1970-01-01 00:00:00 +0000
+++ .bzrignore	2012-12-17 22:56:21 +0000
@@ -0,0 +1,1 @@
+.emacs*

=== added file 'hooks/config-changed'
--- hooks/config-changed	1970-01-01 00:00:00 +0000
+++ hooks/config-changed	2012-12-17 22:56:21 +0000
@@ -0,0 +1,130 @@
+#!/usr/bin/env python2
+# -*- python -*-
+
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+import os.path
+from shutil import rmtree
+from subprocess import CalledProcessError
+import sys
+
+from charmhelpers import (
+    get_config,
+    log,
+    log_entry,
+    log_exit,
+    service_control,
+    START,
+    STOP,
+)
+from shelltoolbox import (
+    DictDiffer,
+    su,
+    )
+
+from utils import (
+    AGENT,
+    build,
+    cmd_log,
+    config_json,
+    fetch,
+    GUI,
+    IMPROV,
+    start_agent,
+    start_gui,
+    start_improv,
+    stop,
+    )
+
+
+def handle_config_changes(config, diff):
+    # Handle all configuration file changes.
+    log('Updating configuration.')
+
+    staging = config.get('staging')
+    staging_environment = config.get('staging-environment')
+    juju_api_port = config.get('juju-api-port')
+    added_or_changed = diff.added_or_changed
+
+    # Fetch new branches?
+    gui_branch = None
+    api_branch = None
+    if 'juju-gui-branch' in added_or_changed:
+        if os.path.exists('juju-gui'):
+            rmtree('juju-gui')
+        gui_branch = config['juju-gui-branch']
+    if 'juju-api-branch' in added_or_changed:
+        if os.path.exists('juju'):
+            rmtree('juju')
+        api_branch = config['juju-api-branch']
+    if gui_branch or api_branch:
+        fetch(gui_branch, api_branch)
+        build(config['command-log-file'])
+        # Restarting of the gui and api services is handled below.
+
+    # Handle changes to the improv server configuration.
+    if staging:
+        staging_properties = set(
+            ['staging', 'staging-environment', 'juju-api-port'])
+        if bool(added_or_changed & staging_properties) or api_branch is not None:
+            if 'staging' in added_or_changed:
+                # 'staging' went from False to True, so the agent server is
+                # running and must be stopped.
+                current_api = AGENT
+            else:
+                # Only staging parameters changed, so the existing staging
+                # server must be stopped and later restarted.
+                current_api = IMPROV
+            log('Stopping %s.' % current_api)
+            service_control(current_api, STOP)
+
+            # Now the improv server can be cleanly started.
+            log('Starting or restarting improv')
+            start_improv(juju_api_port, staging_environment)
+    else:
+        agent_properties = set(
+            ['juju-api-port', 'staging'])
+        if bool(added_or_changed & agent_properties) or api_branch is not None:
+            if 'staging' in added_or_changed:
+                # If 'staging' transitions to False we need to stop the backend
+                # and start the agent.
+                current_api = IMPROV
+            else:
+                # The agent is still running but the configuration has been
+                # updated -- bounce it.
+                current_api = AGENT
+            service_control(current_api, STOP)
+            start_agent(juju_api_port)
+
+    # Handle changes to the juju-gui configuration.
+    gui_properties = set(['juju-gui-console-enabled', 'juju-api-port', 'staging'])
+    if bool(added_or_changed & gui_properties) or gui_branch is not None:
+        with su('root'):
+            service_control('nginx', STOP)
+            service_control(GUI, STOP)
+        start_gui(juju_api_port, config.get('juju-gui-console-enabled'), staging)
+
+
+def main():
+    config = get_config()
+    prev_config = config_json.get()
+    diff = DictDiffer(config, prev_config)
+
+    if not diff.modified:
+        log("No configuration changes, exiting.")
+        sys.exit(0)
+    handle_config_changes(config, diff)
+    config_json.set(config)
+
+
+if __name__ == '__main__':
+    log_entry()
+    try:
+        main()
+    except CalledProcessError as e:
+        log('Exception caught:')
+        log(e.output)
+        raise
+    finally:
+        log_exit()

=== modified file 'hooks/install'
--- hooks/install	2012-12-12 17:29:39 +0000
+++ hooks/install	2012-12-17 22:56:21 +0000
@@ -6,7 +6,6 @@
     CalledProcessError,
     check_call,
     )
-import tempfile
 
 # python-shelltoolbox is installed as a dependency of python-charmhelpers.
 check_call(['apt-get', 'install', '-y', 'python-charmhelpers'])
@@ -22,17 +21,19 @@
 )
 from shelltoolbox import (
     apt_get_install,
-    cd,
-    command,
     install_extra_repositories,
     run,
 )
 
-from utils import cmd_log
-
-
-def fetch(juju_gui_branch, juju_api_branch):
-    """Install required dependencies and retrieve Juju/Juju GUI branches."""
+from utils import (
+    build,
+    cmd_log,
+    config_json,
+    fetch,
+    )
+
+
+def get_dependencies():
     log('Installing dependencies.')
     cmd_log(install_extra_repositories('ppa:chris-lea/node.js'))
     cmd_log(
@@ -44,32 +45,12 @@
     cmd_log(bzr_checkout(juju_api_branch, 'juju'))
 
 
-def build(logpath):
-    """Set up Juju GUI and nginx."""
-    log('Building Juju GUI.')
-    with cd('juju-gui'):
-        logdir = os.path.dirname(logpath)
-        fd, name = tempfile.mkstemp(prefix='make-', dir=logdir)
-        log('Output from "make" sent to', name)
-        run('make', stdout=fd, stderr=fd)
-    log('Setting up nginx.')
-    nginx_default_site = '/etc/nginx/sites-enabled/default'
-    juju_gui_site = '/etc/nginx/sites-available/juju-gui'
-    if os.path.exists(nginx_default_site):
-        os.remove(nginx_default_site)
-    if not os.path.exists(juju_gui_site):
-        cmd_log(run('touch', juju_gui_site))
-        cmd_log(run('chown', 'ubuntu:', juju_gui_site))
-        cmd_log(
-            run('ln', '-s', juju_gui_site,
-                '/etc/nginx/sites-enabled/juju-gui'))
-
-
 def main():
     config = get_config()
+    get_dependencies()
     fetch(config['juju-gui-branch'], config['juju-api-branch'])
     build(config['command-log-file'])
-
+    config_json.set(config)
 
 if __name__ == '__main__':
     log_entry()

=== modified file 'hooks/start'
--- hooks/start	2012-12-12 11:42:12 +0000
+++ hooks/start	2012-12-17 22:56:21 +0000
@@ -1,7 +1,6 @@
 #!/usr/bin/env python2
 #-*- python -*-
 
-import json
 import os
 
 from charmhelpers import (
@@ -12,7 +11,6 @@
     open_port,
     service_control,
     START,
-    unit_get,
 )
 from shelltoolbox import (
     run,
@@ -21,80 +19,13 @@
 
 from utils import (
     get_zookeeper_address,
+    GUI,
     render_to_file,
+    start_agent,
+    start_gui,
+    start_improv,
 )
 
-CURRENT_DIR = os.getcwd()
-JUJU_DIR = os.path.join(CURRENT_DIR, 'juju')
-JUJU_GUI_DIR = os.path.join(CURRENT_DIR, 'juju-gui')
-
-
-def start_gui(juju_api_port, console_enabled, staging):
-    """Set up and start the Juju GUI server."""
-    with su('root'):
-        run('chown', '-R', 'ubuntu:', JUJU_GUI_DIR)
-    build_dir = JUJU_GUI_DIR + '/build-'
-    build_dir += 'debug' if staging else 'prod'
-    log('Setting up Juju GUI start up script.')
-    render_to_file(
-        'juju-gui.conf.template', {'juju_gui_dir': JUJU_GUI_DIR},
-        '/etc/init/juju-gui.conf')
-    log('Generating the Juju GUI configuration file.')
-    context = {
-        'address': unit_get('public-address'),
-        'console_enabled': json.dumps(console_enabled),
-        'port': juju_api_port,
-    }
-    render_to_file(
-        'config.js.template', context,
-        os.path.join(build_dir, 'juju-ui', 'assets', 'config.js'))
-    log('Generating the nginx site configuration file.')
-    context = {
-        'server_root': build_dir
-    }
-    render_to_file(
-        'nginx.conf.template', context,
-        '/etc/nginx/sites-available/juju-gui')
-    log('Starting Juju GUI.')
-    with su('root'):
-        service_control('juju-gui', START)
-
-
-def start_improv(juju_api_port, staging_env):
-    """Start a simulated juju environment using ``improv.py``."""
-    log('Setting up staging start up script.')
-    context = {
-        'juju_dir': JUJU_DIR,
-        'port': juju_api_port,
-        'staging_env': staging_env,
-    }
-    render_to_file(
-        'juju-api-improv.conf.template', context,
-        '/etc/init/juju-api-improv.conf')
-    log('Starting the staging backend.')
-    with su('root'):
-        service_control('juju-api-improv', START)
-
-
-def start_agent(juju_api_port):
-    """Start the Juju agent and connect to the current environment."""
-    # Retrieve the Zookeeper address from the start up script.
-    unit_dir = os.path.realpath(os.path.join(CURRENT_DIR, '..'))
-    agent_file = '/etc/init/juju-{0}.conf'.format(os.path.basename(unit_dir))
-    zookeeper = get_zookeeper_address(agent_file)
-    log('Setting up API agent start up script.')
-    context = {
-        'juju_dir': JUJU_DIR,
-        'port': juju_api_port,
-        'zookeeper': zookeeper,
-    }
-    render_to_file(
-        'juju-api-agent.conf.template', context,
-        '/etc/init/juju-api-agent.conf')
-    log('Starting API agent.')
-    with su('root'):
-        service_control('juju-api-agent', START)
-
 
 def open_ports(juju_api_port):
     """Expose Juju GUI web server and websocket server ports."""

=== modified file 'hooks/stop'
--- hooks/stop	2012-12-10 14:37:15 +0000
+++ hooks/stop	2012-12-17 22:56:21 +0000
@@ -2,29 +2,11 @@
 #-*- python -*-
 
 from charmhelpers import (
-    get_config,
-    log,
     log_entry,
     log_exit,
-    service_control,
-    STOP,
+    stop,
 )
-from shelltoolbox import su
-
-
-def stop():
-    """Stop the Juju API agent."""
-    config = get_config()
-    with su('root'):
-        log('Stopping Juju GUI.')
-        service_control('juju-gui', STOP)
-        if config.get('staging'):
-            log('Stopping the staging backend.')
-            service_control('juju-api-improv', STOP)
-        else:
-            log('Stopping API agent.')
-            service_control('juju-api-agent', STOP)
-
+from utils import stop
 
 if __name__ == '__main__':
     log_entry()

=== modified file 'hooks/utils.py'
--- hooks/utils.py	2012-12-07 21:16:55 +0000
+++ hooks/utils.py	2012-12-17 22:56:21 +0000
@@ -1,10 +1,48 @@
 """Juju GUI charm utilities."""
 
+
+__all__ = [
+    'AGENT',
+    'build',
+    'cmd_log',
+    'get_zookeeper_address',
+    'GUI',
+    'IMPROV',
+    'render_to_file',
+    'start_agent',
+    'start_gui',
+    'start_improv',
+    'stop',
+    ]
+
+import json
 import os
 import logging
-
-from shelltoolbox import search_file
-from charmhelpers import get_config
+import tempfile
+
+from shelltoolbox import (
+    cd,
+    command,
+    run,
+    search_file,
+    Serializer,
+    su,
+    )
+from charmhelpers import (
+    get_config,
+    log,
+    service_control,
+    START,
+    STOP,
+    unit_get,
+)
+
+AGENT = 'juju-api-agent'
+IMPROV = 'juju-api-improv'
+GUI = 'juju-gui'
+
+# Store the configuration from on invocation to the next.
+config_json = Serializer('/tmp/config.json')
 
 
 def get_zookeeper_address(agent_file_path):
@@ -45,7 +83,7 @@
         filename=config['command-log-file'],
         level=logging.INFO,
         format="%(asctime)s: %(name)s@%(levelname)s %(message)s")
-    results_log = logging.getLogger('juju-gui')
+    results_log = logging.getLogger(GUI)
 
 
 def cmd_log(results):
@@ -57,3 +95,128 @@
     # Since 'results' may be multi-line output, start it on a separate line
     # from the logger timestamp, etc.
     results_log.info('\n' + results)
+
+CURRENT_DIR = os.getcwd()
+JUJU_DIR = os.path.join(CURRENT_DIR, 'juju')
+JUJU_GUI_DIR = os.path.join(CURRENT_DIR, 'juju-gui')
+
+
+def start_improv(juju_api_port, staging_env,
+                 config_path='/etc/init/juju-api-improv.conf'):
+    """Start a simulated juju environment using ``improv.py``."""
+    log('Setting up staging start up script.')
+    context = {
+        'juju_dir': JUJU_DIR,
+        'port': juju_api_port,
+        'staging_env': staging_env,
+    }
+    render_to_file(
+        'juju-api-improv.conf.template', context,
+        config_path)
+    log('Starting the staging backend.')
+    with su('root'):
+        service_control(IMPROV, START)
+
+
+def start_agent(juju_api_port, config_path='/etc/init/juju-api-agent.conf'):
+    """Start the Juju agent and connect to the current environment."""
+    # Retrieve the Zookeeper address from the start up script.
+    unit_dir = os.path.realpath(os.path.join(CURRENT_DIR, '..'))
+    agent_file = '/etc/init/juju-{0}.conf'.format(os.path.basename(unit_dir))
+    zookeeper = get_zookeeper_address(agent_file)
+    log('Setting up API agent start up script.')
+    context = {
+        'juju_dir': JUJU_DIR,
+        'port': juju_api_port,
+        'zookeeper': zookeeper,
+    }
+    render_to_file(
+        'juju-api-agent.conf.template', context,
+        config_path)
+    log('Starting API agent.')
+    with su('root'):
+        service_control(AGENT, START)
+
+
+def start_gui(juju_api_port, console_enabled, staging,
+              config_path='/etc/init/juju-gui.conf',
+              nginx_path='/etc/nginx/sites-available/juju-gui',
+              config_js_path=None):
+    """Set up and start the Juju GUI server."""
+    with su('root'):
+        run('chown', '-R', 'ubuntu:', JUJU_GUI_DIR)
+    build_dir = JUJU_GUI_DIR + '/build-'
+    build_dir += 'debug' if staging else 'prod'
+    log('Setting up Juju GUI start up script.')
+    render_to_file(
+        'juju-gui.conf.template', {}, config_path)
+    log('Generating the Juju GUI configuration file.')
+    context = {
+        'address': unit_get('public-address'),
+        'console_enabled': json.dumps(console_enabled),
+        'port': juju_api_port,
+    }
+    if config_js_path is None:
+        config_js_path = os.path.join(
+            build_dir, 'juju-ui', 'assets', 'config.js')
+    render_to_file(
+        'config.js.template', context,
+        config_js_path)
+    log('Generating the nginx site configuration file.')
+    context = {
+        'server_root': build_dir
+    }
+    render_to_file(
+        'nginx.conf.template', context, nginx_path)
+    log('Starting Juju GUI.')
+    with su('root'):
+        # Stop nginx so it will restart cleanly with the gui.
+        service_control('nginx', STOP)
+        service_control(GUI, START)
+
+
+def stop():
+    """Stop the Juju API agent."""
+    config = get_config()
+    with su('root'):
+        log('Stopping Juju GUI.')
+        service_control(GUI, STOP)
+        if config.get('staging'):
+            log('Stopping the staging backend.')
+            service_control(IMPROV, STOP)
+        else:
+            log('Stopping API agent.')
+            service_control(AGENT, STOP)
+
+
+def fetch(juju_gui_branch, juju_api_branch):
+    """Install required dependencies and retrieve Juju/Juju GUI branches."""
+    log('Retrieving source checkouts.')
+    bzr_checkout = command('bzr', 'co', '--lightweight')
+    if juju_gui_branch is not None:
+        cmd_log(run('rm', '-rf', 'juju-gui'))
+        cmd_log(bzr_checkout(juju_gui_branch, 'juju-gui'))
+    if juju_api_branch is not None:
+        cmd_log(run('rm', '-rf', 'juju'))
+        cmd_log(bzr_checkout(juju_api_branch, 'juju'))
+
+
+def build(logpath):
+    """Set up Juju GUI and nginx."""
+    log('Building Juju GUI.')
+    with cd('juju-gui'):
+        logdir = os.path.dirname(logpath)
+        fd, name = tempfile.mkstemp(prefix='make-', dir=logdir)
+        log('Output from "make" sent to', name)
+        run('make', stdout=fd, stderr=fd)
+    log('Setting up nginx.')
+    nginx_default_site = '/etc/nginx/sites-enabled/default'
+    juju_gui_site = '/etc/nginx/sites-available/juju-gui'
+    if os.path.exists(nginx_default_site):
+        os.remove(nginx_default_site)
+    if not os.path.exists(juju_gui_site):
+        cmd_log(run('touch', juju_gui_site))
+        cmd_log(run('chown', 'ubuntu:', juju_gui_site))
+        cmd_log(
+            run('ln', '-s', juju_gui_site,
+                '/etc/nginx/sites-enabled/juju-gui'))

=== modified file 'revision'
--- revision	2012-12-14 16:48:29 +0000
+++ revision	2012-12-17 22:56:21 +0000
@@ -1,1 +1,1 @@
-15
+16

=== modified file 'tests/test_utils.py'
--- tests/test_utils.py	2012-12-12 17:29:39 +0000
+++ tests/test_utils.py	2012-12-17 22:56:21 +0000
@@ -1,5 +1,6 @@
 #!/usr/bin/env python2
 
+from contextlib import contextmanager
 import os
 import tempfile
 import unittest
@@ -10,8 +11,13 @@
     cmd_log,
     get_zookeeper_address,
     render_to_file,
+    start_agent,
+    start_gui,
+    start_improv,
+    stop,
 )
-
+# Import the whole utils package for monkey patching.
+import utils
 
 class GetZookeeperAddressTest(unittest.TestCase):
 
@@ -68,5 +74,101 @@
         self.assertTrue(line.endswith(': juju-gui@INFO \nfoo\n'))
 
 
+class StartStopTest(unittest.TestCase):
+
+    def setUp(self):
+        self.service_names = []
+        self.actions = []
+        self.svc_ctl_call_count = 0
+        self.fake_zk_address = '192.168.5.26'
+        # Monkey patches.
+        self.command = charmhelpers.command
+        def service_control_mock(service_name, action):
+            self.svc_ctl_call_count += 1
+            self.service_names.append(service_name)
+            self.actions.append(action)
+        def noop(*args):
+            pass
+        @contextmanager
+        def su(user):
+            yield None
+        def get_zookeeper_address_mock(fp):
+            return self.fake_zk_address
+        self.functions = dict(
+            service_control=(utils.service_control, service_control_mock),
+            log=(utils.log, noop),
+            su=(utils.su, su),
+            run=(utils.run, noop),
+            unit_get=(utils.unit_get, noop),
+            get_zookeeper_address=(utils.get_zookeeper_address, get_zookeeper_address_mock)
+            )
+        # Apply the patches.
+        for fn,fcns in self.functions.items():
+            setattr(utils, fn, fcns[1])
+
+        self.destination_file = tempfile.NamedTemporaryFile()
+        self.addCleanup(self.destination_file.close)
+
+        def tearDown(self):
+            # Undo all of the monkey patching.
+            for fn,fcns in self.functions.items():
+                setattr(utils, fn, fcns[0])
+            charmhelpers.command = self.command
+
+    def test_start_improv(self):
+        port = '1234'
+        staging_env = 'large'
+        start_improv(port, staging_env, self.destination_file.name)
+        conf = self.destination_file.read()
+        self.assertTrue('--port %s' % port in conf)
+        self.assertTrue(staging_env + '.json' in conf)
+        self.assertEqual(self.svc_ctl_call_count, 1)
+        self.assertEqual(self.service_names, ['juju-api-improv'])
+        self.assertEqual(self.actions, [charmhelpers.START])
+
+    def test_start_agent(self):
+        port = '1234'
+        start_agent(port, self.destination_file.name)
+        conf = self.destination_file.read()
+        self.assertTrue('--port %s' % port in conf)
+        self.assertTrue('JUJU_ZOOKEEPER=%s' % self.fake_zk_address in conf)
+        self.assertEqual(self.svc_ctl_call_count, 1)
+        self.assertEqual(self.service_names, ['juju-api-agent'])
+        self.assertEqual(self.actions, [charmhelpers.START])
+
+    def test_start_gui(self):
+        port = '1234'
+        nginx_file = tempfile.NamedTemporaryFile()
+        self.addCleanup(nginx_file.close)
+        config_js_file = tempfile.NamedTemporaryFile()
+        self.addCleanup(config_js_file.close)
+        start_gui(port, False, True, self.destination_file.name,
+                  nginx_file.name, config_js_file.name)
+        conf = self.destination_file.read()
+        self.assertTrue('exec service nginx start' in conf)
+        nginx_conf = nginx_file.read()
+        self.assertTrue('juju-gui/build-debug' in nginx_conf)
+        self.assertEqual(self.svc_ctl_call_count, 1)
+        self.assertEqual(self.service_names, ['juju-gui'])
+        self.assertEqual(self.actions, [charmhelpers.START])
+
+    def test_stop_staging(self):
+        mock_config = {'staging': True}
+        charmhelpers.command = lambda *args: lambda: dumps(mock_config)
+        stop()
+        self.assertEqual(self.svc_ctl_call_count, 2)
+        self.assertEqual(self.service_names, ['juju-gui', 'juju-api-improv'])
+        self.assertEqual(self.actions, [charmhelpers.STOP, charmhelpers.STOP])
+
+    def test_stop_production(self):
+        mock_config = {'staging': False}
+        charmhelpers.command = lambda *args: lambda: dumps(mock_config)
+        stop()
+        self.assertEqual(self.svc_ctl_call_count, 2)
+        self.assertEqual(self.service_names, ['juju-gui', 'juju-api-agent'])
+        self.assertEqual(self.actions, [charmhelpers.STOP, charmhelpers.STOP])
+
+
+
 if __name__ == '__main__':
     unittest.main(verbosity=2)