From b570e36cba93affe67594ef820afae731b746ad7 Mon Sep 17 00:00:00 2001 From: David Ames Date: Mon, 13 Jun 2016 15:55:40 -0700 Subject: [PATCH] Fix for status-set race - related to bug 1588462 This change fixes the obvious race for a status_set() between check_optional_interfaces() and assess_status() as the later calls the former which calls status_set(), returns the status, which is then potentially set again by the assess_status() function. This cleans up the code so that only a single status_set() is performed when calling assess_status(). Change-Id: Ie37a4d98de9c5e7bd26304e096796ce6287ea52b Related-Bug:#1588462 --- .gitignore | 4 +++ hooks/nova_cc_hooks.py | 7 ++--- hooks/nova_cc_utils.py | 53 ++++++++++++++++++++++---------- unit_tests/test_nova_cc_utils.py | 9 ++++-- 4 files changed, 50 insertions(+), 23 deletions(-) diff --git a/.gitignore b/.gitignore index 25d8aecb..6b6b49c0 100644 --- a/.gitignore +++ b/.gitignore @@ -5,3 +5,7 @@ bin tags *.sw[nop] *.pyc +.unit-state.db +trusty/ +xenial/ +tests/cirros-*-disk.img diff --git a/hooks/nova_cc_hooks.py b/hooks/nova_cc_hooks.py index fe02f9a4..d22dea00 100755 --- a/hooks/nova_cc_hooks.py +++ b/hooks/nova_cc_hooks.py @@ -49,7 +49,6 @@ from charmhelpers.contrib.openstack.utils import ( os_release, os_requires_version, sync_db_with_multi_ipv6_addresses, - set_os_workload_status, pausable_restart_on_change as restart_on_change, is_unit_paused_set, ) @@ -96,9 +95,8 @@ from nova_cc_utils import ( guard_map, get_topics, setup_ipv6, - REQUIRED_INTERFACES, - check_optional_relations, is_db_initialised, + assess_status, ) from charmhelpers.contrib.hahelpers.cluster import ( @@ -1068,8 +1066,7 @@ def main(): hooks.execute(sys.argv) except UnregisteredHookError as e: log('Unknown hook {} - skipping.'.format(e)) - set_os_workload_status(CONFIGS, REQUIRED_INTERFACES, - charm_func=check_optional_relations) + assess_status(CONFIGS) if __name__ == '__main__': main() diff --git a/hooks/nova_cc_utils.py b/hooks/nova_cc_utils.py index f3df14a7..39e3bca8 100644 --- a/hooks/nova_cc_utils.py +++ b/hooks/nova_cc_utils.py @@ -37,7 +37,6 @@ from charmhelpers.contrib.openstack.utils import ( is_ip, os_release, save_script_rc as _save_script_rc, - set_os_workload_status, is_unit_paused_set, make_assess_status_func, pause_unit, @@ -61,7 +60,6 @@ from charmhelpers.core.hookenv import ( DEBUG, INFO, ERROR, - status_get, status_set, related_units, local_unit, @@ -1167,27 +1165,44 @@ def git_post_install(projects_yaml): apt_install(LATE_GIT_PACKAGES, fatal=True) +def get_optional_interfaces(): + """Return the optional interfaces that should be checked if the relavent + relations have appeared. + + :returns: {general_interface: [specific_int1, specific_int2, ...], ...} + """ + optional_interfaces = {} + if relation_ids('quantum-network-service'): + optional_interfaces['quantum'] = ['quantum-network-service'] + if relation_ids('cinder-volume-service'): + optional_interfaces['cinder'] = ['cinder-volume-service'] + if relation_ids('neutron-api'): + optional_interfaces['neutron-api'] = ['neutron-api'] + + return optional_interfaces + + def check_optional_relations(configs): - required_interfaces = {} + """Check that if we have a relation_id for high availability that we can + get the hacluster config. If we can't then we are blocked. + + This function is called from assess_status/set_os_workload_status as the + charm_func and needs to return either None, None if there is no problem or + the status, message if there is a problem. + + :param configs: an OSConfigRender() instance. + :return 2-tuple: (string, string) = (status, message) + """ if relation_ids('ha'): - required_interfaces['ha'] = ['cluster'] try: get_hacluster_config() except: return ('blocked', 'hacluster missing configuration: ' 'vip, vip_iface, vip_cidr') - if relation_ids('quantum-network-service'): - required_interfaces['quantum'] = ['quantum-network-service'] - if relation_ids('cinder-volume-service'): - required_interfaces['cinder'] = ['cinder-volume-service'] - if relation_ids('neutron-api'): - required_interfaces['neutron-api'] = ['neutron-api'] - if required_interfaces: - set_os_workload_status(configs, required_interfaces) - return status_get() - else: - return 'unknown', 'No optional relations' + # return 'unknown' as the lowest priority to not clobber an existing + # status. + return "unknown", "" def is_api_ready(configs): @@ -1215,14 +1230,20 @@ def assess_status_func(configs): Used directly by assess_status() and also for pausing and resuming the unit. + NOTE: REQUIRED_INTERFACES is augmented with the optional interfaces + depending on the current config before being passed to the + make_assess_status_func() function. + NOTE(ajkavanagh) ports are not checked due to race hazards with services that don't behave sychronously w.r.t their service scripts. e.g. apache2. @param configs: a templating.OSConfigRenderer() object @return f() -> None : a function that assesses the unit's workload status """ + required_interfaces = REQUIRED_INTERFACES.copy() + required_interfaces.update(get_optional_interfaces()) return make_assess_status_func( - configs, REQUIRED_INTERFACES, + configs, required_interfaces, charm_func=check_optional_relations, services=services(), ports=None) diff --git a/unit_tests/test_nova_cc_utils.py b/unit_tests/test_nova_cc_utils.py index 2bbefe62..84a0d0cc 100644 --- a/unit_tests/test_nova_cc_utils.py +++ b/unit_tests/test_nova_cc_utils.py @@ -911,6 +911,7 @@ class NovaCCUtilsTests(CharmTestCase): asf.assert_called_once_with('test-config') callee.assert_called_once_with() + @patch.object(utils, 'get_optional_interfaces') @patch.object(utils, 'check_optional_relations') @patch.object(utils, 'REQUIRED_INTERFACES') @patch.object(utils, 'services') @@ -921,13 +922,17 @@ class NovaCCUtilsTests(CharmTestCase): determine_ports, services, REQUIRED_INTERFACES, - check_optional_relations): + check_optional_relations, + get_optional_interfaces): services.return_value = 's1' + REQUIRED_INTERFACES.copy.return_value = {'int': ['test 1']} + get_optional_interfaces.return_value = {'opt': ['test 2']} determine_ports.return_value = 'p1' utils.assess_status_func('test-config') # ports=None whilst port checks are disabled. make_assess_status_func.assert_called_once_with( - 'test-config', REQUIRED_INTERFACES, + 'test-config', + {'int': ['test 1'], 'opt': ['test 2']}, charm_func=check_optional_relations, services='s1', ports=None)