Merge lp:~hopem/charms/trusty/keystone/fix-db-migrate-race into lp:~openstack-charmers-archive/charms/trusty/keystone/next

[Edward Hope-Morley]

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

charm_lint_check #2042 keystone-next for hopem mp249904
    LINT OK: passed

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

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

charm_unit_test #1832 keystone-next for hopem mp249904
    UNIT OK: passed

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

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

charm_amulet_test #1980 keystone-next for hopem mp249904
    AMULET OK: passed

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

[Liam Young (gnuoy)]

Approve

review: Approve

=== modified file 'hooks/keystone_hooks.py'
--- hooks/keystone_hooks.py	2015-02-16 13:35:14 +0000
+++ hooks/keystone_hooks.py	2015-02-17 00:12:48 +0000
@@ -4,7 +4,6 @@
 import os
 import stat
 import sys
-import time
 
 from subprocess import check_call
 
@@ -72,6 +71,7 @@
     is_ssl_cert_master,
     is_db_ready,
     clear_ssl_synced_units,
+    is_db_initialised,
 )
 
 from charmhelpers.contrib.hahelpers.cluster import (
@@ -198,17 +198,18 @@
             level=INFO)
         return
 
-    try:
-        migrate_database()
-    except Exception as exc:
-        log("Database initialisation failed (%s) - db not ready?" % (exc),
-            level=WARNING)
-    else:
+    if not is_db_initialised():
+        log("Database not yet initialised - deferring identity-relation "
+            "updates", level=INFO)
+        return
+
+    if is_elected_leader(CLUSTER_RES):
         ensure_initial_admin(config)
-        log('Firing identity_changed hook for all related services.')
-        for rid in relation_ids('identity-service'):
-                for unit in related_units(rid):
-                    identity_changed(relation_id=rid, remote_unit=unit)
+
+    log('Firing identity_changed hook for all related services.')
+    for rid in relation_ids('identity-service'):
+            for unit in related_units(rid):
+                identity_changed(relation_id=rid, remote_unit=unit)
 
 
 @synchronize_ca_if_changed(force=True)
@@ -233,8 +234,10 @@
                     level=INFO)
                 return
 
+            migrate_database()
+
             # Ensure any existing service entries are updated in the
-            # new database backend
+            # new database backend. Also avoid duplicate db ready check.
             update_all_identity_relation_units(check_db_ready=False)
 
 
@@ -247,9 +250,15 @@
     else:
         CONFIGS.write(KEYSTONE_CONF)
         if is_elected_leader(CLUSTER_RES):
+            if not is_db_ready(use_current_context=True):
+                log('Allowed_units list provided and this unit not present',
+                    level=INFO)
+                return
+
+            migrate_database()
             # Ensure any existing service entries are updated in the
-            # new database backend
-            update_all_identity_relation_units()
+            # new database backend. Also avoid duplicate db ready check.
+            update_all_identity_relation_units(check_db_ready=False)
 
 
 @hooks.hook('identity-service-relation-changed')
@@ -265,6 +274,11 @@
                 "ready - deferring until db ready", level=WARNING)
             return
 
+        if not is_db_initialised():
+            log("Database not yet initialised - deferring identity-relation "
+                "updates", level=INFO)
+            return
+
         add_service_to_keystone(relation_id, remote_unit)
         settings = relation_get(rid=relation_id, unit=remote_unit)
         service = settings.get('service', None)
@@ -394,7 +408,7 @@
     # NOTE(jamespage) re-echo passwords for peer storage
     echo_whitelist, overrides = \
         apply_echo_filters(settings, ['_passwd', 'identity-service:',
-                                      'ssl-cert-master'])
+                                      'ssl-cert-master', 'db-initialised'])
     log("Peer echo overrides: %s" % (overrides), level=DEBUG)
     relation_set(**overrides)
     if echo_whitelist:
@@ -487,15 +501,8 @@
 
     clustered = relation_get('clustered')
     if clustered and is_elected_leader(CLUSTER_RES):
-        if not is_db_ready():
-                log('Allowed_units list provided and this unit not present',
-                    level=INFO)
-                return
-
-        ensure_initial_admin(config)
         log('Cluster configured, notifying other services and updating '
             'keystone endpoint configuration')
-
         update_all_identity_relation_units()
 
 
@@ -546,7 +553,6 @@
     if is_elected_leader(CLUSTER_RES):
         log('Cluster leader - ensuring endpoint configuration is up to '
             'date', level=DEBUG)
-        time.sleep(10)
         update_all_identity_relation_units()
 
 

=== modified file 'hooks/keystone_utils.py'
--- hooks/keystone_utils.py	2015-02-16 14:48:02 +0000
+++ hooks/keystone_utils.py	2015-02-17 00:12:48 +0000
@@ -314,7 +314,32 @@
     configs.write_all()
 
     if is_elected_leader(CLUSTER_RES):
-        migrate_database()
+        if is_db_ready():
+            migrate_database()
+        else:
+            log("Database not ready - deferring to shared-db relation",
+                level=INFO)
+            return
+
+
+def set_db_initialised():
+    for rid in relation_ids('cluster'):
+        relation_set(relation_settings={'db-initialised': 'True'},
+                     relation_id=rid)
+
+
+def is_db_initialised():
+    for rid in relation_ids('cluster'):
+        units = related_units(rid) + [local_unit()]
+        for unit in units:
+            db_initialised = relation_get(attribute='db-initialised',
+                                          unit=unit, rid=rid)
+            if db_initialised:
+                log("Database is initialised", level=DEBUG)
+                return True
+
+    log("Database is NOT initialised", level=DEBUG)
+    return False
 
 
 def migrate_database():
@@ -328,10 +353,11 @@
     subprocess.check_output(cmd)
     service_start('keystone')
     time.sleep(10)
-
+    set_db_initialised()
 
 # OLD
 
+
 def get_local_endpoint():
     """Returns the URL for the local end-point bypassing haproxy/ssl"""
     if config('prefer-ipv6'):

=== modified file 'unit_tests/test_keystone_hooks.py'
--- unit_tests/test_keystone_hooks.py	2015-02-05 17:48:25 +0000
+++ unit_tests/test_keystone_hooks.py	2015-02-17 00:12:48 +0000
@@ -63,7 +63,6 @@
     'execd_preinstall',
     'mkdir',
     'os',
-    'time',
     # ip
     'get_iface_for_address',
     'get_netmask_for_address',
@@ -203,6 +202,7 @@
         configs.write = MagicMock()
         hooks.pgsql_db_changed()
 
+    @patch.object(hooks, 'is_db_initialised')
     @patch.object(hooks, 'is_db_ready')
     @patch('keystone_utils.log')
     @patch('keystone_utils.ensure_ssl_cert_master')
@@ -210,7 +210,9 @@
     @patch.object(hooks, 'identity_changed')
     def test_db_changed_allowed(self, identity_changed, configs,
                                 mock_ensure_ssl_cert_master,
-                                mock_log, mock_is_db_ready):
+                                mock_log, mock_is_db_ready,
+                                mock_is_db_initialised):
+        mock_is_db_initialised.return_value = True
         mock_is_db_ready.return_value = True
         mock_ensure_ssl_cert_master.return_value = False
         self.relation_ids.return_value = ['identity-service:0']
@@ -247,12 +249,14 @@
 
     @patch('keystone_utils.log')
     @patch('keystone_utils.ensure_ssl_cert_master')
+    @patch.object(hooks, 'is_db_initialised')
     @patch.object(hooks, 'is_db_ready')
     @patch.object(hooks, 'CONFIGS')
     @patch.object(hooks, 'identity_changed')
     def test_postgresql_db_changed(self, identity_changed, configs,
-                                   mock_is_db_ready,
+                                   mock_is_db_ready, mock_is_db_initialised,
                                    mock_ensure_ssl_cert_master, mock_log):
+        mock_is_db_initialised.return_value = True
         mock_is_db_ready.return_value = True
         mock_ensure_ssl_cert_master.return_value = False
         self.relation_ids.return_value = ['identity-service:0']
@@ -269,6 +273,7 @@
 
     @patch('keystone_utils.log')
     @patch('keystone_utils.ensure_ssl_cert_master')
+    @patch.object(hooks, 'is_db_initialised')
     @patch.object(hooks, 'is_db_ready')
     @patch.object(hooks, 'peer_units')
     @patch.object(hooks, 'ensure_permissions')
@@ -283,8 +288,9 @@
             self, configure_https, identity_changed,
             configs, get_homedir, ensure_user, cluster_joined,
             admin_relation_changed, ensure_permissions, mock_peer_units,
-            mock_is_db_ready,
+            mock_is_db_ready, mock_is_db_initialised,
             mock_ensure_ssl_cert_master, mock_log):
+        mock_is_db_initialised.return_value = True
         mock_is_db_ready.return_value = True
         self.openstack_upgrade_available.return_value = False
         self.is_elected_leader.return_value = True
@@ -302,7 +308,6 @@
         configure_https.assert_called_with()
         self.assertTrue(configs.write_all.called)
 
-        self.migrate_database.assert_called_with()
         self.assertTrue(self.ensure_initial_admin.called)
         self.log.assert_called_with(
             'Firing identity_changed hook for all related services.')
@@ -343,6 +348,7 @@
 
     @patch('keystone_utils.log')
     @patch('keystone_utils.ensure_ssl_cert_master')
+    @patch.object(hooks, 'is_db_initialised')
     @patch.object(hooks, 'is_db_ready')
     @patch.object(hooks, 'peer_units')
     @patch.object(hooks, 'ensure_permissions')
@@ -361,9 +367,11 @@
                                                    ensure_permissions,
                                                    mock_peer_units,
                                                    mock_is_db_ready,
+                                                   mock_is_db_initialised,
                                                    mock_ensure_ssl_cert_master,
                                                    mock_log):
         mock_is_db_ready.return_value = True
+        mock_is_db_initialised.return_value = True
         self.openstack_upgrade_available.return_value = True
         self.is_elected_leader.return_value = True
         # avoid having to mock syncer
@@ -382,7 +390,6 @@
         configure_https.assert_called_with()
         self.assertTrue(configs.write_all.called)
 
-        self.migrate_database.assert_called_with()
         self.assertTrue(self.ensure_initial_admin.called)
         self.log.assert_called_with(
             'Firing identity_changed hook for all related services.')
@@ -391,6 +398,7 @@
             remote_unit='unit/0')
         admin_relation_changed.assert_called_with('identity-service:0')
 
+    @patch.object(hooks, 'is_db_initialised')
     @patch.object(hooks, 'is_db_ready')
     @patch('keystone_utils.log')
     @patch('keystone_utils.ensure_ssl_cert_master')
@@ -398,7 +406,9 @@
     @patch.object(hooks, 'send_notifications')
     def test_identity_changed_leader(self, mock_send_notifications,
                                      mock_hashlib, mock_ensure_ssl_cert_master,
-                                     mock_log, mock_is_db_ready):
+                                     mock_log, mock_is_db_ready,
+                                     mock_is_db_initialised):
+        mock_is_db_initialised.return_value = True
         mock_is_db_ready.return_value = True
         mock_ensure_ssl_cert_master.return_value = False
         hooks.identity_changed(
@@ -557,13 +567,16 @@
     @patch('keystone_utils.log')
     @patch('keystone_utils.ensure_ssl_cert_master')
     @patch.object(hooks, 'is_db_ready')
+    @patch.object(hooks, 'is_db_initialised')
     @patch.object(hooks, 'identity_changed')
     @patch.object(hooks, 'CONFIGS')
     def test_ha_relation_changed_clustered_leader(self, configs,
                                                   identity_changed,
+                                                  mock_is_db_initialised,
                                                   mock_is_db_ready,
                                                   mock_ensure_ssl_cert_master,
                                                   mock_log):
+        mock_is_db_initialised.return_value = True
         mock_is_db_ready.return_value = True
         mock_ensure_ssl_cert_master.return_value = False
         self.relation_get.return_value = True
@@ -610,6 +623,8 @@
         cmd = ['a2dissite', 'openstack_https_frontend']
         self.check_call.assert_called_with(cmd)
 
+    @patch.object(hooks, 'is_db_ready')
+    @patch.object(hooks, 'is_db_initialised')
     @patch('keystone_utils.log')
     @patch('keystone_utils.relation_ids')
     @patch('keystone_utils.is_elected_leader')
@@ -623,7 +638,11 @@
                                   mock_ensure_ssl_cert_master,
                                   mock_is_elected_leader,
                                   mock_relation_ids,
-                                  mock_log):
+                                  mock_log,
+                                  mock_is_db_ready,
+                                  mock_is_db_initialised):
+        mock_is_db_initialised.return_value = True
+        mock_is_db_ready.return_value = True
         mock_is_elected_leader.return_value = False
         mock_relation_ids.return_value = []
         mock_ensure_ssl_cert_master.return_value = True