Code review comment for ~yoshikadokawa/charm-nrpe:master-check-lacpdu

Revision history for this message
Alvaro Uria (aluria) wrote :

* 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

« Back to merge proposal