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

@@ -5,3 +5,7 @@ bin
 tags
 *.sw[nop]
 *.pyc
+.unit-state.db
+trusty/
+xenial/
+tests/cirros-*-disk.img

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()

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)
 

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)