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

Control plane mtu #34103

Merged
merged 2 commits into from Jul 28, 2017

Conversation

Projects
None yet
7 participants
@fcrisciani
Contributor

fcrisciani commented Jul 14, 2017

- What I did
Add the control plane MTU option in the daemon config. This will be useful in clusters with
MTU higher than 1500. The first user of this option will be libnetwork that will use it to
seed the networkDB library.

- Description for the changelog

Add the control plane MTU option in the daemon config

- A picture of a cute animal (not mandatory but encouraged)

funny-cute-animal-pics-part119-17-w630

@fcrisciani

This comment has been minimized.

Show comment
Hide comment
@fcrisciani

fcrisciani Jul 14, 2017

Contributor

Waiting for the vendoring of the libnetwork side, but will take CR or comments in the mean time

Contributor

fcrisciani commented Jul 14, 2017

Waiting for the vendoring of the libnetwork side, but will take CR or comments in the mean time

@friism

This comment has been minimized.

Show comment
Hide comment
@friism

friism Jul 14, 2017

Contributor

@fcrisciani will this help with this problem? #33596

Contributor

friism commented Jul 14, 2017

@fcrisciani will this help with this problem? #33596

@fcrisciani

This comment has been minimized.

Show comment
Hide comment
@fcrisciani

fcrisciani Jul 14, 2017

Contributor

@friism unfortunately nope, this is for control plane traffic, that is instead data plane traffic issue. Good to know though because gossip cap the packet at 1400, so this feature with Microsoft node most likely won't work till they fix it.

Contributor

fcrisciani commented Jul 14, 2017

@friism unfortunately nope, this is for control plane traffic, that is instead data plane traffic issue. Good to know though because gossip cap the packet at 1400, so this feature with Microsoft node most likely won't work till they fix it.

@fcrisciani

This comment has been minimized.

Show comment
Hide comment
@fcrisciani

fcrisciani Jul 14, 2017

Contributor

@friism looks like we support MTU on the overlay: docker/libnetwork#1349, did they try this?

Contributor

fcrisciani commented Jul 14, 2017

@friism looks like we support MTU on the overlay: docker/libnetwork#1349, did they try this?

fcrisciani added some commits Jul 13, 2017

Libnetwork vendoring
Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>
Allow to set the control plane MTU
Add daemon config to allow the user to specify the MTU of the control plane network.
The first user of this new parameter is actually libnetwork that can seed the
gossip with the proper MTU value allowing to pack multiple messages per UDP packet sent.
If the value is not specified or is lower than 1500 the logic will set it to the default.

Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>
@fcrisciani

This comment has been minimized.

Show comment
Hide comment
@fcrisciani

fcrisciani Jul 28, 2017

Contributor

@cpuguy83 ready for a second pass

Contributor

fcrisciani commented Jul 28, 2017

@cpuguy83 ready for a second pass

@cpuguy83

Code and Design LGTM

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Jul 28, 2017

Collaborator

LGTM

Collaborator

vieux commented Jul 28, 2017

LGTM

@vieux vieux merged commit 115f578 into moby:master Jul 28, 2017

7 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 35916 has succeeded
Details
janky Jenkins build Docker-PRs 44530 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 4915 has succeeded
Details
vendor Jenkins build Docker-PRs-vendor 3621 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 15913 has succeeded
Details
z Jenkins build Docker-PRs-s390x 4611 has succeeded
Details

@fcrisciani fcrisciani deleted the fcrisciani:control-plane-mtu branch Aug 1, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Aug 5, 2017

Member

If the value is not specified or is lower than 1500 the logic will set it to the default.

@fcrisciani should values lower than 1500 produce an error instead of silently ignoring? I can see it being unexpected if someone sets a lower value and it's not being used (or; if lower values become valid in future, they become active after updating to a newer version)

Member

thaJeztah commented Aug 5, 2017

If the value is not specified or is lower than 1500 the logic will set it to the default.

@fcrisciani should values lower than 1500 produce an error instead of silently ignoring? I can see it being unexpected if someone sets a lower value and it's not being used (or; if lower values become valid in future, they become active after updating to a newer version)

@fcrisciani

This comment has been minimized.

Show comment
Hide comment
@fcrisciani

fcrisciani Aug 5, 2017

Contributor

@thaJeztah in reality that is a comment that did not get updated, on the libnetwork side we decided to allow any configuration but we will log a warning (https://github.com/docker/libnetwork/blob/bump_17.07/config/config.go#L231)

if exp < 1500 {
     logrus.Warnf("Received a MTU of %d, this value is very low, the network control plane can misbehave", exp)
}
Contributor

fcrisciani commented Aug 5, 2017

@thaJeztah in reality that is a comment that did not get updated, on the libnetwork side we decided to allow any configuration but we will log a warning (https://github.com/docker/libnetwork/blob/bump_17.07/config/config.go#L231)

if exp < 1500 {
     logrus.Warnf("Received a MTU of %d, this value is very low, the network control plane can misbehave", exp)
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment