Merge lp:~fwereade/pyjuju/cloud-init-class-used into lp:pyjuju

[William Reade]

Another attempt at a cleaner cloud-init/machine-data mechanism. Comments on https://code.launchpad.net/~fwereade/ensemble/cloud-init-class-replace/+merge/72399 still apply, except that the unbugged prereq (whose intent has not changed) is now lp:~fwereade/ensemble/cloud-init-class-prepare; the only thing to add is that I *know* the docstrings are a mess, and I'm just about to start a new stacked branch to bring them all in line with the agreed standard.

[Gustavo Niemeyer (niemeyer)]

This looks _very_ nice. Thanks William!

+1, with the following sorted:

[1]

+ self._zookeeper_names = [m.private_dns_name for m in machines]
(...)
+ def _zookeeper_hosts(self):
+ all_names = self._zookeeper_names[:]

The branch has two different names for the same data in different
formats. The better approach is the opposite: the same name with
different actions that give a hint about the undelying operation.

I suggest using "zookeeper_hosts" for both, and naming the last
function as join_zookeeper_hosts or something similar.

[2]

+ def _validate(self):
+ missing = []

Very nice logic there.

[3]

+ def _create_cloud_init(self, machine_id, zookeepers):
+ config = self._provider.config
+ cloud_init = CloudInit()
+ cloud_init.add_ssh_key(get_user_authorized_keys(config))
+ cloud_init.set_machine_id(machine_id)
+ cloud_init.set_zookeeper_machines(zookeepers)
+ if config.get("ensemble-branch"):
+ cloud_init.set_ensemble_source(config["ensemble-branch"])
+ if self._master:
+ cloud_init.enable_bootstrap()
+ cloud_init.set_zookeeper_secret(config["admin-secret"])
+ return cloud_init

Beautiful! :-)

[4]

+ instance_type = self._provider.config.get(
+ "default-instance-type", "m1.small")
+ image_id = yield get_image_id(self._provider.config, self._constraints)
+ security_groups = yield self._ensure_groups(machine_id)
+
+ instances = yield self._provider.ec2.run_instances(
+ image_id=image_id,
+ instance_type=instance_type,
+ min_count=1,
+ max_count=1,
+ security_groups=security_groups,
+ user_data=user_data)

Very clean as well, thanks!

[5]

+def get_image_id(config, constraints):
(...)
+_KWARG_NAMES = {
+ "ubuntu_release": "ubuntu_release_name",
+ "architecture": "arch",
+ "daily": "daily",
+ "persistent_storage": "ebs"}
(...)
+def _convert_constraints(constraints):
+ kwargs = {}
+ for (constraint_name, kwarg_name) in _KWARG_NAMES.items():
+ if constraint_name in constraints:
+ kwargs[kwarg_name] = constraints[constraint_name]
+ return kwargs
(...)
+ kwargs = _convert_constraints(constraints)

Why are we doing this? These names in the right-hand-side of the
_KWARG_NAMES are keyword arguments of get_current_ami, which is a function
on the same file (!). The left-hand-side ones are not even used in
any sensible way in our current implementation.

Let's just unify the naming and avoid any conversions entirely. Feel
free to pick sensible ones.

[6]

Again, this looks very nice. Please just make sure you merge trunk into
this, and do one final real-world test on EC2 before committing back
into trunk.

review: Approve

[lp:~fwereade/pyjuju/cloud-init-class-used]

339. By William Reade on 2011-09-06

merge parent

340. By William Reade on 2011-09-06

renaming in CloudInit

341. By William Reade on 2011-09-06

get_current_ami now uses same keys as machine_data constraints dict

[Gustavo Niemeyer (niemeyer)]

Thanks for the changes William.

[7]

+# XXX ideally should come from latest available or client release name.

FWIW, it should come from the formula Ubuntu series instead.

review: Approve

[Gustavo Niemeyer (niemeyer)]

Please go ahead and merge this William. It's been in review for 2 weeks already.

[Kapil Thangavelu (hazmat)]

looks great, bye-bye anonymous blobs. some comments.

[0]

- @param machine_data: a dictionary of data to pass along to the newly
- launched machine.
+ `machine_data`: a dictionary describing the newly launched machine.

'describing the machine to be launched.'

[1]

+ def set_provider_type(self, type_):
+ self._provider_type = type_

type isn't a keyword

[2]
+ def set_machine_id(self, id):
+ self._machine_id = id
+
+ def set_instance_id(self, expr):
+ self._instance_id = expr

+ if self._zookeeper:
+ require("_instance_id", "set_instance_id")
+ require("_zookeeper_secret", "set_zookeeper_secret")

what's the distinction between the two variables. instance id comes from the provider
implementation as a consequence of actually running the machine, its an output value, not
an input value to cloud init afaics.

its apparent a little bit latter into the diff, this is actually a shell command for retrieval
of instance id.. the naming should be reflective of that. perhaps set_instance_id_accessor

what's the case for the instance id not being set? is that just for testing ease, if so it should probably be commented as such.

review: Approve

[William Reade (fwereade)]

> looks great, bye-bye anonymous blobs. some comments.
>
> [0]
>
> - @param machine_data: a dictionary of data to pass along to the newly
> - launched machine.
> + `machine_data`: a dictionary describing the newly launched machine.
>
> 'describing the machine to be launched.'

Thanks

> [1]
>
> + def set_provider_type(self, type_):
> + self._provider_type = type_
>
> type isn't a keyword

But it is a builtin, and I prefer not to shadow them unless I absopositively have to.

> [2]
> + def set_machine_id(self, id):
> + self._machine_id = id
> +
> + def set_instance_id(self, expr):
> + self._instance_id = expr
>
> + if self._zookeeper:
> + require("_instance_id", "set_instance_id")
> + require("_zookeeper_secret", "set_zookeeper_secret")
>
> what's the distinction between the two variables. instance id comes from the
> provider
> implementation as a consequence of actually running the machine, its an output
> value, not
> an input value to cloud init afaics.
>
> its apparent a little bit latter into the diff, this is actually a shell
> command for retrieval
> of instance id.. the naming should be reflective of that. perhaps
> set_instance_id_accessor

Good idea

> what's the case for the instance id not being set? is that just for testing
> ease, if so it should probably be commented as such.

It's just that it's not actually used, except when we're running a bootstrap node. This could well change in the future.

[lp:~fwereade/pyjuju/cloud-init-class-used]

342. By William Reade on 2011-09-13

merge parent

343. By William Reade on 2011-09-13

fixes per hazmat's review

344. By William Reade on 2011-09-13

cloud-inits now contain lists of ssh keys, not list-of-lists of ssh keys

=== modified file 'ensemble/providers/common/base.py'
--- ensemble/providers/common/base.py	2011-08-29 02:23:11 +0000
+++ ensemble/providers/common/base.py	2011-09-13 10:56:30 +0000
@@ -65,11 +65,10 @@
     def start_machine(self, machine_data, master=False):
         """Start a machine in the provider.
 
-        @param machine_data: a dictionary of data to pass along to the newly
-           launched machine.
+        `machine_data`: a dictionary describing the machine to be launched.
 
-        @param master: if True, machine will initialize the ensemble admin
-            and run a provisioning agent.
+        `master`: if True, machine will initialize the ensemble admin and run
+            a provisioning agent, in addition to running a machine agent
         """
         raise NotImplementedError()
 

=== modified file 'ensemble/providers/common/cloudinit.py'
--- ensemble/providers/common/cloudinit.py	2011-09-13 10:56:30 +0000
+++ ensemble/providers/common/cloudinit.py	2011-09-13 10:56:30 +0000
@@ -60,7 +60,7 @@
         self._ssh_keys = []
         self._provision = False
         self._zookeeper = False
-        self._zookeeper_names = []
+        self._zookeeper_hosts = []
         self._zookeeper_secret = None
 
     def add_ssh_key(self, key):
@@ -83,14 +83,14 @@
     def set_machine_id(self, id):
         self._machine_id = id
 
-    def set_instance_id(self, expr):
+    def set_instance_id_accessor(self, expr):
         self._instance_id = expr
 
     def set_provider_type(self, type_):
         self._provider_type = type_
 
     def set_zookeeper_machines(self, machines):
-        self._zookeeper_names = [m.private_dns_name for m in machines]
+        self._zookeeper_hosts = [m.private_dns_name for m in machines]
 
     def set_zookeeper_secret(self, secret):
         self._zookeeper_secret = secret
@@ -115,20 +115,19 @@
         require("_machine_id", "set_machine_id")
         require("_provider_type", "set_provider_type")
         if self._zookeeper:
-            require("_instance_id", "set_instance_id")
+            require("_instance_id", "set_instance_id_accessor")
             require("_zookeeper_secret", "set_zookeeper_secret")
         else:
-            require("_zookeeper_names", "set_zookeeper_machines")
+            require("_zookeeper_hosts", "set_zookeeper_machines")
         if missing:
             raise CloudInitError("Incomplete cloud-init: you need to call %s"
                                  % ", ".join(missing))
 
-    @property
-    def _zookeeper_hosts(self):
-        all_names = self._zookeeper_names[:]
+    def _join_zookeeper_hosts(self):
+        all_hosts = self._zookeeper_hosts[:]
         if self._zookeeper:
-            all_names.append("localhost")
-        return ",".join(["%s:2181" % name for name in all_names])
+            all_hosts.append("localhost")
+        return ",".join(["%s:2181" % host for host in all_hosts])
 
     def _collect_packages(self):
         packages = [
@@ -150,13 +149,13 @@
             scripts.extend(_zookeeper_scripts(
                 self._instance_id, self._zookeeper_secret))
         scripts.extend(_machine_scripts(
-            self._machine_id, self._zookeeper_hosts))
+            self._machine_id, self._join_zookeeper_hosts()))
         if self._provision:
-            scripts.extend(_provision_scripts(self._zookeeper_hosts))
+            scripts.extend(_provision_scripts(self._join_zookeeper_hosts()))
         return scripts
 
     def _collect_machine_data(self):
         return {
             "machine-id": self._machine_id,
             "ensemble-provider-type": self._provider_type,
-            "ensemble-zookeeper-hosts": self._zookeeper_hosts}
+            "ensemble-zookeeper-hosts": self._join_zookeeper_hosts()}

=== modified file 'ensemble/providers/common/launch.py'
--- ensemble/providers/common/launch.py	2011-08-17 16:01:43 +0000
+++ ensemble/providers/common/launch.py	2011-09-13 10:56:30 +0000
@@ -1,195 +1,71 @@
-import copy
-
 from twisted.internet.defer import inlineCallbacks, returnValue
 
-from ensemble.errors import ProviderError
-from ensemble.state.auth import make_identity
-
+from .cloudinit import CloudInit
 from .utils import get_user_authorized_keys
 
