Merge lp:~bcsaller/pyjuju/unit-placement-policy into lp:pyjuju

[Benjamin Saller]

This branch allows for selecting a policy when deploying and adding units via the ensemble cli

ensemble deploy mysql myblog [--placement local]

ensemble add-unit myblog [--placement local]

if the environment.yaml file has a `placement: <policyname>`

That string will be used to resolve policy when placement isn't provided on the cmd line.

[William Reade (fwereade)]

Generally all looks good to me; please address the following minor issues before you merge.

[1]

It would be good to keep the config requirements in sync between orchestra and ec2, so please (1) merge latest trunk and (2) add handling and tests for placement in the orchestra config schema.

[2]

+class InvalidPlacementPolicy(EnsembleError):

We don't seem to be placing exceptions in consistent places; I see that some things here are using ensemble.state.errors, but InvalidPlacementPolicy is in ensemble.state.placement; meanwhile, several provider-specific errors are in ensemble.errors rather than (say) ensemble.providers.errors . I feel it would be good to rationalise this a bit; I don't strongly favour any particular scheme, though.

[3]

+ self._policy_name = policy_name

I have a slight bias towards making exception attributes public, because I don't see the value of hiding their information: it might be useful to read it from code at some point, without having to change the exception definition. Conversely, anyone crazy enough to actually modify a caught exception isn't going to be put off by a mere leading _ ;-). Just a suggestion, not a condition.

[4]

+def local_placement(client, machine_state_manager, unit_state):

+def unassigned_placement(client, machine_state_manager, unit_state):

+placement_lookup = {

It would be good to make these module-_private, and to rename placement_lookup to _PLACEMENT_LOOKUP, just to make its intent a little clearer at first glance.

[5]

The python string "unassigned" appears in enough places (in the code, not just the tests) that I think I'd prefer it to be named, even if we don't expect it to change all that much (and if you do that, you may as well name "local" as well).

[6]

There are a couple of little pep8 issues that could use a look.

review: Approve

[Benjamin Saller (bcsaller)]

> Generally all looks good to me; please address the following minor issues
> before you merge.
>
> [1]
>
> It would be good to keep the config requirements in sync between orchestra and
> ec2, so please (1) merge latest trunk and (2) add handling and tests for
> placement in the orchestra config schema.

Merged and resolved.

>
> [2]
>
> +class InvalidPlacementPolicy(EnsembleError):
>
> We don't seem to be placing exceptions in consistent places; I see that some
> things here are using ensemble.state.errors, but InvalidPlacementPolicy is in
> ensemble.state.placement; meanwhile, several provider-specific errors are in
> ensemble.errors rather than (say) ensemble.providers.errors . I feel it would
> be good to rationalise this a bit; I don't strongly favour any particular
> scheme, though.

moved to state/errors.py

>
> [3]
>
> + self._policy_name = policy_name
>
> I have a slight bias towards making exception attributes public, because I
> don't see the value of hiding their information: it might be useful to read it
> from code at some point, without having to change the exception definition.
> Conversely, anyone crazy enough to actually modify a caught exception isn't
> going to be put off by a mere leading _ ;-). Just a suggestion, not a
> condition.
>
> [4]
>
> +def local_placement(client, machine_state_manager, unit_state):
>
> +def unassigned_placement(client, machine_state_manager, unit_state):
>
> +placement_lookup = {
>
> It would be good to make these module-_private, and to rename placement_lookup
> to _PLACEMENT_LOOKUP, just to make its intent a little clearer at first
> glance.
>

Done

> [5]
>
> The python string "unassigned" appears in enough places (in the code, not just
> the tests) that I think I'd prefer it to be named, even if we don't expect it
> to change all that much (and if you do that, you may as well name "local" as
> well).
>

defined.

> [6]
>
> There are a couple of little pep8 issues that could use a look.

removed some E303 too many blank lines errors. There are a few line too long errors but only in tests.

[Gustavo Niemeyer (niemeyer)]

Good stuff, +1!

[1]

+ env_defaults = environment.get_serialization_data()
+ options.placement = env_defaults[environment.name].get(
+ "placement", UNASSIGNED_POLICY)

We need a proper API for this rather than poking at the serialization
data, and the default should live in a single place (several call sites
currently carry the default-decision logic).

[2]

- raise NotImplementedError()
+ data = copy.deepcopy(self.config)
+ return data

These changes don't seem testsed in the orchestra API.

[3]

+class InvalidPlacementPolicy(EnsembleError):
+ def __init__(self, policy_name):

Some testing for __str__ please. I know it's trivial, but it's been
wrong before, and the test is also trivial.

[4]

+def _local_placement(client, machine_state_manager, unit_state):
+ machine = yield machine_state_manager.get_machine_state(0)
+ yield unit_state.assign_to_machine(machine)
+ returnValue(machine)

The overall organization feels simple. Thanks.

[5]

+def place_unit(client, policy_name, machine_state_manager, unit_state):
+ """Return machine state of unit_states assignment."""
+
+ placement = _PLACEMENT_LOOKUP.get(policy_name)

I suggest handling the default here. Move policy_name to the end, default to None, and if None assign the default.

[6]

+ # These shouldn't be used with local (but should be available
+ # to prove a different policy is at work)

Nice testing there.

review: Approve

[Benjamin Saller (bcsaller)]

On Mon, Aug 8, 2011 at 4:49 PM, Gustavo Niemeyer <email address hidden> wrote:
> Review: Approve
> Good stuff, +1!
>
>
> [1]
>
> +        env_defaults = environment.get_serialization_data()
> +        options.placement = env_defaults[environment.name].get(
> +            "placement", UNASSIGNED_POLICY)
>
> We need a proper API for this rather than poking at the serialization
> data, and the default should live in a single place (several call sites
> currently carry the default-decision logic).

Added as a property and modified existing test to assert.

>
> [2]
>
> -        raise NotImplementedError()
> +        data = copy.deepcopy(self.config)
> +        return data
>
> These changes don't seem testsed in the orchestra API.
>

Added test

> [3]
>
> +class InvalidPlacementPolicy(EnsembleError):
> +    def __init__(self, policy_name):
>
> Some testing for __str__ please.  I know it's trivial, but it's been
> wrong before, and the test is also trivial.
>

Added test

> [4]
>
> +def _local_placement(client, machine_state_manager, unit_state):
> +    machine = yield machine_state_manager.get_machine_state(0)
> +    yield unit_state.assign_to_machine(machine)
> +    returnValue(machine)
>
> The overall organization feels simple. Thanks.
>
:)

> [5]
>
> +def place_unit(client, policy_name, machine_state_manager, unit_state):
> +    """Return machine state of unit_states assignment."""
> +
> +    placement = _PLACEMENT_LOOKUP.get(policy_name)
>
> I suggest handling the default here.  Move policy_name to the end, default to None, and if None assign the default.

changed.
>
> [6]
>
> +        # These shouldn't be used with local (but should be available
> +        # to prove a different policy is at work)
>
> Nice testing there.
>
> --
> https://code.launchpad.net/~bcsaller/ensemble/unit-placement-policy/+merge/70387
> You are the owner of lp:~bcsaller/ensemble/unit-placement-policy.
>

[lp:~bcsaller/pyjuju/unit-placement-policy]

302. By Benjamin Saller on 2011-08-09

review changes

=== modified file 'ensemble/control/add_unit.py'
--- ensemble/control/add_unit.py	2011-02-15 03:06:33 +0000
+++ ensemble/control/add_unit.py	2011-08-09 01:43:25 +0000
@@ -2,10 +2,10 @@
 
 from twisted.internet.defer import inlineCallbacks
 
-from ensemble.state.errors import NoUnusedMachines
+from ensemble.control.utils import get_environment
+from ensemble.state.machine import MachineStateManager
+from ensemble.state.placement import place_unit, UNASSIGNED_POLICY
 from ensemble.state.service import ServiceStateManager
-from ensemble.state.machine import MachineStateManager
-from ensemble.control.utils import get_environment
 
 
 def configure_subparser(subparsers):
@@ -17,22 +17,32 @@
     sub_parser.add_argument(
         "service_name",
         help="Name of the service a unit should be added for")
+    sub_parser.add_argument(
+        "--placement",
+        help="Placement policy used to place units of this service")
+
     return sub_parser
 
 
 def command(options):
     """Add a new service unit."""
     environment = get_environment(options)
+    if not options.placement:
+        env_defaults = environment.get_serialization_data()
+        options.placement = env_defaults[environment.name].get(
+            "placement", UNASSIGNED_POLICY)
+
     return add_unit(
         options.environments,
         environment,
         options.verbose,
         options.log,
-        options.service_name)
+        options.service_name,
+        options.placement)
 
 
 @inlineCallbacks
-def add_unit(config, environment, verbose, log, service_name):
+def add_unit(config, environment, verbose, log, service_name, policy_name):
     provider = environment.get_machine_provider()
     client = yield provider.connect()
     try:
@@ -41,11 +51,8 @@
 
         service_state = yield service_manager.get_service_state(service_name)
         unit_state = yield service_state.add_unit_state()
-        try:
-            yield unit_state.assign_to_unused_machine()
-        except NoUnusedMachines:
-            machine_state = yield machine_manager.add_machine_state()
-            yield unit_state.assign_to_machine(machine_state)
+
+        yield place_unit(client, policy_name, machine_manager, unit_state)
         log.info("Unit %r added to service %r",
             unit_state.unit_name, service_state.service_name)
     finally:

=== modified file 'ensemble/control/deploy.py'
--- ensemble/control/deploy.py	2011-07-24 21:26:27 +0000
+++ ensemble/control/deploy.py	2011-08-09 01:43:25 +0000
@@ -1,4 +1,3 @@
-import argparse
 import os
 
 import yaml
@@ -13,8 +12,8 @@
 
 from ensemble.state.endpoint import RelationEndpoint
 from ensemble.state.environment import EnvironmentStateManager
-from ensemble.state.errors import NoUnusedMachines
 from ensemble.state.machine import MachineStateManager
+from ensemble.state.placement import place_unit, UNASSIGNED_POLICY
 from ensemble.state.relation import RelationStateManager
 from ensemble.state.service import ServiceStateManager
 
@@ -43,7 +42,9 @@
         "--config",
         help="YAML file containing service options")
 
-
+    sub_parser.add_argument(
+        "--placement",
+        help="Placement policy used to place units of this service")
 
     return sub_parser
 
@@ -53,6 +54,12 @@
     Deploy a formula to Ensemble!
     """
     environment = get_environment(options)
+
+    if not options.placement:
+        env_defaults = environment.get_serialization_data()
+        options.placement = env_defaults[environment.name].get(
+            "placement", UNASSIGNED_POLICY)
+
     return deploy(
         options.environments,
         environment,
@@ -60,12 +67,15 @@
         options.formula,
         options.service_name,
         options.log,
-        options.config)
+        options.config,
+        options.placement)
+
 
 def parse_config_options(config_file, service_name):
     if not os.path.exists(config_file) or \
             not os.access(config_file, os.R_OK):
-        raise ServiceConfigError("Config file '%s' not accessible." % config_file)
+        raise ServiceConfigError("Config file '%s' not accessible." %
+                                 config_file)
 
     options = yaml.load(open(config_file, "r").read())
     if not options or not isinstance(options, dict) or \
@@ -77,10 +87,10 @@
     return options[service_name]
 
 
-
 @inlineCallbacks
 def deploy(env_config, environment, repository_path, formula_name,
-           service_name, log, config_file=None):
+           service_name, log, config_file=None,
+           placement_policy=UNASSIGNED_POLICY):
     """Deploy a formula within an environment.
 
     This will publish the formula to the environment, creating
