feat(node): setup-networking component #3135
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| network: | ||
| version: 2 | ||
| renderer: networkd | ||
| ethernets: | ||
| # Interfaces will be dynamically inserted here | ||
| bonds: | ||
| bond0: | ||
| interfaces: [{INTERFACES}] | ||
| parameters: | ||
| mode: active-backup | ||
| mii-monitor-interval: 5s | ||
| up-delay: 500ms | ||
| down-delay: 500ms | ||
| macaddress: {MAC_ADDRESS} | ||
| mtu: 1500 | ||
| optional: true | ||
| bridges: | ||
| br6: | ||
| interfaces: [bond0] | ||
| addresses: [{IPV6_ADDR}] | ||
| routes: | ||
| - to: ::/0 | ||
| via: {IPV6_GATEWAY} | ||
| nameservers: | ||
| addresses: [2606:4700:4700::1111, 2606:4700:4700::1001, 2001:4860:4860::8888, 2001:4860:4860::8844] | ||
| parameters: | ||
| forward-delay: 0 | ||
| stp: false | ||
| link-local: ["ipv6"] | ||
| accept-ra: no |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| [Unit] | ||
| Description=Setup networking configuration for HostOS | ||
| After=systemd-modules-load.service | ||
| After=systemd-udev-settle.service | ||
| Wants=systemd-udev-settle.service | ||
| Before=network.target network-online.target | ||
|
|
||
| [Service] | ||
| Type=oneshot | ||
| RemainAfterExit=yes | ||
| ExecStart=/opt/ic/bin/setup-networking.sh HostOS | ||
|
|
||
| [Install] | ||
| WantedBy=multi-user.target |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| [Unit] | ||
| Description=Setup networking configuration for SetupOS | ||
| Before=setupos.service | ||
| Before=network.target network-online.target | ||
|
|
||
| [Service] | ||
| Type=oneshot | ||
| RemainAfterExit=yes | ||
| ExecStart=/opt/ic/bin/setup-networking.sh SetupOS | ||
|
|
||
| [Install] | ||
| WantedBy=multi-user.target |
|
There was a problem hiding this comment. Moving this discussion to a thread (@tmshlvck)
Is having the setup-networking.service files running 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)
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. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,138 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| set -o nounset | ||
| set -o pipefail | ||
|
|
||
| SHELL="/bin/bash" | ||
| PATH="${PATH}:/sbin:/bin:/usr/sbin:/usr/bin" | ||
|
|
||
| function parse_args() { | ||
| if [ $# -ne 1 ]; then | ||
| echo "Usage: $0 <SetupOS|HostOS>" | ||
| exit 1 | ||
| fi | ||
|
|
||
| NODE_TYPE="$1" | ||
| if [ "$NODE_TYPE" != "SetupOS" ] && [ "$NODE_TYPE" != "HostOS" ]; then | ||
| echo "Invalid node type: $NODE_TYPE" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Allow overriding config paths via environment variables | ||
| if [ "$NODE_TYPE" = "SetupOS" ]; then | ||
| CONFIG_BASE_PATH="${CONFIG_BASE_PATH:-/var/ic/config}" | ||
| else | ||
| CONFIG_BASE_PATH="${CONFIG_BASE_PATH:-/boot/config}" | ||
| fi | ||
|
|
||
| CONFIG="${CONFIG_BASE_PATH}/config.ini" | ||
| } | ||
|
|
||
| function read_variables() { | ||
| # Read limited set of keys. Be extra-careful quoting values as it could | ||
| # otherwise lead to executing arbitrary shell code! | ||
| while IFS="=" read -r key value; do | ||
| case "$key" in | ||
| "ipv6_gateway") IPV6_GATEWAY="${value}" ;; | ||
| esac | ||
| done <"${CONFIG}" | ||
|
|
||
| if [ -z "$IPV6_GATEWAY" ]; then | ||
| echo "No IPv6 gateway found in $CONFIG." | ||
| exit 1 | ||
| fi | ||
| } | ||
|
|
||
| function generate_addresses() { | ||
| echo "Generating MAC and IPv6 addresses..." | ||
| IC_BIN_PATH="${IC_BIN_PATH:-/opt/ic/bin}" | ||
|
|
||
| if [ "$NODE_TYPE" = "SetupOS" ]; then | ||
| MAC_ADDR=$("${IC_BIN_PATH}/setupos_tool" generate-mac-address --node-type SetupOS) | ||
| IPV6_ADDR=$("${IC_BIN_PATH}/setupos_tool" generate-ipv6-address --node-type SetupOS) | ||
| else | ||
| MAC_ADDR=$("${IC_BIN_PATH}/hostos_tool" generate-mac-address --node-type HostOS) | ||
| IPV6_ADDR=$("${IC_BIN_PATH}/hostos_tool" generate-ipv6-address --node-type HostOS) | ||
| fi | ||
|
|
||
| if [ -z "$MAC_ADDR" ] || [ -z "$IPV6_ADDR" ]; then | ||
| echo "Failed to generate MAC or IPv6 address." | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "Generated MAC address: $MAC_ADDR" | ||
| echo "Generated IPv6 address: $IPV6_ADDR" | ||
| } | ||
|
|
||
| function gather_interfaces_by_speed() { | ||
| echo "Gathering and sorting interfaces by speed..." | ||
|
|
||
| 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) | ||
|
Comment on lines
+70
to
+76
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) |
||
| else | ||
| SPEED=0 | ||
| echo "Warning: Unable to determine speed for interface $IFACE, defaulting to 0." >&2 | ||
| fi | ||
| SPEED_MAP["$IFACE"]=$SPEED | ||
| done | ||
|
|
||
| # Sort interfaces by speed descending | ||
| SORTED_INTERFACES=$(for IFACE in "${!SPEED_MAP[@]}"; do | ||
| echo "${SPEED_MAP[$IFACE]} $IFACE" | ||
| done | sort -nrk1 | awk '{print $2}') | ||
|
|
||
| if [ -z "$SORTED_INTERFACES" ]; then | ||
| echo "No interfaces found." | ||
| exit 1 | ||
| fi | ||
|
|
||
| INTERFACE_LIST=$(echo "$SORTED_INTERFACES" | paste -sd, -) | ||
| echo "Interfaces sorted by speed: $INTERFACE_LIST" | ||
| } | ||
|
|
||
| function configure_netplan() { | ||
| echo "Configuring netplan..." | ||
| NETPLAN_TEMPLATE_PATH="${NETPLAN_TEMPLATE_PATH:-/opt/ic/share}" | ||
| NETPLAN_TEMPLATE="${NETPLAN_TEMPLATE_PATH}/99-setup-netplan.yaml.template" | ||
| NETPLAN_RUN_PATH="${NETPLAN_RUN_PATH:-/run/netplan}" | ||
| NETPLAN_OUTPUT="${NETPLAN_RUN_PATH}/99-setup-netplan.yaml" | ||
|
|
||
| mkdir -p "$NETPLAN_RUN_PATH" | ||
| cp "$NETPLAN_TEMPLATE" "$NETPLAN_OUTPUT" | ||
|
|
||
| sed -i "s|{INTERFACES}|${INTERFACE_LIST}|g" "$NETPLAN_OUTPUT" | ||
| sed -i "s|{MAC_ADDRESS}|${MAC_ADDR}|g" "$NETPLAN_OUTPUT" | ||
| sed -i "s|{IPV6_ADDR}|${IPV6_ADDR}|g" "$NETPLAN_OUTPUT" | ||
| sed -i "s|{IPV6_GATEWAY}|${IPV6_GATEWAY}|g" "$NETPLAN_OUTPUT" | ||
|
|
||
| # 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): 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 |
||
| done | ||
|
|
||
| # Fix netplan configuration file permissions to silence warnings | ||
| chmod 0600 "$NETPLAN_OUTPUT" | ||
|
|
||
| echo "Netplan configuration written to $NETPLAN_OUTPUT" | ||
| echo "Applying netplan..." | ||
| netplan generate | ||
| netplan apply | ||
| echo "Network configuration applied successfully for $NODE_TYPE." | ||
| } | ||
|
|
||
| function main() { | ||
| parse_args "$@" | ||
| read_variables | ||
| generate_addresses | ||
| gather_interfaces_by_speed | ||
| configure_netplan | ||
| } | ||
|
|
||
| if [ "$0" = "$BASH_SOURCE" ]; then | ||
| main "$@" | ||
| fi | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| set -euo pipefail | ||
|
|
||
| if [ $# -ne 2 ]; then | ||
| echo "Usage: $0 <path-to-setup-networking.sh> <path-to-99-setup-netplan.yaml.template>" | ||
| exit 1 | ||
| fi | ||
|
|
||
| SETUP_SCRIPT="$1" | ||
| TEMPLATE_FILE="$2" | ||
|
|
||
| TEST_DIR="$(mktemp -d)" | ||
| trap 'rm -rf "$TEST_DIR"' EXIT | ||
|
|
||
| SHELL=/bin/bash | ||
|
|
||
| function mock_tools() { | ||
| export SYS_NET_PATH="${TEST_DIR}/sys/class/net" | ||
| mkdir -p "${SYS_NET_PATH}" | ||
|
|
||
| mkdir -p "${TEST_DIR}/sys/devices/mock_devices_eth0" | ||
| mkdir -p "${TEST_DIR}/sys/devices/mock_devices_eth1" | ||
| mkdir -p "${TEST_DIR}/sys/devices/mock_devices_eth2" | ||
|
|
||
| ln -sf "${TEST_DIR}/sys/devices/mock_devices_eth0" "${SYS_NET_PATH}/eth0" | ||
| ln -sf "${TEST_DIR}/sys/devices/mock_devices_eth1" "${SYS_NET_PATH}/eth1" | ||
| ln -sf "${TEST_DIR}/sys/devices/mock_devices_eth2" "${SYS_NET_PATH}/eth2" | ||
|
|
||
| # Intentionally absent eth0/speed: | ||
| # echo "0" >"$SYS_NET_PATH/eth0/speed" | ||
|
|
||
| echo "1000" >"$SYS_NET_PATH/eth1/speed" | ||
| echo "2500" >"$SYS_NET_PATH/eth2/speed" | ||
|
|
||
| mkdir -p "${TEST_DIR}/opt/ic/bin" | ||
| cat >"${TEST_DIR}/opt/ic/bin/setupos_tool" <<'EOF' | ||
| #!/bin/bash | ||
| if [ "$1" = "generate-mac-address" ]; then | ||
| echo "02:00:00:aa:bb:cc" | ||
| elif [ "$1" = "generate-ipv6-address" ]; then | ||
| echo "2001:db8::1234" | ||
| fi | ||
| EOF | ||
| chmod +x "${TEST_DIR}/opt/ic/bin/setupos_tool" | ||
|
|
||
| cat >"${TEST_DIR}/netplan" <<'EOF' | ||
| #!/bin/bash | ||
| if [ "$1" = "generate" ] || [ "$1" = "apply" ]; then | ||
| exit 0 | ||
| fi | ||
| EOF | ||
| chmod +x "${TEST_DIR}/netplan" | ||
|
|
||
| mkdir -p "${TEST_DIR}/var/ic/config" | ||
| cat >"${TEST_DIR}/var/ic/config/config.ini" <<'EOF' | ||
| ipv6_gateway=fe80::2 | ||
| EOF | ||
|
|
||
| mkdir -p "${TEST_DIR}/run/netplan" | ||
| } | ||
|
|
||
| function test_gather_interfaces_by_speed() { | ||
| export PATH="${TEST_DIR}:${PATH}" | ||
| hash -r | ||
|
|
||
| source "$SETUP_SCRIPT" | ||
| gather_interfaces_by_speed | ||
| local EXPECTED_INTERFACES="eth2,eth1,eth0" | ||
| if [ "${INTERFACE_LIST}" = "${EXPECTED_INTERFACES}" ]; then | ||
| echo "TEST PASSED: gather_interfaces_by_speed" | ||
| else | ||
| echo "TEST FAILED: gather_interfaces_by_speed" | ||
| exit 1 | ||
| fi | ||
| } | ||
|
|
||
| function test_netplan_config() { | ||
| export PATH="${TEST_DIR}:${PATH}" | ||
| hash -r | ||
|
|
||
| CONFIG_BASE_PATH="${TEST_DIR}/var/ic/config" \ | ||
| NETPLAN_TEMPLATE_PATH="$(dirname "$TEMPLATE_FILE")" \ | ||
| NETPLAN_RUN_PATH="${TEST_DIR}/run/netplan" \ | ||
| IC_BIN_PATH="${TEST_DIR}/opt/ic/bin" \ | ||
| SHELL=/bin/bash \ | ||
| /bin/bash "$SETUP_SCRIPT" SetupOS | ||
|
|
||
| local OUTPUT_FILE="${TEST_DIR}/run/netplan/99-setup-netplan.yaml" | ||
|
|
||
| [ -f "$OUTPUT_FILE" ] || { | ||
| echo "Test failed: netplan output file not created" | ||
| exit 1 | ||
| } | ||
| grep -q "macaddress: 02:00:00:aa:bb:cc" "$OUTPUT_FILE" || { | ||
| echo "Test failed: MAC address substitution" | ||
| exit 1 | ||
| } | ||
| grep -q "2001:db8::1234" "$OUTPUT_FILE" || { | ||
| echo "Test failed: IPv6 address substitution" | ||
| exit 1 | ||
| } | ||
| grep -q "fe80::2" "$OUTPUT_FILE" || { | ||
| echo "Test failed: IPv6 gateway substitution" | ||
| exit 1 | ||
| } | ||
| grep -Eq "interfaces:\s*\[eth2,eth1,eth0\]" "$OUTPUT_FILE" || { | ||
| echo "Test failed: Interfaces insertion" | ||
| exit 1 | ||
| } | ||
|
|
||
| echo "TEST PASSED: netplan_config" | ||
| } | ||
|
|
||
| mock_tools | ||
| test_gather_interfaces_by_speed | ||
| test_netplan_config | ||
|
|
||
| echo "All tests passed." | ||
| exit 0 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| [Unit] | ||
| Description=SetupOS config process | ||
| Before=setup-networking.service | ||
| Before=setupos.service | ||
|
|
||
| [Service] | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.