Merge ~canonical-bootstack/charm-openstack-service-checks:check-loadbalancers into ~canonical-bootstack/charm-openstack-service-checks:master

[Joe Guo]

Add checks for loadbalancers.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[James Hebden (ec0)]

Please see comments inline.
Overall, looks really cool!

One other check I would suggest, is verifying that the image exists in glance for booting new Amphorae if possible. The tag used by Octavia should be available as the amp-image-tag setting on Octavia, so we should either provide a setting on this charm to configure the tag we will monitor (i.e. a string setting called octavia-image-tag with default of 'amp-image-tag') or get this information from Octavia. We can then check that there is an image with this tag present in Glance, as if the image is deleted, we have a problem, because new amphora can not be booted.

I'd be happy to approve this with the below changes, even without the glance check, and we can handle the glance check as a follow up activity.

18472a4... by Joe Guo on 2019-12-23

add octavia checks

Signed-off-by: Joe Guo <email address hidden>

[Alvaro Uria (aluria)]

I've added a comment on a minor typo. OTOH, the best practice suggested for charms is to have as many things autoconfigured as possible (ie. reduce config parameters). So, I would suggest a new interface between charm-octavia and charm-openstack-service-checks which:
1) It would make "check-loadbalancer" be "True", which would create the nrpe check (as James mentioned, an extra config param could exist in charm-openstack-service-checks to disable certain "modules" (lb, amphorae, pools...)
2) The relation would share the amp-image-tag value automatically (if not set, charm-openstack-service-checks would only check for any image with the "octavia-amphora" tag.
3) Possibly do the same for the flavor id, which is a config param in Octavia (however, I don't know the way to guess if a flavor that will be used by the Amphorae exists - the default is that charm-octavia handles it, so an internal value probably exists)

Thank you for the change. Is there a bug we can link this MP to?

[Joe Guo (guoqiao)]

Hi,
Thanks for the review and feedback.
I've fixed all, and refactored the code to make it more flexible/reasonable.

Could you please review again when you get the time?

NOTE: for image check, it hardcoded `amp_img_tag` to `octavia-amphora` at the moment, and it's disabled by default. Work in progress to find a proper/minimal way to set it.

[James Hebden (ec0)]

Just a couple more comments, around config.yaml and default checks.

[Joe Guo (guoqiao)]

> Just a couple more comments, around config.yaml and default checks.

Hi James,
I've made the changes, please review again when you get the time. Thanks:)

0bfc801... by Jeremy Lounder on 2020-03-03

Updated maintainers in metadata.yaml

bd33e8e... by Xav Paice on 2020-03-09

Add ability to remove NRPE endpoint checks

When a service is removed from the Keystone catalog, this adds a
mechanism to remove that service from the list of NRPE checks so it
doesn't alert CRITICAL for services that aren't there any more.

Closes-Bug: LP: #1836385

[Jose Guedez (jfguedez)]

comments inline...

[Joe Guo (guoqiao)]

Hi Jose,
Thanks for the great review, they are all good advice. I will make those changes in my new Engineer rotation. Thanks.

[Joe Guo (guoqiao)]

Hi Jose,
I've made following changes:
1. split octavia nagios check into 4(loadbalancers|amphorae|pools|image), so I don't need to handle the complex logic of which ERROR to return
2. remove the image tag and days default on check_image function. But I still have them on both cli option and config.yaml since they are different levels.

I didn't import nagios3_plugin module for the status codes, so I can keep this script standalone, and run it from any location.

Another review appreciated:)

[Jose Guedez (jfguedez)]

Hi Joe - Good stuff, much cleaner now. Comments inline. Just one real comment with the functional test and the new filenames of the checks.

[Joe Guo (guoqiao)]

Hi Jose,
Changed and replied, see my inline replies.

BTW: I've verified the new octavia checks are added correctly with openstack-on-lxd env: https://pastebin.canonical.com/p/D26hdX3KDH/

[Jose Guedez (jfguedez)]

+1 LGTM

review: Approve

Unmerged commits

18472a4... by Joe Guo on 2019-12-23

add octavia checks

Signed-off-by: Joe Guo <email address hidden>

bd33e8e... by Xav Paice on 2020-03-09

Add ability to remove NRPE endpoint checks

When a service is removed from the Keystone catalog, this adds a
mechanism to remove that service from the list of NRPE checks so it
doesn't alert CRITICAL for services that aren't there any more.

Closes-Bug: LP: #1836385

0bfc801... by Jeremy Lounder on 2020-03-03

Updated maintainers in metadata.yaml

diff --git a/actions.yaml b/actions.yaml
new file mode 100644
index 0000000..9e5a96a
--- /dev/null
+++ b/actions.yaml
@@ -0,0 +1,3 @@
+refresh-endpoint-checks:
+  description: >-
+    Trigger the Keystone endpoints to be reloaded on next update-status hook.
diff --git a/actions/actions.py b/actions/actions.py
new file mode 100755
index 0000000..1e679b6
--- /dev/null
+++ b/actions/actions.py
@@ -0,0 +1,75 @@
+#!/usr/local/sbin/charm-env python3
+# Copyright 2020 Canonical Ltd
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#  http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import os
+import sys
+import charmhelpers.core.hookenv as hookenv
+import charmhelpers.core.unitdata as unitdata
+from traceback import format_exc
+
+# Load modules from $CHARM_DIR/lib
+sys.path.append("lib")
+
+import charms.reactive  # NOQA: E402
+from charms.layer import basic  # NOQA: E402
+
+basic.bootstrap_charm_deps()
+basic.init_config_states()
+
+from charms.reactive.flags import clear_flag  # NOQA: E402
+
+
+def refresh_endpoint_checks(*args):
+    """Clear the openstack-service-checks.endpoints.configured flag
+    so that next time update-status runs, the Keystone catalog is re-read
+    and nrpe checks refreshed.
+    """
+    clear_flag("openstack-service-checks.endpoints.configured")
+
+
+# Actions to function mapping, to allow for illegal python action names that
+# can map to a python function.
+ACTIONS = {
+    "refresh-endpoint-checks": refresh_endpoint_checks,
+}
+
+
+def main(args):
+    action_name = os.path.basename(args[0])
+    try:
+        action = ACTIONS[action_name]
+    except KeyError:
+        return "Action %s undefined" % action_name
+    else:
+        try:
+            action(args)
+        except Exception:
+            exc = format_exc()
+            hookenv.log(exc, hookenv.ERROR)
+            hookenv.action_fail(exc.splitlines()[-1])
+        else:
+            # we were successful, so commit changes from the action
+            unitdata.kv().flush()
+            # try running handlers based on new state
+            try:
+                charms.reactive.main()
+            except Exception:
+                exc = format_exc()
+                hookenv.log(exc, hookenv.ERROR)
+                hookenv.action_fail(exc.splitlines()[-1])
+
+
+if __name__ == "__main__":
+    sys.exit(main(sys.argv))
diff --git a/actions/refresh-endpoint-checks b/actions/refresh-endpoint-checks
new file mode 120000
index 0000000..405a394
--- /dev/null
+++ b/actions/refresh-endpoint-checks
@@ -0,0 +1 @@
+actions.py
\ No newline at end of file
diff --git a/config.yaml b/config.yaml
index 073ba9b..edfda51 100644
--- a/config.yaml
+++ b/config.yaml
@@ -10,6 +10,24 @@ options:
     description: |
       Switch to turn on or off neutron agents checks. By default, neutron_agents nrpe check is enabled.
       If a different SDN (ie. Contrail) is in use, you may want to disable this check.
+  check-octavia:
+    default: True
+    type: boolean
+    description: |
+      Switch to turn on or off check for octavia services.
+  octavia-amp-image-tag:
+    default: "octavia-amphora"
+    type: string
+    description: |
+      The glance image tag octavia will use to create amphora.
+  octavia-amp-image-days:
+    default: 365
+    type: int
+    description: |
+      If latest glance image tagged with above octavia-amp-image-tag is updated more than these days ago,
+      a Nagios warning will be raised. The version of octavia agent builtin in amphora image must match
+      version of octavia controller, otherwise octavia will fail to communicate with new amphora,
+      failover will also fail.
   check-rally:
     default: False
     type: boolean
diff --git a/files/plugins/check_octavia.py b/files/plugins/check_octavia.py
new file mode 100755
index 0000000..09fb6b1
--- /dev/null
+++ b/files/plugins/check_octavia.py
@@ -0,0 +1,227 @@
+#!/usr/bin/env python3
+
+import os
+import sys
+import json
+import argparse
+import subprocess
+from datetime import datetime, timedelta
+import openstack
+
+NAGIOS_STATUS_OK = 0
+NAGIOS_STATUS_WARNING = 1
+NAGIOS_STATUS_CRITICAL = 2
+NAGIOS_STATUS_UNKNOWN = 3
+
+NAGIOS_STATUS = {
+    NAGIOS_STATUS_OK: 'OK',
+    NAGIOS_STATUS_WARNING: 'WARNING',
+    NAGIOS_STATUS_CRITICAL: 'CRITICAL',
+    NAGIOS_STATUS_UNKNOWN: 'UNKNOWN',
+}
+
+
+def nagios_exit(status, message):
+    assert status in NAGIOS_STATUS, "Invalid Nagios status code"
+    # prefix status name to message
+    output = '{}: {}'.format(NAGIOS_STATUS[status], message)
+    print(output)  # nagios requires print to stdout, no stderr
+    sys.exit(status)
+
+
+def check_loadbalancers(connection):
+    """check loadbalancers status."""
+
+    lb_mgr = connection.load_balancer
+    lb_all = lb_mgr.load_balancers()
+
+    # only check enabled lbs
+    lb_enabled = [lb for lb in lb_all if lb.is_admin_state_up]
+
+    # check provisioning_status is ACTIVE for each lb
+    bad_lbs = [lb for lb in lb_enabled if lb.provisioning_status != 'ACTIVE']
+    if bad_lbs:
+        parts = ['loadbalancer {} provisioning_status is {}'.format(
+            lb.id, lb.provisioning_status) for lb in bad_lbs]
+        message = ', '.join(parts)
+        return NAGIOS_STATUS_CRITICAL, message
+
+    # raise WARNING if operating_status is not ONLINE
+    bad_lbs = [lb for lb in lb_enabled if lb.operating_status != 'ONLINE']
+    if bad_lbs:
+        parts = ['loadbalancer {} operating_status is {}'.format(
+            lb.id, lb.operating_status) for lb in bad_lbs]
+        message = ', '.join(parts)
+        return NAGIOS_STATUS_CRITICAL, message
+
+    net_mgr = connection.network
+    # check vip port exists for each lb
+    bad_lbs = []
+    for lb in lb_enabled:
+        try:
+            net_mgr.get_port(lb.vip_port_id)
+        except openstack.exceptions.NotFoundException:
+            bad_lbs.append(lb)
+    if bad_lbs:
+        parts = ['vip port {} for loadbalancer {} not found'.format(
+            lb.vip_port_id, lb.id) for lb in bad_lbs]
+        message = ', '.join(parts)
+        return NAGIOS_STATUS_CRITICAL, message
+
+    # warn about disabled lbs if no other error found
+    lb_disabled = [lb for lb in lb_all if not lb.is_admin_state_up]
+    if lb_disabled:
+        parts = ['loadbalancer {} admin_state_up is False'.format(lb.id)
+                 for lb in lb_disabled]
+        message = ', '.join(parts)
+        return NAGIOS_STATUS_WARNING, message
+
+    return NAGIOS_STATUS_OK, 'loadbalancers are happy'
+
+
+def check_pools(connection):
+    """check pools status."""
+    lb_mgr = connection.load_balancer
+    pools_all = lb_mgr.pools()
+    pools_enabled = [pool for pool in pools_all if pool.is_admin_state_up]
+
+    # check provisioning_status is ACTIVE for each pool
+    bad_pools = [pool for pool in pools_enabled if pool.provisioning_status != 'ACTIVE']
+    if bad_pools:
+        parts = ['pool {} provisioning_status is {}'.format(
+            pool.id, pool.provisioning_status) for pool in bad_pools]
+        message = ', '.join(parts)
+        return NAGIOS_STATUS_CRITICAL, message
+
+    # raise CRITICAL if operating_status is ERROR
+    bad_pools = [pool for pool in pools_enabled if pool.operating_status == 'ERROR']
+    if bad_pools:
+        parts = ['pool {} operating_status is {}'.format(
+            pool.id, pool.operating_status) for pool in bad_pools]
+        message = ', '.join(parts)
+        return NAGIOS_STATUS_CRITICAL, message
+
+    # raise WARNING if operating_status is NO_MONITOR
+    bad_pools = [pool for pool in pools_enabled if pool.operating_status == 'NO_MONITOR']
+    if bad_pools:
+        parts = ['pool {} operating_status is {}'.format(
+            pool.id, pool.operating_status) for pool in bad_pools]
+        message = ', '.join(parts)
+        return NAGIOS_STATUS_WARNING, message
+
+    return NAGIOS_STATUS_OK, 'pools are happy'
+
+
+def check_amphorae(connection):
+    """check amphorae status."""
+
+    lb_mgr = connection.load_balancer
+
+    resp = lb_mgr.get('/v2/octavia/amphorae')
+    # python api is not available yet, use url
+    if resp.status_code != 200:
+        return NAGIOS_STATUS_WARNING, 'amphorae api not working'
+
+    data = json.loads(resp.content)
+    # ouput is like {"amphorae": [{...}, {...}, ...]}
+    items = data.get('amphorae', [])
+
+    # raise CRITICAL for ERROR status
+    bad_status_list = ('ERROR',)
+    bad_items = [item for item in items if item['status'] in bad_status_list]
+    if bad_items:
+        parts = [
+            'amphora {} status is {}'.format(item['id'], item['status'])
+            for item in bad_items]
+        message = ', '.join(parts)
+        return NAGIOS_STATUS_CRITICAL, message
+
+    # raise WARNING for these status
+    bad_status_list = (
+        'PENDING_CREATE', 'PENDING_UPDATE', 'PENDING_DELETE', 'BOOTING')
+    bad_items = [item for item in items if item['status'] in bad_status_list]
+    if bad_items:
+        parts = [
+            'amphora {} status is {}'.format(item['id'], item['status'])
+            for item in bad_items]
+        message = ', '.join(parts)
+        return NAGIOS_STATUS_WARNING, message
+
+    return NAGIOS_STATUS_OK, 'amphorae are happy'
+
+
+def check_image(connection, tag, days):
+    img_mgr = connection.image
+    images = list(img_mgr.images(tag=tag))
+
+    if not images:
+        message = ('Octavia requires image with tag {} to create amphora, '
+                   'but none exist').format(tag)
+        return NAGIOS_STATUS_CRITICAL, message
+
+    active_images = [image for image in images if image.status == 'active']
+    if not active_images:
+        parts = ['{}({})'.format(image.name, image.id) for image in images]
+        message = ('Octavia requires image with tag {} to create amphora, '
+                   'but none is active: {}').format(tag, ', '.join(parts))
+        return NAGIOS_STATUS_CRITICAL, message
+
+    # raise WARNING if image is too old
+    when = (datetime.now() - timedelta(days=days)).isoformat()
+    # updated_at str format: '2019-12-05T18:21:25Z'
+    fresh_images = [image for image in active_images if image.updated_at > when]
+    if not fresh_images:
+        message = ('Octavia requires image with tag {} to create amphora, '
+                   'but it is older than {} days').format(tag, days)
+        return NAGIOS_STATUS_WARNING, message
+
+    return NAGIOS_STATUS_OK, 'image is ready'
+
+
+if __name__ == '__main__':
+    parser = argparse.ArgumentParser(
+        description='Check Octavia status',
+        formatter_class=argparse.ArgumentDefaultsHelpFormatter,
+    )
+    parser.add_argument(
+        '--env', dest='env', default='/var/lib/nagios/nagios.novarc',
+        help='Novarc file to use for this check')
+
+    check_choices = ['loadbalancers', 'amphorae', 'pools', 'image']
+    parser.add_argument(
+        '--check', dest='check', metavar='|'.join(check_choices),
+        type=str, choices=check_choices,
+        default=check_choices[0],
+        help='which check to run')
+
+    parser.add_argument(
+        '--amp-image-tag', dest='amp_image_tag', default='octavia-amphora',
+        help='amphora image tag for image check')
+
+    parser.add_argument(
+        '--amp-image-days', dest='amp_image_days', type=int, default=365,
+        help='raise warning if amphora image is older than these days')
+
+    args = parser.parse_args()
+    # source environment vars
+    command = ['/bin/bash', '-c', 'source {} && env'.format(args.env)]
+    proc = subprocess.Popen(command, stdout=subprocess.PIPE)
+    for line in proc.stdout:
+        (key, _, value) = line.partition(b'=')
+        os.environ[key.decode('utf-8')] = value.rstrip().decode('utf-8')
+    proc.communicate()
+
+    # use closure to make all checks have same signature
+    # so we can handle them in same way
+    def _check_image(connection):
+        return check_image(connection, args.amp_image_tag, args.amp_image_days)
+
+    checks = {
+        'loadbalancers': check_loadbalancers,
+        'amphorae': check_amphorae,
+        'pools': check_pools,
+        'image': _check_image,
+    }
+
+    connection = openstack.connect(cloud='envvars')
+    nagios_exit(*checks[args.check](connection))
diff --git a/layer.yaml b/layer.yaml
index 4053919..0606d45 100644
--- a/layer.yaml
+++ b/layer.yaml
@@ -20,6 +20,8 @@ options:
       - python3-keystoneclient
       - python3-openstackclient
       - python-openstackclient
+      - python3-octaviaclient
+      - python-octaviaclient
   snap:
     fcbtest:
       channel: stable
diff --git a/lib/lib_openstack_service_checks.py b/lib/lib_openstack_service_checks.py
index 9d123fb..38e5f0f 100644
--- a/lib/lib_openstack_service_checks.py
+++ b/lib/lib_openstack_service_checks.py
@@ -65,6 +65,18 @@ class OSCHelper():
         return self.charm_config['check-neutron-agents']
 
     @property
+    def is_octavia_check_enabled(self):
+        return self.charm_config['check-octavia']
+
+    @property
+    def octavia_amp_image_tag(self):
+        return self.charm_config['octavia-amp-image-tag']
+
+    @property
+    def octavia_amp_image_days(self):
+        return self.charm_config['octavia-amp-image-days']
+
+    @property
     def skipped_rally_checks(self):
         skipped_os_components = self.charm_config['skip-rally'].strip()
         if not skipped_os_components:
@@ -186,6 +198,28 @@ class OSCHelper():
         else:
             nrpe.remove_check(shortname='neutron_agents')
 
+        if self.is_octavia_check_enabled:
+            script = os.path.join(self.plugins_dir, 'check_octavia.py')
+
+            for check in ('loadbalancers', 'amphorae', 'pools'):
+                nrpe.add_check(
+                    shortname='octavia_{}'.format(check),
+                    description='Check octavia {} status'.format(check),
+                    check_cmd='{} --check {}'.format(script, check),
+                )
+
+            # image check has extra args, add it separately
+            check = 'image'
+            nrpe.add_check(
+                shortname='octavia_{}'.format(check),
+                description='Check octavia {} status'.format(check),
+                check_cmd='{} --check {} --amp-image-tag {} --amp-image-days {}'.format(
+                    script, check, self.octavia_amp_image_tag, self.octavia_amp_image_days),
+            )
+        else:
+            for check in ('loadbalancers', 'amphorae', 'pools', 'image'):
+                nrpe.remove_check(shortname='octavia_{}'.format(check))
+
         if self.contrail_analytics_vip:
             contrail_check_command = '{} --host {}'.format(
                 os.path.join(self.plugins_dir, 'check_contrail_analytics_alarms.py'),
@@ -272,7 +306,7 @@ class OSCHelper():
         endpoints = self.keystone_endpoints
         services = [svc for svc in self.keystone_services if svc.enabled]
         nrpe = NRPE()
-        skip_service = set()
+        configured_endpoint_checks = dict()
         for endpoint in endpoints:
             endpoint.service_names = [x.name
                                       for x in services
@@ -281,21 +315,18 @@ class OSCHelper():
             endpoint.healthcheck_url = health_check_params.get(service_name, '/')
 
             # Note(aluria): glance-simplestreams-sync does not provide an API to check
-            if service_name == 'image-stream':
+            # Note(aluria): filter:healthcheck is not configured in Keystone v2
+            # https://docs.openstack.org/keystone/pike/configuration.html#health-check-middleware
+            if service_name == 'image-stream' or service_name == 'keystone':
                 continue
 
             if not hasattr(endpoint, 'interface'):
-                if service_name == 'keystone':
-                    # Note(aluria): filter:healthcheck is not configured in v2
-                    # https://docs.openstack.org/keystone/pike/configuration.html#health-check-middleware
-                    continue
                 for interface in 'admin internal public'.split():
                     old_interface_name = '{}url'.format(interface)
                     if not hasattr(endpoint, old_interface_name):
                         continue
                     endpoint.interface = interface
                     endpoint.url = getattr(endpoint, old_interface_name)
-                    skip_service.add(service_name)
                     break
 
             check_url = urlparse(endpoint.url)
@@ -324,10 +355,26 @@ class OSCHelper():
                                check_cmd=' '.join(cmd_params_cert))
 
             # Add the actual health check for the URL
-            nrpe.add_check(shortname='{}_{}'.format(service_name, endpoint.interface),
+            nrpe_shortname = '{}_{}'.format(service_name, endpoint.interface)
+            nrpe.add_check(shortname=nrpe_shortname,
                            description='Endpoint url check for {} {}'.format(service_name, endpoint.interface),
                            check_cmd=' '.join(cmd_params))
+            configured_endpoint_checks[nrpe_shortname] = True
+        nrpe.write()
+        self._remove_old_nrpe_endpoint_checks(nrpe, configured_endpoint_checks)
 
+    def _remove_old_nrpe_endpoint_checks(self, nrpe, configured_endpoint_checks):
+        """Loop through the old and new endpoint checks, if there are checks that aren't needed any more,
+        remove them.
+        """
+        kv = unitdata.kv()
+        endpoint_delta = kv.delta(configured_endpoint_checks, 'endpoint_checks')
+        kv.update(configured_endpoint_checks, 'endpoint_checks')
+        for nrpe_shortname in endpoint_delta.items():
+            # generates tuples of the format ('heat_public', Delta(previous=None, current=True))
+            # remove any that are not current
+            if not nrpe_shortname[1].current:
+                nrpe.remove_check(shortname=nrpe_shortname[0])
         nrpe.write()
 
     def get_keystone_client(self, creds):
diff --git a/metadata.yaml b/metadata.yaml
index d4b7e87..94a7db8 100644
--- a/metadata.yaml
+++ b/metadata.yaml
@@ -1,7 +1,7 @@
 name: openstack-service-checks
 summary: OpenStack Services NRPE Checks
 description: OpenStack Services NRPE Checks
-maintainer: LMA Charmers <[e-mail]>
+maintainer: Llama (LMA) Charmers <[e-mail]>
 subordinate: false
 tags:
   - openstack
diff --git a/tests/functional/test_deploy.py b/tests/functional/test_deploy.py
index d0eea26..1716339 100644
--- a/tests/functional/test_deploy.py
+++ b/tests/functional/test_deploy.py
@@ -128,6 +128,10 @@ async def test_openstackservicechecks_verify_default_nrpe_checks(deploy_app, mod
     filenames.extend([
         '/etc/nagios/nrpe.d/check_nova_services.cfg',
         '/etc/nagios/nrpe.d/check_neutron_agents.cfg',
+        '/etc/nagios/nrpe.d/check_octavia_loadbalancers.cfg',
+        '/etc/nagios/nrpe.d/check_octavia_amphorae.cfg',
+        '/etc/nagios/nrpe.d/check_octavia_pools.cfg',
+        '/etc/nagios/nrpe.d/check_octavia_image.cfg',
     ])
     for filename in filenames:
         test_stat = await file_stat(filename, unit)