Merge "Ensure that manager.py is stopped when keystone restarted"

hooks/keystone_hooks.py

@@ -125,6 +125,7 @@ from keystone_utils import (
     pause_unit_helper,
     resume_unit_helper,
     remove_old_packages,
+    stop_manager_instance,
 )
 
 from charmhelpers.contrib.hahelpers.cluster import (
@@ -243,6 +244,7 @@ def config_changed_postupgrade():
             CONFIGS.write(WSGI_KEYSTONE_API_CONF)
         if not is_unit_paused_set():
             restart_pid_check('apache2')
+            stop_manager_instance()
 
     if enable_memcache(release=release):
         # If charm or OpenStack have been upgraded then the list of required
@@ -262,6 +264,7 @@ def config_changed_postupgrade():
 
     if snap_install_requested() and not is_unit_paused_set():
         service_restart('snap.keystone.*')
+        stop_manager_instance()
 
     if (is_db_initialised() and is_elected_leader(CLUSTER_RES) and not
             is_unit_paused_set()):
@@ -670,6 +673,7 @@ def upgrade_charm():
         log("Package purge detected, restarting services", "INFO")
         for s in services():
             service_restart(s)
+        stop_manager_instance()
 
     if is_elected_leader(CLUSTER_RES):
         log('Cluster leader - ensuring endpoint configuration is up to '

hooks/keystone_utils.py

@@ -557,14 +557,38 @@ def restart_pid_check(service_name, ptable_string=None):
 def restart_function_map():
     """Return a dict of services with any custom functions that should be
        used to restart that service
-    @returns dict of {'svc1': restart_func, 'svc2', other_func, ...}
+
+    :returns: dict of {'svc1': restart_func, 'svc2', other_func, ...}
+    :rtype: Dict[str, Callable]
     """
     rfunc_map = {}
-    if run_in_apache():
-        rfunc_map['apache2'] = restart_pid_check
+    rfunc_map[keystone_service()] = restart_keystone
     return rfunc_map
 
 
+def restart_keystone(*args):
+    """Restart the keystone process.
+
+    This will either keystone or apache2 depending on OpenStack version.
+    Also stop the ManagerServer (and thus manager.py script) which will
+    reconnect to keystone on next usage of the ManagerServer.
+
+    Note, as restart_keystone is used in the restart_functions map, when it is
+    called it is passed the service name.  However, this function determines
+    the actual service name to call, so that is discarded, hence the *args in
+    the function signature.
+    """
+    if not is_unit_paused_set():
+        if snap_install_requested():
+            service_restart('snap.keystone.*')
+        else:
+            if run_in_apache():
+                restart_pid_check(keystone_service())
+            else:
+                service_restart(keystone_service())
+        stop_manager_instance()
+
+
 def run_in_apache(release=None):
     """Return true if keystone API is run under apache2 with mod_wsgi in
     this release.
@@ -770,6 +794,7 @@ def migrate_database():
         service_start(keystone_service())
     time.sleep(10)
     leader_set({'db-initialised': True})
+    stop_manager_instance()
 
 # OLD
 
@@ -1083,12 +1108,25 @@ def _get_server_instance():
     return _the_manager_instance.server
 
 
+def stop_manager_instance():
+    """If a ManagerServer instance exists, then try to kill it, clean-up the
+    environment and reset the global singleton for it.
+    """
+    global _the_manager_instance
+    if _the_manager_instance is not None:
+        _the_manager_instance.clean_up()
+    _the_manager_instance = None
+
+
+# If a ManagerServer is still running at the end of the charm hook execution
+# then kill it off:
+atexit(stop_manager_instance)
+
+
 class ManagerServer():
     """This is a singleton server that launches and kills the manager.py script
     that is used to allow 'calling' into Keystone when it is in a completely
-    different process.  The object handles kill/quiting the manager.py script
-    when this keystone charm exits using the atexit charmhelpers `atexit()`
-    command to do the cleanup.
+    different process.
 
     The server() method also ensures that the manager.py script is still
     running, and if not, relaunches it.  This is to try to make the using the
@@ -1099,7 +1137,6 @@ class ManagerServer():
         self.pvar = None
         self._server = None
         self.socket_file = os.path.join(tempfile.gettempdir(), "keystone-uds")
-        atexit(lambda: self.clean_up())
 
     @property
     def server(self):
@@ -2092,14 +2129,6 @@ def post_snap_install():
         shutil.copy(PASTE_SRC, PASTE_DST)
 
 
-def restart_keystone():
-    if not is_unit_paused_set():
-        if snap_install_requested():
-            service_restart('snap.keystone.*')
-        else:
-            service_restart(keystone_service())
-
-
 def key_setup():
     """Initialize Fernet and Credential encryption key repositories
 

unit_tests/test_keystone_hooks.py

@@ -38,6 +38,7 @@ with patch('charmhelpers.core.hookenv.config') as config, \
         mock_dec.side_effect = (lambda *dargs, **dkwargs: lambda f:
                                 lambda *args, **kwargs: f(*args, **kwargs))
         with patch.object(utils, 'run_in_apache') as mock_run_in_apache:
+            mock_run_in_apache.return_value = True
             import keystone_hooks as hooks
             importlib.reload(hooks)
 
@@ -542,7 +543,9 @@ class KeystoneRelationTests(CharmTestCase):
     @patch.object(hooks, 'is_db_initialised')
     @patch('keystone_utils.log')
     @patch('keystone_utils.relation_ids')
+    @patch.object(hooks, 'stop_manager_instance')
     def test_upgrade_charm_leader(self,
+                                  mock_stop_manager_instance,
                                   mock_relation_ids,
                                   mock_log,
                                   mock_is_db_initialised,
@@ -562,6 +565,7 @@ class KeystoneRelationTests(CharmTestCase):
         self.assertTrue(update.called)
         self.remove_old_packages.assert_called_once_with()
         self.service_restart.assert_called_with('apache2')
+        mock_stop_manager_instance.assert_called_once_with()
 
     @patch.object(hooks, 'update_all_identity_relation_units')
     @patch.object(hooks, 'is_db_initialised')
@@ -707,7 +711,9 @@ class KeystoneRelationTests(CharmTestCase):
     @patch.object(utils, 'os_release')
     @patch('keystone_utils.log')
     @patch('keystone_utils.relation_ids')
+    @patch.object(hooks, 'stop_manager_instance')
     def test_upgrade_charm_not_leader(self,
+                                      mock_stop_manager_instance,
                                       mock_relation_ids,
                                       mock_log,
                                       os_release, update):
@@ -719,6 +725,7 @@ class KeystoneRelationTests(CharmTestCase):
         self.assertTrue(self.apt_install.called)
         self.assertTrue(self.log.called)
         self.assertFalse(update.called)
+        mock_stop_manager_instance.assert_called_once()
 
     def test_domain_backend_changed_v2(self):
         self.get_api_version.return_value = 2
@@ -741,9 +748,9 @@ class KeystoneRelationTests(CharmTestCase):
     @patch.object(hooks, 'is_unit_paused_set')
     @patch.object(hooks, 'is_db_initialised')
     @patch.object(utils, 'run_in_apache')
-    @patch.object(utils, 'service_restart')
+    @patch.object(utils, 'restart_pid_check')
     def test_domain_backend_changed_complete(self,
-                                             service_restart,
+                                             restart_pid_check,
                                              run_in_apache,
                                              is_db_initialised,
                                              is_unit_paused_set):
@@ -770,7 +777,7 @@ class KeystoneRelationTests(CharmTestCase):
                  rid=None),
         ])
         self.create_or_show_domain.assert_called_with('mydomain')
-        service_restart.assert_called_with('apache2')
+        restart_pid_check.assert_called_with('apache2')
         mock_kv.set.assert_called_with('domain-restart-nonce-mydomain',
                                        'nonce2')
         self.assertTrue(mock_kv.flush.called)
@@ -778,9 +785,9 @@ class KeystoneRelationTests(CharmTestCase):
     @patch.object(hooks, 'is_unit_paused_set')
     @patch.object(hooks, 'is_db_initialised')
     @patch.object(utils, 'run_in_apache')
-    @patch.object(utils, 'service_restart')
+    @patch.object(utils, 'restart_pid_check')
     def test_domain_backend_changed_complete_follower(self,
-                                                      service_restart,
+                                                      restart_pid_check,
                                                       run_in_apache,
                                                       is_db_initialised,
                                                       is_unit_paused_set):
@@ -808,7 +815,7 @@ class KeystoneRelationTests(CharmTestCase):
         ])
         # Only lead unit will create the domain
         self.assertFalse(self.create_or_show_domain.called)
-        service_restart.assert_called_with('apache2')
+        restart_pid_check.assert_called_with('apache2')
         mock_kv.set.assert_called_with('domain-restart-nonce-mydomain',
                                        'nonce2')
         self.assertTrue(mock_kv.flush.called)
@@ -818,10 +825,10 @@ class KeystoneRelationTests(CharmTestCase):
     @patch.object(hooks, 'is_unit_paused_set')
     @patch.object(hooks, 'is_db_initialised')
     @patch.object(utils, 'run_in_apache')
-    @patch.object(utils, 'service_restart')
+    @patch.object(utils, 'restart_pid_check')
     def test_fid_service_provider_changed_complete(
             self,
-            service_restart,
+            restart_pid_check,
             run_in_apache,
             is_db_initialised,
             is_unit_paused_set,
@@ -847,7 +854,7 @@ class KeystoneRelationTests(CharmTestCase):
         self.relation_get.assert_has_calls([
             call('restart-nonce'),
         ])
-        service_restart.assert_called_with('apache2')
+        restart_pid_check.assert_called_with('apache2')
         mock_kv.set.assert_called_with(
             'fid-restart-nonce-{}'.format(rel), 'nonce2')
         self.assertTrue(mock_kv.flush.called)
@@ -857,10 +864,10 @@ class KeystoneRelationTests(CharmTestCase):
     @patch.object(hooks, 'is_unit_paused_set')
     @patch.object(hooks, 'is_db_initialised')
     @patch.object(utils, 'run_in_apache')
-    @patch.object(utils, 'service_restart')
+    @patch.object(utils, 'restart_pid_check')
     def test_fid_service_provider_changed_complete_follower(
             self,
-            service_restart,
+            restart_pid_check,
             run_in_apache,
             is_db_initialised,
             is_unit_paused_set,
@@ -886,7 +893,7 @@ class KeystoneRelationTests(CharmTestCase):
         self.relation_get.assert_has_calls([
             call('restart-nonce'),
         ])
-        service_restart.assert_called_with('apache2')
+        restart_pid_check.assert_called_with('apache2')
         mock_kv.set.assert_called_with(
             'fid-restart-nonce-{}'.format(rel),
             'nonce2')

unit_tests/test_keystone_utils.py

@@ -302,8 +302,9 @@ class TestKeystoneUtils(CharmTestCase):
         _leader_get.return_value = None
         self.assertFalse(utils.is_db_initialised())
 
+    @patch.object(utils, 'stop_manager_instance')
     @patch.object(utils, 'leader_set')
-    def test_migrate_database(self, _leader_set):
+    def test_migrate_database(self, _leader_set, mock_stop_manager_instance):
         self.os_release.return_value = 'havana'
         utils.migrate_database()
 
@@ -312,6 +313,7 @@ class TestKeystoneUtils(CharmTestCase):
         self.subprocess.check_output.assert_called_with(cmd)
         self.service_start.assert_called_with('keystone')
         _leader_set.assert_called_with({'db-initialised': True})
+        mock_stop_manager_instance.assert_called_once_with()
 
     @patch.object(utils, 'leader_get')
     @patch.object(utils, 'get_api_version')
@@ -913,16 +915,74 @@ class TestKeystoneUtils(CharmTestCase):
             f.assert_called_once_with('assessor', services='s1', ports='p1')
 
     @patch.object(utils, 'run_in_apache')
-    @patch.object(utils, 'restart_pid_check')
-    def test_restart_function_map(self, restart_pid_check, run_in_apache):
+    @patch.object(utils, 'restart_keystone')
+    def test_restart_function_map(self, restart_keystone, run_in_apache):
         run_in_apache.return_value = True
         self.assertEqual(utils.restart_function_map(),
-                         {'apache2': restart_pid_check})
+                         {'apache2': restart_keystone})
+
+    @patch.object(utils, 'stop_manager_instance')
+    @patch.object(utils, 'is_unit_paused_set')
+    def test_restart_keystone_unit_paused(self,
+                                          mock_is_unit_paused_set,
+                                          mock_stop_manager_instance):
+        mock_is_unit_paused_set.return_value = True
+        utils.restart_keystone()
+        mock_stop_manager_instance.assert_not_called()
+
+    @patch.object(utils, 'snap_install_requested')
+    @patch.object(utils, 'service_restart')
+    @patch.object(utils, 'stop_manager_instance')
+    @patch.object(utils, 'is_unit_paused_set')
+    def test_restart_keystone_unit_not_paused_snap_install(
+            self,
+            mock_is_unit_paused_set,
+            mock_stop_manager_instance,
+            mock_service_restart,
+            mock_snap_install_requested):
+        mock_is_unit_paused_set.return_value = False
+        mock_snap_install_requested.return_value = True
+        utils.restart_keystone()
+        mock_service_restart.assert_called_once_with('snap.keystone.*')
+        mock_stop_manager_instance.assert_called_once_with()
 
     @patch.object(utils, 'run_in_apache')
-    def test_restart_function_map_legacy(self, run_in_apache):
-        run_in_apache.return_value = False
-        self.assertEqual(utils.restart_function_map(), {})
+    @patch.object(utils, 'snap_install_requested')
+    @patch.object(utils, 'service_restart')
+    @patch.object(utils, 'stop_manager_instance')
+    @patch.object(utils, 'is_unit_paused_set')
+    def test_restart_keystone_unit_not_paused_legacy(
+            self,
+            mock_is_unit_paused_set,
+            mock_stop_manager_instance,
+            mock_service_restart,
+            mock_snap_install_requested,
+            mock_run_in_apache):
+        mock_is_unit_paused_set.return_value = False
+        mock_snap_install_requested.return_value = False
+        mock_run_in_apache.return_value = False
+        utils.restart_keystone()
+        mock_service_restart.assert_called_once_with('keystone')
+        mock_stop_manager_instance.assert_called_once_with()
+
+    @patch.object(utils, 'run_in_apache')
+    @patch.object(utils, 'snap_install_requested')
+    @patch.object(utils, 'restart_pid_check')
+    @patch.object(utils, 'stop_manager_instance')
+    @patch.object(utils, 'is_unit_paused_set')
+    def test_restart_keystone_unit_not_paused(
+            self,
+            mock_is_unit_paused_set,
+            mock_stop_manager_instance,
+            mock_restart_pid_check,
+            mock_snap_install_requested,
+            mock_run_in_apache):
+        mock_is_unit_paused_set.return_value = False
+        mock_snap_install_requested.return_value = False
+        mock_run_in_apache.return_value = True
+        utils.restart_keystone()
+        mock_restart_pid_check.assert_called_once_with('apache2')
+        mock_stop_manager_instance.assert_called_once_with()
 
     def test_restart_pid_check(self):
         self.subprocess.call.return_value = 1