Merge lp:~bcsaller/pyjuju/lxc-repairs into lp:pyjuju

[Benjamin Saller]

Remove LXC restart partial support

This branch includes the bug fix (and a few trivials). The extra checks and features are in
the history but not included here.

https://codereview.appspot.com/6404052/

[Benjamin Saller (bcsaller)]

Reviewers: mp+115206_code.launchpad.net,

Message:
Please take a look.

Description:
Remove LXC restart partial support

Local lxc containers were initially intended to be ephemeral. This patch
makes that official
removing the partial restart of services via upstart from local
containers. It includes
a number of other minor tweaks including warning when a /tmp dir is used
for keeping
container state (as a developer could still manually restart containers
if state was stable).

With the change to ephemeral containers the /tmp change might not make
sense but the warning
shouldn't prove an issue and can easily prove useful in the future if we
choose to fully support
restarts.

https://code.launchpad.net/~bcsaller/juju/lxc-repairs/+merge/115206

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M juju/agents/machine.py
   M juju/control/destroy_environment.py
   A juju/lib/juju.png
   M juju/lib/lxc/__init__.py
   M juju/lib/lxc/data/juju-create
   M juju/lib/lxc/tests/test_lxc.py
   A juju/lib/notifier.py
   A juju/lib/service.py
   A juju/lib/tests/test_notifier.py
   A juju/lib/tests/test_service.py
   M juju/lib/upstart.py
   M juju/machine/tests/test_unit_deployment.py
   M juju/machine/unit.py
   M juju/providers/local/__init__.py
   M juju/providers/local/agent.py
   M juju/providers/local/tests/test_agent.py
   M juju/providers/local/tests/test_provider.py
   M setup.py

[Kapil Thangavelu (hazmat)]

as discussed in irc, this branch has three other features in addition to the bug fix which should be in another branch. The tmp folder guards are irrelevant given the branches focus. The ephemeral stuff needs more vetting with actual charms/usage. The desktop notification is cool, but again none of those is relevant to landing the core and important of the linked bug. pls yank those out and resubmit.

Besides that the actual bug fix uses /tmp paths for pid and logfiles which feels questionable. Also the generic service name unless its truly generic rather than a twisted daemon process launcher.

review: Needs Fixing

[Benjamin Saller (bcsaller)]

> as discussed in irc, this branch has three other features in addition to the
> bug fix which should be in another branch. The tmp folder guards are
> irrelevant given the branches focus. The ephemeral stuff needs more vetting
> with actual charms/usage. The desktop notification is cool, but again none of
> those is relevant to landing the core and important of the linked bug. pls
> yank those out and resubmit.
>
> Besides that the actual bug fix uses /tmp paths for pid and logfiles which
> feels questionable. Also the generic service name unless its truly generic
> rather than a twisted daemon process launcher.

Thanks for the review, I'll separate out the branch as we spoke about. I couldn't parse the last line of the review though, any help?

[Benjamin Saller (bcsaller)]

Please take a look.

https://codereview.appspot.com/6404052/

[Kapil Thangavelu (hazmat)]

lgtm, with two minors.

https://codereview.appspot.com/6404052/diff/1/juju/lib/service.py
File juju/lib/service.py (right):

https://codereview.appspot.com/6404052/diff/1/juju/lib/service.py#newcode56
juju/lib/service.py:56: return self._output_path
feels like this should be required, if pid file is. hardcoded /tmp files
are a red flag.

https://codereview.appspot.com/6404052/diff/1/juju/lib/service.py#newcode72
juju/lib/service.py:72: if "--pidfile" not in command:
class should probably get renamed twisted daemon service, or class
string documentation of this the parameter requirement.

https://codereview.appspot.com/6404052/diff/2001/juju/lib/service.py
File juju/lib/service.py (right):

https://codereview.appspot.com/6404052/diff/2001/juju/lib/service.py#newcode33
juju/lib/service.py:33: class ServiceManager(object):
naming nitpick, service manager is a bit ambiguious, and sounds like its
managing multiple services. Given the description it seems more like an
AgentService.

