Merge lp:~teknico/charms/precise/juju-gui/integrate-builtin-server-2 into lp:~juju-gui/charms/precise/juju-gui/trunk

[Nicola Larosa]

Integrate the built-in server into the charm.

Add a "builtin-server" option to config.yaml, defaulting to false, to enable
a new Tornado-based built-in web server, in place of haproxy and Apache (still
the default).

Add a "guiserver.conf" Upstart config file, generated via a template.

Add a BuiltinServerMixin to hooks/backend.py .

Add tests for the new functions in hooks/utils.py .

Clean up all paths and constants in hooks/utils.py .

Miscellaneuos cleanup.

QA1 (quick):

$ make unittest

QA2 (long, 40 minutes):

$ make ftest

QA3 (long but shorter than the former):

$ juju bootstrap
$ make deploy
  (check web access)
$ juju set juju-gui builtin-server=true
  (check web access)
$ juju set juju-gui builtin-server=false
  (check web access)
$ juju destroy-environment

https://codereview.appspot.com/12650044/

[Nicola Larosa (teknico)]

Reviewers: mp+179138_code.launchpad.net,

Message:
Please take a look.

Description:
Integrate the built-in server into the charm.

Add a "builtin-server" option to config.yaml, defaulting to false, to
enable
a new Tornado-based built-in web server, in place of haproxy and Apache
(still
the default).

Add a "guiserver.conf" Upstart config file, generated via a template.

Add a BuiltinServerMixin to hooks/backend.py .

Add tests for the new functions in hooks/utils.py .

Clean up all paths and constants in hooks/utils.py .

Miscellaneuos cleanup.

QA1 (quick):

$ make unittest

QA2 (long, 40 minutes):

$ make ftest

QA3 (long but shorter than the former):

$ juju bootstrap
$ make deploy
   (check web access)
$ juju set juju-gui builtin-server=true
   (check web access)
$ juju set juju-gui builtin-server=false
   (check web access)
$ juju destroy-environment

https://code.launchpad.net/~teknico/charms/precise/juju-gui/integrate-builtin-server-2/+merge/179138

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/12650044/

Affected files:
   M Operation.md
   A [revision details]
   M config.yaml
   A config/guiserver.conf.template
   A deps/tornado-3.1.tar.gz
   M hooks/backend.py
   M hooks/utils.py
   M revision
   M server/setup.py
   M tests/20-functional.test
   M tests/test_backends.py
   M tests/test_utils.py

[Francesco Banconi (frankban)]

This branch is nice Nicola, we are almost there, thank you.
Please fix the problem I described below, I will QA again later.

https://codereview.appspot.com/12650044/diff/1/config/guiserver.conf.template
File config/guiserver.conf.template (right):

https://codereview.appspot.com/12650044/diff/1/config/guiserver.conf.template#newcode7
config/guiserver.conf.template:7: exec /usr/bin/python
/usr/local/bin/runserver.py \
Cool.

https://codereview.appspot.com/12650044/diff/1/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/12650044/diff/1/hooks/utils.py#newcode110
hooks/utils.py:110: SERVER_DIR = os.path.join(CURRENT_DIR, 'server')
I am not sure I like all those constants. I agree they are used in more
than one place (but usually only two times AFAIK, and e.g. UNIT_NAME is
used only once), so this prevents typos, but I also liked the
possibility to read the functions without having to jump at the
beginning of the file. As I said, I see the value in this change, and I
can live with this.
The real request here is to fix all those "os.path.join('', ...": see
below.

https://codereview.appspot.com/12650044/diff/1/hooks/utils.py#newcode132
hooks/utils.py:132: config_json = Serializer(os.path.join('', 'tmp',
'config.json'))
I don't think this is the same as before: this results in
"tmp/config.json".
You might want to use "os.path.join(os.path.sep, 'tmp', 'config.json')",
but I am also ok with the original version.
Note that this generates an install error, that prevented me to QA this
branch. I'll retry it later when this is fixed.

https://codereview.appspot.com/12650044/diff/1/hooks/utils.py#newcode549
hooks/utils.py:549: api_url = '{}://127.0.0.1:{}'.format(
The "x if y else z" is not so easy to read when it is not a one-liner
IMHO.
I had to read this carefully, perhaps the more usual "if: else:" can be
better?

https://codereview.appspot.com/12650044/diff/1/hooks/utils.py#newcode564
hooks/utils.py:564: def start_builtin_server(build_dir, serve_tests,
ssl_cert_path, insecure):
What do you think about inverting serve_tests and ssl_cert_path? This
way we can have a similar signature for start_builtin_server and
write_builtin_server_startup.

https://codereview.appspot.com/12650044/diff/1/tests/20-functional.test
File tests/20-functional.test (right):

https://codereview.appspot.com/12650044/diff/1/tests/20-functional.test#newcode189
tests/20-functional.test:189: self.assertIn('TornadoServer',
server_header)
The header check is great. I'd also add an environment check, i.e. we
must ensure that the WebSocket is connected (basically the same thing
we do in the other functional tests).

https://codereview.appspot.com/12650044/

[Nicola Larosa (teknico)]

frankban wrote:

https://codereview.appspot.com/12650044/diff/1/hooks/utils.py#newcode110
> hooks/utils.py:110: SERVER_DIR = os.path.join(CURRENT_DIR, 'server')
> I also liked the possibility to read the functions without having
> to jump at the beginning of the file.

Hopefully the names of the constants are meaningful enough that you
don't need to see the associated values to be able to understand the
code.

> As I said, I see the value in this change, and I can live with this.

Good.

https://codereview.appspot.com/12650044/diff/1/hooks/utils.py#newcode132
> hooks/utils.py:132: config_json = Serializer(os.path.join('', 'tmp',
> 'config.json'))
> I don't think this is the same as before: this results in
> "tmp/config.json". You might want to use "os.path.join(os.path.sep,
> 'tmp', 'config.json')", but I am also ok with the original version.

