Merge lp:~fwereade/pyjuju/fix-docstrings into lp:pyjuju

[William Reade]

This adds (1) automatic doc generation and (2) hopefully-consistent docstrings in ensemble.providers; so, it doesn't really the currently-linked bug as it currently stands. I was thinking I might rewrite that one, and open a series of others for other packages, but that doesn't feel like quite the right approach, so I'm open to suggestions.

[Gustavo Niemeyer (niemeyer)]

As mentioned on IRC:

<niemeyer> fwereade: Thanks for the docstring changes
<niemeyer> fwereade: Very nice to have it being standardized
<niemeyer> fwereade: I'm slightly concerned it might be going overboard in terms of syntax in some cases.. not sure if the benefit of the linkage is worth the reduced readability in the code itself
<niemeyer> fwereade: Simple example:
<niemeyer> - @param instance_id: instance_id of the `ProviderMachine` you want.
<niemeyer> vs.
<niemeyer> 316 + :param str instance_id: :attr:`instance_id` of the
<niemeyer> 317 + :class:`ensemble.machine.ProviderMachine` you want.

Either way, I'm happy to experiment with this. +1!

review: Approve

[William Reade (fwereade)]

> <niemeyer> fwereade: I'm slightly concerned it might be going overboard in
> terms of syntax in some cases.. not sure if the benefit of the linkage is
> worth the reduced readability in the code itself

IMO, if you're reading the docs, you're trying to figure things out at a high level, and the linkage is beneficial (what exactly is the distinction between a MachineProvider and a ProviderMachine, again?); if you're reading the code, you're reading the *code*, and your attitude towards docstrings and comments alike is one of bitter cynicism (on a good day, you might get around to fixing them...).

> Either way, I'm happy to experiment with this. +1!

Cool -- we'll see how it goes :).

[lp:~fwereade/pyjuju/fix-docstrings]

344. By William Reade on 2011-09-06

merge parent

[Jim Baker (jimbaker)]

+1, overall this looks very good. It would seem possible to cross-reference imported names without being explicit as to their path (eg :class:`ProviderMachine` instead of :class:`ensemble.machine.ProviderMachine`) as long as that was unambiguous, but a quick experiment shows that Sphinx as configured right now won't do that.

Minor nits: when using singlehtml, links to source do not work. Also, the headings in that view really don't work well with the size of the left column. Maybe in general we would want to collapse the headings to the second level of the package namespace, under ensemble, that is just ensemble.agents instead of also seeing ensemble.agents.base, ensemble.agents.dummy, etc.

review: Approve

[lp:~fwereade/pyjuju/fix-docstrings]

345. By William Reade on 2011-09-13

merge trunk

346. By William Reade on 2011-09-13

singlehtml builds eschew source links; module documentation titles are now abbreviated if > 2 packages deep

[William Reade (fwereade)]

> Minor nits: when using singlehtml, links to source do not work.

AFAICT, that's a Sphinx issue, sadly. I've (somewhat nastily...) disabled the viewcode extension for singlehtml builds.

> Also, the
> headings in that view really don't work well with the size of the left column.
> Maybe in general we would want to collapse the headings to the second level of
> the package namespace, under ensemble, that is just ensemble.agents instead of
> also seeing ensemble.agents.base, ensemble.agents.dummy, etc.

IMO batching all docs at the second level is too much -- you certainly don't want all of ensemble.providers in a single file (do you? maybe I'm missing context). I strongly favour batching them up in exactly the same way we do the source code -- by choosing module boundaries, we've implicitly chosen a "sensible" chunking for the concepts in play, and if there's a problem with those divisions we should be rearranging the code rather than the documentation. IMO ;).

Given the undeniable ugliness, though, module titles are now abbreviated as follows (approximately inspired by twisted's documentation):

ensemble -> ensemble
ensemble.providers -> ensemble.providers
ensemble.providers.orchestra -> e.providers.orchestra
ensemble.providers.orchestra.cobbler -> e.p.orchestra.cobbler

I think it's a reasonable tradeoff: you're unlikely to get lost with two unabbreviated names' worth of context, and while titles still sometimes overflow the sidebar, it's only by a few chars.

=== modified file '.bzrignore'
--- .bzrignore	2011-02-08 10:16:30 +0000
+++ .bzrignore	2011-09-13 12:37:42 +0000
@@ -1,5 +1,6 @@
 /debian/files
 /docs/build
+/docs/source/generated
 /_trial_temp
 /tags
 zookeeper.log

=== modified file 'docs/Makefile'
--- docs/Makefile	2011-03-17 17:59:16 +0000
+++ docs/Makefile	2011-09-13 12:37:42 +0000
@@ -3,7 +3,7 @@
 
 # You can set these variables from the command line.
 SPHINXOPTS    =
-SPHINXBUILD   = sphinx-build
+SPHINXBUILD   = python source/generate_modules.py ../ensemble source/generated && sphinx-build
 PAPER         =
 BUILDDIR      = build
 
@@ -36,6 +36,7 @@
 
 clean:
 	-rm -rf $(BUILDDIR)/*
+	-rm -rf source/generated
 
 html:
 	$(SPHINXBUILD) -b html $(ALLSPHINXOPTS) $(BUILDDIR)/html

=== modified file 'docs/source/conf.py'
--- docs/source/conf.py	2011-05-04 16:04:46 +0000
+++ docs/source/conf.py	2011-09-13 12:37:42 +0000
@@ -16,7 +16,7 @@
 # If extensions (or modules to document with autodoc) are in another directory,
 # add these directories to sys.path here. If the directory is relative to the
 # documentation root, use os.path.abspath to make it absolute, like shown here.
-#sys.path.append(os.path.abspath('.'))
+sys.path.insert(0, os.path.abspath('../..'))
 
 # -- General configuration -----------------------------------------------------
 
@@ -30,7 +30,10 @@
 extensions = ['sphinx.ext.autodoc']
 
 if [int(x) for x in sphinx.__version__.split(".")] > [1, 0]:
-    extensions.append('sphinx.ext.viewcode')
+    if "singlehtml" not in sys.argv:
+        # singlehtml builder skips the step that would cause the _modules
+        # directory to be created, so source links don't work
+        extensions.append('sphinx.ext.viewcode')
 
 # Add any paths that contain templates here, relative to this directory.
 templates_path = ['_templates']
@@ -144,7 +147,7 @@
 #html_additional_pages = {}
 
 # If false, no module index is generated.
-#html_domain_indices = True
+html_domain_indices = False
 
 # If false, no index is generated.
 #html_use_index = True

=== added file 'docs/source/generate_modules.py'
--- docs/source/generate_modules.py	1970-01-01 00:00:00 +0000
+++ docs/source/generate_modules.py	2011-09-13 12:37:42 +0000
@@ -0,0 +1,107 @@
+import os
+import sys
+
+INIT = "__init__.py"
+TESTS = "tests"
+
+
+def get_modules(names):
+    if INIT in names:
+        names.remove(INIT)
+    return [n[:-3] for n in names if n.endswith(".py")]
+
+
+def trim_dirs(root, dirs):
+    for dir_ in dirs[:]:
+        if dir_ == TESTS:
+            dirs.remove(TESTS)
+        if not os.path.exists(os.path.join(root, dir_, INIT)):
+            dirs.remove(dir_)
+    return dirs
+
+
+def module_name(base, root, name=None):
+    path = root[len(base) + 1:]
+    if name:
+        path = os.path.join(path, name)
+    return path.replace("/", ".")
+
+
+def collect_modules(src):
+    src = os.path.abspath(src)
+    base = os.path.dirname(src)
+
+    names = []
+    for root, dirs, files in os.walk(src):
+        modules = get_modules(files)
+        packages = trim_dirs(root, dirs)
+        if modules or packages:
+            names.append(module_name(base, root))
+            for name in modules:
+                names.append(module_name(base, root, name))
+    return sorted(names)
+
+
+def subpackages(names, parent):
+    return [name for name in names
+            if name.startswith(parent) and name != parent]
+
+
+def dst_file(dst, name):
+    return open(os.path.join(dst, "%s.rst" % name), "w")
+
+
+def write_title(f, name, kind):
+    f.write("%s\n%s\n\n" % (name, kind * len(name)))
+
+
+def write_packages(f, names):
+    for name in names:
+        f.write("    " * name.count("."))
+        f.write("* :mod:`%s`\n" % name)
+    f.write("\n")
+
+
+def abbreviate(name):
+    parts = name.split(".")
+    short_parts = [part[0] for part in parts[:-2]]
+    return ".".join(short_parts + parts[-2:])
+
+
+def write_module(f, name, subs):
+    write_title(f, abbreviate(name), "=")
+    f.write(".. automodule:: %s\n"
+            "    :members:\n"
+            "    :undoc-members:\n"
+            "    :show-inheritance:\n\n"
+            % name)
+    if subs:
+        write_title(f, "Subpackages", "-")
+        write_packages(f, subs)
+
+
+def write_module_list(f, names):
+    write_title(f, "Ensemble modules", "=")
+    write_packages(f, names)
+    f.write(".. toctree::\n    :hidden:\n\n")
+    for name in names:
+        f.write("    %s\n" % name)
+
+
+def generate(src, dst):
+    names = collect_modules(src)
+
+    if not os.path.exists(dst):
+        os.makedirs(dst)
+
+    with dst_file(dst, "modules") as f:
+        write_module_list(f, names)
+
+    for name in names:
+        with dst_file(dst, name) as f:
+            write_module(f, name, subpackages(names, name))
+
+
+if __name__ == "__main__":
+    src, dst = sys.argv[1:]
+    generate(src, dst)

=== modified file 'docs/source/index.rst'
--- docs/source/index.rst	2011-08-10 22:47:54 +0000
+++ docs/source/index.rst	2011-09-13 12:37:42 +0000
@@ -23,6 +23,7 @@
    ensemble-internals
    ensemble-drafts
    glossary
+   generated/modules
 
 
 Index and Glossary

=== modified file 'ensemble/providers/__init__.py'
--- ensemble/providers/__init__.py	2010-06-14 22:56:13 +0000
+++ ensemble/providers/__init__.py	2011-09-13 12:37:42 +0000
@@ -0,0 +1,1 @@
+"""Contains code specific to the various machine provider backends"""

=== modified file 'ensemble/providers/common/__init__.py'
--- ensemble/providers/common/__init__.py	2011-07-13 02:26:21 +0000
+++ ensemble/providers/common/__init__.py	2011-09-13 12:37:42 +0000
@@ -0,0 +1,1 @@
+"""Contains code common to more than one machine provider"""

=== modified file 'ensemble/providers/common/base.py'
--- ensemble/providers/common/base.py	2011-09-13 09:53:33 +0000
+++ ensemble/providers/common/base.py	2011-09-13 12:37:42 +0000
@@ -18,20 +18,20 @@
     To write a working subclass, you will need to override the following
     methods:
 
-        * get_file_storage
-        * start_machine
-        * get_machines
-        * shutdown_machines
-        * open_port
-        * close_port
-        * get_opened_ports
+        * :meth:`get_file_storage`
+        * :meth:`start_machine`
+        * :meth:`get_machines`
+        * :meth:`shutdown_machines`
+        * :meth:`open_port`
+        * :meth:`close_port`
+        * :meth:`get_opened_ports`
 
     You may want to override the following methods, but you should be careful
-    to call MachineProviderBase's implementation (or be very sure you don't
-    need to:
+    to call :class:`MachineProviderBase`'s implementation (or be very sure you
+    don't need to:
 
-        * __init__
-        * get_serialization_data
+        * :meth:`__init__`
+        * :meth:`get_serialization_data`
 
     You probably shouldn't override anything else.
     """
@@ -65,29 +65,39 @@
     def start_machine(self, machine_data, master=False):
         """Start a machine in the provider.
 
-        `machine_data`: a dictionary describing the machine to be launched.
+        :param dict machine_data: desired characteristics of the new machine;
+            it must include a "machine-id" key, and may include a "constraints"
+            key to specify the underlying OS and hardware (where available).
 
-        `master`: if True, machine will initialize the ensemble admin and run
-            a provisioning agent, in addition to running a machine agent
+        :param bool master: if True, machine will initialize the ensemble admin
+            and run a provisioning agent, in addition to running a machine
+            agent.
         """
         raise NotImplementedError()
 
     def get_machines(self, instance_ids=()):
         """List machines running in the provider.
 
-        @param instance_ids: ids of instances you want to get. Leave empty
-            to list all machines owned by this provider.
-
-        @return: a list of provider-specific ProviderMachines.
-
-        @raise: MachinesNotFound
+        :param list instance_ids: ids of instances you want to get. Leave empty
+            to list every :class:`ensemble.machine.ProviderMachine` owned by
+            this provider.
+
+        :return: a list of :class:`ensemble.machine.ProviderMachine` instances
+        :rtype: :class:`twisted.internet.defer.Deferred`
+
+        :raises: :exc:`ensemble.errors.MachinesNotFound`
         """
         raise NotImplementedError()
 
     def shutdown_machines(self, machines):
         """Terminate machines associated with this provider.
 
-        @param machines: list of machines to shut down
+        :param machines: machines to shut down
+        :type machines: list of :class:`ensemble.machine.ProviderMachine`
+
+        :return: list of terminated :class:`ensemble.machine.ProviderMachine`
+            instances
+        :rtype: :class:`twisted.internet.defer.Deferred`
         """
         raise NotImplementedError()
 
@@ -110,22 +120,27 @@
     def get_zookeeper_machines(self):
         """Find running zookeeper instances.
 
-        @return: the first valid instance found as a single element list.
+        :return: all running or starting machines configured to run zookeeper,
+            as a list of :class:`ensemble.machine.ProviderMachine`
+        :rtype: :class:`twisted.internet.defer.Deferred`
 
-        @raise: `EnvironmentNotFound`
+        :raises: :class:`ensemble.errors.EnvironmentNotFound`
         """
         return find_zookeepers(self)
 
     def connect(self, share=False):
         """Attempt to connect to a running zookeeper node.
 
-        @param share: where feasible, attempt to share a connection with other
-            clients
-
-        @return: an open `txzookeeper.client.ZookeeperClient`
-
-        @raise: `EnvironmentNotFound` when no zookeepers exist
-        @raise: `EnvironmentPending` when zookeepers exist but connect fails
+        :param bool share: where feasible, attempt to share a connection with
+            other clients
+
+        :return: an open :class:`txzookeeper.client.ZookeeperClient`
+        :rtype: :class:`twisted.internet.defer.Deferred`
+
+        :raises: :exc:`ensemble.errors.EnvironmentNotFound` when no zookeepers
+            exist
+        :raises: :exc:`ensemble.errors.EnvironmentPending` when zookeepers
+            exist but connection attempt fails
         """
         return ZookeeperConnect(self).run(share=share)
 
@@ -136,11 +151,13 @@
     def get_machine(self, instance_id):
         """Retrieve a provider machine by instance id.
 
-        @param instance_id: instance_id of the `ProviderMachine` you want.
-
-        @return: a single `ProviderMachine`
-
-        @raise: `MachinesNotFound`
+        :param str instance_id: :attr:`instance_id` of the
+            :class:`ensemble.machine.ProviderMachine` you want.
+
+        :return: the requested :class:`ensemble.machine.ProviderMachine`
+        :rtype: :class:`twisted.internet.defer.Deferred`
+
+        :raises: :exc:`ensemble.errors.MachinesNotFound`
         """
         d = self.get_machines([instance_id])
         d.addCallback(itemgetter(0))
@@ -149,9 +166,12 @@
     def shutdown_machine(self, machine):
         """Terminate one machine associated with this provider.
 
-        @param machine: ProviderMachine to shut down.
-
-        @raise: MachinesNotFound
+        :param machine: :class:`ensemble.machine.ProviderMachine` to shut down.
+
+        :return: the terminated :class:`ensemble.machine.ProviderMachine`.
+        :rtype: :class:`twisted.internet.defer.Deferred`
+
+        :raises: :exc:`ensemble.errors.MachinesNotFound`
         """
         d = self.shutdown_machines([machine])
         d.addCallback(itemgetter(0))
@@ -159,7 +179,10 @@
 
     @inlineCallbacks
     def destroy_environment(self):
-        """Clear ensemble state and terminate all associated machines"""
+        """Clear ensemble state and terminate all associated machines
+
+        :rtype: :class:`twisted.internet.defer.Deferred`
+        """
         try:
             yield self.save_state({})
         finally:
@@ -170,13 +193,16 @@
     def save_state(self, state):
         """Save state to the provider.
 
-        @param state: a dictionary to persist
+        :param dict state: state to persist
+
+        :rtype: :class:`twisted.internet.defer.Deferred`
         """
         return SaveState(self).run(state)
 
     def load_state(self):
         """Load state from the provider.
 
-        @return: a previously-persisted dictionary, or False
+        :return: the current state dict, or False
+        :rtype: :class:`twisted.internet.defer.Deferred`
         """
         return LoadState(self).run()

=== modified file 'ensemble/providers/common/bootstrap.py'
--- ensemble/providers/common/bootstrap.py	2011-08-17 12:04:56 +0000
+++ ensemble/providers/common/bootstrap.py	2011-09-13 12:37:42 +0000
@@ -16,8 +16,10 @@
     def run(self):
         """Get an existing zookeeper, or launch a new one.
 
-        @return: single-element list containing an appropriate
-            ProviderMachine instance for the zookeeper.
+        :return: a single-element list containing an appropriate
+            :class:`ensemble.machine.ProviderMachine` instance, set up to run
+            zookeeper and a provisioning agent.
+        :rtype: :class:`twisted.internet.defer.Deferred`
         """
         machines = self._provider.get_zookeeper_machines()
         machines.addCallback(self._on_machines_found)

=== modified file 'ensemble/providers/common/cloudinit.py'
--- ensemble/providers/common/cloudinit.py	2011-09-13 09:53:33 +0000
+++ ensemble/providers/common/cloudinit.py	2011-09-13 12:37:42 +0000
@@ -51,6 +51,11 @@
 
 
 class CloudInit(object):
+    """Encapsulates ensemble-specific machine initialisation.
+
+    For more information on the mechanism used, see
+    :func:`ensemble.providers.common.utils.format_cloud_init`.
+    """
 
     def __init__(self):
         self._machine_id = None
@@ -64,13 +69,38 @@
         self._zookeeper_secret = None
 
     def add_ssh_key(self, key):
+        """Add an SSH public key.
+
+        :param key: an SSH key to allow to connect to the machine
+
+        You have to set at least one SSH key.
+        """
         self._ssh_keys.append(key)
 
     def enable_bootstrap(self):
+        """Make machine run a zookeeper and a provisioning agent."""
         self._zookeeper = True
         self._provision = True
 
     def set_ensemble_source(self, branch=None, ppa=False, distro=False):
+        """Set the version of ensemble the machine should run.
+
+        :param branch: location from which to check out ensemble; for example,
+            "lp:~someone/ensemble/some-feature-branch".
+        :type branch: str or None
+
+        :param bool ppa: if True, get the latest version of ensemble from its
+            Private Package Archive.
+
+        :param bool distro: if True, get the default ensemble version from the
+            OS distribution.
+
+        :raises: :exc:`ensemble.errors.CloudInitError` if more than one option,
+            or fewer than one options, are specified.
+
+        Note that you don't need to call this method; the ensemble source
+        defaults to the ensemble PPA.
+        """
         if len(filter(None, (branch, ppa, distro))) != 1:
             raise CloudInitError("Please specify one source")
         if branch:
@@ -81,21 +111,59 @@
             self._source = _DISTRO
 
     def set_machine_id(self, id):
+        """Specify the ensemble machine ID.
+
+        :param str id: the desired ID.
+
+        You have to set the machine ID.
+        """
         self._machine_id = id
 
     def set_instance_id_accessor(self, expr):
+        """Specify the provider-specific instance ID.
+
+        :param str expr: a snippet of shell script that will, when executed on
+            the machine, evaluate to the machine's instance ID.
+
+        You have to set the instance ID.
+        """
         self._instance_id = expr
 
     def set_provider_type(self, type_):
+        """Specify the provider type for this machine.
+
+        :param str type_: the provider type.
+
+        You have to set the provider type.
+        """
         self._provider_type = type_
 
     def set_zookeeper_machines(self, machines):
+        """Specify master :class:`ensemble.machine.ProviderMachine` instances.
+
+        :param machines: machines running zookeeper which already exist.
+        :type machines: list of :class:`ensemble.machine.ProviderMachine`
+
+        You don't have to set this, so long as the machine you are starting is
+        itself a zookeeper instance.
+        """
         self._zookeeper_hosts = [m.private_dns_name for m in machines]
 
     def set_zookeeper_secret(self, secret):
+        """Specify the admin password for zookeepeer.
+
+        You only need to set this if this machine will be a zookeeper instance.
+        """
         self._zookeeper_secret = secret
 
     def render(self):
+        """Get content for a cloud-init file with appropriate specifications.
+
+        :rtype: str
+
+        :raises: :exc:`ensemble.errors.CloudInitError` if there isn't enough
+            information to create a useful cloud-init.
+        """
         self._validate()
         return format_cloud_init(
             self._ssh_keys,

=== modified file 'ensemble/providers/common/connect.py'
--- ensemble/providers/common/connect.py	2011-08-23 09:17:27 +0000
+++ ensemble/providers/common/connect.py	2011-09-13 12:37:42 +0000
@@ -17,13 +17,16 @@
     def run(self, share=False):
         """Attempt to connect to a running zookeeper node.
 
-        @param share: where feasible, attempt to share a connection with other
-            clients
-
-        @return: an open txzookeeper.client.ZookeeperClient
-
-        @raise: EnvironmentNotFound when no zookeepers exist
-        @raise: EnvironmentPending when zookeepers exist but connect fails
+        :param bool share: where feasible, attempt to share a connection with
+            other clients
+
+        :return: an open :class:`txzookeeper.client.ZookeeperClient`
+        :rtype: :class:`twisted.internet.defer.Deferred`
+
+        :raises: :exc:`ensemble.errors.EnvironmentNotFound` when no zookeepers
+            exist
+        :raises: :exc:`ensemble.errors.EnvironmentPending` when zookeepers
+            exist but connection attempt fails
         """
         candidates = yield self._provider.get_zookeeper_machines()
         chosen = yield self._pick_machine(candidates)

=== modified file 'ensemble/providers/common/findzookeepers.py'
--- ensemble/providers/common/findzookeepers.py	2011-08-18 13:00:47 +0000
+++ ensemble/providers/common/findzookeepers.py	2011-09-13 12:37:42 +0000
@@ -10,6 +10,16 @@
 
 @inlineCallbacks
 def find_zookeepers(provider):
+    """Find running zookeeper instances.
+
+    :param provider: the MachineProvider in charge of the ensemble
+
+    :return: all running or starting machines configured to run zookeeper, as a
+        list of :class:`ensemble.machine.ProviderMachine`
+    :rtype: :class:`twisted.internet.defer.Deferred`
+
+    :raises: :class:`ensemble.errors.EnvironmentNotFound`
+    """
     state = yield provider.load_state()
     _require(state)
     instance_ids = state.get("zookeeper-instances")

=== modified file 'ensemble/providers/common/launch.py'
--- ensemble/providers/common/launch.py	2011-08-31 15:39:20 +0000
+++ ensemble/providers/common/launch.py	2011-09-13 12:37:42 +0000
@@ -8,13 +8,20 @@
     """Abstract class with generic instance-launching logic.
 
     To create your own subclass, you will certainly need to override
-    `start_machine`, and will very probably want to implement it with the aid
-    of a provider-specific subclass of `ProviderCloudInit`.
-
-    `provider`: the necessary `MachineProvider`
-
-    `master`: if True, the machine will run a zookeeper and a provisioning
-        agent, in addition to the machine agent that every machine runs
+    :meth:`start_machine`, which will very probably want to use the incomplete
+    :class:`ensemble.providers.common.cloudinit.CloudInit` returned by
+    :meth:`_create_cloud_init`.
+
+    :param provider: the `MachineProvider` that will administer the machine
+
+    :param bool master: if True, the machine will run a zookeeper and a
+        provisioning agent, in addition to the machine agent that every
+        machine runs automatically.
+
+    :param dict constraints: specifies the underlying OS and hardware (where
+        available)
+
+    .. automethod:: _create_cloud_init
     """
 
     def __init__(self, provider, master=False, constraints=None):
@@ -26,7 +33,7 @@
     def run(self, machine_id):
         """Launch an instance node within the machine provider environment.
 
-        `machine_id`: the ensemble machine ID
+        :param str machine_id: the ensemble machine ID to assign
         """
         # XXX at some point, we'll want to start multiple zookeepers
         # that know about each other; for now, this'll do
@@ -43,17 +50,27 @@
     def start_machine(self, machine_id, zookeepers):
         """Actually launch a machine for the appropriate provider.
 
-        `machine_id`: the ensemble machine ID
-
-        `zookeepers`: a list of `ProviderMachine`s already running zookeeper,
-            which the machine will need to connect to. May be empty.
-
-        Returns a singe-entry list containing a `ProviderMachine` for the
-        new instance
+        :param str machine_id: the ensemble machine ID to assign
+
+        :param zookeepers: the machines currently running zookeeper, to which
+            the new machine will need to connect
+        :type zookeepers: list of :class:`ensemble.machine.ProviderMachine`
+
+        :return: a singe-entry list containing a provider-specific
+            :class:`ensemble.machine.ProviderMachine` representing the newly-
+            launched machine
+        :rtype: :class:`twisted.internet.defer.Deferred`
         """
         raise NotImplementedError()
 
     def _create_cloud_init(self, machine_id, zookeepers):
+        """Construct a provider-independent but incomplete :class:`CloudInit`.
+
+        :return: a :class:`ensemble.providers.common.cloudinit.cloudInit`; it
+            will not be ready to render, but will be configured to the greatest
+            extent possible with the available information.
+        :rtype: :class:`twisted.internet.defer.Deferred`
+        """
         config = self._provider.config
         cloud_init = CloudInit()
         cloud_init.add_ssh_key(get_user_authorized_keys(config))

=== modified file 'ensemble/providers/common/state.py'
--- ensemble/providers/common/state.py	2011-07-26 09:13:08 +0000
+++ ensemble/providers/common/state.py	2011-09-13 12:37:42 +0000
@@ -7,11 +7,20 @@
 
 
 class LoadState(object):
+    """Generic state-loading operation.
+
+    Note that most ensemble state should be stored in zookeeper nodes; this
+    is only for state which must be knowable without access to a zookeeper.
+    """
 
     def __init__(self, provider):
         self._provider = provider
 
     def run(self):
+        """Actually load the state.
+
+        :rtype: dict or False
+        """
         storage = self._provider.get_file_storage()
         d = storage.get(_STATE_FILE)
         d.addCallback(self._deserialize)
@@ -27,11 +36,20 @@
 
 
 class SaveState(object):
+    """Generic state-saving operation.
+
+    Note that most ensemble state should be stored in zookeeper nodes; this
+    is only for state which must be knowable without access to a zookeeper.
+    """
 
     def __init__(self, provider):
         self._provider = provider
 
     def run(self, state):
+        """Actually save new state.
+
+        :param dict state: state to save.
+        """
         storage = self._provider.get_file_storage()
         data = safe_dump(state)
         return storage.put(_STATE_FILE, StringIO(data))

=== modified file 'ensemble/providers/common/utils.py'
--- ensemble/providers/common/utils.py	2011-09-13 10:55:35 +0000
+++ ensemble/providers/common/utils.py	2011-09-13 12:37:42 +0000
@@ -10,19 +10,20 @@
 
 
 def convert_unknown_error(failure):
-    """Convert any non ensemble errors to a provider interaction error.
+    """Convert any non-ensemble errors to a provider interaction error.
 
     Supports both usage from within an except clause, and as an
     errback handler ie. both the following forms are supported.
 
     ...
-    try:
-       something()
-    except Exception, e:
-       convert_unknown_errors(e)
+        try:
+           something()
+        except Exception, e:
+           convert_unknown_errors(e)
 
     ...
-    d.addErrback(convert_unknown_errors)
+        d.addErrback(convert_unknown_errors)
+
     """
     if isinstance(failure, Failure):
         error = failure.value
@@ -46,9 +47,10 @@
     If neither "authorized-keys" nor "authorized-keys-path" is
     present in config, will look in the user's .ssh directory.
 
-    @return: a public key.
+    :return: an SSH public key
+    :rtype: str
 
-    @raise LookupError: Raised if a public ssh key is not found.
+    :raises: :exc:`LookupError` if an SSH public key is not found.
     """
     key_names = ["id_dsa.pub", "id_rsa.pub", "identity.pub"]
     if config.get("authorized-keys"):
@@ -83,23 +85,23 @@
     Further documentation on the capabilities of cloud-init
     https://help.ubuntu.com/community/CloudInit
 
-    @param authorized_keys The authorized SSH keys to be used when populating
-           the newly launched machine's.
-    @type list of strings
-
-    @param packages A list of packages to be installed on a machine.
-    @type list of strings
-
-    @param repository A listing of debian repostiories to add the machine's
-           apt sources. 'ppa:' syntax can be utilized.
-    @type list of strings
-
-    @param scripts A list of scripts to be executed on machine start.
-    @type list of strings
-
-    @param data An optional dictionary of data to be passed via cloud config.
-           It will be accessible via the key 'machine-data' from the yaml
-           data structure.
+    :param authorized_keys: The authorized SSH key to be used when
+        populating the newly launched machines.
+    :type authorized_keys: list of strings
+
+    :param packages: The packages to be installed on a machine.
+    :type packages: list of strings
+
+    :param repositories: Debian repostiories to be used as apt sources on the
+        machine. 'ppa:' syntax can be used.
+    :type repositories: list of strings
+
+    :param scripts: Scripts to be executed (in order) on machine start.
+    :type scripts: list of strings
+
+    :param dict data: Optional additional data to be passed via cloud config.
+        It will be accessible via the key 'machine-data' from the yaml data
+        structure.
     """
     cloud_config = {
         "apt-update": True,

=== modified file 'ensemble/providers/ec2/__init__.py'
--- ensemble/providers/ec2/__init__.py	2011-09-13 09:53:33 +0000
+++ ensemble/providers/ec2/__init__.py	2011-09-13 12:37:42 +0000
@@ -20,6 +20,7 @@
 
 
 class MachineProvider(MachineProviderBase):
+    """MachineProvider for use in an EC2/S3 environment"""
 
     def __init__(self, environment_name, config):
         super(MachineProvider, self).__init__(environment_name, config)
@@ -48,16 +49,19 @@
         return data
 
     def get_file_storage(self):
-        """Retrieve an S3-backed `FileStorage`."""
+        """Retrieve an S3-backed :class:`FileStorage`."""
         return FileStorage(self.s3, self.config["control-bucket"])
 
     def start_machine(self, machine_data, master=False):
-        """Start a machine in the provider.
-
-        `machine_data`: a dictionary describing the machine to be launched.
-
-        @param master: if True, machine will initialize the ensemble admin
-            and run a provisioning agent.
+        """Start an EC2 machine.
+
+        :param dict machine_data: desired characteristics of the new machine;
+            it must include a "machine-id" key, and may include a "constraints"
+            key to specify the underlying OS and hardware.
+
+        :param bool master: if True, machine will initialize the ensemble admin
+            and run a provisioning agent, in addition to running a machine
+            agent.
         """
         if "machine-id" not in machine_data:
             return fail(ProviderError(
@@ -70,12 +74,17 @@
     def get_machines(self, instance_ids=()):
         """List machines running in the provider.
 
-        @param instance_ids: ids of instances you want to get. Leave empty
-            to list all machines owned by this provider.
-
-        @return: a list of EC2ProviderMachines.
-
-        @raise: MachinesNotFound
+        :param list instance_ids: ids of instances you want to get. Leave empty
+            to list every
+            :class:`ensemble.providers.ec2.machine.EC2ProviderMachine` owned by
+            this provider.
+
+        :return: a list of
+            :class:`ensemble.providers.ec2.machine.EC2ProviderMachine`
+            instances
+        :rtype: :class:`twisted.internet.defer.Deferred`
+
+        :raises: :exc:`ensemble.errors.MachinesNotFound`
         """
         group_name = "ensemble-%s" % self.environment_name
         try:
@@ -115,6 +124,8 @@
         The super defintion of this method terminates each machine in
         the environment; this needs to be augmented here by also
         removing the security group for the environment.
+
+        :rtype: :class:`twisted.internet.defer.Deferred`
         """
         try:
             killed_machines = yield super(MachineProvider, self).\
@@ -127,7 +138,14 @@
     def shutdown_machines(self, machines):
         """Terminate machines associated with this provider.
 
-        @param machines: list of machines to shut down
+        :param machines: machines to shut down
+        :type machines: list of
+            :class:`ensemble.providers.ec2.machine.EC2ProviderMachine`
+
+        :return: list of terminated
+            :class:`ensemble.providers.ec2.machine.EC2ProviderMachine`
+            instances
+        :rtype: :class:`twisted.internet.defer.Deferred`
         """
         if not machines:
             returnValue([])

=== modified file 'ensemble/providers/ec2/files.py'
--- ensemble/providers/ec2/files.py	2011-08-26 14:52:48 +0000
+++ ensemble/providers/ec2/files.py	2011-09-13 12:37:42 +0000
@@ -27,6 +27,7 @@
 
 
 class FileStorage(object):
+    """S3-backed :class:`FileStorage` abstraction"""
 
     def __init__(self, s3, bucket):
         self._s3 = s3
@@ -37,6 +38,11 @@
 
         S3 time authenticated URL reference:
         http://s3.amazonaws.com/doc/s3-developer-guide/RESTAuthentication.html
+
+        :param unicode name: the S3 key for which to provide a URL
+
+        :return: a signed URL, expiring 10 years from now
+        :rtype: str
         """
         # URLs are good for 10 years.
         expires = int(time.time()) + 365 * 24 * 3600 * 10
@@ -55,13 +61,14 @@
         return url
 
     def get(self, name):
-        """
-        If the file exists in the storage, this method will download the file
-        and return a file object. If it doesn't exist then a FileNotFound
-        failure is returned.
-
-        @return An open file object
-        @type Deferred
+        """Get a file object from S3.
+
+        :param unicode name: S3 key for the desired file
+
+        :return: an open file object
+        :rtype: :class:`twisted.internet.defer.Deferred`
+
+        :raises: :exc:`ensemble.errors.FileNotFound` if the file doesn't exist
         """
         # for now we do the simplest thing and just fetch the file
         # in a single call, s3 limits this to some mb, as we grow
@@ -85,15 +92,13 @@
         return d
 
     def put(self, remote_path, file_object):
-        """
-        Upload data to S3.
-
-        @param remote_path: key on which to store the content
-
-        @param file_object: open file object containing the content
-
-        @return A deferred with a boolean value
-        @type Deferred
+        """Upload a file to S3.
+
+        :param unicode remote_path: key on which to store the content
+
+        :param file_object: open file object containing the content
+
+        :rtype: :class:`twisted.internet.defer.Deferred`
         """
         content = file_object.read()
         path = _safe_string(remote_path)

=== modified file 'ensemble/providers/ec2/launch.py'
--- ensemble/providers/ec2/launch.py	2011-09-13 09:53:33 +0000
+++ ensemble/providers/ec2/launch.py	2011-09-13 12:37:42 +0000
@@ -16,13 +16,17 @@
     def start_machine(self, machine_id, zookeepers):
         """Actually launch an instance on EC2.
 
-        `machine_id`: the ensemble machine ID
-
-        `zookeepers`: a list of `ProviderMachine`s already running zookeeper,
-            which the machine will need to connect to. May be empty.
-
-        Returns a singe-entry list containing a ProviderMachine for the
-        new instance
+        :param str machine_id: the ensemble machine ID to assign
+
+        :param zookeepers: the machines currently running zookeeper, to which
+            the new machine will need to connect
+        :type zookeepers: list of
+            :class:`ensemble.providers.ec2.machine.EC2ProviderMachine`
+
+        :return: a singe-entry list containing a
+            :class:`ensemble.providers.ec2.machine.EC2ProviderMachine`
+            representing the newly-launched machine
+        :rtype: :class:`twisted.internet.defer.Deferred`
         """
         cloud_init = self._create_cloud_init(machine_id, zookeepers)
         cloud_init.set_provider_type("ec2")
@@ -56,7 +60,7 @@
         specific machine security group is created for each machine,
         so that its firewall rules can be configured per machine.
 
-        `machine_id`: The external machine ID of this machine instance
+        :param machine_id: The ensemble machine ID of the new machine
         """
         ensemble_group = "ensemble-%s" % self._provider.environment_name
         ensemble_machine_group = "ensemble-%s-%s" % (

=== modified file 'ensemble/providers/ec2/machine.py'
--- ensemble/providers/ec2/machine.py	2011-08-29 18:34:02 +0000
+++ ensemble/providers/ec2/machine.py	2011-09-13 12:37:42 +0000
@@ -9,6 +9,12 @@
 
 
 def machine_from_instance(instance):
+    """Create an :class:`EC2ProviderMachine` from a txaws :class:`Instance`
+
+    :param instance: the EC2 Instance
+
+    :return: a matching :class:`EC2ProviderMachine`
+    """
     return EC2ProviderMachine(
         instance.instance_id,
         instance.dns_name,

=== modified file 'ensemble/providers/ec2/utils.py'
--- ensemble/providers/ec2/utils.py	2011-09-06 13:11:30 +0000
+++ ensemble/providers/ec2/utils.py	2011-09-13 12:37:42 +0000
@@ -48,6 +48,16 @@
 
 
 def get_image_id(config, constraints):
+    """Get an EC2 image ID.
+
+    :param dict config: environment configuration (which can override
+        `constraints`)
+    :param dict constraints: specific requirements for requested machine; TODO
+        improve documentation once requirements firm up.
+
+    :return: An AMI ID
+    :rtype: :class:`twisted.internet.defer.Deferred`
+    """
     image_id = config.get("default-image-id", None)
     if image_id:
         return succeed(image_id)

=== modified file 'ensemble/providers/orchestra/__init__.py'
--- ensemble/providers/orchestra/__init__.py	2011-09-13 09:53:33 +0000
+++ ensemble/providers/orchestra/__init__.py	2011-09-13 12:37:42 +0000
@@ -14,6 +14,7 @@
 
 
 class MachineProvider(MachineProviderBase):
+    """MachineProvider for use in an Orchestra environment"""
 
     def __init__(self, environment_name, config):
         super(MachineProvider, self).__init__(environment_name, config)
@@ -27,12 +28,15 @@
         return FileStorage(self.config["storage-url"])
 
     def start_machine(self, machine_data, master=False):
-        """Start a machine in the provider.
-
-        `machine_data`: a dictionary describing the machine to be launched.
-
-        `master`: if True, machine will initialize the ensemble admin and run
-            a provisioning agent, in addition to running a machine agent
+        """Start an Orchestra machine.
+
+        :param dict machine_data: desired characteristics of the new machine;
+            it must include a "machine-id" key, and may include a "constraints"
+            key (which is currently ignored by this provider).
+
+        :param bool master: if True, machine will initialize the ensemble admin
+            and run a provisioning agent, in addition to running a machine
+            agent.
         """
         if "machine-id" not in machine_data:
             return fail(ProviderError(
@@ -44,12 +48,16 @@
     def get_machines(self, instance_ids=()):
         """List machines running in the provider.
 
-        @param instance_ids: ids of instances you want to get. Leave empty
-            to list all machines owned by this provider.
-
-        @return: a list of OrchestraMachines.
-
-        @raise: MachinesNotFound
+        :param list instance_ids: ids of instances you want to get. Leave empty
+            to list every
+            :class:`ensemble.providers.orchestra.machine.OrchestraMachine`
+            owned by this provider.
+
+        :return: a list of
+            :class:`ensemble.providers.orchestra.machine.OrchestraMachine`
+        :rtype: :class:`twisted.internet.defer.Deferred`
+
+        :raises: :exc:`ensemble.errors.MachinesNotFound`
         """
         instances = yield self.cobbler.describe_systems(*instance_ids)
         returnValue([machine_from_dict(i) for i in instances])
@@ -58,7 +66,14 @@
     def shutdown_machines(self, machines):
         """Terminate machines associated with this provider.
 
-        @param machines: list of machines to shut down
+        :param machines: machines to shut down
+        :type machines: list of
+            :class:`ensemble.providers.orchestra.machine.OrchestraMachine`
+
+        :return: list of terminated
+            :class:`ensemble.providers.orchestra.machine.OrchestraMachine`
+            instances
+        :rtype: :class:`twisted.internet.defer.Deferred`
         """
         if not machines:
             returnValue([])

=== modified file 'ensemble/providers/orchestra/cobbler.py'
--- ensemble/providers/orchestra/cobbler.py	2011-08-26 12:24:00 +0000
+++ ensemble/providers/orchestra/cobbler.py	2011-09-13 12:37:42 +0000
@@ -96,13 +96,16 @@
     def set_on_system(self, instance_id, key, value):
         """Set an attribute on a Cobbler system.
 
-        @param instance_id: the Cobbler uid of the system
-
-        @param key: the name of the attribute
-
-        @param value: the desired value of the attribute
-
-        @raise: MachinesNotFound, ProviderError
+        :param str instance_id: the Cobbler uid of the system
+
+        :param str key: the name of the attribute
+
+        :param value: the desired value of the attribute (appropriate types
+            depend on `value`)
+
+        :raises: :exc:`ensemble.errors.ProviderError` on invalid cobbler state
+        :raises: :exc:`ensemble.errors.MachinesNotFound` when `instance_id` is
+            not acquired
         """
         name = yield self._get_name(instance_id)
         try:
@@ -158,9 +161,11 @@
     def acquire_system(self):
         """Find a system marked as available and mark it as acquired.
 
-        @return: the instance id (cobbler uid) of the acquired system.
+        :return: the instance id (Cobbler uid) str of the acquired system.
+        :rtype: :class:`twisted.internet.defer.Deferred`
 
-        @raise: ProviderError if no suitable system can be found.
+        :raises: :exc:`ensemble.errors.ProviderError` if no suitable system can
+            be found.
         """
         instance_id, new_classes = yield self._get_available_system()
         yield self.set_on_system(instance_id, "mgmt_classes", new_classes)
@@ -168,7 +173,10 @@
 
     @inlineCallbacks
     def release_system(self, instance_id):
-        """Take a system marked as acquired, and make it available again"""
+        """Take a system marked as acquired, and make it available again
+
+        :rtype: :class:`twisted.internet.defer.Deferred`
+        """
         (system,) = yield self.describe_systems(instance_id)
         new_classes = map(self._class_swapper, system["mgmt_classes"])
         yield self.set_on_system(instance_id, "mgmt_classes", new_classes)
@@ -178,9 +186,14 @@
     def describe_systems(self, *instance_ids):
         """Get all available information about systems.
 
-        @return: a list of dictionaries
-
-        @raise: MachineNotFound if any requested instance_id doesn't exist
+        :param instance_ids: Cobbler uids of requested systems; leave blank to
+            return information about all acquired systems.
+
+        :return: a list of dictionaries describing acquired systems
+        :rtype: :class:`twisted.internet.defer.Deferred`
+
+        :raises: :exc:`ensemble.errors.MachinesNotFound` if any requested
+            instance_id doesn't exist
         """
         all_systems = yield self._caller.call("get_systems")
         acquired_systems = [s for s in all_systems
@@ -207,7 +220,12 @@
                                  auth=True)
     @inlineCallbacks
     def start_system(self, instance_id):
-        """Launch a cobbler system."""
+        """Launch a cobbler system.
+
+        :param instance_id: The Cobbler uid of the desired system.
+
+        :rtype: :class:`twisted.internet.defer.Deferred`
+        """
         name = yield self.set_on_system(instance_id, "netboot_enabled", True)
         yield self._power_call("on", [name])
 
@@ -216,6 +234,10 @@
         """Shut down a cobbler system.
 
         Also reenables netboot for next time.
+
+        :param instance_id: The Cobbler uid of the condemned system.
+
+        :rtype: :class:`twisted.internet.defer.Deferred`
         """
         name = yield self.set_on_system(instance_id, "netboot_enabled", True)
         yield self._power_call("off", [name])

=== modified file 'ensemble/providers/orchestra/files.py'
--- ensemble/providers/orchestra/files.py	2011-08-29 09:41:12 +0000
+++ ensemble/providers/orchestra/files.py	2011-09-13 12:37:42 +0000
@@ -9,11 +9,19 @@
 
 
 class FileStorage(object):
+    """A WebDAV-backed :class:`FileStorage` abstraction"""
 
     def __init__(self, base_url):
         self._base_url = base_url
 
     def get_url(self, name):
+        """Return a URL that can be used to access a stored file.
+
+        :param unicode name: the file path for which to provide a URL
+
+        :return: a URL
+        :rtype: str
+        """
         url = u"/".join((self._base_url, name))
         # query and fragment are irrelevant to our purposes
         scheme, netloc, path = urlparse.urlsplit(url)[:3]
@@ -24,6 +32,15 @@
             "", ""))
 
     def get(self, name):
+        """Get a file object from the Orchestra WebDAV server.
+
+        :param unicode name: path to for the desired file
+
+        :return: an open file object
+        :rtype: :class:`twisted.internet.defer.Deferred`
+
+        :raises: :exc:`ensemble.errors.FileNotFound` if the file doesn't exist
+        """
         url = self.get_url(name)
         d = getPage(url)
         d.addCallback(StringIO)
@@ -37,6 +54,14 @@
         return d
 
     def put(self, remote_path, file_object):
+        """Upload a file to S3.
+
+        :param unicode remote_path: path on which to store the content
+
+        :param file_object: open file object containing the content
+
+        :rtype: :class:`twisted.internet.defer.Deferred`
+        """
         url = self.get_url(remote_path)
         data = file_object.read()
         d = getPage(url, method="PUT", postdata=data)

=== modified file 'ensemble/providers/orchestra/launch.py'
--- ensemble/providers/orchestra/launch.py	2011-09-13 09:53:33 +0000
+++ ensemble/providers/orchestra/launch.py	2011-09-13 12:37:42 +0000
@@ -43,18 +43,23 @@
 
 
 class OrchestraLaunchMachine(LaunchMachine):
+    """rchestra operation for launching an instance"""
 
     @inlineCallbacks
     def start_machine(self, machine_id, zookeepers):
-        """Actually launch a Cobbler system.
-
-        `machine_id`: the ensemble machine ID
-
-        `zookeepers`: a list of `ProviderMachine`s already running zookeeper,
-            which the machine will need to connect to. May be empty.
-
-        Returns a singe-entry list containing a ProviderMachine for the
-        new instance
+        """Actually launch an instance with Orchestra.
+
+        :param str machine_id: the ensemble machine ID to assign
+
+        :param zookeepers: the machines currently running zookeeper, to which
+            the new machine will need to connect
+        :type zookeepers: list of
+            :class:`ensemble.providers.orchestra.machine.OrchestraMachine`
+
+        :return: a singe-entry list containing a
+            :class:`ensemble.providers.orchestra.machine.OrchestraMachine`
+            representing the newly-launched machine
+        :rtype: :class:`twisted.internet.defer.Deferred`
         """
         cobbler = self._provider.cobbler
         instance_id = yield cobbler.acquire_system()

=== modified file 'ensemble/providers/orchestra/machine.py'
--- ensemble/providers/orchestra/machine.py	2011-08-19 08:00:15 +0000
+++ ensemble/providers/orchestra/machine.py	2011-09-13 12:37:42 +0000
@@ -6,8 +6,13 @@
 
 
 def machine_from_dict(d):
-    """Convert a `dict` into an `OrchestraMachine`.
-
-    Expects dicts as returned by `CobblerClient.get_system[s]`
+    """Convert a `dict` into a :class:`OrchestraMachine`.
+
+    :param dict d: a dict as returned by
+        :meth:`ensemble.providers.orchestra.cobbler.CobblerClient.get_system`
+        or
+        :meth:`ensemble.providers.orchestra.cobbler.CobblerClient.get_systems`
+
+    :rtype: :class:`OrchestraMachine`
     """
     return OrchestraMachine(d["uid"], d["name"], d["name"])