Merge lp:~jimbaker/pyjuju/expose-provider-ec2 into lp:pyjuju

[Jim Baker]

This branch implements the following:

 * When launching a machine, it ensures the creation of an overall
   Ensemble security group for all machines, with only port 22/tcp
   open, named "ensemble-ENVIRONMENT", along with a per-machine
   security group named "ensemble-ENVIRONMENT-MACHINE_ID", with no
   ports open.

 * The EC2 provider implementation of open_port, close_port, and
   get_opened_ports corresponds to EC2 operations of
   authorize_security_group, revoke_security_group, and
   describe_security_groups (using txaws). For the latter, the
   IPPermissions model is parsed to return a set of (port, proto)
   pairs for a given machine.

Lastly, this branch also adds this formula change to the wordpress
db-relation-changed hook. Originally this was going to be in a
separate branch, but this change makes it possible to really test!

  # Make it publicly visible, once the wordpress service is exposed
  open-port 80/tcp

Given the above, the branch can be readily tested by deploying the
wordpress and mysql example formulas as usual, then doing:

  ensemble expose wordpress

This will result in debug log output like so:

  2011-07-19 16:24:58,498 provision:ec2: ensemble.agents.provision DEBUG: Service 'wordpress' is exposed
  2011-07-19 16:24:58,840 provision:ec2: ensemble.ec2 DEBUG: Opened 80/tcp on provider machine 'i-4bc10a2a'

Further operations of unexpose/expose are also supported, given the
dependence on lp:~jimbaker/ensemble/expose-provision-machines-reexpose

[William Reade (fwereade)]

* Tiny, irrelevant note: rather than directly creating an EC2ProviderMachine, there's now a machine_from_instance function in ec2.machine which is a touch more convenient.

* Significant issue: I don't like having both 'machine' and 'machine_id' parameters: if there isn't currently a way to determine machine_id given a machine, I think it would be worth adding one and using it, to avoid cluttering up the Provider interface.

review: Needs Fixing

[Gustavo Niemeyer (niemeyer)]

[1]

On an overlook, this feels nice. We just debated online about one detail that
would be good to get handled before this is merged: there's no reason to have
that logic inside classes. Let's talk further online if you want to debate
more on this.

review: Needs Fixing

[Jim Baker (jimbaker)]

On Tue, Jul 26, 2011 at 9:19 AM, William Reade
<email address hidden>wrote:

> Review: Needs Fixing
> * Tiny, irrelevant note: rather than directly creating an
> EC2ProviderMachine, there's now a machine_from_instance function in
> ec2.machine which is a touch more convenient.
>

Now using

>
> * Significant issue: I don't like having both 'machine' and 'machine_id'
> parameters: if there isn't currently a way to determine machine_id given a
> machine, I think it would be worth adding one and using it, to avoid
> cluttering up the Provider interface.
>

Changed such that ProviderMachine now takes an optional machine_id so this
can be passed around. In general, this field needs to be assigned after the
fact (EC2 doesn't know these machine IDs from our topology), and this is
what is done in the provisioning agent. But it does provide a central place
for this information instead of having to pass around two related pieces.

[Jim Baker (jimbaker)]

On Tue, Jul 26, 2011 at 9:51 AM, Gustavo Niemeyer <email address hidden>wrote:

> Review: Needs Fixing
> [1]
>
> On an overlook, this feels nice. We just debated online about one detail
> that
> would be good to get handled before this is merged: there's no reason to
> have
> that logic inside classes. Let's talk further online if you want to debate
> more on this.
>

Removed class wrappers in favor of just functions.

[William Reade (fwereade)]

> Changed such that ProviderMachine now takes an optional machine_id so this
> can be passed around. In general, this field needs to be assigned after the
> fact (EC2 doesn't know these machine IDs from our topology), and this is
> what is done in the provisioning agent. But it does provide a central place
> for this information instead of having to pass around two related pieces.

Cool. One day, it would be good to ensure we do always have machine_id set on ProviderMachine instances, but no clean way to do so springs to mind now, so I don't think it blocks anything.

Everything else looks sensible to me, +1. (And I definitely like the operations as functions :))

review: Approve

[Gustavo Niemeyer (niemeyer)]

Here are a few additional points Jim. Nothing huge, I think:

[1]

         self._watched_services = {}
+ self._watched_machines = set()

Bogus line. It's duplicated.

[2]

             machine = yield self.provider.get_machine(instance_id)
      machine.machine_id = machine_id
      (...)
             current_ports = yield self.provider.get_opened_ports(machine)

We really need to fix this. You ask the provider for a machine, it
gives you the machine with a field it has no idea about how to
retrieve, you stuff an id into that object, and then give it _back_
to the provider because it will need it.

Result:

- The provider is the one responsible for its machines, but can't
  create one with the needed data.
- The provider expects a machine_id in arbitrary moments, and doesn't
  tell when.
- Arbitrary locations set the id, because they magically know the dance.

No one but you understand that. We need a more consistent API to
handle this.

If get_open_ports and friends need unique identifier, let's provide it
explicitly. This should be the external id, not the internal id used
within zookeeper.

Unless the providers can set the machine_id attribute, please take it
off from the machine.

[3]

+ def __init__(self, instance_id, dns_name=None,
+ private_dns_name=None, launch_time=None, machine_id=None):

Per above, please drop this.

[4]

- launch_deferred = self.get_machine_variables(machine_data)
- launch_deferred.addCallback(self.start_machine)
+ machine_variables = yield self.get_machine_variables(machine_data)
+ provider_machines = yield self.start_machine(
+ machine_variables, machine_data)

The idea of having a data bag like machine_data was a bad one. Now we
have logic all around which depends on arbitrary fields inside it, and
do not say so. Let's not increase the problem.

If start_machine needs the machine_id, let's just provide the machine_id
rather than a bag.

[5]

+ return open_provider_port(self, machine, port, protocol)

Much better, thanks!

[6]

+ except EC2Error:
+ # AWS may return an error when the instance is not found
+ # (but presumably eventually will be), eg,
+ # txaws.ec2.exception.EC2Error: Error Message:
+ # The instance ID 'i-afe113ce' does not exist
+ raise ProviderInteractionError(
+ "Machine (id: %r) was not found" % provider_id)

This looks too generic. What other errors can EC2Error represent?

Also, should we perhaps try again a couple of times before giving
up if the provider_id was indeed not found, to compensate for the
boredom of eventual consistency?

For the latter, free to just add a comment in case you don't want
to do now. The former looks like an issue we'll want to address
before merging.

[7]

- # TODO: For now authorize external traffic to the instance,
- # except for the ZooKeeper port. The expose functionality

Good to see that stuff going away.

[8]

+ if ensemble_machine_group in group_ids:
+ yield self._provider.ec2.delete_security_group(
+ ensemble_machine_group)

Has this logic ...

review: Needs Fixing

[Jim Baker (jimbaker)]

On Fri, Jul 29, 2011 at 3:09 PM, Gustavo Niemeyer <email address hidden>wrote:

> Review: Needs Fixing
> Here are a few additional points Jim. Nothing huge, I think:
>
>
> [1]
>
> self._watched_services = {}
> + self._watched_machines = set()
>
>
> Bogus line. It's duplicated.
>

Fixed

>
>
> [2]
>
> machine = yield self.provider.get_machine(instance_id)
> machine.machine_id = machine_id
> (...)
> current_ports = yield self.provider.get_opened_ports(machine)
>
> We really need to fix this. You ask the provider for a machine, it
> gives you the machine with a field it has no idea about how to
> retrieve, you stuff an id into that object, and then give it _back_
> to the provider because it will need it.
>
> Result:
>
> - The provider is the one responsible for its machines, but can't
> create one with the needed data.
> - The provider expects a machine_id in arbitrary moments, and doesn't
> tell when.
> - Arbitrary locations set the id, because they magically know the dance.
>
> No one but you understand that. We need a more consistent API to
> handle this.
>
> If get_open_ports and friends need unique identifier, let's provide it
> explicitly. This should be the external id, not the internal id used
> within zookeeper.
>
> Unless the providers can set the machine_id attribute, please take it
> off from the machine.
>

Changed accordingly to a model where machine_id is passed explicitly.
Confirmed with William that this is OK.

>
>
> [3]
>
> + def __init__(self, instance_id, dns_name=None,
> + private_dns_name=None, launch_time=None,
> machine_id=None):
>
> Per above, please drop this.
>

Reverted

>
> [4]
>
> - launch_deferred = self.get_machine_variables(machine_data)
> - launch_deferred.addCallback(self.start_machine)
> + machine_variables = yield self.get_machine_variables(machine_data)
> + provider_machines = yield self.start_machine(
> + machine_variables, machine_data)
>
> The idea of having a data bag like machine_data was a bad one. Now we
> have logic all around which depends on arbitrary fields inside it, and
> do not say so. Let's not increase the problem.
>
> If start_machine needs the machine_id, let's just provide the machine_id
> rather than a bag.
>

Changed so that start_machine takes machine_id. Note there are two
start_machine functions, which is probably not ideal. I only refactored this
one.

>
> [5]
>
> + return open_provider_port(self, machine, port, protocol)
>
> Much better, thanks!
>

Cool!

>
> [6]
>
> + except EC2Error:
> + # AWS may return an error when the instance is not found
> + # (but presumably eventually will be), eg,
> + # txaws.ec2.exception.EC2Error: Error Message:
> + # The instance ID 'i-afe113ce' does not exist
> + raise ProviderInteractionError(
> + "Machine (id: %r) was not found" % provider_id)
>
> This looks too generic. What other errors can EC2Error represent?
>

Well, I have certainly seen them, but I don't know of a complete list. I
will look. I have made this and the similar u...

[Gustavo Niemeyer (niemeyer)]

Looks good, +1 with a few extra comments:

[13]

+ raise ProviderInteractionError(
+ "Got EC2 error (id %r: %s) when looking up machine" % (
+ provider_id, ex))

Please reword it like this:

    "EC2 error when looking up instance %s: %s" % (provider_id, e)

[14]

+ raise ProviderInteractionError(
+ "Machine (id: %r) was not found" % provider_id)

Please reword as:

"EC2 instance %s was not found"

[15]

+ except EC2Error:
+ log.debug("Ignoring machine group %s that just disappeared",
+ ensemble_machine_group)
+ pass # Security group for machine is no longer around, ignore

The pass + comment may be dropped as it's redundant with the previous line.

[16]

+# TODO These security group functions do not handle the eventual
+# consistency seen with EC2. This is likely not the right place to
+# address this issue. The provisioning agent is more likely the place
+# to do this.

This would mean we're introducing eventual consistency within our API,
which sounds really bad to handle. The eventual consistency aspect
should be handled as closely to the EC2 API as possible, rather than
across all of the library.

Please adapt the comment to reflect this.

[17]

+ "Got EC2 error (id %r: %s) when attempting to open %s/%s" % (
+ machine.instance_id, ex, port, protocol))

"EC2 error when attempting to open %s/%s on machine %s: %s"

[18]

Similar as the above for the other messages.

[19]

+ for ip_permission in security_groups[0].allowed_ips:
+ if ip_permission.cidr_ip != "0.0.0.0/0":

Is it possible for this list to be empty? In which circumstances?
Do we have to handle it to avoid breaking the agent entirely?

[20]

+ if from_port == to_port:
+ # Ignore ranges of ports, since they are set outside of
+ # Ensemble (at this time at least)

This comment seems reversed. This branch of logic is the one that
does _not_ ignore. It should probably be reworded in the positive
side to be more clear ("Only take ...").

[21]

It would be awesome to take the chance we have Mark and Clint here
and take them through a whole run of that branch before merging,
both to share the knowledge and to help ensuring the latest changes
work fine.

review: Approve

[lp:~jimbaker/pyjuju/expose-provider-ec2]

331. By Jim Baker on 2011-08-09

Added logging on creating machine security group

332. By Jim Baker on 2011-08-10

Revised error messages

333. By Jim Baker on 2011-08-10

Add testing around a security group is still active and cannot be deleted

334. By Jim Baker on 2011-08-10

PEP8 & PyFlakes

335. By Jim Baker on 2011-08-10

Merged upstream & resolve conflicts

336. By Jim Baker on 2011-08-10

PEP8/PyFlakes/leftover text conflict

[Jim Baker (jimbaker)]

On Tue, Aug 9, 2011 at 11:41 AM, Gustavo Niemeyer <email address hidden>wrote:

> Review: Approve
> Looks good, +1 with a few extra comments:
>
> [13]
>
> + raise ProviderInteractionError(
> + "Got EC2 error (id %r: %s) when looking up machine" % (
> + provider_id, ex))
>
> Please reword it like this:
>
> "EC2 error when looking up instance %s: %s" % (provider_id, e)
>

Done

>
>
> [14]
>
> + raise ProviderInteractionError(
> + "Machine (id: %r) was not found" % provider_id)
>
> Please reword as:
>
> "EC2 instance %s was not found"
>

Done

>
> [15]
>
> + except EC2Error:
> + log.debug("Ignoring machine group %s that just
> disappeared",
> + ensemble_machine_group)
> + pass # Security group for machine is no longer around,
> ignore
>
> The pass + comment may be dropped as it's redundant with the previous line.
>

Removed pass, but per conversation in sprint room, the logic was wrong, so
it nows raises this as ProviderInteractionError. This is because this is
almost certainly because of a security group that cannot be deleted because
it's used by another instance (generally being shut down). This will be
addressed in another branch that does security group cleanup on ensemble
shutdown.

>
> [16]
>
> +# TODO These security group functions do not handle the eventual
> +# consistency seen with EC2. This is likely not the right place to
> +# address this issue. The provisioning agent is more likely the place
> +# to do this.
>
> This would mean we're introducing eventual consistency within our API,
> which sounds really bad to handle. The eventual consistency aspect
> should be handled as closely to the EC2 API as possible, rather than
> across all of the library.
>
> Please adapt the comment to reflect this.
>

Fixed

>
> [17]
>
> + "Got EC2 error (id %r: %s) when attempting to open %s/%s" % (
> + machine.instance_id, ex, port, protocol))
>
> "EC2 error when attempting to open %s/%s on machine %s: %s"
>

Fixed

>
> [18]
>
> Similar as the above for the other messages.
>

Fixed

>
> [19]
>
> + for ip_permission in security_groups[0].allowed_ips:
> + if ip_permission.cidr_ip != "0.0.0.0/0":
>
> Is it possible for this list to be empty? In which circumstances?
> Do we have to handle it to avoid breaking the agent entirely?
>

security_groups will not be empty

>
> [20]
>
> + if from_port == to_port:
> + # Ignore ranges of ports, since they are set outside of
> + # Ensemble (at this time at least)
>
> This comment seems reversed. This branch of logic is the one that
> does _not_ ignore. It should probably be reworded in the positive
> side to be more clear ("Only take ...").
>

Changed

>
> [21]
>
> It would be awesome to take the chance we have Mark and Clint here
> and take them through a whole run of that branch before merging,
> both to share the knowledge and to help ensuring the latest changes
> work fine.
>

Mark and Jorge both ran this successfully (a reasonable sub for Clint).

=== modified file 'ensemble/agents/provision.py'
--- ensemble/agents/provision.py	2011-08-10 18:22:54 +0000
+++ ensemble/agents/provision.py	2011-08-10 18:22:55 +0000
@@ -317,8 +317,10 @@
                 yield self.open_close_ports(unit_state)
 
             if not exposed:
