Merge lp:~ajkavanagh/charms/trusty/keystone/add-service-checks-lp1524388 into lp:~openstack-charmers-archive/charms/trusty/keystone/next

[Alex Kavanagh]

For review/comments only:

  Synced in lp:~ajkavanagh/charm-helpers/add-service-checks-lp1524388 version
  of charm-helpers to test that modification will work - this is for demo
  purposes only and will be revised prior to merge proposal.

  The modified version of charm-helpers actually checks if the service is running
  and listening on any ports specified. This change to keystone adds the service
  and port checks as part of assess_status().

  Also changed tests to to work with revision 514:
  "misc changes for haproxy template and context ..."
  which required changes to the test_haproxy_context_service_enabled test

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #386 keystone-next for ajkavanagh mp285608
    LINT OK: passed

Build: http://10.245.162.36:8080/job/charm_lint_check/386/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #307 keystone-next for ajkavanagh mp285608
    UNIT OK: passed

Build: http://10.245.162.36:8080/job/charm_unit_test/307/

[David Ames (thedac)]

This looks great. And will be ready to merge after two more steps.

One, we have some text conflicts that need to be resolved.

Two, my manual run of unit tests are failing due to issues not related to your change. I'd like to see the result of the automated run of unit tests so I have moved this from WIP to needs review which should kick that off.

review: Needs Fixing

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #157 keystone-next for ajkavanagh mp285608
    AMULET OK: passed

Build: http://10.245.162.36:8080/job/charm_amulet_test/157/

[Alex Kavanagh (ajkavanagh)]

> One, we have some text conflicts that need to be resolved.

I'm guessing this means some more changes in keystone?

I'm also guessing we need to get the charm-helpers change(s) merged first, then re-sync on keystone (on this one) + any further changes on keystone, and get that in as a merge? Also, did the 'determine_ports()' change alleviate your concerns around ports and HA?

Thanks again for the review :)

[David Ames (thedac)]

Alex, Sorry about this I meant my comments above to be on the Charm helpers MP.

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #832 keystone-next for ajkavanagh mp285608
    LINT OK: passed

Build: http://10.245.162.36:8080/job/charm_lint_check/832/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #735 keystone-next for ajkavanagh mp285608
    UNIT OK: passed

Build: http://10.245.162.36:8080/job/charm_unit_test/735/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #333 keystone-next for ajkavanagh mp285608
    AMULET OK: passed

Build: http://10.245.162.36:8080/job/charm_amulet_test/333/

[David Ames (thedac)]

> > One, we have some text conflicts that need to be resolved.
>
> I'm guessing this means some more changes in keystone?

We still have text conflicts for this one.

bzr branch lp:~openstack-charmers/charms/trusty/keystone/next
cd keystone
bzr merge lp:~ajkavanagh/charms/trusty/keystone/add-service-checks-lp1524388
bzr st # shows text conflicts

> I'm also guessing we need to get the charm-helpers change(s) merged first,
> then re-sync on keystone (on this one) + any further changes on keystone, and
> get that in as a merge? Also, did the 'determine_ports()' change alleviate
> your concerns around ports and HA?
>
> Thanks again for the review :)

CH changes were merged. They may need to be resynced to fix part of the above text conflicts.

I like the determine_ports() solution. We might want to add the ports the keystone service is listening on as well (not haproxy) for extra thouroughness. 35347 and 4990 while still including the well known ports 35357 and 5000.

After the above is resolved, we can merge this thing.

review: Needs Fixing

[lp:~ajkavanagh/charms/trusty/keystone/add-service-checks-lp1524388]

201. By Alex Kavanagh on 2016-02-19

Resync charmhelpers to current version

202. By Alex Kavanagh on 2016-02-19

Merge keystone/next into change to get ready for final merge

[Alex Kavanagh (ajkavanagh)]

> > > One, we have some text conflicts that need to be resolved.
> >
> > I'm guessing this means some more changes in keystone?
>
> We still have text conflicts for this one.
>
> bzr branch lp:~openstack-charmers/charms/trusty/keystone/next
> cd keystone
> bzr merge lp:~ajkavanagh/charms/trusty/keystone/add-service-checks-lp1524388
> bzr st # shows text conflicts
>
> > I'm also guessing we need to get the charm-helpers change(s) merged first,
> > then re-sync on keystone (on this one) + any further changes on keystone,
> and
> > get that in as a merge? Also, did the 'determine_ports()' change alleviate
> > your concerns around ports and HA?
> >
> > Thanks again for the review :)
>
> CH changes were merged. They may need to be resynced to fix part of the above
> text conflicts.
>
> I like the determine_ports() solution. We might want to add the ports the
> keystone service is listening on as well (not haproxy) for extra
> thouroughness. 35347 and 4990 while still including the well known ports 35357
> and 5000.
>
> After the above is resolved, we can merge this thing.

Sortee the CH changes and also merged latest keystone/next into the branch.

Re the extra ports, would you like them hard-coded into the file, or determined from config (if there is any?)

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #917 keystone-next for ajkavanagh mp285608
    LINT OK: passed

Build: http://10.245.162.36:8080/job/charm_lint_check/917/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #817 keystone-next for ajkavanagh mp285608
    UNIT OK: passed

Build: http://10.245.162.36:8080/job/charm_unit_test/817/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #363 keystone-next for ajkavanagh mp285608
    AMULET OK: passed

Build: http://10.245.162.36:8080/job/charm_amulet_test/363/

[David Ames (thedac)]

This looks good. We can discuss the other ports in our next meeting and do a trivial push if needed.
Merging.

review: Approve

=== modified file 'charmhelpers/contrib/network/ip.py'
--- charmhelpers/contrib/network/ip.py	2016-01-04 21:27:51 +0000
+++ charmhelpers/contrib/network/ip.py	2016-02-19 14:51:35 +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/context.py'
--- charmhelpers/contrib/openstack/context.py	2016-02-11 14:43:33 +0000
+++ charmhelpers/contrib/openstack/context.py	2016-02-19 14:51:35 +0000
@@ -410,6 +410,7 @@
                 auth_host = format_ipv6_addr(auth_host) or auth_host
                 svc_protocol = rdata.get('service_protocol') or 'http'
                 auth_protocol = rdata.get('auth_protocol') or 'http'
+                api_version = rdata.get('api_version') or '2.0'
                 ctxt.update({'service_port': rdata.get('service_port'),
                              'service_host': serv_host,
                              'auth_host': auth_host,
@@ -418,7 +419,8 @@
                              'admin_user': rdata.get('service_username'),
                              'admin_password': rdata.get('service_password'),
                              'service_protocol': svc_protocol,
-                             'auth_protocol': auth_protocol})
+                             'auth_protocol': auth_protocol,
+                             'api_version': api_version})
 
                 if self.context_complete(ctxt):
                     # NOTE(jamespage) this is required for >= icehouse
@@ -1471,6 +1473,8 @@
                     rdata.get('service_protocol') or 'http',
                     'auth_protocol':
                     rdata.get('auth_protocol') or 'http',
+                    'api_version':
+                    rdata.get('api_version') or '2.0',
                 }
                 if self.context_complete(ctxt):
                     return ctxt
 
=== modified file 'charmhelpers/contrib/openstack/templates/section-keystone-authtoken'
--- charmhelpers/contrib/openstack/templates/section-keystone-authtoken	2015-03-25 09:18:33 +0000
+++ charmhelpers/contrib/openstack/templates/section-keystone-authtoken	2016-02-19 14:51:35 +0000
@@ -1,4 +1,14 @@
 {% if auth_host -%}
+{% if api_version == '3' -%}
+[keystone_authtoken]
+auth_url = {{ service_protocol }}://{{ service_host }}:{{ service_port }}
+project_name = {{ admin_tenant_name }}
+username = {{ admin_user }}
+password = {{ admin_password }}
+project_domain_name = default
+user_domain_name = default
+auth_plugin = password
+{% else -%}
 [keystone_authtoken]
 identity_uri = {{ auth_protocol }}://{{ auth_host }}:{{ auth_port }}/{{ auth_admin_prefix }}
 auth_uri = {{ service_protocol }}://{{ service_host }}:{{ service_port }}/{{ service_admin_prefix }}
@@ -7,3 +17,4 @@
 admin_password = {{ admin_password }}
 signing_dir = {{ signing_dir }}
 {% endif -%}
+{% endif -%}
 
=== modified file 'charmhelpers/contrib/openstack/utils.py'
--- charmhelpers/contrib/openstack/utils.py	2016-02-11 15:44:13 +0000
+++ charmhelpers/contrib/openstack/utils.py	2016-02-19 14:51:35 +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 'hooks/keystone_utils.py'
--- hooks/keystone_utils.py	2016-02-18 10:02:59 +0000
+++ hooks/keystone_utils.py	2016-02-19 14:51:35 +0000
@@ -1871,4 +1871,5 @@
 
     # set the status according to the current state of the contexts
     set_os_workload_status(
-        configs, REQUIRED_INTERFACES, charm_func=check_optional_relations)
+        configs, REQUIRED_INTERFACES, charm_func=check_optional_relations,
+        services=services(), ports=determine_ports())
 
=== modified file 'unit_tests/test_keystone_utils.py'
--- unit_tests/test_keystone_utils.py	2016-01-19 16:54:03 +0000
+++ unit_tests/test_keystone_utils.py	2016-02-19 14:51:35 +0000
@@ -758,4 +758,6 @@
             set_os_workload_status.assert_called_with(
                 "TEST CONFIG",
                 utils.REQUIRED_INTERFACES,
-                charm_func=utils.check_optional_relations)
+                charm_func=utils.check_optional_relations,
+                services=['haproxy', 'keystone', 'apache2'],
+                ports=[5000, 35357])