Merge lp:~hazmat/pyjuju/ensemble-resolved into lp:pyjuju

[Kapil Thangavelu]

This adds a new command to ensemble to allow for manual correction of any errors that might occur in unit or unit relation hooks. The new command allows marking a unit or unit relation as needing to be resolved by the unit agent. It validates that the units are not running before scheduling them. It optionally allows specifying that hooks should run again.

[Gustavo Niemeyer (niemeyer)]

Looks good. Mostly just points we already talked about, plus moving
some of the error checking logic into the state API itself.

[1]

+ sub_parser = subparsers.add_parser("resolved", help=command.__doc__)

The nice docs are in the resolved function.

[2]

+ service_name, _ = unit_name.split("/")
+ service_state = yield service_manager.get_service_state(service_name)
+ unit_state = yield service_state.get_unit_state(unit_name)

Please invert the ordering here so that unit_state is retrieved
first. This will automatically validate the unit_name value
provided by the user, and avoid an ugly traceback in case the
user provides a bad unit_name value.

[3]

+ running, workflow_state = yield is_unit_running(client, unit_state)

It feels like that verification should be part of set_resolved(), since it
will never make sense to mark as resolved something which is already
running.

[4]

+ log.info("Unit %r already running:%s", unit_name, workflow_state)

s/running:%s/running: %s/

[5]

