Merge lp:~fwereade/pyjuju/preflight-constraints-redux into lp:pyjuju

[William Reade]

Add ec2 preflighting; made orchestra constraint checking more effective

Providers can now register validation functions on their ConstraintSets, to
allow for more sophisticated checking than can be handled with the tools
otherwise available. Constraints are preflighted at unit-add and machine-add
time; these are the only constraints that directly translate to actual
machine specifications, and therefore the only ones that need to be
preflighted. (Environment and service constraints can vary independently and
changes to either can affect the service's viability; until someone tries to
deploy a unit that definitely cannot be deployed, IMO, it's premature to
preflight and complain.)

The EC2 provider actually makes use of the preflight mechanism; the
orchestra changes (disallowing direct use of acquired-mgmt-class/available-
mgmt-class) -- which were the original driver for this story -- have no
reason to be preflight-time changes and are implemented as normal
constraints.

https://codereview.appspot.com/5970084/

[William Reade (fwereade)]

Reviewers: mp+100869_code.launchpad.net,

Message:
Please take a look.

Description:
Add ec2 preflighting; made orchestra constraint checking more effective

Providers can now register validation functions on their ConstraintSets,
to
allow for more sophisticated checking than can be handled with the tools
otherwise available. Constraints are preflighted at unit-add and
machine-add
time; these are the only constraints that directly translate to actual
machine specifications, and therefore the only ones that need to be
preflighted. (Environment and service constraints can vary independently
and
changes to either can affect the service's viability; until someone
tries to
deploy a unit that definitely cannot be deployed, IMO, it's premature to
preflight and complain.)

The EC2 provider actually makes use of the preflight mechanism; the
orchestra changes (disallowing direct use of
acquired-mgmt-class/available-
mgmt-class) -- which were the original driver for this story -- have no
reason to be preflight-time changes and are implemented as normal
constraints.

https://code.launchpad.net/~fwereade/juju/preflight-constraints-redux/+merge/100869

(do not edit description out of merge proposal)

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

Affected files:
   [revision details]
   juju/errors.py
   juju/machine/constraints.py
   juju/machine/tests/test_constraints.py
   juju/providers/common/base.py
   juju/providers/dummy.py
   juju/providers/ec2/__init__.py
   juju/providers/ec2/tests/test_provider.py
   juju/providers/ec2/tests/test_utils.py
   juju/providers/ec2/utils.py
   juju/providers/orchestra/__init__.py
   juju/providers/orchestra/tests/test_provider.py
   juju/state/environment.py
   juju/state/machine.py
   juju/state/service.py
   juju/state/tests/test_environment.py
   juju/state/tests/test_machine.py
   juju/state/tests/test_service.py
   juju/tests/test_errors.py

Unmerged revisions

513. By William Reade on 2012-04-04

copy over from weirded preflight-constraints branch

=== modified file 'juju/errors.py'
--- juju/errors.py	2012-03-17 14:32:43 +0000
+++ juju/errors.py	2012-04-04 20:07:25 +0000
@@ -132,6 +132,16 @@
     """Machine constraints are inappropriate or incomprehensible"""
 
 
+class PreflightError(ConstraintError):
+    """Machine constraints cannot result in a viable instance"""
+
+    def __init__(self, problem):
+        self.problem = problem
+
+    def __str__(self):
+        return "Constraints preflight failed: %s" % self.problem
+
+
 class UnknownConstraintError(ConstraintError):
     """Constraint name not recognised"""
 
 
=== modified file 'juju/machine/constraints.py'
--- juju/machine/constraints.py	2012-03-30 08:41:02 +0000
+++ juju/machine/constraints.py	2012-04-04 20:07:25 +0000
@@ -2,7 +2,9 @@
 import operator
 from UserDict import DictMixin
 
-from juju.errors import ConstraintError, UnknownConstraintError
+from twisted.internet.defer import fail, succeed
+
+from juju.errors import ConstraintError, PreflightError, UnknownConstraintError
 
 log = logging.getLogger("juju.machine.constraints")
 
