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

cli: perform feature detection lazily #2424

Merged
merged 2 commits into from
Apr 15, 2020

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Apr 7, 2020

alternative to / closes #1747
fixes #1739
fixes #2420

cli: perform feature detection lazily

  • Docker build: check experimental --platform on pre-run instead of during intialization
  • Perform feature detection when actually needed, instead of during initializing
  • Version negotiation is performed either when making an API request, or when (e.g.) running docker help (to hide unsupported features)
  • Use a 2 second timeout when 'pinging' the daemon; this should be sufficient for most cases, and when feature detection failed, the daemon will still perform validation (and produce an error if needed)
    • context.WithTimeout() doesn't currently work with ssh connections (connhelper), so we're only applying this timeout for tcp:// connections, otherwise keep the old behavior.

Before this change:

time sh -c 'DOCKER_HOST=tcp://42.42.42.41:4242 docker help &> /dev/null'
real   0m32.919s
user   0m0.370s
sys    0m0.227s

time sh -c 'DOCKER_HOST=tcp://42.42.42.41:4242 docker context ls &> /dev/null'
real   0m32.072s
user   0m0.029s
sys    0m0.023s

After this change:

time sh -c 'DOCKER_HOST=tcp://42.42.42.41:4242 docker help &> /dev/null'
real   0m 2.28s
user   0m 0.03s
sys    0m 0.03s

time sh -c 'DOCKER_HOST=tcp://42.42.42.41:4242 docker context ls &> /dev/null'
real   0m 0.13s
user   0m 0.02s
sys    0m 0.02s

@thaJeztah
Copy link
Member Author

@gtardif @silvin-lubecki PTAL

@thaJeztah thaJeztah force-pushed the lazy_feature_detection branch 2 times, most recently from 8abec90 to d171265 Compare April 9, 2020 11:27
cmd/docker/docker.go Outdated Show resolved Hide resolved
@thaJeztah thaJeztah force-pushed the lazy_feature_detection branch 2 times, most recently from 7da559d to e4e3aba Compare April 9, 2020 19:25
@thaJeztah
Copy link
Member Author

@silvin-lubecki updated; PTAL

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah force-pushed the lazy_feature_detection branch 14 times, most recently from bbe201a to 988c619 Compare April 10, 2020 14:31
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- Perform feature detection when actually needed, instead of during
  initializing
- Version negotiation is performed either when making an API request,
  or when (e.g.) running `docker help` (to hide unsupported features)
- Use a 2 second timeout when 'pinging' the daemon; this should be
  sufficient for most cases, and when feature detection failed, the
  daemon will still perform validation (and produce an error if needed)
- context.WithTimeout doesn't currently work with ssh connections (connhelper),
  so we're only applying this timeout for tcp:// connections, otherwise
  keep the old behavior.

Before this change:

    time sh -c 'DOCKER_HOST=tcp://42.42.42.41:4242 docker help &> /dev/null'
    real   0m32.919s
    user   0m0.370s
    sys    0m0.227s

    time sh -c 'DOCKER_HOST=tcp://42.42.42.41:4242 docker context ls &> /dev/null'
    real   0m32.072s
    user   0m0.029s
    sys    0m0.023s

After this change:

    time sh -c 'DOCKER_HOST=tcp://42.42.42.41:4242 docker help &> /dev/null'
    real   0m 2.28s
    user   0m 0.03s
    sys    0m 0.03s

    time sh -c 'DOCKER_HOST=tcp://42.42.42.41:4242 docker context ls &> /dev/null'
    real   0m 0.13s
    user   0m 0.02s
    sys    0m 0.02s

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Copy link
Contributor

@gtardif gtardif left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants