Merge lp:~hopem/charms/trusty/keystone/fix-db-access-before-ready into lp:~openstack-charmers-archive/charms/trusty/keystone/next

[Edward Hope-Morley]

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #1038 keystone-next for hopem mp247344
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/1038/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #1030 keystone-next for hopem mp247344
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/1030/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #1218 keystone-next for hopem mp247344
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/1218/

[Liam Young (gnuoy)]

Approve

review: Approve

[lp:~hopem/charms/trusty/keystone/fix-db-access-before-ready]

110. By Liam Young on 2015-01-23

[hopem, r=gnuoy] Wait until DB ready before performing Keystone api ops.

=== modified file 'hooks/keystone_hooks.py'
--- hooks/keystone_hooks.py	2015-01-22 14:35:41 +0000
+++ hooks/keystone_hooks.py	2015-01-22 18:46:36 +0000
@@ -2,7 +2,6 @@
 import hashlib
 import json
 import os
-import re
 import stat
 import sys
 import time
@@ -67,6 +66,7 @@
     get_ssl_sync_request_units,
     is_str_true,
     is_ssl_cert_master,
+    is_db_ready,
 )
 
 from charmhelpers.contrib.hahelpers.cluster import (
@@ -219,8 +219,7 @@
             # Bugs 1353135 & 1187508. Dbs can appear to be ready before the
             # units acl entry has been added. So, if the db supports passing
             # a list of permitted units then check if we're in the list.
-            allowed_units = relation_get('allowed_units')
-            if allowed_units and local_unit() not in allowed_units.split():
+            if not is_db_ready(use_current_context=True):
                 log('Allowed_units list provided and this unit not present')
                 return
             # Ensure any existing service entries are updated in the
@@ -249,21 +248,13 @@
 
     notifications = {}
     if is_elected_leader(CLUSTER_RES):
-        # Catch database not configured error and defer until db ready
-        from keystoneclient.apiclient.exceptions import InternalServerError
-        try:
-            add_service_to_keystone(relation_id, remote_unit)
-        except InternalServerError as exc:
-            key = re.compile("'keystone\..+' doesn't exist")
-            if re.search(key, exc.message):
-                log("Keystone database not yet ready (InternalServerError "
-                    "raised) - deferring until *-db relation completes.",
-                    level=WARNING)
-                return
-
-            log("Unexpected exception occurred", level=ERROR)
-            raise
-
+
+        if not is_db_ready():
+            log("identity-service-relation-changed hook fired before db "
+                "ready - deferring until db ready", level=WARNING)
+            return
+
+        add_service_to_keystone(relation_id, remote_unit)
         settings = relation_get(rid=relation_id, unit=remote_unit)
         service = settings.get('service', None)
         if service:

=== modified file 'hooks/keystone_utils.py'
--- hooks/keystone_utils.py	2015-01-22 14:35:41 +0000
+++ hooks/keystone_utils.py	2015-01-22 18:46:36 +0000
@@ -62,6 +62,7 @@
     local_unit,
     relation_get,
     relation_set,
+    relation_id,
     relation_ids,
     related_units,
     DEBUG,
@@ -1356,3 +1357,43 @@
         level=DEBUG)
     for rid in rel_ids:
         relation_set(relation_id=rid, relation_settings=_notifications)
+
+
+def is_db_ready(use_current_context=False, db_rel=None):
+    """Database relations are expected to provide a list of 'allowed' units to
+    confirm that the database is ready for use by those units.
+
+    If db relation has provided this information and local unit is a member,
+    returns True otherwise False.
+    """
+    key = 'allowed_units'
+    db_rels = ['shared-db', 'pgsql-db']
+    if db_rel:
+        db_rels = [db_rel]
+
+    rel_has_units = False
+
+    if use_current_context:
+        if not any([relation_id() in relation_ids(r) for r in db_rels]):
+            raise Exception("use_current_context=True but not in one of %s "
+                            "rel hook contexts (currently in %s)." %
+                            (', '.join(db_rels), relation_id()))
+
+        allowed_units = relation_get(attribute=key)
+        if allowed_units and local_unit() in allowed_units.split():
+            return True
+    else:
+        for rel in db_rels:
+            for rid in relation_ids(rel):
+                for unit in related_units(rid):
+                    allowed_units = relation_get(rid=rid, unit=unit,
+                                                 attribute=key)
+                    if allowed_units and local_unit() in allowed_units.split():
+                        return True
+
+                    # If relation has units
+                    return False
+
+    # If neither relation has units then we are probably in sqllite mode return
+    # True.
+    return not rel_has_units

