Merge ~xavpaice/charm-nrpe:dev/check_mtu into ~nrpe-charmers/charm-nrpe:master

[Xav Paice]

MTU checker for neutron-gateway

[Giuseppe Petralia (peppepetra)]

Looks good to me.

review: Approve

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

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

[Xav Paice (xavpaice)]

Example output:

./check_mtu.py --br br-int
br-tun: {'tap1da46df0-92': '1500', 'tap2677f6d5-30': '1500', 'tap2f307b01-8f': '1500', 'tap3ec55528-46': '1500', 'tap419c0544-fb': '1500', 'tap459fbadd-fb': '1500', 'tap490f00ee-2a': '1500', 'tap5f347b80-bc': '1500', 'tap650c3391-0b': '1500', 'tap697a22b3-a9': '8900', 'tap6e473310-2e': '1500', 'tap832fb175-1b': '1500', 'tap88db0ccc-0f': '1500', 'tap8a586526-f3': '1500', 'tap8de5df09-4e': '1500', 'tap9486dc0f-39': '1500', 'tap9aa78180-c4': '1500', 'tapaf94b5dc-0e': '1500', 'tapb3729a8b-c9': '1500', 'tapb91190ec-58': '1500', 'tapc88975d5-c0': '1500', 'tapcb431075-db': '1500', 'tapd60ad8ba-3e': '1500', 'tapdf6a3c1b-d2': '1500', 'tapea6c2a8f-07': '1500', 'tapee3b1760-c7': '1500', 'tapeea0bf18-0d': '1500', 'tapf4d69938-c5': '1500', 'tapfd05a248-11': '8900'}
br-int: {'tap697a22b3-a9': '8900', 'tapfd05a248-11': '8900'}
WARNING: found tenant MTUs greater than bridge MTU

The standards for Nagios plugins dicatate that the plugin should output just one line of info, rather than multiple - otherwise Nagios can't process it.

I don't see any implementation of the plugin here, this just delivers the plugin for something else to make use of it - assuming there's a separate change to the neutron-gateway charm or some such, in order to do that.

review: Needs Fixing

[🤖 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)]

Great idea. A few comments inline.

[James Troup (elmo)]

Some commentary.

[James Troup (elmo)]

Yep, replying to myself

[Xav Paice (xavpaice)]

Thanks for the reviews - I added a couple of comments below, and will submit an additional commit to address these issues.

6cf0d35... by Xav Paice on 2020-07-23

Address review comments from check_mtu.py

- some documentation fixes
- change get_mtu() to return an int()
- fix the output detail being reset for each interface

[Xav Paice (xavpaice)]

Ready for another look.

review: Needs Resubmitting

[Paul Goins (vultaire)]

Added my feedback, hopefully I'm not being too nitpicky.

review: Needs Fixing

2728bb5... by Xav Paice on 2020-07-24

update check_mtu arg syntax

[Xav Paice (xavpaice)]

Thanks for that, I've updated the change and the change at https://review.opendev.org/#/c/718168 has been updated to reflect the changed CLI args.

review: Needs Resubmitting

[Paul Goins (vultaire)]

LGTM

review: Approve

[Drew Freiberger (afreiberger)]

minor nit on wording of "OK" response. "is sane" seems judgemental and unscientific. Would be more accurate to say "matches expected value".

[James Troup (elmo)]

Minor commentary; don't feel obliged to block on it.

[Drew Freiberger (afreiberger)]

+1 on the typecasting overkill noted by James

eed1103... by Xav Paice on 2020-07-26

minor rework on check_mtu.py

Adjust some wording and layout in response to review feedback for the
check_mtu.py NRPE check.

[Xav Paice (xavpaice)]

Thanks for these, adjusted and added a fresh commit.

[Xav Paice (xavpaice)]