https://codereview.appspot.com/6404052/diff/2001/juju/lib/service.py#newcode55
juju/lib/service.py:55: return "/tmp/%s.output" % self._name
this should use the data dir for output.

https://codereview.appspot.com/6404052/diff/2001/juju/lib/service.py#newcode139
juju/lib/service.py:139: return False
nice

https://codereview.appspot.com/6404052/diff/2001/juju/lib/tests/test_service.py
File juju/lib/tests/test_service.py (right):

https://codereview.appspot.com/6404052/diff/2001/juju/lib/tests/test_service.py#newcode45
juju/lib/tests/test_service.py:45:
there should also be a real tests here, twistd has several builtin
services.

https://codereview.appspot.com/6404052/diff/2001/juju/providers/local/agent.py
File juju/providers/local/agent.py (right):

https://codereview.appspot.com/6404052/diff/2001/juju/providers/local/agent.py#newcode54
juju/providers/local/agent.py:54: pidfile =
get_temp_filename(juju_unit_namespace)
these can go into the data-dir instead of /tmp, and can be named instead
of mkstemp

https://codereview.appspot.com/6404052/

[lp:~bcsaller/pyjuju/lxc-repairs]

558. By Benjamin Saller on 2012-08-03

minor review feedback pre-merge

559. By Benjamin Saller on 2012-08-03

merge trunk

560. By Benjamin Saller on 2012-08-03

fix changed import

561. By Benjamin Saller on 2012-08-03

valid webserver test via twistd to test TwistedDaemon service manager

[Benjamin Saller (bcsaller)]

Changes made, test add, merging.

https://codereview.appspot.com/6404052/diff/2001/juju/lib/tests/test_service.py
File juju/lib/tests/test_service.py (right):

https://codereview.appspot.com/6404052/diff/2001/juju/lib/tests/test_service.py#newcode45
juju/lib/tests/test_service.py:45:
On 2012/07/26 13:03:13, hazmat wrote:
> there should also be a real tests here, twistd has several builtin
services.
Added test using twistd to spawn a webserver, some of the arg handling
of twistd is more specific than the manager expected, this was corrected
in the code, the result is more flexible.

https://codereview.appspot.com/6404052/

=== modified file 'juju/agents/machine.py'
--- juju/agents/machine.py	2012-06-21 22:12:18 +0000
+++ juju/agents/machine.py	2012-08-03 12:30:27 +0000
@@ -122,5 +122,5 @@
         return parser
 
 
-if __name__ == '__main__':
+if __name__ == "__main__":
     MachineAgent().run()

=== modified file 'juju/control/destroy_environment.py'
--- juju/control/destroy_environment.py	2012-04-04 13:00:13 +0000
+++ juju/control/destroy_environment.py	2012-08-03 12:30:27 +0000
@@ -24,7 +24,7 @@
     value = raw_input(
         "WARNING: this command will destroy the %r environment (type: %s).\n"
         "This includes all machines, services, data, and other resources. "
-        "Continue [y/N]" % (
+        "Continue [y/N] " % (
             environment.name, environment.type))
 
     if value.strip().lower() not in ("y", "yes"):

=== modified file 'juju/lib/lxc/__init__.py'
--- juju/lib/lxc/__init__.py	2012-05-22 22:08:15 +0000
+++ juju/lib/lxc/__init__.py	2012-08-03 12:30:27 +0000
@@ -61,7 +61,7 @@
 
 
 def _lxc_start(container_name, debug_log=None, console_log=None):
-    args = ["sudo", "lxc-start", "--daemon", "-n", container_name]
+    args = ["sudo", "lxc-start", "-d", "-n", container_name]
     if console_log:
         args.extend(["-c", console_log])
     if debug_log:

=== modified file 'juju/lib/lxc/data/juju-create'
--- juju/lib/lxc/data/juju-create	2012-06-22 17:01:27 +0000
+++ juju/lib/lxc/data/juju-create	2012-08-03 12:30:27 +0000
@@ -107,8 +107,8 @@
         apt-get update
         apt-get install $APT_OPTIONS juju python-txzookeeper
     elif [ $JUJU_ORIGIN = "proposed" ]; then
-	# Enabled testing of proposed updates
-	echo "deb http://archive.ubuntu.com/ubuntu/ $JUJU_SERIES-proposed main universe" >> /etc/apt/sources.list
+        # Enabled testing of proposed updates
+        echo "deb http://archive.ubuntu.com/ubuntu/ $JUJU_SERIES-proposed main universe" >> /etc/apt/sources.list
         apt-get update
         apt-get install $APT_OPTIONS juju python-txzookeeper
 
@@ -147,12 +147,6 @@
     echo "Without JUJU_PUBLIC_KEY you will not have ssh access to your container"
 fi
 
-
-if [ "$(id -u)" != "0" ]; then
-    echo "This script should be run as 'root'"
-    exit 1
-fi
-
 setup_apt_cache
 setup_networking
 setup_juju

=== added file 'juju/lib/service.py'
--- juju/lib/service.py	1970-01-01 00:00:00 +0000
+++ juju/lib/service.py	2012-08-03 12:30:27 +0000
@@ -0,0 +1,148 @@
+from twisted.internet.defer import inlineCallbacks
+from twisted.internet.threads import deferToThread
+
+from juju.errors import ServiceError
+
+import os
+import subprocess
+
+
+def _check_call(args, env=None, output_path=None):
+    if not output_path:
+        output_path = os.devnull
+
+    with open(output_path, "a") as f:
+        return subprocess.check_call(
+            args,
+            stdout=f.fileno(), stderr=f.fileno(),
+            env=env)
+
+
+def _cat(filename, use_sudo=False):
+    args = ("cat", filename)
+    if use_sudo and not os.access(filename, os.R_OK):
+        args = ("sudo",) + args
+
+    p = subprocess.Popen(
+        args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+    stdout_data, _ = p.communicate()
+    r = p.returncode
+    return (r, stdout_data)
+
+
+class TwistedDaemonService(object):
+    """Manage the starting and stopping of an Agent.
+
+    This manager tracks the agent via its --pidfile. The pidfile argument
+    specifies the location of the pid file that is used to track this service.
+
+    """
+    def __init__(self, name, pidfile, use_sudo=False):
+        self._name = name
+        self._use_sudo = use_sudo
+        self._description = None
+        self._environ = None
+        self._command = None
+        self._output_path = None
+
+        self._pid_path = pidfile
+        self._pid = None
+
+    @property
+    def output_path(self):
+        if self._output_path is not None:
+            return self._output_path
+        return "/tmp/%s.output" % self._name
+
+    @output_path.setter
+    def output_path(self, path):
+        self._output_path = path
+
+    def set_description(self, description):
+        self._description = description
+
+    def set_environ(self, environ):
+        for k, v in environ.items():
+            environ[k] = str(v)
+        self._environ = environ
+
+    def set_command(self, command):
+        if "--pidfile" not in command:
+            command += ["--pidfile", self._pid_path]
+        else:
+            # pid file is in command (consume it for get_pid)
+            idx = command.index("--pidfile")
+            self._pid_path = command[idx+1]
+
+        self._command = command
+
+    @inlineCallbacks
+    def _trash_output(self):
+        if os.path.exists(self.output_path):
+            # Just using os.unlink will fail when we're running TEST_SUDO
+            # tests which hit this code path (because root will own
+            # self.output_path)
+            yield self._call("rm", "-f", self.output_path)
+
+        if os.path.exists(self._pid_path):
+            yield self._call("rm", "-f", self._pid_path)
+
+    def _call(self, *args, **kwargs):
+        if self._use_sudo:
+            args = ("sudo", "-E") + args
+        return deferToThread(_check_call, args, env=self._environ,
+                             output_path=self.output_path)
+
+    def install(self):
+        if self._command is None:
+            raise ServiceError("Cannot manage agent: %s no command set" % (
+                self._name))
+
+    @inlineCallbacks
+    def start(self):
+        if (yield self.is_running()):
+            raise ServiceError(
+                "%s already running: pid (%s)" % (
+                    self._name, self.get_pid()))
+
+        if not self.is_installed():
+            yield self.install()
+
+        yield self._trash_output()
+        yield self._call(*self._command, output_path=self.output_path)
+
+    @inlineCallbacks
+    def destroy(self):
+        if (yield self.is_running()):
+            yield self._call("kill", self.get_pid())
+        yield self._trash_output()
+
+    def get_pid(self):
+        if self._pid != None:
+            return self._pid
+
+        if not os.path.exists(self._pid_path):
+            return None
+        r, data = _cat(self._pid_path, use_sudo=self._use_sudo)
+        if r != 0:
+            return None
+
+        # verify that pid is a number but leave
+        # it as a string suitable for passing to kill
+        if not data.strip().isdigit():
+            return None
+        pid = data.strip()
+        self._pid = pid
+        return self._pid
+
+    def is_running(self):
+        pid = self.get_pid()
+        if not pid:
+            return False
+        proc_file = "/proc/%s" % pid
+        if not os.path.exists(proc_file):
+            return False
+        return True
+
+    def is_installed(self):
+        return False

=== added file 'juju/lib/tests/test_service.py'
--- juju/lib/tests/test_service.py	1970-01-01 00:00:00 +0000
+++ juju/lib/tests/test_service.py	2012-08-03 12:30:27 +0000
@@ -0,0 +1,100 @@
+from twisted.internet.defer import inlineCallbacks
+
+from juju.lib.testing import TestCase
+from juju.lib.mocker import MATCH, KWARGS
+from juju.lib.service import TwistedDaemonService
+
+import os
+
+
+class TwistedDaemonServiceTest(TestCase):
+
+    @inlineCallbacks
+    def setUp(self):
+        yield super(TwistedDaemonServiceTest, self).setUp()
+        self.setup_service()
+
+    def setup_service(self):
+        service = TwistedDaemonService("juju-machine-agent",
+                                       "/tmp/.juju-test.pid",
+                                       use_sudo=False)
+        service.set_description("Juju machine agent")
+        service.set_environ({"JUJU_MACHINE_ID": 0})
+        service.set_command(["/bin/true", ])
+        self.service = service
+
+        if os.path.exists("/tmp/.juju-test.pid"):
+            os.remove("/tmp/.juju-test.pid")
+
+        return service
+
+    def setup_mock(self):
+        self.check_call = self.mocker.replace("subprocess.check_call")
+
+    def mock_call(self, args):
+        def exe_match(cmd):
+            cmd = " ".join(str(s) for s in cmd)
+            return cmd.startswith(args)
+
+        self.check_call(MATCH(exe_match), KWARGS)
+        self.mocker.result(0)
+
+    @inlineCallbacks
+    def test_simple_service_start(self):
+        self.setup_mock()
+        self.mock_call("/bin/true")
+        self.mocker.replay()
+
+        yield self.service.start()
+
+    def test_set_output_path(self):
+        # defaults work
+        self.assertEqual(self.service.output_path,
+                         "/tmp/juju-machine-agent.output")
+        # override works
+        self.service.output_path = "/tmp/valid.log"
+        self.assertEqual(self.service.output_path, "/tmp/valid.log")
+
+    @inlineCallbacks
+    def test_simple_service_start_destroy(self):
+        self.setup_mock()
+        mock_service = self.mocker.patch(self.service)
+        get_pid = mock_service.get_pid
+        get_pid()
+        self.mocker.result(1337)
+
+        is_running = mock_service.is_running
+        is_running()
+        self.mocker.result(False)
+
+        is_running()
+        self.mocker.result(True)
+
+        self.mock_call(("/bin/true", ))
+        self.mock_call(("kill", "1337"))
+        self.mocker.replay()
+
+        yield self.service.start()
+        yield self.service.destroy()
+
+    @inlineCallbacks
+    def test_webservice_start(self):
+        # test using a real twisted service (with --pidfile)
+        # arg ordering matters here so we set pidfile manually
+        self.service.set_command([
+            "env", "twistd",
+            "--pidfile", "/tmp/.juju-test.pid",
+            "--logfile", "/tmp/.juju-test.log",
+            "web",
+            "--port", "9871",
+            "--path", "/lib",
+            ])
+
+        yield self.service.start()
+        yield self.sleep(0.5)
+        self.assertTrue(self.service.get_pid())
+        self.assertTrue(self.service.is_running())
+        self.assertTrue(os.path.exists("/tmp/.juju-test.pid"))
+        yield self.service.destroy()
+        yield self.sleep(0.1)
+        self.assertFalse(os.path.exists("/tmp/.juju-test.pid"))
\ No newline at end of file

=== modified file 'juju/machine/unit.py'
--- juju/machine/unit.py	2012-04-04 10:13:38 +0000
+++ juju/machine/unit.py	2012-08-03 12:30:27 +0000
@@ -244,6 +244,7 @@
             "Juju unit agent for %s" % self.unit_name)
         service.set_environ(_get_environment(
             self.unit_name, "/var/lib/juju", machine_id, zookeeper_hosts))
+
         service.set_output_path(
             "/var/log/juju/unit-%s-output.log" % self.unit_path_name)
         service.set_command(" ".join((

=== modified file 'juju/providers/local/__init__.py'
--- juju/providers/local/__init__.py	2012-04-06 16:35:31 +0000
+++ juju/providers/local/__init__.py	2012-08-03 12:30:27 +0000
@@ -58,6 +58,9 @@
     def bootstrap(self, constraints):
         """Bootstrap a local development environment.
         """
+        # Validate `data-dir` or raise/warn
+        self._validate_data_dir()
+
         # Check for existing environment
         state = yield self.load_state()
         if state is not False:
@@ -77,11 +80,6 @@
         except LookupError, e:
             raise ProviderError(str(e))
 
-        # Get/create directory for zookeeper and files
-        zookeeper_dir = os.path.join(self._directory, "zookeeper")
-        if not os.path.exists(zookeeper_dir):
-            os.makedirs(zookeeper_dir)
-
         # Start networking, and get an open port.
         log.info("Starting networking...")
         net = Network("default", subnet=122)
@@ -92,8 +90,13 @@
         net_attributes = yield net.get_attributes()
         port = get_open_port(net_attributes["ip"]["address"])
 
-        # Start zookeeper
-        log.info("Starting zookeeper...")
+        # Get/create directory for zookeeper and files
+        zookeeper_dir = os.path.join(self._directory, "zookeeper")
+        if not os.path.exists(zookeeper_dir):
+            os.makedirs(zookeeper_dir)
+
+        # Start ZooKeeper
+        log.info("Starting ZooKeeper...")
         # Run zookeeper as the current user, unless we're being run as root
         # in which case run zookeeper as the 'zookeeper' user.
         zookeeper_user = None
@@ -102,7 +105,8 @@
         zookeeper = Zookeeper(zookeeper_dir,
                               port=port,
                               host=net_attributes["ip"]["address"],
-                              user=zookeeper_user, group=zookeeper_user)
+                              user=zookeeper_user,
+                              group=zookeeper_user)
 
         yield zookeeper.start()
 
@@ -143,12 +147,21 @@
                                     juju_origin=juju_origin,
                                     public_key=public_key,
                                     juju_series=self.config["default-series"])
+
         log.info(
             "Starting machine agent (origin: %s)... ", agent.juju_origin)
         yield agent.start()
 
         log.info("Environment bootstrapped")
 
+    def _validate_data_dir(self):
+        data_dir = self.config["data-dir"]
+        if not os.path.exists(data_dir):
+            raise ProviderError("`data-dir` does not exist")
+
+        if not os.access(data_dir, os.W_OK):
+            raise ProviderError("`data-dir` not writable")
+
     @inlineCallbacks
     def destroy_environment(self):
         """Shutdown the machine environment.

=== modified file 'juju/providers/local/agent.py'
--- juju/providers/local/agent.py	2012-02-21 12:14:11 +0000
+++ juju/providers/local/agent.py	2012-08-03 12:30:27 +0000
@@ -1,9 +1,18 @@
 import sys
+import tempfile
 
-from juju.lib.upstart import UpstartService
+from juju.lib.service import TwistedDaemonService
 from juju.providers.common.cloudinit import get_default_origin, BRANCH
 
 
+def get_temp_filename(basename):
+    if basename:
+        basename += "-"
+
+    fd, fn = tempfile.mkstemp(prefix=basename, suffix=".pid")
+    return fn
+
+
 class ManagedMachineAgent(object):
 
     agent_module = "juju.agents.machine"
@@ -11,7 +20,7 @@
     def __init__(
             self, juju_unit_namespace, zookeeper_hosts=None,
             machine_id="0", log_file=None, juju_directory="/var/lib/juju",
-            public_key=None, juju_origin="ppa", juju_series=None):
+            public_key=None, juju_origin=None, juju_series=None):
         """
         :param juju_series: The release series to use (maverick, natty, etc).
         :param machine_id: machine id for the local machine.
@@ -42,14 +51,22 @@
         if public_key:
             env["JUJU_PUBLIC_KEY"] = public_key
 
-        self._service = UpstartService(
-            "juju-%s-machine-agent" % juju_unit_namespace, use_sudo=True)
+        pidfile = get_temp_filename(juju_unit_namespace)
+        self._service = TwistedDaemonService(
+            "juju-%s-machine-agent" % juju_unit_namespace,
+            pidfile,
+            use_sudo=True)
         self._service.set_description(
             "Juju machine agent for %s" % juju_unit_namespace)
         self._service.set_environ(env)
+        self._service.output_path = log_file
+
         self._service_args = [
             "/usr/bin/python", "-m", self.agent_module,
-            "--nodaemon", "--logfile", log_file,
+            "--logfile", log_file,
+            "--zookeeper-servers", zookeeper_hosts,
+            "--juju-directory", juju_directory,
+            "--machine-id", machine_id,
             "--session-file",
             "/var/run/juju/%s-machine-agent.zksession" % juju_unit_namespace]
 
@@ -59,7 +76,7 @@
 
     def start(self):
         """Start the machine agent."""
-        self._service.set_command(" ".join(self._service_args))
+        self._service.set_command(self._service_args)
         return self._service.start()
 
     def stop(self):

=== modified file 'juju/providers/local/tests/test_agent.py'
--- juju/providers/local/tests/test_agent.py	2012-02-21 12:14:11 +0000
+++ juju/providers/local/tests/test_agent.py	2012-08-03 12:30:27 +0000
@@ -7,7 +7,6 @@
 
 from juju.lib.lxc.tests.test_lxc import uses_sudo
 from juju.lib.testing import TestCase
-from juju.lib.upstart import UpstartService
 from juju.tests.common import get_test_zookeeper_address
 from juju.providers.local.agent import ManagedMachineAgent
 
@@ -19,26 +18,14 @@
         subprocess_calls = []
 
         def intercept_args(args, **kwargs):
+            self.assertEquals(args[0], "sudo")
+            self.assertEquals(args[1], "-E")
+            if args[2] == "rm":
+                return 0
             subprocess_calls.append(args)
-            self.assertEquals(args[0], "sudo")
-            if args[1] == "cp":
-                return real_check_call(args[1:], **kwargs)
             return 0
 
-        real_check_call = self.patch(subprocess, "check_call", intercept_args)
-        init_dir = self.makeDir()
-        self.patch(UpstartService, "init_dir", init_dir)
-
-        # Mock out the repeated checking for unstable pid, after an initial
-        # stop/waiting to induce the actual start
-        getProcessOutput = self.mocker.replace(
-            "twisted.internet.utils.getProcessOutput")
-        getProcessOutput("/sbin/status", ["juju-ns1-machine-agent"])
-        self.mocker.result(succeed("stop/waiting"))
-        for _ in range(5):
-            getProcessOutput("/sbin/status", ["juju-ns1-machine-agent"])
-            self.mocker.result(succeed("start/running 123"))
-        self.mocker.replay()
+        self.patch(subprocess, "check_call", intercept_args)
 
         juju_directory = self.makeDir()
         log_file = self.makeFile()
@@ -51,44 +38,21 @@
             juju_origin="lp:juju/trunk")
 
         try:
-            os.remove("/tmp/juju-ns1-machine-agent.output")
+            os.remove(agent._service.output_path)
         except OSError:
             pass  # just make sure it's not there, so the .start()
                   # doesn't insert a spurious rm
-
         yield agent.start()
 
-        conf_dest = os.path.join(
-            init_dir, "juju-ns1-machine-agent.conf")
-        chmod, start = subprocess_calls[1:]
-        self.assertEquals(chmod, ("sudo", "chmod", "644", conf_dest))
-        self.assertEquals(
-            start, ("sudo", "/sbin/start", "juju-ns1-machine-agent"))
-
-        env = []
-        with open(conf_dest) as f:
-            for line in f:
-                if line.startswith("env"):
-                    env.append(line[4:-1].split("=", 1))
-                if line.startswith("exec"):
-                    exec_ = line[5:-1]
-
-        expect_exec = (
-            "/usr/bin/python -m juju.agents.machine --nodaemon --logfile %s "
-            "--session-file /var/run/juju/ns1-machine-agent.zksession "
-            ">> /tmp/juju-ns1-machine-agent.output 2>&1"
-            % log_file)
-        self.assertEquals(exec_, expect_exec)
-
-        env = dict((k, v.strip('"')) for (k, v) in env)
-        self.assertEquals(env, {
-            "JUJU_ZOOKEEPER": get_test_zookeeper_address(),
-            "JUJU_MACHINE_ID": "0",
-            "JUJU_HOME": juju_directory,
-            "JUJU_ORIGIN": "lp:juju/trunk",
-            "JUJU_UNIT_NS": "ns1",
-            "JUJU_SERIES": "precise",
-            "PYTHONPATH": ":".join(sys.path)})
+        start = subprocess_calls[0]
+        self.assertEquals(start, (
+            "sudo", "-E", "/usr/bin/python", "-m", "juju.agents.machine",
+            "--logfile", log_file,
+            "--zookeeper-servers", get_test_zookeeper_address(),
+            "--juju-directory", juju_directory,
+            "--machine-id", "0",
+            "--session-file",  "/var/run/juju/ns1-machine-agent.zksession",
+            "--pidfile", agent._service._pid_path))
 
     @uses_sudo
     @inlineCallbacks

=== modified file 'juju/providers/local/tests/test_provider.py'
--- juju/providers/local/tests/test_provider.py	2012-03-29 01:37:57 +0000
+++ juju/providers/local/tests/test_provider.py	2012-08-03 12:30:27 +0000
@@ -101,7 +101,7 @@
             sorted(children))
         output = self.output.getvalue()
         self.assertIn("Starting networking...", output)
