Add LAG for RouterOS7 - #3851
Conversation
ipspace
left a comment
ipspace
left a comment
There was a problem hiding this comment.
You over-engineered the quirk. We don't update the interface MTU based on the VLAN MTU it carries (maybe we should, but for the moment, the user must handle that).
I would therefore only check if a LAG interface has any vlan attributes, and if so, increase the MTU of the LAG members to LAG MTU + 4.
On a totally unrelated note: just wondering what problem you were solving with the "dynamic=no" option in the VLAN find command?
|
Hello Ivan, I will make the quirk more basic. As for why dynamic=no, Below is output from the As we can see we have a |
|
I tried a much simpler version of the check. Have updated with slightly more concise check now, but still works for all lag tests |
ipspace
left a comment
ipspace
left a comment
There was a problem hiding this comment.
A few things I'd fix to make life easier for my future self ;)
| When creating a LAG that uses VLANs, we need to add 4 bytes to the parent interface | ||
| for the VLAN Header so the VLAN MTU is the expected size ie. 1500 vs 1496 | ||
| ''' | ||
| for lag in node.interfaces: |
There was a problem hiding this comment.
You've created an O(n^2) solution for an O(n) problem. What you could do instead is:
- Iterate over all LAG interfaces, collect their maximum MTU, store it into a dict based on ifindex
- Iterate over all L3 VLAN subinterfaces, adjust the maximum MTU of the LAG interface based on parent_ifindex (if the parent_ifindex is in the LAG_MTU dict)
- Iterate over all member interfaces, adjust their MTU from the LAG_MTU dict.
| for the VLAN Header so the VLAN MTU is the expected size ie. 1500 vs 1496 | ||
| ''' | ||
| for lag in node.interfaces: | ||
| if lag.get('type') != 'lag': |
There was a problem hiding this comment.
The code is hard to read as you use "lag" to mean "an interface that could be a LAG interface. How about "lag_intf" or some such, so it's clear you're dealing with an interface.
| required_mtu = lag.get('mtu', 1500) + 4 | ||
|
|
||
| # Routed VLAN subinterfaces stored separately from the LAG. | ||
| for vlan in node.interfaces: |
There was a problem hiding this comment.
This is even worse from the naming perspective, as it looks like you're iterating over VLANs
| continue | ||
|
|
||
| for member in node.interfaces: | ||
| if member.get('lag._parentindex') == lag.lag.ifindex: |
There was a problem hiding this comment.
Again, member of what? How about "lag_member".
| if not required_mtu: | ||
| continue | ||
|
|
||
| for member in node.interfaces: |
There was a problem hiding this comment.
Would the code be cleaner to read if you used list comprehension? For example:
for lag_member in [ intf for intf in node.interfaces if intf.get('lag._parentindex') ]:
I'm not leaning one way or the other, just wondering what would be easier to read.
| if member.get('lag._parentindex') == lag.lag.ifindex: | ||
| member.mtu = max(member.get('mtu', 1500),required_mtu,) | ||
|
|
||
| def adjust_lag_vlan_mtu2(node: Box) -> None: |
|
Sorry about the extra code in the upload, thought I had removed it. I will work on your other suggestions. |
2 participants
This enables LAGs for RouterOS7
Had to create a new quirk for this process.
With RouterOS7 bonds, apparently you must increase the MTU of the slave interfaces by 4 bytes for the VLAN header.
Yet putting a VLAN on a ethernet interface directly does not need this and keeps the default 1500 MTU.
Retested the VLAN and new LAG integration tests below.
LAG integration tests
VLAN integration tests