Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

networking.bonds: add support for arbitrary driverOptions #22388

Merged
merged 2 commits into from Feb 16, 2017
Merged

networking.bonds: add support for arbitrary driverOptions #22388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2130c5
networking.bonds: add support for arbitrary driverOptions
Profpatsch Feb 2, 2017
3c078f4
networking.networkd: adjust autmatic mapping of bonds
Profpatsch Feb 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion nixos/modules/system/boot/networkd.nix
View file
Open in desktop
Expand Up @@ -79,7 +79,7 @@ let
checkBond = checkUnitConfig "Bond" [
(assertOnlyFields [
"Mode" "TransmitHashPolicy" "LACPTransmitRate" "MIIMonitorSec"
"UpDelaySec" "DownDelaySec"
"UpDelaySec" "DownDelaySec" "GratuitousARP"
])
(assertValueOneOf "Mode" [
"balance-rr" "active-backup" "balance-xor"
Expand Down
36 changes: 29 additions & 7 deletions nixos/modules/tasks/network-interfaces-scripted.nix
View file
Open in desktop
Expand Up @@ -37,11 +37,24 @@ let
ip link del "${i}" 2>/dev/null || true
'';

in
# warn that these attributes are deprecated (2017-2-2)
# Should be removed in the release after next
bondDeprecation = rec {
deprecated = [ "lacp_rate" "miimon" "mode" "xmit_hash_policy" ];
filterDeprecated = bond: (filterAttrs (attrName: attr:
elem attrName deprecated && attr != null) bond);
};

{
bondWarnings =
let oneBondWarnings = bondName: bond:
mapAttrsToList (bondText bondName) (bondDeprecation.filterDeprecated bond);
bondText = bondName: optName: _:
"${bondName}.${optName} is deprecated, use ${bondName}.driverOptions";
in {
warnings = flatten (mapAttrsToList oneBondWarnings cfg.bonds);
};

config = mkIf (!cfg.useNetworkd) {
normalConfig = {

systemd.services =
let
Expand Down Expand Up @@ -288,10 +301,11 @@ in

echo "Creating new bond ${n}..."
ip link add name "${n}" type bond \
${optionalString (v.mode != null) "mode ${toString v.mode}"} \
${optionalString (v.miimon != null) "miimon ${toString v.miimon}"} \
${optionalString (v.xmit_hash_policy != null) "xmit_hash_policy ${toString v.xmit_hash_policy}"} \
${optionalString (v.lacp_rate != null) "lacp_rate ${toString v.lacp_rate}"}
${let opts = (mapAttrs (const toString)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird indentation... I'd join this into something like: ip link add name "${n}" type bond ${let ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation is done by the function that creates the lines.

ip link add … \
  mode foo \
  other_option bar

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant the indentation on the Nix side.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it’s like that because otherwise the first line will be indented by two spaces more (since it starts with two spaces already).

(bondDeprecation.filterDeprecated v))
// v.driverOptions;
in concatStringsSep "\n"
(mapAttrsToList (set: val: " ${set} ${val} \\") opts)}

# !!! There must be a better way to wait for the interface
while [ ! -d "/sys/class/net/${n}" ]; do sleep 0.1; done;
Expand Down Expand Up @@ -402,6 +416,14 @@ in
KERNEL=="tun", TAG+="systemd"
'';


};

in

{
config = mkMerge [
bondWarnings
(mkIf (!cfg.useNetworkd) normalConfig)
];
}
68 changes: 58 additions & 10 deletions nixos/modules/tasks/network-interfaces-systemd.nix
View file
Open in desktop
Expand Up @@ -109,17 +109,65 @@ in
Name = name;
Kind = "bond";
};
bondConfig =
(optionalAttrs (bond.lacp_rate != null) {
LACPTransmitRate = bond.lacp_rate;
}) // (optionalAttrs (bond.miimon != null) {
MIIMonitorSec = bond.miimon;
}) // (optionalAttrs (bond.mode != null) {
Mode = bond.mode;
}) // (optionalAttrs (bond.xmit_hash_policy != null) {
TransmitHashPolicy = bond.xmit_hash_policy;
});
bondConfig = let
# manual mapping as of 2017-02-03
# man 5 systemd.netdev [BOND]
# to https://www.kernel.org/doc/Documentation/networking/bonding.txt
# driver options.
driverOptionMapping = let
trans = f: optName: { valTransform = f; optNames = [optName]; };
simp = trans id;
ms = trans (v: v + "ms");
in {
Mode = simp "mode";
TransmitHashPolixy = simp "xmit_hash_policy";
LACPTransmitRate = simp "lacp_rate";
MIIMonitorSec = ms "miimon";
UpDelaySec = ms "updelay";
DownDelaySec = ms "downdelay";
LearnPacketIntervalSec = simp "lp_interval";
AdSelect = simp "ad_select";
FailOverMACPolicy = simp "fail_over_mac";
ARPValidate = simp "arp_validate";
# apparently in ms for this value?! Upstream bug?
ARPIntervalSec = simp "arp_interval";
ARPIPTargets = simp "arp_ip_target";
ARPAllTargets = simp "arp_all_targets";
PrimaryReselectPolicy = simp "primary_reselect";
ResendIGMP = simp "resend_igmp";
PacketsPerSlave = simp "packets_per_slave";
GratuitousARP = { valTransform = id;
optNames = [ "num_grat_arp" "num_unsol_na" ]; };
AllSlavesActive = simp "all_slaves_active";
MinLinks = simp "min_links";
};

do = bond.driverOptions;
assertNoUnknownOption = let
knownOptions = flatten (mapAttrsToList (_: kOpts: kOpts.optNames)
driverOptionMapping);
# options that apparently don’t exist in the networkd config
unknownOptions = [ "primary" ];
assertTrace = bool: msg: if bool then true else builtins.trace msg false;
in assert all (driverOpt: assertTrace
(elem driverOpt (knownOptions ++ unknownOptions))
"The bond.driverOption `${driverOpt}` cannot be mapped to the list of known networkd bond options. Please add it to the mapping above the assert or to `unknownOptions` should it not exist in networkd.")
(mapAttrsToList (k: _: k) do); "";
# get those driverOptions that have been set
filterSystemdOptions = filterAttrs (sysDOpt: kOpts:
any (kOpt: do ? "${kOpt}") kOpts.optNames);
# build final set of systemd options to bond values
buildOptionSet = mapAttrs (_: kOpts: with kOpts;
# we simply take the first set kernel bond option
# (one option has multiple names, which is silly)
head (map (optN: valTransform (do."${optN}"))
# only map those that exist
(filter (o: do ? "${o}") optNames)));
in seq assertNoUnknownOption
(buildOptionSet (filterSystemdOptions driverOptionMapping));

};

networks = listToAttrs (flip map bond.interfaces (bi:
nameValuePair "40-${bi}" (mkMerge [ (genericNetwork (mkOverride 999)) {
DHCP = mkOverride 0 (dhcpStr false);
Expand Down
20 changes: 20 additions & 0 deletions nixos/modules/tasks/network-interfaces.nix
View file
Open in desktop
Expand Up @@ -550,11 +550,28 @@ in
description = "The interfaces to bond together";
};

driverOptions = mkOption {
type = types.attrsOf types.str;
default = {};
example = literalExample {
interfaces = [ "eth0" "wlan0" ];
miimon = 100;
mode = "active-backup";
};
description = ''
Options for the bonding driver.
Documentation can be found in
<link xlink:href="https://www.kernel.org/doc/Documentation/networking/bonding.txt" />
'';

};

lacp_rate = mkOption {
default = null;
example = "fast";
type = types.nullOr types.str;
description = ''
DEPRECATED, use `driverOptions`.
Option specifying the rate in which we'll ask our link partner
to transmit LACPDU packets in 802.3ad mode.
'';
Expand All @@ -565,6 +582,7 @@ in
example = 100;
type = types.nullOr types.int;
description = ''
DEPRECATED, use `driverOptions`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of writing "DEPRECATED" in the description, I'd either pass down miimon/lacp_rate/mode/... to driverOptions in the definition or properly deprecate/remove it via rename.nix.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, it’s not really renamed, it’s been deprecated and is still being merged right now (with a warning), but will be removed in 17.09 or something.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not true, at least not using the scripted networking. This breaks bonding for me. Where do you merge the deprecated settings?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we have a LACP bond that is configured with the deprecated options but after this change came up with balance-rr instead. Which broke its networking. 😞

This should either trigger an error or merging should work. As this breaks peoples' setups I'm inclined to revert this for now.

👍 on the change, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, after trying to reproduce this: This problem only applies to networkd. Investigating.

Miimon is the number of millisecond in between each round of polling
by the device driver for failed links. By default polling is not
enabled and the driver is trusted to properly detect and handle
Expand All @@ -577,6 +595,7 @@ in
example = "active-backup";
type = types.nullOr types.str;
description = ''
DEPRECATED, use `driverOptions`.
The mode which the bond will be running. The default mode for
the bonding driver is balance-rr, optimizing for throughput.
More information about valid modes can be found at
Expand All @@ -589,6 +608,7 @@ in
example = "layer2+3";
type = types.nullOr types.str;
description = ''
DEPRECATED, use `driverOptions`.
Selects the transmit hash policy to use for slave selection in
balance-xor, 802.3ad, and tlb modes.
'';
Expand Down
2 changes: 1 addition & 1 deletion nixos/tests/networking.nix
View file
Open in desktop
Expand Up @@ -236,8 +236,8 @@ let
firewall.allowPing = true;
useDHCP = false;
bonds.bond = {
mode = "balance-rr";
interfaces = [ "eth1" "eth2" ];
driverOptions.mode = "balance-rr";
};
interfaces.eth1.ip4 = mkOverride 0 [ ];
interfaces.eth2.ip4 = mkOverride 0 [ ];
Expand Down