feat(node): setup-networking component #3135
Conversation
andrewbattat
changed the title
There was a problem hiding this comment.
Must merge setupos/hostos addition of Netplan packages before merging this PR: #3289
|
I wonder what is the state when the script runs? Do we have any systemd-networkd config active at that point? Assuming that Netplan (that uses systemd-networkd is installed there can be some interference that would eventually lead to install time issues. Maybe using netns for the tests may avoid that by just adding a few commands to the script. Another concern is the interface name - we use the interface name in bridge setup and then in ethernet we use it as an identifier but we do not rename based on MAC. This will work as long as kernel+ACPI+udev do not change the interface name (unlikely but possible - there is a BIOS setting on some SuperMicro servers that allows to enable/disable ACPI tree element that carry "predictable name" -> interface name may change based on BIOS settings). If there is a change it would bind the interface based on MAC but it would fail bridge setup. Consider adding set-name statement (see https://netplan.readthedocs.io/en/latest/netplan-yaml/). |
|
Btw. is the in |
|
|
||
| # Dynamically add ethernets for each interface in descending speed | ||
| for IFACE in $SORTED_INTERFACES; do | ||
| sed -i "/^ ethernets:/a \ \ \ \ $IFACE:\n mtu: 1500\n optional: true\n emit-lldp: true\n" "$NETPLAN_OUTPUT" |
There was a problem hiding this comment.
Spawned from: #3135 (comment)
In our old code, we were writing the following values to systemd-networkd directly (see Systemd.rs):
LLDP=true
EmitLLDP=true
Netplan does have an LLDP property, and only an emit-lldp property. Is this sufficient?
From my basic understanding, it seems the default of routers-only is sufficient
| SYS_NET_PATH="${SYS_NET_PATH:-/sys/class/net}" | ||
| INTERFACES=($(find "$SYS_NET_PATH" -type l -not -lname '*virtual*' -exec basename '{}' ';')) | ||
| declare -A SPEED_MAP | ||
|
|
||
| for IFACE in "${INTERFACES[@]}"; do | ||
| if [ -f "$SYS_NET_PATH/$IFACE/speed" ]; then | ||
| SPEED=$(cat "$SYS_NET_PATH/$IFACE/speed" 2>/dev/null || echo 0) |
There was a problem hiding this comment.
I want to draw attention to the way we're gathering interface speeds. I'm curious what PFOPS thinks of this. Does this seem proper? In the old code, we were parsing the output of Ethtool to gather speeds (see interfaces.rs)
There was a problem hiding this comment.
At a high level, do we like moving forward with this idea of putting all the interfaces in a single bond and prioritizing interface usage by speed?
The only significant concern I have is if there are two interfaces connected, but the one selected first is just an internal connection. If that’s the case, I’m concerned that the bond won’t fall through to the next interface, and therefore, networking will fail. I understand that the internal networks are usually set up on the 1G ports, so hopefully this won’t be an issue as the 10G interfaces will be prioritized by speed, but this is still something I want to test.
There was a problem hiding this comment.
Moving this discussion to a thread (@tmshlvck)
I wonder what is the state when the script runs? Do we have any systemd-networkd config active at that point? Assuming that Netplan (that uses systemd-networkd is installed there can be some interference that would eventually lead to install time issues. Maybe using netns for the tests may avoid that by just adding a few commands to the script.
Is having the setup-networking.service files running Before=network.target network-online.target not sufficient?
Or is the "script" you're talking about the test-setup-networking.sh script? Sorry, I don't entirely follow.
There was a problem hiding this comment.
Moving this discussion to a thread (@tmshlvck)
Another concern is the interface name - we use the interface name in bridge setup and then in ethernet we use it as an identifier but we do not rename based on MAC. This will work as long as kernel+ACPI+udev do not change the interface name (unlikely but possible - there is a BIOS setting on some SuperMicro servers that allows to enable/disable ACPI tree element that carry "predictable name" -> interface name may change based on BIOS settings). If there is a change it would bind the interface based on MAC but it would fail bridge setup. Consider adding set-name statement (see https://netplan.readthedocs.io/en/latest/netplan-yaml/).
I do want to keep the networking code as simple as possible. Is this change necessary? I'll follow you're judgement, I just want to be certain.
NODE-1542
The old networking crate would:
The new networking component is much simpler and less error prone: it creates a bond containing all the interfaces sorted by speed and assigns the bond to the bridge using netplan.
A test script has been created to accompany the script.