Merge lp:~bcsaller/pyjuju/subordinate-implicit-interfaces into lp:pyjuju

[Benjamin Saller]

Implict relations

Adds support for implicit relations. These changes are largely independent of the other subordinate work
though that work depends on this. Following this merge implicit relations (juju-info) are provided by
all services.

https://codereview.appspot.com/5645061/

Manually setting prereq branch

[Benjamin Saller (bcsaller)]

Reviewers: mp+92195_code.launchpad.net,

Message:
Please take a look.

Description:
Adds support for implicit relations. These changes are largely
independent of the other subordinate work
though that work depends on this. Following this merge implicit
relations (juju-info) are provided by
all services.

https://code.launchpad.net/~bcsaller/juju/subordinate-implicit-interfaces/+merge/92195

(do not edit description out of merge proposal)

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

Affected files:
   M .bzrignore
   M docs/source/charm-upgrades.rst
   M docs/source/charm.rst
   A docs/source/drafts/implicit-relations.rst
   A docs/source/drafts/subordinate-internals.rst
   A docs/source/drafts/subordinate-services.rst
   M juju/charm/metadata.py
   A juju/charm/tests/repository/series/logging/.ignored
   A juju/charm/tests/repository/series/logging/hooks/install
   A juju/charm/tests/repository/series/logging/metadata.yaml
   A juju/charm/tests/repository/series/logging/revision
   M juju/charm/tests/test_metadata.py
   M juju/control/add_relation.py
   M juju/control/tests/test_add_relation.py
   M juju/state/service.py
   M juju/state/tests/test_service.py

[Jim Baker (jimbaker)]

+1 LGTM! I should not have waited on the CR in Reitveld (which still is not taking in account the prereq), this is a very small change.

review: Approve

[Kapil Thangavelu (hazmat)]

The error messages here are inscrutable to a user, the notion of 'namespaces' isn't documented.

[Benjamin Saller (bcsaller)]

I'll fix this in a later branch

On Sat, Mar 3, 2012 at 7:25 PM, Kapil Thangavelu
<email address hidden> wrote:
> The error messages here are inscrutable to a user, the notion of 'namespaces' isn't documented.
> --
> https://code.launchpad.net/~bcsaller/juju/subordinate-implicit-interfaces/+merge/92333
> You are the owner of lp:~bcsaller/juju/subordinate-implicit-interfaces.

=== modified file 'juju/charm/metadata.py'
--- juju/charm/metadata.py	2012-02-09 21:13:21 +0000
+++ juju/charm/metadata.py	2012-02-09 21:13:21 +0000
@@ -72,7 +72,7 @@
             return {
                 "interface": UTF8_SCHEMA.coerce(value, path),
                 "limit": self.limit,
-                "scope": "global",
+                "scope": SCOPE_GLOBAL,
                 "optional": False}
         else:
             # Optional values are context-sensitive and/or have
@@ -83,7 +83,7 @@
                 value["limit"] = self.limit
             if "optional" not in value:
                 value["optional"] = False
-            value["scope"] = value.get("scope", "global")
+            value["scope"] = value.get("scope", SCOPE_GLOBAL)
             return INTERFACE_SCHEMA.coerce(value, path)
 
 
@@ -169,7 +169,7 @@
             return False
 
         for relation_data in self.requires.values():
-            if relation_data.get("scope") == "container":
+            if relation_data.get("scope") == SCOPE_CONTAINER:
                 return True
         return False
 
@@ -216,11 +216,23 @@
                 "%s: revision field is obsolete. Move it to the 'revision' "
                 "file." % path)
 
+        if self.provides:
+            for rel in self.provides:
+                if rel.startswith("juju-"):
+                    raise MetaDataError(
+                        "Charm %s attempting to provide relation in implicit relation namespace: %s" %
+                        (self.name, rel))
+                interface = self.provides[rel]["interface"]
+                if interface.startswith("juju-"):
+                    raise MetaDataError(
+                        "Charm %s attempting to provide interface in implicit namespace: "
+                        "%s (relation: %s)" % (self.name, interface, rel))
+
         if self._data.get("subordinate", False) is True:
             proper_subordinate = False
             if self.requires:
                 for relation_data in self.requires.values():
