Add support for link aggregation

[troglobit]

Description

Most network operating systems for switches and routers support link aggregation. In Linux we have both the bond and team drivers and since bond supports LACP it should be sufficient for Infix.

YANG Alternative 1

Using choice-case:

     +--rw infix-if:lag
     |  +--rw (infix-if:mode)?
     |  |  +--:(infix-if:lacp)
     |  |  |  +--rw infix-if:lacp
     |  |  |     +--rw infix-if:active?   boolean
     |  |  |     +--rw infix-if:rate?     enumeration
     |  |  +--:(infix-if:static)
     |  |     +--rw infix-if:static
     |  |        +--rw infix-if:arp-monitor
     |  |           +--rw infix-if:arp-interval?    uint32
     |  |           +--rw infix-if:arp-ip-target*   inet:ipv4-address
     |  +--rw infix-if:mii-monitor

YANG Alternative 2

     +--rw infix-if:lag
     |  +--rw infix-if:mode              lag-type
     |  +--rw infix-if:lacp
     |  |  +--rw infix-if:active?   boolean
     |  |  +--rw infix-if:rate?     enumeration
     |  +--rw (infix-if:link-monitor)?
     |     +--:(infix-if:arp-monitor)
     |     |  +--rw infix-if:arp-monitor
     |     |     +--rw infix-if:arp-interval?    uint32
     |     |     +--rw infix-if:arp-ip-target*   inet:ipv4-address
     |     +--:(infix-if:mii-monitor)
     |        +--rw infix-if:mii-monitor
     |           +--rw infix-if:interval?    uint32
     |           +--rw infix-if:carrier?     boolean
     |           +--rw infix-if:updelay?     uint32
     |           +--rw infix-if:downdelay?   uint32

Additional Information

No response

General Information

Anyone can help out by sponsoring development of new features or contributing pull requests.
Please use this issue for discussions related to the feature.

[wkz]

Alt. 1 vs. 2

I think if we go for alternative 1, with lacp and static both being presence containers, then I don't see any need for an explicit mode leaf. I.e.:

     +--rw infix-if:lag
     |  +--rw (infix-if:mode)?
     |  |  +--:(infix-if:lacp)
-    |  |  |  +--rw infix-if:lacp
+    |  |  |  +--rw infix-if:lacp!
     |  |  |     +--rw infix-if:active?   boolean
     |  |  |     +--rw infix-if:rate?     enumeration
     |  |  +--:(infix-if:static)
-    |  |     +--rw infix-if:static
+    |  |     +--rw infix-if:static!
     |  |        +--rw infix-if:arp-monitor
     |  |           +--rw infix-if:arp-interval?    uint32
     |  |           +--rw infix-if:arp-ip-target*   inet:ipv4-address
     |  +--rw infix-if:mii-monitor

MII Monitor

I'm not opposed to having this configurable. But I think we should think about just going with some sane deaults to begin with. I can see the use-case for {up,down}delay in active-backup configs, but when would you not want to react to the physical link going down?

If we do decide to support it being configurable, then I would like us to find a better name than mii-monitor. I get that it's what the kernel calls it, but it sounds very idiosyncratic to me at least.

ARP Monitor

Is this something you run per-link or on the LAG itself?

Is this something we can leave out and model via a generic arp/ping trigger that we could also associate with other objects (e.g. routes)?

[troglobit]

I'm thinking we go for balance-xor and 802.3ad (really 802.1AX, LACP, if I understood it correctly) in the first run. So static == balance-xor here, obviously.

For all the balance-* modes, the arp-monitor can be used, but it cannot be used at the same time as the mii-monitor. The mii-monitor otoh is required for the LACP mode. That's why I exposed the setting, and made it into a choice-case.

Here is more about the ARP Monitor https://www.kernel.org/doc/html/latest/networking/bonding.html#arp-monitor-operation

When changing the mode of an aggregate the aggregate needs to be down and not have any link members. So to make the code easier I added the mode setting, simply checking for changes to that YANG node means we have to delete the lag before recreating it. With choice-case or presence containers this is more cumbersome, in particular if we add all the different modes.

Also, changing mode the choice-case alternative, or with presence containers, require the user to no current-mode first, while in the first alternative we can detect changes to mode and infer an invalidation of the current mode's settings.

Finally, some of the balance-* modes share other settings which can be included with a conditionalized uses statement.

[wkz]

Thanks for the link! 👍

Re: mii-monitor

After reading that, I'm even more sure that carrier true is the only reasonable option 😄, so I don't think we need that to be configurable. IIUC, the other two are either "read the PHY registers", which sounds like an interop nightmare; or "ask ethtool", which should always be in sync with the carrier state.

Re: arp-monitor

I get why this could be useful, but I just think it being mutually exclusive with monitoring the carrier makes it much less useful. Why not just skip it for now? If we do it add a generic trigger facility later on, then we could also combine it with the carrier monitoring.

Re: Explicit mode

If it makes the code that much simpler, then fair enough.

(Just in case you want to get rid of it later:)

As for the inference part of the problem, the CLI does allow you to configure multiple choice branches:

root@infix-00-00-00:/> configure
root@infix-00-00-00:/config/> set interface br0
root@infix-00-00-00:/config/> set interface e1 bridge-port bridge br0
root@infix-00-00-00:/config/> set interface e1 container-network
root@infix-00-00-00:/config/> check
Error: Data for both cases "container-network" and "bridge-port" exist. (path "/ietf-interfaces:interfaces/interface/infix-interfaces:port")
Error: Invalid candidate configuration

So I think we could to the same clean-up of the old case when a new one is set - if we want to go that route.

[troglobit]

Sure we can skip both mii- and arp-monitor for now. I really liked the up/down delay thingy though, would be great protection against link flapping, unless we find other useful link qualifying mechanisms, of course. The arp-monitor is, I guess, only useful in cases where the link partner does not support LACP, so yeah let's skip for now.

Conclusion, not exposing mii- or arp-monitor now, going with the defaults in the bond driver means:

• miimonitor 100 msec
• use_carrier=1

This works with both lacp and static (balance-xor) modes.

Alright, going with the explicit mode setting. This really only affects the lacp specific options, which are in their own container.

Thanks for the feedback!! ❤️

[wkz]

I really liked the up/down delay thingy though, would be great protection against link flapping

💯 I also liked that part! It was just the carrier part that seemed like some ancient legacy stuff that we might want to avoid. I'm happy to have the delays be configurable. Sorry for the confusion 😅

[troglobit]

Alright, for future compatibility, how about this (choice-case based) simplified link-monitoring support?

     +--rw infix-if:lag
     |  +--rw infix-if:mode              lag-type
     |  +--rw infix-if:lacp
     |  |  +--rw infix-if:active?   boolean
     |  |  +--rw infix-if:rate?     enumeration
     |  +--rw (infix-if:link-monitor)?
     |     +--:(infix-if:mii-monitor)
     |        +--rw infix-if:mii-monitor
     |           +--rw infix-if:updelay?     uint32
     |           +--rw infix-if:downdelay?   uint32

• Hard-coded default interval 100 milliseconds.
• Hard-coded use_carrier=1
• No ARP Monitor for now

[wkz]

s/mii-monitor/link-monitor/g and we're golden 🌟 !

[troglobit]

s/mii-monitor/link-monitor/g and we're golden 🌟 !

Um, OK just to be perfectly clear: in a distant future we may want to add ARP Monitor support, in which case the model would look like this:

     +--rw infix-if:lag
     |  ...
     |  +--rw (infix-if:link-monitor)?
     |     +--:(infix-if:arp-monitor)
     |     |  +--rw infix-if:arp-monitor
     |     |     +--rw infix-if:arp-interval?    uint32
     |     |     +--rw infix-if:arp-ip-target*   inet:ipv4-address
     |     +--:(infix-if:see-below-for-more ...)

Admittedly I struggled with "mii" monitor, but what you want is this, right?

     +--rw infix-if:lag
     |  ...
     |  +--rw (infix-if:link-monitor)?
     |     +--:(infix-if:arp-monitor)
     |     |  +--rw infix-if:arp-monitor
     |     |     +--rw infix-if:arp-interval?    uint32
     |     |     +--rw infix-if:arp-ip-target*   inet:ipv4-address
     |     +--:(infix-if:link-monitor)
     |        +--rw infix-if:link-monitor
     |           +--rw infix-if:updelay?     uint32
     |           +--rw infix-if:downdelay?   uint32

I'm absolutely fine with s/mii-monitor/link-monitor/g, just want to make sure we 💯 agree on the same thing 🤓

[wkz]

ACK 😄

My guess would be that the original feaure was the "read the PHY registers" mode, which in the kernel has historically been called "MII registers" instead of "MDIO registers"; ergo: mii-monitor. But I think link-monitor makes much more sense, since we are going to use the carrier information to determine the link state.

[troglobit]

ACK 😄

My guess would be that the original feaure was the "read the PHY registers" mode, which in the kernel has historically been called "MII registers" instead of "MDIO registers"; ergo: mii-monitor. But I think link-monitor makes much more sense, since we are going to use the carrier information to determine the link state.

Makes sense, thanks! 🙏