@@ -71,8 +73,9 @@
     Individual providers can construct ConstraintSets which will be used to
     construct Constraints objects directly relevant to that provider."""
 
-    def __init__(self, provider_type):
+    def __init__(self, provider_type, validator=None):
         self._provider_type = provider_type
+        self._validator = validator
         self._registry = {}
         self._conflicts = {}
 
@@ -81,6 +84,21 @@
         self.register("ubuntu-series", visible=False)
         self.register("provider-type", visible=False)
 
+    def preflight(self, constraints):
+        """Check constraints for sanity.
+
+        Always checks for completeness, and calls any validation function
+        that has been set.
+
+        Will raise PreflightError if constraints cannot be used to provision
+        a machine.
+        """
+        if not constraints.complete:
+            return fail(PreflightError("incomplete constraints"))
+        if self._validator is None:
+            return succeed(None)
+        return self._validator(constraints)
+
     def register(self, name, default=None, converter=_dont_convert,
                  comparer=operator.eq, visible=True):
         """Register a constraint to be handled by this ConstraintSet.
@@ -254,6 +272,19 @@
         return None not in (
             self.get("provider-type"), self.get("ubuntu-series"))
 
+    def preflight(self):
+        """Fails with ConstraintError if the constraints are not valid.
+
+        This allows us to detect invalid combinations of constraints that do
+        not otherwise conflict For example, on Amazon EC2, various instance
+        types can be launched with a choice of architectures, but:
+
+            instance-type=cc2.8xlarge arch=i386
+
+        ...can be guaranteed to fail.
+        """
+        return self._available.preflight(self)
+
     @property
     def data(self):
         """Return a dict suitable for serialisation and reconstruction.
 
=== modified file 'juju/machine/tests/test_constraints.py'
--- juju/machine/tests/test_constraints.py	2012-03-29 10:27:30 +0000
+++ juju/machine/tests/test_constraints.py	2012-04-04 20:07:25 +0000
@@ -1,30 +1,27 @@
 import operator
 
-from juju.errors import ConstraintError
+from twisted.internet.defer import inlineCallbacks, fail, succeed
+
+from juju.errors import ConstraintError, PreflightError
 from juju.lib.testing import TestCase
 from juju.machine.constraints import Constraints, ConstraintSet
 
+
+def _dummy_validate(constraints):
+    if constraints["cpu"] == 666.666:
+        return fail(PreflightError("disturbing symbolism"))
+    return succeed(None)
+
+
 # These objects exist for the convenience of other test files
-dummy_cs = ConstraintSet("dummy")
+dummy_cs = ConstraintSet("dummy", _dummy_validate)
 dummy_cs.register_generics([])
 dummy_constraints = dummy_cs.parse([])
 series_constraints = dummy_constraints.with_series("series")
 
-generic_defaults = {
+dummy_defaults = {
     "arch": "amd64", "cpu": 1, "mem": 512,
-    "ubuntu-series": None, "provider-type": None}
-dummy_defaults = dict(generic_defaults, **{
-    "provider-type": "dummy"})
-ec2_defaults = dict(generic_defaults, **{
-    "provider-type": "ec2",
-    "ec2-zone": None,
-    "instance-type": None})
-orchestra_defaults = {
-    "provider-type": "orchestra",
-    "ubuntu-series": None,
-    "orchestra-classes": None}
-
-all_providers = ["dummy", "ec2", "orchestra"]
+    "ubuntu-series": None, "provider-type": "dummy"}
 
 
 class ConstraintsTestCase(TestCase):
@@ -404,3 +401,27 @@
         cs.register("foo", converter=blam)
         e = self.assertRaises(ConstraintError, cs.load, {"foo": "bar"})
         self.assertEquals(str(e), "Bad 'foo' constraint 'bar': bar")
+
+    @inlineCallbacks
+    def test_preflight(self):
+        self.allow_names("foo", "bar")
+
+        def validate(constraints):
+            if constraints["foo"] != constraints["bar"]:
+                return fail(PreflightError("BORKEN"))
+            return succeed(None)
+
+        cs = ConstraintSet("provider", validate)
+        cs.register("foo")
+        cs.register("bar")
+        constraints = cs.parse([])
+        e = yield self.assertFailure(constraints.preflight(), ConstraintError)
+        self.assertEquals(
+            str(e), "Constraints preflight failed: incomplete constraints")
+
+        constraints = constraints.with_series("series")
+        yield constraints.preflight()
+
+        constraints = cs.parse(["foo=1"]).with_series("series")
+        e = yield self.assertFailure(constraints.preflight(), ConstraintError)
+        self.assertEquals(str(e), "Constraints preflight failed: BORKEN")
 
=== modified file 'juju/providers/common/base.py'
--- juju/providers/common/base.py	2012-03-29 01:37:57 +0000
+++ juju/providers/common/base.py	2012-04-04 20:07:25 +0000
@@ -38,6 +38,7 @@
         * :meth:`get_legacy_config_keys`
         * :meth:`get_placement_policy`
         * :meth:`get_constraint_set`
+        * :meth:`preflight_constraints`
 
     You probably shouldn't override anything else.
     """
