Merge lp:~ajkavanagh/charm-helpers/add-service-checks-lp1524388 into lp:charm-helpers

[Alex Kavanagh]

  Add service running and ports open checks to set_os_workload_status()
  This adds optional checks to the set_os_workload_status(...) function to
  make sure that the services that the charm says should be running are
  actually running (according to service_running()) and also, optionally,
  that the ports that are supposed to be open are open (on 0.0.0.0) and
  have something listening to them.

[David Ames (thedac)]

Needs fixing:

We are going to want to set the state to 'blocked' which means user action is required. The state 'unknown' is reserved for before a state has been declared or for charms which never set state.

Hacking around a bit with your keystone example (I had to add servcies to the set_os_workload_status). When I used a simple ['keystone'] it fails:

    # set the status according to the current state of the contexts
    set_os_workload_status(
        configs, REQUIRED_INTERFACES, charm_func=check_optional_relations,
        services=['keystone'])

Traceback (most recent call last):
  File "hooks/update-status", line 654, in <module>
    main()
  File "hooks/update-status", line 650, in main
    assess_status(CONFIGS)
  File "/var/lib/juju/agents/unit-keystone-0/charm/hooks/keystone_utils.py", line 1845, in assess_status
    services=['keystone'])
  File "/var/lib/juju/agents/unit-keystone-0/charm/hooks/charmhelpers/contrib/openstack/utils.py", line 936, in set_os_workload_status
    _s = [s['service'] for s in services]
TypeError: string indices must be integers, not str

Using the full dictionary works beautifully.
    set_os_workload_status(
        configs, REQUIRED_INTERFACES, charm_func=check_optional_relations,
        services=[{'service': 'keystone', 'ports': [5000, 4990, 35357, 35347]}])

For discussion:

We need to have a discussion about the default ports for a service vrs our haproxy with non-default ports for the service. For example keystone's default is 5000 which haproxy listens for but our charm has the keystone service itself listen on 4990.

Is there anyway we can automatically "guess" these?

[Alex Kavanagh (ajkavanagh)]

I've updated charm-helpers to support the simple list of services (oversight, apologies), added tests, and also updated the keystone example branch (lp:~ajkavanagh/charms/trusty/keystone/add-service-checks-lp1524388) to use the new version.

I've also changed the set_os_workload_status(...) call in assess_status(...) to be:

    # set the status according to the current state of the contexts
    set_os_workload_status(
        configs, REQUIRED_INTERFACES, charm_func=check_optional_relations,
        services=services(), ports=determine_ports())

The utils function services() and determine_ports() 'appear' (for keystone, at least) the list of running services and the ports it manages. Is this true, and thus, does it answer the question you posed for discussion?

[David Ames (thedac)]

This looks good. Requesting a merge.

review: Approve

[Liam Young (gnuoy)]

Approve

review: Approve

=== modified file 'charmhelpers/contrib/network/ip.py'
--- charmhelpers/contrib/network/ip.py	2015-12-14 10:43:51 +0000
+++ charmhelpers/contrib/network/ip.py	2016-02-12 11:00:52 +0000
@@ -456,3 +456,18 @@
             return result
     else:
         return result.split('.')[0]
+
+
+def port_has_listener(address, port):
+    """
+    Returns True if the address:port is open and being listened to,
+    else False.
+
+    @param address: an IP address or hostname
+    @param port: integer port
+
+    Note calls 'zc' via a subprocess shell
+    """
+    cmd = ['nc', '-z', address, str(port)]
+    result = subprocess.call(cmd)
+    return not(bool(result))

=== modified file 'charmhelpers/contrib/openstack/utils.py'
--- charmhelpers/contrib/openstack/utils.py	2016-02-11 15:34:00 +0000
+++ charmhelpers/contrib/openstack/utils.py	2016-02-12 11:00:52 +0000
@@ -23,6 +23,7 @@
 import os
 import sys
 import re
+import itertools
 
 import six
 import tempfile
@@ -60,6 +61,7 @@
 from charmhelpers.contrib.network.ip import (
     get_ipv6_addr,
     is_ipv6,
+    port_has_listener,
 )
 
 from charmhelpers.contrib.python.packages import (
@@ -67,7 +69,7 @@
     pip_install,
 )
 
-from charmhelpers.core.host import lsb_release, mounts, umount
+from charmhelpers.core.host import lsb_release, mounts, umount, service_running
 from charmhelpers.fetch import apt_install, apt_cache, install_remote
 from charmhelpers.contrib.storage.linux.utils import is_block_device, zap_disk
 from charmhelpers.contrib.storage.linux.loopback import ensure_loopback_device
@@ -860,13 +862,23 @@
     return wrap
 
 
-def set_os_workload_status(configs, required_interfaces, charm_func=None):
+def set_os_workload_status(configs, required_interfaces, charm_func=None, services=None, ports=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.
+
+    This function also checks for whether the services defined are ACTUALLY
+    running and that the ports they advertise are open and being listened to.
+
+    @param services - OPTIONAL: a [{'service': <string>, 'ports': [<int>]]
+                      The ports are optional.
+                      If services is a [<string>] then ports are ignored.
+    @param ports - OPTIONAL: an [<int>] representing ports that shoudl be
+                   open.
+    @returns None
     """
     incomplete_rel_data = incomplete_relation_data(configs, required_interfaces)
     state = 'active'
@@ -945,6 +957,65 @@
             else:
                 message = charm_message
 
+    # If the charm thinks the unit is active, check that the actual services
+    # really are active.
+    if services is not None and state == 'active':
+        # if we're passed the dict() then just grab the values as a list.
+        if isinstance(services, dict):
+            services = services.values()
+        # either extract the list of services from the dictionary, or if
+        # it is a simple string, use that. i.e. works with mixed lists.
+        _s = []
+        for s in services:
+            if isinstance(s, dict) and 'service' in s:
+                _s.append(s['service'])
+            if isinstance(s, str):
+                _s.append(s)
+        services_running = [service_running(s) for s in _s]
+        if not all(services_running):
+            not_running = [s for s, running in zip(_s, services_running)
+                           if not running]
+            message = ("Services not running that should be: {}"
+                       .format(", ".join(not_running)))
+            state = 'blocked'
+        # also verify that the ports that should be open are open
+        # NB, that ServiceManager objects only OPTIONALLY have ports
+        port_map = OrderedDict([(s['service'], s['ports'])
+                                for s in services if 'ports' in s])
+        if state == 'active' and port_map:
+            all_ports = list(itertools.chain(*port_map.values()))
+            ports_open = [port_has_listener('0.0.0.0', p)
+                          for p in all_ports]
+            if not all(ports_open):
+                not_opened = [p for p, opened in zip(all_ports, ports_open)
+                              if not opened]
+                map_not_open = OrderedDict()
+                for service, ports in port_map.items():
+                    closed_ports = set(ports).intersection(not_opened)
+                    if closed_ports:
+                        map_not_open[service] = closed_ports
+                # find which service has missing ports. They are in service
+                # order which makes it a bit easier.
+                message = (
+                    "Services with ports not open that should be: {}"
+                    .format(
+                        ", ".join([
+                            "{}: [{}]".format(
+                                service,
+                                ", ".join([str(v) for v in ports]))
+                            for service, ports in map_not_open.items()])))
+                state = 'blocked'
+
+    if ports is not None and state == 'active':
+        # and we can also check ports which we don't know the service for
+        ports_open = [port_has_listener('0.0.0.0', p) for p in ports]
+        if not all(ports_open):
+            message = (
+                "Ports which should be open, but are not: {}"
+                .format(", ".join([str(p) for p, v in zip(ports, ports_open)
+                                   if not v])))
+            state = 'blocked'
+
     # Set to active if all requirements have been met
     if state == 'active':
         message = "Unit is ready"

=== modified file 'tests/contrib/network/test_ip.py'
--- tests/contrib/network/test_ip.py	2015-12-14 15:17:42 +0000
+++ tests/contrib/network/test_ip.py	2016-02-12 11:00:52 +0000
@@ -703,3 +703,12 @@
         with patch(builtin_import, side_effect=[fake_dns]):
             hn = net_ip.get_hostname('4.2.2.1')
         self.assertEquals(hn, "www.ubuntu.com")
+
+    @patch('charmhelpers.contrib.network.ip.subprocess.call')
+    def test_port_has_listener(self, subprocess_call):
+        subprocess_call.return_value = 1
+        self.assertEqual(net_ip.port_has_listener('ip-address', 50), False)
+        subprocess_call.assert_called_with(['nc', '-z', 'ip-address', '50'])
+        subprocess_call.return_value = 0
+        self.assertEqual(net_ip.port_has_listener('ip-address', 70), True)
+        subprocess_call.assert_called_with(['nc', '-z', 'ip-address', '70'])

=== modified file 'tests/contrib/openstack/test_openstack_utils.py'
--- tests/contrib/openstack/test_openstack_utils.py	2016-02-11 15:34:00 +0000
+++ tests/contrib/openstack/test_openstack_utils.py	2016-02-12 11:00:52 +0000
@@ -1021,6 +1021,165 @@
         self.assertTrue(actual_parm1 == 'blocked')
         self.assertTrue(actual_parm2 == expected1 or actual_parm2 == expected2)
 
+    @patch('charmhelpers.contrib.openstack.utils.service_running')
+    @patch('charmhelpers.contrib.openstack.utils.port_has_listener')
+    @patch.object(openstack, 'juju_log')
+    @patch('charmhelpers.contrib.openstack.utils.status_set')
+    def test_set_os_workload_status_complete_with_services_list(
+            self, status_set, log, port_has_listener, service_running):
+        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']}
+
+        services = ['database', 'identity']
+        # Assume that the service and ports are open.
+        port_has_listener.return_value = True
+        service_running.return_value = True
+
+        openstack.set_os_workload_status(
+            configs, required_interfaces, services=services)
+        status_set.assert_called_with('active', 'Unit is ready')
+
+    @patch('charmhelpers.contrib.openstack.utils.service_running')
+    @patch('charmhelpers.contrib.openstack.utils.port_has_listener')
+    @patch.object(openstack, 'juju_log')
+    @patch('charmhelpers.contrib.openstack.utils.status_set')
+    def test_set_os_workload_status_complete_services_list_not_running(
+            self, status_set, log, port_has_listener, service_running):
+        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']}
+
+        services = ['database', 'identity']
+        port_has_listener.return_value = True
+        # Fail the identity service
+        service_running.side_effect = [True, False]
+
+        openstack.set_os_workload_status(
+            configs, required_interfaces, services=services)
+        status_set.assert_called_with(
+            'blocked',
+            'Services not running that should be: identity')
+
+    @patch('charmhelpers.contrib.openstack.utils.service_running')
+    @patch('charmhelpers.contrib.openstack.utils.port_has_listener')
+    @patch.object(openstack, 'juju_log')
+    @patch('charmhelpers.contrib.openstack.utils.status_set')
+    def test_set_os_workload_status_complete_with_services(
+            self, status_set, log, port_has_listener, service_running):
+        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']}
+
+        services = [
+            {'service': 'database', 'ports': [10, 20]},
+            {'service': 'identity', 'ports': [30]},
+        ]
+        # Assume that the service and ports are open.
+        port_has_listener.return_value = True
+        service_running.return_value = True
+
+        openstack.set_os_workload_status(
+            configs, required_interfaces, services=services)
+        status_set.assert_called_with('active', 'Unit is ready')
+
+    @patch('charmhelpers.contrib.openstack.utils.service_running')
+    @patch('charmhelpers.contrib.openstack.utils.port_has_listener')
+    @patch.object(openstack, 'juju_log')
+    @patch('charmhelpers.contrib.openstack.utils.status_set')
+    def test_set_os_workload_status_complete_service_not_running(
+            self, status_set, log, port_has_listener, service_running):
+        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']}
+
+        services = [
+            {'service': 'database', 'ports': [10, 20]},
+            {'service': 'identity', 'ports': [30]},
+        ]
+        port_has_listener.return_value = True
+        # Fail the identity service
+        service_running.side_effect = [True, False]
+
+        openstack.set_os_workload_status(
+            configs, required_interfaces, services=services)
+        status_set.assert_called_with(
+            'blocked',
+            'Services not running that should be: identity')
+
+    @patch('charmhelpers.contrib.openstack.utils.service_running')
+    @patch('charmhelpers.contrib.openstack.utils.port_has_listener')
+    @patch.object(openstack, 'juju_log')
+    @patch('charmhelpers.contrib.openstack.utils.status_set')
+    def test_set_os_workload_status_complete_port_not_open(
+            self, status_set, log, port_has_listener, service_running):
+        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']}
+
+        services = [
+            {'service': 'database', 'ports': [10, 20]},
+            {'service': 'identity', 'ports': [30]},
+        ]
+        port_has_listener.side_effect = [True, False, True]
+        # Fail the identity service
+        service_running.return_value = True
+
+        openstack.set_os_workload_status(
+            configs, required_interfaces, services=services)
+        status_set.assert_called_with(
+            'blocked',
+            'Services with ports not open that should be:'
+            ' database: [20]')
+
+    @patch('charmhelpers.contrib.openstack.utils.port_has_listener')
+    @patch.object(openstack, 'juju_log')
+    @patch('charmhelpers.contrib.openstack.utils.status_set')
+    def test_set_os_workload_status_complete_ports_not_open(
+            self, status_set, log, port_has_listener):
+        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']}
+
+        ports = [50, 60, 70]
+        port_has_listener.side_effect = [True, False, True]
+
+        openstack.set_os_workload_status(
+            configs, required_interfaces, ports=ports)
+        status_set.assert_called_with(
+            'blocked',
+            'Ports which should be open, but are not: 60')
+
     @patch.object(openstack, 'juju_log')
     @patch.object(openstack, 'action_set')
     @patch.object(openstack, 'action_fail')