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

NetworkDB to honor the Network Control Plane MTU #1839

Merged
merged 1 commit into from
Jul 27, 2017

Conversation

fcrisciani
Copy link

Introduce the possibility to specify the max buffer length
in network DB. This will allow to pass the proper MTU to
networkDB
Signed-off-by: Flavio Crisciani flavio.crisciani@docker.com

@fcrisciani
Copy link
Author

This is the preliminary step, still needs the value to be set, so far will keep using the default one of 1400

@fcrisciani fcrisciani force-pushed the datapath-mtu branch 8 times, most recently from ae47a6c to d61fe86 Compare July 17, 2017 15:41
@fcrisciani fcrisciani changed the title NetworkDB to honor the DataPath MTU NetworkDB to honor the Network Control Plane MTU Jul 18, 2017
@fcrisciani fcrisciani force-pushed the datapath-mtu branch 4 times, most recently from d4877b3 to 9bf8207 Compare July 25, 2017 16:06
config/config.go Outdated
if exp < 1500 {
// if exp == 0 the value won't be used
logrus.Warnf("Received a Network Control Plane MTU of %d, this value is very LOW,",
"the database can misbehave, 0 value will be ignored", exp)
Copy link
Contributor

Choose a reason for hiding this comment

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

LOW -> low pls :)
database -> network control-plane
pls remove 0 value will be ignored ... I don't think it is required.

Copy link
Author

Choose a reason for hiding this comment

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

ok for low also if I wanted to convey fear :D
ok for network control-plane
I put the 0 value because here https://github.com/docker/libnetwork/pull/1839/files#diff-2fe34e1b6f7f9aece0af74b1635502eeR217 it is possible to see 0 if the user set it so.

if printStats {
healthScore := nDB.memberlist.GetHealthScore()
if healthScore == 0 {
logrus.Infof("NetworkDB healthscore:%d Node totally healthy", healthScore)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove totally

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this going to be printed every 5 seconds ? I think that is very aggressive.

Copy link
Author

Choose a reason for hiding this comment

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

The definition of totally healthy comes directly from memberlist:

GetHealthScore gives this instance's idea of how well it is meeting the soft real-time requirements of the protocol. Lower numbers are better, and zero means "totally healthy".

Will change

@mavenugo
Copy link
Contributor

@fcrisciani minor comments. Otherwise LGTM

@fcrisciani fcrisciani force-pushed the datapath-mtu branch 3 times, most recently from 1f554b8 to 7f3a0e6 Compare July 26, 2017 16:19
- Introduce the possibility to specify the max buffer length
  in network DB. This will allow to use the whole MTU limit of
  the interface

- Add queue stats per network, it can be handy to identify the
  node's throughput per network and identify unbalance between
  nodes that can point to an MTU missconfiguration

Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>
Copy link
Contributor

@mavenugo mavenugo left a comment

Choose a reason for hiding this comment

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

LGTM

@mavenugo mavenugo merged commit cbf3165 into moby:master Jul 27, 2017
@fcrisciani fcrisciani deleted the datapath-mtu branch July 31, 2017 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants