Add support for static & lacp link aggragetes #873
Conversation
troglobit
force-pushed
the
lag
branch
2 times, most recently
from
bbec2cb to
2a39a38
Compare
| #!/bin/sh | ||
| # Initialize speed/duplex of virtio interfaces | ||
| # For virtual test systems (lacp tests) | ||
|
|
||
| ifaces=$(ip -d -json link show | jq -r '.[] | select(.parentbus == "virtio") | .ifname') | ||
| for iface in $ifaces; do | ||
| ethtool -s "$iface" speed 1000 duplex full | ||
| done |
There was a problem hiding this comment.
Very interesting! 👀
Wasn't this the whole raison d'être for the ethernet/etherlike split?
Autoneg-support can be detected using ethtool, so that should not be a problem to avoid on these interfaces. Since we can set the speed, shouldn't we get rid of etherlike all together?
Then you could set the speed to whatever you want in the config instead.
There was a problem hiding this comment.
I honestly don't remember all the details for the split, put this thing in here honestly as green goblin for you 🤣
But you're right, and it brushed my mind when I did this. I can have a look at that later as a separate cleanup task. Adding an issue as reminder to self rn.
| err = lag_gen_ports(net, dif, cif, ip); | ||
| if (err) | ||
| goto err_close_ip; | ||
|
|
There was a problem hiding this comment.
Should we move this, along with bridge_port_gen(), to a netdag_gen_iface_lower() or something?
There was a problem hiding this comment.
Possibly. Need to zoom out on the whole refactor you've made and take it all in, for real. There's also a master detection we could improve on a bit at the same time.
| /* infix-if-lag.c */ | ||
| int lag_gen_ports(struct dagger *net, struct lyd_node *dif, struct lyd_node *cif, FILE *ip); | ||
| int netdag_gen_lag(sr_session_ctx_t *session, struct dagger *net, struct lyd_node *dif, | ||
| struct lyd_node *cif, FILE *ip, int add); | ||
|
|
There was a problem hiding this comment.
To be consistent with the bridge code (after the refactor), I suggest lag_gen() and lag_port_gen()
| fprintf(ip, "link set %s down master %s\n", ifname, lagname); | ||
|
|
||
| /* We depend on lag to exist before we can set master ... */ | ||
| err = dagger_add_dep(net, ifname, lagname); | ||
| if (err) | ||
| ERROR("%s: unable to add dep to %s", ifname, lagname); |
There was a problem hiding this comment.
Just like when detaching the port from the LAG, attaching needs to happen in the init action of the LAG, not in the LAG port. Have a look att gen_join_leave() in the new infix-if-bridge-port.c - I think you can almost copy-paste it to use for LAG ports as well.
Then: you can reverse the dependency. I.e., the LAG depends on all of its ports being setup before we can create the LAG itself.
There was a problem hiding this comment.
Ahaa, now I finally understand what you were talking about last week! Yeah, that makes much more sense when the lag does the attaching. 👍
| if (add) { | ||
| struct lyd_node *node, *cifs; | ||
|
|
||
| cifs = lydx_get_descendant(lyd_parent(cif), "interfaces", "interface", NULL); | ||
| LYX_LIST_FOR_EACH(cifs, node, "interface") | ||
| err += lag_gen_ports(net, NULL, node, ip); | ||
| } |
There was a problem hiding this comment.
Is this not taken care of by netdag_gen_iface()'s call to lag_gen_ports()?
There was a problem hiding this comment.
Not when netdag_must_del() is triggered, this is the passage when we re-attach unmodified ports when recreating a lag that has had its mode changed.
But you're half right, at initial creation when the lag members are modified as well, this will cause duplicate setups. That's why the code must be idempotent, including the dagger changes I made. I could not see any other way of fixing this chicken and egg problem.
I'll add a comment about this in the code here for future readers.
Signed-off-by: Joachim Wiberg <[email protected]>
Signed-off-by: Joachim Wiberg <[email protected]>
The speed/duplex of a virtio interface is not eenforced in any way. The driver initializes them to unknown/unkown: https://github.com/torvalds/linux/blob/dccbe2047a5b0859de24bf463dae9eeea8e01c1e/drivers/net/virtio_net.c#L5375-L5381 It is however possible to set them since there are kernel subsystems that rely on them, e.g., miimon in the bond driver, which in turn is required for use with the 802.3ad (lacp) mode. If we do not initialize speed/duplex the miimon will complain in the log on virtual test systems every second: Dec 13 22:34:08 laggy kernel: lag0: (slave e1): failed to get link speed/duplex Signed-off-by: Joachim Wiberg <[email protected]>
Signed-off-by: Joachim Wiberg <[email protected]>
Signed-off-by: Joachim Wiberg <[email protected]>
Signed-off-by: Joachim Wiberg <[email protected]>
Signed-off-by: Joachim Wiberg <[email protected]>
Signed-off-by: Joachim Wiberg <[email protected]>
Some callbacks may run twice, e.g., lag_gen_ports(). Check if the link already exists, and is the same, then we can exit silently. However, in case the link exists and does *not* point to the same target, we log an error with current target for the post mortem. Signed-off-by: Joachim Wiberg <[email protected]>
This commit adds two SVGs for previewing the virtual test topologies.
quad: fix a lot of copy-paste mistakes, rename nodes and ports to their
correct names. Replace shell comments with C++ style comments,
not all tools, e.g., dotty, support shell comments. The neato
absolute positioning has also been fixed to make it possible to
see all the edges, to facilitate this the used ports have been
swapped around a abit. Colors are used according to the schema
agreed on for tests. Finally, a second link for lag tests have
been added between dut2 and dut3.
dual: add coloring similar to quad.
Signed-off-by: Joachim Wiberg <[email protected]>
Verify connectivity from host to the second DUT via the first, over a link aggregate. The lag starts in static mode and then changes to an LACP aggregate. This verifies not just basic aggregate functionality, but also changing mode, which is quite tricky to get right. Signed-off-by: Joachim Wiberg <[email protected]>
Signed-off-by: Joachim Wiberg <[email protected]>
- YANG model, fold in groupings, add enums - Add LACP system priorty, actor/partner key, partner mac, etc. Signed-off-by: Joachim Wiberg <[email protected]>
When reading lag status, like LACP actor system prio, port key, etc., we need CAP_NET_ADMIN. See net/bonding/bond_netlink.c Signed-off-by: Joachim Wiberg <[email protected]>
Signed-off-by: Joachim Wiberg <[email protected]>
Description
WIP
Checklist
Tick relevant boxes, this PR is-a or has-a: