-
Notifications
You must be signed in to change notification settings - Fork 53
feat: add support for MTU in Netdog #686
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.
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
base: develop
Are you sure you want to change the base?
Conversation
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.
It's been a while since I've reviewed this code, and the original maintainer has moved on, but my sense is we're meant to add new versions of every struct that gets updated to include an MTU, and tie that all together in a NetConfigV4 struct.
pub(crate) struct NetConfigV4 {
pub(crate) net_devices: IndexMap<InterfaceId, NetworkDeviceV2>,
}
pub(crate) enum NetworkDeviceV2 {
Interface(NetInterfaceV3),
BondDevice(NetBondV2),
VlanDevice(NetVlanV2),
}
Otherwise the versioning scheme in net.toml will break down; an older netdog that supports version = 3 won't be able to parse the new MTU fields if present.
Ideally also common shapes and implementations would be shifted into shared modules rather than copy-pasted.
It's rather painful when the goal is just to add one new field, which is one reason why net.toml has been so slow to evolve. The pain involved also creates an incentive to wait and group a lot of new features together as part of each version increment.
| if let Some(m) = mtu { | ||
| network.with_mtu(*m); | ||
| } |
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.
Same suggestion for the other occurrences of this pattern:
| if let Some(m) = mtu { | |
| network.with_mtu(*m); | |
| } | |
| maybe_add_some!(network, with_mut, mtu); |
| static_address_tests!(3); | ||
| vlan_tests!(3); | ||
| bonding_tests!(3); | ||
| mtu_tests!(3); |
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.
We'll need to increment the schema version for the MTU-related changes:
| mtu_tests!(3); | |
| mtu_tests!(4); |
It was something I was hoping wasn't true as I was implementing this 🫠 But yes I'll sort it out. |
Issue number: bottlerocket-os/bottlerocket#3341
Closes bottlerocket-os/bottlerocket#3341
Description of changes: This introduces the ability to set MTU as part of netdog configuration in net.toml.
Testing done: Only the testing contained in the PR so far.
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.