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.

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

Explicitly set MTU on bridge devices. #46849

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

robmry
Copy link
Contributor

@robmry robmry commented Nov 24, 2023

- What I did

This is purely cosmetic - if a non-default MTU is configured, the bridge will have the default MTU=1500 until a container's 'veth' is connected and an MTU is set on the veth.

That's a disconcerting, it looks like the config has been ignored - so, set the bridge's MTU explicitly.

Closes #37937

- How I did it

When a bridge is created or updated, set its MTU to the configured value.

- How to verify it

Updated integration tests.

- Description for the changelog

Explicitly set MTU on bridge devices.

@thaJeztah
Copy link
Member

Are there any possibly consequences of doing this? I saw your reply on the ticket, and was wondering if there's any reason we took this approach (e.g. "we don't own the bridge, so don't touch it directly"), but looking at existing code, IIUC, we're already modifying things, so guess that's not the case.

Also from your other comment;

... but a Linux bridge's MTU is the min MTU of any device connected to it. When a 'veth' device is added to the bridge to connect a container, the MTU is set on that 'veth'.

Does setting the value here limit what range can be set on the veth? Or no effect there either?

(sorry if these are silly questions, just trying to see the scope of this change)

@robmry
Copy link
Contributor Author

robmry commented Nov 24, 2023

Thank you for taking a look - fair questions! ...

Are there any possibly consequences of doing this? I saw your reply on the ticket, and was wondering if there's any reason we took this approach (e.g. "we don't own the bridge, so don't touch it directly"), but looking at existing code, IIUC, we're already modifying things, so guess that's not the case.

Explicitly setting the bridge's MTU stops it from taking the min value of anything that's hooked up to it - it'll just keep the value it was given.

The MTU config belongs to the bridge, not the container. So, any 'veth' that's set up for a new container will get the same MTU as bridge.

So, I think it's ok. Unless there's any daemon-restart case I'm not aware of?

(I tried changing the config and bouncing the daemon with live-restore set ... the new config had no effect on the bridge or veths for new containers until the deamon was non-live-restore restarted - presumably as-expected.)

I see I've broken a couple of Windows tests by using netlink in an integration test - I'll sort that out.

This is purely cosmetic - if a non-default MTU is configured, the bridge
will have the default MTU=1500 until a container's 'veth' is connected
and an MTU is set on the veth. That's a disconcerting, it looks like the
config has been ignored - so, set the bridge's MTU explicitly.

Fixes moby#37937

Signed-off-by: Rob Murray <rob.murray@docker.com>
@thaJeztah thaJeztah added status/2-code-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. area/networking labels Nov 27, 2023
@thaJeztah thaJeztah added this to the 25.0.0 milestone Nov 27, 2023
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

@akerouanton @corhere PTAL

@thaJeztah
Copy link
Member

Let me bring this one in

@thaJeztah thaJeztah merged commit d3533ee into moby:master Nov 30, 2023
105 checks passed
@robmry robmry deleted the 37937_explicit_bridge_mtu branch December 4, 2023 12:32
@ivasilyev-mxr
Copy link

@robmry Hello, this seems to cause issues setting the MTU above 1500 ?

Trying to restart the daemon setting a mtu: >1500 in the config hits this error https://github.com/moby/moby/pull/46849/files#diff-77245372071f1b23e0b41e6ac4bd8212a33512dce6159146fff873d3da252c75R50 with message Invalid Argument.

Tried deleting the docker0 device manually, deleting out <docker-root>/network/* to let docker re-create those fresh on start, and still same issue

@thaJeztah
Copy link
Member

@ivasilyev-mxr thanks for reporting; could you open a ticket for this with details (and steps to reproduce)? It's easy for comments on merged PRs to get lost 😅

@ivasilyev-mxr
Copy link

Hi @thaJeztah thanks, I have created an issue: #47308

@thaJeztah
Copy link
Member

Thank you!

Comment on lines +738 to +741
// Always set the bridge's MTU if specified. This is purely cosmetic; a bridge's
// MTU is the min MTU of device connected to it, and MTU will be set on each
// 'veth'. But, for a non-default MTU, the bridge's MTU will look wrong until a
// container is attached.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not purely cosmetic: on Linux 4.17 and newer, manually setting the MTU disables MTU auto tuning. torvalds/linux@804b854

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The MTU is defined for the network, and all the endpoints attached to the bridge get that MTU. So, auto tuning would select the network's MTU ... and, I thought, the only effect of setting it explicitly on the bridge would be to have it report that MTU before an endpoint was attached (so, cosmetic in that sense).

I don't have laptop access at the moment (won't be properly back online until Tues) - but if this change is shown to be causing problems, we should just revert it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I'm speaking in a more general sense that setting the MTU on a Linux Ethernet Bridge is sticky on newer kernels, whereas on older kernels it would get clobbered every time a link is attached to it. Since we control all the links and set the MTU on all of them to the bridge MTU, it's a distinction without a difference to the libnetwork bridge driver unless someone manually changes the MTU of host-side veths. I don't think we need to revert once we patch the bridge driver setupMTU() to ignore -EINVAL.

Choose a reason for hiding this comment

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

Forgive the potential dumb question as I am not well versed in linux networking internals.

What will be end result behavior on older version of linux after patching that function to ignore the EINVAL error response ? Will it be like it was before this MR and the docker0 interface will switch to the desired MTU > 1500 once a container is attached ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Will it be like it was before this MR and the docker0 interface will switch to the desired MTU > 1500 once a container is attached ?

Yes, exactly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/networking kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. status/2-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing MTU of default bridge network doesn´t work
4 participants