diff --git a/hooks/keystone_hooks.py b/hooks/keystone_hooks.py index 3bf637fc..f1df6564 100755 --- a/hooks/keystone_hooks.py +++ b/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 ' diff --git a/hooks/keystone_utils.py b/hooks/keystone_utils.py index f336ffd2..0104f0a1 100644 --- a/hooks/keystone_utils.py +++ b/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 diff --git a/unit_tests/test_keystone_hooks.py b/unit_tests/test_keystone_hooks.py index 4f158bbd..5d7d9564 100644 --- a/unit_tests/test_keystone_hooks.py +++ b/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') diff --git a/unit_tests/test_keystone_utils.py b/unit_tests/test_keystone_utils.py index 2c08a1ba..01bd7dce 100644 --- a/unit_tests/test_keystone_utils.py +++ b/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