@@ -54,7 +55,19 @@
 
     def get_constraint_set(self):
         """Return the set of constraints that are valid for this provider."""
-        return succeed(ConstraintSet(self.provider_type))
+        return succeed(
+            ConstraintSet(self.provider_type, self.preflight_constraints))
+
+    def preflight_constraints(self, constraints):
+        """Check a complete Constraints object for any knowable problems.
+
+        Passing a preflight does not guarantee that the Constraints will result
+        in a viable instance, but it allows us to detect cases in which
+        interactions between nominally independent constraints will result in
+        nonsensical requests. This should raise PreflightError if problems are
+        detected.
+        """
+        return succeed(None)
 
     def get_legacy_config_keys(self):
         """Return any deprecated config keys that are set."""
 
=== modified file 'juju/providers/dummy.py'
--- juju/providers/dummy.py	2012-03-29 01:37:57 +0000
+++ juju/providers/dummy.py	2012-04-04 20:07:25 +0000
@@ -11,7 +11,7 @@
 
 
 from juju.machine import ProviderMachine
-from juju.machine.constraints import ConstraintSet
+from juju.machine.tests.test_constraints import dummy_cs
 from juju.state.placement import UNASSIGNED_POLICY
 from juju.providers.common.files import FileStorage
 
@@ -46,9 +46,7 @@
         return self.config.get("placement", UNASSIGNED_POLICY)
 
     def get_constraint_set(self):
-        cs = ConstraintSet(self.provider_type)
-        cs.register_generics([])
-        return succeed(cs)
+        return succeed(dummy_cs)
 
     @property
     def provider_type(self):
 
=== modified file 'juju/providers/ec2/__init__.py'
--- juju/providers/ec2/__init__.py	2012-03-28 19:48:24 +0000
+++ juju/providers/ec2/__init__.py	2012-04-04 20:07:25 +0000
@@ -7,7 +7,7 @@
 from txaws.service import AWSServiceRegion
 
 from juju.errors import (
-    MachinesNotFound, ProviderError, ProviderInteractionError)
+    MachinesNotFound, ProviderError, ProviderInteractionError, PreflightError)
 from juju.providers.common.base import MachineProviderBase
 
 from .files import FileStorage
@@ -16,7 +16,9 @@
 from .securitygroup import (
     open_provider_port, close_provider_port, get_provider_opened_ports,
     remove_security_groups, destroy_environment_security_group)
-from .utils import convert_zone, get_region_uri, DEFAULT_REGION, INSTANCE_TYPES
+from .utils import (
+    convert_zone, get_region_uri, get_machine_spec,
+    DEFAULT_REGION, INSTANCE_TYPES)
 
 
 class MachineProvider(MachineProviderBase):
