Skip to content

[19.03 backport] cli: perform feature detection lazily - #2457

Merged
silvin-lubecki merged 3 commits into
docker:19.03from
thaJeztah:19.03_backport_lazy_feature_detection
Apr 30, 2020
Merged

[19.03 backport] cli: perform feature detection lazily#2457
silvin-lubecki merged 3 commits into
docker:19.03from
thaJeztah:19.03_backport_lazy_feature_detection

Conversation

@thaJeztah

Copy link
Copy Markdown
Member

based on top of #2456

backports of:

@thaJeztahthaJeztah added this to the 19.03.9 milestone Apr 20, 2020
@thaJeztah
thaJeztahforce-pushed the 19.03_backport_lazy_feature_detection branch 2 times, most recently from 906feb7 to c079435CompareApril 21, 2020 15:19
Anca Iordacheand others added 3 commits April 29, 2020 17:12
Signed-off-by: Anca Iordache <anca.iordache@docker.com>
Possible approach for client info
- split ClientInfo() into ClientInfo() and loadClientInfo()
- split ConfigFile() into ConfigFile() and loadConfigFile()
- ConfigFile() and ClientInfo() call their corresponding loadXX function
if it has not yet been loaded; this allows them to be used before
Initialize() was called.
- Initialize() *always* (re-)loads the configuration; this makes sure
that the correct configuration is used when actually calling commands.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 22a5dad)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit a88a1be)
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>
(cherry picked from commit b397391)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
thaJeztahforce-pushed the 19.03_backport_lazy_feature_detection branch from c079435 to cf663b5CompareApril 29, 2020 15:13
@thaJeztah
thaJeztah marked this pull request as ready for review April 29, 2020 15:13
Comment threadcli/command/cli.go
func (cli *DockerCli) ClientInfo() ClientInfo {
return cli.clientInfo
if cli.clientInfo == nil {
_ = cli.loadClientInfo()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

find this a bit weird but oh well

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

the _ = you mean?
mostly to satisfy linters from possibly warning "you didn't check the error!"

@silvin-lubeckisilvin-lubecki 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

@silvin-lubecki
silvin-lubecki merged commit 3929933 into docker:19.03Apr 30, 2020
@thaJeztah
thaJeztah deleted the 19.03_backport_lazy_feature_detection branch April 30, 2020 15:44
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@thaJeztah@tiborvass@silvin-lubecki