Oh, sorry, I did not check it, will fix.

> Note that this generates an install error, that prevented me to
> QA this branch. I'll retry it later when this is fixed.

Weird, this did not happen to me in QA.

https://codereview.appspot.com/12650044/diff/1/hooks/utils.py#newcode549
> hooks/utils.py:549: api_url = '{}://127.0.0.1:{}'.format(
> The "x if y else z" is not so easy to read when it is not a
> one-liner IMHO. I had to read this carefully, perhaps the more
> usual "if: else:" can be better?

Hrmpf. You know I like it, and I know you don't like it. Is it really
that ugly? :-)

https://codereview.appspot.com/12650044/diff/1/hooks/utils.py#newcode564
> hooks/utils.py:564: def start_builtin_server(build_dir, serve_tests,
> ssl_cert_path, insecure):
> What do you think about inverting serve_tests and ssl_cert_path?
> This way we can have a similar signature for start_builtin_server
> and write_builtin_server_startup.

Uhm, I don't like it either but there was a reason for it, I'll check.

https://codereview.appspot.com/12650044/diff/1/tests/20-functional.test#newcode189
> tests/20-functional.test:189: self.assertIn('TornadoServer',
server_header)
> The header check is great. I'd also add an environment check, i.e. we
> must ensure that the WebSocket is connected (basically the same thing
> we do in the other functional tests).

Will do.

https://codereview.appspot.com/12650044/

[lp:~teknico/charms/precise/juju-gui/integrate-builtin-server-2]

89. By Nicola Larosa on 2013-08-08

Fixes per review.

90. By Nicola Larosa on 2013-08-08

More fixes.

91. By Nicola Larosa on 2013-08-08

Merge from trunk.

[Nicola Larosa (teknico)]

Please take a look.

https://codereview.appspot.com/12650044/

[Madison Scott-Clary (makyo)]

Code looks good with Francesco's comments, QAing now.

