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

[Jeremy Lounder]

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.

[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:)

[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

[Celia Wang (ziyiwang)]

LGTM.
I'm going to change the status to "Approved" for merging.

review: Approve

[Jose Guedez (jfguedez)]

+1. Had already been approved before being re-submitted.

review: Approve

[James Hebden (ec0)]

Looks good to me, thanks

review: Approve

[Alvaro Uria (aluria)]

I haven't reviewed the code since it was already approved, but I have run "make test" and I caught 3 issues on func tests:
1) test_deploy_app relates "keystone" and "openstack-service-checks", but there is ambiguity between two interfaces: identity-credentials and identity-notifications. "keystone:identity-credentials" needs to be used.

2) Xenial test fails because octavia packages can't be located. "check-octavia" default value should be False, or at least False in Xenial. https://pastebin.ubuntu.com/p/gmRRbCsPnr/

3) Bionic tests fail because file_stat fails:
"""
> return json.loads(results['Stdout'])
E KeyError: 'Stdout'
"""

review: Needs Fixing

[Joe Guo (guoqiao)]

Hi Alvaro,
Thanks for the review.
For 2), I can set it to False.
For 1) 3), they are issues from existing functional test, not related to this patch, maybe we can create a separate bug for that?

[Joe Guo (guoqiao)]

Hi Alvaro,
For 2), I've add a condition check:

    if host.lsb_release()['DISTRIB_RELEASE'] >= '18.04':
        ...

That will bypass octavia checks on 16.04 even it's enabled.
With this, I can keep check-octavia default to True.

For 1) 3), I have trouble to run functional test on my laptop(can not connect to charm store, not sure why). But I've asked help from Jose to prove that they will also happen on master branch. We will create a bug for that soon.

[Jeremy Lounder (jldev)]

review: Approve

[Joe Guo (guoqiao)]

Hi Alvaro,
The bug for functional tests has been created at:

https://bugs.launchpad.net/charm-openstack-service-checks/+bug/1869308

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 716512b..3111b92 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,30 @@ class OSCHelper():
         else:
             nrpe.remove_check(shortname='neutron_agents')
 
+        # only care about octavia after 18.04
+        if host.lsb_release()['DISTRIB_RELEASE'] >= '18.04':
+            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'),