=== modified file 'unit_tests/test_keystone_hooks.py'
--- unit_tests/test_keystone_hooks.py	2015-01-22 14:35:41 +0000
+++ unit_tests/test_keystone_hooks.py	2015-01-22 18:46:36 +0000
@@ -203,13 +203,15 @@
         configs.write = MagicMock()
         hooks.pgsql_db_changed()
 
+    @patch.object(hooks, 'is_db_ready')
     @patch('keystone_utils.log')
     @patch('keystone_utils.ensure_ssl_cert_master')
     @patch.object(hooks, 'CONFIGS')
     @patch.object(hooks, 'identity_changed')
     def test_db_changed_allowed(self, identity_changed, configs,
                                 mock_ensure_ssl_cert_master,
-                                mock_log):
+                                mock_log, mock_is_db_ready):
+        mock_is_db_ready.return_value = True
         mock_ensure_ssl_cert_master.return_value = False
         self.relation_ids.return_value = ['identity-service:0']
         self.related_units.return_value = ['unit/0']
@@ -223,12 +225,15 @@
             relation_id='identity-service:0',
             remote_unit='unit/0')
 
+    @patch.object(hooks, 'is_db_ready')
     @patch('keystone_utils.log')
     @patch('keystone_utils.ensure_ssl_cert_master')
     @patch.object(hooks, 'CONFIGS')
     @patch.object(hooks, 'identity_changed')
     def test_db_changed_not_allowed(self, identity_changed, configs,
-                                    mock_ensure_ssl_cert_master, mock_log):
+                                    mock_ensure_ssl_cert_master, mock_log,
+                                    mock_is_db_ready):
+        mock_is_db_ready.return_value = False
         mock_ensure_ssl_cert_master.return_value = False
         self.relation_ids.return_value = ['identity-service:0']
         self.related_units.return_value = ['unit/0']
@@ -374,13 +379,15 @@
             remote_unit='unit/0')
         admin_relation_changed.assert_called_with('identity-service:0')
 
+    @patch.object(hooks, 'is_db_ready')
     @patch('keystone_utils.log')
     @patch('keystone_utils.ensure_ssl_cert_master')
     @patch.object(hooks, 'hashlib')
     @patch.object(hooks, 'send_notifications')
     def test_identity_changed_leader(self, mock_send_notifications,
                                      mock_hashlib, mock_ensure_ssl_cert_master,
-                                     mock_log):
+                                     mock_log, mock_is_db_ready):
+        mock_is_db_ready.return_value = True
         mock_ensure_ssl_cert_master.return_value = False
         hooks.identity_changed(
             relation_id='identity-service:0',

=== modified file 'unit_tests/test_keystone_utils.py'
--- unit_tests/test_keystone_utils.py	2015-01-16 14:50:19 +0000
+++ unit_tests/test_keystone_utils.py	2015-01-22 18:46:36 +0000
@@ -33,6 +33,10 @@
     'service_start',
     'relation_get',
     'relation_set',
+    'relation_ids',
+    'relation_id',
+    'local_unit',
+    'related_units',
     'https',
     'is_relation_made',
     'peer_store',
@@ -345,3 +349,23 @@
         isfile.return_value = False
         self.subprocess.check_output.return_value = 'supersecretgen'
         self.assertEqual(utils.get_admin_passwd(), 'supersecretgen')
+
+    def test_is_db_ready(self):
+        self.relation_id.return_value = 'shared-db:0'
+        self.relation_ids.return_value = [self.relation_id.return_value]
+        self.local_unit.return_value = 'unit/0'
+        self.relation_get.return_value = 'unit/0'
+        self.assertTrue(utils.is_db_ready(use_current_context=True))
+
+        self.relation_ids.return_value = ['acme:0']
+        self.assertRaises(utils.is_db_ready, use_current_context=True)
+
+        self.related_units.return_value = ['unit/0']
+        self.relation_ids.return_value = [self.relation_id.return_value]
+        self.assertTrue(utils.is_db_ready())
+
+        self.relation_get.return_value = 'unit/1'
+        self.assertFalse(utils.is_db_ready())
+
+        self.related_units.return_value = []
+        self.assertTrue(utils.is_db_ready())