https://codereview.appspot.com/12650044/diff/1/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/12650044/diff/1/hooks/utils.py#newcode549
hooks/utils.py:549: api_url = '{}://127.0.0.1:{}'.format(
On 2013/08/08 11:12:58, frankban wrote:
> The "x if y else z" is not so easy to read when it is not a one-liner
IMHO.
> I had to read this carefully, perhaps the more usual "if: else:" can
be better?

I agree in this instance. The x if y else z construct works much better
in a single-line format than across multiple lines; in this instance,
it's hard to tell that this is conditional until halfway through, then
you have to parse it inside out across multiple lines.

https://codereview.appspot.com/12650044/

[Madison Scott-Clary (makyo)]

QA3 worked for me, LGTM

https://codereview.appspot.com/12650044/

[Francesco Banconi (frankban)]

LGTM with the changes below.

I run the functional tests, they were passing, the last one was stopped
by the timeout: 40 mins are not enough, please increase that value (60
mins?).

QA:
switching from the legacy to the new server (and back) worked well. The
builtin server stopped working after setting secure=false on the charm.
I think upstart is not happy with the generated configuration file when
--insecure is passed.

I believe this branch can land with the timeout fix, but please add a
card to fix the "insecure" bug. Thank you!

https://codereview.appspot.com/12650044/

[Nicola Larosa (teknico)]

frankban wrote:
> I run the functional tests, they were passing, the last one was
stopped
> by the timeout: 40 mins are not enough, please increase that value
> (60 mins?).

Timeout increased to 60 minutes.

> QA:
> switching from the legacy to the new server (and back) worked well.
The
> I think upstart builtin server stopped working after setting
> secure=false on the charm. is not happy with the generated
> configuration file when --insecure is passed.

Indeed, there was a mistake in config/guiserver.conf.template. Fixed,
thank you.

https://codereview.appspot.com/12650044/

[lp:~teknico/charms/precise/juju-gui/integrate-builtin-server-2]

92. By Nicola Larosa on 2013-08-09

Increase functional tests timeout, fix config/guiserver.conf.template .

93. By Nicola Larosa on 2013-08-09

Fix the insecure and testsroot options handling of the server.

94. By Nicola Larosa on 2013-08-09

Remove leftover assert in test.

95. By Nicola Larosa on 2013-08-09

Better invocation example, shorten conditions.

[Nicola Larosa (teknico)]

*** Submitted:

Integrate the built-in server into the charm.

Add a "builtin-server" option to config.yaml, defaulting to false, to
enable
a new Tornado-based built-in web server, in place of haproxy and Apache
(still
the default).

Add a "guiserver.conf" Upstart config file, generated via a template.

Add a BuiltinServerMixin to hooks/backend.py .

Add tests for the new functions in hooks/utils.py .

Clean up all paths and constants in hooks/utils.py .

Miscellaneuos cleanup.

QA1 (quick):

$ make unittest

QA2 (long, 40 minutes):

$ make ftest

QA3 (long but shorter than the former):

$ juju bootstrap
$ make deploy
   (check web access)
$ juju set juju-gui builtin-server=true
   (check web access)
$ juju set juju-gui builtin-server=false
   (check web access)
$ juju destroy-environment

R=frankban, matthew.scott
CC=
https://codereview.appspot.com/12650044

https://codereview.appspot.com/12650044/

=== modified file 'Makefile'
--- Makefile	2013-08-06 14:17:23 +0000
+++ Makefile	2013-08-09 13:41:32 +0000
@@ -14,7 +14,7 @@
 # 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/>.
 
-JUJUTEST = juju-test --timeout=40m -v -e "$(JUJU_ENV)" --upload-tools
+JUJUTEST = juju-test --timeout=60m -v -e "$(JUJU_ENV)" --upload-tools
 VENV = ./tests/.venv
 
 all: setup

=== modified file 'Operation.md'
--- Operation.md	2013-07-29 10:40:50 +0000
+++ Operation.md	2013-08-09 13:41:32 +0000
@@ -12,8 +12,8 @@
 ## How it works ##
 
 The Juju GUI is a client-side, JavaScript application that runs inside a
-web browser. The browser connects to a custom-made server deployed by
-the charm.
+web browser. The browser connects to a built-in server deployed by the
+charm.
 
 ## Server ##
 
@@ -27,3 +27,14 @@
 Juju connections are bidirectional, using the WebSocket protocol on the
 same port as the HTTPS connection, allowing changes in the Juju
 environment to be propagated and shown immediately by the browser.
+
+## Activation ##
+
+Previously the Juju GUI has been served by a combination of haproxy and
+Apache, specifically deployed and configured by the charm.
+
+The new built-in server replaces them both and can be enabled by
+setting the config option `builtin-server` to `true`.
+
+In the future haproxy, Apache and the mentioned config option will be
+removed; only the built-in server will remain.

=== modified file 'config.yaml'
--- config.yaml	2013-08-05 09:38:39 +0000
+++ config.yaml	2013-08-09 13:41:32 +0000
@@ -166,3 +166,10 @@
       a link to juju.ubuntu.com
     type: boolean
     default: false
+  builtin-server:
+    description: |
+      Enable the built-in server, disabling both haproxy and Apache.
+      This is a temporary option: the built-in server will be
+      the only server in the future.
+    type: boolean
+    default: false

=== modified file 'config/apache-site.template'
--- config/apache-site.template	2013-08-01 14:33:00 +0000
+++ config/apache-site.template	2013-08-09 13:41:32 +0000
@@ -21,7 +21,7 @@
 
 	CustomLog ${APACHE_LOG_DIR}/access.log combined
 
-    {{if serve_tests}}
+    {{if tests_root}}
         Alias /test {{tests_root}}
     {{endif}}
 

=== added file 'config/guiserver.conf.template'
--- config/guiserver.conf.template	1970-01-01 00:00:00 +0000
+++ config/guiserver.conf.template	2013-08-09 13:41:32 +0000
@@ -0,0 +1,13 @@
+description "GUIServer"
+author "Canonical"
+
+start on (filesystem and net-device-up IFACE=lo)
+stop on runlevel [!2345]
+
+exec /usr/bin/python /usr/local/bin/runserver.py \
+    --guiroot="{{gui_root}}" \
+    --apiurl="{{api_url}}" \
+    --apiversion="{{api_version}}" \
+    --sslpath="{{ssl_cert_path}}" \
+    {{if tests_root}} --testsroot="{{tests_root}}" {{endif}} \
+    {{if insecure}} --insecure {{endif}}

=== added directory 'deps'
=== added file 'deps/tornado-3.1.tar.gz'
Binary files deps/tornado-3.1.tar.gz	1970-01-01 00:00:00 +0000 and deps/tornado-3.1.tar.gz	2013-08-09 13:41:32 +0000 differ
=== modified file 'hooks/backend.py'
--- hooks/backend.py	2013-08-05 09:59:17 +0000
+++ hooks/backend.py	2013-08-09 13:41:32 +0000
@@ -180,6 +180,28 @@
         utils.stop_haproxy_apache()
 
 
+class BuiltinServerMixin(ServerInstallMixinBase):
+    """Manage the builtin server via Upstart."""
+
+    debs = ('openssl', 'python-pip')
+
+    def install(self, backend):
+        utils.install_tornado()
+        utils.install_builtin_server()
+        self._setup_certificates(backend)
+
+    def start(self, backend):
+        config = backend.config
+        build_dir = utils.compute_build_dir(
+            config['staging'], config['serve-tests'])
+        utils.start_builtin_server(
+            build_dir, config['ssl-cert-path'], config['serve-tests'],
+            not config['secure'])
+
+    def stop(self, backend):
+        utils.stop_builtin_server()
+
+
 def chain_methods(name):
     """Helper to compose a set of mixin objects into a callable.
 
@@ -249,7 +271,12 @@
 
         # We always install and start the GUI.
         self.mixins.append(GuiMixin())
-        self.mixins.append(HaproxyApacheMixin())
+        # TODO: eventually this option will go away, as well as haproxy and
+        # Apache.
+        if config['builtin-server']:
+            self.mixins.append(BuiltinServerMixin())
+        else:
+            self.mixins.append(HaproxyApacheMixin())
 
     def different(self, *keys):
         """Return a boolean indicating if the current config

=== modified file 'hooks/utils.py'
--- hooks/utils.py	2013-08-08 08:13:24 +0000
+++ hooks/utils.py	2013-08-09 13:41:32 +0000
@@ -17,19 +17,14 @@
 """Juju GUI charm utilities."""
 
 __all__ = [
-    'AGENT',
-    'APACHE',
     'APACHE_SITE',
     'APACHE_PORTS',
     'API_PORT',
     'CURRENT_DIR',
-    'HAPROXY',
-    'IMPROV',
     'JUJU_DIR',
     'JUJU_GUI_DIR',
     'JUJU_PEM',
     'WEB_PORT',
-    'bzr_checkout',
     'cmd_log',
     'compute_build_dir',
     'fetch_api',
@@ -53,9 +48,11 @@
     'setup_gui',
     'setup_haproxy_config',
     'start_agent',
+    'start_builtin_server',
     'start_haproxy_apache',
     'start_improv',
     'stop_agent',
+    'stop_builtin_server',
     'stop_haproxy_apache',
     'stop_improv',
     'write_gui_config',
@@ -99,20 +96,31 @@
 
 AGENT = 'juju-api-agent'
 APACHE = 'apache2'
+BUILTIN_SERVER = 'guiserver'
+HAPROXY = 'haproxy'
 IMPROV = 'juju-api-improv'
-HAPROXY = 'haproxy'
 
 API_PORT = 8080
 WEB_PORT = 8000
 
 CURRENT_DIR = os.getcwd()
+CONFIG_DIR = os.path.join(CURRENT_DIR, 'config')
 JUJU_DIR = os.path.join(CURRENT_DIR, 'juju')
 JUJU_GUI_DIR = os.path.join(CURRENT_DIR, 'juju-gui')
-APACHE_SITE = '/etc/apache2/sites-available/juju-gui'
-APACHE_PORTS = '/etc/apache2/ports.conf'
-HAPROXY_PATH = '/etc/haproxy/haproxy.cfg'
-SYS_INIT_DIR = '/etc/init/'
-CONFIG_DIR = os.path.join(os.path.dirname(__file__),  '..', 'config')
+SERVER_DIR = os.path.join(CURRENT_DIR, 'server')
+TORNADO_PATH = os.path.join(CURRENT_DIR, 'deps', 'tornado-3.1.tar.gz')
+
+APACHE_CFG_DIR = os.path.join(os.path.sep, 'etc', 'apache2')
+APACHE_PORTS = os.path.join(APACHE_CFG_DIR, 'ports.conf')
+APACHE_SITE = os.path.join(APACHE_CFG_DIR, 'sites-available', 'juju-gui')
+HAPROXY_CFG_PATH = os.path.join(os.path.sep, 'etc', 'haproxy', 'haproxy.cfg')
+
+SYS_INIT_DIR = os.path.join(os.path.sep, 'etc', 'init')
+AGENT_INIT_PATH = os.path.join(SYS_INIT_DIR, 'juju-api-agent.conf')
+GUISERVER_INIT_PATH = os.path.join(SYS_INIT_DIR, 'guiserver.conf')
+HAPROXY_INIT_PATH = os.path.join(SYS_INIT_DIR, 'haproxy.conf')
+IMPROV_INIT_PATH = os.path.join(SYS_INIT_DIR, 'juju-api-improv.conf')
+
 JUJU_PEM = 'juju.includes-private-key.pem'
 DEB_BUILD_DEPENDENCIES = (
     'bzr', 'g++', 'imagemagick', 'make',  'nodejs', 'npm',
@@ -120,7 +128,7 @@
 
 
 # Store the configuration from on invocation to the next.
-config_json = Serializer('/tmp/config.json')
+config_json = Serializer(os.path.join(os.path.sep, 'tmp', 'config.json'))
 # Bazaar checkout command.
 bzr_checkout = command('bzr', 'co', '--lightweight')
 # Whether or not the charm is deployed using juju-core.
@@ -289,8 +297,7 @@
     The argument *destination* is a file path.
     The argument *context* is a dict-like object.
     """
-    template_path = os.path.join(
-        os.path.dirname(__file__), '..', 'config', template_name)
+    template_path = os.path.join(CONFIG_DIR, template_name)
     template = tempita.Template.from_filename(template_path)
     with open(destination, 'w') as stream:
         stream.write(template.substitute(context))
@@ -328,9 +335,7 @@
         'port': API_PORT,
         'staging_env': staging_env,
     }
-    render_to_file(
-        'juju-api-improv.conf.template', context,
-        '/etc/init/juju-api-improv.conf')
+    render_to_file('juju-api-improv.conf.template', context, IMPROV_INIT_PATH)
     log('Starting the staging backend.')
     with su('root'):
         service_control(IMPROV, START)
@@ -342,15 +347,15 @@
     with su('root'):
         service_control(IMPROV, STOP)
     log('Removing the staging Upstart script.')
-    cmd_log(run('rm', '-f', '/etc/init/juju-api-improv.conf'))
+    cmd_log(run('rm', '-f', IMPROV_INIT_PATH))
 
 
 def start_agent(ssl_cert_path, read_only=False):
     """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 = os.path.join(
-        SYS_INIT_DIR, 'juju-{}.conf'.format(os.path.basename(unit_dir)))
+    unit_name = os.path.basename(
+        os.path.realpath(os.path.join(CURRENT_DIR, '..')))
+    agent_file = os.path.join(SYS_INIT_DIR, 'juju-{}.conf'.format(unit_name))
     zookeeper = get_zookeeper_address(agent_file)
     log('Setting up the API agent Upstart script.')
     context = {
@@ -360,9 +365,7 @@
         'zookeeper': zookeeper,
         'read_only': read_only
     }
-    render_to_file(
-        'juju-api-agent.conf.template', context,
-        '/etc/init/juju-api-agent.conf')
+    render_to_file('juju-api-agent.conf.template', context, AGENT_INIT_PATH)
     log('Starting the API agent.')
     with su('root'):
         service_control(AGENT, START)
@@ -374,7 +377,7 @@
     with su('root'):
         service_control(AGENT, STOP)
     log('Removing the API agent Upstart script.')
-    cmd_log(run('rm', '-f', '/etc/init/juju-api-agent.conf'))
+    cmd_log(run('rm', '-f', AGENT_INIT_PATH))
 
 
 def compute_build_dir(in_staging, serve_tests):
@@ -442,7 +445,7 @@
     is_legacy_juju = legacy_juju()
     if is_legacy_juju:
         # The PyJuju API agent is listening on localhost.
-        api_address = '127.0.0.1:{0}'.format(API_PORT)
+        api_address = '127.0.0.1:{}'.format(API_PORT)
     else:
         # Retrieve the juju-core API server address.
         api_address = get_api_address()
@@ -458,25 +461,24 @@
         'web_port': WEB_PORT,
         'secure': secure
     }
-    render_to_file('haproxy.cfg.template', context, HAPROXY_PATH)
+    render_to_file('haproxy.cfg.template', context, HAPROXY_CFG_PATH)
 
 
 def remove_haproxy_setup():
     """Remove haproxy setup."""
     log('Removing haproxy setup.')
-    cmd_log(run('rm', '-f', HAPROXY_PATH))
-    config_path = os.path.join(SYS_INIT_DIR, 'haproxy.conf')
-    cmd_log(run('rm', '-f', config_path))
+    cmd_log(run('rm', '-f', HAPROXY_CFG_PATH))
+    cmd_log(run('rm', '-f', HAPROXY_INIT_PATH))
 
 
 def setup_apache_config(build_dir, serve_tests=False):
     """Set up the Apache configuration."""
     log('Generating the Apache site configuration files.')
+    tests_root = os.path.join(JUJU_GUI_DIR, 'test', '') if serve_tests else ''
     context = {
         'port': WEB_PORT,
-        'serve_tests': serve_tests,
         'server_root': build_dir,
-        'tests_root': os.path.join(JUJU_GUI_DIR, 'test', ''),
+        'tests_root': tests_root,
     }
     render_to_file('apache-ports.template', context, APACHE_PORTS)
     cmd_log(run('chown', 'ubuntu:', APACHE_PORTS))
@@ -523,6 +525,62 @@
     remove_apache_setup()
 
 
+def install_tornado():
+    """Install Tornado from a local tarball."""
+    log('Installing Tornado.')
+    with su('root'):
+        cmd_log(run('pip', 'install', TORNADO_PATH))
+
+
+def install_builtin_server():
+    """Install the builtin server code."""
+    log('Installing the builtin server.')
+    setup_cmd = os.path.join(SERVER_DIR, 'setup.py')
+    with su('root'):
+        cmd_log(run('/usr/bin/python', setup_cmd, 'install'))
+
+
+def write_builtin_server_startup(
+        gui_root, ssl_cert_path, serve_tests=False, insecure=False):
+    """Generate the builtin server Upstart file."""
+    log('Generating the builtin server Upstart file.')
+    url_prefix = 'ws' if insecure else 'wss'
+    is_legacy_juju = legacy_juju()
+    api_address = get_api_address()
+    if is_legacy_juju:
+        api_url = '{}://127.0.0.1:{}'.format(url_prefix, API_PORT)
+    else:
+        api_url = '{}://{}'.format(url_prefix, api_address)
+    tests_root = os.path.join(JUJU_GUI_DIR, 'test', '') if serve_tests else ''
+    context = {
+        'gui_root': gui_root,
+        'api_url': api_url,
+        'api_version': 'python' if is_legacy_juju else 'go',
+        'ssl_cert_path': ssl_cert_path,
+        'tests_root': tests_root,
+        'insecure': insecure,
+    }
+    render_to_file(
+        'guiserver.conf.template', context, GUISERVER_INIT_PATH)
+
+
+def start_builtin_server(build_dir, ssl_cert_path, serve_tests, insecure):
+    """Start the builtin server."""
+    write_builtin_server_startup(
+        build_dir, ssl_cert_path, serve_tests, insecure)
+    log('Starting the builtin server.')
+    with su('root'):
+        service_control(BUILTIN_SERVER, RESTART)
+
+
+def stop_builtin_server():
+    """Stop the builtin server."""
+    log('Stopping the builtin server.')
+    with su('root'):
+        service_control(BUILTIN_SERVER, STOP)
+    cmd_log(run('rm', '-f', GUISERVER_INIT_PATH))
+
+
 def get_npm_cache_archive_url(Launchpad=Launchpad):
     """Figure out the URL of the most recent NPM cache archive on Launchpad."""
     launchpad = Launchpad.login_anonymously('Juju GUI charm', 'production')

=== modified file 'revision'
--- revision	2013-08-08 13:09:10 +0000
+++ revision	2013-08-09 13:41:32 +0000
@@ -1,1 +1,1 @@
-67
+68

=== modified file 'server/guiserver/apps.py'
--- server/guiserver/apps.py	2013-08-07 16:00:27 +0000
+++ server/guiserver/apps.py	2013-08-09 13:41:32 +0000
@@ -50,8 +50,8 @@
         (r'^/juju-ui/(.*)', web.StaticFileHandler, {'path': static_path}),
         (r'^/(favicon\.ico)$', web.StaticFileHandler, {'path': guiroot}),
     ]
-    if options.servetests:
-        params = {'path': options.servetests, 'default_filename': 'index.html'}
+    if options.testsroot:
+        params = {'path': options.testsroot, 'default_filename': 'index.html'}
         server_handlers.append(
             # Serve the Juju GUI tests.
             (r'^/test/(.*)', web.StaticFileHandler, params),

=== modified file 'server/guiserver/manage.py'
--- server/guiserver/manage.py	2013-08-05 09:38:39 +0000
+++ server/guiserver/manage.py	2013-08-09 13:41:32 +0000
@@ -100,8 +100,9 @@
         help='the Juju API version/implementation. Currently the possible '
              'values are "go" (default) or "python".')
     define(
-        'servetests', type=str,
-        help='The Juju GUI tests path. If not provided, tests are not served.')
+        'testsroot', type=str,
+        help='The filesystem path of the Juju GUI tests directory. '
+             'If not provided, tests are not served.')
     define(
         'sslpath', type=str, default=DEFAULT_SSL_PATH,
         help='The path where the SSL certificates are stored.')

