Merge lp:~bcsaller/pyjuju/config-set into lp:pyjuju

[Benjamin Saller]

This is `ensemble set` minus the hook transition invocation. I will follow with that in another branch but want to get this part reviewed sooner than later so we can move on.

[Kapil Thangavelu (hazmat)]

One note from the pair programming session on agent support for config-set, was that this subcommand shouldn't require a repository argument, it should instead fetch the service's formula and inspect that for its config.

Probably this should modify the formula publisher and formula state to capture the config schema into zk directly when dpeloying, else it will need to be parsed from provider storage.

[Benjamin Saller (bcsaller)]

> One note from the pair programming session on agent support for config-set,
> was that this subcommand shouldn't require a repository argument, it should
> instead fetch the service's formula and inspect that for its config.
>
> Probably this should modify the formula publisher and formula state to capture
> the config schema into zk directly when dpeloying, else it will need to be
> parsed from provider storage.

While there was some related code in the option handling around this that had to be removed the code was actually using the formula state object properly. I think this comment came from the bad explanation I gave while pairing, but didn't reflect the current state of the code at the time.

[Gustavo Niemeyer (niemeyer)]

Very nicely organized, thanks. Just a few details before we can merge this:

[1]

+ """Set Service Options.

s/Service/service/
s/Options/options/

[2]

+ Services provide various options which can be set using this
+ command.

Here is a slightly more comprehensive version:

  Service formulas may define dynamic options which may be tweaked
  at deployment time, or over the lifetime of the service. This
  command allows changing these settings.

[3]

The overall logic in the command looks good.

Some minor details:

+ formula_manager = FormulaStateManager(client)
+ formula = yield formula_manager.get_formula_by_service_name(service_name)
+
+ service_manager = ServiceStateManager(client)
+ service = yield service_manager.get_service_state(service_name)

The function get_formula_by_service_name is untested and is likely
not necessary. E.g.:

    formula = yield service.get_formula_state()

[4]

+ # Parse the options using the formula config.yaml metadata to
+ # validate
+ # This will raise (and must be caught) on error
+ # parse config options

This comment would benefit from some additional love.

[5]

+ except KeyError:
+ # its ok not to contain a config.yaml
+ # it means no options will be handled
+ pass

KeyError can mean lots of things in Python. It'd be nice if rather than
catching this exception the logic would check for the existence of the
file in advance. This way we'll know we're never ignoring a KeyError
which we didn't intend to.

[6]

+def parse_keyvalue_pairs(options):

This needs tests.

[7]

+ @classmethod
+ def load_file(cls, pathname, optional=False):
(...)
+ def load(self, data):

Why is 'load_file' a class method and 'load' a normal one?

[8]

+ if not data or not isinstance(data, dict):
+ raise ValueError("Expected YAML dict of options metadata")

We need a more formal definition and verification of the config.yaml schema,
similar to how we do it for the metadata file.

[9]

+ raise ValueError(
+ "Expected `options` top level key in options metadata")
(...)
+ raise KeyError(
+ "%s is not a valid configuration option." % (
+ option))

We should never raise KeyError or ValueError or any other standard Python
exceptions, or we'll have no way to catch the *application* error without
wondering if we're not catching something we didn't intend to by mistake.

[10]

+ # Setup the watch deferred callback after the user defined callback
+ # has returned successfully from the existence invocation.
+ callback_d = maybeDeferred(callback, bool(exists))
+ callback_d.addCallback(
+ lambda x: watch_d.addCallback(watcher) and x)

This will need some tweaks along the lines of the work Kapil has been
pushing forward. Please catch up with him about it.

review: Needs Fixing

[Benjamin Saller (bcsaller)]

On Mon, May 23, 2011 at 2:37 PM, Gustavo Niemeyer <email address hidden> wrote:
> Review: Needs Fixing
>
> Very nicely organized, thanks.  Just a few details before we can merge this:
>
>
> [1]
>
> +    """Set Service Options.
>
> s/Service/service/
> s/Options/options/
>
>
Done

> [2]
>
> +    Services provide various options which can be set using this
> +    command.
>
> Here is a slightly more comprehensive version:
>
>  Service formulas may define dynamic options which may be tweaked
>  at deployment time, or over the lifetime of the service.  This
>  command allows changing these settings.
>

Changed.

> [3]
>
> The overall logic in the command looks good.
>
> Some minor details:
>
> +    formula_manager = FormulaStateManager(client)
> +    formula = yield formula_manager.get_formula_by_service_name(service_name)
> +
> +    service_manager = ServiceStateManager(client)
> +    service = yield service_manager.get_service_state(service_name)
>
> The function get_formula_by_service_name is untested and is likely
> not necessary. E.g.:
>
>    formula = yield service.get_formula_state()
>

Changed and removed formula_by_service_name.

> [4]
>
> +    # Parse the options using the formula config.yaml metadata to
> +    # validate
> +    # This will raise (and must be caught) on error
> +    # parse config options
>
> This comment would benefit from some additional love.
Cleaned
>
> [5]
>
> +        except KeyError:
> +            # its ok not to contain a config.yaml
> +            # it means no options will be handled
> +            pass
>
> KeyError can mean lots of things in Python.  It'd be nice if rather than
> catching this exception the logic would check for the existence of the
> file in advance.  This way we'll know we're never ignoring a KeyError
> which we didn't intend to.

Changed to ServiceConfigError throughout.

>
> [6]
>
> +def parse_keyvalue_pairs(options):
>
> This needs tests.

It had coverage in terms of the cli test which exercised the arg
parsing but a specific test has been added as well.

>
> [7]
>
> +    @classmethod
> +    def load_file(cls, pathname, optional=False):
> (...)
> +    def load(self, data):
>
> Why is 'load_file' a class method and 'load' a normal one?
>

The thinking was that load_file was just a convenience method wrapping
the common usage of init and load calls but I agree the distinction
isn't very clear.

> [8]
>
> +        if not data or not isinstance(data, dict):
> +            raise ValueError("Expected YAML dict of options metadata")
>
> We need a more formal definition and verification of the config.yaml schema,
> similar to how we do it for the metadata file.

I included a proposed version of this. It out of the scope of the spec
we agreed to but does reflect some of our talks about possible
directions for this.

>
> [9]
>
> +            raise ValueError(
> +                "Expected `options` top level key in options metadata")
> (...)
> +                raise KeyError(
> +                    "%s is not a valid configuration option." % (
> +                        option))
>
> We should never raise KeyError or ValueError or any other standard Python
> exceptions, or we'll have no way to cat...

[Gustavo Niemeyer (niemeyer)]

Interactive interaction online, and +1!

<niemeyer> bcsaller: Oh, cool.. just a small note re. the review: [5] seems untouched
<bcsaller> checking
<niemeyer> bcsaller: Re. [6], thanks for the test. We really need tests specific to the feature, otherwise you remove the other method later and this logic goes untested
<niemeyer> bcsaller: It would be fine to move the logic from the other tests though
<niemeyer> bcsaller: and only test on the layer above what you haven't already tested for the layer below
<niemeyer> bcsaller: Having a ton of logic on a layer below which is only touched by tests for upper layers is the issue
<bcsaller> yeah, looks like maybe I did miss [5], I think I made changes related to [9] and thought I got it
<niemeyer> bcsaller: No worris
<niemeyer> es
<niemeyer> bcsaller: re [8], is there a reason why you didn't use our schema framework in the same way we do for metadata and configuration?
<bcsaller> niemeyer: yes, the formula author has to write these things, so either I did something simple or I had to write a yaml serialization of the schema denifnition
<niemeyer> bcsaller: I don't understand.. how's that different from the metadata file, or the environments configuration file?
<bcsaller> because the schema is coded in Python by us for the metadata file, for the config.yaml they are writing a validation spec of their own
<niemeyer> bcsaller: No, it's not..
<niemeyer> bcsaller: The schema is a generic piece of framework which you can use to parse plain dictionaries and values
<niemeyer> bcsaller: and is used both for the metadata and the environments file, which are also written by hand
<bcsaller> yes, but the desc of that is done in python
<niemeyer> bcsaller: Yes, and we can do the same for the config yaml, I believe
<niemeyer> ?
<bcsaller> I didn't want formula authors to have to write in python to use the schema system
<niemeyer> bcsaller: They don't have to..
<niemeyer> bcsaller: We're talking about validating a yaml file
<bcsaller> oh, you mean to validate that the config.yaml is correct
<niemeyer> bcsaller: Yes
<niemeyer> bcsaller: Point [8] in the review
<bcsaller> that's fine, yes, I implemented validation of option values
<niemeyer> bcsaller: Yes, and why isn't that written using our schema framework?
<bcsaller> I think I misunderstood in that case, oh well we get a new feature
<niemeyer> bcsaller: in declarative style.. that was the actual questoin
<bcsaller> validating the config.yaml (which isn't really done now) using the schema would be fine
<niemeyer> bcsaller: Yeah, that was point [8]
<niemeyer> bcsaller: You can do that in a follow up branch
<bcsaller> I have no objection to that, I thought you wanted to have them write schema to validate the options they allow for the service
<niemeyer> bcsaller: Nope
<niemeyer> bcsaller: We just have to validate the file format before bundling a formula
<niemeyer> bcsaller: Otherwise it will blow up when the formula is deployed only, which would be quite sad
<bcsaller> ok, I can add that tonight, it's a small change
<niemeyer> bcsaller: Should be indeed, no rush, and thanks!

review: Approve

[Benjamin Saller (bcsaller)]

This version includes both validation of config.yaml using lib.schema and subsequent runtime validation of values set by the user via simple validators (type and/or regex). As discussed these two features are being pushed at once to speed the approval process along.

[Gustavo Niemeyer (niemeyer)]

Idea looks good, implementation looks good, several small issues
in terms of polishing the branch for merging.

[11]

+description and an optional default value. Additionally type may be

s/type/'type'/

[12]

+ config = ConfigOptions()
+ content = None

The second line may be dropped.

[13]

+ "options": Dict(String(),
+ OneOf(
+ # regex type requires validator
+ KeyDict({"type": Constant("regex"),

Indentation should be revised here. Feel free to define an
auxiliary variable externally.

[14]

+ "validator": String(),

We need a new RegEx type in the schema to validate the regular
expression.

[15]

+ optional=["default", "description",]),

s/",]/"]/

Same on the bottom one.

[16]

+ @classmethod
+ def load_file(cls, pathname, optional=False):

Drop the @classmethod here. load() doesn't have it, and MetaData doesn't use it
either, so let's make the interface even. Also, please rename the functions to
conform to the same conventions established by MetaData: parse() and load(), the
former with a data _string_, the latter with a filename. Add parse_serialization_data
to handle the dict case. The behavior of these functions should also mimic MetaData:
load() raises FileNotFound, and so on.

These two classes are exactly alike. There's no reason for us to diverge and
then having to remind of which interface does what, which load accepts a dict,
etc.

[17]

+ def validate(self, options):
+ """Validate options using the file at `path`.

Which path?

[18]

+ This method will return a dict with options that have passed
+ validation. This means options not declared in the formula
+ metadata will trigger a ServiceConfigError exception.
+
+ This method includes defaults for options which have no values
+ supplied if such defaults are defined in the metadata.

Please reword:

This method validates all the provided options, and returns a new
dictionary with values properly typed and default values set for
missing keys. If a provided option is unknown or its value
fails validation, ServiceConfigError is raised.

[19]

+ "%s is not a valid configuration option." % (option))
(...)
+ "a normal Python regex pattern for %s" % (
+ option))
(...)
+ "See http://docs.python.org/library/re.html" % (
+ option))
(...)
+ raise ServiceConfigError("Invalid options specification: %s" % (
+ error))

This is a tuple: (option,)

This is an expression with redundant parenthesis: (option)

Drop the parenthesis in all of those cases.

[20]

+ validator = validation_kinds[kind]
+ if kind == "regex":
+ pattern = self._data[option].get("validator")

The interface for validators can deal with such distinctions so that
we don't have to special case them in the generic function. E.g.:

validator = validation_kinds[kind]
value, valid = validator(value, self._data[option])

[21]

+class ServiceConfigError(Exception):
+ """Indicates an issue related to service options."""
+ ...

review: Needs Fixing

[Benjamin Saller (bcsaller)]

On Wed, Jun 1, 2011 at 3:01 PM, Gustavo Niemeyer <email address hidden> wrote:
> Review: Needs Fixing
> Idea looks good, implementation looks good, several small issues
> in terms of polishing the branch for merging.
>
>
> [11]
>
> +description and an optional default value. Additionally type may be
>
> s/type/'type'/
>
>

Changed

> [12]
>
> +        config = ConfigOptions()
> +        content = None
>
> The second line may be dropped.
>
>

Fixed.

> [13]
>
>
> +    "options": Dict(String(),
> +                    OneOf(
> +    # regex type requires validator
> +    KeyDict({"type": Constant("regex"),
>
> Indentation should be revised here.  Feel free to define an
> auxiliary variable externally.
>

Done

>
> [14]
>
> +             "validator": String(),
>
> We need a new RegEx type in the schema to validate the regular
> expression.
>

Added with simple tests.

>
> [15]
>
> +            optional=["default", "description",]),
>
> s/",]/"]/
>
> Same on the bottom one.
>

Done

>
> [16]
>
> +    @classmethod
> +    def load_file(cls, pathname, optional=False):
>
> Drop the @classmethod here.  load() doesn't have it, and MetaData doesn't use it
> either, so let's make the interface even.  Also, please rename the functions to
> conform to the same conventions established by MetaData: parse() and load(), the
> former with a data _string_, the latter with a filename.  Add parse_serialization_data
> to handle the dict case.  The behavior of these functions should also mimic MetaData:
> load() raises FileNotFound, and so on.
>
> These two classes are exactly alike.  There's no reason for us to diverge and
> then having to remind of which interface does what, which load accepts a dict,
> etc.
>

While I made these changes its worth noting that they are not exactly
the same as a missing config.yaml means the service takes no options.
So, for example, file not found isn't considered an error. This is to
simplify formula development, all services take no options now and
should continue to run that way. Once `formulate` becomes standard we
might require this file with an empty options dict as that will be
generated for the author.

> [17]
>
> +    def validate(self, options):
> +        """Validate options using the file at `path`.
>
> Which path?

Corrected ;)
>
>
> [18]
>
> +        This method will return a dict with options that have passed
> +        validation. This means options not declared in the formula
> +        metadata will trigger a ServiceConfigError exception.
> +
> +        This method includes defaults for options which have no values
> +        supplied if such defaults are defined in the metadata.
>
> Please reword:
>
> This method validates all the provided options, and returns a new
> dictionary with values properly typed and default values set for
> missing keys.  If a provided option is unknown or its value
> fails validation, ServiceConfigError is raised.
>
Done

>
> [19]
>
> +                    "%s is not a valid configuration option." % (option))
> (...)
> +                        "a normal  Python regex pattern for %s" % (
> +                        option))
> (...)
> +                        "See http://docs.python.org/lib...

