Merge ~yoshikadokawa/charm-nrpe:master-check-lacpdu into ~nrpe-charmers/charm-nrpe:master

[Yoshi Kadokawa]

Add additional checks for LACP bond status

Add checks if the LACPDU port state matches for the underlaying interfaces.

LP: #1877255

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

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

[Alvaro Uria (aluria)]

* LACP Port States (aka. Actor states) shared in binary format can be found at [1].
* Bond netlink code on latest stable [2]
* Partner state - what is considered partner state? Is LACP considered to be configured with 2 slaves (max and min number)?

I would like to understand use cases for this change and why the aggregator ID mismatch is not enough. On the other hand, there could be more than 2 slaves, so I'm not clear on what the "partner" would be in such cases. Or, there are deployments that initially use a single interface (no real purpose for bonding except for a near future reconfig to add a second slave). Would that mean that no "partner" would exist and "WARNING" would be alerted?

Once we have use cases (what it could have been spotted with this fix), I'd also like to know if these checks could be made optional (ie. check_lacp_bond.py --skip-extra-port-state).

With regard to the code, there are a couple of for loops over the same values (list of slaves). Could a single for loop be used?

Thank you.

1. http://movingpackets.net/2017/10/17/decoding-lacp-port-state/
2. https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/net/bonding/bond_netlink.c?h=v5.6.12

review: Needs Information

[Yoshi Kadokawa (yoshikadokawa)]

> * Partner state - what is considered partner state? Is LACP considered to be
> configured with 2 slaves (max and min number)?
For each underlying interface of an LACP bond, it has an actor state and partner state.
Actor state is the status of the host side, and partner state is the status coming from the switch side. Basically these have to be identical.

> I would like to understand use cases for this change and why the aggregator ID
> mismatch is not enough. On the other hand, there could be more than 2 slaves,
> so I'm not clear on what the "partner" would be in such cases. Or, there are
> deployments that initially use a single interface (no real purpose for bonding
> except for a near future reconfig to add a second slave). Would that mean that
> no "partner" would exist and "WARNING" would be alerted?
In my experience, aggregator ID mismatch is a more critical situation, which is usually one interface is DOWN/Unstable or not configured as LACP, etc.
And of course, the actor port state and partner port state will be in a mismatch as well.
However, there are use cases when aggregator ID are identical for the underlying interfaces, but a mismatch between actor port state and partner port state.
This is often seen with different LACP port rate configuration between host conf and switch conf.
For example, slow(30s) is configured on the switch side and fast(1s) is configured on the host side.

> Once we have use cases (what it could have been spotted with this fix), I'd
> also like to know if these checks could be made optional (ie.
> check_lacp_bond.py --skip-extra-port-state).
Since we already have the aggregator ID mismatch check, I think this can be optional.
I'll update it to make this optional.

> With regard to the code, there are a couple of for loops over the same values
> (list of slaves). Could a single for loop be used?
I did this to separate the output for aggregator ID mismatch and port state mismatch. If aggregator ID mismatch happens, there will be port state mismatch as well, and since the priority for aggregator ID mismatch is higher, I wanted to make sure that the warning message for aggregator ID comes up in Nagios.
However, if I make this port state mismatch check optional, I can put this check into the same for loop, and show the message only if the option is true.

Thank you

Unmerged commits

6a41da3... by Yoshi Kadokawa on 2020-05-07

add details of lacpdu port mismatch

LP: #1877255

f79a231... by Yoshi Kadokawa on 2020-04-10

add LACPDU port state check

diff --git a/files/plugins/check_lacp_bond.py b/files/plugins/check_lacp_bond.py
index 3e39986..b6e27d7 100755
--- a/files/plugins/check_lacp_bond.py
+++ b/files/plugins/check_lacp_bond.py
@@ -17,6 +17,30 @@ from nagios_plugin3 import (
     try_check,
 )
 
+# LACPDU port states in binary
+LACPDU_ACTIVE = 0b1
+LACPDU_RATE = 0b10
+LACPDU_AGGREGATED = 0b100
+LACPDU_SYNC = 0b1000
+LACPDU_COLLECT = 0b10000
+LACPDU_DIST = 0b100000
+LACPDU_DEFAULT = 0b1000000
+LACPDU_EXPIRED = 0b10000000
+
+
+def check_lacpdu_port(actor_port, partner_port):
+    diff = int(actor_port) ^ int(partner_port)
+    msg = ''
+    if diff & LACPDU_RATE:
+        msg += 'lacp rate mismatch,'
+    if diff & LACPDU_AGGREGATED:
+        msg += 'not aggregated,'
+    if diff & LACPDU_SYNC:
+        msg += 'not in sync,'
+    if diff & LACPDU_COLLECT:
+        msg += 'not collecting,'
+    return msg
+
 
 def check_lacp_bond(iface):
     """Checks LACP bonds are correctly configured (AD Aggregator IDs match)
@@ -25,6 +49,12 @@ def check_lacp_bond(iface):
     BOND_SLAVES_TEMPLATE = '/sys/class/net/{0}/bonding/slaves'
     BOND_MODE_TEMPLATE = '/sys/class/net/{0}/bonding/mode'
     SLAVE_TEMPLATE = '/sys/class/net/{0}/bonding_slave/ad_aggregator_id'
+    ACTOR_PORT_STATE = (
+        '/sys/class/net/{0}/bonding_slave/ad_actor_oper_port_state'
+    )
+    PARTNET_PORT_STATE = (
+        '/sys/class/net/{0}/bonding_slave/ad_partner_oper_port_state'
+    )
 
     bond_aggr = BOND_AGGR_TEMPLATE.format(iface)
     bond_slaves = BOND_SLAVES_TEMPLATE.format(iface)
@@ -44,7 +74,7 @@ def check_lacp_bond(iface):
 
         with open(bond_slaves) as fd:
             slaves = fd.readline().strip().split(' ')
-
+        # Check aggregator ID
         for slave in slaves:
             with open(SLAVE_TEMPLATE.format(slave)) as fd:
                 slave_aggr_value = fd.readline().strip()
@@ -58,6 +88,20 @@ def check_lacp_bond(iface):
                 msg = msg.format(iface, bond_aggr_value,
                                  slave, slave_aggr_value)
                 raise WarnError(msg)
+        # Check LACPDU port state
+        for slave in slaves:
+            with open(ACTOR_PORT_STATE.format(slave)) as fd:
+                actor_port_value = fd.readline().strip()
+            with open(PARTNET_PORT_STATE.format(slave)) as fd:
+                partner_port_value = fd.readline().strip()
+            if actor_port_value != partner_port_value:
+                res = check_lacpdu_port(actor_port_value, partner_port_value)
+                msg = 'WARNING: LACPDU port state mismatch '
+                msg += '({0}: {1} - actor_port_state={2}, '
+                msg += 'partner_port_state={3})'
+                msg = msg.format(res, slave, actor_port_value,
+                                 partner_port_value)
+                raise WarnError(msg)
 
     else:
         msg = 'CRITICAL: {} is not a bonding interface'.format(iface)
@@ -90,5 +134,6 @@ def main():
     try_check(check_lacp_bond,
               args.iface)
 
+
 if __name__ == '__main__':
     main()