@@ -130,14 +140,7 @@
         # Create a service unit
         unit_state = yield service_state.add_unit_state()
 
-        try:
-            # Attempt to assign to an unused machine
-            yield unit_state.assign_to_unused_machine()
-        except NoUnusedMachines:
-            # Otherwise create the machine state
-            machine_state = yield machine_manager.add_machine_state()
-            # Associate the unit to the machine
-            yield unit_state.assign_to_machine(machine_state)
+        yield place_unit(client, placement_policy, machine_manager, unit_state)
 
         # Check if we have any peer relations to establish
         if formula.metadata.peers:

=== modified file 'ensemble/control/tests/test_add_unit.py'
--- ensemble/control/tests/test_add_unit.py	2011-02-15 21:07:22 +0000
+++ ensemble/control/tests/test_add_unit.py	2011-08-09 01:43:25 +0000
@@ -86,4 +86,75 @@
             self.output.getvalue())
         yield self.assert_machine_assignments("wordpress", [None])
         yield self.assert_machine_assignments("mysql", [1, 2])
-        
+
+    @inlineCallbacks
+    def test_add_unit_with_local_policy(self):
+        """
+        'ensemble add-unit <service_name>' will add a new service
+        unit of the given service.
+        """
+        # rework setUp so that service_unit1 is on machine 0
+        # to simulate local deployment
+        ms0 = yield self.machine_state_manager.get_machine_state(0)
+        yield self.service_unit1.unassign_from_machine()
+        yield self.service_unit1.assign_to_machine(ms0)
+
+        unit_names = yield self.service_state1.get_unit_names()
+        self.assertEqual(len(unit_names), 1)
+
+        finished = self.setup_cli_reactor()
+        self.setup_exit(0)
+        self.mocker.replay()
+        main(["add-unit", "mysql", "--placement", "local"])
+        yield finished
+
+        # verify the unit and its machine assignment.
+        unit_names = yield self.service_state1.get_unit_names()
+        self.assertEqual(len(unit_names), 2)
+
+        topology = yield self.get_topology()
+        unit = yield self.service_state1.get_unit_state("mysql/1")
+        machine_id = topology.get_service_unit_machine(
+            self.service_state1.internal_id, unit.internal_id)
+        self.assertNotEqual(machine_id, None)
+        self.assertIn(
+            "Unit 'mysql/1' added to service 'mysql'",
+            self.output.getvalue())
+        # adding a second unit still assigns to machine 0 with local policy
+        yield self.assert_machine_assignments("mysql", [0, 0])
+
+    @inlineCallbacks
+    def test_policy_from_environment(self):
+        config = {
+            "ensemble": "environments",
+            "environments": {"firstenv": {
+                    "placement": "local",
+                    "type": "dummy"}}}
+
+        self.write_config(dump(config))
+        self.config.load()
+
+        ms0 = yield self.machine_state_manager.get_machine_state(0)
+        yield self.service_unit1.unassign_from_machine()
+        yield self.service_unit1.assign_to_machine(ms0)
+
+        unit_names = yield self.service_state1.get_unit_names()
+        self.assertEqual(len(unit_names), 1)
+
+        finished = self.setup_cli_reactor()
+        self.setup_exit(0)
+        self.mocker.replay()
+        main(["add-unit", "mysql"])
+        yield finished
+
+        # Verify the local policy was used
+        topology = yield self.get_topology()
+        unit = yield self.service_state1.get_unit_state("mysql/1")
+        machine_id = topology.get_service_unit_machine(
+            self.service_state1.internal_id, unit.internal_id)
+        self.assertNotEqual(machine_id, None)
+        self.assertIn(
+            "Unit 'mysql/1' added to service 'mysql'",
+            self.output.getvalue())
+        # adding a second unit still assigns to machine 0 with local policy
+        yield self.assert_machine_assignments("mysql", [0, 0])

=== modified file 'ensemble/control/tests/test_deploy.py'
--- ensemble/control/tests/test_deploy.py	2011-07-25 01:28:41 +0000
+++ ensemble/control/tests/test_deploy.py	2011-08-09 01:43:25 +0000
@@ -29,7 +29,9 @@
             "ensemble": "environments",
             "environments": {
                 "firstenv": {
-                    "type": "dummy", "admin-secret": "homer"}}}
+                    "type": "dummy",
+                    "admin-secret": "homer",
+                    "placement": "local"}}}
         self.write_config(yaml.dump(config))
         self.config.load()
 
@@ -149,7 +151,7 @@
         command(MATCH(match_config), MATCH(match_environment),
                 self.unbundled_repo_path, "sample", None,
                 MATCH(lambda x: isinstance(x, logging.Logger)),
-                None)
+                None, "unassigned")
         self.mocker.replay()
         self.mocker.result(succeed(True))
 
@@ -239,7 +241,6 @@
         error = yield self.assertFailure(failure, ServiceConfigError)
         self.assertIn("Expected a YAML dict with service name ('myblog').", str(error))
 
-
     @inlineCallbacks
     def test_deploy_with_config(self):
         """Valid config options should be available to the deployed
@@ -271,3 +272,60 @@
             service_state)
         self.assertEqual(len(relations), 1)
         self.assertEqual(relations[0].relation_name, "ring")
+
+    @inlineCallbacks
+    def test_deploy_with_local_policy(self):
+        environment = self.config.get("firstenv")
+        yield deploy.deploy(self.config, environment, self.unbundled_repo_path,
+                            "sample", "myblog",
+                            logging.getLogger("deploy"),
+                            placement_policy="local")
+
+        topology = yield self.get_topology()
+        service_id = topology.find_service_with_name("myblog")
+        self.assertEqual(service_id, "service-%010d" % 0)
+        exists = yield self.client.exists("/services/%s" % service_id)
+        self.assertTrue(exists)
+
+        machine_ids = topology.get_machines()
+        # There should just be the one machine with local deployment
+        self.assertEqual(machine_ids, ["machine-%010d" % 0])
+        exists = yield self.client.exists("/machines/%s" % machine_ids[0])
+        self.assertTrue(exists)
+
+        # Verify the process still completes as expected
+        unit_ids = topology.get_service_units(service_id)
+        self.assertEqual(unit_ids, ["unit-%010d" % 0])
+        exists = yield self.client.exists("/units/%s" % unit_ids[0])
+        self.assertTrue(exists)
+
+        service = yield self.service_state_manager.get_service_state("myblog")
+        unit_state = yield service.get_unit_state("myblog/0")
+        machine_id = yield unit_state.get_assigned_machine_id()
+        self.assertEqual(machine_id, 0)
+
+    @inlineCallbacks
+    def test_deploy_policy_from_environment(self):
+        config = {
+            "ensemble": "environments",
+            "environments": {"firstenv": {
+                    "placement": "local",
+                    "type": "dummy"}}}
+
+        self.write_config(yaml.dump(config))
+        self.config.load()
+
+        finished = self.setup_cli_reactor()
+        self.setup_exit(0)
+        self.mocker.replay()
+        main(["deploy", "--environment", "firstenv",
+              "--repository", self.unbundled_repo_path,
+              "sample", "beekeeper"])
+        yield finished
+
+        # and verify its placed on node 0 (as per local policy)
+        service = yield self.service_state_manager.get_service_state("beekeeper")
+        units = yield service.get_all_unit_states()
+        unit = units[0]
+        machine_id = yield unit.get_assigned_machine_id()
+        self.assertEqual(machine_id, 0)

=== modified file 'ensemble/environment/config.py'
--- ensemble/environment/config.py	2011-08-02 08:12:16 +0000
+++ ensemble/environment/config.py	2011-08-09 01:43:25 +0000
@@ -41,18 +41,27 @@
                         "default-instance-type": String(),
                         "default-ami": String(),
                         "ec2-uri": String(),
-                        "s3-uri": String()},
+                        "s3-uri": String(),
+                        "placement": OneOf(
+                                Constant("unassigned"),
+                                Constant("local"))},
                        optional=["access-key", "secret-key",
                                  "default-instance-type", "default-ami",
-                                 "region", "ec2-uri", "s3-uri"]),
+                                 "region", "ec2-uri", "s3-uri", "placement"]),
         "orchestra": KeyDict({"orchestra-server": String(),
                               "orchestra-user": String(),
                               "orchestra-pass": String(),
                               "admin-secret": String(),
                               "acquired-mgmt-class": String(),
+<<<<<<< TREE
                               "available-mgmt-class": String(),
                               "storage-url": String()},
                              optional=["storage-url"]),
+=======
+                              "available-mgmt-class": String(),
+                              "placement": String()},
+                             optional=["placement"]),
+>>>>>>> MERGE-SOURCE
         "dummy": KeyDict({})}))},
     optional=["default"])
 

=== modified file 'ensemble/environment/environment.py'
--- ensemble/environment/environment.py	2010-10-15 04:51:17 +0000
+++ ensemble/environment/environment.py	2011-08-09 01:43:25 +0000
@@ -37,3 +37,8 @@
             self._machine_provider = MachineProvider(
                 self._name, self._environment_config)
         return self._machine_provider
+
+    @property
+    def placement(self):
+        """Unit placement policy name or None"""
+        return self._environment_config.get("placement")

=== modified file 'ensemble/environment/tests/test_config.py'
--- ensemble/environment/tests/test_config.py	2011-08-05 07:17:27 +0000
+++ ensemble/environment/tests/test_config.py	2011-08-09 01:43:25 +0000
@@ -553,6 +553,31 @@
             self.fail("Did not properly require "
                       "control-buckets when type == 'ec2'")
 
+    def test_ec2_verifies_placement(self):
+        self.config.write_sample()
+        with open(self.default_path) as file:
+            config = yaml.load(file.read())
+            config["environments"]["sample"]["placement"] = "random"
+
+            self.write_config(yaml.dump(config), other_path=True)
+
+        e = self.assertRaises(EnvironmentsConfigError,
+                          self.config.load,
+                          self.other_path)
+        self.assertIn("expected 'unassigned', got 'random'",
+                       str(e))
+
+        with open(self.default_path) as file:
+            config = yaml.load(file.read())
+            # Authorized keys are required for environment serialization.
+            config["environments"]["sample"]["authorized-keys"] = "mickey"
+            config["environments"]["sample"]["placement"] = "local"
+            self.write_config(yaml.dump(config), other_path=True)
+
+        self.config.load(self.other_path)
+        data = self.config.get_default().get_serialization_data()
+        self.assertEqual(data["sample"]["placement"], "local")
+
     def test_orchestra_schema_requires(self):
         requires = (
             "type orchestra-server orchestra-user orchestra-pass "
@@ -573,3 +598,14 @@
             else:
                 self.fail("Did not properly require %s when type == orchestra"
                           % require)
+
+    def test_orchestra_verifies_placement(self):
+        self.config.write_sample()
+        config = yaml.load(SAMPLE_ORCHESTRA)
+
+        config["environments"]["sample"]["placement"] = "local"
+        self.write_config(yaml.dump(config), other_path=True)
+        self.config.load(self.other_path)
+
+        data = self.config.get_default().placement
+        self.assertEqual(data, "local")

=== modified file 'ensemble/environment/tests/test_environment.py'
--- ensemble/environment/tests/test_environment.py	2010-10-08 23:23:48 +0000
+++ ensemble/environment/tests/test_environment.py	2011-08-09 01:43:25 +0000
@@ -78,3 +78,4 @@
         self.mocker.throw(SyntaxError())
         self.mocker.replay()
         self.assertRaises(SyntaxError, env.get_serialization_data)
+

=== modified file 'ensemble/providers/orchestra/__init__.py'
--- ensemble/providers/orchestra/__init__.py	2011-08-05 07:17:27 +0000
+++ ensemble/providers/orchestra/__init__.py	2011-08-09 01:43:25 +0000
@@ -1,4 +1,10 @@
+<<<<<<< TREE
 from twisted.internet.defer import fail
+=======
+import copy
+
+from twisted.internet.defer import fail, succeed
+>>>>>>> MERGE-SOURCE
 
 from ensemble.errors import EnvironmentNotFound
 from ensemble.providers.common.bootstrap import Bootstrap
@@ -99,7 +105,8 @@
 
         Additionally this extracts credential information from the environment.
         """