[Gustavo Niemeyer (niemeyer)]

This looks good. +1 considering the following points:

[16]

This point still has the same issues, as discussed online.

[24]

+ })
+
+
+
+
+
+class ConfigOptions(object):

Two spaces only please.

[25]

+ try:
+ value = int(value)
+ except ValueError:
+ pass
+ return value, isinstance(value, int)

This is validating twice. I suggest something like this:

try:
    return int(value), True
except ValueError:
    return value, False

Same for the other equivalent type validators.

[26]

+ return value, re.match(pattern, value)
+ return re.match(pat, str) is not None

Some dead code.

review: Approve

=== modified file 'Makefile'
--- Makefile	2011-05-27 15:17:10 +0000
+++ Makefile	2011-06-08 15:41:33 +0000
@@ -37,7 +37,7 @@
 	@test -n "$(modified)" && echo $(modified) | xargs pyflakes
 
 
-modified=$(shell bzr status -S -r branch::prev  |grep -P '^\s*M' | awk '{print $$2;}'| grep -P "test_.*\.py$$")
+modified=$(shell bzr status -S -r branch::prev  |grep -P '^\s*\+?[MN]' | awk '{print $$2;}'| grep -P "test_.*\.py$$")
 ptests:
 	@test -n "$(modified)" && echo $(modified) | xargs ./test
 
 
=== modified file 'docs/source/drafts/service-config.rst'
--- docs/source/drafts/service-config.rst	2011-05-05 09:41:11 +0000
+++ docs/source/drafts/service-config.rst	2011-06-08 15:41:33 +0000
@@ -81,7 +81,12 @@
 The specification of possible configuration values is intentionally
 minimal, but still evolving.  Currently the formula define a list of
 names which they react. Information includes a human readable
-description and an optional default value.
+description and an optional default value. Additionally `type` may be
+specified. All options have a default type of 'str' which means its
+value will only be treated as a text string. Other valid options are
+'int', 'float' and 'regex'. When 'regex' is used an addtional element
+must be provided, 'validator'. This must be a valid Python regex as
+specified at http://docs.python.org/lib/re.html
 
 The following `config.yaml` would be included in the top level
 directory of a formula and includes a list of option definitions::
@@ -96,6 +101,8 @@
         password:
             default: changeme
             description: Password to be used for the account specified by 'username'
+            type: regex
+            validator: '.{6,12}'
         username:
             default: admin
             description: The name of the initial account (given admin permissions).
 
=== modified file 'ensemble/control/__init__.py'
--- ensemble/control/__init__.py	2011-05-24 14:20:01 +0000
+++ ensemble/control/__init__.py	2011-06-08 15:41:33 +0000
@@ -9,6 +9,7 @@
 import add_relation
 import add_unit
 import bootstrap
+import config_set
 import debug_hook
 import debug_log
 import deploy
@@ -32,6 +33,7 @@
     add_relation,
     add_unit,
     bootstrap,
+    config_set,
     debug_log,
     debug_hook,
     deploy,
 
