Gate initial update of relations on having reached expected scale

At present the Keystone charm frequently initiates updates to its
relations before it has reached a stable state.

Make use information from ``juju goal-state`` to predict scale and gate
initial update of relations on having reached expected scale.

Depends-On: https://github.com/juju/charm-helpers/pull/226
Change-Id: I96d4aff7c4ec9fb9ea160c7e294581bab3103df8

charmhelpers/contrib/openstack/ha/utils.py

@@ -28,6 +28,7 @@ import json
 import re
 
 from charmhelpers.core.hookenv import (
+    expected_related_units,
     log,
     relation_set,
     charm_name,
@@ -110,12 +111,17 @@ def assert_charm_supports_dns_ha():
 def expect_ha():
     """ Determine if the unit expects to be in HA
 
-    Check for VIP or dns-ha settings which indicate the unit should expect to
+    Check juju goal-state if ha relation is expected, check for VIP or dns-ha
-    be related to hacluster.
+    settings which indicate the unit should expect to be related to hacluster.
 
     @returns boolean
     """
-    return config('vip') or config('dns-ha')
+    ha_related_units = []
+    try:
+        ha_related_units = list(expected_related_units(reltype='ha'))
+    except (NotImplementedError, KeyError):
+        pass
+    return len(ha_related_units) > 0 or config('vip') or config('dns-ha')
 
 
 def generate_ha_relation_data(service):

charmhelpers/core/hookenv.py

@@ -509,6 +509,67 @@ def related_units(relid=None):
         subprocess.check_output(units_cmd_line).decode('UTF-8')) or []
 
 
+def expected_peer_units():
+    """Get a generator for units we expect to join peer relation based on
+    goal-state.
+
+    The local unit is excluded from the result to make it easy to gauge
+    completion of all peers joining the relation with existing hook tools.
+
+    Example usage:
+    log('peer {} of {} joined peer relation'
+        .format(len(related_units()),
+                len(list(expected_peer_units()))))
+
+    This function will raise NotImplementedError if used with juju versions
+    without goal-state support.
+
+    :returns iterator
+    :rtype: types.GeneratorType
+    :raises: NotImplementedError
+    """
+    if not has_juju_version("2.4.0"):
+        # goal-state first appeared in 2.4.0.
+        raise NotImplementedError("goal-state")
+    _goal_state = goal_state()
+    return (key for key in _goal_state['units']
+            if '/' in key and key != local_unit())
+
+
+def expected_related_units(reltype=None):
+    """Get a generator for units we expect to join relation based on
+    goal-state.
+
+    Note that you can not use this function for the peer relation, take a look
+    at expected_peer_units() for that.
+
+    This function will raise KeyError if you request information for a
+    relation type for which juju goal-state does not have information.  It will
+    raise NotImplementedError if used with juju versions without goal-state
+    support.
+
+    Example usage:
+    log('consumer {} of {} joined relation {}'
+        .format(len(related_units()),
+                len(list(expected_related_units())),
+                relation_type()))
+
+    :param reltype: Relation type to list data for, default is to list data for
+                    the realtion type we are currently executing a hook for.
+    :type reltype: str
+    :returns: iterator
+    :rtype: types.GeneratorType
+    :raises: KeyError, NotImplementedError
+    """
+    if not has_juju_version("2.4.4"):
+        # goal-state existed in 2.4.0, but did not list individual units to
+        # join a relation in 2.4.1 through 2.4.3. (LP: #1794739)
+        raise NotImplementedError("goal-state relation unit count")
+    reltype = reltype or relation_type()
+    _goal_state = goal_state()
+    return (key for key in _goal_state['relations'][reltype] if '/' in key)
+
+
 @cached
 def relation_for_unit(unit=None, rid=None):
     """Get the json represenation of a unit's relation"""
@@ -997,6 +1058,7 @@ def application_version_set(version):
 
 
 @translate_exc(from_exc=OSError, to_exc=NotImplementedError)
+@cached
 def goal_state():
     """Juju goal state values"""
     cmd = ['goal-state', '--format=json']

hooks/keystone_hooks.py