+ # Check for the matching relations
+ service_relations = yield relation_manager.get_relations_for_service(

As per our conversation, the mapping between user-provided relation names and
internal ids this should be moved into the state.

[6]

+ # Verify the relations are in need of resolution.
+ resolve_relations = {}

Same case as unit resolution: the error checking can be put within
set_relation_resolved() itself, and the command becomes a simple user
of the API.

review: Needs Fixing

[Kapil Thangavelu (hazmat)]

Excerpts from Gustavo Niemeyer's message of Tue Apr 26 19:30:57 UTC 2011:
> Review: Needs Fixing
> Looks good. Mostly just points we already talked about, plus moving
> some of the error checking logic into the state API itself.
>
>
> [1]
>
> + sub_parser = subparsers.add_parser("resolved", help=command.__doc__)
>
> The nice docs are in the resolved function.

Moved the long docs to subcommand description for resolved -h, updated the
command doc with a better summary.

>
> [2]
>
> + service_name, _ = unit_name.split("/")
> + service_state = yield service_manager.get_service_state(service_name)
> + unit_state = yield service_state.get_unit_state(unit_name)
>
> Please invert the ordering here so that unit_state is retrieved
> first. This will automatically validate the unit_name value
> provided by the user, and avoid an ugly traceback in case the
> user provides a bad unit_name value.
>

done.

[3-6]

as per irc discussions, moving the validation logic and internal-ids to inside
set-relation-resolved is eing deferred to another bug. The unit agent
implementation of resolved will need to verify values as well before acting
upon a unit/relation.

[lp:~hazmat/pyjuju/ensemble-resolved]

255. By Kapil Thangavelu on 2011-05-02

Merged resolved-state-api into ensemble-resolved.

[Gustavo Niemeyer (niemeyer)]

This looks great, +1! Only one nit related to terminology:

[7]

+ # Verify the relations are in need of resolution.
+ resolve_relations = {}
(...)
+ # Mark the relations for resolution.
+ yield unit_state.set_relation_resolved(resolve_relations)

There's some terminology conflict here, in the spirit we debated in the IRC channel regarding the pre-requisite branch.

E.g.:

s/are in need of resolution/are not running/
s/resolve_relations/resolved_relations/
s/for resolution/as resolved/

review: Approve

[lp:~hazmat/pyjuju/ensemble-resolved]

256. By Kapil Thangavelu on 2011-05-04

Merged resolved-state-api into ensemble-resolved.

257. By Kapil Thangavelu on 2011-05-05

Merged resolved-state-api into ensemble-resolved.

258. By Kapil Thangavelu on 2011-05-05

Merged resolved-state-api into ensemble-resolved.

259. By Kapil Thangavelu on 2011-05-05

update code comments per review comment

=== modified file 'ensemble/control/__init__.py'
--- ensemble/control/__init__.py	2011-05-03 08:54:13 +0000
+++ ensemble/control/__init__.py	2011-05-05 16:55:55 +0000
@@ -17,6 +17,7 @@
 import open_tunnel
 import remove_relation
 import remove_unit
+import resolved
 import status
 import shutdown
 import ssh
@@ -39,6 +40,7 @@
     open_tunnel,
     remove_relation,
     remove_unit,
+    resolved,
     status,
     ssh,
     shutdown,
 
=== added file 'ensemble/control/resolved.py'
--- ensemble/control/resolved.py	1970-01-01 00:00:00 +0000
+++ ensemble/control/resolved.py	2011-05-05 16:55:55 +0000
@@ -0,0 +1,110 @@
+"""Implementation of resolved subcommand"""
+
+
+from twisted.internet.defer import inlineCallbacks, returnValue
+
+from ensemble.control.utils import get_environment
+
+from ensemble.state.service import ServiceStateManager, RETRY_HOOKS, NO_HOOKS
+from ensemble.state.relation import RelationStateManager
+from ensemble.state.errors import RelationStateNotFound
+from ensemble.unit.workflow import is_unit_running, is_relation_running
+
+
+def configure_subparser(subparsers):
+    """Configure resolved subcommand"""
+    sub_parser = subparsers.add_parser(
+        "resolved", help=command.__doc__, description=resolved.__doc__)
+    sub_parser.add_argument(
+        "--retry", "-r", action="store_true",
+        help="Retry failed hook."),
+    sub_parser.add_argument(
+        "--environment", "-e",
+        help="Ensemble environment to operate in.")
+    sub_parser.add_argument(
+        "service_unit_name",
+        help="Name of the service unit that should be resolved")
+    sub_parser.add_argument(
+       "relation_name", nargs="?", default=None,
+        help="Name of the unit relation that should be resolved")
+    return sub_parser
+
+
+def command(options):
+    """Mark an error as resolved in a unit or unit relation."""
+    environment = get_environment(options)
+    return resolved(
+        options.environments,
+        environment,
+        options.verbose,
+        options.log,
+        options.service_unit_name,
+        options.relation_name,
+        options.retry)
+
+
+@inlineCallbacks
+def resolved(
+    config, environment, verbose, log, unit_name, relation_name, retry):
+    """Mark an error as resolved in a unit or unit relation.
+
+    If one of a unit's formula non-relation hooks returns a non-zero exit
+    status, the entire unit can be considered to be in a non-running state.
+
+    As a resolution, the the unit can be manually returned a running state
+    via the ensemble resolved command. Optionally this command can also
+    rerun the failed hook.
+
+    This resolution also applies separately to each of the unit's relations.
+    If one of the relation-hooks failed. In that case there is no
+    notion of retrying (the change is gone), but resolving will allow
+    additional relation hooks for that relation to proceed.
+    """
+    provider = environment.get_machine_provider()
+    client = yield provider.connect()
+    service_manager = ServiceStateManager(client)
+    relation_manager = RelationStateManager(client)
+
+    unit_state = yield service_manager.get_unit_state(unit_name)
+    service_state = yield service_manager.get_service_state(
+        unit_name.split("/")[0])
+
+    retry = retry and RETRY_HOOKS or NO_HOOKS
+
+    if not relation_name:
+        running, workflow_state = yield is_unit_running(client, unit_state)
+        if running:
+            log.info("Unit %r already running: %s", unit_name, workflow_state)
+            client.close()
+            returnValue(False)
+
+        yield unit_state.set_resolved(retry)
+        log.info("Marked unit %r as resolved", unit_name)
+        returnValue(True)
+
+    # Check for the matching relations
+    service_relations = yield relation_manager.get_relations_for_service(
+        service_state)
+    service_relations = [
+        sr for sr in service_relations if sr.relation_name == relation_name]
+    if not service_relations:
+        raise RelationStateNotFound()
+
+    # Verify the relations are in need of resolution.
+    resolved_relations = {}
+    for service_relation in service_relations:
+        unit_relation = yield service_relation.get_unit_state(unit_state)
+        running, state = yield is_relation_running(client, unit_relation)
+        if not running:
+            resolved_relations[unit_relation.internal_relation_id] = retry
+
+    if not resolved_relations:
+        log.warning("Matched relations are all running")
+        client.close()
+        returnValue(False)
+
+    # Mark the relations as resolved.
+    yield unit_state.set_relation_resolved(resolved_relations)
+    log.info(
+        "Marked unit %r relation %r as resolved", unit_name, relation_name)
+    client.close()
 
=== added file 'ensemble/control/tests/test_resolved.py'
--- ensemble/control/tests/test_resolved.py	1970-01-01 00:00:00 +0000
+++ ensemble/control/tests/test_resolved.py	2011-05-05 16:55:55 +0000
@@ -0,0 +1,390 @@
+from twisted.internet.defer import inlineCallbacks, returnValue
+from yaml import dump
+
+from ensemble.control import main
+from ensemble.control.resolved import resolved
+from ensemble.control.tests.common import ControlToolTest
+from ensemble.formula.tests.test_repository import RepositoryTestBase
+
+from ensemble.state.service import RETRY_HOOKS, NO_HOOKS
+from ensemble.state.tests.test_service import ServiceStateManagerTestBase
+from ensemble.state.errors import ServiceStateNotFound
+
+from ensemble.unit.workflow import UnitWorkflowState, RelationWorkflowState
+from ensemble.unit.lifecycle import UnitRelationLifecycle
+from ensemble.hooks.executor import HookExecutor
+
+
+class ControlResolvedTest(
+    ServiceStateManagerTestBase, ControlToolTest, RepositoryTestBase):
+
+    @inlineCallbacks
+    def setUp(self):
+        yield super(ControlResolvedTest, self).setUp()
+        config = {
+            "ensemble": "environments",
+            "environments": {"firstenv": {"type": "dummy"}}}
+
+        self.write_config(dump(config))
+        self.config.load()
+
+        yield self.add_relation_state("wordpress", "mysql")
+        yield self.add_relation_state("wordpress", "varnish")
+
+        self.service1 = yield self.service_state_manager.get_service_state(
+            "mysql")
+        self.service_unit1 = yield self.service1.add_unit_state()
+        self.service_unit2 = yield self.service1.add_unit_state()
+
+        self.unit1_workflow = UnitWorkflowState(
+            self.client, self.service_unit1, None, self.makeDir())
+        yield self.unit1_workflow.set_state("started")
+
+        self.environment = self.config.get_default()
+        self.provider = self.environment.get_machine_provider()
+
+        self.output = self.capture_logging()
+        self.stderr = self.capture_stream("stderr")
+        self.executor = HookExecutor()
+
+    @inlineCallbacks
+    def add_relation_state(self, *service_names):
+        for service_name in service_names:
+            try:
+                yield self.service_state_manager.get_service_state(
+                    service_name)
+            except ServiceStateNotFound:
+                yield self.add_service_from_formula(service_name)
+
+        endpoint_pairs = yield self.service_state_manager.join_descriptors(
+            *service_names)
+        endpoints = endpoint_pairs[0]
+        endpoints = endpoint_pairs[0]
+        if endpoints[0] == endpoints[1]:
+            endpoints = endpoints[0:1]
+        relation_state = (yield self.relation_state_manager.add_relation_state(
+            *endpoints))[0]
+        returnValue(relation_state)
+
+    @inlineCallbacks
+    def get_named_service_relation(self, service_state, relation_name):
+        if isinstance(service_state, str):
+            service_state = yield self.service_state_manager.get_service_state(
+                service_state)
+
+        rels = yield self.relation_state_manager.get_relations_for_service(
+            service_state)
+
+        rels = [sr for sr in rels if sr.relation_name == relation_name]
+        if len(rels) == 1:
+            returnValue(rels[0])
+        returnValue(rels)
+
+    @inlineCallbacks
+    def setup_unit_relations(self, service_relation, *units):
+        """
+        Given a service relation and set of unit tuples in the form
+        unit_state, unit_relation_workflow_state, will add unit relations
+        for these units and update their workflow state to the desired/given
+        state.
+        """
+        for unit, state in units:
+            unit_relation = yield service_relation.add_unit_state(unit)
+            lifecycle = UnitRelationLifecycle(
+                self.client, unit_relation, service_relation.relation_name,
+                self.makeDir(), self.executor)
+            workflow_state = RelationWorkflowState(
+                self.client, unit_relation, lifecycle, self.makeDir())
+            yield workflow_state.set_state(state)
+
+    @inlineCallbacks
+    def test_resolved(self):
+        """
+        'ensemble resolved <unit_name>' will schedule a unit for
+        retrying from an error state.
+        """
+        # Push the unit into an error state
+        yield self.unit1_workflow.set_state("start_error")
+        self.setup_exit(0)
+        finished = self.setup_cli_reactor()
+        self.mocker.replay()
+
+        self.assertEqual(
+            (yield self.service_unit1.get_resolved()), None)
+
+        main(["resolved", "mysql/0"])
+        yield finished
+
+        self.assertEqual(
+            (yield self.service_unit1.get_resolved()), {"retry": NO_HOOKS})
+        self.assertIn(
+            "Marked unit 'mysql/0' as resolved",
+            self.output.getvalue())
+
+    @inlineCallbacks
+    def test_resolved_retry(self):
+        """
+        'ensemble resolved --retry <unit_name>' will schedule a unit
+        for retrying from an error state with a retry of hooks
+        executions.
+        """
+        yield self.unit1_workflow.set_state("start_error")
+        self.setup_exit(0)
+        finished = self.setup_cli_reactor()
+        self.mocker.replay()
+
+        self.assertEqual(
+            (yield self.service_unit1.get_resolved()), None)
+
+        main(["resolved", "--retry", "mysql/0"])
+        yield finished
+
+        self.assertEqual(
+            (yield self.service_unit1.get_resolved()), {"retry": RETRY_HOOKS})
+        self.assertIn(
+            "Marked unit 'mysql/0' as resolved",
+            self.output.getvalue())
+
+    @inlineCallbacks
+    def test_relation_resolved(self):
+        """
+        'ensemble relation <unit_name> <rel_name>' will schedule
+        the broken unit relations for being resolved.
+        """
+        service_relation = yield self.get_named_service_relation(
+            self.service1, "server")
+
+        self.setup_unit_relations(
+            service_relation,
+            (self.service_unit1, "down"),
+            (self.service_unit2, "up"))
+
+        yield self.unit1_workflow.set_state("start_error")
+        self.setup_exit(0)
+        finished = self.setup_cli_reactor()
+        self.mocker.replay()
+
+        self.assertEqual(
+            (yield self.service_unit1.get_relation_resolved()), None)
+
+        main(["resolved", "--retry", "mysql/0",
+              service_relation.relation_name])
+        yield finished
+
+        self.assertEqual(
+            (yield self.service_unit1.get_relation_resolved()),
+            {service_relation.internal_relation_id: RETRY_HOOKS})
+        self.assertEqual(
+            (yield self.service_unit2.get_relation_resolved()),
+            None)
+        self.assertIn(
+            "Marked unit 'mysql/0' relation 'server' as resolved",
+            self.output.getvalue())
+
+    @inlineCallbacks
+    def test_resolved_relation_some_already_resolved(self):
+        """
+        'ensemble resolved <service_name> <rel_name>' will mark
+        resolved all down units that are not already marked resolved.
+        """
+
+        service2 = yield self.service_state_manager.get_service_state(
+            "wordpress")
+        service_unit1 = yield service2.add_unit_state()
+
+        service_relation = yield self.get_named_service_relation(
+            service2, "db")
+        yield self.setup_unit_relations(
+            service_relation, (service_unit1, "down"))
+
+        service_relation2 = yield self.get_named_service_relation(
+            service2, "cache")
+        yield self.setup_unit_relations(
+            service_relation2, (service_unit1, "down"))
+
+        yield service_unit1.set_relation_resolved(
+            {service_relation.internal_relation_id: NO_HOOKS})
+
+        self.setup_exit(0)
+        finished = self.setup_cli_reactor()
+        self.mocker.replay()
+
+        main(["resolved", "--retry", "wordpress/0", "cache"])
+        yield finished
+
+        self.assertEqual(
+            (yield service_unit1.get_relation_resolved()),
+            {service_relation.internal_relation_id: NO_HOOKS,
+             service_relation2.internal_relation_id: RETRY_HOOKS})
+
+        self.assertIn(
+            "Marked unit 'wordpress/0' relation 'cache' as resolved",
+            self.output.getvalue())
+
+    @inlineCallbacks
+    def test_resolved_relation_some_already_resolved_conflict(self):
+        """
+        'ensemble resolved <service_name> <rel_name>' will mark
+        resolved all down units that are not already marked resolved.
+        """
+
+        service2 = yield self.service_state_manager.get_service_state(
+            "wordpress")
+        service_unit1 = yield service2.add_unit_state()
+
+        service_relation = yield self.get_named_service_relation(
+            service2, "db")
+        yield self.setup_unit_relations(
+            service_relation, (service_unit1, "down"))
+
+        yield service_unit1.set_relation_resolved(
+            {service_relation.internal_relation_id: NO_HOOKS})
+
+        self.setup_exit(0)
+        finished = self.setup_cli_reactor()
+        self.mocker.replay()
+
+        main(["resolved", "--retry", "wordpress/0", "db"])
+        yield finished
+
+        self.assertEqual(
+            (yield service_unit1.get_relation_resolved()),
+            {service_relation.internal_relation_id: NO_HOOKS})
+
+        self.assertIn(
+            "Service unit 'wordpress/0' already has relations marked as resol",
+            self.output.getvalue())
+
+    @inlineCallbacks
+    def test_resolved_unknown_service(self):
+        """
+        'ensemble resolved <unit_name>' will report if a service is
+        invalid.
+        """
+        self.setup_exit(0)
+        finished = self.setup_cli_reactor()
+        self.mocker.replay()
+        main(["resolved", "zebra/0"])
+        yield finished
+        self.assertIn("Service 'zebra' was not found", self.stderr.getvalue())
+
+    @inlineCallbacks
+    def test_resolved_unknown_unit(self):
+        """
+        'ensemble resolved <unit_name>' will report if a unit is
+        invalid.
+        """
+        self.setup_exit(0)
+        finished = self.setup_cli_reactor()
+        self.mocker.replay()
+        main(["resolved", "mysql/5"])
+        yield finished
+        self.assertIn(
+            "Service unit 'mysql/5' was not found", self.output.getvalue())
+
+    @inlineCallbacks
+    def test_resolved_unknown_unit_relation(self):
+        """
+        'ensemble resolved <unit_name>' will report if a relation is
+        invalid.
+        """
+        self.setup_exit(0)
+        finished = self.setup_cli_reactor()
+        self.mocker.replay()
+
+        self.assertEqual(
+            (yield self.service_unit1.get_resolved()), None)
+
+        main(["resolved", "mysql/0", "magic"])
+        yield finished
+
+        self.assertIn("Relation not found", self.output.getvalue())
+
+    @inlineCallbacks
+    def test_resolved_already_running(self):
+        """
+        'ensemble resolved <unit_name>' will report if
+        the unit is already running.
+        """
+        # Just verify we don't accidentally mark up another unit of the service
+        unit2_workflow = UnitWorkflowState(
+            self.client, self.service_unit2, None, self.makeDir())
+        unit2_workflow.set_state("start_error")
+
+        self.setup_exit(0)
+        finished = self.setup_cli_reactor()
+        self.mocker.replay()
+
+        main(["resolved", "mysql/0"])
+        yield finished
+
+        self.assertEqual(
+            (yield self.service_unit2.get_resolved()), None)
+        self.assertEqual(
+            (yield self.service_unit1.get_resolved()), None)
+
+        self.assertNotIn(
+            "Unit 'mysql/0 already running: started",
+            self.output.getvalue())
+
+    @inlineCallbacks
+    def test_resolved_already_resolved(self):
+        """
+        'ensemble resolved <unit_name>' will report if
+        the unit is already resolved.
+        """
+        # Mark the unit as resolved and as in an error state.
+        yield self.service_unit1.set_resolved(RETRY_HOOKS)
+        yield self.unit1_workflow.set_state("start_error")
+
+        unit2_workflow = UnitWorkflowState(
+            self.client, self.service_unit1, None, self.makeDir())
+        unit2_workflow.set_state("start_error")
+
+        self.assertEqual(
+            (yield self.service_unit2.get_resolved()), None)
+
+        self.setup_exit(0)
+        finished = self.setup_cli_reactor()
+        self.mocker.replay()
+
+        main(["resolved", "mysql/0"])
+        yield finished
+
+        self.assertEqual(
+            (yield self.service_unit1.get_resolved()),
+            {"retry": RETRY_HOOKS})
+        self.assertNotIn(
+            "Marked unit 'mysql/0' as resolved",
+            self.output.getvalue())
+        self.assertIn(
+            "Service unit 'mysql/0' is already marked as resolved.",
+            self.stderr.getvalue(), "")
+
+    @inlineCallbacks
+    def test_resolved_relation_already_running(self):
+        """
+        'ensemble resolved <unit_name> <rel_name>' will report
+        if the relation is already running.
+        """
+        service2 = yield self.service_state_manager.get_service_state(
+            "wordpress")
+        service_unit1 = yield service2.add_unit_state()
+
+        service_relation = yield self.get_named_service_relation(
+            service2, "db")
+        yield self.setup_unit_relations(
+            service_relation, (service_unit1, "up"))
+
+        self.setup_exit(0)
+        finished = self.setup_cli_reactor()
+        self.mocker.replay()
+
+        main(["resolved", "wordpress/0", "db"])
+        yield finished
+
+        self.assertIn("Matched relations are all running",
+                      self.output.getvalue())
+        self.assertEqual(
+            (yield service_unit1.get_relation_resolved()),
+            None)
 
=== modified file 'ensemble/unit/lifecycle.py'
--- ensemble/unit/lifecycle.py	2011-05-05 14:31:41 +0000
+++ ensemble/unit/lifecycle.py	2011-05-05 16:55:55 +0000
@@ -9,7 +9,7 @@
 from ensemble.state.hook import RelationChange
 from ensemble.state.errors import StopWatcher, UnitRelationStateNotFound
 
-from ensemble.unit.workflow import RelationWorkflowState, RelationWorkflow
+from ensemble.unit.workflow import RelationWorkflowState
 
 
 HOOK_SOCKET_FILE = ".ensemble.hookcli.sock"