[hopem, r=gnuoy] Fix upgrade breakage whereby if upgrading from

version of charm that did not support
db-initialised peer setting db ops get stuck
waiting infinitely for db to be intialised.

Closes-Bug: 1519035

hooks/keystone_hooks.py

@@ -271,6 +271,33 @@ def update_all_identity_relation_units_force_sync():
     update_all_identity_relation_units()
 
 
+def leader_init_db_if_ready(use_current_context=False):
+    """ Initialise the keystone db if it is ready and mark it as initialised.
+
+    NOTE: this must be idempotent.
+    """
+    if not is_elected_leader(CLUSTER_RES):
+        log("Not leader - skipping db init", level=DEBUG)
+        return
+
+    if is_db_initialised():
+        log("Database already initialised - skipping db init", level=DEBUG)
+        return
+
+    # 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.
+    if not is_db_ready(use_current_context=use_current_context):
+        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. Also avoid duplicate db ready check.
+    update_all_identity_relation_units(check_db_ready=False)
+
+
 @hooks.hook('shared-db-relation-changed')
 @restart_on_change(restart_map())
 @synchronize_ca_if_changed()
@@ -279,19 +306,7 @@ def db_changed():
         log('shared-db relation incomplete. Peer not ready?')
     else:
         CONFIGS.write(KEYSTONE_CONF)
-        if is_elected_leader(CLUSTER_RES):
-            # 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.
-            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. Also avoid duplicate db ready check.
-            update_all_identity_relation_units(check_db_ready=False)
+        leader_init_db_if_ready(use_current_context=True)
 
 
 @hooks.hook('pgsql-db-relation-changed')
@@ -302,16 +317,7 @@ def pgsql_db_changed():
         log('pgsql-db relation incomplete. Peer not ready?')
     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. Also avoid duplicate db ready check.
-            update_all_identity_relation_units(check_db_ready=False)
+        leader_init_db_if_ready(use_current_context=True)
 
 
 @hooks.hook('identity-service-relation-changed')
@@ -612,6 +618,10 @@ def upgrade_charm():
     ensure_ssl_dirs()
 
     CONFIGS.write_all()
+
+    # See LP bug 1519035
+    leader_init_db_if_ready()
+
     update_nrpe_config()
 
     if is_elected_leader(CLUSTER_RES):

unit_tests/test_keystone_hooks.py

@@ -60,7 +60,6 @@
     'synchronize_ca_if_changed',
     'update_nrpe_config',
     'ensure_ssl_dirs',
-    'is_db_initialised',
     'is_db_ready',
     # other
     'check_call',
@@ -246,14 +245,30 @@ def _postgresql_db_test(self, configs):
         configs.write = MagicMock()
         hooks.pgsql_db_changed()
 
+    @patch('keystone_utils.relation_ids')
+    @patch('keystone_utils.peer_retrieve')
+    @patch('keystone_utils.peer_store')
     @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):
-        self.is_db_initialised.return_value = True
+                                mock_ensure_ssl_cert_master, mock_log,
+                                mock_peer_store,
+                                mock_peer_retrieve, mock_relation_ids):
+        mock_relation_ids.return_value = ['peer/0']
+        peer_settings = {}
+
+        def fake_peer_store(key, val):
+            peer_settings[key] = val
+
+        def fake_migrate():
+            fake_peer_store('db-initialised', 'True')
+
+        self.migrate_database.side_effect = fake_migrate
+        mock_peer_store.side_effect = fake_peer_store
+        mock_peer_retrieve.side_effect = lambda key: peer_settings.get(key)
+
         self.is_db_ready.return_value = True
         mock_ensure_ssl_cert_master.return_value = False
         self.relation_ids.return_value = ['identity-service:0']
@@ -268,12 +283,15 @@ def test_db_changed_allowed(self, identity_changed, configs,
             relation_id='identity-service:0',
             remote_unit='unit/0')
 
+    @patch('keystone_utils.relation_ids')
     @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_relation_ids):
+        mock_relation_ids.return_value = []
         self.is_db_ready.return_value = False
         mock_ensure_ssl_cert_master.return_value = False
         self.relation_ids.return_value = ['identity-service:0']
@@ -286,13 +304,31 @@ def test_db_changed_not_allowed(self, identity_changed, configs,
         self.assertFalse(self.ensure_initial_admin.called)
         self.assertFalse(identity_changed.called)
 
+    @patch('keystone_utils.relation_ids')
+    @patch('keystone_utils.peer_retrieve')
+    @patch('keystone_utils.peer_store')
     @patch('keystone_utils.log')
     @patch('keystone_utils.ensure_ssl_cert_master')
     @patch.object(hooks, 'CONFIGS')
     @patch.object(hooks, 'identity_changed')
     def test_postgresql_db_changed(self, identity_changed, configs,
-                                   mock_ensure_ssl_cert_master, mock_log):
-        self.is_db_initialised.return_value = True
+                                   mock_ensure_ssl_cert_master, mock_log,
+                                   mock_peer_store, mock_peer_retrieve,
+                                   mock_relation_ids):
+        mock_relation_ids.return_value = ['peer/0']
+
+        peer_settings = {}
+
+        def fake_peer_store(key, val):
+            peer_settings[key] = val
+
+        def fake_migrate():
+            fake_peer_store('db-initialised', 'True')
+
+        self.migrate_database.side_effect = fake_migrate
+        mock_peer_store.side_effect = fake_peer_store
+        mock_peer_retrieve.side_effect = lambda key: peer_settings.get(key)
+
         self.is_db_ready.return_value = True
         mock_ensure_ssl_cert_master.return_value = False
         self.relation_ids.return_value = ['identity-service:0']
@@ -307,6 +343,7 @@ def test_postgresql_db_changed(self, identity_changed, configs,
             relation_id='identity-service:0',
             remote_unit='unit/0')
 
+    @patch.object(hooks, 'is_db_initialised')
     @patch.object(hooks, 'git_install_requested')
     @patch('keystone_utils.log')
     @patch('keystone_utils.ensure_ssl_cert_master')
@@ -340,11 +377,12 @@ def test_config_changed_no_upgrade_leader(self, configure_https,
                                               mock_ensure_pki_dir_permissions,
                                               mock_ensure_ssl_dirs,
                                               mock_ensure_ssl_cert_master,
-                                              mock_log, git_requested):
+                                              mock_log, git_requested,
+                                              mock_is_db_initialised):
         git_requested.return_value = False
         mock_is_pki_enabled.return_value = True
         mock_is_ssl_cert_master.return_value = True
-        self.is_db_initialised.return_value = True
+        mock_is_db_initialised.return_value = True
         self.is_db_ready.return_value = True
         self.openstack_upgrade_available.return_value = False
         self.is_elected_leader.return_value = True
@@ -421,6 +459,7 @@ def test_config_changed_no_upgrade_not_leader(self, configure_https,
         self.assertFalse(self.ensure_initial_admin.called)
         self.assertFalse(identity_changed.called)
 
+    @patch.object(hooks, 'is_db_initialised')
     @patch.object(hooks, 'git_install_requested')
     @patch('keystone_utils.log')
     @patch('keystone_utils.ensure_ssl_cert_master')
@@ -453,12 +492,13 @@ def test_config_changed_with_openstack_upgrade(self, configure_https,
                                                    mock_ensure_pki_permissions,
                                                    mock_ensure_ssl_dirs,
                                                    mock_ensure_ssl_cert_master,
-                                                   mock_log, git_requested):
+                                                   mock_log, git_requested,
+                                                   mock_is_db_initialised):
         git_requested.return_value = False
         mock_is_pki_enabled.return_value = True
         mock_is_ssl_cert_master.return_value = True
         self.is_db_ready.return_value = True
-        self.is_db_initialised.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
@@ -544,6 +584,7 @@ def test_config_changed_git_updated(self, configure_https,
         self.assertFalse(self.openstack_upgrade_available.called)
         self.assertFalse(self.do_openstack_upgrade_reexec.called)
 
+    @patch.object(hooks, 'is_db_initialised')
     @patch.object(hooks, 'git_install_requested')
     @patch.object(hooks, 'config_value_changed')
     @patch.object(hooks, 'ensure_ssl_dir')
@@ -562,7 +603,8 @@ def test_config_changed_with_openstack_upgrade_action(self,
                                                           is_pki, config_https,
                                                           ensure_ssl_dir,
                                                           config_value_changed,
-                                                          git_requested):
+                                                          git_requested,
+                                                          mock_db_init):
         ensure_ssl_cert.return_value = False
         is_pki.return_value = False
         peer_units.return_value = []
@@ -575,14 +617,15 @@ def test_config_changed_with_openstack_upgrade_action(self,
 
         self.assertFalse(self.do_openstack_upgrade_reexec.called)
 
+    @patch.object(hooks, 'is_db_initialised')
     @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):
-        self.is_db_initialised.return_value = True
+                                     mock_log, mock_is_db_initialised):
+        mock_is_db_initialised.return_value = True
         self.is_db_ready.return_value = True
         mock_ensure_ssl_cert_master.return_value = False
         hooks.identity_changed(
@@ -784,15 +827,17 @@ def test_ha_relation_changed_not_clustered_not_leader(self, configs,
         self.assertTrue(configs.write_all.called)
         self.assertFalse(mock_synchronize_ca.called)
 
+    @patch.object(hooks, 'is_db_initialised')
     @patch('keystone_utils.log')
     @patch('keystone_utils.ensure_ssl_cert_master')
     @patch.object(hooks, 'identity_changed')
     @patch.object(hooks, 'CONFIGS')
     def test_ha_relation_changed_clustered_leader(self, configs,
                                                   identity_changed,
                                                   mock_ensure_ssl_cert_master,
-                                                  mock_log):
-        self.is_db_initialised.return_value = True
+                                                  mock_log,
+                                                  mock_is_db_initialised):
+        mock_is_db_initialised.return_value = True
         self.is_db_ready.return_value = True
         mock_ensure_ssl_cert_master.return_value = False
         self.relation_get.return_value = True
@@ -906,5 +951,5 @@ def test_upgrade_charm_not_leader(self, ssh_authorized_peers,
         ssh_authorized_peers.assert_called_with(
             user=self.ssh_user, group='juju_keystone',
             peer_interface='cluster', ensure_local_user=True)
-        self.assertFalse(self.log.called)
+        self.assertTrue(self.log.called)
         self.assertFalse(self.ensure_initial_admin.called)