-DEFAULT_REPOSITORIES = [
-    "ppa:ensemble/ppa"]
-
-DEFAULT_PACKAGES = [
-    "bzr",
-    "byobu",
-    "tmux",
-    "python-setuptools",
-    "python-twisted",
-    "python-argparse",
-    "python-txaws",
-    "python-zookeeper"]
-
-BOOTSTRAP_PACKAGES = [
-    "bzr",
-    "default-jre-headless",
-    "zookeeper",
-    "zookeeperd"]
-
-
-def _get_initialize_script(instance_id_command, admin_secret):
-    admin_identity = make_identity("admin:%s" % admin_secret)
-    template = ('ensemble-admin initialize '
-               '--instance-id=%s --admin-identity="%s"')
-    return template % (instance_id_command, admin_identity)
-
-
-def _get_machine_agent_script(vars):
-    template = (
-        "ENSEMBLE_MACHINE_ID=%(machine-id)s "
-        "ENSEMBLE_ZOOKEEPER=%(ensemble-zookeeper-hosts)s "
-        "python -m ensemble.agents.machine -n "
-        "--logfile=/var/log/ensemble/machine-agent.log "
-        "--pidfile=/var/run/ensemble/machine-agent.pid")
-    return template % vars
-
-
-def _get_provision_agent_script(vars):
-    template = (
-        "ENSEMBLE_ZOOKEEPER=%(ensemble-zookeeper-hosts)s "
-        "python -m ensemble.agents.provision -n "
-        "--logfile=/var/log/ensemble/provision-agent.log "
-        "--pidfile=/var/run/ensemble/provision-agent.pid")
-    return template % vars
-
 
 class LaunchMachine(object):
     """Abstract class with generic instance-launching logic.
 
-    Constructing with master=True will cause the run method to
-    construct a machine which is also running a zookeeper for the
-    cluster, and a provisioning agent, as well as the usual machine
-    agent.
-
     To create your own subclass, you will certainly need to override
-    C{start_machine} and C{get_instance_id_command}, and may find it
-    convenient to override C{get_machine_variables} as well.
+    `start_machine`, and will very probably want to implement it with the aid
+    of a provider-specific subclass of `ProviderCloudInit`.
+
+    `provider`: the necessary `MachineProvider`
+
+    `master`: if True, the machine will run a zookeeper and a provisioning
+        agent, in addition to the machine agent that every machine runs
     """
 
-    def __init__(self, provider, master=False):
+    def __init__(self, provider, master=False, constraints=None):
         self._provider = provider
         self._master = master
+        self._constraints = constraints or {}
 
     @inlineCallbacks
-    def run(self, machine_data):
+    def run(self, machine_id):
         """Launch an instance node within the machine provider environment.
 
-        `machine_data`: a dictionary of data that is passed along to
-            provided to the machine as serialized yaml. 'machine-id' is a
-            required key, denoting the machine's zookeeper node for its
-            machine agent.
+        `machine_id`: the ensemble machine ID
         """
-        if "machine-id" not in machine_data:
-            raise ProviderError(
-                "Machine state `machine-id` not provided in machine_data.")
-
-        machine_variables = yield self.get_machine_variables(machine_data)
-        provider_machines = yield self.start_machine(
-            machine_variables, machine_data["machine-id"])
-        if self._master:
-            yield self._on_master_launched(provider_machines)
-        returnValue(provider_machines)
-
-    def start_machine(self, machine_variables, machine_id):
+        # XXX at some point, we'll want to start multiple zookeepers
+        # that know about each other; for now, this'll do
+        if self._master:
+            zookeepers = []
+        else:
+            zookeepers = yield self._provider.get_zookeeper_machines()
+
+        machines = yield self.start_machine(machine_id, zookeepers)
+        if self._master:
+            yield self._on_new_zookeepers(machines)
+        returnValue(machines)
+
+    def start_machine(self, machine_id, zookeepers):
         """Actually launch a machine for the appropriate provider.
 
-        `machine_variables`: non-provider-specific data, sufficient to
-            define the machine's behaviour once it exists.
-
-        `machine_id`: the external machine ID (also exists in the
-            above bag of data)
-
-        Returns a singe-entry list containing a ProviderMachine for the
+        `machine_id`: the ensemble machine ID
+
+        `zookeepers`: a list of `ProviderMachine`s already running zookeeper,
+            which the machine will need to connect to. May be empty.
+
+        Returns a singe-entry list containing a `ProviderMachine` for the
         new instance
         """
         raise NotImplementedError()
 
-    def get_instance_id_command(self):
-        """Snippet to discover local instance-id
-
-        @return: a snippet of shell script that evaluates to the
-            local machine's instance_id.
-        """
-        raise NotImplementedError()
-
-    @inlineCallbacks
-    def get_machine_variables(self, vars_in):
-        """Get the variables used for launching a machine and its cloud-init"""
-        vars_out = copy.deepcopy(vars_in)
-        vars_out[
-            "ensemble-zookeeper-hosts"] = yield self._get_zookeeper_hosts()
-
-        authorized_keys = get_user_authorized_keys(self._provider.config)
-        vars_out["authorized_keys"] = authorized_keys
-        vars_out["packages"] = self._get_packages()
-        vars_out["repositories"] = list(DEFAULT_REPOSITORIES)
-        vars_out["scripts"] = self._get_launch_scripts(vars_out)
-        returnValue(vars_out)
-
-    @inlineCallbacks
-    def _get_zookeeper_hosts(self):
-        if self._master:
-            returnValue("localhost:2181")
-        machines = yield self._provider.get_zookeeper_machines()
-        hosts = [m.private_dns_name for m in machines]
-        # format multiple hosts in string suitable for client
-        returnValue(",".join(["%s:2181" % h for h in hosts]))
-
-    def _get_packages(self):
-        result = list(DEFAULT_PACKAGES)
-        if self._master:
-            result.extend(BOOTSTRAP_PACKAGES)
-        return result
-
-    def _get_launch_scripts(self, machine_data):
-        """Get the scripts that will execute after the machine has launched."""
-        launch_scripts = self._get_install_scripts()
-
-        # These should be in the deb
-        launch_scripts.extend([
-            "sudo mkdir -p /var/lib/ensemble",
-            "sudo mkdir -p /var/log/ensemble"])
-
-        if self._master:
-            launch_scripts.append(self._get_initialize_script())
-
-        # Every machine has its own agent.
-        machine_agent_script_template = (
-            "ENSEMBLE_MACHINE_ID=%(machine-id)s "
-            "ENSEMBLE_ZOOKEEPER=%(ensemble-zookeeper-hosts)s "
-            "python -m ensemble.agents.machine -n "
-            "--logfile=/var/log/ensemble/machine-agent.log "
-            "--pidfile=/var/run/ensemble/machine-agent.pid")
-        launch_scripts.append(machine_agent_script_template % machine_data)
-
-        if self._master:
-            launch_scripts.append(_get_provision_agent_script(machine_data))
-
-        return launch_scripts
-
-    def _get_install_scripts(self):
-        branch = self._provider.config.get("ensemble-branch")
-        if branch is None:
-            return ["sudo apt-get -y install ensemble"]
-        # If we're using a custom branch, pass it via cloud-init to the agents,
-        # so the entire cluster uses it.
-        return [
-            "sudo apt-get install -y python-txzookeeper",
-            "sudo mkdir -p /usr/lib/ensemble",
-            "cd /usr/lib/ensemble && sudo /usr/bin/bzr co %s ensemble" % \
-                branch,
-            "cd /usr/lib/ensemble/ensemble && sudo python setup.py develop"]
-
-    def _get_initialize_script(self):
-        admin_secret = self._provider.config["admin-secret"]
-        return _get_initialize_script(
-            self.get_instance_id_command(), admin_secret)
-
-    def _on_master_launched(self, machines):
-        # TODO this should be part of Bootstrap (and should really extend,
-        # rather than effectively replace, the result of
-        # self._provider.get_zookeeper_machines)
+    def _create_cloud_init(self, machine_id, zookeepers):
+        config = self._provider.config
+        cloud_init = CloudInit()
+        cloud_init.add_ssh_key(get_user_authorized_keys(config))
+        cloud_init.set_machine_id(machine_id)
+        cloud_init.set_zookeeper_machines(zookeepers)
+        if config.get("ensemble-branch"):
+            cloud_init.set_ensemble_source(config["ensemble-branch"])
+        if self._master:
+            cloud_init.enable_bootstrap()
+            cloud_init.set_zookeeper_secret(config["admin-secret"])
+        return cloud_init
+
+    def _on_new_zookeepers(self, machines):
         instance_ids = [m.instance_id for m in machines]
-        d = self._provider.save_state({"zookeeper-instances": instance_ids})
-        d.addCallback(lambda _: machines)
-        return d
+        return self._provider.save_state({"zookeeper-instances": instance_ids})

=== modified file 'ensemble/providers/common/tests/data/cloud_init_bootstrap'
--- ensemble/providers/common/tests/data/cloud_init_bootstrap	2011-09-13 10:56:30 +0000
+++ ensemble/providers/common/tests/data/cloud_init_bootstrap	2011-09-13 10:56:30 +0000
@@ -14,5 +14,4 @@
     -n --logfile=/var/log/ensemble/machine-agent.log --pidfile=/var/run/ensemble/machine-agent.pid',
   'ENSEMBLE_ZOOKEEPER=localhost:2181 python -m ensemble.agents.provision -n --logfile=/var/log/ensemble/provision-agent.log
     --pidfile=/var/run/ensemble/provision-agent.pid']
-ssh_authorized_keys:
-- [chubb]
+ssh_authorized_keys: [chubb]

=== modified file 'ensemble/providers/common/tests/data/cloud_init_bootstrap_zookeepers'
--- ensemble/providers/common/tests/data/cloud_init_bootstrap_zookeepers	2011-09-13 10:56:30 +0000
+++ ensemble/providers/common/tests/data/cloud_init_bootstrap_zookeepers	2011-09-13 10:56:30 +0000
@@ -15,5 +15,4 @@
     --pidfile=/var/run/ensemble/machine-agent.pid', 'ENSEMBLE_ZOOKEEPER=cotswold:2181,longleat:2181,localhost:2181
     python -m ensemble.agents.provision -n --logfile=/var/log/ensemble/provision-agent.log
     --pidfile=/var/run/ensemble/provision-agent.pid']
-ssh_authorized_keys:
-- [chubb]
+ssh_authorized_keys: [chubb]

=== modified file 'ensemble/providers/common/tests/data/cloud_init_branch'
--- ensemble/providers/common/tests/data/cloud_init_branch	2011-09-13 10:56:30 +0000
+++ ensemble/providers/common/tests/data/cloud_init_branch	2011-09-13 10:56:30 +0000
@@ -14,5 +14,4 @@
   sudo mkdir -p /var/log/ensemble, 'ENSEMBLE_MACHINE_ID=passport ENSEMBLE_ZOOKEEPER=cotswold:2181,longleat:2181
     python -m ensemble.agents.machine -n --logfile=/var/log/ensemble/machine-agent.log
     --pidfile=/var/run/ensemble/machine-agent.pid']
-ssh_authorized_keys:
-- [chubb]
+ssh_authorized_keys: [chubb]

=== modified file 'ensemble/providers/common/tests/data/cloud_init_distro'
--- ensemble/providers/common/tests/data/cloud_init_distro	2011-09-13 10:56:30 +0000
+++ ensemble/providers/common/tests/data/cloud_init_distro	2011-09-13 10:56:30 +0000
@@ -10,5 +10,4 @@
     -p /var/log/ensemble, 'ENSEMBLE_MACHINE_ID=passport ENSEMBLE_ZOOKEEPER=cotswold:2181,longleat:2181
     python -m ensemble.agents.machine -n --logfile=/var/log/ensemble/machine-agent.log
     --pidfile=/var/run/ensemble/machine-agent.pid']
-ssh_authorized_keys:
-- [chubb]
+ssh_authorized_keys: [chubb]

=== modified file 'ensemble/providers/common/tests/data/cloud_init_normal'
--- ensemble/providers/common/tests/data/cloud_init_normal	2011-09-13 10:56:30 +0000
+++ ensemble/providers/common/tests/data/cloud_init_normal	2011-09-13 10:56:30 +0000
@@ -12,5 +12,4 @@
     -p /var/log/ensemble, 'ENSEMBLE_MACHINE_ID=passport ENSEMBLE_ZOOKEEPER=cotswold:2181,longleat:2181
     python -m ensemble.agents.machine -n --logfile=/var/log/ensemble/machine-agent.log
     --pidfile=/var/run/ensemble/machine-agent.pid']
-ssh_authorized_keys:
-- [chubb]
+ssh_authorized_keys: [chubb]

=== added file 'ensemble/providers/common/tests/test_bootstrap.py'
--- ensemble/providers/common/tests/test_bootstrap.py	1970-01-01 00:00:00 +0000
+++ ensemble/providers/common/tests/test_bootstrap.py	2011-09-13 10:56:30 +0000
@@ -0,0 +1,102 @@
+import logging
+import tempfile
+
+from twisted.internet.defer import fail, succeed
+
+from ensemble.errors import EnvironmentNotFound, ProviderError
+from ensemble.lib.testing import TestCase
+from ensemble.providers.common.base import MachineProviderBase
+from ensemble.providers.dummy import DummyMachine, FileStorage
+
+
+class SomeError(Exception):
+    pass
+
+
+class WorkingFileStorage(FileStorage):
+
+    def __init__(self):
+        super(WorkingFileStorage, self).__init__(tempfile.mkdtemp())
+
+
+class UnwritableFileStorage(object):
+
+    def put(self, name, f):
+        return fail(Exception("oh noes"))
+
+
+class DummyProvider(MachineProviderBase):
+
+    def __init__(self, file_storage, zookeeper):
+        self._file_storage = file_storage
+        self._zookeeper = zookeeper
+
+    def get_file_storage(self):
+        return self._file_storage
+
+    def get_zookeeper_machines(self):
+        if isinstance(self._zookeeper, Exception):
+            return fail(self._zookeeper)
+        if self._zookeeper:
+            return succeed([self._zookeeper])
+        return fail(EnvironmentNotFound())
+
+    def start_machine(self, machine_id, master=False):
+        assert master is True
+        return [DummyMachine("i-keepzoos")]
+
+
+class BootstrapTest(TestCase):
+
+    def test_unknown_error(self):
+        provider = DummyProvider(None, SomeError())
+        d = provider.bootstrap()
+        self.assertFailure(d, SomeError)
+        return d
+
+    def test_zookeeper_exists(self):
+        log = self.capture_logging("ensemble.common", level=logging.DEBUG)
+        provider = DummyProvider(
+            WorkingFileStorage(), DummyMachine("i-alreadykeepzoos"))
+        d = provider.bootstrap()
+
+        def verify(machines):
+            (machine,) = machines
+            self.assertTrue(isinstance(machine, DummyMachine))
+            self.assertEquals(machine.instance_id, "i-alreadykeepzoos")
+
+            log_text = log.getvalue()
+            self.assertIn(
+                "Ensemble environment previously bootstrapped", log_text)
+            self.assertNotIn("Launching", log_text)
+        d.addCallback(verify)
+        return d
+
+    def test_bad_storage(self):
+        provider = DummyProvider(UnwritableFileStorage(), None)
+        d = provider.bootstrap()
+        self.assertFailure(d, ProviderError)
+
+        def verify(error):
+            self.assertEquals(
+                str(error),
+                "Bootstrap aborted because file storage is not writable: "
+                "oh noes")
+        d.addCallback(verify)
+        return d
+
+    def test_create_zookeeper(self):
+        log = self.capture_logging("ensemble.common", level=logging.DEBUG)
+        provider = DummyProvider(WorkingFileStorage(), None)
+        d = provider.bootstrap()
+
+        def verify(machines):
+            (machine,) = machines
+            self.assertTrue(isinstance(machine, DummyMachine))
+            self.assertEquals(machine.instance_id, "i-keepzoos")
+
+            log_text = log.getvalue()
+            self.assertIn("Launching Ensemble bootstrap instance", log_text)
+            self.assertNotIn("previously bootstrapped", log_text)
+        d.addCallback(verify)
+        return d

=== modified file 'ensemble/providers/common/tests/test_cloudinit.py'
--- ensemble/providers/common/tests/test_cloudinit.py	2011-09-13 10:56:30 +0000
+++ ensemble/providers/common/tests/test_cloudinit.py	2011-09-13 10:56:30 +0000
@@ -26,7 +26,7 @@
         cloud_init.add_ssh_key("chubb")
         cloud_init.set_machine_id("passport")
         cloud_init.set_provider_type("dummy")
-        cloud_init.set_instance_id("token")
+        cloud_init.set_instance_id_accessor("token")
         cloud_init.set_zookeeper_secret("seekrit")
         if with_zookeepers:
             cloud_init.set_zookeeper_machines([
@@ -54,7 +54,7 @@
         self.assertEquals(
             str(error),
             "Incomplete cloud-init: you need to call add_ssh_key, "
-            "set_machine_id, set_provider_type, set_instance_id, "
+            "set_machine_id, set_provider_type, set_instance_id_accessor, "
             "set_zookeeper_secret")
 
     def test_source_validate(self):

=== modified file 'ensemble/providers/common/tests/test_launch.py'
--- ensemble/providers/common/tests/test_launch.py	2011-08-17 12:04:56 +0000
+++ ensemble/providers/common/tests/test_launch.py	2011-09-13 10:56:30 +0000
@@ -1,243 +1,80 @@
-import logging
 import tempfile
 
-from twisted.internet.defer import fail, inlineCallbacks, succeed
+from twisted.internet.defer import fail, succeed
 
-from ensemble.errors import EnvironmentNotFound, ProviderError
+from ensemble.errors import EnvironmentNotFound
 from ensemble.lib.testing import TestCase
 from ensemble.providers.common.base import MachineProviderBase
-from ensemble.providers.common.launch import (
-    BOOTSTRAP_PACKAGES, DEFAULT_PACKAGES, DEFAULT_REPOSITORIES, LaunchMachine)
+from ensemble.providers.common.launch import LaunchMachine
 from ensemble.providers.dummy import DummyMachine, FileStorage
 
-DEFAULT_INSTALL = ["sudo apt-get -y install ensemble"]
-CREATE_DIRS = ["sudo mkdir -p /var/lib/ensemble",
-               "sudo mkdir -p /var/log/ensemble"]
-
-
-class DummyLaunchMachine(LaunchMachine):
-
-    def start_machine(self, variables, data):
-        if self._master:
-            name = "bootstrapped-instance-id"
-        else:
-            name = "some-instance-id"
-        return [DummyMachine(name)]
-
-    def get_instance_id_command(self):
-        return "$(magic)"
-
-
-class WorkingFileStorage(FileStorage):
-
-    def __init__(self):
-        super(WorkingFileStorage, self).__init__(tempfile.mkdtemp())
-
-
-class UnwritableFileStorage(object):
-
-    def put(self, name, f):
-        return fail(Exception("oh noes"))
-
-
-def get_provider(launch_class=DummyLaunchMachine, config=None, zookeeper=True,
-                 file_storage_class=WorkingFileStorage):
+
+def launch_machine(test, master, zookeeper):
 
     class DummyProvider(MachineProviderBase):
 
-        def __init__(self, config):
-            super(DummyProvider, self).__init__("venus", config)
-            self._file_storage = file_storage_class()
-
-        def get_zookeeper_machines(self):
-            # this is mocked out to avoid insane complexity
-            if isinstance(zookeeper, Exception):
-                return fail(zookeeper)
-            return succeed(
-               [DummyMachine("zookeeper-instance-id",
-                             private_dns_name="zookeeper.internal")])
+        def __init__(self):
+            self.config = {"admin-secret": "BLAH"}
+            self._file_storage = FileStorage(tempfile.mkdtemp())
 
         def get_file_storage(self):
             return self._file_storage
 
-        def start_machine(self, machine_data, master=False):
-            return launch_class(self, master).run(machine_data)
-
-    create_config = {"authorized-keys": "abc"}
-    if config is not None:
-        create_config.update(config)
-    return DummyProvider(create_config)
-
-
-def get_launch(launch_class=DummyLaunchMachine, config=None,
-               zookeeper=True, master=False):
-    provider = get_provider(launch_class, config, zookeeper)
-    return launch_class(provider, master=master)
+        def get_zookeeper_machines(self):
+            if zookeeper:
+                return succeed([zookeeper])
+            return fail(EnvironmentNotFound())
+
+    class DummyLaunchMachine(LaunchMachine):
+
+        def start_machine(self, machine_id, zookeepers):
+            test.assertEquals(machine_id, "1234")
+            test.assertEquals(zookeepers, filter(None, [zookeeper]))
+            test.assertEquals(self._master, master)
+            test.assertEquals(self._constraints, {})
+            return succeed([DummyMachine("i-malive")])
+
+    provider = DummyProvider()
+    d = DummyLaunchMachine(provider, master).run("1234")
+    return provider, d
 
 
 class LaunchMachineTest(TestCase):
 
-    def _verify_machines(self, machines):
-        (machine,) = machines
-        self.assertTrue(isinstance(machine, DummyMachine))
-        self.assertEquals(machine.instance_id, "some-instance-id")
-
-    def test_no_machine_id(self):
-        launch = get_launch()
-        d = launch.run({})
-        self.assertFailure(d, ProviderError)
-        return d
-
-    def test_basic_run(self):
-        launch = get_launch()
-        d = launch.run({"machine-id": "machine-77"})
-        d.addCallback(self._verify_machines)
-        return d
-
-    @inlineCallbacks
-    def test_bootstrap_run(self):
-        provider = get_provider(config={"admin-secret": "whatever"})
-        launch = DummyLaunchMachine(provider, master=True)
-        yield launch.run({"machine-id": "machine-32767"})
-        saved_state = yield provider.load_state()
-        self.assertEquals(
-            saved_state, {"zookeeper-instances": ["bootstrapped-instance-id"]})
-
-    def test_get_machine_variables_normal(self):
-        launch = get_launch()
-        d = launch.get_machine_variables({"machine-id": "machine-999"})
-
-        def verify_vars(vars):
-            expect_vars = {
-                "authorized_keys": "abc",
-                "ensemble-zookeeper-hosts": "zookeeper.internal:2181",
-                "machine-id": "machine-999",
-                "packages": DEFAULT_PACKAGES,
-                "repositories": DEFAULT_REPOSITORIES,
-                "scripts": DEFAULT_INSTALL + CREATE_DIRS +
-                           ["ENSEMBLE_MACHINE_ID=machine-999 "
-                            "ENSEMBLE_ZOOKEEPER=zookeeper.internal:2181 "
-                            "python -m ensemble.agents.machine -n "
-                            "--logfile=/var/log/ensemble/machine-agent.log "
-                            "--pidfile=/var/run/ensemble/machine-agent.pid"]}
-            self.assertEquals(vars, expect_vars)
-        d.addCallback(verify_vars)
-        return d
-
-    def test_get_machine_variables_with_ensemble_branch(self):
-        branch_name = "lp:~my/ensemble/branch"
-        launch = get_launch(config={"ensemble-branch": branch_name})
-        d = launch.get_machine_variables({"machine-id": "machine-303"})
-
-        def verify_vars(vars):
-            scripts = vars["scripts"]
-            install_scripts = ["sudo apt-get install -y python-txzookeeper",
-                               "sudo mkdir -p /usr/lib/ensemble",
-                               "cd /usr/lib/ensemble && "
-                               "sudo /usr/bin/bzr co "
-                               + branch_name + " ensemble",
-                               "cd /usr/lib/ensemble/ensemble && "
-                               "sudo python setup.py develop"]
-            install_count = len(install_scripts)
-            self.assertEquals(scripts[:install_count], install_scripts)
-
-            other_scripts = scripts[install_count:]
-            self.assertEquals(other_scripts,
-                              CREATE_DIRS + [
-                              "ENSEMBLE_MACHINE_ID=machine-303 "
-                              "ENSEMBLE_ZOOKEEPER=zookeeper.internal:2181 "
-                              "python -m ensemble.agents.machine -n "
-                              "--logfile=/var/log/ensemble/machine-agent.log "
-                              "--pidfile=/var/run/ensemble/machine-agent.pid"])
-        d.addCallback(verify_vars)
-        return d
-
-    def test_get_machine_variables_bootstrap(self):
-        launch = get_launch(config={"admin-secret": "SEEKRIT"}, master=True)
-        d = launch.get_machine_variables({"machine-id": "machine-757"})
-
-        def verify_vars(vars):
-            expect_vars = {
-                "authorized_keys": "abc",
-                "ensemble-zookeeper-hosts": "localhost:2181",
-                "machine-id": "machine-757",
-                "packages": DEFAULT_PACKAGES + BOOTSTRAP_PACKAGES,
-                "repositories": DEFAULT_REPOSITORIES}
-            for key, value in expect_vars.items():
-                self.assertEquals(vars[key], value)
-
-            scripts = vars["scripts"]
-            self.assertEquals(scripts[:3], DEFAULT_INSTALL + CREATE_DIRS)
-            admin_init, machine_agent, provision_agent = scripts[3:]
-
-            self.assertTrue(admin_init.startswith(
-                "ensemble-admin initialize "
-                '--instance-id=$(magic) --admin-identity="admin:'))
-            self.assertFalse("SEEKRIT" in admin_init)
-
-            expect_machine = (
-                "ENSEMBLE_MACHINE_ID=machine-757 "
-                "ENSEMBLE_ZOOKEEPER=localhost:2181 "
-                "python -m ensemble.agents.machine -n "
-                "--logfile=/var/log/ensemble/machine-agent.log "
-                "--pidfile=/var/run/ensemble/machine-agent.pid")
-            self.assertEquals(machine_agent, expect_machine)
-
-            expect_provision = (
-                "ENSEMBLE_ZOOKEEPER=localhost:2181 "
-                "python -m ensemble.agents.provision -n "
-                "--logfile=/var/log/ensemble/provision-agent.log "
-                "--pidfile=/var/run/ensemble/provision-agent.pid")
-            self.assertEquals(provision_agent, expect_provision)
-        d.addCallback(verify_vars)
-        return d
-
-
-class BootstrapTest(TestCase):
-
-    def test_bootstrap_unknown_error(self):
-        class BBQError(Exception):
-            pass
-        provider = get_provider(zookeeper=BBQError())
-        d = provider.bootstrap()
-        self.assertFailure(d, BBQError)
-        return d
-
-    def test_bootstrap_unwritable_storage(self):
-        provider = get_provider(zookeeper=EnvironmentNotFound(),
-                                file_storage_class=UnwritableFileStorage)
-        d = provider.bootstrap()
-        self.assertFailure(d, ProviderError)
-        return d
-
-    def test_bootstrap_no_launch(self):
-        log = self.capture_logging("ensemble.common", level=logging.DEBUG)
-        provider = get_provider()
-        d = provider.bootstrap()
-
-        def verify_machines(machines):
-            (machine,) = machines
-            self.assertTrue(isinstance(machine, DummyMachine))
-            self.assertEquals(machine.instance_id, "zookeeper-instance-id")
-            log_text = log.getvalue()
-            self.assertIn(
-                "Ensemble environment previously bootstrapped", log_text)
-            self.assertNotIn("Launching", log_text)
-        d.addCallback(verify_machines)
-        return d
-
-    def test_bootstrap_launch(self):
-        log = self.capture_logging("ensemble.common", level=logging.DEBUG)
-        provider = get_provider(config={"admin-secret": "SEEKRIT"},
-                                zookeeper=EnvironmentNotFound())
-        d = provider.bootstrap()
-
-        def verify_machines(machines):
-            (machine,) = machines
-            self.assertTrue(isinstance(machine, DummyMachine))
-            self.assertEquals(machine.instance_id, "bootstrapped-instance-id")
-            log_text = log.getvalue()
-            self.assertIn("Launching Ensemble bootstrap instance", log_text)
-            self.assertNotIn("previously bootstrapped", log_text)
-        d.addCallback(verify_machines)
+    def assert_success(self, master, zookeeper):
+        provider, d = launch_machine(self, master, zookeeper)
+
+        def verify(machines):
+            (machine,) = machines
+            self.assertTrue(isinstance(machine, DummyMachine))
+            self.assertEquals(machine.instance_id, "i-malive")
+            return provider
+        d.addCallback(verify)
+        return d
+
+    def test_start_nonzookeeper_no_zookeepers(self):
+        """Starting a non-zookeeper without a zookeeper is an error"""
+        unused, d = launch_machine(self, False, None)
+        self.assertFailure(d, EnvironmentNotFound)
+        return d
+
+    def test_start_zookeeper_no_zookeepers(self):
+        """A new zookeeper should be recorded in provider state"""
+        d = self.assert_success(True, None)
+
+        def verify(provider):
+            provider_state = yield provider.load_state()
+            self.assertEquals(
+                provider_state, {"zookeeper-instances": ["i-malive"]})
+        d.addCallback(verify)
+        return d
+
+    def test_works_with_zookeeper(self):
+        """Creating a non-zookeeper machine should not alter provider state"""
+        d = self.assert_success(False, DummyMachine("i-keepzoos"))
+
+        def verify(provider):
+            provider_state = yield provider.load_state()
+            self.assertEquals(provider_state, False)
+        d.addCallback(verify)
         return d

=== modified file 'ensemble/providers/common/tests/test_utils.py'
--- ensemble/providers/common/tests/test_utils.py	2011-08-18 12:19:22 +0000
+++ ensemble/providers/common/tests/test_utils.py	2011-09-13 10:56:30 +0000
@@ -1,5 +1,4 @@
 import os
-
 from yaml import load
 
 from twisted.python.failure import Failure
@@ -100,7 +99,7 @@
         scripts = ["wget http://lwn.net > /tmp/out"]
         repositories = ["ppa:ensemble/ppa"]
         output = format_cloud_init(
-            "zebra",
+            ["zebra"],
             packages=packages,
             scripts=scripts,
             repositories=repositories,

=== modified file 'ensemble/providers/common/utils.py'
--- ensemble/providers/common/utils.py	2011-08-31 21:12:40 +0000
+++ ensemble/providers/common/utils.py	2011-09-13 10:56:30 +0000
@@ -1,6 +1,5 @@
 import logging
 import os
-
 from yaml import safe_dump
 
 from twisted.python.failure import Failure
@@ -40,6 +39,7 @@
     raise error
 
 
+# XXX this should either be singular, or return a list
 def get_user_authorized_keys(config):
     """Locate a public key for the user.
 
@@ -85,7 +85,7 @@
 
     @param authorized_keys The authorized SSH keys to be used when populating
            the newly launched machine's.
-    @type string
+    @type list of strings
 
     @param packages A list of packages to be installed on a machine.
     @type list of strings
@@ -104,7 +104,7 @@
     cloud_config = {
         "apt-update": True,
         "apt-upgrade": True,
-        "ssh_authorized_keys": [authorized_keys],
+        "ssh_authorized_keys": authorized_keys,
         "packages": [],
         "output": { "all": "| tee -a /var/log/cloud-init-output.log" }}
 

=== modified file 'ensemble/providers/ec2/__init__.py'
--- ensemble/providers/ec2/__init__.py	2011-08-29 18:34:02 +0000
+++ ensemble/providers/ec2/__init__.py	2011-09-13 10:56:30 +0000
@@ -1,7 +1,8 @@
 import os
 import re
 
-from twisted.internet.defer import inlineCallbacks, returnValue
+from twisted.internet.defer import fail, inlineCallbacks, returnValue
+
 from txaws.ec2.exception import EC2Error
 from txaws.service import AWSServiceRegion
 
@@ -53,13 +54,17 @@
     def start_machine(self, machine_data, master=False):
         """Start a machine in the provider.
 
-        @param machine_data: a dictionary of data to pass along to the newly
-           launched machine.
+        `machine_data`: a dictionary describing the machine to be launched.
 
-        @param master if True, machine will initialize the ensemble admin
+        @param master: if True, machine will initialize the ensemble admin
             and run a provisioning agent.
         """
-        return EC2LaunchMachine(self, master).run(machine_data)
+        if "machine-id" not in machine_data:
+            return fail(ProviderError(
+                "Cannot launch a machine without specifying a machine-id"))
+        machine_id = machine_data["machine-id"]
+        constraints = machine_data.get("constraints", {})
+        return EC2LaunchMachine(self, master, constraints).run(machine_id)
 
     @inlineCallbacks
     def get_machines(self, instance_ids=()):

=== modified file 'ensemble/providers/ec2/launch.py'
--- ensemble/providers/ec2/launch.py	2011-08-23 09:59:59 +0000
+++ ensemble/providers/ec2/launch.py	2011-09-13 10:56:30 +0000
@@ -1,67 +1,52 @@
 from twisted.internet.defer import inlineCallbacks, returnValue
+
 from txaws.ec2.exception import EC2Error
 
 from ensemble.errors import ProviderInteractionError
 from ensemble.providers.common.launch import LaunchMachine
 
 from .machine import machine_from_instance
-from .utils import log, get_launch_options
+from .utils import get_image_id, log
 
 
 class EC2LaunchMachine(LaunchMachine):
     """Amazon EC2 operation for launching an instance"""
 
     @inlineCallbacks
-    def start_machine(self, machine_variables, machine_id):
+    def start_machine(self, machine_id, zookeepers):
         """Actually launch an instance on EC2.
 
-        `machine_variables`: non-provider-specific data, sufficient to
-            define the machine's behaviour once it exists.
+        `machine_id`: the ensemble machine ID
 
-        `machine_id`: the external machine ID (also exists in the
-            above bag of data)
+        `zookeepers`: a list of `ProviderMachine`s already running zookeeper,
+            which the machine will need to connect to. May be empty.
 
         Returns a singe-entry list containing a ProviderMachine for the
         new instance
         """
-        # Retrieves standard ec2 run_instances arguments, of note
-        # image id, and instance type.
-        machine_options = yield get_launch_options(
-            self._provider.config,
-            **machine_variables)
-
-        # Ensure the ensemble security groups exist (both the overall
-        # group and the machine-specific group) and are included in
-        # the machine instance launch options.
-        machine_options["security_groups"] = yield self._ensure_group(
-            machine_options["security_groups"],
-            machine_id)
-
-        # Launch the instance.
-        instances = yield self._provider.ec2.run_instances(**machine_options)
-        returnValue([machine_from_instance(instance)
-                     for instance in instances])
-
-    def get_instance_id_command(self):
-        """Snippet to discover local instance-id
-
-        @return: a snippet of shell script that evaluates to the
-            local machine's instance_id.
-        """
-        return "$(curl http://169.254.169.254/1.0/meta-data/instance-id)"
-
-    def get_machine_variables(self, vars_in):
-        vars_deferred = super(
-            EC2LaunchMachine, self).get_machine_variables(vars_in)
-
-        def set_provider_type(vars):
-            vars["ensemble-provider-type"] = "ec2"
-            return vars
-        vars_deferred.addCallback(set_provider_type)
-        return vars_deferred
+        cloud_init = self._create_cloud_init(machine_id, zookeepers)
+        cloud_init.set_provider_type("ec2")
+        cloud_init.set_instance_id_accessor(
+            "$(curl http://169.254.169.254/1.0/meta-data/instance-id)")
+        user_data = cloud_init.render()
+
+        instance_type = self._provider.config.get(
+            "default-instance-type", "m1.small")
+        image_id = yield get_image_id(self._provider.config, self._constraints)
+        security_groups = yield self._ensure_groups(machine_id)
+
+        instances = yield self._provider.ec2.run_instances(
+            image_id=image_id,
+            instance_type=instance_type,
+            min_count=1,
+            max_count=1,
+            security_groups=security_groups,
+            user_data=user_data)
+
+        returnValue([machine_from_instance(i) for i in instances])
 
     @inlineCallbacks
-    def _ensure_group(self, groups, machine_id):
+    def _ensure_groups(self, machine_id):
         """Ensure the ensemble group is the machine launch groups.
 
         Machines launched by ensemble are tagged with a group so they
@@ -71,21 +56,15 @@
         specific machine security group is created for each machine,
         so that its firewall rules can be configured per machine.
 
-        `groups`: The configured security groups for a machine instance.
-
         `machine_id`: The external machine ID of this machine instance
         """
-        # Label the instance with the ensemble name.
-        security_groups = yield self._provider.ec2.describe_security_groups()
         ensemble_group = "ensemble-%s" % self._provider.environment_name
         ensemble_machine_group = "ensemble-%s-%s" % (
             self._provider.environment_name, machine_id)
+
+        security_groups = yield self._provider.ec2.describe_security_groups()
         group_ids = [group.name for group in security_groups]
 
-        # Ensure the provider group is added to the instance groups.
-        if not ensemble_group in groups:
-            groups.append(ensemble_group)
-
         # Create the provider group if doesn't exist.
         if not ensemble_group in group_ids:
             log.debug("Creating ensemble provider group %s", ensemble_group)
@@ -131,6 +110,6 @@
             ensemble_machine_group,
             "Ensemble group for %s machine %s" % (
                 self._provider.environment_name, machine_id))
-        groups.append(ensemble_machine_group)
-
-        returnValue(groups)
+
+        returnValue([ensemble_group, ensemble_machine_group])
+

=== modified file 'ensemble/providers/ec2/tests/common.py'
--- ensemble/providers/ec2/tests/common.py	2011-08-29 02:23:11 +0000
+++ ensemble/providers/ec2/tests/common.py	2011-09-13 10:56:30 +0000
@@ -12,8 +12,6 @@
 from ensemble.providers.ec2 import MachineProvider
 from ensemble.providers.ec2.machine import EC2ProviderMachine
 
-
-MATCH_AMI = MATCH(lambda image_id: image_id.startswith("ami-"))
 MATCH_GROUP = MATCH(lambda x: x.startswith("ensemble-moon"))
 
 
@@ -82,7 +80,7 @@
 
 class EC2MachineLaunchMixin(object):
 
-    def _mock_launch_utils(self, **get_ami_kwargs):
+    def _mock_launch_utils(self, ami_name="ami-default", **get_ami_kwargs):
         get_public_key = self.mocker.replace(
             "ensemble.providers.common.utils.get_user_authorized_keys")
 
@@ -100,7 +98,7 @@
 
         def check_kwargs(**kwargs):
             self.assertEquals(kwargs, get_ami_kwargs)
-            return succeed("ami-714ba518")
+            return succeed(ami_name)
         self.mocker.call(check_kwargs)
 
     def _mock_create_group(self):

=== added file 'ensemble/providers/ec2/tests/data/bootstrap_cloud_init'
--- ensemble/providers/ec2/tests/data/bootstrap_cloud_init	1970-01-01 00:00:00 +0000
+++ ensemble/providers/ec2/tests/data/bootstrap_cloud_init	2011-09-13 10:56:30 +0000
@@ -0,0 +1,18 @@
+#cloud-config
+apt-update: true
+apt-upgrade: true
+apt_sources:
+- {source: 'ppa:ensemble/ppa'}
+machine-data: {ensemble-provider-type: ec2, ensemble-zookeeper-hosts: 'localhost:2181',
+  machine-id: '0'}
+output: {all: '| tee -a /var/log/cloud-init-output.log'}
+packages: [bzr, byobu, tmux, python-setuptools, python-twisted, python-argparse, python-txaws,
+  python-zookeeper, default-jre-headless, zookeeper, zookeeperd]
+runcmd: [sudo apt-get -y install ensemble, sudo mkdir -p /var/lib/ensemble, sudo mkdir
+    -p /var/log/ensemble, 'ensemble-admin initialize --instance-id=$(curl http://169.254.169.254/1.0/meta-data/instance-id)
+    --admin-identity=admin:JbJ6sDGV37EHzbG9FPvttk64cmg=', 'ENSEMBLE_MACHINE_ID=0 ENSEMBLE_ZOOKEEPER=localhost:2181
+    python -m ensemble.agents.machine -n --logfile=/var/log/ensemble/machine-agent.log
+    --pidfile=/var/run/ensemble/machine-agent.pid', 'ENSEMBLE_ZOOKEEPER=localhost:2181
+    python -m ensemble.agents.provision -n --logfile=/var/log/ensemble/provision-agent.log
+    --pidfile=/var/run/ensemble/provision-agent.pid']
+ssh_authorized_keys: [zebra]

=== added file 'ensemble/providers/ec2/tests/data/launch_cloud_init'
--- ensemble/providers/ec2/tests/data/launch_cloud_init	1970-01-01 00:00:00 +0000
+++ ensemble/providers/ec2/tests/data/launch_cloud_init	2011-09-13 10:56:30 +0000
@@ -0,0 +1,15 @@
+#cloud-config
+apt-update: true
+apt-upgrade: true
+apt_sources:
+- {source: 'ppa:ensemble/ppa'}
+machine-data: {ensemble-provider-type: ec2, ensemble-zookeeper-hosts: 'es.example.internal:2181',
+  machine-id: '1'}
+output: {all: '| tee -a /var/log/cloud-init-output.log'}
+packages: [bzr, byobu, tmux, python-setuptools, python-twisted, python-argparse, python-txaws,
+  python-zookeeper]
+runcmd: [sudo apt-get -y install ensemble, sudo mkdir -p /var/lib/ensemble, sudo mkdir
+    -p /var/log/ensemble, 'ENSEMBLE_MACHINE_ID=1 ENSEMBLE_ZOOKEEPER=es.example.internal:2181
+    python -m ensemble.agents.machine -n --logfile=/var/log/ensemble/machine-agent.log
+    --pidfile=/var/run/ensemble/machine-agent.pid']
+ssh_authorized_keys: [zebra]

=== modified file 'ensemble/providers/ec2/tests/test_bootstrap.py'
--- ensemble/providers/ec2/tests/test_bootstrap.py	2011-08-11 05:59:30 +0000
+++ ensemble/providers/ec2/tests/test_bootstrap.py	2011-09-13 10:56:30 +0000
@@ -1,6 +1,6 @@
 import logging
-
-from yaml import dump, load
+import os
+from yaml import dump
 
 from twisted.internet.defer import succeed
 
@@ -8,12 +8,11 @@
 
 from ensemble.lib.mocker import MATCH
 from ensemble.lib.testing import TestCase
-from ensemble.providers.common.launch import (
-    BOOTSTRAP_PACKAGES, DEFAULT_REPOSITORIES, DEFAULT_PACKAGES)
 from ensemble.providers.ec2.machine import EC2ProviderMachine
-from ensemble.state.auth import make_identity
-
-from .common import EC2TestMixin, EC2MachineLaunchMixin, MATCH_AMI
+
+from .common import EC2TestMixin, EC2MachineLaunchMixin
+
+DATA_DIR = os.path.join(os.path.abspath(os.path.dirname(__file__)), "data")
 
 
 class EC2BootstrapTest(EC2TestMixin, EC2MachineLaunchMixin, TestCase):
@@ -34,55 +33,21 @@
             MATCH(match_string))
         self.mocker.result(succeed(True))
 
-    def _mock_launch(self, machine_id):
+    def _mock_launch(self):
         """Mock launching a bootstrap machine on ec2."""
-        credentials = "admin:%s" % self.get_config()["admin-secret"]
-        admin_identity = make_identity(credentials)
-
         def verify_user_data(data):
-
-            lines = data.split("\n")
-            self.assertEqual(lines.pop(0), "#cloud-config")
-            config = load("\n".join(lines))
-            repos = [dict(source=r) for r in DEFAULT_REPOSITORIES]
-            self.assertEqual(config["apt_sources"], repos)
-            self.assertEqual(
-                config["packages"],
-                list(DEFAULT_PACKAGES) + BOOTSTRAP_PACKAGES)
-            self.failUnlessIn("admin-identity", config["runcmd"][-3])
-
-            script = (
-                'ensemble-admin initialize --instance-id=%s'
-                ' --admin-identity="%s"') % (
-                    "$(curl http://169.254.169.254/1.0/meta-data/instance-id)",
-                    admin_identity)
-
-            self.assertEqual(config["runcmd"][-3], script)
-
-            script = (
-                "ENSEMBLE_MACHINE_ID=0 ENSEMBLE_ZOOKEEPER=localhost:2181 "
-                "python -m ensemble.agents.machine -n "
-                "--logfile=/var/log/ensemble/machine-agent.log "
-                "--pidfile=/var/run/ensemble/machine-agent.pid")
-
-            self.assertEqual(config["runcmd"][-2], script)
-
-            provision_agent_script = (
-                "ENSEMBLE_ZOOKEEPER=localhost:2181 "
-                "python -m ensemble.agents.provision -n "
-                "--logfile=/var/log/ensemble/provision-agent.log "
-                "--pidfile=/var/run/ensemble/provision-agent.pid")
-            self.assertEqual(config["runcmd"][-1], provision_agent_script)
+            expect_path = os.path.join(DATA_DIR, "bootstrap_cloud_init")
+            with open(expect_path) as f:
+                expect_cloud_init = f.read()
+            self.assertEquals(data, expect_cloud_init)
             return True
 
         self.ec2.run_instances(
-            image_id=MATCH_AMI,
+            image_id="ami-default",
             instance_type="m1.small",
             max_count=1,
             min_count=1,
-            security_groups=[
-                "%s-%s" % ("ensemble", self.env_name),
-                "%s-%s-%s" % ("ensemble", self.env_name, machine_id)],
+            security_groups=["ensemble-moon", "ensemble-moon-0"],
             user_data=MATCH(verify_user_data))
 
     def test_launch_bootstrap(self):
@@ -98,7 +63,7 @@
         self._mock_create_group()
         self._mock_create_machine_group(0)
         self._mock_launch_utils(region="us-east-1")
-        self._mock_launch(0)
+        self._mock_launch()
         self.mocker.result(succeed([]))
         self._mock_save()
         self.mocker.replay()
@@ -128,7 +93,7 @@
             SecurityGroup("ensemble-%s" % self.env_name, "")]))
         self._mock_create_machine_group(0)
         self._mock_launch_utils(region="us-east-1")
-        self._mock_launch(0)
+        self._mock_launch()
         self.mocker.result(succeed([]))
         self._mock_save()
         self.mocker.replay()
@@ -177,7 +142,7 @@
             SecurityGroup("ensemble-%s" % self.env_name, "")]))
         self._mock_create_machine_group(0)
         self._mock_launch_utils(region="us-east-1")
-        self._mock_launch(0)
+        self._mock_launch()
         self.mocker.result(succeed([self.get_instance("i-foobar")]))
         self._mock_save()
         self.mocker.replay()

=== modified file 'ensemble/providers/ec2/tests/test_launch.py'
--- ensemble/providers/ec2/tests/test_launch.py	2011-08-26 12:24:00 +0000
+++ ensemble/providers/ec2/tests/test_launch.py	2011-09-13 10:56:30 +0000
@@ -1,51 +1,54 @@
-from yaml import load
+import os
 
 from twisted.internet.defer import inlineCallbacks, succeed
 
 from txaws.ec2.model import Instance, SecurityGroup
 
-from ensemble.errors import EnvironmentNotFound, ProviderInteractionError
-from ensemble.providers.common.launch import (
-    DEFAULT_REPOSITORIES, DEFAULT_PACKAGES)
+from ensemble.errors import (
+    EnvironmentNotFound, ProviderError, ProviderInteractionError)
 from ensemble.providers.ec2.machine import EC2ProviderMachine
 
 from ensemble.lib.testing import TestCase
 from ensemble.lib.mocker import MATCH
 
-from .common import EC2TestMixin, EC2MachineLaunchMixin, MATCH_AMI
+from .common import EC2TestMixin, EC2MachineLaunchMixin
+
+DATA_DIR = os.path.join(os.path.abspath(os.path.dirname(__file__)), "data")
 
 
 class EC2MachineLaunchTest(EC2TestMixin, EC2MachineLaunchMixin, TestCase):
 
-    def _mock_launch(self, instance=None, custom_verify=None, machine_id=None):
+    def _mock_launch(self, instance, expect_ami="ami-default",
+                     expect_instance_type="m1.small"):
 
         def verify_user_data(data):
-            lines = data.split("\n")
-            self.assertEqual(lines.pop(0), "#cloud-config")
-            config = load("\n".join(lines))
-            repos = [dict(source=r) for r in DEFAULT_REPOSITORIES]
-            self.assertEqual(config["apt_sources"], repos)
-            self.assertEqual(config["packages"], DEFAULT_PACKAGES)
-            self.assertEqual(config["machine-data"]["machine-id"], "machine-1")
-
-            if custom_verify:
-                custom_verify(config)
+            expect_path = os.path.join(DATA_DIR, "launch_cloud_init")
+            with open(expect_path) as f:
+                expect_cloud_init = f.read()
+            self.assertEquals(data, expect_cloud_init)
             return True
 
         self.ec2.run_instances(
-            image_id=MATCH_AMI,
-            instance_type="m1.small",
+            image_id=expect_ami,
+            instance_type=expect_instance_type,
             max_count=1,
             min_count=1,
-            security_groups=[
-                "%s-%s" % ("ensemble", self.env_name),
-                "%s-%s-%s" % ("ensemble", self.env_name, machine_id)],
+            security_groups=["ensemble-moon", "ensemble-moon-1"],
             user_data=MATCH(verify_user_data))
 
-        if instance:
-            self.mocker.result(succeed([instance]))
-        else:
-            self.mocker.result(succeed([]))
+        self.mocker.result(succeed([instance]))
+
+    def test_bad_data(self):
+        self.mocker.replay()
+        d = self.get_provider().start_machine({})
+        self.assertFailure(d, ProviderError)
+
+        def verify(error):
+            self.assertEquals(
+                str(error),
+                "Cannot launch a machine without specifying a machine-id")
+        d.addCallback(verify)
+        return d
 
     def test_provider_launch(self):
         """
@@ -55,18 +58,17 @@
         self.ec2.describe_security_groups()
         self.mocker.result(succeed([]))
         self._mock_create_group()
-        self._mock_create_machine_group("machine-1")
+        self._mock_create_machine_group("1")
         self._mock_launch_utils(region="us-east-1")
         self._mock_get_zookeeper_hosts()
-        self._mock_launch(
-            self.get_instance("i-foobar"), machine_id="machine-1")
+        self._mock_launch(self.get_instance("i-foobar"))
         self.mocker.replay()
 
         def verify_result(result):
             (machine,) = result
             self.assert_machine(machine, "i-foobar", "")
         provider = self.get_provider()
-        d = provider.start_machine({"machine-id": "machine-1"})
+        d = provider.start_machine({"machine-id": "1"})
         d.addCallback(verify_result)
         return d
 
@@ -78,15 +80,14 @@
 
         self.ec2.describe_security_groups()
         self.mocker.result(succeed([security_group]))
-        self._mock_create_machine_group("machine-1")
+        self._mock_create_machine_group("1")
         self._mock_launch_utils(region="us-east-1")
         self._mock_get_zookeeper_hosts()
-        self._mock_launch(instance, machine_id="machine-1")
+        self._mock_launch(instance)
         self.mocker.replay()
 
         provider = self.get_provider()
-        provided_machines = yield provider.start_machine(
-            {"machine-id": "machine-1"})
+        provided_machines = yield provider.start_machine({"machine-id": "1"})
         self.assertEqual(len(provided_machines), 1)
         self.assertTrue(isinstance(provided_machines[0], EC2ProviderMachine))
         self.assertEqual(
@@ -97,21 +98,20 @@
         """Verify that the launch works if the machine security group exists"""
         instance = Instance("i-foobar", "running", dns_name="x1.example.com")
         machine_group = SecurityGroup(
-            "ensemble-moon-machine-1", "some description")
+            "ensemble-moon-1", "some description")
 
         self.ec2.describe_security_groups()
         self.mocker.result(succeed([machine_group]))
         self._mock_create_group()
-        self._mock_delete_machine_group("machine-1")  # delete existing sg
-        self._mock_create_machine_group("machine-1")  # then recreate
+        self._mock_delete_machine_group("1")  # delete existing sg
+        self._mock_create_machine_group("1")  # then recreate
         self._mock_launch_utils(region="us-east-1")
         self._mock_get_zookeeper_hosts()
-        self._mock_launch(instance, machine_id="machine-1")
+        self._mock_launch(instance)
         self.mocker.replay()
 
         provider = self.get_provider()
-        provided_machines = yield provider.start_machine(
-            {"machine-id": "machine-1"})
+        provided_machines = yield provider.start_machine({"machine-id": "1"})
         self.assertEqual(len(provided_machines), 1)
         self.assertTrue(isinstance(provided_machines[0], EC2ProviderMachine))
         self.assertEqual(
@@ -125,49 +125,27 @@
         that security group, generally because it is still shutting
         down."""
         machine_group = SecurityGroup(
-            "ensemble-moon-machine-1", "some description")
+            "ensemble-moon-1", "some description")
 
         self.ec2.describe_security_groups()
         self.mocker.result(succeed([machine_group]))
         self._mock_create_group()
-        self._mock_delete_machine_group_was_deleted("machine-1")  # sg is gone!
+        self._mock_delete_machine_group_was_deleted("1")  # sg is gone!
         self._mock_launch_utils(region="us-east-1")
         self._mock_get_zookeeper_hosts()
         self.mocker.replay()
 
         provider = self.get_provider()
         ex = yield self.assertFailure(
-            provider.start_machine({"machine-id": "machine-1"}),
+            provider.start_machine({"machine-id": "1"}),
             ProviderInteractionError)
         self.assertEqual(
             str(ex),
             "Unexpected EC2Error deleting security group "
-            "ensemble-moon-machine-1: There are active instances using "
-            "security group 'ensemble-moon-machine-1'")
-
-    def test_provider_type_machine_variable(self):
-        """The provider type is available via cloud-init."""
-        self.ec2.describe_security_groups()
-        self.mocker.result(succeed([]))
-        self._mock_create_group()
-        self._mock_create_machine_group("machine-1")
-        self._mock_launch_utils(region="us-east-1")
-        self._mock_get_zookeeper_hosts()
-
-        def verify_provider_type(data):
-            self.assertEqual(
-                data["machine-data"]["ensemble-provider-type"], "ec2")
-
-        self._mock_launch(custom_verify=verify_provider_type,
-                          machine_id="machine-1")
-        self.mocker.replay()
-
-        provider = self.get_provider()
-        d = provider.start_machine({"machine-id": "machine-1"})
-        return d
+            "ensemble-moon-1: There are active instances using security group "
+            "'ensemble-moon-1'")
 
     def test_launch_with_no_ensemble_s3_state(self):
-        # XXX not really testing anything EC2-specific
         """
         Attempting to launch without any ensemble saved state, means
         we can't provide a way for a launched instance to connect
@@ -178,12 +156,11 @@
         self.mocker.replay()
 
         provider = self.get_provider()
-        d = provider.start_machine({"machine-id": "machine-1"})
+        d = provider.start_machine({"machine-id": "1"})
         self.assertFailure(d, EnvironmentNotFound)
         return d
 
     def test_launch_with_no_ensemble_zookeeper_hosts(self):
-        # XXX not really testing anything EC2-specific
         """
         Attempting to launch without any ensemble zookeeper hosts, means
         we can't provide a way for a launched instance to connect
@@ -194,55 +171,42 @@
         self.mocker.replay()
 
         provider = self.get_provider()
-        d = provider.start_machine({"machine-id": "machine-1"})
+        d = provider.start_machine({"machine-id": "1"})
         self.assertFailure(d, EnvironmentNotFound)
         return d
 
-    def test_launch_starts_machine_agent(self):
-        # XXX not really testing anything EC2-specific
-        """A launch'd machine should have a machine agent."""
+    def test_launch_options_known_instance_type(self):
         self.ec2.describe_security_groups()
         self.mocker.result(succeed([]))
         self._mock_create_group()
-        self._mock_create_machine_group("machine-1")
+        self._mock_create_machine_group("1")
         self._mock_launch_utils(region="us-east-1")
         self._mock_get_zookeeper_hosts()
-
-        def verify_machine_agent(data):
-            script = ("ENSEMBLE_MACHINE_ID=machine-1 "
-                      "ENSEMBLE_ZOOKEEPER=es.example.internal:2181 "
-                      "python -m ensemble.agents.machine "
-                      "-n --logfile=/var/log/ensemble/machine-agent.log "
-                      "--pidfile=/var/run/ensemble/machine-agent.pid")
-            self.assertEqual(script, data["runcmd"][-1])
-
-        self._mock_launch(custom_verify=verify_machine_agent,
-                          machine_id="machine-1")
-        self.mocker.replay()
-
-        provider = self.get_provider()
-        d = provider.start_machine({"machine-id": "machine-1"})
-        return d
-
-    def _mock_launch_with_ami_params(self, get_ami_kwargs):
-        self.ec2.describe_security_groups()
-        self.mocker.result(succeed([]))
-        self._mock_create_group()
-        self._mock_create_machine_group("machine-1")
-        self._mock_launch_utils(**get_ami_kwargs)
-        self._mock_get_zookeeper_hosts()
         self._mock_launch(
-            self.get_instance("i-foobar"), machine_id="machine-1")
+            self.get_instance("i-foobar"), expect_instance_type="m1.ginormous")
+        self.mocker.replay()
+
+        provider = self.get_provider()
+        provider.config["default-instance-type"] = "m1.ginormous"
+        return provider.start_machine({"machine-id": "1"})
+
+    def _mock_launch_with_ami_params(self, get_ami_kwargs, expect_ami=None):
+        expect_ami = expect_ami or "ami-default"
+        self.ec2.describe_security_groups()
+        self.mocker.result(succeed([]))
+        self._mock_create_group()
+        self._mock_create_machine_group("1")
+        self._mock_launch_utils(ami_name=expect_ami, **get_ami_kwargs)
+        self._mock_get_zookeeper_hosts()
+        self._mock_launch(self.get_instance("i-foobar"), expect_ami)
 
     def test_launch_options_known_ami(self):
-        self._mock_launch_with_ami_params({})
+        self._mock_launch_with_ami_params({}, expect_ami="ami-different")
         self.mocker.replay()
 
         provider = self.get_provider()
-        # TODO shouldn't something be testing we actually launch
-        # an instance of the expected AMI?
-        provider.config["default-image-id"] = "ami-XXXXXX"
-        return provider.start_machine({"machine-id": "machine-1"})
+        provider.config["default-image-id"] = "ami-different"
+        return provider.start_machine({"machine-id": "1"})
 
     def test_launch_options_region(self):
         self._mock_launch_with_ami_params({"region": "somewhere-else-1"})
@@ -250,21 +214,22 @@
 
         provider = self.get_provider()
         provider.config["region"] = "somewhere-else-1"
-        return provider.start_machine({"machine-id": "machine-1"})
+        return provider.start_machine({"machine-id": "1"})
 
-    def test_launch_options_other(self):
+    def test_launch_options_custom_image_options(self):
         self._mock_launch_with_ami_params({
             "region": "us-east-1",
-            "ubuntu_release_name": "choleric",
-            "arch": "quantum86",
-            "ebs": "maybe",
+            "ubuntu_release": "choleric",
+            "architecture": "quantum86",
+            "persistent_storage": "maybe",
             "daily": "perhaps"})
         self.mocker.replay()
 
         provider = self.get_provider()
-        return provider.start_machine({
-            "machine-id": "machine-1",
-            "image_release_name": "choleric",
-            "image_arch": "quantum86",
-            "image_ebs": "maybe",
-            "image_daily": "perhaps"})
+        constraints = {
+            "ubuntu_release": "choleric",
+            "architecture": "quantum86",
+            "persistent_storage": "maybe",
+            "daily": "perhaps"}
+        return provider.start_machine({"machine-id": "1",
+                                       "constraints": constraints})

=== modified file 'ensemble/providers/ec2/tests/test_utils.py'
--- ensemble/providers/ec2/tests/test_utils.py	2011-08-12 19:54:54 +0000
+++ ensemble/providers/ec2/tests/test_utils.py	2011-09-13 10:56:30 +0000
@@ -1,14 +1,12 @@
 import inspect
 import os
 
-
 from twisted.internet.defer import succeed
 
+from ensemble.lib.testing import TestCase
 from ensemble.providers import ec2
-from ensemble.providers.common.utils import format_cloud_init
-from ensemble.providers.ec2.utils import get_current_ami, get_launch_options
+from ensemble.providers.ec2.utils import get_current_ami, get_image_id
 
-from ensemble.lib.testing import TestCase
 
 IMAGE_URI_TEMPLATE = "\
 http://uec-images.ubuntu.com/query/%s/server/released.current.txt"
@@ -17,7 +15,7 @@
     os.path.dirname(inspect.getabsfile(ec2)), "tests", "data")
 
 
-class EC2UtilsTest(TestCase):
+class GetCurrentAmiTest(TestCase):
 
     def test_umatched_ami(self):
         """
@@ -28,7 +26,7 @@
         page(IMAGE_URI_TEMPLATE % "lucid")
         self.mocker.result(succeed(""))
         self.mocker.replay()
-        d = get_current_ami(ubuntu_release_name="lucid")
+        d = get_current_ami(ubuntu_release="lucid")
         self.failUnlessFailure(d, LookupError)
         return d
 
@@ -40,7 +38,7 @@
             open(os.path.join(IMAGE_DATA_DIR, "lucid.txt")).read()))
 
         self.mocker.replay()
-        d = get_current_ami(ubuntu_release_name="lucid")
+        d = get_current_ami(ubuntu_release="lucid")
 
         def verify_result(result):
             self.assertEqual(result, "ami-714ba518")
@@ -56,7 +54,7 @@
             succeed(open(
                 os.path.join(IMAGE_DATA_DIR, "lucid.txt")).read()))
         self.mocker.replay()
-        d = get_current_ami(ubuntu_release_name="lucid", region="us-west-1")
+        d = get_current_ami(ubuntu_release="lucid", region="us-west-1")
 
         def verify_result(result):
             self.assertEqual(result, "ami-cb97c68e")
@@ -74,7 +72,7 @@
         self.mocker.result(succeed(
             open(os.path.join(IMAGE_DATA_DIR, "lucid.txt")).read()))
         self.mocker.replay()
-        d = get_current_ami(ubuntu_release_name="lucid", ebs=False)
+        d = get_current_ami(ubuntu_release="lucid", persistent_storage=False)
 
         def verify_result(result):
             self.assertEqual(result, "ami-2d4aa444")
@@ -82,101 +80,37 @@
         d.addCallback(verify_result)
         return d
 
-    def test_get_launch_options(self):
-        """
-        F{get_launch_options}, returns a dictionary of arguments for the ec2
-        client's run instance method.
-        """
-        config = {}
-        config["default-image-id"] = "ami-foobar"
-        config["default-instance-type"] = "m1.medium"
-        authorized_keys = "zebra"
-        repositories = ["ppa:ensemble/ppa"]
-        packages = ["python-zookeeper", "zookeeperd"]
-
-        d = get_launch_options(
-            config,
-            authorized_keys=authorized_keys,
-            packages=packages,
-            repositories=repositories,
-            magic=[1, 2, 3])
-
-        cloud_init_config = format_cloud_init(
-            authorized_keys, packages, repositories,
-            data={"magic": [1, 2, 3]})
-
-        def verify_result(result):
-            self.assertTrue(isinstance(result, dict))
-            self.assertEqual(result["image_id"], "ami-foobar")
-            self.assertEqual(result["user_data"], cloud_init_config)
-            self.assertEqual(result["security_groups"], [])
-
-        d.addCallback(verify_result)
-        return d
-
-    def test_get_launch_options_defaults(self):
-        """
-        F{get_launch_options}, returns a dictionary of arguments for
-        the ec2 client's run instance method. A known set of default
-        is utilized.
-        """
-
-        page = self.mocker.replace("twisted.web.client.getPage")
-        page(IMAGE_URI_TEMPLATE % "natty")
-        self.mocker.result(
-            succeed(open(
-                os.path.join(IMAGE_DATA_DIR, "natty.txt")).read()))
-        self.mocker.replay()
-
-        config = {}
-        d = get_launch_options(config, "zebra")
-
-        def verify_result(result):
-            self.assertTrue(isinstance(result, dict))
-            self.assertEqual(result["image_id"], "ami-06ad526f")
-            self.assertEqual(result["instance_type"], "m1.small")
-
-        d.addCallback(verify_result)
-        return d
-
-    def test_get_launch_options_with_params(self):
-        """
-        Image parameters can be specified to determine which ami is to
-        be launched. Image paramters specified are not passed to the
-        instance via user data.
-        """
-        page = self.mocker.replace("twisted.web.client.getPage")
-        page(IMAGE_URI_TEMPLATE % "natty")
-        self.mocker.result(
-            succeed(open(
-                os.path.join(IMAGE_DATA_DIR, "natty.txt")).read()))
-        self.mocker.replay()
-
-        d = get_launch_options(
-            {"default-instance-type": "m1.large"},
-            "zebra", image_arch="amd64", image_ebs=False)
-
-        def verify_result(result):
-            self.assertTrue(isinstance(result, dict))
-            self.assertEqual(result["image_id"], "ami-68ad5201")
-            self.assertEqual(result["instance_type"], "m1.large")
-            self.assertNotIn(
-                "arch", result["user_data"])
-            self.assertNotIn(
-                "ebs", result["user_data"])
-
-        d.addCallback(verify_result)
-        return d
-
-    def test_configured_default_image_has_precedence_over_region(self):
-        """If the server machine image is specified it takes precendence over
-        region."""
-        d = get_launch_options(
-            {"region": "eu-west-1", "default-image-id": "21down"},
-            authorized_keys="")
-
-        def verify_result(result):
-            self.assertEqual(result["image_id"], "21down")
-
-        d.addCallback(verify_result)
+
+class GetImageIdTest(TestCase):
+
+    def test_default_image_id(self):
+        d = get_image_id({"default-image-id": "ami-burble"}, {})
+        d.addCallback(self.assertEquals, "ami-burble")
+        return d
+
+    def test_no_constraints(self):
+        get_current_ami_m = self.mocker.replace(get_current_ami)
+        get_current_ami_m(region="us-east-1")
+        self.mocker.result(succeed("ami-giggle"))
+        self.mocker.replay()
+
+        d = get_image_id({}, {})
+        d.addCallback(self.assertEquals, "ami-giggle")
+        return d
+
+    def test_uses_constraints(self):
+        get_current_ami_m = self.mocker.replace(get_current_ami)
+        get_current_ami_m(ubuntu_release="serendipitous", architecture="x512",
+                          daily=False, persistent_storage=True,
+                          region="blah-north-6")
+        self.mocker.result(succeed("ami-tinkle"))
+        self.mocker.replay()
+
+        constraints = {
+            "architecture": "x512",
+            "ubuntu_release": "serendipitous",
+            "persistent_storage": True,
+            "daily": False}
+        d = get_image_id({"region": "blah-north-6"}, constraints)
+        d.addCallback(self.assertEquals, "ami-tinkle")
         return d

=== modified file 'ensemble/providers/ec2/utils.py'
--- ensemble/providers/ec2/utils.py	2011-08-14 19:24:03 +0000
+++ ensemble/providers/ec2/utils.py	2011-09-13 10:56:30 +0000
@@ -5,22 +5,11 @@
 from twisted.web.client import getPage
 from twisted.internet.defer import succeed
 
-from ensemble.providers.common.utils import format_cloud_init
-
 log = logging.getLogger("ensemble.ec2")
 
-# default image for each region.
-AMI_REGION_MAP = {
-    "us-east-1": "ami-d2ba45bb",
-    "us-west-1": "ami-15693a50",
-    "ap-northeast-1": "ami-80bb1181",
-    "ap-southeast-1": "ami-b4671ee6",
-    "eu-west-1": "ami-0991a67d"
-    }
-
-BASE_IMAGE_URI = "http://uec-images.ubuntu.com/query/"
-CURRENT_IMAGE_URI = BASE_IMAGE_URI + \
-              "%(ubuntu_release_name)s/%(variant)s/%(version)s.current.txt"
+_CURRENT_IMAGE_URI_TEMPLATE = (
+    "http://uec-images.ubuntu.com/query/"
+    "%(ubuntu_release_name)s/%(variant)s/%(version)s.current.txt")
 
 
 def get_region_uri(region):
@@ -28,17 +17,18 @@
     return "https://ec2.%s.amazonaws.com" % region
 
 
-def get_current_ami(ubuntu_release_name="natty", arch="i386", ebs=True,
-                    region="us-east-1", daily=False, desktop=False,
-                    url_fetch=None):
+# XXX ideally should come from latest available or client release name.
+def get_current_ami(ubuntu_release="natty", architecture="i386",
+                    persistent_storage=True, region="us-east-1", daily=False,
+                    desktop=False, url_fetch=None):
     """Get the latest ami for the last release of ubuntu."""
     data = {}
-    data["ubuntu_release_name"] = ubuntu_release_name
+    data["ubuntu_release_name"] = ubuntu_release
     data["version"] = daily and "daily" or "released"
     data["variant"] = desktop and "desktop" or "server"
-    ebs_match = ebs and "ebs" or "instance-store"
+    ebs_match = persistent_storage and "ebs" or "instance-store"
 
-    url = CURRENT_IMAGE_URI % data
+    url = _CURRENT_IMAGE_URI_TEMPLATE % data
     url_fetch = url_fetch or getPage
 
     d = url_fetch(url)
@@ -48,60 +38,18 @@
         for tokens in csv.reader(data_stream, "excel-tab"):
             if tokens[4] != ebs_match:
                 continue
-            if tokens[5] == arch and tokens[6] == region:
+            if tokens[5] == architecture and tokens[6] == region:
                 return tokens[7]
-        raise LookupError((ubuntu_release_name, arch, region,
+        raise LookupError((ubuntu_release, architecture, region,
                            data["version"], data["variant"]))
 
     d.addCallback(extract_ami)
     return d
 
 
-def get_launch_options(config, authorized_keys, packages=None,
-                       repositories=None, scripts=None, **kw):
-    """Determine launch options for the machine.
-
-    Given an ensemble environment configuration, extract the proper machine
-    information to be used for launching the machine via the ec2 api.
-    """
-
-    instance_type = config.get("default-instance-type", "m1.small")
-
-    # XXX ideally should come from latest available or client release name.
+def get_image_id(config, constraints):
+    image_id = config.get("default-image-id", None)
+    if image_id:
+        return succeed(image_id)
     region = config.get("region", "us-east-1")
-    image_id = config.get("default-image-id", None)
-
-    # Lookup a standard ami by region, if none is specified, use any
-    # specified image specification to locate an image, and remove the
-    # image specification so it doesn't propogate to user data.
-    if not image_id:
-        params = {}
-        for a_name, p_name  in (
-            ("image_release_name", "ubuntu_release_name"),
-            ("image_arch", "arch"),
-            ("image_ebs", "ebs"),
-            ("image_daily", "daily")):
-            if a_name in kw:
-                params[p_name] = kw[a_name]
-                del kw[a_name]
-        params["region"] = region
-        d = get_current_ami(**params)
-    else:
-        d = succeed(image_id)
-
-    user_data = format_cloud_init(
-        authorized_keys=authorized_keys,
-        packages=packages,
-        repositories=repositories,
-        scripts=scripts,
-        data=kw)
-
-    def on_machine_image(current_image_id):
-        return {"image_id": current_image_id,
-                "min_count": 1,
-                "max_count": 1,
-                "instance_type": instance_type,
-                "user_data": user_data,
-                "security_groups": []}
-    d.addCallback(on_machine_image)
-    return d
+    return get_current_ami(region=region, **constraints)

=== modified file 'ensemble/providers/orchestra/__init__.py'
--- ensemble/providers/orchestra/__init__.py	2011-08-25 16:41:03 +0000
+++ ensemble/providers/orchestra/__init__.py	2011-09-13 10:56:30 +0000
@@ -1,6 +1,6 @@
 import logging
 
-from twisted.internet.defer import inlineCallbacks, returnValue, succeed
+from twisted.internet.defer import fail, inlineCallbacks, returnValue, succeed
 
 from ensemble.errors import ProviderError
 from ensemble.providers.common.base import MachineProviderBase
@@ -29,13 +29,16 @@
     def start_machine(self, machine_data, master=False):
         """Start a machine in the provider.
 
-        @param machine_data: a dictionary of data to pass along to the newly
-           launched machine.
+        `machine_data`: a dictionary describing the machine to be launched.
 
-        @param master: if True, machine will initialize the ensemble admin
-            and run a provisioning agent.
+        `master`: if True, machine will initialize the ensemble admin and run
+            a provisioning agent, in addition to running a machine agent
         """
-        return OrchestraLaunchMachine(self, master).run(machine_data)
+        if "machine-id" not in machine_data:
+            return fail(ProviderError(
+                "Cannot launch a machine without specifying a machine-id"))
+        machine_id = machine_data["machine-id"]
+        return OrchestraLaunchMachine(self, master).run(machine_id)
 
     @inlineCallbacks
     def get_machines(self, instance_ids=()):

=== modified file 'ensemble/providers/orchestra/launch.py'
--- ensemble/providers/orchestra/launch.py	2011-08-11 23:27:45 +0000
+++ ensemble/providers/orchestra/launch.py	2011-09-13 10:56:30 +0000
@@ -5,14 +5,8 @@
 from twisted.internet.defer import inlineCallbacks, returnValue
 
 from ensemble.providers.common.launch import LaunchMachine
-from ensemble.providers.common.utils import format_cloud_init
-from ensemble.providers.orchestra.machine import machine_from_dict
-
-_INSTANCE_ID_VAR = "ENSEMBLE_INSTANCE_ID"
-
-_SET_INSTANCE_ID_TEMPLATE = "export %s=%%s" % _INSTANCE_ID_VAR
-
-_CLOUD_INIT_KEYS = "authorized_keys packages repositories scripts".split()
+
+from .machine import machine_from_dict
 
 _LATE_COMMAND_TEMPLATE = """
 seed_d=/var/lib/cloud/seed/nocloud-net
@@ -36,17 +30,6 @@
     "' %s /root/late-command")
 
 
-def _cloud_init_vars(raw_vars):
-    # XXX this is nasty; see bug#820892
-    vars = raw_vars.copy()
-    data = {}
-    for key in vars.keys():
-        if key not in _CLOUD_INIT_KEYS:
-            data[key] = vars.pop(key)
-    vars["data"] = data
-    return vars
-
-
 def _base64_gzip(content):
     gzipped = StringIO()
     gzip_file = GzipFile(fileobj=gzipped, mode="wb", compresslevel=9)
@@ -62,28 +45,34 @@
 class OrchestraLaunchMachine(LaunchMachine):
 
     @inlineCallbacks
-    def start_machine(self, variables, machine_id):
+    def start_machine(self, machine_id, zookeepers):
+        """Actually launch a Cobbler system.
+
+        `machine_id`: the ensemble machine ID
+
+        `zookeepers`: a list of `ProviderMachine`s already running zookeeper,
+            which the machine will need to connect to. May be empty.
+
+        Returns a singe-entry list containing a ProviderMachine for the
+        new instance
+        """
         cobbler = self._provider.cobbler
         instance_id = yield cobbler.acquire_system()
-        ks_meta = self._build_ks_meta(instance_id, variables)
+
+        cloud_init = self._create_cloud_init(machine_id, zookeepers)
+        cloud_init.set_provider_type("orchestra")
+        cloud_init.set_instance_id_accessor(instance_id)
+        ks_meta = self._build_ks_meta(
+            cloud_init.render(), machine_id, instance_id)
+
         yield cobbler.set_on_system(instance_id, "ks_meta", ks_meta)
         yield cobbler.start_system(instance_id)
         (info,) = yield cobbler.describe_systems(instance_id)
         returnValue([machine_from_dict(info)])
 
-    def get_instance_id_command(self):
-        return "$" + _INSTANCE_ID_VAR
-
-    def _build_ks_meta(self, instance_id, variables):
-        # XXX it's a little bit dirty to modify scripts at this stage;
-        # but we don't actually know the instance_id at get_variables time.
-        set_instance_id = _SET_INSTANCE_ID_TEMPLATE % instance_id
-        variables["scripts"].insert(0, set_instance_id)
-
-        cloud_init_vars = _cloud_init_vars(variables)
-        user_data = format_cloud_init(**cloud_init_vars)
+    def _build_ks_meta(self, user_data, machine_id, instance_id):
         late_command = _LATE_COMMAND_TEMPLATE % (instance_id, user_data)
         encoded = _base64_gzip(late_command)
         ensemble_late_command = _KSMETA_LATE_COMMAND_TEMPLATE % encoded
-        return _ks_meta(MACHINE_ID=variables["machine-id"],
-                        ENSEMBLE_LATE_COMMAND=ensemble_late_command)
+        return _ks_meta(
+            MACHINE_ID=machine_id, ENSEMBLE_LATE_COMMAND=ensemble_late_command)

=== modified file 'ensemble/providers/orchestra/tests/common.py'
--- ensemble/providers/orchestra/tests/common.py	2011-08-11 23:27:45 +0000
+++ ensemble/providers/orchestra/tests/common.py	2011-09-13 10:56:30 +0000
@@ -108,7 +108,8 @@
             late_command = _extract_late_command(leftover)
 
             expect_path = os.path.join(DATA_DIR, late_command_filename)
-            expect_late_command = open(expect_path).read()
+            with open(expect_path) as f:
+                expect_late_command = f.read()
             self.assertEquals(late_command, expect_late_command)
             return True
         return verify

=== modified file 'ensemble/providers/orchestra/tests/data/bootstrap_late_command'
--- ensemble/providers/orchestra/tests/data/bootstrap_late_command	2011-09-07 22:06:57 +0000
+++ ensemble/providers/orchestra/tests/data/bootstrap_late_command	2011-09-13 10:56:30 +0000
@@ -11,13 +11,13 @@
 apt-upgrade: true
 apt_sources:
 - {source: 'ppa:ensemble/ppa'}
-machine-data: {ensemble-zookeeper-hosts: 'localhost:2181', machine-id: '0'}
+machine-data: {ensemble-provider-type: orchestra, ensemble-zookeeper-hosts: 'localhost:2181',
+  machine-id: '0'}
 output: {all: '| tee -a /var/log/cloud-init-output.log'}
 packages: [bzr, byobu, tmux, python-setuptools, python-twisted, python-argparse, python-txaws,
-  python-zookeeper, bzr, default-jre-headless, zookeeper, zookeeperd]
-runcmd: [export ENSEMBLE_INSTANCE_ID=winston-uid, sudo apt-get -y install ensemble,
-  sudo mkdir -p /var/lib/ensemble, sudo mkdir -p /var/log/ensemble, 'ensemble-admin
-    initialize --instance-id=$ENSEMBLE_INSTANCE_ID --admin-identity="admin:qRBXC1ubEEUqRL6wcBhgmc9xkaY="',
+  python-zookeeper, default-jre-headless, zookeeper, zookeeperd]
+runcmd: [sudo apt-get -y install ensemble, sudo mkdir -p /var/lib/ensemble, sudo mkdir
+    -p /var/log/ensemble, 'ensemble-admin initialize --instance-id=winston-uid --admin-identity=admin:qRBXC1ubEEUqRL6wcBhgmc9xkaY=',
   'ENSEMBLE_MACHINE_ID=0 ENSEMBLE_ZOOKEEPER=localhost:2181 python -m ensemble.agents.machine
     -n --logfile=/var/log/ensemble/machine-agent.log --pidfile=/var/run/ensemble/machine-agent.pid',
   'ENSEMBLE_ZOOKEEPER=localhost:2181 python -m ensemble.agents.provision -n --logfile=/var/log/ensemble/provision-agent.log

=== modified file 'ensemble/providers/orchestra/tests/data/launch_late_command'
--- ensemble/providers/orchestra/tests/data/launch_late_command	2011-09-07 22:06:57 +0000
+++ ensemble/providers/orchestra/tests/data/launch_late_command	2011-09-13 10:56:30 +0000
@@ -11,13 +11,14 @@
 apt-upgrade: true
 apt_sources:
 - {source: 'ppa:ensemble/ppa'}
-machine-data: {ensemble-zookeeper-hosts: 'jennifer:2181', machine-id: '42'}
+machine-data: {ensemble-provider-type: orchestra, ensemble-zookeeper-hosts: 'jennifer:2181',
+  machine-id: '42'}
 output: {all: '| tee -a /var/log/cloud-init-output.log'}
 packages: [bzr, byobu, tmux, python-setuptools, python-twisted, python-argparse, python-txaws,
   python-zookeeper]
-runcmd: [export ENSEMBLE_INSTANCE_ID=winston-uid, sudo apt-get -y install ensemble,
-  sudo mkdir -p /var/lib/ensemble, sudo mkdir -p /var/log/ensemble, 'ENSEMBLE_MACHINE_ID=42
-    ENSEMBLE_ZOOKEEPER=jennifer:2181 python -m ensemble.agents.machine -n --logfile=/var/log/ensemble/machine-agent.log
+runcmd: [sudo apt-get -y install ensemble, sudo mkdir -p /var/lib/ensemble, sudo mkdir
+    -p /var/log/ensemble, 'ENSEMBLE_MACHINE_ID=42 ENSEMBLE_ZOOKEEPER=jennifer:2181
+    python -m ensemble.agents.machine -n --logfile=/var/log/ensemble/machine-agent.log
     --pidfile=/var/run/ensemble/machine-agent.pid']
 ssh_authorized_keys: [this-is-a-public-key]
 

=== modified file 'ensemble/providers/orchestra/tests/test_launch.py'
--- ensemble/providers/orchestra/tests/test_launch.py	2011-08-26 12:24:00 +0000
+++ ensemble/providers/orchestra/tests/test_launch.py	2011-09-13 10:56:30 +0000
@@ -11,6 +11,12 @@
     def test_bad_data(self):
         d = self.get_provider().start_machine({})
         self.assertFailure(d, ProviderError)
+
+        def verify(error):
+            self.assertEquals(
+                str(error),
+                "Cannot launch a machine without specifying a machine-id")
+        d.addCallback(verify)
         return d
 
     def test_no_zookeeper(self):