This needs some rework and confirmation of assumptions, marking WIP and it needs to head back to the backlog (along with an associated bug report which I've not found yet)

review: Needs Fixing

[Xav Paice (xavpaice)]

https://bugs.launchpad.net/charm-neutron-gateway/+bug/1888741

[Xav Paice (xavpaice)]

I think this belongs in charmhelpers (under contrib/openstack/files) or in the neutron-gateway charm itself, rather than in nrpe. I'll draft a change in charmhelpers, since the plugin seems to be something that could be useful outside the neutron-gateway unit itself.

review: Disapprove

Unmerged commits

eed1103... by Xav Paice on 2020-07-26

minor rework on check_mtu.py

Adjust some wording and layout in response to review feedback for the
check_mtu.py NRPE check.

2728bb5... by Xav Paice on 2020-07-24

update check_mtu arg syntax

6cf0d35... by Xav Paice on 2020-07-23

Address review comments from check_mtu.py

- some documentation fixes
- change get_mtu() to return an int()
- fix the output detail being reset for each interface

a70f663... by Xav Paice on 2020-07-23

update check_mtu.py plugin for one line of output

172acab... by David O Neill on 2020-04-08

MTU checker for neutron-gateway

diff --git a/files/plugins/check_mtu.py b/files/plugins/check_mtu.py
new file mode 100755
index 0000000..cdd8854
--- /dev/null
+++ b/files/plugins/check_mtu.py
@@ -0,0 +1,121 @@
+#!/usr/bin/env python3
+
+# Author: David O Neill <[e-mail]>
+
+import argparse
+import subprocess
+
+from nagios_plugin3 import UnknownError, WarnError, try_check
+
+
+# Note:
+# GRE padding typically 22 bytes,
+# The recommended padding for vxlan is 50.
+# Assuming 50 for all encapped networks.
+PADDING = 50
+
+
+def get_mtu(device):
+    """Get the MTU of a device.
+
+    Reads /sys/class/net/ for the device MTU
+
+    Args:
+        device (string): The name of the device
+
+    Returns:
+        int: The MTU of the device, or 0 if there's any kind of error
+    """
+    try:
+        with open("/sys/class/net/" + device + "/mtu") as f:
+            mtu = f.read().strip()
+            return int(mtu)
+    except FileNotFoundError:
+        return 0
+    except AttributeError:
+        return 0
+
+
+def get_taps(bridge):
+    """Return taps array associated with a bridge."""
+    try:
+        process = subprocess.Popen(
+            ["ovs-vsctl", "list-ports", bridge], stdout=subprocess.PIPE
+        )
+        return (process.communicate()[0]).decode("utf-8").split("\n")
+    except subprocess.CalledProcessError:
+        raise UnknownError("UNKNOWN: Unable to read bridge information from ovs-vsctl")
+
+
+def check_bridge_mtus(iface_br, iface_remap):
+    """Check that the tap devices associated with bridge have smaller MTUs."""
+    bridge_iface = iface_remap if iface_remap is not False else iface_br
+    ifaces = get_taps(bridge_iface)
+
+    bridge_mtu = get_mtu(iface_br)
+    if bridge_mtu <= 0:
+        raise UnknownError(
+            "UNKNOWN: Unable to read bridge information from /sys/class/net"
+        )
+    bad_mtus = {}
+
+    for iface in ifaces:
+        mtu = get_mtu(iface)
+        if iface_remap is not False:
+            if mtu + PADDING > bridge_mtu:
+                bad_mtus[iface] = mtu
+        else:
+            if mtu > bridge_mtu:
+                bad_mtus[iface] = mtu
+
+    return bad_mtus
+
+
+def iterate_bridges(bridges):
+    """Iterates the bridges."""
+    results = {}
+    to_check = {"br-tun": "br-int"}
+
+    for brbinding in bridges:
+        to_check[brbinding] = False
+
+    for key, value in to_check.items():
+        # format is br-ex:eno2
+        iface_br = key if ":" not in key else key.split(":")[0]
+        results[iface_br] = check_bridge_mtus(iface_br, value)
+
+    nagios_ok = True
+    msg_detail = []
+    for bridge in results.keys():
+        if len(results[bridge].keys()) > 0:
+            msg_detail.append(bridge + ": " + str(results[bridge]))
+            nagios_ok = False
+
+    if nagios_ok:
+        print("OK: Tenant MTU configuration is acceptable")
+    else:
+        msg = "WARNING: found tenant MTUs greater than bridge MTU, {}".format(
+            "; ".join(msg_detail)
+        )
+        raise WarnError(msg)
+
+
+def parse_args():
+    parser = argparse.ArgumentParser(description="Check MTU on bridge devices")
+    parser.add_argument("br", nargs="*", help="space separated list of bridges")
+    args = parser.parse_args()
+    return args
+
+
+def main():
+    """Nagios plugin to check MTUs in OVS bridges."""
+    args = parse_args()
+
+    if len(args.br) < 1:
+        raise UnknownError("UNKNOWN: No bridges provided for MTU check?")
+
+    try_check(iterate_bridges, args.br)
+
+
+if __name__ == "__main__":
+    main()