-                    if relation_data.get("scope") == "container":
+                    if relation_data.get("scope") == SCOPE_CONTAINER:
                         proper_subordinate = True
             if not proper_subordinate:
                 log.warning(

=== modified file 'juju/charm/tests/repository/series/logging/metadata.yaml'
--- juju/charm/tests/repository/series/logging/metadata.yaml	2012-02-09 21:13:21 +0000
+++ juju/charm/tests/repository/series/logging/metadata.yaml	2012-02-09 21:13:21 +0000
@@ -11,3 +11,6 @@
     logging-directory:
        interface: logging
        scope: container
+    juju-info-fallback:
+        interface: juju-info
+        scope: container
\ No newline at end of file

=== modified file 'juju/charm/tests/test_metadata.py'
--- juju/charm/tests/test_metadata.py	2012-02-09 21:13:21 +0000
+++ juju/charm/tests/test_metadata.py	2012-02-09 21:13:21 +0000
@@ -218,6 +218,27 @@
         serialization_data = self.metadata.get_serialization_data()
         self.assertEquals(serialization_data["name"], "dummy")
 
+    def test_provide_implicit_relation(self):
+        """Verify providing a juju-* reserved relation errors"""
+        with self.change_sample() as data:
+            data["provides"] = {"juju-foo": {"interface": "juju-magic", "scope": "container"}}
+
+        # verify relation level error
+        error = self.assertRaises(MetaDataError,
+                                  self.metadata.parse, self.sample)
+        self.assertIn("Charm dummy attempting to provide relation in implicit relation namespace: juju-foo",
+                      str(error))
+
+        # verify interface level error
+        with self.change_sample() as data:
+            data["provides"] = {"foo-rel": {"interface": "juju-magic", "scope": "container"}}
+
+        error = self.assertRaises(MetaDataError,
+                                  self.metadata.parse, self.sample)
+        self.assertIn(
+            "Charm dummy attempting to provide interface in implicit namespace: juju-magic (relation: foo-rel)",
+            str(error))
+
 
 class ParseTest(TestCase):
     """Test the parsing of some well-known sample files"""

=== modified file 'juju/control/add_relation.py'
--- juju/control/add_relation.py	2011-10-07 12:37:03 +0000
+++ juju/control/add_relation.py	2012-02-09 21:13:21 +0000
@@ -50,7 +50,10 @@
     if len(endpoint_pairs) == 0:
         raise NoMatchingEndpoints()
     elif len(endpoint_pairs) > 1:
-        raise AmbiguousRelation(descriptors, endpoint_pairs)
+        for pair in endpoint_pairs[1:]:
+            if not (pair[0].relation_name.startswith("juju-") or
+                    pair[1].relation_name.startswith("juju-")):
+                raise AmbiguousRelation(descriptors, endpoint_pairs)
 
     # At this point we just have one endpoint pair. We need to pick
     # just one of the endpoints if it's a peer endpoint, since that's

=== modified file 'juju/control/tests/test_add_relation.py'
--- juju/control/tests/test_add_relation.py	2012-01-23 23:17:03 +0000
+++ juju/control/tests/test_add_relation.py	2012-02-09 21:13:21 +0000
@@ -95,7 +95,7 @@
 
     @inlineCallbacks
     def test_add_relation_multiple(self):
-        """Test that the command can be create multiple relations."""
+        """Test that the command can be used to create multiple relations."""
         environment = self.config.get("firstenv")
         yield self.add_service_from_charm("mysql")
         yield self.add_service_from_charm("wordpress")
@@ -217,3 +217,17 @@
         self.assertIn(
             "Invalid environment 'roman-candle'",
             self.stderr.getvalue())
+
+    @inlineCallbacks
+    def test_relate_to_implicit(self):
+        """Validate we can implicitly relate to an implicitly provided relation"""
+        wait_on_reactor_stopped = self.setup_cli_reactor()
+        self.setup_exit(0)
+        self.mocker.replay()
+        yield self.add_service_from_charm("mysql")
+        yield self.add_service_from_charm("logging")
+        main(["add-relation", "mysql", "logging"])
+        yield wait_on_reactor_stopped
+        self.assertIn(
+            "Added juju-info relation to all service units.",
+            self.output.getvalue())

=== modified file 'juju/state/service.py'
--- juju/state/service.py	2012-02-04 00:01:06 +0000
+++ juju/state/service.py	2012-02-09 21:13:21 +0000
@@ -140,13 +140,15 @@
         A `descriptor` is of the form ``<service name>[:<relation name>]``.
         Returns the following:
 
-          - Returns a set of matching endpoints, drawn from the peers,
-            provides, and requires interfaces. The empty set is
+          - Returns a list of matching endpoints, drawn from the
+            peers, provides, and requires interfaces. An empty list is
             returned if there are no endpoints matching the
-            `descriptor`.
+            `descriptor`. This list is sorted such that implicit
+            relations appear last.
 
           - Raises a `BadDescriptor` exception if `descriptor` cannot
             be parsed.
+
         """
         tokens = descriptor.split(":")
         if len(tokens) == 1 and bool(tokens[0]):
@@ -175,16 +177,35 @@
                             relation_type=spec["interface"],
                             relation_name=relation_name,
                             relation_role=relation_role))
+
+        # Add in implicit relations
+        endpoints.add(
+            RelationEndpoint(
+                query_service_name, "juju-info", "juju-info", "server"))
+
+        # when offering matches implicit relations should be considered last.
+        # this cmpfunc pushes implicit methods to the end of the list of possible
+        # endpoints
+        def low_priority_implicit_cmp(rel1, rel2):
+            if rel1.relation_name.startswith("juju-"):
+                return 1
+            if rel2.relation_name.startswith("juju-"):
+                return -1
+            return cmp(rel1.relation_name, rel2.relation_name)
+
+        endpoints = sorted(endpoints, low_priority_implicit_cmp)
+
         returnValue(endpoints)
 
     @inlineCallbacks
     def join_descriptors(self, descriptor1, descriptor2):
         """Return a list of pairs of RelationEndpoints joining descriptors."""
         result = []
-        relation_set1 = yield self.get_relation_endpoints(descriptor1)
-        relation_set2 = yield self.get_relation_endpoints(descriptor2)
-        for relation1 in relation_set1:
-            for relation2 in relation_set2:
+        relations_1 = yield self.get_relation_endpoints(descriptor1)
+        relations_2 = yield self.get_relation_endpoints(descriptor2)
+
+        for relation1 in relations_1:
+            for relation2 in relations_2:
                 if relation1.may_relate_to(relation2):
                     result.append((relation1, relation2))
         returnValue(result)

=== modified file 'juju/state/tests/test_service.py'
--- juju/state/tests/test_service.py	2012-02-04 00:00:08 +0000
+++ juju/state/tests/test_service.py	2012-02-09 21:13:21 +0000
@@ -1642,17 +1642,18 @@
         self.assertEqual(
             (yield self.service_state_manager.get_relation_endpoints(
             "wordpress")),
-            set([RelationEndpoint("wordpress", "http", "url", "server"),
-                 RelationEndpoint("wordpress", "mysql", "db", "client"),
-                 RelationEndpoint(
-                     "wordpress", "varnish", "cache", "client")]))
+            [ RelationEndpoint("wordpress", "varnish", "cache", "client"),
+            RelationEndpoint("wordpress", "mysql", "db", "client"),
+            RelationEndpoint("wordpress", "http", "url", "server"),
+            RelationEndpoint("wordpress", "juju-info", "juju-info", "server")])
         yield self.add_service_from_charm("riak")
         self.assertEqual(
             (yield self.service_state_manager.get_relation_endpoints(
             "riak")),
-            set([RelationEndpoint("riak", "http", "admin", "server"),
-                 RelationEndpoint("riak", "http", "endpoint", "server"),
-                 RelationEndpoint("riak", "riak", "ring", "peer")]))
+            [RelationEndpoint("riak", "http", "admin", "server"),
+            RelationEndpoint("riak", "http", "endpoint", "server"),
+            RelationEndpoint("riak", "riak", "ring", "peer"),
+            RelationEndpoint("riak", "juju-info", "juju-info", "server")])
 
     @inlineCallbacks
     def test_get_relation_endpoints_service_name_relation_name(self):
@@ -1661,34 +1662,41 @@
         self.assertEqual(
             (yield self.service_state_manager.get_relation_endpoints(
             "wordpress:url")),
-            set([RelationEndpoint("wordpress", "http", "url", "server")]))
+            [RelationEndpoint("wordpress", "http", "url", "server"),
+            RelationEndpoint("wordpress", "juju-info", "juju-info", "server")])
         self.assertEqual(
             (yield self.service_state_manager.get_relation_endpoints(
             "wordpress:db")),
-            set([RelationEndpoint("wordpress", "mysql", "db", "client")]))
+            [RelationEndpoint("wordpress", "mysql", "db", "client"),
+            RelationEndpoint("wordpress", "juju-info", "juju-info", "server")])
         self.assertEqual(
             (yield self.service_state_manager.get_relation_endpoints(
             "wordpress:cache")),
-             set([RelationEndpoint(
-                "wordpress", "varnish", "cache", "client")]))
+            [RelationEndpoint("wordpress", "varnish", "cache", "client"),
+            RelationEndpoint("wordpress", "juju-info", "juju-info", "server")])
         yield self.add_service_from_charm("riak")
         self.assertEqual(
             (yield self.service_state_manager.get_relation_endpoints(
             "riak:ring")),
-            set([RelationEndpoint("riak", "riak", "ring", "peer")]))
+            [RelationEndpoint("riak", "riak", "ring", "peer"),
+            RelationEndpoint("riak", "juju-info", "juju-info", "server")])
 
     @inlineCallbacks
     def test_descriptor_for_services_without_charms(self):
         """Test with services that have no corresponding charms defined"""
         yield self.add_service("nocharm")
+        # verify we get the implicit interface
         self.assertEqual(
             (yield self.service_state_manager.get_relation_endpoints(
                 "nocharm")),
-            set())
+                [RelationEndpoint("nocharm",
+                "juju-info", "juju-info", "server")])
+
         self.assertEqual(
             (yield self.service_state_manager.get_relation_endpoints(
                 "nocharm:nonsense")),
-            set())
+                [RelationEndpoint("nocharm",
+                "juju-info", "juju-info", "server")])
 
     @inlineCallbacks
     def test_descriptor_for_missing_service(self):