Merge lp:~mbruzek/charm-helpers/peers into lp:charm-helpers

[Matt Bruzek]

While writing a layer charm that uses reactive and the peers relation we encountered a problem. The peers relation handlers were not called.

With Cory's help we tracked this down to hookenv.py where the code is looking for "peer" and not "peers".

This merge corrects the hookenv.py and the coordination.py file's use of "peers".

The test_hookenv.py originally failed after I made the change so I fixed the test to have the correct "peers" relation in the examples.

[Marco Ceppi (marcoceppi)]

+1

review: Approve

=== modified file 'charmhelpers/coordinator.py'
--- charmhelpers/coordinator.py	2015-06-19 07:26:06 +0000
+++ charmhelpers/coordinator.py	2015-12-10 22:46:25 +0000
@@ -29,10 +29,10 @@
 Services Framework Usage
 ========================
 
-Ensure a peer relation is defined in metadata.yaml. Instantiate a
+Ensure a peers relation is defined in metadata.yaml. Instantiate a
 BaseCoordinator subclass before invoking ServiceManager.manage().
 Ensure that ServiceManager.manage() is wired up to the leader-elected,
-leader-settings-changed, peer relation-changed and peer
+leader-settings-changed, peers relation-changed and peers
 relation-departed hooks in addition to any other hooks you need, or your
 service will deadlock.
 
@@ -90,7 +90,7 @@
 Traditional Usage
 =================
 
-Ensure a peer relationis defined in metadata.yaml.
+Ensure a peers relation is defined in metadata.yaml.
 
 If you are using charmhelpers.core.hookenv.Hooks, ensure that a
 BaseCoordinator subclass is instantiated before calling Hooks.execute.
@@ -151,7 +151,7 @@
         hookenv.service_restart('myservice')
 
     @hooks.hook('install', 'config-changed', 'upgrade-charm',
-                # Peer and leader hooks must be wired up.
+                # Peers and leader hooks must be wired up.
                 'cluster-relation-changed', 'cluster-relation-departed',
                 'leader-elected', 'leader-settings-changed')
     def default_hook():
@@ -174,7 +174,7 @@
 Locks are released at the end of the hook they are acquired in. This may
 be the current hook if the unit is leader and the lock is free. It is
 more likely a future hook (probably leader-settings-changed, possibly
-the peer relation-changed or departed hook, potentially any hook).
+the peers relation-changed or departed hook, potentially any hook).
 
 Whenever a charm needs to perform a coordinated action it will acquire()
 the lock and perform the action immediately if acquisition is
@@ -189,16 +189,16 @@
 If the unit is the leader, then it may be able to grant its own lock
 and perform the action immediately in the source hook. If the unit is
 the leader and cannot immediately grant the lock, then its only