@@ -56,6 +58,22 @@
             cs.register("ec2-zone", converter=convert_zone)
         returnValue(cs)
 
+    @inlineCallbacks
+    def preflight_constraints(self, constraints):
+        """Check a complete Constraints object for any knowable problems.
+
+        Passing a preflight does not guarantee that the Constraints will result
+        in a viable instance, but it allows us to detect cases in which
+        interactions between nominally independent constraints will result in
+        nonsensical requests. This should raise PreflightError if problems are
+        detected.
+        """
+        yield super(MachineProvider, self).preflight_constraints(constraints)
+        try:
+            yield get_machine_spec(self.config, constraints)
+        except Exception as e:
+            raise PreflightError(e)
+
     def get_legacy_config_keys(self):
         """Return any deprecated config keys that are set"""
         legacy = super(MachineProvider, self).get_legacy_config_keys()
 
=== modified file 'juju/providers/ec2/tests/test_provider.py'
--- juju/providers/ec2/tests/test_provider.py	2012-03-28 19:48:24 +0000
+++ juju/providers/ec2/tests/test_provider.py	2012-04-04 20:07:25 +0000
@@ -205,6 +205,19 @@
             "provider-type": "ec2",
             "ubuntu-series": None})
 
+    @inlineCallbacks
+    def test_preflight_constraints(self):
+        cs = yield self.constraint_set()
+        constraints = cs.parse(["instance-type=cc2.8xlarge", "arch=i386"])
+        e = yield self.assertFailure(constraints.preflight(), ConstraintError)
+        self.assertEquals(
+            str(e), "Constraints preflight failed: incomplete constraints")
+
+        constraints = constraints.with_series("series")
+        e = yield self.assertFailure(constraints.preflight(), ConstraintError)
+        self.assertTrue(str(e).startswith(
+            "Constraints preflight failed: No instance type satisfies {"))
+
 
 class FailCreateTest(TestCase):
 
 
=== modified file 'juju/providers/ec2/tests/test_utils.py'
--- juju/providers/ec2/tests/test_utils.py	2012-03-28 09:14:33 +0000
+++ juju/providers/ec2/tests/test_utils.py	2012-04-04 20:07:25 +0000
@@ -235,6 +235,8 @@
         yield self.assert_no_instance_type(["mem=100G"])
         yield self.assert_no_instance_type(["arch=i386", "mem=5G"])
         yield self.assert_no_instance_type(["arch=arm"])
+        yield self.assert_no_instance_type([
+            "arch=i386", "instance-type=m1.large"])
 
     @inlineCallbacks
     def test_config_override(self):
 
=== modified file 'juju/providers/ec2/utils.py'
--- juju/providers/ec2/utils.py	2012-03-28 19:48:24 +0000
+++ juju/providers/ec2/utils.py	2012-04-04 20:07:25 +0000
@@ -132,8 +132,9 @@
         return instance_type
     instance_type = constraints["instance-type"]
     if instance_type is not None:
-        return instance_type
-    possible_types = list(INSTANCE_TYPES)
+        possible_types = [instance_type]
+    else:
+        possible_types = list(INSTANCE_TYPES)
     for f in _get_filters(constraints):
         possible_types = filter(f, possible_types)
     if not possible_types:
 
=== modified file 'juju/providers/orchestra/__init__.py'
--- juju/providers/orchestra/__init__.py	2012-03-28 02:00:16 +0000
+++ juju/providers/orchestra/__init__.py	2012-04-04 20:07:25 +0000
@@ -2,7 +2,7 @@
 
 from twisted.internet.defer import inlineCallbacks, returnValue, succeed
 
-from juju.errors import ProviderError
+from juju.errors import ProviderError, ConstraintError
 from juju.providers.common.base import MachineProviderBase
 
 from .cobbler import CobblerClient
@@ -13,6 +13,11 @@
 log = logging.getLogger("juju.orchestra")
 
 
