Catch MAASConfigIncomplete and set workload message

validate_dns_ha() shouldn't have the side effect of setting the workload
status and message, so now it will raise MAASConfigIncomplete with a
message and ha_relation_changed() is catching the exception, setting the
workload in blocked status and short circuiting the hook execution until
the user set maas_url and maas_credentials.

Change-Id: I3f1bc6571d2461cb65f384f042423cfe33f4d2f8
Closes-Bug: 1790559

hooks/hooks.py

@@ -27,6 +27,7 @@ from charmhelpers.core.hookenv import (
     log,
     DEBUG,
     INFO,
+    ERROR,
     related_units,
     relation_ids,
     relation_set,
@@ -81,6 +82,7 @@ from utils import (
     maintenance_mode,
     needs_maas_dns_migration,
     write_maas_dns_address,
+    MAASConfigIncomplete,
 )
 
 from charmhelpers.contrib.charmsupport import nrpe
@@ -263,25 +265,28 @@ def ha_relation_changed():
 
     if True in [ra.startswith('ocf:maas')
                 for ra in resources.values()]:
-        if validate_dns_ha():
-            log('Setting up access to MAAS API', level=INFO)
-            setup_maas_api()
-            # Update resource_parms for DNS resources to include MAAS URL and
-            # credentials
-            for resource in resource_params.keys():
-                if resource.endswith("_hostname"):
-                    res_ipaddr = get_ip_addr_from_resource_params(
-                        resource_params[resource])
-                    resource_params[resource] += (
-                        ' maas_url="{}" maas_credentials="{}"'
-                        ''.format(config('maas_url'),
-                                  config('maas_credentials')))
-                    write_maas_dns_address(resource, res_ipaddr)
-        else:
-            msg = ("DNS HA is requested but maas_url "
-                   "or maas_credentials are not set")
-            status_set('blocked', msg)
-            raise ValueError(msg)
+        try:
+            validate_dns_ha()
+        except MAASConfigIncomplete as ex:
+            log(ex.args[0], level=ERROR)
+            status_set('blocked', ex.args[0])
+            # if an exception is raised the hook will end up in error state
+            # which will obfuscate the workload status and message.
+            return
+
+        log('Setting up access to MAAS API', level=INFO)
+        setup_maas_api()
+        # Update resource_parms for DNS resources to include MAAS URL and
+        # credentials
+        for resource in resource_params.keys():
+            if resource.endswith("_hostname"):
+                res_ipaddr = get_ip_addr_from_resource_params(
+                    resource_params[resource])
+                resource_params[resource] += (
+                    ' maas_url="{}" maas_credentials="{}"'
+                    ''.format(config('maas_url'),
+                              config('maas_credentials')))
+                write_maas_dns_address(resource, res_ipaddr)
 
     # NOTE: this should be removed in 15.04 cycle as corosync
     # configuration should be set directly on subordinate

hooks/utils.py

@@ -685,6 +685,8 @@ def validate_dns_ha():
 
     Assert the charm will support DNS HA
     Check MAAS related configuration options are properly set
+
+    :raises MAASConfigIncomplete: if maas_url and maas_credentials are not set
     """
 
     # Will raise an exception if unable to continue
@@ -695,7 +697,6 @@ def validate_dns_ha():
     else:
         msg = ("DNS HA is requested but the maas_url or maas_credentials "
                "settings are not set")
-        status_set('blocked', msg)
         raise MAASConfigIncomplete(msg)
 
 

unit_tests/test_hacluster_hooks.py

@@ -23,6 +23,7 @@ import test_utils
 mock_apt = mock.MagicMock()
 sys.modules['apt_pkg'] = mock_apt
 import hooks
+import utils
 
 
 @mock.patch.object(hooks, 'log', lambda *args, **kwargs: None)
@@ -229,7 +230,10 @@ class TestCorosyncConf(unittest.TestCase):
             configure_corosync, oldest_peer, crm_opt_exists, peer_units,
             wait_for_pcmk, validate_dns_ha, setup_maas_api):
 
-        validate_dns_ha.return_value = False
+        def fake_validate():
+            raise utils.MAASConfigIncomplete('DNS HA invalid config')
+
+        validate_dns_ha.side_effect = fake_validate
         crm_opt_exists.return_value = False
         oldest_peer.return_value = True
         related_units.return_value = ['ha/0', 'ha/1', 'ha/2']
@@ -259,9 +263,10 @@ class TestCorosyncConf(unittest.TestCase):
             return rel_get_data.get(key, {})
 
         parse_data.side_effect = fake_parse_data
-
-        with self.assertRaises(ValueError):
+        with mock.patch.object(hooks, 'status_set') as mock_status_set:
             hooks.ha_relation_changed()
+            mock_status_set.assert_called_with('blocked',
+                                               'DNS HA invalid config')
 
 
 class TestHooks(test_utils.CharmTestCase):

unit_tests/test_hacluster_utils.py

@@ -190,6 +190,7 @@ class UtilsTestCase(unittest.TestCase):
         self.assertRaises(utils.MAASConfigIncomplete,
                           lambda: utils.validate_dns_ha())
         self.assertTrue(assert_charm_supports_dns_ha.called)
+        status_set.assert_not_called()
 
     @mock.patch.object(utils, 'apt_install')
     @mock.patch.object(utils, 'apt_update')