=== added file 'ensemble/control/config_set.py'
--- ensemble/control/config_set.py	1970-01-01 00:00:00 +0000
+++ ensemble/control/config_set.py	2011-06-08 15:41:33 +0000
@@ -0,0 +1,75 @@
+from twisted.internet.defer import inlineCallbacks
+
+from ensemble.environment.errors import EnvironmentsConfigError
+from ensemble.hooks.cli import parse_keyvalue_pairs
+from ensemble.state.service import ServiceStateManager
+
+
+def configure_subparser(subparsers):
+    sub_parser = subparsers.add_parser("set",
+                                       help=command.__doc__)
+
+    sub_parser.add_argument(
+        "--environment", "-e",
+        help="Environment to status.")
+
+    sub_parser.add_argument("service_name",
+                            help="The name of the service the options apply to.")
+    sub_parser.add_argument("service_options",
+                            nargs="+",
+                            help="""name=value for option to set""")
+
+    return sub_parser
+
+
+def command(options):
+    """Set service options.
+
+    Service formulas may define dynamic options which may be tweaked
+    at deployment time, or over the lifetime of the service.  This
+    command allows changing these settings.
+
+    $ ensemble set <service_name> option=value [option=value]
+
+    or
+
+    $ ensemble set <service_name> --filename local.yaml
+
+    """
+    environment = options.environments.get(options.environment)
+    if environment is None and options.environment:
+        raise EnvironmentsConfigError(
+            "Invalid environment %r" % options.environment)
+    elif environment is None:
+        environment = options.environments.get_default()
+
+    return config_set(environment,
+                      options.service_name,
+                      options.service_options)
+
+
+@inlineCallbacks
+def config_set(environment, service_name, service_options):
+    """Collect and render status information. """
+    provider = environment.get_machine_provider()
+    client = yield provider.connect()
+
+    # Get the service and the formula
+    #
+    service_manager = ServiceStateManager(client)
+    service = yield service_manager.get_service_state(service_name)
+    formula = yield service.get_formula_state()
+
+    # Use the formula's ConfigOptions instance to validate the
+    # arguments to config_set. Invalid options passed to this method
+    # will thrown an exception.
+    options = parse_keyvalue_pairs(service_options)
+
+    config = yield formula.get_config()
+    options = config.validate(options)
+
+    # Apply the change
+    state = yield service.get_config()
+    state.update(options)
+    yield state.write()
+
 
=== added file 'ensemble/control/tests/test_config_set.py'
--- ensemble/control/tests/test_config_set.py	1970-01-01 00:00:00 +0000
+++ ensemble/control/tests/test_config_set.py	2011-06-08 15:41:33 +0000
@@ -0,0 +1,89 @@
+from twisted.internet.defer import inlineCallbacks
+
+from ensemble.control import main
+from ensemble.control.config_set import config_set
+from ensemble.control.tests.test_status import StatusTestBase
+from ensemble.formula.tests.test_metadata import test_repository_path
+
+class ControlEnsembleSetTest(StatusTestBase):
+
+    @inlineCallbacks
+    def setUp(self):
+        yield super(ControlEnsembleSetTest, self).setUp()
+        self.output = self.capture_logging()
+        self.stderr = self.capture_stream("stderr")
+
+    @inlineCallbacks
+    def tearDown(self):
+        yield super(ControlEnsembleSetTest, self).tearDown()
+        self.reset_logging()
+
+    @inlineCallbacks
+    def test_set_and_get(self):
+        system = yield self.build_topology()
+
+        self.mock_environment()
+        self.mocker.replay()
+
+        yield config_set(self.environment,
+                         "wordpress",
+                         ["blog-title=That\'ll do, pig."])
+
+
+        # Verify the state is accessible
+        wordpress = system["services"]["wordpress"]
+        state = yield wordpress.get_config()
+        self.assertEqual(state, {"blog-title": "That\'ll do, pig."})
+
+
+    @inlineCallbacks
+    def test_set_invalid_option(self):
+        yield self.build_topology()
+        self.mock_environment()
+        finished = self.setup_cli_reactor()
+        self.setup_exit(0)
+        self.mocker.replay()
+
+        main(["set",
+              "wordpress",
+              "blog-roll=What's a blog-roll?"])
+        yield finished
+
+        # Make sure we got an error message to the user
+        self.assertIn("blog-roll is not a valid configuration option.",
+                      self.stderr.getvalue())
+
+    @inlineCallbacks
+    def test_set_invalid_service(self):
+        yield self.build_topology()
+        self.mock_environment()
+        finished = self.setup_cli_reactor()
+        self.setup_exit(0)
+        self.mocker.replay()
+
+        main(["set",
+              "whatever",
+              "blog-roll=What's a blog-roll?"])
+        yield finished
+
+        self.assertIn("Service 'whatever' was not found",
+                      self.stderr.getvalue())
+
+
+    @inlineCallbacks
+    def test_set_valid_option(self):
+        system = yield self.build_topology()
+        self.mock_environment()
+        finished = self.setup_cli_reactor()
+        self.setup_exit(0)
+        self.mocker.replay()
+
+        main(["set",
+              "wordpress",
+              'blog-title=My title'])
+        yield finished
+
+        # Verify the state is accessible
+        wordpress = system["services"]["wordpress"]
+        state = yield wordpress.get_config()
+        self.assertEqual(state, {"blog-title": "My title"})
 
=== modified file 'ensemble/formula/bundle.py'
--- ensemble/formula/bundle.py	2011-01-19 18:20:47 +0000
+++ ensemble/formula/bundle.py	2011-06-08 15:41:33 +0000
@@ -5,6 +5,7 @@
 from zipfile import ZipFile, BadZipfile
 
 from ensemble.formula.base import FormulaBase
+from ensemble.formula.config import ConfigOptions
 from ensemble.formula.metadata import MetaData
 from ensemble.errors import FormulaError
 from ensemble.lib.filehash import compute_file_hash
@@ -20,15 +21,21 @@
             raise FormulaError(path, "Must be a ZIP-file (%s)." % str(exc))
 
         metadata = MetaData()
-        try:
-            content = zf.read("metadata.yaml")
-        except KeyError:
+        if "metadata.yaml" not in zf.namelist():
             raise FormulaError(path,
                                "Archive does not contain required file: "
                                "\"metadata.yaml\".")
 
+        content = zf.read("metadata.yaml")
         metadata.parse(content)
         self.metadata = metadata
+
+        config = ConfigOptions()
+        if "config.yaml" in zf.namelist():
+            content = zf.read("config.yaml")
+            config.parse(content)
+        self.config = config
+
         self.path = isinstance(path, file) and path.name or path
 
     def compute_sha256(self):
 
=== added file 'ensemble/formula/config.py'
--- ensemble/formula/config.py	1970-01-01 00:00:00 +0000
+++ ensemble/formula/config.py	2011-06-08 15:41:33 +0000
@@ -0,0 +1,177 @@
+import os
+import re
+
+import yaml
+
+from ensemble.lib.schema import (SchemaError, KeyDict, Dict, String,
+                                 Constant, OneOf, Int, Float)
+from ensemble.formula.errors import ServiceConfigError
+
+
+# regex type requires validator
+REGEX_TYPE_SCHEMA = KeyDict({"type": Constant("regex"),
+                        "validator": String(),
+                        "default": OneOf(String(), Int(), Float()),
+                        "description": String(),
+                        },
+                       optional=["default", "description"])
+
+SIMPLE_TYPE_SCHEMA =  KeyDict({"type": OneOf(Constant("str"),
+                                             Constant("int"),
+                                             Constant("float")),
+                               "default": OneOf(String(), Int(), Float()),
+                               "description": String(),
+                               },
+                              optional=["default", "description"])
+
+# Schema used to validate ConfigOptions specifications
+CONFIG_SCHEMA = KeyDict({
+    "options": Dict(
+                     String(),
+                     OneOf(REGEX_TYPE_SCHEMA, SIMPLE_TYPE_SCHEMA))
+    })
+
+
+
+
+
+class ConfigOptions(object):
+    """Represents the configuration options exposed by a formula.
+
+    The intended usage is that Forumla provide access to these objects
+    and then use them to `validate` inputs provided in the `ensemble
+    set` and `ensemble deploy` code paths.
+    """
+
+    def __init__(self):
+        self._data = {}
+
+    def load(self, pathname, optional=False):
+        """Construct a ConfigOptions instance from a YAML file."""
+        data = None
+        if os.path.exists(pathname):
+            data = open(pathname, "r").read()
+
+        if not data and optional:
+            data = "options: {}\n"
+
+        if not data:
+            raise ServiceConfigError(
+                "Missing required service options metadata: %s" % (
+                pathname))
+
+        self.parse(data)
+        return self
+
+    def parse(self, data):
+        """Load data into the config object.
+
+        Data can be a properly encoded YAML string or an dict, such as
+        one returned by `get_serialization_data`.
+
+        Each call to `load` replaces any existing data.
+        """
+        if isinstance(data, basestring):
+            raw_data = yaml.load(data)
+        elif isinstance(data, dict):
+            raw_data = data
+        else:
+            raise TypeError("Unknown data type for `load`: %s" % type(data))
+
+        data = self._parse(raw_data)
+        self._data = data
+
+    def _parse(self, data):
+        """Verify we have sensible option metadata.
+
+        Returns the `options` dict from within the YAML data.
+        """
+        if not data or not isinstance(data, dict):
+            raise ServiceConfigError("Expected YAML dict of options metadata")
+
+        try:
+            data = CONFIG_SCHEMA.coerce(data, [])
+        except SchemaError, error:
+            raise ServiceConfigError("Invalid options specification: %s" % (
+                error))
+
+        return data["options"]
+
+    def get_defaults(self):
+        """Return a mapping of option: default for all options."""
+        d = {}
+        for name, options in self._data.items():
+            if "default" in options:
+                d[name] = options["default"]
+
+        return d
+
+    def validate(self, options):
+        """Validate options using the loaded validation data.
+
+        This method validates all the provided options, and returns a
+        new dictionary with values properly typed and default values
+        set for missing keys.  If a provided option is unknown or its
+        value fails validation, ServiceConfigError is raised.
+
+        This method includes defaults for options which have no values
+        supplied if such defaults are defined in the metadata.
+        """
+        d = self.get_defaults()
+
+        for option, value in options.items():
+            if option not in self._data:
+                raise ServiceConfigError(
+                    "%s is not a valid configuration option." % (option))
+
+            # see if there is a type associated with the option
+            kind = self._data[option].get("type", "str")
+            if kind not in validation_kinds:
+                raise ServiceConfigError(
+                    "Unknown service option type: %s" % kind)
+
+            # apply validation
+            validator = validation_kinds[kind]
+            value, valid = validator(value, self._data[option])
+            if not valid:
+                raise ServiceConfigError(
+                    "Invalid value for %s: %s" % (option, value))
+
+            d[option] = value
+
+        return d
+
+    def get_serialization_data(self):
+        return dict(options=self._data.copy())
+
+
+# Validators return (type mapped value, valid boolean)
+def validate_str(value, options):
+    return str(value), True
+
+def validate_int(value, options):
+    try:
+        value = int(value)
+    except ValueError:
+        pass
+    return value, isinstance(value, int)
+
+def validate_float(value, options):
+    try:
+        value = float(value)
+    except ValueError:
+        pass
+    return value, isinstance(value, float)
+
+def validate_regex(value, options):
+    pattern = options["validator"]
+    return value, re.match(pattern, value)
+    return re.match(pat, str) is not None
+
+# maps service option types to callables
+validation_kinds = {
+    "str": validate_str,
+    "int": validate_int,
+    "float": validate_float,
+    "regex": validate_regex
+    }
 
=== modified file 'ensemble/formula/directory.py'
--- ensemble/formula/directory.py	2011-01-19 17:00:12 +0000
+++ ensemble/formula/directory.py	2011-06-08 15:41:33 +0000
@@ -2,6 +2,7 @@
 import zipfile
 import tempfile
 
+from ensemble.formula.config import ConfigOptions
 from ensemble.formula.metadata import MetaData
 from ensemble.formula.bundle import FormulaBundle
 from ensemble.formula.base import FormulaBase
@@ -21,6 +22,8 @@
     def __init__(self, path):
         self.path = path
         self.metadata = MetaData(os.path.join(path, "metadata.yaml"))
+        self.config = ConfigOptions()
+        self.config.load(os.path.join(path, "config.yaml"), optional=True)
         self._temp_bundle = None
         self._temp_bundle_file = None
 
 
=== modified file 'ensemble/formula/errors.py'
--- ensemble/formula/errors.py	2011-04-08 05:45:41 +0000
+++ ensemble/formula/errors.py	2011-06-08 15:41:33 +0000
@@ -46,3 +46,8 @@
 
     def __str__(self):
         return "Formula %r is the latest revision known"
+
+
+class ServiceConfigError(Exception):
+    """Indicates an issue related to service options."""
+
 
=== added file 'ensemble/formula/tests/repository/dummy/config.yaml'
--- ensemble/formula/tests/repository/dummy/config.yaml	1970-01-01 00:00:00 +0000
+++ ensemble/formula/tests/repository/dummy/config.yaml	2011-06-08 15:41:33 +0000
@@ -0,0 +1,5 @@
+options:
+  title: {default: My Title, description: A descriptive title used for the service., type: str}
+  outlook: {description: No default outlook., type: str}
+  username: {default: admin, description: The name of the initial account (given admin permissions)., type: regex, validator: '\w{8}'}
+  skill-level: {description: A number indicating skill., type: int}
 
=== added file 'ensemble/formula/tests/repository/wordpress/config.yaml'
--- ensemble/formula/tests/repository/wordpress/config.yaml	1970-01-01 00:00:00 +0000
+++ ensemble/formula/tests/repository/wordpress/config.yaml	2011-06-08 15:41:33 +0000
@@ -0,0 +1,3 @@
+options:
+  blog-title: {default: My Title, description: A descriptive title used for the blog., type: str}
+
 
=== modified file 'ensemble/formula/tests/test_bundle.py'
--- ensemble/formula/tests/test_bundle.py	2011-01-24 17:15:28 +0000
+++ ensemble/formula/tests/test_bundle.py	2011-06-08 15:41:33 +0000
@@ -59,6 +59,13 @@
 
         self.fail("Expected formula error.")
 
+    def test_bundled_config(self):
+        """Make sure that config is accessible from a bundle."""
+        from ensemble.formula.tests.test_config import sample_yaml_data
+        bundle = FormulaBundle(self.filename)
+        self.assertEquals(bundle.config.get_serialization_data(),
+                          sample_yaml_data)
+
     def test_info(self):
         bundle = FormulaBundle(self.filename)
         self.assertTrue(bundle.metadata is not None)
 
=== added file 'ensemble/formula/tests/test_config.py'
--- ensemble/formula/tests/test_config.py	1970-01-01 00:00:00 +0000
+++ ensemble/formula/tests/test_config.py	2011-06-08 15:41:33 +0000
@@ -0,0 +1,141 @@
+# -*- encoding: utf-8 -*-
+import yaml
+
+from ensemble.lib.testing import TestCase
+from ensemble.formula.config import ConfigOptions
+from ensemble.formula.errors import ServiceConfigError
+
+sample_configuration = """
+options:
+  title: {default: My Title, description: A descriptive title used for the service., type: str}
+  outlook: {description: No default outlook., type: str}
+  username: {default: admin, description: The name of the initial account (given admin permissions)., type: regex, validator: '\w{8}'}
+  skill-level: {description: A number indicating skill., type: int}
+"""
+
+sample_yaml_data = yaml.load(sample_configuration)
+
+sample_config_defaults = {
+    "title": "My Title",
+    "username": "admin"
+    }
+
+class ConfigOptionsTest(TestCase):
+
+    def setUp(self):
+        self.config = ConfigOptions()
+
+    def test_load(self):
+        """Validate we can load data or get expected errors."""
+
+        # load valid data
+        filename = self.makeFile(sample_configuration)
+        self.config.load(filename)
+        self.assertEqual(self.config.get_serialization_data(),
+                         sample_yaml_data)
+
+        # and with bad data expected exceptions
+        self.assertRaises(ServiceConfigError,
+                          self.config.load, "foo: [1, 2")
+
+        # test with dict based data
+        self.config.parse(sample_yaml_data)
+        self.assertEqual(self.config.get_serialization_data(),
+                         sample_yaml_data)
+
+
+        # and with an unhandled type
+        self.assertRaises(TypeError, self.config.load, 1.234)
+
+    def test_load_file(self):
+        sample_path = self.makeFile(sample_configuration)
+        config = ConfigOptions()
+        config.load(sample_path)
+
+        self.assertEqual(config.get_serialization_data(),
+                         sample_yaml_data)
+
+        # and an expected exception
+        error = self.assertRaises(ServiceConfigError, config.load, "missing_file")
+        self.assertEqual(error.message,
+                         "Missing required service options metadata: missing_file")
+
+        # but the optional flag handles missing files
+        config = config.load("missing_file", optional=True)
+
+
+    def test_defaults(self):
+        self.config.parse(sample_configuration)
+        defaults = self.config.get_defaults()
+        self.assertEqual(defaults, sample_config_defaults)
+
+    def test_parse(self):
+        """Verify that _parse checks and raises."""
+        # no options dict
+        self.assertRaises(ServiceConfigError, self.config.parse, {"foo": "bar"})
+
+
+    def test_validate(self):
+        sample_input = {
+            "title": "Helpful Title",
+            "outlook": "Peachy",
+            }
+
+        self.config.parse(sample_configuration)
+        data = self.config.validate(sample_input)
+
+        # This should include an overridden value, a default and a new
+        # value.
+        self.assertEqual(data,
+                         {"username": "admin",
+                          "outlook": "Peachy",
+                          "title": "Helpful Title"})
+
+        # now try to set a value outside the expected
+        sample_input["bad"] = "value"
+        error = self.assertRaises(ServiceConfigError, self.config.validate, sample_input)
+        self.assertEqual(error.message,
+                         "bad is not a valid configuration option.")
+
+        # validating with an empty instance
+        # the service takes no options
+        config = ConfigOptions()
+        self.assertRaises(ServiceConfigError, config.validate, sample_input)
+
+    def test_validate_regex(self):
+        self.config.parse(sample_configuration)
+
+        error = self.assertRaises(ServiceConfigError,
+                                  self.config.validate, dict(username="1234"))
+        self.assertIn(str(error), "Invalid value for username: 1234")
+
+
+        data = self.config.validate(dict(username="12345678"))
+        self.assertEqual(data, {"username": "12345678", "title": "My Title"})
+
+
+    def test_validate_regex_wo_validator(self):
+        """Assure that validator is required for type=regex."""
+        local_configuration = sample_configuration + \
+          """  fails: {description: missing required., type:  regex}"""
+        error = self.assertRaises(ServiceConfigError,
+                          self.config.parse,
+                          local_configuration)
+        self.assertIn("options.fails.validator: required value not found",
+                      str(error))
+
+
+    def test_validate_types(self):
+
+        self.config.parse(sample_configuration)
+
+        error = self.assertRaises(ServiceConfigError,
+                                  self.config.validate, {"skill-level": "NaN"})
+        self.assertIn(str(error), "Invalid value for skill-level: NaN")
+
+
+        data = self.config.validate({"skill-level": "9001"})
+        # its over 9000!
+        self.assertEqual(data, {"skill-level": 9001,
+                                "title": "My Title",
+                                "username": "admin"})
 
=== modified file 'ensemble/formula/tests/test_directory.py'
--- ensemble/formula/tests/test_directory.py	2011-03-21 17:42:43 +0000
+++ ensemble/formula/tests/test_directory.py	2011-06-08 15:41:33 +0000
@@ -56,7 +56,7 @@
         included = [info.filename for info in zf.infolist()]
         self.assertEqual(
             set(included),
-            set(("metadata.yaml", "empty/", "src/", "src/hello.c")))
+            set(("metadata.yaml", "empty/", "src/", "src/hello.c", "config.yaml")))
 
     def test_as_bundle(self):
         directory = FormulaDirectory(self.sample_dir1)
@@ -123,3 +123,11 @@
     def test_as_directory(self):
         directory = FormulaDirectory(self.sample_dir1)
         self.assertIs(directory.as_directory(), directory)
+
+    def test_config(self):
+        """Validate that ConfigOptions are available on the formula"""
+        from ensemble.formula.tests.test_config import sample_yaml_data
+        directory = FormulaDirectory(sample_directory)
+        self.assertEquals(directory.config.get_serialization_data(),
+                          sample_yaml_data)
+
 
=== modified file 'ensemble/hooks/cli.py'
--- ensemble/hooks/cli.py	2011-05-03 08:54:13 +0000
+++ ensemble/hooks/cli.py	2011-06-08 15:41:33 +0000
@@ -141,7 +141,7 @@
             raise exit
 
         if self.keyvalue_pairs:
-            self.parse_kvpairs()
+            self.parse_kvpairs(self.options.keyvalue_pairs)
 
         return options
 
@@ -151,23 +151,9 @@
             level=self.options.log_level,
             stream=self.options.log_file)
 
-    def parse_kvpairs(self):
-        data = {}
-        for kv in self.options.keyvalue_pairs:
-            k, v = kv.split("=", 1)
-            if v.startswith("@"):
-                # handle fileinput per spec
-                # XXX: todo -- sanitize input
-                filename = v[1:]
-                try:
-                    v = open(filename, "r").read()
-                except IOError:
-                    raise EnsembleError(
-                        "No such file or directory: %s (argument:%s)" % (
-                            filename,
-                            k))
-            data[k] = v
-
+    def parse_kvpairs(self, options):
+        data = parse_keyvalue_pairs(options)
+        # cache
         self.options.keyvalue_pairs = data
         return data
 
@@ -255,3 +241,23 @@
         logging.error("Invalid log level %s" % level)
         level = logging.INFO
     return level
+
+
+def parse_keyvalue_pairs(options):
+    data = {}
+    for kv in options:
+        k, v = kv.split("=", 1)
+        if v.startswith("@"):
+            # handle fileinput per spec
+            # XXX: todo -- sanitize input
+            filename = v[1:]
+            try:
+                v = open(filename, "r").read()
+            except IOError:
+                raise EnsembleError(
+                    "No such file or directory: %s (argument:%s)" % (
+                        filename,
+                        k))
+        data[k] = v
+
+    return data
 
=== modified file 'ensemble/hooks/tests/hooks/sleep-hook' (properties changed: +x to -x)
=== modified file 'ensemble/hooks/tests/test_cli.py'
--- ensemble/hooks/tests/test_cli.py	2011-05-03 08:54:13 +0000
+++ ensemble/hooks/tests/test_cli.py	2011-06-08 15:41:33 +0000
@@ -4,7 +4,9 @@
 
 from twisted.internet.defer import inlineCallbacks, returnValue
 
-from ensemble.hooks.cli import CommandLineClient, parse_log_level
+from ensemble.errors import EnsembleError
+from ensemble.hooks.cli import (CommandLineClient, parse_log_level,
+                                parse_keyvalue_pairs)
 from ensemble.lib.testing import TestCase
 
 
@@ -321,3 +323,21 @@
         self.assertEquals(parse_log_level("ERROR"), logging.ERROR)
         self.assertEquals(parse_log_level(logging.INFO), logging.INFO)
         self.assertEquals(parse_log_level(logging.ERROR), logging.ERROR)
+
+
+    def test_parse_keyvalue_pairs(self):
+        sample = self.makeFile("INPUT DATA")
+
+        # test various styles of options being read
+        options = ["alpha=beta",
+                   "content=@%s" % sample]
+
+        data = parse_keyvalue_pairs(options)
+        self.assertEquals(data["alpha"], "beta")
+        self.assertEquals(data["content"], "INPUT DATA")
+
+        # and check an error condition
+        options = ["content=@missing"]
+        error = self.assertRaises(EnsembleError, parse_keyvalue_pairs, options)
+        self.assertIn("No such file", str(error))
+
 
=== modified file 'ensemble/lib/schema.py'
--- ensemble/lib/schema.py	2010-10-28 20:18:36 +0000
+++ ensemble/lib/schema.py	2011-06-08 15:41:33 +0000
@@ -1,4 +1,5 @@
 """A schema system for validation of dict-based values."""
+import re
 
 
 class SchemaError(Exception):
@@ -110,6 +111,17 @@
             raise SchemaExpectationError(path, "unicode", repr(value))
         return value
 
+class Regex(object):
+    """Something that must be a valid Python regular expression."""
+
+    def coerce(self, value, path):
+        try:
+            regex = re.compile(value)
+        except re.error:
+            raise SchemaExpectationError(path,
+                                         "regex",
+                                         repr(value))
+        return regex
 
 class UnicodeOrString(object):
     """Something that must be a C{unicode} or {str}.
 
=== modified file 'ensemble/lib/tests/test_schema.py'
--- ensemble/lib/tests/test_schema.py	2010-11-03 21:12:16 +0000
+++ ensemble/lib/tests/test_schema.py	2011-06-08 15:41:33 +0000
@@ -1,9 +1,11 @@
+import re
+
 from ensemble.lib.testing import TestCase
 
 from ensemble.lib.schema import (
     SchemaError, SchemaExpectationError, Constant, Bool,
     Int, Float, String, Unicode, UnicodeOrString, List, KeyDict,
-    Dict, Tuple, OneOf, Any)
+    Dict, Tuple, OneOf, Any, Regex)
 
 
 PATH = ["<pa", "th>"]
@@ -161,6 +163,17 @@
                           r"got '\xff'")
 
 
+    def test_regex(self):
+        exp = "\w+"
+        pat = re.compile(exp)
+        self.assertEquals(Regex().coerce(exp, PATH), pat)
+
+    def test_regex_bad_regex(self):
+        exp = "([a-"
+        error = self.assertRaises(
+            SchemaError, Regex().coerce,  exp, PATH)
+        self.assertIn("expected regex", str(error))
+
 
     def test_list(self):
         schema = List(Int())
 
=== modified file 'ensemble/state/formula.py'
--- ensemble/state/formula.py	2010-10-29 19:55:50 +0000
+++ ensemble/state/formula.py	2011-06-08 15:41:33 +0000
@@ -5,16 +5,15 @@
 
 from zookeeper import NoNodeException
 
+from ensemble.formula.config import ConfigOptions
 from ensemble.formula.metadata import MetaData
+from ensemble.state.base import StateBase
 from ensemble.state.errors import FormulaStateNotFound
 
 
-class FormulaStateManager(object):
+class FormulaStateManager(StateBase):
     """Manages the state of formulas in an environment."""
 
-    def __init__(self, client):
-        self._client = client
-
     @inlineCallbacks
     def add_formula_state(self, namespace, formula):
         """Register metadata about the provided Formula.
@@ -26,6 +25,7 @@
         revision = formula.metadata.revision
 
         formula_data = {
+            "config": formula.config.get_serialization_data(),
             "metadata": formula.metadata.get_serialization_data(),
             "sha256": formula.get_sha256(),
         }
@@ -71,6 +71,9 @@
         self._metadata = MetaData()
         self._metadata.parse_serialization_data(formula_data["metadata"])
 
+        self._config = ConfigOptions()
+        self._config.parse(formula_data["config"])
+
         # Just a health check:
         assert self._metadata.name == name
         assert self._metadata.revision == revision
@@ -104,6 +107,11 @@
         """Return deferred MetaData."""
         return succeed(self._metadata)
 
+    def get_config(self):
+        """Return deferred ConfigOptions."""
+        return succeed(self._config)
+
     def get_sha256(self):
         """Return deferred sha256 for the formula."""
         return succeed(self._sha256)
+
 
=== modified file 'ensemble/state/service.py'
--- ensemble/state/service.py	2011-05-26 19:56:11 +0000
+++ ensemble/state/service.py	2011-06-08 15:41:33 +0000
@@ -271,6 +271,10 @@
         return "/services/" + self._internal_id
 
     @property
+    def _config_path(self):
+        return "%s/config" % self._zk_path
+
+    @property
     def _exposed_path(self):
         """Path of ZK node that if it exists, indicates service is exposed."""
         return "/services/%s/exposed" % self._internal_id
@@ -467,6 +471,44 @@
 
         return self._watch_topology(watch_topology)
 
+    @inlineCallbacks
+    def watch_config_state(self, callback):
+        """Observe changes to config state for a service.
+
+        @param callback: A function/method which accepts the YAMLState
+        node of the changed service. No effort is made to present
+        deltas to the change function.
+
+        Note there are no guarantees that this function will be called
+        once for *every* change in the topology, which means that multiple
+        modifications may be observed as a single call.
+
+        This method currently sets a pretty much perpetual watch (errors
+        will make it bail out).  In order to cleanly stop the watcher, a
+        StopWatch exception can be raised by the callback.
+        """
+        @inlineCallbacks
+        def watcher(change_event):
+            if self._client.connected:
+                exists_d, watch_d = self._client.exists_and_watch(
+                    self._config_path)
+            yield callback(change_event)
+            watch_d.addCallback(watcher)
+
+        exists_d, watch_d = self._client.exists_and_watch(self._config_path)
+
+        exists = yield exists_d
+
+        # Setup the watch deferred callback after the user defined callback
+        # has returned successfully from the existence invocation.
+        callback_d = maybeDeferred(callback, bool(exists))
+        callback_d.addCallback(
+            lambda x: watch_d.addCallback(watcher) and x)
+
+        # Wait on the first callback, reflecting present state, not a zk watch
+        yield callback_d
+
+
     def watch_service_unit_states(self, callback):
         """Observe changes in service unit membership for this service.
 
 
=== modified file 'ensemble/state/tests/test_formula.py'
--- ensemble/state/tests/test_formula.py	2010-12-17 20:46:18 +0000
+++ ensemble/state/tests/test_formula.py	2011-06-08 15:41:33 +0000
@@ -36,6 +36,7 @@
         formula_data = yaml.load(content)
         self.assertEquals(formula_data, {
             "metadata": self.formula.metadata.get_serialization_data(),
+            "config": self.formula.config.get_serialization_data(),
             "sha256": self.formula.get_sha256(),
         })
 
@@ -80,6 +81,21 @@
         self.assertEquals(metadata.revision, 1)
 
     @inlineCallbacks
+    def test_formula_state_config_options(self):
+        """Verify ConfigOptions present and correct."""
+        from ensemble.formula.tests.test_config import sample_yaml_data
+
+        yield self.formula_state_manager.add_formula_state(
+            "namespace", self.formula)
+        formula_state = yield self.formula_state_manager.get_formula_state(
+            "namespace:dummy-1")
+        config = yield formula_state.get_config()
+        self.assertEquals(config.get_serialization_data(),
+                          sample_yaml_data)
+
+
+
+    @inlineCallbacks
     def test_get_non_existing_formula_prior_to_initialization(self):
         """
         Getting a formula before the formulas node was even
 
=== modified file 'ensemble/state/tests/test_service.py'
--- ensemble/state/tests/test_service.py	2011-05-26 19:56:11 +0000
+++ ensemble/state/tests/test_service.py	2011-06-08 15:41:33 +0000
@@ -2282,6 +2282,34 @@
         self.assertEqual(metadata.summary, "Blog engine")
 
     @inlineCallbacks
+    def test_watch_config_options(self):
+        """Verify callback trigger on config options modification"""
+
+        service_state = yield self.service_state_manager.add_service_state(
+            "wordpress", self.formula_state)
+        results = []
+
+        def callback(value):
+            results.append(value)
+
+        yield service_state.watch_config_state(callback)
+        config = yield service_state.get_config()
+        config["alpha"] = "beta"
+        yield config.write()
+
+        yield self.poke_zk()
+        self.assertIdentical(results.pop(0), True)
+        self.assertIdentical(results.pop(0).type_name, "changed")
+
+        # and changing it again should trigger the callback again
+        config["gamma"] = "delta"
+        yield config.write()
+
+        yield self.poke_zk()
+        self.assertEqual(len(results), 1)
+        self.assertIdentical(results.pop(0).type_name, "changed")
+
+    @inlineCallbacks
     def test_get_open_ports(self):
         """Verify introspection and that the ports changes are immediate."""
         service_state = yield self.add_service("wordpress")
@@ -2478,3 +2506,4 @@
         self.assertEqual(results[-1].type_name, "changed")
         self.assertEqual(contents[-1], [{'port': 53, 'proto': 'udp'}, {'port': 443, 'proto': 'tcp'}])
         yield self.poke_zk()
+