-        raise NotImplementedError()
+        data = copy.deepcopy(self.config)
+        return data
 
     def open_port(self, machine, port, protocol="tcp"):
         """Expose port to the environment.

=== modified file 'ensemble/providers/orchestra/tests/test_provider.py'
--- ensemble/providers/orchestra/tests/test_provider.py	2011-07-31 00:40:19 +0000
+++ ensemble/providers/orchestra/tests/test_provider.py	2011-08-09 01:43:25 +0000
@@ -15,3 +15,27 @@
             "tetrascape", CONFIG)
         self.assertEquals(provider.environment_name, "tetrascape")
         self.assertEquals(provider.config, CONFIG)
+<<<<<<< TREE
+=======
+
+    def test_config_serialization(self):
+        """
+        The provider configuration can be serialized to yaml.
+        """
+        keys_path = self.makeFile("my-keys")
+
+        config = { "orchestra-user": "chaosmonkey",
+                   "orchestra-pass": "crash",
+                   "acquired-mgmt-class": "madeup",
+                   "available-mgmt-class": "madeup",
+                   "orchestra-server": "127.0.0.01",
+                   "authorized-keys-path": keys_path,
+                   }
+
+        expected_serialization = config.copy()
+
+        provider = MachineProvider("tetrascape", config)
+        serialized = provider.get_serialization_data()
+        self.assertEqual(serialized, expected_serialization)
+
+>>>>>>> MERGE-SOURCE

=== modified file 'ensemble/state/errors.py'
--- ensemble/state/errors.py	2011-07-25 19:36:48 +0000
+++ ensemble/state/errors.py	2011-08-09 01:43:25 +0000
@@ -298,3 +298,13 @@
 
     def __str__(self):
         return "Ambiguous endpoints: %r" % (self.endpoints,)
+
+class InvalidPlacementPolicy(EnsembleError):
+    def __init__(self, policy_name):
+        self._policy_name = policy_name
+
+    def __str__(self):
+        return "Invalid unit placement policy: %s" % self._policy_name
+
+
+

=== added file 'ensemble/state/placement.py'
--- ensemble/state/placement.py	1970-01-01 00:00:00 +0000
+++ ensemble/state/placement.py	2011-08-09 01:43:25 +0000
@@ -0,0 +1,58 @@
+"""Various unit placement strategies for use in deploy and add-unit.
+
+The API used by the `place_unit` method is:
+
+machine_state = yield placement_strategy(zk_client,
+                                         machine_state_manager,
+                                         unit_state)
+
+The placement strategy is passed the machine manager for the
+deployment and the unit_state it is attempting to place. According to
+its policy it should yield back the machine_state for where it placed
+the unit.
+"""
+
+from twisted.internet.defer import inlineCallbacks, returnValue
+
+from ensemble.state.errors import NoUnusedMachines, InvalidPlacementPolicy
+
+LOCAL_POLICY = "local"
+UNASSIGNED_POLICY = "unassigned"
+
+
+@inlineCallbacks
+def _local_placement(client, machine_state_manager, unit_state):
+    machine = yield machine_state_manager.get_machine_state(0)
+    yield unit_state.assign_to_machine(machine)
+    returnValue(machine)
+
+
+@inlineCallbacks
+def _unassigned_placement(client, machine_state_manager, unit_state):
+    try:
+        machine_state = yield unit_state.assign_to_unused_machine()
+    except NoUnusedMachines:
+        machine_state = yield machine_state_manager.add_machine_state()
+        yield unit_state.assign_to_unused_machine()
+
+    returnValue(machine_state)
+
+
+_PLACEMENT_LOOKUP = {
+    LOCAL_POLICY: _local_placement,
+    UNASSIGNED_POLICY: _unassigned_placement,
+    }
+
+
+def place_unit(client, policy_name, machine_state_manager, unit_state):
+    """Return machine state of unit_states assignment."""
+
+    # default policy handling
+    if policy_name is None:
+        placement = _unassigned_placement
+    else:
+        placement = _PLACEMENT_LOOKUP.get(policy_name)
+        if placement is None:
+            raise InvalidPlacementPolicy(policy_name)
+
+    return placement(client, machine_state_manager, unit_state)

=== added file 'ensemble/state/tests/test_placement.py'
--- ensemble/state/tests/test_placement.py	1970-01-01 00:00:00 +0000
+++ ensemble/state/tests/test_placement.py	2011-08-09 01:43:25 +0000
@@ -0,0 +1,57 @@
+
+from twisted.internet.defer import inlineCallbacks
+
+from ensemble.state.errors import InvalidPlacementPolicy
+from ensemble.state.placement import place_unit
+from ensemble.state.tests.test_service import ServiceStateManagerTestBase
+
+
+class TestPlacement(ServiceStateManagerTestBase):
+
+    @inlineCallbacks
+    def setUp(self):
+        yield super(TestPlacement, self).setUp()
+
+        self.service = yield self.add_service_from_formula("mysql")
+        self.unit_state = yield self.service.add_unit_state()
+
+    @inlineCallbacks
+    def test_unassign_placement(self):
+        machine1 = yield self.machine_state_manager.add_machine_state()
+        machine2 = yield self.machine_state_manager.add_machine_state()
+
+        unit2 = yield self.service.add_unit_state()
+
+        # Take machine 1 manually
+        yield self.unit_state.assign_to_machine(machine1)
+
+        ms2 = yield place_unit(self.client, "unassigned", self.machine_state_manager, unit2)
+        self.assertEqual(ms2.id, machine2.id)
+
+        # and placing a new unit creates a new machine state
+        unit3 = yield self.service.add_unit_state()
+        ms3 = yield place_unit(self.client, "unassigned", self.machine_state_manager, unit3)
+        self.assertEqual(ms3.id, machine2.id + 1)
+
+    @inlineCallbacks
+    def test_local_placement(self):
+        ms0 = yield self.machine_state_manager.add_machine_state()
+        self.assertEqual(ms0.id, 0)
+
+        # These shouldn't be used with local (but should be available
+        # to prove a different policy is at work)
+        yield self.machine_state_manager.add_machine_state()
+        yield self.machine_state_manager.add_machine_state()
+        unit2 = yield self.service.add_unit_state()
+
+        ms1 = yield place_unit(self.client, "local", self.machine_state_manager, self.unit_state)
+        ms2 = yield place_unit(self.client, "local", self.machine_state_manager, unit2)
+
+        # Everything should end up on machine 0 with local placement
+        # even though other machines are available
+        self.assertEqual(ms0.id, ms1.id)
+        self.assertEqual(ms0.id, ms2.id)
+
+    def test_invalid_placement_policy_exception(self):
+        ex = InvalidPlacementPolicy("foo")
+        self.assertEqual(str(ex), "Invalid unit placement policy: foo")