Netdb debug tool #35677
Merged
Netdb debug tool #35677
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fcrisciani
force-pushed
the
netdb-debug-tool
branch
from
53fce7c
to
cf82ff9
Compare
fcrisciani
commented
Dec 1, 2017
vendor.conf
Outdated
| @@ -30,7 +30,7 @@ github.com/moby/buildkit aaff9d591ef128560018433fe61beb802e149de8 | |||
| github.com/tonistiigi/fsutil dea3a0da73aee887fc02142d995be764106ac5e2 | |||
|
|
|||
| #get libnetwork packages | |||
| github.com/docker/libnetwork 72fd7e5495eba86e28012e39b5ed63ef9ca9a97b | |||
| github.com/docker/libnetwork fd95ac779f2b9fc3cbd2c256a22887e26f479653 https://github.com/fcrisciani/libnetwork | |||
There was a problem hiding this comment.
waiting for libnetwork side to be in
fcrisciani
force-pushed
the
netdb-debug-tool
branch
2 times, most recently
from
e1fa350
to
677789d
Compare
|
libnetwork side; moby/libnetwork#2027 |
fcrisciani
force-pushed
the
netdb-debug-tool
branch
7 times, most recently
from
d3feb7e
to
6a64fef
Compare
|
Windows CI failed: |
fcrisciani
force-pushed
the
netdb-debug-tool
branch
2 times, most recently
from
afda59f
to
d5c084a
Compare
thaJeztah
requested changes
Dec 6, 2017
daemon/reload.go
Outdated
| } | ||
| // Enable the network diagnose if the flag is set with a valid port withing the range | ||
| if conf.IsValueSet("network-diagnose") && conf.NetworkDiagnose > 0 && conf.NetworkDiagnose < 65536 { | ||
| logrus.Infof("Calling the diagnose start with %d", conf.NetworkDiagnose) |
There was a problem hiding this comment.
Wondering if we should make this a warning; having this option enabled without knowing it is enabled may be dangerous, correct?
cmd/dockerd/config.go
Outdated
| @@ -59,6 +59,7 @@ func installCommonConfigFlags(conf *config.Config, flags *pflag.FlagSet) { | |||
| flags.IntVar(&maxConcurrentDownloads, "max-concurrent-downloads", config.DefaultMaxConcurrentDownloads, "Set the max concurrent downloads for each pull") | |||
| flags.IntVar(&maxConcurrentUploads, "max-concurrent-uploads", config.DefaultMaxConcurrentUploads, "Set the max concurrent uploads for each push") | |||
| flags.IntVar(&conf.ShutdownTimeout, "shutdown-timeout", defaultShutdownTimeout, "Set the default shutdown timeout") | |||
| flags.IntVar(&conf.NetworkDiagnose, "network-diagnose", -1, "TCP port number of the network diagnose server") | |||
There was a problem hiding this comment.
Can we make the default 0 or is that a valid port?
Also, per our discussion; I think we wanted to make this a config-file only feature (i.e., only support setting this on daemon.json, but not through a flag. Not sure if a flag is required to make the config work (I think we can do without), but if it's required, we can hide the flag perhaps?
There was a problem hiding this comment.
Also s/diagnose server/diagnostics server/ ?
There was a problem hiding this comment.
@thaJeztah 0 means pick an available port automatically for me
let me add some bikeshed too :D
could we maybe use port in the flag name ? such as "metrics-addr" instead of just "metrics" ? When I look at the current flag it looks like a bool to me.
There was a problem hiding this comment.
like network-diagnostic-port ?
There was a problem hiding this comment.
My reason for 0 was that it's the default value for an integer; while we do -1 for other options, most we use it if 0 itself is a valid value.
Agree on adding -port (was thinking about that, but thought it may be too verbose)
fcrisciani
force-pushed
the
netdb-debug-tool
branch
2 times, most recently
from
911c302
to
5095932
Compare
added 3 commits
December 6, 2017 13:19
Add a new configuration option to allow the enabling of the networkDB debug. The option is only parsed using the reload event. This will protect the daemon on start or restart if the option is left behind in the config file Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>
Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>
Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>
fcrisciani
force-pushed
the
netdb-debug-tool
branch
from
5095932
to
2e75d32
Compare
thaJeztah
added
impact/documentation
status/4-merge
and removed
status/1-design-review
labels
|
Experimental failure is not related; |
|
Merging, after discussion with @vieux since the failures are not related |
This was referenced Dec 18, 2017
Closed
|
This also includes moby/libnetwork#1976, which fixes #35261 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
- What I did
New daemon config for networking diagnose
-How I tested it
Added a unit test in the reload_test section
- Description for the changelog
Add a new daemon config to enable the diagnose server for the networking layer
The libnetwork vendoring also bring:
Fix for #34515
Fix for watchMiss leak and ipvs deadlock
Netlink revendoring for netlink timeout: vishvananda/netlink@bd6d5de...b2de5d1
Libnetwork revendoring: moby/libnetwork@64ae588...9bca9a4
- A picture of a cute animal (not mandatory but encouraged)
