networking.bonds: add support for arbitrary driverOptions #22388
Conversation
|
@Profpatsch, thanks for your PR! By analyzing the history of the files in this pull request, we identified @wkennington, @edolstra and @aszlig to be potential reviewers. |
| description = '' | ||
| Options for the bonding driver. | ||
| Documentation can be found in | ||
| https://www.kernel.org/doc/Documentation/networking/bonding.txt |
There was a problem hiding this comment.
<link xlink:href="https://www.kernel.org/doc/Documentation/networking/bonding.txt" />
Until now the four attributes available very selectively provided a small subset, while copying upstream documentation. We make driver options an arbitrary key-value set and point to kernel documentation, which is always up-to-date. This way every option can be set. The four already existing options are deprecated with a warning.
|
I'm not sure why you deprecate the explicit options instead of the generic |
| @@ -565,6 +582,7 @@ in | |||
| example = 100; | |||
| type = types.nullOr types.int; | |||
| description = '' | |||
| DEPRECATED, use `driverOptions`. | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is not true, at least not using the scripted networking. This breaks bonding for me. Where do you merge the deprecated settings?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Okay, after trying to reproduce this: This problem only applies to networkd. Investigating.
Because they have to stay in sync with upstream options. And who watches that? |
| ${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) |
There was a problem hiding this comment.
Weird indentation... I'd join this into something like: ip link add name "${n}" type bond ${let ...
There was a problem hiding this comment.
The indentation is done by the function that creates the lines.
ip link add … \
mode foo \
other_option bar
There was a problem hiding this comment.
I meant the indentation on the Nix side.
There was a problem hiding this comment.
Yeah, it’s like that because otherwise the first line will be indented by two spaces more (since it starts with two spaces already).
Since the bonds interface changed to a lot more possible values we create a mapping of kernel bond attribute names and values to networkd attributes. Those match for the most part, but have to transformed slightly. There is also an assert that unknown options won’t slip through silently.
|
Please don't merge stuff like this yourself which can break (and broke) networking and can possibly lock people out of machines that are not on-site. This heavily frustrates people and shipping broken networking code even on unstable is highly unprofessional.. |
|
Point taken; the code itself is correct, the interaction of automatic translation to networkd options left out the deprecated options. I’m not sure how to pass the merged options through to the networkd implementation. |
|
@qknight noticed me that the descriptions are off. I’ll fix. |
|
Fixed in #23555 |
Until now the four attributes available very selectively provided a small
subset, while copying upstream documentation.
We make driver options an arbitrary key-value set and point to kernel
documentation, which is always up-to-date. This way every option can be set.
The four already existing options are deprecated with a warning.
Motivation for this change
Things done