@@ -104,6 +104,7 @@ from keystone_utils import (
     send_notifications,
     is_db_ready,
     is_db_initialised,
+    is_expected_scale,
     filter_null,
     is_service_present,
     delete_service_entry,
@@ -310,6 +311,10 @@ def update_all_identity_relation_units(check_db_ready=True):
         log("Database not yet initialised - deferring identity-relation "
             "updates", level=INFO)
         return
+    if not is_expected_scale():
+        log("Keystone charm and it's dependencies not yet at expected scale "
+            "- deferring identity-relation updates", level=INFO)
+        return
 
     log('Firing identity_changed hook for all related services.')
     for rid in relation_ids('identity-service'):

hooks/keystone_utils.py

@@ -40,6 +40,10 @@ from charmhelpers.contrib.network.ip import (
     get_ipv6_addr
 )
 
+from charmhelpers.contrib.openstack.ha.utils import (
+    expect_ha,
+)
+
 from charmhelpers.contrib.openstack.ip import (
     resolve_address,
     PUBLIC,
@@ -73,6 +77,8 @@ from charmhelpers.core.decorators import (
 from charmhelpers.core.hookenv import (
     atexit,
     config,
+    expected_peer_units,
+    expected_related_units,
     is_leader,
     leader_get,
     leader_set,
@@ -2183,3 +2189,41 @@ def fernet_keys_rotate_and_sync(log_func=log):
     key_leader_set()
     log_func("Rotated and started sync (via leader settings) of fernet keys",
              level=INFO)
+
+
+def is_expected_scale():
+    """Query juju goal-state to determine whether our peer- and dependency-
+    relations are at the expected scale.
+
+    Useful for deferring per unit per relation housekeeping work until we are
+    ready to complete it successfully and without unnecessary repetiton.
+
+    Always returns True if version of juju used does not support goal-state.
+
+    :returns: True or False
+    :rtype: bool
+    """
+    peer_type = 'cluster'
+    peer_rid = next((rid for rid in relation_ids(reltype=peer_type)), None)
+    if not peer_rid:
+        return False
+    deps = [
+        ('shared-db',
+         next((rid for rid in relation_ids(reltype='shared-db')), None)),
+    ]
+    if expect_ha():
+        deps.append(('ha',
+                     next((rid for rid in relation_ids(reltype='ha')), None)))
+    try:
+        if (len(related_units(relid=peer_rid)) <
+                len(list(expected_peer_units()))):
+            return False
+        for dep in deps:
+            if not dep[1]:
+                return False
+            if (len(related_units(relid=dep[1])) <
+                    len(list(expected_related_units(reltype=dep[0])))):
+                return False
+    except NotImplementedError:
+        return True
+    return True

unit_tests/test_keystone_hooks.py

@@ -342,6 +342,7 @@ class KeystoneRelationTests(CharmTestCase):
         self.assertTrue(update.called)
         self.assertTrue(mock_update_domains.called)
 
+    @patch.object(hooks, 'is_expected_scale')
     @patch.object(hooks, 'os_release')
     @patch.object(hooks, 'run_in_apache')
     @patch.object(hooks, 'is_db_initialised')
@@ -350,10 +351,12 @@ class KeystoneRelationTests(CharmTestCase):
                                                           config_https,
                                                           mock_db_init,
                                                           mock_run_in_apache,
-                                                          os_release):
+                                                          os_release,
+                                                          is_expected_scale):
         os_release.return_value = 'ocata'
         self.enable_memcache.return_value = False
         mock_run_in_apache.return_value = False
+        is_expected_scale.return_value = True
 
         self.openstack_upgrade_available.return_value = True
         self.test_config.set('action-managed-upgrade', True)
@@ -754,6 +757,7 @@ class KeystoneRelationTests(CharmTestCase):
         self.assertFalse(self.migrate_database.called)
         self.assertFalse(update.called)
 
+    @patch.object(hooks, 'is_expected_scale')
     @patch.object(hooks, 'configure_https')
     @patch.object(hooks, 'admin_relation_changed')
     @patch.object(hooks, 'identity_credentials_changed')
@@ -765,9 +769,11 @@ class KeystoneRelationTests(CharmTestCase):
                                                 identity_changed,
                                                 identity_credentials_changed,
                                                 admin_relation_changed,
-                                                configure_https):
+                                                configure_https,
+                                                is_expected_scale):
         """ Verify all identity relations are updated """
         is_db_initialized.return_value = True
+        is_expected_scale.return_value = True
         self.relation_ids.return_value = ['identity-relation:0']
         self.related_units.return_value = ['unit/0']
         log_calls = [call('Firing identity_changed hook for all related '
@@ -811,26 +817,30 @@ class KeystoneRelationTests(CharmTestCase):
                                     level='INFO')
         self.assertFalse(self.relation_ids.called)
 
+    @patch.object(hooks, 'is_expected_scale')
     @patch.object(hooks, 'configure_https')
     @patch.object(hooks, 'is_db_initialised')
     @patch.object(hooks, 'CONFIGS')
     def test_update_all_leader(self, configs, is_db_initialized,
-                               configure_https):
+                               configure_https, is_expected_scale):
         """ Verify update identity relations when the leader"""
         self.is_elected_leader.return_value = True
         is_db_initialized.return_value = True
+        is_expected_scale.return_value = True
         hooks.update_all_identity_relation_units(check_db_ready=False)
         # Still updates relations
         self.assertTrue(self.relation_ids.called)
 
+    @patch.object(hooks, 'is_expected_scale')
     @patch.object(hooks, 'configure_https')
     @patch.object(hooks, 'is_db_initialised')
     @patch.object(hooks, 'CONFIGS')
     def test_update_all_not_leader(self, configs, is_db_initialized,
-                                   configure_https):
+                                   configure_https, is_expected_scale):
         """ Verify update identity relations when not the leader"""
         self.is_elected_leader.return_value = False
         is_db_initialized.return_value = True
+        is_expected_scale.return_value = True
         hooks.update_all_identity_relation_units(check_db_ready=False)
         self.assertFalse(self.ensure_initial_admin.called)
         # Still updates relations

unit_tests/test_keystone_utils.py

@@ -1283,3 +1283,75 @@ class TestKeystoneUtils(CharmTestCase):
         utils.fernet_keys_rotate_and_sync()
         mock_fernet_rotate.assert_called_once_with()
         mock_key_leader_set.assert_called_once_with()
+
+    @patch.object(utils, 'expected_related_units')
+    @patch.object(utils, 'expected_peer_units')
+    @patch.object(utils, 'related_units')
+    @patch.object(utils, 'expect_ha')
+    @patch.object(utils, 'relation_ids')
+    def test_is_expected_scale(self, relation_ids, expect_ha, related_units,
+                               expected_peer_units, expected_related_units):
+        relation_ids.return_value = ['FAKE_RID']
+        expect_ha.return_value = False
+        related_units.return_value = ['unit/0', 'unit/1', 'unit/2']
+        expected_peer_units.return_value = iter(related_units.return_value)
+        expected_related_units.return_value = iter(related_units.return_value)
+        self.assertTrue(utils.is_expected_scale())
+        relation_ids.assert_has_calls([
+            call(reltype='cluster'),
+            call(reltype='shared-db')])
+        related_units.assert_called_with(relid='FAKE_RID')
+
+    @patch.object(utils, 'expected_related_units')
+    @patch.object(utils, 'expected_peer_units')
+    @patch.object(utils, 'related_units')
+    @patch.object(utils, 'expect_ha')
+    @patch.object(utils, 'relation_ids')
+    def test_is_expected_scale_ha(self, relation_ids, expect_ha, related_units,
+                                  expected_peer_units, expected_related_units):
+        relation_ids.return_value = ['FAKE_RID']
+        expect_ha.return_value = True
+        related_units.return_value = ['unit/0', 'unit/1', 'unit/2']
+        expected_peer_units.return_value = iter(related_units.return_value)
+        expected_related_units.return_value = iter(related_units.return_value)
+        self.assertTrue(utils.is_expected_scale())
+        relation_ids.assert_has_calls([
+            call(reltype='cluster'),
+            call(reltype='shared-db'),
+            call(reltype='ha')])
+        related_units.assert_called_with(relid='FAKE_RID')
+
+    @patch.object(utils, 'expected_related_units')
+    @patch.object(utils, 'expected_peer_units')
+    @patch.object(utils, 'related_units')
+    @patch.object(utils, 'expect_ha')
+    @patch.object(utils, 'relation_ids')
+    def test_not_is_expected_scale(self, relation_ids, expect_ha,
+                                   related_units, expected_peer_units,
+                                   expected_related_units):
+        relation_ids.return_value = ['FAKE_RID']
+        expect_ha.return_value = False
+        related_units.return_value = ['unit/0', 'unit/1']
+        expected_peer_units.return_value = iter(['unit/0', 'unit/1', 'unit/2'])
+        expected_related_units.return_value = iter(
+            ['unit/0', 'unit/1', 'unit/2'])
+        self.assertFalse(utils.is_expected_scale())
+        relation_ids.assert_has_calls([
+            call(reltype='cluster'),
+            call(reltype='shared-db')])
+        related_units.assert_called_with(relid='FAKE_RID')
+
+    @patch.object(utils, 'expected_related_units')
+    @patch.object(utils, 'expected_peer_units')
+    @patch.object(utils, 'related_units')
+    @patch.object(utils, 'expect_ha')
+    @patch.object(utils, 'relation_ids')
+    def test_is_expected_scale_no_goal_state_support(self, relation_ids,
+                                                     expect_ha, related_units,
+                                                     expected_peer_units,
+                                                     expected_related_units):
+        relation_ids.return_value = ['FAKE_RID']
+        related_units.return_value = ['unit/0', 'unit/1', 'unit/2']
+        expected_peer_units.side_effect = NotImplementedError
+        self.assertTrue(utils.is_expected_scale())
+        expected_related_units.assert_not_called()