=== modified file 'server/guiserver/tests/test_apps.py'
--- server/guiserver/tests/test_apps.py	2013-08-07 16:36:38 +0000
+++ server/guiserver/tests/test_apps.py	2013-08-09 13:41:32 +0000
@@ -77,7 +77,7 @@
 
     def test_serving_gui_tests(self):
         # The server can be configured to serve GUI unit tests.
-        app = self.get_app(servetests='/my/tests/')
+        app = self.get_app(testsroot='/my/tests/')
         spec = self.get_url_spec(app, r'^/test/(.*)$')
         self.assertIsNotNone(spec)
         self.assertIn('path', spec.kwargs)
@@ -85,7 +85,7 @@
 
     def test_not_serving_gui_tests(self):
         # The server can be configured to avoid serving GUI unit tests.
-        app = self.get_app(servetests=None)
+        app = self.get_app(testsroot=None)
         spec = self.get_url_spec(app, r'^/test/(.*)$')
         self.assertIsNone(spec)
 

=== modified file 'server/runserver.py'
--- server/runserver.py	2013-08-07 09:50:50 +0000
+++ server/runserver.py	2013-08-09 13:41:32 +0000
@@ -20,8 +20,8 @@
     --guiroot="/var/lib/juju/agents/unit-juju-gui-0/charm/juju-gui/build-prod"
     --apiurl="wss://ec2-75-101-177-185.compute-1.example.com:17070"
     --apiversion="go"
-    --sslpath=/etc/ssl/juju-gui
-    --servetests
+    --sslpath="/etc/ssl/juju-gui"
+    --tests_root="/var/lib/juju/agents/unit-juju-gui-0/charm/juju-gui/test/"
     --insecure
 
 The --sslpath option is ignored if --insecure is set.

=== modified file 'server/setup.py'
--- server/setup.py	2013-07-17 15:20:47 +0000
+++ server/setup.py	2013-08-09 13:41:32 +0000
@@ -38,7 +38,7 @@
     keywords='juju gui server',
     packages=[
         PROJECT_NAME,
-        '{0}.tests'.format(PROJECT_NAME),
+        '{}.tests'.format(PROJECT_NAME),
     ],
     scripts=['runserver.py'],
     classifiers=[

=== modified file 'tests/20-functional.test'
--- tests/20-functional.test	2013-08-05 15:00:27 +0000
+++ tests/20-functional.test	2013-08-09 13:41:32 +0000
@@ -82,7 +82,7 @@
         Retry loading the page until the page is found or a timeout exception
         is raised.
         """
-        base_url = 'https://{0}:{1}'.format(hostname, self.port)
+        base_url = 'https://{}:{}'.format(hostname, self.port)
         url = urlparse.urljoin(base_url, path)
 
         def page_ready(driver):
@@ -134,7 +134,7 @@
             # started by the charm in the machine. Right now this does not
             # work in pyJuju, so the desired effect is achieved by keeping
             # track of started services and manually stopping them here.
-        target = 'ubuntu@{0}'.format(hostname)
+        target = 'ubuntu@{}'.format(hostname)
         for service in services:
             ssh(target, 'sudo', 'service', service, 'stop')
 
@@ -177,6 +177,20 @@
         # The staging environment contains five deployed services.
         self.assertSetEqual(set(STAGING_SERVICES), self.get_service_names())
 
+    def test_builtin_server(self):
+        # Ensure the Juju GUI and builtin server are correctly set up.
+        unit_info = self.juju_deploy(
+            self.charm, options={'builtin-server': 'true'})
+        hostname = unit_info['public-address']
+        self.navigate_to(hostname)
+        self.handle_browser_warning()
+        self.assertEnvironmentIsConnected()
+        conn = httplib.HTTPSConnection(hostname)
+        conn.request('HEAD', '/')
+        headers = conn.getresponse().getheaders()
+        server_header = dict(headers)['server']
+        self.assertIn('TornadoServer', server_header)
+
     def test_branch_source(self):
         # Ensure the Juju GUI is correctly deployed from a Bazaar branch.
         unit_info = self.juju_deploy(
@@ -192,7 +206,7 @@
             self.charm, options={'juju-gui-source': JUJU_GUI_TEST_BRANCH})
         hostname = unit_info['public-address']
         conn = httplib.HTTPSConnection(hostname)
-        conn.request('GET', '/')
+        conn.request('HEAD', '/')
         headers = conn.getresponse().getheaders()
         # There is only one Cache-Control header.
         self.assertEqual(zip(*headers)[0].count('cache-control'), 1)

=== modified file 'tests/test_backends.py'
--- tests/test_backends.py	2013-08-05 09:59:17 +0000
+++ tests/test_backends.py	2013-08-09 13:41:32 +0000
@@ -46,8 +46,8 @@
     """Ensure the correct mixins and property values are collected."""
 
     def test_staging_backend(self):
-        test_backend = backend.Backend(
-            config={'sandbox': False, 'staging': True})
+        test_backend = backend.Backend(config={
+            'sandbox': False, 'staging': True, 'builtin-server': False})
         mixin_names = get_mixin_names(test_backend)
         self.assertEqual(
             ('ImprovMixin', 'GuiMixin', 'HaproxyApacheMixin'),
@@ -57,8 +57,8 @@
             test_backend.debs)
 
     def test_sandbox_backend(self):
-        test_backend = backend.Backend(
-            config={'sandbox': True, 'staging': False})
+        test_backend = backend.Backend(config={
+            'sandbox': True, 'staging': False, 'builtin-server': False})
         mixin_names = get_mixin_names(test_backend)
         self.assertEqual(
             ('SandboxMixin', 'GuiMixin', 'HaproxyApacheMixin'),
@@ -68,8 +68,8 @@
             test_backend.debs)
 
     def test_python_backend(self):
-        test_backend = backend.Backend(
-            config={'sandbox': False, 'staging': False})
+        test_backend = backend.Backend(config={
+            'sandbox': False, 'staging': False, 'builtin-server': False})
         mixin_names = get_mixin_names(test_backend)
         self.assertEqual(
             ('PythonMixin', 'GuiMixin', 'HaproxyApacheMixin'),
@@ -86,8 +86,8 @@
         # Create a fake agent file.
         agent_path = os.path.join(base_dir, 'agent.conf')
         open(agent_path, 'w').close()
-        test_backend = backend.Backend(
-            config={'sandbox': False, 'staging': False})
+        test_backend = backend.Backend(config={
+            'sandbox': False, 'staging': False, 'builtin-server': False})
         # Cleanup.
         utils.CURRENT_DIR = orig_current_dir
         shutil.rmtree(base_dir)
@@ -118,6 +118,8 @@
             'find_missing_packages': utils.find_missing_packages,
             'get_api_address': utils.get_api_address,
             'get_npm_cache_archive_url': utils.get_npm_cache_archive_url,
+            'install_builtin_server': utils.install_builtin_server,
+            'install_tornado': utils.install_tornado,
             'parse_source': utils.parse_source,
             'prime_npm_cache': utils.prime_npm_cache,
             'remove_apache_setup': utils.remove_apache_setup,
@@ -128,6 +130,7 @@
             'setup_haproxy_config': utils.setup_haproxy_config,
             'start_agent': utils.start_agent,
             'start_improv': utils.start_improv,
+            'write_builtin_server_startup': utils.write_builtin_server_startup,
             'write_gui_config': utils.write_gui_config,
         }
         self.charmhelpers_mocks = {
@@ -192,11 +195,12 @@
             self.assertTrue(
                 self.called.get(mocked), '{} was not called'.format(mocked))
 
-    def test_install_improv(self):
+    def test_install_improv_builtin(self):
         test_backend = backend.Backend(config=self.alwaysTrue)
         test_backend.install()
         for mocked in (
             'apt_get_install', 'fetch_api', 'find_missing_packages',
+            'install_builtin_server', 'install_tornado',
         ):
             self.assertTrue(
                 self.called.get(mocked), '{} was not called'.format(mocked))
@@ -211,12 +215,12 @@
             self.assertTrue(
                 self.called.get(mocked), '{} was not called'.format(mocked))
 
-    def test_start_improv(self):
+    def test_start_improv_builtin(self):
         test_backend = backend.Backend(config=self.alwaysTrue)
         test_backend.start()
         for mocked in (
             'compute_build_dir', 'open_port', 'start_improv', 'su',
-            'write_gui_config',
+            'write_builtin_server_startup', 'write_gui_config',
         ):
             self.assertTrue(
                 self.called.get(mocked), '{} was not called'.format(mocked))
@@ -231,16 +235,20 @@
 
     def test_same_config(self):
         test_backend = backend.Backend(
-            config={'sandbox': False, 'staging': False},
-            prev_config={'sandbox': False, 'staging': False},
+            config={
+                'sandbox': False, 'staging': False, 'builtin-server': False},
+            prev_config={
+                'sandbox': False, 'staging': False, 'builtin-server': False},
         )
         self.assertFalse(test_backend.different('sandbox'))
         self.assertFalse(test_backend.different('staging'))
 
     def test_different_config(self):
         test_backend = backend.Backend(
-            config={'sandbox': False, 'staging': False},
-            prev_config={'sandbox': True, 'staging': False},
+            config={
+                'sandbox': False, 'staging': False, 'builtin-server': False},
+            prev_config={
+                'sandbox': True, 'staging': False, 'builtin-server': False},
         )
         self.assertTrue(test_backend.different('sandbox'))
         self.assertFalse(test_backend.different('staging'))

=== modified file 'tests/test_utils.py'
--- tests/test_utils.py	2013-08-07 09:50:50 +0000
+++ tests/test_utils.py	2013-08-09 13:41:32 +0000
@@ -45,6 +45,8 @@
     log_hook,
     parse_source,
     get_npm_cache_archive_url,
+    install_builtin_server,
+    install_tornado,
     remove_apache_setup,
     remove_haproxy_setup,
     render_to_file,
@@ -52,11 +54,14 @@
     setup_apache_config,
     setup_haproxy_config,
     start_agent,
+    start_builtin_server,
     start_haproxy_apache,
     start_improv,
     stop_agent,
+    stop_builtin_server,
     stop_haproxy_apache,
     stop_improv,
+    write_builtin_server_startup,
     write_gui_config,
 )
 # Import the whole utils package for monkey patching.
@@ -379,7 +384,7 @@
 
     def setUp(self):
         self.zookeeper_address = 'example.com:2000'
-        contents = 'env JUJU_ZOOKEEPER="{0}"\n'.format(self.zookeeper_address)
+        contents = 'env JUJU_ZOOKEEPER="{}"\n'.format(self.zookeeper_address)
         with tempfile.NamedTemporaryFile(delete=False) as agent_file:
             agent_file.write(contents)
             self.agent_file_path = agent_file.name
@@ -674,30 +679,15 @@
                 result, build_dir, 'in_staging: {}, serve_tests: {}'.format(
                     in_staging, serve_tests))
 
-    def test_write_gui_config(self):
-        write_gui_config(
-            False, 'This is login help.', True, True, self.charmworld_url,
-            self.build_dir, use_analytics=True, config_js_path='config')
-        js_conf = self.files['config']
-        self.assertIn('consoleEnabled: false', js_conf)
-        self.assertIn('user: "admin"', js_conf)
-        self.assertIn('password: "admin"', js_conf)
-        self.assertIn('login_help: "This is login help."', js_conf)
-        self.assertIn('readOnly: true', js_conf)
-        self.assertIn("socket_url: 'wss://", js_conf)
-        self.assertIn('socket_protocol: "wss"', js_conf)
-        self.assertIn('charmworldURL: "http://charmworld.example"', js_conf)
-        self.assertIn('useAnalytics: true', js_conf)
-
     def test_setup_haproxy_config(self):
         setup_haproxy_config(self.ssl_cert_path)
         haproxy_conf = self.files['haproxy.cfg']
-        self.assertIn('ca-base {0}'.format(self.ssl_cert_path), haproxy_conf)
-        self.assertIn('crt-base {0}'.format(self.ssl_cert_path), haproxy_conf)
-        self.assertIn('ws1 127.0.0.1:{0}'.format(API_PORT), haproxy_conf)
-        self.assertIn('web1 127.0.0.1:{0}'.format(WEB_PORT), haproxy_conf)
-        self.assertIn('ca-file {0}'.format(JUJU_PEM), haproxy_conf)
-        self.assertIn('crt {0}'.format(JUJU_PEM), haproxy_conf)
+        self.assertIn('ca-base {}'.format(self.ssl_cert_path), haproxy_conf)
+        self.assertIn('crt-base {}'.format(self.ssl_cert_path), haproxy_conf)
+        self.assertIn('ws1 127.0.0.1:{}'.format(API_PORT), haproxy_conf)
+        self.assertIn('web1 127.0.0.1:{}'.format(WEB_PORT), haproxy_conf)
+        self.assertIn('ca-file {}'.format(JUJU_PEM), haproxy_conf)
+        self.assertIn('crt {}'.format(JUJU_PEM), haproxy_conf)
         self.assertIn('redirect scheme https', haproxy_conf)
 
     def test_remove_haproxy_setup(self):
@@ -708,9 +698,9 @@
         setup_apache_config(self.build_dir, serve_tests=True)
         apache_site_conf = self.files['SITE_NOT_THERE']
         self.assertIn('juju-gui/build-', apache_site_conf)
-        self.assertIn('VirtualHost *:{0}'.format(WEB_PORT), apache_site_conf)
+        self.assertIn('VirtualHost *:{}'.format(WEB_PORT), apache_site_conf)
         self.assertIn(
-            'Alias /test {0}/test/'.format(JUJU_GUI_DIR), apache_site_conf)
+            'Alias /test {}/test/'.format(JUJU_GUI_DIR), apache_site_conf)
         apache_ports_conf = self.files['PORTS_NOT_THERE']
         self.assertIn('NameVirtualHost *:8000', apache_ports_conf)
         self.assertIn('Listen 8000', apache_ports_conf)
@@ -720,7 +710,7 @@
         self.assertEqual(self.run_call_count, 3)
 
     def test_start_haproxy_apache(self):
-        start_haproxy_apache('build_dir', False, self.ssl_cert_path, True)
+        start_haproxy_apache(JUJU_GUI_DIR, False, self.ssl_cert_path, True)
         self.assertEqual(self.svc_ctl_call_count, 2)
         self.assertEqual(self.service_names, ['apache2', 'haproxy'])
         self.assertEqual(
@@ -732,6 +722,54 @@
         self.assertEqual(self.service_names, ['haproxy', 'apache2'])
         self.assertEqual(self.actions, [charmhelpers.STOP, charmhelpers.STOP])
 
+    def test_install_tornado(self):
+        install_tornado()
+        self.assertEqual(self.run_call_count, 1)
+
+    def test_install_builtin_server(self):
+        install_builtin_server()
+        self.assertEqual(self.run_call_count, 1)
+
+    def test_write_builtin_server_startup(self):
+        write_builtin_server_startup(
+            JUJU_GUI_DIR, self.ssl_cert_path, serve_tests=True, insecure=True)
+        guiserver_conf = self.files['guiserver.conf']
+        self.assertIn('description "GUIServer"', guiserver_conf)
+        self.assertIn('--apiurl="ws://127.0.0.1:8080"', guiserver_conf)
+        self.assertIn('--apiversion="python"', guiserver_conf)
+        self.assertIn(
+            '--testsroot="{}/test/"'.format(JUJU_GUI_DIR), guiserver_conf)
+        self.assertIn('--insecure', guiserver_conf)
+
+    def test_start_builtin_server(self):
+        start_builtin_server(
+            JUJU_GUI_DIR, self.ssl_cert_path, False, insecure=False)
+        self.assertEqual(self.svc_ctl_call_count, 1)
+        self.assertEqual(self.service_names, ['guiserver'])
+        self.assertEqual(self.actions, [charmhelpers.RESTART])
+
+    def test_stop_builtin_server(self):
+        stop_builtin_server()
+        self.assertEqual(self.svc_ctl_call_count, 1)
+        self.assertEqual(self.service_names, ['guiserver'])
+        self.assertEqual(self.actions, [charmhelpers.STOP])
+        self.assertEqual(self.run_call_count, 1)
+
+    def test_write_gui_config(self):
+        write_gui_config(
+            False, 'This is login help.', True, True, self.charmworld_url,
+            self.build_dir, use_analytics=True, config_js_path='config')
+        js_conf = self.files['config']
+        self.assertIn('consoleEnabled: false', js_conf)
+        self.assertIn('user: "admin"', js_conf)
+        self.assertIn('password: "admin"', js_conf)
+        self.assertIn('login_help: "This is login help."', js_conf)
+        self.assertIn('readOnly: true', js_conf)
+        self.assertIn("socket_url: 'wss://", js_conf)
+        self.assertIn('socket_protocol: "wss"', js_conf)
+        self.assertIn('charmworldURL: "http://charmworld.example"', js_conf)
+        self.assertIn('useAnalytics: true', js_conf)
+
     def test_write_gui_config_insecure(self):
         write_gui_config(
             False, 'This is login help.', True, True, self.charmworld_url,