Skip to content

Explicitly set MTU on bridge devices. #46849

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

Merged
thaJeztah merged 1 commit into from
Nov 30, 2023
Merged

Explicitly set MTU on bridge devices. #46849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion integration/network/network_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ func TestDefaultNetworkOpts(t *testing.T) {

// Create a new network
networkName := "testnet"
network.CreateNoError(ctx, t, c, networkName, func(create *types.NetworkCreate) {
networkId := network.CreateNoError(ctx, t, c, networkName, func(create *types.NetworkCreate) {
if tc.configFrom {
create.ConfigFrom = &ntypes.ConfigReference{
Network: "from-net",
Expand All @@ -249,6 +249,15 @@ func TestDefaultNetworkOpts(t *testing.T) {
})
defer c.NetworkRemove(ctx, networkName)

// Check the MTU of the bridge itself, before any devices are connected. (The
// bridge's MTU will be set to the minimum MTU of anything connected to it, but
// it's set explicitly on the bridge anyway - so it doesn't look like the option
// was ignored.)
cmd := exec.Command("ip", "link", "show", "br-"+networkId[:12])
output, err := cmd.CombinedOutput()
assert.NilError(t, err)
assert.Check(t, is.Contains(string(output), fmt.Sprintf(" mtu %d ", tc.mtu)), "Bridge MTU should have been set to %d", tc.mtu)

// Start a container to inspect the MTU of its network interface
id1 := container.Run(ctx, t, c, container.WithNetworkMode(networkName))
defer c.ContainerRemove(ctx, id1, containertypes.RemoveOptions{Force: true})
Expand Down
8 changes: 8 additions & 0 deletions libnetwork/drivers/bridge/bridge_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -735,6 +735,14 @@ func (d *driver) createNetwork(config *networkConfiguration) (err error) {
bridgeSetup.queueStep(setupDefaultSysctl)
}

// 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.
Comment on lines +738 to +741
Copy link
Copy Markdown
Contributor

@corhere corhere Feb 2, 2024

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
Copy Markdown
Contributor Author

@robmry robmry Feb 2, 2024

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
Copy Markdown
Contributor

@corhere corhere Feb 2, 2024

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.

Copy link
Copy Markdown

@ivasilyev-mxr ivasilyev-mxr Feb 2, 2024

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
Copy Markdown
Contributor

@corhere corhere Feb 2, 2024

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

if config.Mtu > 0 {
bridgeSetup.queueStep(setupMTU)
}

// Even if a bridge exists try to setup IPv4.
bridgeSetup.queueStep(setupBridgeIPv4)

Expand Down
8 changes: 8 additions & 0 deletions libnetwork/drivers/bridge/setup_device_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@ func setupDevice(config *networkConfiguration, i *bridgeInterface) error {
return nil
}

func setupMTU(config *networkConfiguration, i *bridgeInterface) error {
if err := i.nlh.LinkSetMTU(i.Link, config.Mtu); err != nil {
log.G(context.TODO()).WithError(err).Errorf("Failed to set bridge MTU %s via netlink", config.BridgeName)
return err
}
return nil
}

func setupDefaultSysctl(config *networkConfiguration, i *bridgeInterface) error {
// Disable IPv6 router advertisements originating on the bridge
sysPath := filepath.Join("/proc/sys/net/ipv6/conf/", config.BridgeName, "accept_ra")
Expand Down