libnet: bridge: ignore EINVAL when configuring bridge MTU#47309
libnet: bridge: ignore EINVAL when configuring bridge MTU#47309thaJeztah merged 1 commit intomoby:masterfrom
Conversation
akerouanton
added
status/2-code-review
area/networking
akerouanton
force-pushed
the
libnet-bridge-mtu-ignore-einval
branch
from
11c707b to
0c1f5f5
Compare
| } | ||
| defer nh.Close() | ||
|
|
||
| config := &networkConfiguration{BridgeName: DefaultBridgeName, Mtu: 100000} |
There was a problem hiding this comment.
| config := &networkConfiguration{BridgeName: DefaultBridgeName, Mtu: 100000} | |
| config := &networkConfiguration{BridgeName: DefaultBridgeName, Mtu: 9000} |
There was a problem hiding this comment.
Just realized your previous suggestion meant we'd be unable to write a proper regression test for #47308. But I prefer to keep that more selective condition as my comment is already explicit enough to hardly break this fix unintentionally.
There was a problem hiding this comment.
I was thinking more about failing fast if a user sets an absurd MTU, and future-proofing against a custom kernel which accepts MTU > 65535 for some reason. But we didn't have fail-fast behaviour before, so it's not a big deal if we don't do it (while EL7 is supported)
Since 964ab71, we explicitly set the bridge MTU if it was specified. Unfortunately, kernel <v4.17 have a check preventing us to manually set the MTU to anything greater than 1500 if no links is attached to the bridge, which is how we do things -- create the bridge, set its MTU and later on, attach veths to it. Relevant kernel commit: torvalds/linux@804b854 As we still have to support CentOS/RHEL 7 (and their old v3.10 kernels) for a few more months, we need to ignore EINVAL if the MTU is > 1500 (but <= 65535). Signed-off-by: Albin Kerouanton <[email protected]>
akerouanton
force-pushed
the
libnet-bridge-mtu-ignore-einval
branch
from
0c1f5f5 to
89470a7
Compare
|
Woops, changed the labels in the wrong tab 🤦 |
| } | ||
| } | ||
|
|
||
| func TestMTUBiggerThan1500(t *testing.T) { |
There was a problem hiding this comment.
For a follow-up we could add a GoDoc to these tests to mention where these specific ranges came from (what's "magical" about Bigger than 1500, or bigger than 64k?
And perhaps combining both to a test-table / sub-tests would allow adding a "docs" to the tests (and more easily allow more cases to be added)
thaJeztah
added
process/cherry-picked
and removed
process/cherry-pick
process/cherry-pick/25.0
labels
4 participants
- What I did
Since 964ab71, we explicitly set the bridge MTU if it was specified. Unfortunately, kernel <v4.17 have a check preventing us to manually set the MTU to anything greater than 1500 if no links is attached to the bridge, which is how we do things -- create the bridge, set its MTU and later on, attach veths to it.
Relevant kernel commit: torvalds/linux@804b854
As we still have to support CentOS/RHEL 7 (and their old v3.10 kernels) for a few more months, we need to ignore EINVAL. An error will still be logged nonetheless, as the MTU could very well be invalid.
- How to verify it
We have no CI runners with Linux v3.10 to test the exact issue reported in #47308. So the best proxy is the added test: make sure
setupMTUdoesn't return an error if it tries to set a too high MTU.- Description for the changelog