-guaranteed chance of acquiring the lock is in the peer relation-joined,
-relation-changed or peer relation-departed hooks when another unit has
-released it (the only channel to communicate to the leader is the peer
+guaranteed chance of acquiring the lock is in the peers relation-joined,
+relation-changed or peers relation-departed hooks when another unit has
+released it (the only channel to communicate to the leader is the peers
 relation). If the unit is not the leader, then it is unlikely the lock
 is granted in the source hook (a previous hook must have also made the
 request for this to happen). A non-leader is notified about the lock via
 leader settings. These changes may be visible in any hook, even before
 the leader-settings-changed hook has been invoked. Or the requesting
 unit may be promoted to leader after making a request, in which case the
-lock may be granted in leader-elected or in a future peer
+lock may be granted in leader-elected or in a future peers
 relation-changed or relation-departed hook.
 
 This could be simpler if leader-settings-changed was invoked on the
@@ -255,10 +255,10 @@
     def __init__(self, relation_key='coordinator', peer_relation_name=None):
         '''Instatiate a Coordinator.
 
-        Data is stored on the peer relation and in leadership storage
+        Data is stored on the peers relation and in leadership storage
         under the provided relation_key.
 
-        The peer relation is identified by peer_relation_name, and defaults
+        The peers relation is identified by peer_relation_name, and defaults
         to the first one found in metadata.yaml.
         '''
         # Most initialization is deferred, since invoking hook tools from
@@ -310,13 +310,13 @@
 
         Do not mindlessly call this method, as it triggers a cascade of
         hooks. For example, if you call acquire() every time in your
-        peer relation-changed hook you will end up with an infinite loop
+        peers relation-changed hook you will end up with an infinite loop
         of hooks. It should almost always be guarded by some condition.
         '''
         unit = hookenv.local_unit()
         ts = self.requests[unit].get(lock)
         if not ts:
-            # If there is no outstanding request on the peer relation,
+            # If there is no outstanding request on the peers relation,
             # create one.
             self.requests.setdefault(lock, {})
             self.requests[unit][lock] = _timestamp()
@@ -329,7 +329,7 @@
 
         # If the unit making the request also happens to be the
         # leader, it must handle the request now. Even though the
-        # request has been stored on the peer relation, the peer
+        # request has been stored on the peers relation, the peers
         # relation-changed hook will not be triggered.
         if hookenv.is_leader():
             return self.grant(lock, unit)
@@ -476,13 +476,13 @@
 
         local_unit = hookenv.local_unit()
 
-        # All requests must be stored on the peer relation. This is
+        # All requests must be stored on the peers relation. This is
         # the only channel units have to communicate with the leader.
         # Even the leader needs to store its requests here, as a
         # different unit may be leader by the time the request can be
         # granted.
         if self.relid is None:
-            # The peer relation is not available. Maybe we are early in
+            # The peers relation is not available. Maybe we are early in
             # the units's lifecycle. Maybe this unit is standalone.
             # Fallback to using local state.
             self.msg('No peer relation. Loading local state')
@@ -490,7 +490,7 @@
         else:
             self.requests = self._load_peer_state()
             if local_unit not in self.requests:
-                # The peer relation has just been joined. Update any state
+                # The peers relation has just been joined. Update any state
                 # loaded from our peers with our local state.
                 self.msg('New peer relation. Merging local state')
                 self.requests[local_unit] = self._load_local_state()
@@ -513,7 +513,7 @@
         local_unit = hookenv.local_unit()
 
         if self.relid is None:
-            # No peer relation yet. Fallback to local state.
+            # No peers relation yet. Fallback to local state.
             self.msg('No peer relation. Saving local state')
             self._save_local_state(self.requests[local_unit])
         else:

=== modified file 'charmhelpers/core/hookenv.py'
--- charmhelpers/core/hookenv.py	2015-12-08 17:47:29 +0000
+++ charmhelpers/core/hookenv.py	2015-12-10 22:46:25 +0000
@@ -492,7 +492,7 @@
 
 @cached
 def peer_relation_id():
-    '''Get a peer relation id if a peer relation has been joined, else None.'''
+    '''Get the peers relation id if a peers relation has been joined, else None.'''
     md = metadata()
     section = md.get('peers')
     if section:
@@ -517,12 +517,12 @@
 def relation_to_role_and_interface(relation_name):
     """
     Given the name of a relation, return the role and the name of the interface
-    that relation uses (where role is one of ``provides``, ``requires``, or ``peer``).
+    that relation uses (where role is one of ``provides``, ``requires``, or ``peers``).
 
     :returns: A tuple containing ``(role, interface)``, or ``(None, None)``.
     """
     _metadata = metadata()
-    for role in ('provides', 'requires', 'peer'):
+    for role in ('provides', 'requires', 'peers'):
         interface = _metadata.get(role, {}).get(relation_name, {}).get('interface')
         if interface:
             return role, interface
@@ -534,7 +534,7 @@
     """
     Given a role and interface name, return a list of relation names for the
     current charm that use that interface under that role (where role is one
-    of ``provides``, ``requires``, or ``peer``).
+    of ``provides``, ``requires``, or ``peers``).
 
     :returns: A list of relation names.
     """
@@ -555,7 +555,7 @@
     :returns: A list of relation names.
     """
     results = []
-    for role in ('provides', 'requires', 'peer'):
+    for role in ('provides', 'requires', 'peers'):
         results.extend(role_and_interface_to_relations(role, interface_name))
     return results
 

=== modified file 'tests/core/test_hookenv.py'
--- tests/core/test_hookenv.py	2015-12-08 17:47:29 +0000
+++ tests/core/test_hookenv.py	2015-12-10 22:46:25 +0000
@@ -1448,7 +1448,7 @@
                     'interface': 'req-int',
                 },
             },
-            'peer': {
+            'peers': {
                 'pee-rel': {
                     'interface': 'pee-int',
                 },
@@ -1457,7 +1457,7 @@
         rtri = hookenv.relation_to_role_and_interface
         self.assertEqual(rtri('pro-rel'), ('provides', 'pro-int'))
         self.assertEqual(rtri('req-rel'), ('requires', 'req-int'))
-        self.assertEqual(rtri('pee-rel'), ('peer', 'pee-int'))
+        self.assertEqual(rtri('pee-rel'), ('peers', 'pee-int'))
 
     @patch.object(hookenv, 'metadata')
     def test_role_and_interface_to_relations(self, metadata):
@@ -1475,7 +1475,7 @@
                     'interface': 'int',
                 },
             },
-            'peer': {
+            'peers': {
                 'pee-rel': {
                     'interface': 'int',
                 },
@@ -1485,7 +1485,7 @@
         assertItemsEqual = getattr(self, 'assertItemsEqual', getattr(self, 'assertCountEqual', None))
         assertItemsEqual(ritr('provides', 'pro-int'), ['pro-rel', 'pro-rel2'])
         assertItemsEqual(ritr('requires', 'int'), ['req-rel'])
-        assertItemsEqual(ritr('peer', 'int'), ['pee-rel'])
+        assertItemsEqual(ritr('peers', 'int'), ['pee-rel'])
 
     @patch.object(hookenv, 'metadata')
     def test_interface_to_relations(self, metadata):
@@ -1503,7 +1503,7 @@
                     'interface': 'req-int',
                 },
             },
-            'peer': {
+            'peers': {
                 'pee-rel': {
                     'interface': 'pee-int',
                 },