+def _compare_classes(candidate, benchmark):
+    """Constraint comparer for orchestra-classes"""
+    return set(candidate) >= set(benchmark)
+
+
 class MachineProvider(MachineProviderBase):
     """MachineProvider for use in an Orchestra environment"""
 
@@ -25,14 +30,28 @@
     def provider_type(self):
         return "orchestra"
 
+    def _convert_classes(self, s):
+        """Constraint converter for orchestra-classes"""
+        reserved = [
+            self.config["acquired-mgmt-class"],
+            self.config["available-mgmt-class"]]
+        mgmt_classes = filter(None, s.split(","))
+        for mgmt_class in mgmt_classes:
+            if mgmt_class in reserved:
+                raise ConstraintError(
+                    "The management class %r is used internally and may not "
+                    "be specified directly."
+                    % mgmt_class)
+        return mgmt_classes or None
+
     @inlineCallbacks
     def get_constraint_set(self):
         """Return the set of constraints that are valid for this provider."""
         cs = yield super(MachineProvider, self).get_constraint_set()
-        converter = lambda s: s.split(",")
-        comparer = lambda c, b: set(c) >= set(b)
         cs.register(
-            "orchestra-classes", converter=converter, comparer=comparer)
+            "orchestra-classes",
+            converter=self._convert_classes,
+            comparer=_compare_classes)
         returnValue(cs)
 
     def get_file_storage(self):
 
=== modified file 'juju/providers/orchestra/tests/test_provider.py'
--- juju/providers/orchestra/tests/test_provider.py	2012-03-28 02:00:16 +0000
+++ juju/providers/orchestra/tests/test_provider.py	2012-04-04 20:07:25 +0000
@@ -1,6 +1,7 @@
 from twisted.internet.defer import inlineCallbacks
 
 from juju.environment.errors import EnvironmentsConfigError
+from juju.errors import ConstraintError
 from juju.lib.testing import TestCase
 from juju.providers.orchestra import MachineProvider
 
@@ -80,10 +81,13 @@
         self.assertIn(
             "Firewalling is not yet implemented in Orchestra", log.getvalue())
 
+    def constraint_set(self):
+        provider = MachineProvider("some-orchestra-env", CONFIG)
+        return provider.get_constraint_set()
+
     @inlineCallbacks
     def test_constraints(self):
-        provider = MachineProvider("some-orchestra-env", CONFIG)
-        cs = yield provider.get_constraint_set()
+        cs = yield self.constraint_set()
         self.assertEquals(cs.parse([]), {
             "provider-type": "orchestra",
             "ubuntu-series": None,
@@ -92,7 +96,30 @@
             "provider-type": "orchestra",
             "ubuntu-series": None,
             "orchestra-classes": ["a", "b", "c"]})
+        self.assertEquals(cs.parse(["orchestra-classes=a,"]), {
+            "provider-type": "orchestra",
+            "ubuntu-series": None,
+            "orchestra-classes": ["a"]})
+        self.assertEquals(cs.parse(["orchestra-classes=,"]), {
+            "provider-type": "orchestra",
+            "ubuntu-series": None,
+            "orchestra-classes": None})
+        e = self.assertRaises(
+            ConstraintError, cs.parse, ["orchestra-classes=foo,available"])
+        self.assertEquals(
+            str(e),
+            "The management class 'available' is used internally and may not "
+            "be specified directly.")
+        e = self.assertRaises(
+            ConstraintError, cs.parse, ["orchestra-classes=acquired,bar"])
+        self.assertEquals(
+            str(e),
+            "The management class 'acquired' is used internally and may not "
+            "be specified directly.")
 
+    @inlineCallbacks
+    def test_satsify_constraints(self):
+        cs = yield self.constraint_set()
         a = cs.parse(["orchestra-classes=a"]).with_series("series")
         ab = cs.parse(["orchestra-classes=a,b"]).with_series("series")
         ac = cs.parse(["orchestra-classes=a,c"]).with_series("series")
 
=== modified file 'juju/state/environment.py'
--- juju/state/environment.py	2012-03-30 08:26:21 +0000
+++ juju/state/environment.py	2012-04-04 20:07:25 +0000
@@ -71,6 +71,15 @@
             data = {}
         returnValue(constraint_set.load(data))
 
+    @inlineCallbacks
+    def get_final_constraints(self, override_data):
+        """Collapse a Constraints' data into environment constraints."""
+        constraint_set = yield self.get_constraint_set()
+        override_constraints = constraint_set.load(override_data)
+        final_constraints = yield self.get_constraints()
+        final_constraints.update(override_constraints)
+        returnValue(final_constraints)
+
     # TODO The environment waiting/watching logic in the
     #      provisioning agent should be moved here (#640726).
 
 
=== modified file 'juju/state/machine.py'
--- juju/state/machine.py	2012-03-29 15:42:21 +0000
+++ juju/state/machine.py	2012-04-04 20:07:25 +0000
@@ -3,7 +3,6 @@
 
 from twisted.internet.defer import inlineCallbacks, returnValue
 
-from juju.errors import ConstraintError
 from juju.state.agent import AgentStateMixin
 from juju.state.environment import EnvironmentStateManager
 from juju.state.errors import MachineStateNotFound, MachineStateInUse
@@ -20,9 +19,7 @@
 
         @return: MachineState for the created machine.
         """
-        if not constraints.complete:
-            raise ConstraintError(
-                "Unprovisionable machine: incomplete constraints")
+        yield constraints.preflight()
         machine_data = {"constraints": constraints.data}
         path = yield self._client.create(
             "/machines/machine-", yaml.dump(machine_data),
 
=== modified file 'juju/state/service.py'
--- juju/state/service.py	2012-03-30 10:13:09 +0000
+++ juju/state/service.py	2012-04-04 20:07:25 +0000
@@ -372,10 +372,8 @@
         be set when changing service constraints).
         """
         esm = EnvironmentStateManager(self._client)
-        constraints = yield esm.get_constraints()
-        constraint_set = yield esm.get_constraint_set()
         service_data = yield self._get_node_value("constraints", {})
-        constraints.update(constraint_set.load(service_data))
+        constraints = yield esm.get_final_constraints(service_data)
         returnValue(constraints)
 
     @inlineCallbacks
@@ -412,6 +410,7 @@
         apparently deployed on wildly inappropriate machines)).
         """
         constraints = yield self.get_constraints()
+        yield constraints.preflight()
         charm_id = yield self.get_charm_id()
         unit_data = {"charm": charm_id, "constraints": constraints.data}
         path = yield self._client.create(
 
=== modified file 'juju/state/tests/test_environment.py'
--- juju/state/tests/test_environment.py	2012-03-29 01:37:57 +0000
+++ juju/state/tests/test_environment.py	2012-04-04 20:07:25 +0000
@@ -106,16 +106,65 @@
             "cpu": 10.0,
             "mem": 512.0})
 
+    @inlineCallbacks
+    def test_get_constraints(self):
+        yield self.push_default_config()
+        constraints = yield self.environment_state_manager.get_constraints()
+        self.assertEquals(constraints, {
+            "ubuntu-series": None,
+            "provider-type": "dummy",
+            "arch": "amd64",
+            "cpu": 1.0,
+            "mem": 512.0})
+
+    @inlineCallbacks
+    def test_get_constraints_env_with_no_node(self):
+        yield self.push_default_config()
+        self.client.delete("/constraints")
+        constraints = yield self.environment_state_manager.get_constraints()
+        self.assertEquals(constraints, {
+            "ubuntu-series": None,
+            "provider-type": None,
+            "arch": "amd64",
+            "cpu": 1.0,
+            "mem": 512.0})
+
     def test_get_constraints_no_env(self):
         d = self.environment_state_manager.get_constraints()
         return self.assertFailure(d, EnvironmentStateNotFound)
 
-    @inlineCallbacks
-    def test_get_constraints_env_with_no_node(self):
+    def _get_final_constraints(self, data):
+        return self.environment_state_manager.get_final_constraints(data)
+
+    @inlineCallbacks
+    def test_get_final_constraints(self):
+        yield self.push_default_config()
+        constraints = yield self._get_final_constraints({
+            "ubuntu-series": "series",
+            "arch": None,
+            "mem": "2G"})
+        self.assertEquals(constraints, {
+            "ubuntu-series": "series",
+            "provider-type": "dummy",
+            "arch": None,
+            "cpu": 1.0,
+            "mem": 2048.0})
+
+    @inlineCallbacks
+    def test_get_final_constraints_env_with_no_node(self):
         yield self.push_default_config()
         self.client.delete("/constraints")
-        constraints = yield self.environment_state_manager.get_constraints()
-        self.assertEquals(constraints.data, {})
+        constraints = yield self._get_final_constraints({"cpu": "512"})
+        self.assertEquals(constraints, {
+            "ubuntu-series": None,
+            "provider-type": None,
+            "arch": "amd64",
+            "cpu": 512.0,
+            "mem": 512.0})
+
+    def test_get_final_constraints_no_env(self):
+        d = self._get_final_constraints({})
+        return self.assertFailure(d, EnvironmentStateNotFound)
 
     @inlineCallbacks
     def test_set_constraints(self):
 
=== modified file 'juju/state/tests/test_machine.py'
--- juju/state/tests/test_machine.py	2012-03-28 00:38:16 +0000
+++ juju/state/tests/test_machine.py	2012-04-04 20:07:25 +0000
@@ -66,11 +66,11 @@
         self.assertTrue(topology.has_machine("machine-0000000001"))
 
     @inlineCallbacks
-    def test_incomplete_constraints(self):
+    def test_preflights_constraints(self):
         e = yield self.assertFailure(
             self.add_machine_state(dummy_constraints), ConstraintError)
         self.assertEquals(
-            str(e), "Unprovisionable machine: incomplete constraints")
+            str(e), "Constraints preflight failed: incomplete constraints")
 
     @inlineCallbacks
     def test_missing_constraints(self):
 
=== modified file 'juju/state/tests/test_service.py'
--- juju/state/tests/test_service.py	2012-03-30 10:13:09 +0000
+++ juju/state/tests/test_service.py	2012-04-04 20:07:25 +0000
@@ -10,12 +10,14 @@
 from juju.charm.directory import CharmDirectory
 from juju.charm.tests import local_charm_id
 from juju.charm.tests.test_metadata import test_repository_path
+from juju.errors import ConstraintError
 from juju.machine.tests.test_constraints import (
     dummy_constraints, dummy_cs, series_constraints)
 from juju.lib.pick import pick_attr
 from juju.state.charm import CharmStateManager
 from juju.charm.tests.test_repository import unbundled_repository
 from juju.state.endpoint import RelationEndpoint
+from juju.state.environment import EnvironmentStateManager
 from juju.state.service import (
     ServiceStateManager, NO_HOOKS, RETRY_HOOKS, parse_service_name)
 from juju.state.machine import MachineStateManager
@@ -394,13 +396,29 @@
         under it, and again the unit should offer attributes allowing
         its identification.
         """
+        arch_constraints = dummy_cs.parse(["arch=arm"])
         service_state0 = yield self.service_state_manager.add_service_state(
-            "wordpress", self.charm_state, dummy_constraints)
+            "wordpress", self.charm_state, arch_constraints)
         service_state1 = yield self.service_state_manager.add_service_state(
             "mysql", self.charm_state, dummy_constraints)
 
