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

Move iptables check out of runtime init() to separate function #10231

Merged
merged 1 commit into from
Jan 21, 2015

Conversation

estesp
Copy link
Contributor

@estesp estesp commented Jan 21, 2015

Due to the iptables package being init()ed at start of the docker
runtime, this means the iptables --wait ... command listing all rules
is run, no matter if the command is simply "docker -h". It makes
more sense to both locate the iptables command lookup and check for the
wait flag support at the time iptables is actually used, as it
may not be used at all if certain network support is off/configured
differently.

Additional details:
The reason this is of interest is that on a significantly large box (many hundreds of CPU threads) and thousands of rules (e.g. few thousand docker containers up), docker -h can take significantly longer than on a standard SMP x86 box with only a few docker containers:

*** POWER8 (640 threads) w/thousands of containers

# time docker -h

real 1.648s

*** x86 VM w/10-15 containers

# time docker -h

real 0m0.027s

While a kernel issue revealed by this problem related to per-CPU counters will be fixed upstream, that may take some time to exist in distros that end users run.

Docker-DCO-1.1-Signed-off-by: Phil Estes estesp@linux.vnet.ibm.com (github: estesp)

@@ -25,6 +25,7 @@ const (

var (
supportsXlock = false
iptablesPath = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not a big fan of this but I see what you are trying to do. It just might be confusing to someone just looking at the code with no background

Copy link
Contributor

Choose a reason for hiding this comment

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

also it can be iptablesPath string no = "" which is a little bit better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point.. I was thinking initialization to force type, but specifying type makes it cleaner, thx

@jessfraz
Copy link
Contributor

can you give the time, with the patch too :) would be interesting to see the difference

@estesp
Copy link
Contributor Author

estesp commented Jan 21, 2015

@jfrazelle I will definitely try and get that.. I'm not the guy with the huge POWER8 box, but I can get him a binary, I think :) I left out all his perf. data that showed where iptables --wait was hanging up on the per_cpu counter loop, so this really is the time-sink in this particular case; not to mention docker -h doesn't do much more than init a bunch of packages and then dump the usage string..

@jessfraz
Copy link
Contributor

oh ok no worries then :)

On Tue, Jan 20, 2015 at 4:18 PM, Phil Estes notifications@github.com
wrote:

@jfrazelle https://github.com/jfrazelle I will definitely try and get
that.. I'm not the guy with the huge POWER8 box, but I can get him a
binary, I think :) I left out all his perf. data that showed where iptables
--wait was hanging up on the per_cpu counter loop, so this really is the
time-sink in this particular case; not to mention docker -h doesn't do
much more than init a bunch of packages and then dump the usage string..


Reply to this email directly or view it on GitHub
#10231 (comment).

Due to the iptables package being `init`ed at start of the docker
runtime, this means the iptables --wait command listing all rules
is run, no matter if the command is simply "docker -h".  It makes
more sense to both locate the iptables command and check for the
wait flag support at the time iptables is actually used, as it
may not be used at all if certain network support is off/configured
differently.

Docker-DCO-1.1-Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com> (github: estesp)
@crosbymichael
Copy link
Contributor

LGTM

1 similar comment
@jessfraz
Copy link
Contributor

LGTM

jessfraz pushed a commit that referenced this pull request Jan 21, 2015
Move iptables check out of runtime init() to separate function
@jessfraz jessfraz merged commit f3aed2a into moby:master Jan 21, 2015
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

3 participants