Merge lp:~thedac/charm-helpers/openstack-object-oriented-status into lp:charm-helpers

[David Ames]

Object oriented workload status for openstack charms

Object oriented workload status for openstack charms

[Liam Young (gnuoy)]

I really like this but I have some concerns around some of the object names used. It looks to me like context_states is really relation_states and incomplete_contexts does not return a list of incomplete contexts but rather a list of incomplete relations so would more correctly be called unsatisfied_relations.

I'm wondering whether this mp should be extended to include the checking of contexts which aren't associated with a relation. For example when doing an HA deployment the user needs to have set a vip for the charm to use, if they don't do this the charm is blocked until they do. If the main hook function communicates this to the user via a status set then the context_status decorator will stamp on that message I think.

review: Needs Fixing

[lp:~thedac/charm-helpers/openstack-object-oriented-status]

407. By David Ames on 2015-09-11

Rename and add charm specific function to avoid clobbering

[David Ames (thedac)]

> I really like this but I have some concerns around some of the object names
> used. It looks to me like context_states is really relation_states

I have renamed this os_workload_status

> and
> incomplete_contexts does not return a list of incomplete contexts but rather a
> list of incomplete relations so would more correctly be called
> unsatisfied_relations.

I actually need you to be more specific on this? Maybe I am missing what a "context" is. I'd like to discuss it further.

> I'm wondering whether this mp should be extended to include the checking of
> contexts which aren't associated with a relation. For example when doing an HA
> deployment the user needs to have set a vip for the charm to use, if they
> don't do this the charm is blocked until they do. If the main hook function
> communicates this to the user via a status set then the context_status
> decorator will stamp on that message I think.

I have added a charm specific function call so that these status settings will not be clobbered.

review: Needs Resubmitting

[lp:~thedac/charm-helpers/openstack-object-oriented-status]

408. By David Ames on 2015-09-15

Rename incomplete_contexts incomplete_relation_data

[Liam Young (gnuoy)]

A couple of nits (see diff comments).

review: Needs Fixing

[lp:~thedac/charm-helpers/openstack-object-oriented-status]

409. By David Ames on 2015-09-15

More renames

[David Ames (thedac)]

Liam, please take another look

review: Needs Resubmitting

[lp:~thedac/charm-helpers/openstack-object-oriented-status]

410. By David Ames on 2015-09-15

Unit is ready

[Liam Young (gnuoy)]

LGTM, thanks!

review: Approve

=== modified file 'charmhelpers/contrib/openstack/context.py'
--- charmhelpers/contrib/openstack/context.py	2015-09-11 12:19:23 +0000
+++ charmhelpers/contrib/openstack/context.py	2015-09-15 17:38:07 +0000
@@ -194,10 +194,50 @@
 class OSContextGenerator(object):
     """Base class for all context generators."""
     interfaces = []
+    related = False
+    complete = False
+    missing_data = []
 
     def __call__(self):
         raise NotImplementedError
 
+    def context_complete(self, ctxt):
+        """Check for missing data for the required context data.
+        Set self.missing_data if it exists and return False.
+        Set self.complete if no missing data and return True.
+        """
+        # Fresh start
+        self.complete = False
+        self.missing_data = []
+        for k, v in six.iteritems(ctxt):
+            if v is None or v == '':
+                if k not in self.missing_data:
+                    self.missing_data.append(k)
+
+        if self.missing_data:
+            self.complete = False
+            log('Missing required data: %s' % ' '.join(self.missing_data), level=INFO)
+        else:
+            self.complete = True
+        return self.complete
+
+    def get_related(self):
+        """Check if any of the context interfaces have relation ids.
+        Set self.related and return True if one of the interfaces
+        has relation ids.
+        """
+        # Fresh start
+        self.related = False
+        try:
+            for interface in self.interfaces:
+                if relation_ids(interface):
+                    self.related = True
+            return self.related
+        except AttributeError as e:
+            log("{} {}"
+                "".format(self, e), 'INFO')
+            return self.related
+
 
 class SharedDBContext(OSContextGenerator):
     interfaces = ['shared-db']
@@ -213,6 +253,7 @@
         self.database = database
         self.user = user
         self.ssl_dir = ssl_dir
+        self.rel_name = self.interfaces[0]
 
     def __call__(self):
         self.database = self.database or config('database')
@@ -246,6 +287,7 @@
             password_setting = self.relation_prefix + '_password'
 
         for rid in relation_ids(self.interfaces[0]):
+            self.related = True
             for unit in related_units(rid):
                 rdata = relation_get(rid=rid, unit=unit)
                 host = rdata.get('db_host')
@@ -257,7 +299,7 @@
                     'database_password': rdata.get(password_setting),
                     'database_type': 'mysql'
                 }
-                if context_complete(ctxt):
+                if self.context_complete(ctxt):
                     db_ssl(rdata, ctxt, self.ssl_dir)
                     return ctxt
         return {}
@@ -278,6 +320,7 @@
 
         ctxt = {}
         for rid in relation_ids(self.interfaces[0]):
+            self.related = True
             for unit in related_units(rid):
                 rel_host = relation_get('host', rid=rid, unit=unit)
                 rel_user = relation_get('user', rid=rid, unit=unit)
@@ -287,7 +330,7 @@
                         'database_user': rel_user,
                         'database_password': rel_passwd,
                         'database_type': 'postgresql'}
-                if context_complete(ctxt):
+                if self.context_complete(ctxt):
                     return ctxt
 
         return {}
@@ -348,6 +391,7 @@
             ctxt['signing_dir'] = cachedir
 
         for rid in relation_ids(self.rel_name):
+            self.related = True
             for unit in related_units(rid):
                 rdata = relation_get(rid=rid, unit=unit)
                 serv_host = rdata.get('service_host')
@@ -366,7 +410,7 @@
                              'service_protocol': svc_protocol,
                              'auth_protocol': auth_protocol})
 
-                if context_complete(ctxt):
+                if self.context_complete(ctxt):
                     # NOTE(jamespage) this is required for >= icehouse
                     # so a missing value just indicates keystone needs
                     # upgrading
@@ -405,6 +449,7 @@
         ctxt = {}
         for rid in relation_ids(self.rel_name):
             ha_vip_only = False
+            self.related = True
             for unit in related_units(rid):
                 if relation_get('clustered', rid=rid, unit=unit):
                     ctxt['clustered'] = True
@@ -437,7 +482,7 @@
                 ha_vip_only = relation_get('ha-vip-only',
                                            rid=rid, unit=unit) is not None
 
-                if context_complete(ctxt):
+                if self.context_complete(ctxt):
                     if 'rabbit_ssl_ca' in ctxt:
                         if not self.ssl_dir:
                             log("Charm not setup for ssl support but ssl ca "
@@ -469,7 +514,7 @@
             ctxt['oslo_messaging_flags'] = config_flags_parser(
                 oslo_messaging_flags)
 
-        if not context_complete(ctxt):
+        if not self.complete:
             return {}
 
         return ctxt
@@ -507,7 +552,7 @@
         if not os.path.isdir('/etc/ceph'):
             os.mkdir('/etc/ceph')
 
-        if not context_complete(ctxt):
+        if not self.context_complete(ctxt):
             return {}
 
         ensure_packages(['ceph-common'])
@@ -1366,6 +1411,6 @@
                     'auth_protocol':
                     rdata.get('auth_protocol') or 'http',
                 }
-                if context_complete(ctxt):
+                if self.context_complete(ctxt):
                     return ctxt
         return {}

=== modified file 'charmhelpers/contrib/openstack/templating.py'
--- charmhelpers/contrib/openstack/templating.py	2015-07-22 01:19:53 +0000
+++ charmhelpers/contrib/openstack/templating.py	2015-09-15 17:38:07 +0000
@@ -112,7 +112,7 @@
 
     def complete_contexts(self):
         '''
-        Return a list of interfaces that have atisfied contexts.
+        Return a list of interfaces that have satisfied contexts.
         '''
         if self._complete_contexts:
             return self._complete_contexts
@@ -293,3 +293,30 @@
         [interfaces.extend(i.complete_contexts())
          for i in six.itervalues(self.templates)]
         return interfaces
+
+    def get_incomplete_context_data(self, interfaces):
+        '''
+        Return dictionary of relation status of interfaces and any missing
+        required context data. Example:
+            {'amqp': {'missing_data': ['rabbitmq_password'], 'related': True},
+             'zeromq-configuration': {'related': False}}
+        '''
+        incomplete_context_data = {}
+
+        for i in six.itervalues(self.templates):
+            for context in i.contexts:
+                for interface in interfaces:
+                    related = False
+                    if interface in context.interfaces:
+                        related = context.get_related()
+                        missing_data = context.missing_data
+                        if missing_data:
+                            incomplete_context_data[interface] = {'missing_data': missing_data}
+                        if related:
+                            if incomplete_context_data.get(interface):
+                                incomplete_context_data[interface].update({'related': True})
+                            else:
+                                incomplete_context_data[interface] = {'related': True}
+                        else:
+                            incomplete_context_data[interface] = {'related': False}
+        return incomplete_context_data

=== modified file 'charmhelpers/contrib/openstack/utils.py'
--- charmhelpers/contrib/openstack/utils.py	2015-09-07 11:09:43 +0000
+++ charmhelpers/contrib/openstack/utils.py	2015-09-15 17:38:07 +0000
@@ -39,7 +39,9 @@
     charm_dir,
     INFO,
     relation_ids,
-    relation_set
+    relation_set,
+    status_set,
+    hook_name
 )
 
 from charmhelpers.contrib.storage.linux.lvm import (
@@ -749,3 +751,173 @@
         return projects[key]
 
     return None
+
+
+def os_workload_status(configs, required_interfaces, charm_func=None):
+    """
+    Decorator to set workload status based on complete contexts
+    """
+    def wrap(f):
+        @wraps(f)
+        def wrapped_f(*args, **kwargs):
+            # Run the original function first
+            f(*args, **kwargs)
+            # Set workload status now that contexts have been
+            # acted on
+            set_os_workload_status(configs, required_interfaces, charm_func)
+        return wrapped_f
+    return wrap
+
+
+def set_os_workload_status(configs, required_interfaces, charm_func=None):
+    """
+    Set workload status based on complete contexts.
+    status-set missing or incomplete contexts
+    and juju-log details of missing required data.
+    charm_func is a charm specific function to run checking
+    for charm specific requirements such as a VIP setting.
+    """
+    incomplete_rel_data = incomplete_relation_data(configs, required_interfaces)
+    state = 'active'
+    missing_relations = []
+    incomplete_relations = []
+    message = None
+    charm_state = None
+    charm_message = None
+
+    for generic_interface in incomplete_rel_data.keys():
+        related_interface = None
+        missing_data = {}
+        # Related or not?
+        for interface in incomplete_rel_data[generic_interface]:
+            if incomplete_rel_data[generic_interface][interface].get('related'):
+                related_interface = interface
+                missing_data = incomplete_rel_data[generic_interface][interface].get('missing_data')
+        # No relation ID for the generic_interface
+        if not related_interface:
+            juju_log("{} relation is missing and must be related for "
+                     "functionality. ".format(generic_interface), 'WARN')
+            state = 'blocked'
+            if generic_interface not in missing_relations:
+                missing_relations.append(generic_interface)
+        else:
+            # Relation ID exists but no related unit
+            if not missing_data:
+                # Edge case relation ID exists but departing
+                if ('departed' in hook_name() or 'broken' in hook_name()) \
+                        and related_interface in hook_name():
+                    state = 'blocked'
+                    if generic_interface not in missing_relations:
+                        missing_relations.append(generic_interface)
+                    juju_log("{} relation's interface, {}, "
+                             "relationship is departed or broken "
+                             "and is required for functionality."
+                             "".format(generic_interface, related_interface), "WARN")
+                # Normal case relation ID exists but no related unit
+                # (joining)
+                else:
+                    juju_log("{} relations's interface, {}, is related but has "
+                             "no units in the relation."
+                             "".format(generic_interface, related_interface), "INFO")
+            # Related unit exists and data missing on the relation
+            else:
+                juju_log("{} relation's interface, {}, is related awaiting "
+                         "the following data from the relationship: {}. "
+                         "".format(generic_interface, related_interface,
+                                   ", ".join(missing_data)), "INFO")
+            if state != 'blocked':
+                state = 'waiting'
+            if generic_interface not in incomplete_relations \
+                    and generic_interface not in missing_relations:
+                incomplete_relations.append(generic_interface)
+
+    if missing_relations:
+        message = "Missing relations: {}".format(", ".join(missing_relations))
+        if incomplete_relations:
+            message += "; incomplete relations: {}" \
+                       "".format(", ".join(incomplete_relations))
+        state = 'blocked'
+    elif incomplete_relations:
+        message = "Incomplete relations: {}" \
+                  "".format(", ".join(incomplete_relations))
+        state = 'waiting'
+
+    # Run charm specific checks
+    if charm_func:
+        charm_state, charm_message = charm_func(configs)
+        if charm_state != 'active' and charm_state != 'unknown':
+            state = workload_state_compare(state, charm_state)
+            if message:
+                message = "{} {}".format(message, charm_message)
+            else:
+                message = charm_message
+
+    # Set to active if all requirements have been met
+    if state == 'active':
+        message = "Unit is ready"
+        juju_log(message, "INFO")
+
+    status_set(state, message)
+
+
+def workload_state_compare(current_workload_state, workload_state):
+    """ Return highest priority of two states"""
+    hierarchy = {'unknown': -1,
+                 'active': 0,
+                 'maintenance': 1,
+                 'waiting': 2,
+                 'blocked': 3,
+                 }
+
+    if hierarchy.get(workload_state) is None:
+        workload_state = 'unknown'
+    if hierarchy.get(current_workload_state) is None:
+        current_workload_state = 'unknown'
+
+    # Set workload_state based on hierarchy of statuses
+    if hierarchy.get(current_workload_state) > hierarchy.get(workload_state):
+        return current_workload_state
+    else:
+        return workload_state
+
+
+def incomplete_relation_data(configs, required_interfaces):
+    """
+    Check complete contexts against required_interfaces
+    Return dictionary of incomplete relation data.
+
+    configs is an OSConfigRenderer object with configs registered
+
+    required_interfaces is a dictionary of required general interfaces
+    with dictionary values of possible specific interfaces.
+    Example:
+    required_interfaces = {'database': ['shared-db', 'pgsql-db']}
+
+    The interface is said to be satisfied if anyone of the interfaces in the
+    list has a complete context.
+
+    Return dictionary of incomplete or missing required contexts with relation
+    status of interfaces and any missing data points. Example:
+        {'message':
+             {'amqp': {'missing_data': ['rabbitmq_password'], 'related': True},
+              'zeromq-configuration': {'related': False}},
+         'identity':
+             {'identity-service': {'related': False}},
+         'database':
+             {'pgsql-db': {'related': False},
+              'shared-db': {'related': True}}}
+    """
+    complete_ctxts = configs.complete_contexts()
+    incomplete_relations = []
+    for svc_type in required_interfaces.keys():
+        # Avoid duplicates
+        found_ctxt = False
+        for interface in required_interfaces[svc_type]:
+            if interface in complete_ctxts:
+                found_ctxt = True
+        if not found_ctxt:
+            incomplete_relations.append(svc_type)
+    incomplete_context_data = {}
+    for i in incomplete_relations:
+        incomplete_context_data[i] = configs.get_incomplete_context_data(required_interfaces[i])
+    return incomplete_context_data

=== modified file 'tests/contrib/openstack/test_openstack_utils.py'
--- tests/contrib/openstack/test_openstack_utils.py	2015-09-03 08:41:45 +0000
+++ tests/contrib/openstack/test_openstack_utils.py	2015-09-15 17:38:07 +0000
@@ -840,5 +840,108 @@
         openstack.git_src_dir(openstack_origin_git, 'keystone')
         join.assert_called_with('/mnt/openstack-git', 'keystone')
 
+    def test_incomplete_relation_data(self):
+        configs = MagicMock()
+        configs.complete_contexts.return_value = ['pgsql-db', 'amqp']
+        required_interfaces = {
+            'database': ['shared-db', 'pgsql-db'],
+            'message': ['amqp', 'zeromq-configuration'],
+            'identity': ['identity-service']}
+        expected_result = 'identity'
+
+        result = openstack.incomplete_relation_data(configs, required_interfaces)
+        self.assertTrue(expected_result in result.keys())
+
+    @patch('charmhelpers.contrib.openstack.utils.status_set')
+    def test_set_os_workload_status_complete(self, status_set):
+        configs = MagicMock()
+        configs.complete_contexts.return_value = ['shared-db',
+                                                  'amqp',
+                                                  'identity-service']
+        required_interfaces = {
+            'database': ['shared-db', 'pgsql-db'],
+            'message': ['amqp', 'zeromq-configuration'],
+            'identity': ['identity-service']}
+
+        openstack.set_os_workload_status(configs, required_interfaces)
+        status_set.assert_called_with('active', 'Unit is ready')
+
+    @patch('charmhelpers.contrib.openstack.utils.incomplete_relation_data',
+           return_value={'identity': {'identity-service': {'related': True}}})
+    @patch('charmhelpers.contrib.openstack.utils.status_set')
+    def test_set_os_workload_status_related_incomplete(self, status_set,
+                                                       incomplete_relation_data):
+        configs = MagicMock()
+        configs.complete_contexts.return_value = ['shared-db', 'amqp']
+        required_interfaces = {
+            'database': ['shared-db', 'pgsql-db'],
+            'message': ['amqp', 'zeromq-configuration'],
+            'identity': ['identity-service']}
+
+        openstack.set_os_workload_status(configs, required_interfaces)
+        status_set.assert_called_with('waiting',
+                                      "Incomplete relations: identity")
+
+    @patch('charmhelpers.contrib.openstack.utils.incomplete_relation_data',
+           return_value={'identity': {'identity-service': {'related': False}}})
+    @patch('charmhelpers.contrib.openstack.utils.status_set')
+    def test_set_os_workload_status_absent(self, status_set,
+                                           incomplete_relation_data):
+        configs = MagicMock()
+        configs.complete_contexts.return_value = ['shared-db', 'amqp']
+        required_interfaces = {
+            'database': ['shared-db', 'pgsql-db'],
+            'message': ['amqp', 'zeromq-configuration'],
+            'identity': ['identity-service']}
+
+        openstack.set_os_workload_status(configs, required_interfaces)
+        status_set.assert_called_with('blocked',
+                                      'Missing relations: identity')
+
+    @patch('charmhelpers.contrib.openstack.utils.hook_name',
+           return_value='identity-service-relation-broken')
+    @patch('charmhelpers.contrib.openstack.utils.incomplete_relation_data',
+           return_value={'identity': {'identity-service': {'related': True}}})
+    @patch('charmhelpers.contrib.openstack.utils.status_set')
+    def test_set_os_workload_status_related_broken(self, status_set,
+                                                   incomplete_relation_data,
+                                                   hook_name):
+        configs = MagicMock()
+        configs.complete_contexts.return_value = ['shared-db', 'amqp']
+        required_interfaces = {
+            'database': ['shared-db', 'pgsql-db'],
+            'message': ['amqp', 'zeromq-configuration'],
+            'identity': ['identity-service']}
+
+        openstack.set_os_workload_status(configs, required_interfaces)
+        status_set.assert_called_with('blocked',
+                                      "Missing relations: identity")
+
+    @patch('charmhelpers.contrib.openstack.utils.incomplete_relation_data',
+           return_value={'identity':
+                         {'identity-service': {'related': True}},
+
+                         'message':
+                         {'amqp': {'missing_data': ['rabbitmq-password'],
+                                   'related': True}},
+
+                         'database':
+                         {'shared-db': {'related': False}}
+                         })
+    @patch('charmhelpers.contrib.openstack.utils.status_set')
+    def test_set_os_workload_status_mixed(self, status_set, incomplete_relation_data):
+        configs = MagicMock()
+        configs.complete_contexts.return_value = ['shared-db', 'amqp']
+        required_interfaces = {
+            'database': ['shared-db', 'pgsql-db'],
+            'message': ['amqp', 'zeromq-configuration'],
+            'identity': ['identity-service']}
+
+        openstack.set_os_workload_status(configs, required_interfaces)
+        status_set.assert_called_with('blocked',
+                                      "Missing relations: database; "
+                                      "incomplete relations: message, "
+                                      "identity")
+
 if __name__ == '__main__':
     unittest.main()

=== modified file 'tests/contrib/openstack/test_os_contexts.py'
--- tests/contrib/openstack/test_os_contexts.py	2015-09-11 12:19:23 +0000
+++ tests/contrib/openstack/test_os_contexts.py	2015-09-15 17:38:07 +0000
@@ -2594,7 +2594,7 @@
         self.related_units.return_value = []
         self.assertEquals(context.NetworkServiceContext()(), {})
 
-    @patch.object(context, 'context_complete')
+    @patch.object(context.OSContextGenerator, 'context_complete')
     def test_network_service_ctxt_no_data(self, mock_context_complete):
         rel = FakeRelation(QUANTUM_NETWORK_SERVICE_RELATION)
         self.relation_ids.side_effect = rel.relation_ids