Skip to content

NetworkDB to honor the Network Control Plane MTU - #1839

Merged
mavenugo merged 1 commit into
moby:masterfrom
fcrisciani:datapath-mtu
Jul 27, 2017
Merged

mavenugo merged 1 commit into
moby:masterfrom
fcrisciani:datapath-mtu

Conversation

@fcrisciani

Copy link
Copy Markdown

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
Copy Markdown
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
Comment thread 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
Copy Markdown
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
Copy Markdown
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.

Comment thread networkdb/cluster.go Outdated
if printStats {
healthScore := nDB.memberlist.GetHealthScore()
if healthScore == 0 {
logrus.Infof("NetworkDB healthscore:%d Node totally healthy", healthScore)

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

@mavenugo mavenugo left a comment

Copy link
Copy Markdown
Contributor

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.

2 participants