-        self.assertIn("Starting zookeeper...", output)
+        self.assertIn("Starting ZooKeeper...", output)
         self.assertIn("Initializing state...", output)
         self.assertIn("Starting storage server", output)
         self.assertIn("Starting machine agent", output)
@@ -117,6 +117,23 @@
                           "zookeeper-instances": ["local"]})
 
     @inlineCallbacks
+    def test_bootstrap_bad_data_dir(self):
+        data_dir = self.makeDir()
+        # make it read only
+        os.chmod(data_dir, 0444)
+
+        self.provider = MachineProvider(
+            "local-test", {
+                "admin-secret": "admin:abc", "data-dir": data_dir,
+                "authorized-keys": "fooabc123", "default-series": "oneiric"})
+
+        self.mocker.replay()
+
+        error = yield self.assertFailure(
+            self.provider.bootstrap(self.constraints), ProviderError)
+        self.assertIn("`data-dir` not writable", str(error))
+
+    @inlineCallbacks
     def test_boostrap_previously_bootstrapped(self):
         """Any local state is a sign that we had a previous bootstrap.
         """

=== modified file 'setup.py'
--- setup.py	2012-03-23 02:13:50 +0000
+++ setup.py	2012-08-03 12:30:27 +0000
@@ -16,14 +16,14 @@
         pass
     packages = []
     for directory, subdirectories, files in os.walk("juju"):
-        if '__init__.py' in files:
+        if "__init__.py" in files:
             packages.append(directory.replace(os.sep, '.'))
     return packages
 
 
 setup(
     name="juju",
-    version="0.5", # Change debian/changelog too, for daily builds.
+    version="0.5",  # Change debian/changelog too, for daily builds.
     description="Cloud automation and orchestration",
     author="Juju Developers",
     author_email="[e-mail]",