+        # Temporarily break preflighting and try to add some states
+        esm = EnvironmentStateManager(self.client)
+        yield esm.set_constraints(dummy_cs.parse(["cpu=666.666"]))
+        for service_state in (service_state0, service_state1):
+            d = service_state.add_unit_state()
+            e = yield self.assertFailure(d, ConstraintError)
+            self.assertEquals(
+                str(e), "Constraints preflight failed: disturbing symbolism")
+        yield esm.set_constraints(dummy_cs.parse(["mem=2G"]))
+        # None of the above should have cause a unit node to be created.
+
         unit_state0 = yield service_state0.add_unit_state()
         unit_state1 = yield service_state1.add_unit_state()
+
+        # Change the effective service constraints so we can verify the unit
+        # constraints are creation-time snapshots of the service state.
+        yield esm.set_constraints(dummy_cs.parse(["mem=4G"]))
         unit_state2 = yield service_state0.add_unit_state()
         unit_state3 = yield service_state1.add_unit_state()
 
@@ -412,18 +430,46 @@
         self.assertEquals(unit_state0.service_name, "wordpress")
         self.assertEquals(unit_state0.internal_id, "unit-0000000000")
         self.assertEquals(unit_state0.unit_name, "wordpress/0")
+        constraints0 = yield unit_state0.get_constraints()
+        self.assertEquals(constraints0, {
+            "provider-type": "dummy",
+            "ubuntu-series": "series",
+            "arch": "arm",
+            "cpu": 1.0,
+            "mem": 2048.0})
 
         self.assertEquals(unit_state1.service_name, "mysql")
         self.assertEquals(unit_state1.internal_id, "unit-0000000001")
         self.assertEquals(unit_state1.unit_name, "mysql/0")
+        constraints1 = yield unit_state1.get_constraints()
+        self.assertEquals(constraints1, {
+            "provider-type": "dummy",
+            "ubuntu-series": "series",
+            "arch": "amd64",
+            "cpu": 1.0,
+            "mem": 2048.0})
 
         self.assertEquals(unit_state2.service_name, "wordpress")
         self.assertEquals(unit_state2.internal_id, "unit-0000000002")
         self.assertEquals(unit_state2.unit_name, "wordpress/1")
+        constraints2 = yield unit_state2.get_constraints()
+        self.assertEquals(constraints2, {
+            "provider-type": "dummy",
+            "ubuntu-series": "series",
+            "arch": "arm",
+            "cpu": 1.0,
+            "mem": 4096.0})
 
         self.assertEquals(unit_state3.service_name, "mysql")
         self.assertEquals(unit_state3.internal_id, "unit-0000000003")
         self.assertEquals(unit_state3.unit_name, "mysql/1")
+        constraints3 = yield unit_state3.get_constraints()
+        self.assertEquals(constraints3, {
+            "provider-type": "dummy",
+            "ubuntu-series": "series",
+            "arch": "amd64",
+            "cpu": 1.0,
+            "mem": 4096.0})
 
         topology = yield self.get_topology()
 
 
=== modified file 'juju/tests/test_errors.py'
--- juju/tests/test_errors.py	2012-03-17 14:32:43 +0000
+++ juju/tests/test_errors.py	2012-04-04 20:07:25 +0000
@@ -4,7 +4,7 @@
     InvalidUser, ProviderError, CloudInitError, ProviderInteractionError,
     CannotTerminateMachine, MachinesNotFound, EnvironmentPending,
     EnvironmentNotFound, IncompatibleVersion, InvalidPlacementPolicy,
-    ServiceError, ConstraintError, UnknownConstraintError)
+    ServiceError, ConstraintError, UnknownConstraintError, PreflightError)
 
 from juju.lib.testing import TestCase
 
@@ -62,6 +62,11 @@
         self.assertTrue(isinstance(error, ConstraintError))
         self.assertEquals(str(error), "Unknown constraint: 'meatball'")
 
+    def test_PreflightError(self):
+        error = PreflightError("cloudy")
+        self.assertTrue(isinstance(error, ConstraintError))
+        self.assertEquals(str(error), "Constraints preflight failed: cloudy")
+
     def test_ProviderError(self):
         error = ProviderError("Invalid credentials")
         self.assertIsJujuError(error)