+                log.debug("Service %r is unexposed", service_name)
                 self._watched_services[service_name] = NotExposed
             else:
+                log.debug("Service %r is exposed", service_name)
                 self._watched_services[service_name] = set()
                 yield self._setup_service_unit_watch(service_state)
 
@@ -332,7 +334,7 @@
         @inlineCallbacks
         def cb_check_service_units(old_service_units, new_service_units):
             watched_units = self._watched_services.get(
-                service_state.service_name)
+                service_state.service_name, NotExposed)
             if not self._running or watched_units is NotExposed:
                 raise StopWatcher()
 
@@ -447,14 +449,15 @@
                     for port in ports:
                         policy_ports.add(
                             (port["port"], port["proto"]))
-
-            current_ports = yield self.provider.get_opened_ports(machine)
+            current_ports = yield self.provider.get_opened_ports(
+                machine, machine_id)
             to_open = policy_ports - current_ports
             to_close = current_ports - policy_ports
             for port, proto in to_open:
-                yield self.provider.open_port(machine, port, proto)
+                yield self.provider.open_port(machine, machine_id, port, proto)
             for port, proto in to_close:
-                yield self.provider.close_port(machine, port, proto)
+                yield self.provider.close_port(
+                    machine, machine_id, port, proto)
         except ProviderInteractionError:
             log.info("No provisioned machine for machine %r", machine_id)
         finally:

=== modified file 'ensemble/agents/tests/test_provision.py'
--- ensemble/agents/tests/test_provision.py	2011-08-10 18:22:54 +0000
+++ ensemble/agents/tests/test_provision.py	2011-08-10 18:22:55 +0000
@@ -652,7 +652,7 @@
         instance_id = yield machine.get_instance_id()
         machine_provider = yield self.agent.provider.get_machine(instance_id)
         provider_ports = yield self.agent.provider.get_opened_ports(
-            machine_provider)
+            machine_provider, machine.id)
         returnValue(provider_ports)
 
     def test_open_close_ports_on_machine(self):

=== modified file 'ensemble/agents/tests/test_unit.py'
--- ensemble/agents/tests/test_unit.py	2011-07-13 09:04:36 +0000
+++ ensemble/agents/tests/test_unit.py	2011-08-10 18:22:55 +0000
@@ -265,8 +265,7 @@
             "app-relation-changed", executor=self.agent.executor)
 
         # trigger the hook that will read service options
-        wordpress_states = yield self.add_opposite_service_unit(
-            self.states)
+        yield self.add_opposite_service_unit(self.states)
 
         # Verify the hook has executed.
         yield hook_complete

=== modified file 'ensemble/agents/unit.py'
--- ensemble/agents/unit.py	2011-07-21 01:15:02 +0000
+++ ensemble/agents/unit.py	2011-08-10 18:22:55 +0000
@@ -188,7 +188,8 @@
         log.debug("Configuration Changed")
 
         if  current_state != "started":
-            log.debug("Configuration updated on service in a non-started state")
+            log.debug(
+                "Configuration updated on service in a non-started state")
             returnValue(None)
 
         yield self.workflow.fire_transition("reconfigure")

=== modified file 'ensemble/machine/__init__.py'
--- ensemble/machine/__init__.py	2011-07-13 22:08:49 +0000
+++ ensemble/machine/__init__.py	2011-08-10 18:22:55 +0000
@@ -2,14 +2,15 @@
 
 class ProviderMachine(object):
     """
-    Representative of a machine resource created by a C{MachineProvider}. The
-    object is typically annotated by the machine provider, such that the
-    provider can perform subsequent actions upon it, using the additional
-    metadata for identification, without leaking these details to consumers of
-    the C{MachineProvider} api.
+    Representative of a machine resource created by a
+    :class:`MachineProvider`. The object is typically annotated by the
+    machine provider, such that the provider can perform subsequent
+    actions upon it, using the additional metadata for identification,
+    without leaking these details to consumers of the
+    :class:`MachineProvider` api.
     """
 
-    def __init__(self, instance_id, dns_name=None, 
+    def __init__(self, instance_id, dns_name=None,
                  private_dns_name=None, launch_time=None):
         self.instance_id = instance_id
         # ideally this would be ip_address, but txaws doesn't expose it.

=== modified file 'ensemble/machine/tests/test_machine.py'
--- ensemble/machine/tests/test_machine.py	2011-01-26 23:39:35 +0000
+++ ensemble/machine/tests/test_machine.py	2011-08-10 18:22:55 +0000
@@ -1,6 +1,4 @@
-
 from ensemble.lib.testing import TestCase
-
 from ensemble.machine import ProviderMachine
 
 

=== modified file 'ensemble/providers/common/launch.py'
--- ensemble/providers/common/launch.py	2011-08-03 14:48:50 +0000
+++ ensemble/providers/common/launch.py	2011-08-10 18:22:55 +0000
@@ -1,6 +1,6 @@
 import copy
 
-from twisted.internet.defer import inlineCallbacks, returnValue, fail
+from twisted.internet.defer import inlineCallbacks, returnValue
 
 from ensemble.errors import ProviderError
 from ensemble.state.auth import make_identity
@@ -69,31 +69,37 @@
         self._provider = provider
         self._bootstrap = bootstrap
 
+    @inlineCallbacks
     def run(self, machine_data):
         """Launch an instance node within the machine provider environment.
 
-        @param 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_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.
         """
         if "machine-id" not in machine_data:
-            return fail(ProviderError(
-                "Machine state `machine-id` not provided in machine_data."))
+            raise ProviderError(
+                "Machine state `machine-id` not provided in machine_data.")
 
-        launch_deferred = self.get_machine_variables(machine_data)
-        launch_deferred.addCallback(self.start_machine)
+        machine_variables = yield self.get_machine_variables(machine_data)
+        provider_machines = yield self.start_machine(
+            machine_variables, machine_data["machine-id"])
         if self._bootstrap:
-            launch_deferred.addCallback(self._on_bootstrap_launched)
-        return launch_deferred
+            yield self._on_bootstrap_launched(provider_machines)
+        returnValue(provider_machines)
 
-    def start_machine(self, variables):
+    def start_machine(self, machine_variables, machine_id):
         """Actually launch a machine for the appropriate provider.
 
-        @param variables: non-provider-specific data, sufficient to
+        `machine_variables`: non-provider-specific data, sufficient to
             define the machine's behaviour once it exists.
 
-        @return: a list of newly-launched ProviderMachines.
+        `machine_id`: the external machine ID (also exists in the
+            above bag of data)
+
+        Returns a singe-entry list containing a ProviderMachine for the
+        new instance
         """
         raise NotImplementedError()
 
@@ -169,7 +175,8 @@
         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 && sudo /usr/bin/bzr co %s ensemble" % \
+                branch,
             "cd /usr/lib/ensemble/ensemble && sudo python setup.py develop"]
 
     def _get_initialize_script(self):

=== modified file 'ensemble/providers/common/tests/test_launch.py'
--- ensemble/providers/common/tests/test_launch.py	2011-08-04 09:50:57 +0000
+++ ensemble/providers/common/tests/test_launch.py	2011-08-10 18:22:55 +0000
@@ -1,10 +1,9 @@
-from cStringIO import StringIO
 import logging
 import tempfile
 
 from twisted.internet.defer import fail, succeed
 
-from ensemble.errors import EnvironmentNotFound, FileNotFound, ProviderError
+from ensemble.errors import EnvironmentNotFound, ProviderError
 from ensemble.lib.testing import TestCase
 from ensemble.providers.common.bootstrap import Bootstrap
 from ensemble.providers.common.launch import (
@@ -18,7 +17,7 @@
 
 class DummyLaunchMachine(LaunchMachine):
 
-    def start_machine(self, variables):
+    def start_machine(self, variables, data):
         if self._bootstrap:
             name = "bootstrapped-instance-id"
         else:

=== modified file 'ensemble/providers/dummy.py'
--- ensemble/providers/dummy.py	2011-08-10 18:22:54 +0000
+++ ensemble/providers/dummy.py	2011-08-10 18:22:55 +0000
@@ -122,7 +122,7 @@
         config["dynamicduck"] = "magic"
         return config
 
-    def open_port(self, machine, port, protocol="tcp"):
+    def open_port(self, machine, machine_id, port, protocol="tcp"):
         """Dummy equivalent of ec2-authorize-group"""
         if not isinstance(machine, DummyMachine):
             return fail(ProviderError("Invalid machine for provider"))
@@ -131,7 +131,7 @@
                   port, protocol, machine.instance_id)
         return succeed(None)
 
-    def close_port(self, machine, port, protocol="tcp"):
+    def close_port(self, machine, machin_id, port, protocol="tcp"):
         """Dummy equivalent of ec2-revoke-group"""
         if not isinstance(machine, DummyMachine):
             return fail(ProviderError("Invalid machine for provider"))
@@ -143,7 +143,7 @@
             pass
         return succeed(None)
 
-    def get_opened_ports(self, machine):
+    def get_opened_ports(self, machine, machine_id):
         """Dummy equivalent of ec2-describe-group
 
         This returns the current exposed ports in the environment for

=== modified file 'ensemble/providers/ec2/__init__.py'
--- ensemble/providers/ec2/__init__.py	2011-08-05 12:43:39 +0000
+++ ensemble/providers/ec2/__init__.py	2011-08-10 18:22:55 +0000
@@ -16,6 +16,8 @@
 from .iterate import EC2MachineIteration
 from .launch import EC2LaunchMachine
 from .machine import get_machine
+from .securitygroup import (
+    open_provider_port, close_provider_port, get_provider_opened_ports)
 from .shutdown import EC2Shutdown, EC2ShutdownMachine
 from .utils import get_region_uri
 
@@ -147,3 +149,15 @@
         @raise: EnvironmentNotFound or EnvironmentPending
         """
         return find_zookeepers(self, get_machine)
+
+    def open_port(self, machine, machine_id, port, protocol="tcp"):
+        """Authorizes `port` using `protocol` on EC2 for `machine`."""
+        return open_provider_port(self, machine, machine_id, port, protocol)
+
+    def close_port(self, machine, machine_id, port, protocol="tcp"):
+        """Revokes `port` using `protocol` on EC2 for `machine`."""
+        return close_provider_port(self, machine, machine_id, port, protocol)
+
+    def get_opened_ports(self, machine, machine_id):
+        """Returns a set of open (port, proto) pairs for `machine`."""
+        return get_provider_opened_ports(self, machine, machine_id)

=== modified file 'ensemble/providers/ec2/accessor.py'
--- ensemble/providers/ec2/accessor.py	2011-07-13 22:08:49 +0000
+++ ensemble/providers/ec2/accessor.py	2011-08-10 18:22:55 +0000
@@ -1,4 +1,5 @@
-from twisted.internet.defer import fail
+from twisted.internet.defer import inlineCallbacks, returnValue
+from txaws.ec2.exception import EC2Error
 
 from ensemble.errors import ProviderInteractionError
 
@@ -9,17 +10,21 @@
 class EC2MachineAccessor(EC2ProviderMachineFilter):
     """Retrieve a specific machine by provider id."""
 
+    @inlineCallbacks
     def run(self, provider_id):
-        d = self._provider.ec2.describe_instances(provider_id)
-        d.addCallback(self._filter_provider_machines)
-        d.addCallback(self._create_provider_machine, provider_id)
-        return d
-
-    def _create_provider_machine(self, instances, provider_id):
-        if not instances:
-            return fail(
-                ProviderInteractionError(
-                    "Machine (id: %r) was not found" % provider_id))
-
-        instance = instances.pop()
-        return machine_from_instance(instance)
+        try:
+            instances = yield self._provider.ec2.describe_instances(
+                provider_id)
+        except EC2Error, e:
+            # AWS may return an error when the instance is not found
+            # (but presumably eventually will be), eg,
+            # txaws.ec2.exception.EC2Error: Error Message:
+            # The instance ID 'i-afe113ce' does not exist
+            raise ProviderInteractionError(
+                "EC2 error when looking up instance %s: %s" % (provider_id, e))
+        filtered = self._filter_provider_machines(instances)
+        if not filtered:
+            raise ProviderInteractionError(
+                "EC2 instance %s was not found" % provider_id)
+        instance = filtered.pop()
+        returnValue(machine_from_instance(instance))

=== modified file 'ensemble/providers/ec2/launch.py'
--- ensemble/providers/ec2/launch.py	2011-07-18 07:07:03 +0000
+++ ensemble/providers/ec2/launch.py	2011-08-10 18:22:55 +0000
@@ -1,5 +1,7 @@
 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
@@ -10,26 +12,30 @@
     """Amazon EC2 operation for launching an instance"""
 
     @inlineCallbacks
-    def start_machine(self, variables):
+    def start_machine(self, machine_variables, machine_id):
         """Actually launch an instance on EC2.
 
-        @param variables: should be a dictionary with entries sufficient
-            to specify everything necessary to launch the requested
-            instance.
-
-        @return: a singe-entry list containing a ProviderMachine for the
-            new instance
+        `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
+        new instance
         """
         # Retrieves standard ec2 run_instances arguments, of note
         # image id, and instance type.
         machine_options = yield get_launch_options(
             self._provider.config,
-            **variables)
+            **machine_variables)
 
-        # Ensure the ensemble security group exists and is included in the
-        # machine instance launch options.
+        # 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_options["security_groups"],
+            machine_id)
 
         # Launch the instance.
         instances = yield self._provider.ec2.run_instances(**machine_options)
@@ -54,24 +60,26 @@
         vars_deferred.addCallback(set_provider_type)
         return vars_deferred
 
-    def _ensure_group(self, groups):
+    @inlineCallbacks
+    def _ensure_group(self, groups, machine_id):
         """Ensure the ensemble group is the machine launch groups.
 
-        Machines launched by ensemble are tagged with a group so they can
-        be distinguished from other machines that might be running on
-        an EC2 account. This group can be specified explicitly or implicitly
-        defined by the environment name.
-
-        @param security_groups: The configured security groups for a machine
-        instance.
+        Machines launched by ensemble are tagged with a group so they
+        can be distinguished from other machines that might be running
+        on an EC2 account. This group can be specified explicitly or
+        implicitly defined by the environment name. In addition, a
+        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.
-        d = self._provider.ec2.describe_security_groups()
-        d.addCallback(self._on_received_groups, groups)
-        return d
-
-    def _on_received_groups(self, security_groups, groups):
+        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)
         group_ids = [group.name for group in security_groups]
 
         # Ensure the provider group is added to the instance groups.
@@ -81,65 +89,49 @@
         # Create the provider group if doesn't exist.
         if not ensemble_group in group_ids:
             log.debug("Creating ensemble provider group %s", ensemble_group)
-            group_created = self._provider.ec2.create_security_group(
+            yield self._provider.ec2.create_security_group(
                 ensemble_group,
                 "Ensemble group for %s" % self._provider.environment_name)
 
             # Authorize SSH.
-            group_created.addCallback(
-                lambda x: self._provider.ec2.authorize_security_group(
-                    ensemble_group,
-                    ip_protocol="tcp",
-                    from_port="22", to_port="22",
-                    cidr_ip="0.0.0.0/0"))
+            yield self._provider.ec2.authorize_security_group(
+                ensemble_group,
+                ip_protocol="tcp",
+                from_port="22", to_port="22",
+                cidr_ip="0.0.0.0/0")
 
             # We need to describe the group to pickup the owner_id for auth.
-            group_created.addCallback(
-                lambda x: self._provider.ec2.describe_security_groups(
-                    ensemble_group))
+            groups_info = yield self._provider.ec2.describe_security_groups(
+                ensemble_group)
 
             # Authorize Internal ZK Traffic
-            group_created.addCallback(
-                lambda groups: self._provider.ec2.authorize_security_group(
-                    ensemble_group,
-                    source_group_name=ensemble_group,
-                    source_group_owner_id=groups.pop().owner_id))
-
-            # TODO: For now authorize external traffic to the instance,
-            # except for the ZooKeeper port.  The expose functionality
-            # that is in progress will address this in a better way.
-            group_created.addCallback(
-                lambda groups: self._provider.ec2.authorize_security_group(
-                    ensemble_group,
-                    ip_protocol="tcp",
-                    from_port="1",
-                    to_port="2180",
-                    cidr_ip="0.0.0.0/0"))
-            group_created.addCallback(
-                lambda groups: self._provider.ec2.authorize_security_group(
-                    ensemble_group,
-                    ip_protocol="tcp",
-                    from_port="2182",
-                    to_port="65535",
-                    cidr_ip="0.0.0.0/0"))
-
-            group_created.addCallback(
-                lambda groups: self._provider.ec2.authorize_security_group(
-                    ensemble_group,
-                    ip_protocol="udp",
-                    from_port="1",
-                    to_port="2180",
-                    cidr_ip="0.0.0.0/0"))
-            group_created.addCallback(
-                lambda groups: self._provider.ec2.authorize_security_group(
-                    ensemble_group,
-                    ip_protocol="udp",
-                    from_port="2182",
-                    to_port="65535",
-                    cidr_ip="0.0.0.0/0"))
-
-            # We use a separate callback here, because we need to wait
-            # on the authorize security group deferred to succeed before
-            # we can return the groups result.
-            return group_created.addCallback(lambda x: groups)
-        return groups
+            yield self._provider.ec2.authorize_security_group(
+                ensemble_group,
+                source_group_name=ensemble_group,
+                source_group_owner_id=groups_info.pop().owner_id)
+
+        # Create the machine-specific group, but first see if there's
+        # one already existing from a previous machine launch;
+        # if so, delete it, since it can have the wrong firewall setup
+        if ensemble_machine_group in group_ids:
+            try:
+                yield self._provider.ec2.delete_security_group(
+                    ensemble_machine_group)
+                log.debug("Deleted existing machine group %s, will replace",
+                          ensemble_machine_group)
+            except EC2Error, e:
+                log.debug("Cannot delete security group %s: %s",
+                          ensemble_machine_group, e)
+                raise ProviderInteractionError(
+                    "EC2 error when attempting to delete "
+                    "security group %s: %s" % (
+                    ensemble_machine_group, e))
+        log.debug("Creating ensemble machine security group %s",
+                  ensemble_machine_group)
+        yield self._provider.ec2.create_security_group(
+            ensemble_machine_group,
+            "Ensemble group for %s machine %s" % (
+                self._provider.environment_name, machine_id))
+        groups.append(ensemble_machine_group)
+
+        returnValue(groups)

=== added file 'ensemble/providers/ec2/securitygroup.py'
--- ensemble/providers/ec2/securitygroup.py	1970-01-01 00:00:00 +0000
+++ ensemble/providers/ec2/securitygroup.py	2011-08-10 18:22:55 +0000
@@ -0,0 +1,90 @@
+from twisted.internet.defer import inlineCallbacks, returnValue
+from txaws.ec2.exception import EC2Error
+
+from ensemble.errors import ProviderInteractionError
+
+from .utils import log
+
+
+def _get_machine_group_name(provider, machine_id):
+    """Get EC2 security group name associated just with `machine_id`."""
+    return "ensemble-%s-%s" % (provider.environment_name, machine_id)
+
+
+# TODO These security group functions do not handle the eventual
+# consistency seen with EC2. A future branch will add support for
+# retry so that using code doesn't have to be aware of this issue.
+#
+# In addition, the functions work with respect to the machine id,
+# since they manipulate a security group permanently associated with
+# the EC2 provided machine, and the machine must be launched into this
+# security group. This security group, per the above
+# `_get_machine_group_name`, embeds the machine id, eg
+# ensemble-moon-42. Ideally, this would not be the case. See the
+# comments associated with the merge proposal of
+# https://code.launchpad.net/~jimbaker/ensemble/expose-provider-ec2/
+
+
+@inlineCallbacks
+def open_provider_port(provider, machine, machine_id, port, protocol):
+    """Authorize `port`/`proto` for the machine security group."""
+    try:
+        yield provider.ec2.authorize_security_group(
+            _get_machine_group_name(provider, machine_id),
+            ip_protocol=protocol,
+            from_port=str(port), to_port=str(port),
+            cidr_ip="0.0.0.0/0")
+        log.debug("Opened %s/%s on provider machine %r",
+                  port, protocol, machine.instance_id)
+    except EC2Error, e:
+        raise ProviderInteractionError(
+            "EC2 error when attempting to open %s/%s on machine %s: %s" % (
+            port, protocol, machine.instance_id, e))
+
+
+@inlineCallbacks
+def close_provider_port(provider, machine, machine_id, port, protocol):
+    """Revoke `port`/`proto` for the machine security group."""
+    try:
+        yield provider.ec2.revoke_security_group(
+            _get_machine_group_name(provider, machine_id),
+            ip_protocol=protocol,
+            from_port=str(port), to_port=str(port),
+            cidr_ip="0.0.0.0/0")
+        log.debug("Closed %s/%s on provider machine %r",
+                  port, protocol, machine.instance_id)
+    except EC2Error, e:
+        raise ProviderInteractionError(
+            "EC2 error when attempting to close %s/%s on machine %s: %s" % (
+            port, protocol, machine.instance_id, e))
+
+
+@inlineCallbacks
+def get_provider_opened_ports(provider, machine, machine_id):
+    """Gets the opened ports for `machine`.
+
+    Retrieves the IP permissions associated with the machine
+    security group, then parses them to return a set of (port,
+    proto) pairs.
+    """
+    try:
+        security_groups = yield provider.ec2.describe_security_groups(
+            _get_machine_group_name(provider, machine_id))
+    except EC2Error, e:
+        raise ProviderInteractionError(
+            "EC2 error when attempting to get opened ports "
+            "on machine %s: %s" % (
+                machine.instance_id, e))
+
+    opened_ports = set()  # made up of (port, protocol) pairs
+    for ip_permission in security_groups[0].allowed_ips:
+        if ip_permission.cidr_ip != "0.0.0.0/0":
+            continue
+        from_port = int(ip_permission.from_port)
+        to_port = int(ip_permission.to_port)
+        if from_port == to_port:
+            # Only return ports that are individually opened. We
+            # ignore multi-port ranges, since they are set outside of
+            # Ensemble (at this time at least)
+            opened_ports.add((from_port, ip_permission.ip_protocol))
+    returnValue(opened_ports)

=== modified file 'ensemble/providers/ec2/tests/common.py'
--- ensemble/providers/ec2/tests/common.py	2011-07-27 10:35:59 +0000
+++ ensemble/providers/ec2/tests/common.py	2011-08-10 18:22:55 +0000
@@ -5,6 +5,7 @@
 from txaws.s3.client import S3Client
 from txaws.s3.exception import S3Error
 from txaws.ec2.client import EC2Client
+from txaws.ec2.exception import EC2Error
 from txaws.ec2.model import Instance, SecurityGroup
 
 from ensemble.lib.mocker import KWARGS, MATCH
@@ -32,6 +33,29 @@
         """
         return MachineProvider(self.env_name, self.get_config())
 
+    def get_ec2_error(self, entity_id,
+                      format="The instance ID %r does not exist",
+                      code=503):
+        """Make a representative EC2Error for `entity_id`, eg AWS instance_id.
+
+        This error is paired with `get_wrapped_ec2_text` below. The
+        default format represents a fairly common error seen in
+        working with EC2. There are others."""
+        message = format % entity_id
+        return EC2Error(
+            "<error><Code>1</Code><Message>%s</Message></error>" % message,
+            code)
+
+    def get_wrapped_ec2_error_text(self, entity_id, reason,
+                                   format="The instance ID %r does not exist"):
+        """By convention, `EC2Error` is wrapped as a `ProviderError`"""
+        message = format % entity_id
+        return (
+            "ProviderError: Interaction with machine provider failed: "
+            "\"EC2 error when attempting to %s %s: "
+            "Error Message: %s\"" % (
+                reason, entity_id, message))
+
     def setUp(self):
         # mock out the aws services
         service_factory = self.mocker.replace(
@@ -95,33 +119,24 @@
             source_group_owner_id="123")
         self.mocker.result(succeed(True))
 
-        self.ec2.authorize_security_group(
-            group_name,
-            ip_protocol="tcp",
-            from_port="1",
-            to_port="2180",
-            cidr_ip="0.0.0.0/0")
-        self.ec2.authorize_security_group(
-            group_name,
-            ip_protocol="tcp",
-            from_port="2182",
-            to_port="65535",
-            cidr_ip="0.0.0.0/0")
-
-        self.ec2.authorize_security_group(
-            group_name,
-            ip_protocol="udp",
-            from_port="1",
-            to_port="2180",
-            cidr_ip="0.0.0.0/0")
-        self.ec2.authorize_security_group(
-            group_name,
-            ip_protocol="udp",
-            from_port="2182",
-            to_port="65535",
-            cidr_ip="0.0.0.0/0")
-
-        self.mocker.result(succeed(True))
+    def _mock_create_machine_group(self, machine_id):
+        machine_group_name = "ensemble-%s-%s" % (self.env_name, machine_id)
+        self.ec2.create_security_group(
+            machine_group_name, "Ensemble group for %s machine %s" % (
+                self.env_name, machine_id))
+        self.mocker.result(succeed(True))
+
+    def _mock_delete_machine_group(self, machine_id):
+        machine_group_name = "ensemble-%s-%s" % (self.env_name, machine_id)
+        self.ec2.delete_security_group(machine_group_name)
+        self.mocker.result(succeed(True))
+
+    def _mock_delete_machine_group_was_deleted(self, machine_id):
+        machine_group_name = "ensemble-%s-%s" % (self.env_name, machine_id)
+        self.ec2.delete_security_group(machine_group_name)
+        self.mocker.result(fail(self.get_ec2_error(
+                    machine_group_name,
+                    "There are active instances using security group %r")))
 
     def _mock_get_zookeeper_hosts(self, hosts=None):
         """
@@ -152,5 +167,3 @@
             # connect grabs the first host of a set.
             self.ec2.describe_instances(hosts[0].instance_id)
             self.mocker.result(succeed([hosts[0]]))
-
-

=== modified file 'ensemble/providers/ec2/tests/test_accessor.py'
--- ensemble/providers/ec2/tests/test_accessor.py	2011-01-26 23:39:35 +0000
+++ ensemble/providers/ec2/tests/test_accessor.py	2011-08-10 18:22:55 +0000
@@ -1,4 +1,4 @@
-from twisted.internet.defer import succeed
+from twisted.internet.defer import fail, succeed
 from txaws.ec2.model import Instance, Reservation
 
 from ensemble.errors import ProviderInteractionError
@@ -54,7 +54,29 @@
 
         def validate_error(error):
             self.assertIn(
-                "Machine (id: 'i-foobar') was not found",
+                "EC2 instance i-foobar was not found",
+                str(error))
+
+        d.addCallback(validate_error)
+        return d
+
+    def test_instance_not_found(self):
+        """Verify that if the instance is not known to EC2,
+        raises ProviderInteractionError.
+        """
+        self.ec2.describe_instances("i-foobar")
+        self.mocker.result(fail(self.get_ec2_error("i-foobar")))
+        self.mocker.replay()
+
+        provider = self.get_provider()
+
+        d = self.assertFailure(
+            provider.get_machine("i-foobar"),
+            ProviderInteractionError)
+
+        def validate_error(error):
+            self.assertIn(
+                "The instance ID \'i-foobar\' does not exist",
                 str(error))
 
         d.addCallback(validate_error)

=== modified file 'ensemble/providers/ec2/tests/test_bootstrap.py'
--- ensemble/providers/ec2/tests/test_bootstrap.py	2011-08-04 09:50:57 +0000
+++ ensemble/providers/ec2/tests/test_bootstrap.py	2011-08-10 18:22:55 +0000
@@ -35,7 +35,7 @@
             MATCH(match_string))
         self.mocker.result(succeed(True))
 
-    def _mock_launch(self):
+    def _mock_launch(self, machine_id):
         """Mock launching a bootstrap machine on ec2."""
         credentials = "admin:%s" % self.get_config()["admin-secret"]
         admin_identity = make_identity(credentials)
@@ -81,7 +81,9 @@
             instance_type="m1.small",
             max_count=1,
             min_count=1,
-            security_groups=["ensemble-%s" % self.env_name],
+            security_groups=[
+                "%s-%s" % ("ensemble", self.env_name),
+                "%s-%s-%s" % ("ensemble", self.env_name, machine_id)],
             user_data=MATCH(verify_user_data))
 
     def test_launch_bootstrap(self):
@@ -95,8 +97,9 @@
         self.ec2.describe_security_groups()
         self.mocker.result(succeed([]))
         self._mock_create_group()
+        self._mock_create_machine_group(0)
         self._mock_launch_utils(region="us-east-1")
-        self._mock_launch()
+        self._mock_launch(0)
         self.mocker.result(succeed([]))
         self._mock_save()
         self.mocker.replay()
@@ -124,8 +127,9 @@
         self.ec2.describe_security_groups()
         self.mocker.result(succeed([
             SecurityGroup("ensemble-%s" % self.env_name, "")]))
+        self._mock_create_machine_group(0)
         self._mock_launch_utils(region="us-east-1")
-        self._mock_launch()
+        self._mock_launch(0)
         self.mocker.result(succeed([]))
         self._mock_save()
         self.mocker.replay()
@@ -201,8 +205,9 @@
         self.ec2.describe_security_groups()
         self.mocker.result(succeed([
             SecurityGroup("ensemble-%s" % self.env_name, "")]))
+        self._mock_create_machine_group(0)
         self._mock_launch_utils(region="us-east-1")
-        self._mock_launch()
+        self._mock_launch(0)
         self.mocker.result(succeed(instances))
         self._mock_save()
         self.mocker.replay()
@@ -229,8 +234,9 @@
         self.ec2.describe_security_groups()
         self.mocker.result(succeed([
             SecurityGroup("ensemble-%s" % self.env_name, "")]))
+        self._mock_create_machine_group(0)
         self._mock_launch_utils(region="us-east-1")
-        self._mock_launch()
+        self._mock_launch(0)
         self.mocker.result(succeed(instances))
         self._mock_save()
         self.mocker.replay()

=== modified file 'ensemble/providers/ec2/tests/test_files.py'
--- ensemble/providers/ec2/tests/test_files.py	2011-08-01 20:25:56 +0000
+++ ensemble/providers/ec2/tests/test_files.py	2011-08-10 18:22:55 +0000
@@ -100,7 +100,7 @@
 
     def test_put_file_slightly_wrong_error(self):
         error = S3Error("<ignored/>", 404)
-        error.errors = [{"Code": "NoSuchKey"}] # note total nonsense
+        error.errors = [{"Code": "NoSuchKey"}]  # note total nonsense
         return self.verify_strange_put_error(error)
 
     def test_put_file_unknown_error(self):

=== modified file 'ensemble/providers/ec2/tests/test_launch.py'
--- ensemble/providers/ec2/tests/test_launch.py	2011-08-02 09:15:47 +0000
+++ ensemble/providers/ec2/tests/test_launch.py	2011-08-10 18:22:55 +0000
@@ -1,10 +1,10 @@
 from yaml import load
 
-from twisted.internet.defer import succeed
-
-from txaws.ec2.model import Instance
-
-from ensemble.errors import EnvironmentNotFound
+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.providers.ec2.machine import EC2ProviderMachine
@@ -17,7 +17,7 @@
 
 class EC2MachineLaunchTest(EC2TestMixin, EC2MachineLaunchMixin, TestCase):
 
-    def _mock_launch(self, instance=None, custom_verify=None):
+    def _mock_launch(self, instance=None, custom_verify=None, machine_id=None):
 
         def verify_user_data(data):
             lines = data.split("\n")
@@ -37,7 +37,9 @@
             instance_type="m1.small",
             max_count=1,
             min_count=1,
-            security_groups=["%s-%s" % ("ensemble", self.env_name)],
+            security_groups=[
+                "%s-%s" % ("ensemble", self.env_name),
+                "%s-%s-%s" % ("ensemble", self.env_name, machine_id)],
             user_data=MATCH(verify_user_data))
 
         if instance:
@@ -45,6 +47,7 @@
         else:
             self.mocker.result(succeed([]))
 
+    @inlineCallbacks
     def test_provider_launch(self):
         """
         The provider can be used to launch a machine with a minimal set of
@@ -55,27 +58,101 @@
         self.ec2.describe_security_groups()
         self.mocker.result(succeed([]))
         self._mock_create_group()
-        self._mock_launch_utils(region="us-east-1")
-        self._mock_get_zookeeper_hosts()
-        self._mock_launch(instance)
-        self.mocker.replay()
-
-        def verify_result(result):
-            self.assertEqual(len(result), 1)
-            result = result.pop()
-            self.assertTrue(isinstance(result, EC2ProviderMachine))
-            self.assertEqual(result.instance_id, instance.instance_id)
-
-        provider = self.get_provider()
-        d = provider.start_machine({"machine-id": "machine-1"})
-        d.addCallback(verify_result)
-        return d
+        self._mock_create_machine_group("machine-1")
+        self._mock_launch_utils(region="us-east-1")
+        self._mock_get_zookeeper_hosts()
+        self._mock_launch(instance, machine_id="machine-1")
+        self.mocker.replay()
+
+        provider = self.get_provider()
+        provided_machines = yield provider.start_machine(
+            {"machine-id": "machine-1"})
+        self.assertEqual(len(provided_machines), 1)
+        self.assertTrue(isinstance(provided_machines[0], EC2ProviderMachine))
+        self.assertEqual(
+            provided_machines[0].instance_id, instance.instance_id)
+
+    @inlineCallbacks
+    def test_provider_launch_existing_security_group(self):
+        """Verify that the launch works if the env security group exists"""
+        instance = Instance("i-foobar", "running", dns_name="x1.example.com")
+        security_group = SecurityGroup("ensemble-moon", "some description")
+
+        self.ec2.describe_security_groups()
+        self.mocker.result(succeed([security_group]))
+        self._mock_create_machine_group("machine-1")
+        self._mock_launch_utils(region="us-east-1")
+        self._mock_get_zookeeper_hosts()
+        self._mock_launch(instance, machine_id="machine-1")
+        self.mocker.replay()
+
+        provider = self.get_provider()
+        provided_machines = yield provider.start_machine(
+            {"machine-id": "machine-1"})
+        self.assertEqual(len(provided_machines), 1)
+        self.assertTrue(isinstance(provided_machines[0], EC2ProviderMachine))
+        self.assertEqual(
+            provided_machines[0].instance_id, instance.instance_id)
+
+    @inlineCallbacks
+    def test_provider_launch_existing_machine_security_group(self):
+        """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")
+
+        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_launch_utils(region="us-east-1")
+        self._mock_get_zookeeper_hosts()
+        self._mock_launch(instance, machine_id="machine-1")
+        self.mocker.replay()
+
+        provider = self.get_provider()
+        provided_machines = yield provider.start_machine(
+            {"machine-id": "machine-1"})
+        self.assertEqual(len(provided_machines), 1)
+        self.assertTrue(isinstance(provided_machines[0], EC2ProviderMachine))
+        self.assertEqual(
+            provided_machines[0].instance_id, instance.instance_id)
+
+    @inlineCallbacks
+    def test_provider_launch_existing_machine_security_group_is_active(self):
+        """Verify launch fails properly if the machine group is stil active.
+
+        This condition occurs when there is a corresponding machine in
+        that security group, generally because it is still shutting
+        down."""
+        machine_group = SecurityGroup(
+            "ensemble-moon-machine-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_get_zookeeper_hosts()
+        self.mocker.replay()
+
+        provider = self.get_provider()
+        ex = yield self.assertFailure(
+            provider.start_machine({"machine-id": "machine-1"}),
+            ProviderInteractionError)
+        self.assertEqual(
+            str(ex),
+            self.get_wrapped_ec2_error_text(
+                "ensemble-moon-machine-1", "delete security group",
+                "There are active instances using security group %r"
+                ))
 
     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()
 
@@ -83,7 +160,8 @@
             self.assertEqual(
                 data["machine-data"]["ensemble-provider-type"], "ec2")
 
-        self._mock_launch(custom_verify=verify_provider_type)
+        self._mock_launch(custom_verify=verify_provider_type,
+                          machine_id="machine-1")
         self.mocker.replay()
 
         provider = self.get_provider()
@@ -128,6 +206,7 @@
         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()
 
@@ -139,7 +218,8 @@
                       "--pidfile=/var/run/ensemble/machine-agent.pid")
             self.assertEqual(script, data["runcmd"][-1])
 
-        self._mock_launch(custom_verify=verify_machine_agent)
+        self._mock_launch(custom_verify=verify_machine_agent,
+                          machine_id="machine-1")
         self.mocker.replay()
 
         provider = self.get_provider()
@@ -151,9 +231,10 @@
         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(instance)
+        self._mock_launch(instance, machine_id="machine-1")
 
     def test_launch_options_known_ami(self):
         self._mock_launch_with_ami_params({})

=== added file 'ensemble/providers/ec2/tests/test_securitygroup.py'
--- ensemble/providers/ec2/tests/test_securitygroup.py	1970-01-01 00:00:00 +0000
+++ ensemble/providers/ec2/tests/test_securitygroup.py	2011-08-10 18:22:55 +0000
@@ -0,0 +1,129 @@
+import logging
+
+from twisted.internet.defer import fail, succeed, inlineCallbacks
+from txaws.ec2.model import IPPermission, SecurityGroup
+
+from ensemble.errors import ProviderInteractionError
+from ensemble.lib.testing import TestCase
+from ensemble.machine import ProviderMachine
+from ensemble.providers.ec2.securitygroup import (
+    open_provider_port, close_provider_port, get_provider_opened_ports)
+
+from .common import EC2TestMixin
+
+
+class EC2SecurityGroupTest(EC2TestMixin, TestCase):
+
+    @inlineCallbacks
+    def test_open_provider_port(self):
+        """Verify open port op will use the correct EC2 API."""
+        log = self.capture_logging("ensemble.ec2", level=logging.DEBUG)
+        machine = ProviderMachine("i-foobar", "x1.example.com")
+        self.ec2.authorize_security_group(
+            "ensemble-moon-machine-1", ip_protocol="tcp", from_port="80",
+            to_port="80", cidr_ip="0.0.0.0/0")
+        self.mocker.result(succeed(True))
+        self.mocker.replay()
+
+        provider = self.get_provider()
+        yield open_provider_port(provider, machine, "machine-1", 80, "tcp")
+        self.assertIn(
+            "Opened 80/tcp on provider machine 'i-foobar'",
+            log.getvalue())
+
+    @inlineCallbacks
+    def test_close_provider_port(self):
+        """Verify close port op will use the correct EC2 API."""
+        log = self.capture_logging("ensemble.ec2", level=logging.DEBUG)
+        machine = ProviderMachine("i-foobar", "x1.example.com")
+        self.ec2.revoke_security_group(
+            "ensemble-moon-machine-1", ip_protocol="tcp", from_port="80",
+            to_port="80", cidr_ip="0.0.0.0/0")
+        self.mocker.result(succeed(True))
+        self.mocker.replay()
+
+        provider = self.get_provider()
+        yield close_provider_port(provider, machine, "machine-1", 80, "tcp")
+        self.assertIn(
+            "Closed 80/tcp on provider machine 'i-foobar'",
+            log.getvalue())
+
+    @inlineCallbacks
+    def test_get_provider_opened_ports(self):
+        """Verify correct parse of IP perms from describe_security_group."""
+        self.ec2.describe_security_groups("ensemble-moon-machine-1")
+        self.mocker.result(succeed([
+                    SecurityGroup(
+                        "ensemble-%s-machine-1" % self.env_name,
+                        "a security group name",
+                        ips=[
+                            IPPermission("udp", "53", "53", "0.0.0.0/0"),
+                            IPPermission("tcp", "80", "80", "0.0.0.0/0"),
+                            # The range 8080-8082 will be ignored
+                            IPPermission("tcp", "8080", "8082", "0.0.0.0/0"),
+                            # Ignore permissions that are not 0.0.0.0/0
+                            IPPermission("tcp", "443", "443", "10.1.2.3")
+                            ])]))
+        self.mocker.replay()
+
+        provider = self.get_provider()
+        machine = ProviderMachine(
+            "i-foobar", "x1.example.com")
+        opened_ports = yield get_provider_opened_ports(
+            provider, machine, "machine-1")
+        self.assertEqual(opened_ports, set([(53, "udp"), (80, "tcp")]))
+
+    @inlineCallbacks
+    def test_open_provider_port_unknown_instance(self):
+        """Verify open port op will use the correct EC2 API."""
+        machine = ProviderMachine("i-foobar", "x1.example.com")
+        self.ec2.authorize_security_group(
+            "ensemble-moon-machine-1", ip_protocol="tcp", from_port="80",
+            to_port="80", cidr_ip="0.0.0.0/0")
+        self.mocker.result(fail(self.get_ec2_error("i-foobar")))
+        self.mocker.replay()
+
+        provider = self.get_provider()
+        ex = yield self.assertFailure(
+            open_provider_port(provider, machine, "machine-1", 80, "tcp"),
+            ProviderInteractionError)
+        self.assertEqual(
+            str(ex),
+            self.get_wrapped_ec2_error_text(
+                "i-foobar", "open 80/tcp on machine"))
+
+    @inlineCallbacks
+    def test_close_provider_port_unknown_instance(self):
+        """Verify open port op will use the correct EC2 API."""
+        machine = ProviderMachine("i-foobar", "x1.example.com")
+        self.ec2.revoke_security_group(
+            "ensemble-moon-machine-1", ip_protocol="tcp", from_port="80",
+            to_port="80", cidr_ip="0.0.0.0/0")
+        self.mocker.result(fail(self.get_ec2_error("i-foobar")))
+        self.mocker.replay()
+
+        provider = self.get_provider()
+        ex = yield self.assertFailure(
+            close_provider_port(provider, machine, "machine-1", 80, "tcp"),
+            ProviderInteractionError)
+        self.assertEqual(
+            str(ex),
+            self.get_wrapped_ec2_error_text(
+                "i-foobar", "close 80/tcp on machine"))
+
+    @inlineCallbacks
+    def test_get_provider_opened_ports_unknown_instance(self):
+        """Verify open port op will use the correct EC2 API."""
+        self.ec2.describe_security_groups("ensemble-moon-machine-1")
+        self.mocker.result(fail(self.get_ec2_error("i-foobar")))
+        self.mocker.replay()
+
+        provider = self.get_provider()
+        machine = ProviderMachine("i-foobar", "x1.example.com")
+        ex = yield self.assertFailure(
+            get_provider_opened_ports(provider, machine, "machine-1"),
+            ProviderInteractionError)
+        self.assertEqual(
+            str(ex),
+            self.get_wrapped_ec2_error_text(
+                "i-foobar", "get opened ports on machine"))

=== modified file 'ensemble/providers/orchestra/launch.py'
--- ensemble/providers/orchestra/launch.py	2011-08-09 21:19:13 +0000
+++ ensemble/providers/orchestra/launch.py	2011-08-10 18:22:55 +0000
@@ -62,7 +62,7 @@
 class OrchestraLaunchMachine(LaunchMachine):
 
     @inlineCallbacks
-    def start_machine(self, variables):
+    def start_machine(self, variables, machine_id):
         cobbler = self._provider.cobbler
         instance_id = yield cobbler.acquire_system()
         ks_meta = self._build_ks_meta(instance_id, variables)

=== modified file 'ensemble/providers/orchestra/tests/common.py'
--- ensemble/providers/orchestra/tests/common.py	2011-08-09 21:19:13 +0000
+++ ensemble/providers/orchestra/tests/common.py	2011-08-10 18:22:55 +0000
@@ -153,4 +153,3 @@
         self.mocker.result(succeed([name]))
         self.proxy_m.callRemote("get_system", name)
         self.mocker.result(succeed({"name": name, "uid": uid}))
-

=== modified file 'ensemble/providers/tests/test_dummy.py'
--- ensemble/providers/tests/test_dummy.py	2011-07-31 00:20:08 +0000
+++ ensemble/providers/tests/test_dummy.py	2011-08-10 18:22:55 +0000
@@ -57,7 +57,8 @@
 
     @inlineCallbacks
     def test_start_machine_accepts_machine_data(self):
-        machines = yield self.provider.start_machine({"machine-id": 0, "a": 1})
+        machines = yield self.provider.start_machine(
+            {"machine-id": 0, "a": 1})
         self.assertEqual(len(machines), 1)
         self.assertTrue(isinstance(machines[0], ProviderMachine))
 
@@ -141,13 +142,13 @@
         """Verifies dummy provider properly works with ports."""
         machines = yield self.provider.start_machine({"machine-id": 0})
         machine = machines[0]
-        yield self.provider.open_port(machine, 25, "tcp")
-        yield self.provider.open_port(machine, 80)
-        yield self.provider.open_port(machine, 53, "udp")
-        yield self.provider.open_port(machine, 443, "tcp")
-        yield self.provider.close_port(machine, 25)
-        yield self.provider.close_port(machine, 25)  # ignored
-        exposed_ports = yield self.provider.get_opened_ports(machine)
+        yield self.provider.open_port(machine, 0, 25, "tcp")
+        yield self.provider.open_port(machine, 0, 80)
+        yield self.provider.open_port(machine, 0, 53, "udp")
+        yield self.provider.open_port(machine, 0, 443, "tcp")
+        yield self.provider.close_port(machine, 0, 25)
+        yield self.provider.close_port(machine, 0, 25)  # ignored
+        exposed_ports = yield self.provider.get_opened_ports(machine, 0)
         self.assertEqual(exposed_ports,
                          set([(53, 'udp'), (80, 'tcp'), (443, 'tcp')]))
 

=== modified file 'examples/wordpress/hooks/db-relation-changed'
--- examples/wordpress/hooks/db-relation-changed	2011-05-03 08:54:13 +0000
+++ examples/wordpress/hooks/db-relation-changed	2011-08-10 18:22:55 +0000
@@ -98,3 +98,6 @@
 # Restart apache
 ensemble-log "Restarting apache2 service"
 /etc/init.d/apache2 restart
+
+# Make it publicly visible, once the wordpress service is exposed
+open-port 80/tcp