Defer API version negotiation #1747

simonferquel · 2019-03-18T11:35:02Z

- What I did

Fix #1739 by deferring the API version negotiation to the point we actually need access to either the ServerInfo or the API client interface.

Also introduce a no-remote annotation for marking commands that do not require anything on the daemon side to work. Commands annotated as such are not evaluated by the conditional hiding feature (which relies on ServerInfo to get access to the daemon OS, experimental flag, etc.).

Context commands are annotated as no-remote

- How to verify it

Create a docker context to an unreachable endpoint : docker context create faulty --docker host=tcp://42.42.42.41:4242
Makes it your default context: docker context use faulty
Commands that require interaction with the daemon should trigger a 1m timeout (e.g docker info)
Commands that do not require any interaction with the daemon should run quickly (e.g. docker context ls)

CC @silvin-lubecki @thaJeztah

cmd/docker/docker.go

thaJeztah · 2019-03-18T12:16:23Z

Can you s/negociation/negotiation in your commit-message? 🤗 (Otherwise I'm sure I won't be able to find back this change 😅 )

silvin-lubecki · 2019-03-18T12:53:05Z

cmd/docker/docker.go

+
+func isNoRemote(cmd *cobra.Command) bool {
+	for cmd != nil {
+		if _, ok := cmd.Annotations["no-remote"]; ok {


nit, but I don't like negations in names, because you can end up with a double negations, like if !isNoRemote(...) ... and double negations are generally brain fucked 😅
That said, I don't have better name to propose... I thought about "local" but I think it is too vague.
@thaJeztah any idea? 😅

IsLocal ?

IsDaemonLess ?

Can you rename the annotation too? no-remote -> local-only ?

codecov-io · 2019-03-18T14:00:41Z

Codecov Report

Merging #1747 into master will increase coverage by 0.18%.
The diff coverage is 76.36%.

@@            Coverage Diff             @@
##           master    #1747      +/-   ##
==========================================
+ Coverage   56.26%   56.44%   +0.18%     
==========================================
  Files         308      308              
  Lines       21293    21354      +61     
==========================================
+ Hits        11980    12053      +73     
- Misses       8439     8442       +3     
+ Partials      874      859      -15

simonferquel · 2019-03-18T14:02:11Z

Rebased, rewrote commit text, small refactoring to support checks for both local-only and experimental CLI

silvin-lubecki

Code looks good, but I fear some regressions as it is like a huge refactoring, and we already saw many regressions on this part before + there are not much tests 😞
Do you think you could add some?

simonferquel · 2019-03-19T15:17:16Z

Added a LOT of tests. PTAL @silvin-lubecki, @thaJeztah

thaJeztah

left some comments inline, but looks good overall

cmd/docker/docker_test.go

cli/command/config/create.go

cmd/docker/docker.go

thaJeztah · 2019-03-21T12:16:01Z

ping @simonferquel 🤗

simonferquel · 2019-03-21T16:40:57Z

@thaJeztah @silvin-lubecki I just rebased, extracted the fixes in different PRs (which I am based onto).
I had a problem because recently, docker build --platform has changed its logic around annotations and requires the serverInfo while constructing the command (putting different annotations depending on buildkit enabled state).

To fix the problem, I added a notion of "LazyFlagCheck" which allows to add custom validation logic on a flag, which will get executed after the cli is actually initialized. It is a bit ugly around the edges though, and I'd be glad if we could have a different approach.
@vdemeester any thought about this one ?

This commit defers API version negotiation within the dockerCli structure to the point where we need to access server info or the API client itself. This also introduce a "no-remote" annotation, to skip the evaluation of server-side support for those commands (which requires access to server-info). The context subcommands are marked with this "no-remote" annotation so that they respond quickly even if the current context points to an unreachable endpoint. Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

simonferquel · 2019-03-27T08:26:04Z

@thaJeztah PTAL :) (and @silvin-lubecki as well)

cli/command/image/build.go

silvin-lubecki

I approve this PR, even if I find this lazycheck logic too complicated. I think it was introduced due to this PR, we should try to fix/simplify that in a follow-up.

Otherwise thank you @simonferquel for the bunch of tests 😍

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

thaJeztah · 2019-04-02T13:39:56Z

Hm.. not sure if it's introduced in this PR, but trying this;

docker -H tcp://localhost:1234 version
Please specify only one -H

Instead of

docker -H tcp://localhost:1234 version

Client: Docker Engine - Community
 Version:           18.09.3
 API version:       1.39
 Go version:        go1.10.8
 Git commit:        774a1f4
 Built:             Thu Feb 28 06:32:50 2019
 OS/Arch:           darwin/amd64
 Experimental:      true
Cannot connect to the Docker daemon at tcp://localhost:1234. Is the docker daemon running?

thaJeztah

Some nits, but I agree with @silvin-lubecki that this adds a lot of complexity; I want to give this another look and see if we can somehow make this less complex. (e.g. just add a local-only and/or lazy-evaluate annotation where needed, and skip those if serverinfo is not present? I'd have to give that some thought.

thaJeztah · 2019-04-02T13:08:06Z

cli/command/commands/lazychecks/lazychecks.go

+}
+
+// EvaluateFlagLazyChacks evaluates the lazy checks associated with a flag depending on client/server info
+func EvaluateFlagLazyChacks(flag *pflag.Flag, clientInfo command.ClientInfo, serverInfo command.ServerInfo, clientVersion string) error {


Suggested change

func EvaluateFlagLazyChacks(flag *pflag.Flag, clientInfo command.ClientInfo, serverInfo command.ServerInfo, clientVersion string) error {

func EvaluateFlagLazyChecks(flag *pflag.Flag, clientInfo command.ClientInfo, serverInfo command.ServerInfo, clientVersion string) error {

thaJeztah · 2019-04-02T13:08:14Z

cli/command/commands/lazychecks/lazychecks.go

+	f.Annotations[lazyCheckAnnotation] = append(f.Annotations[lazyCheckAnnotation], fmt.Sprintf("%d", index))
+}
+
+// EvaluateFlagLazyChacks evaluates the lazy checks associated with a flag depending on client/server info


Suggested change

// EvaluateFlagLazyChacks evaluates the lazy checks associated with a flag depending on client/server info

// EvaluateFlagLazyChecks evaluates the lazy checks associated with a flag depending on client/server info

thaJeztah · 2019-04-02T13:09:15Z

cmd/docker/docker.go

@@ -8,6 +8,8 @@ import (
 	"strings"
 	"syscall"

+	"github.com/docker/cli/cli/command/commands/lazychecks"


this should be grouped with the imports below

simonferquel · 2019-04-03T07:48:39Z

@thaJeztah bad news, it does the same thing on master. Toplevel flags seem to be set twice for some reason. For flags of type "list" it insert the same value twice.

This partially mitigates docker#1739 ("Docker commands take 1 minute to timeout if context endpoint is unreachable") and is a simpler alternative to docker#1747 (which completely defers the client connection until an actual call is attempted). Note that the previous 60s delay was the culmination of two separate 30s timeouts since the ping is tried twice. This with this patch the overall timeout is 20s. moby/moby#39206 will remove the second ping and once that propagates to this tree the timeout will be 10s. Signed-off-by: Ian Campbell <ijc@docker.com>

gtardif · 2020-04-07T14:35:54Z

@thaJeztah hi, digging a bit in history, any reason for holding this one ? It looks like something that might be needed for #2420.

thaJeztah · 2020-04-07T15:02:06Z

I'd have to look into this one again; I know I wasn't particularly happy with the amount of complexity involved. Wondering if using the WithAPIVersionNegotiation (moby/moby#39032) option would do the trick 🤔

thaJeztah · 2020-04-07T15:05:16Z

I see this PR is on master, but not in the 19.03 release; #1845. Haven't tested if it solves the delay though

GordonTheTurtle added the status/0-triage label Mar 18, 2019

silvin-lubecki reviewed Mar 18, 2019

View reviewed changes

cmd/docker/docker.go Outdated Show resolved Hide resolved

silvin-lubecki reviewed Mar 18, 2019

View reviewed changes

simonferquel force-pushed the defer-context-initialization branch from e87de89 to 427e193 Compare March 18, 2019 14:00

simonferquel force-pushed the defer-context-initialization branch from 427e193 to dd6d954 Compare March 18, 2019 14:01

thaJeztah changed the title ~~Defer API version negociation~~ Defer API version negotiation Mar 18, 2019

silvin-lubecki reviewed Mar 18, 2019

View reviewed changes

thaJeztah requested changes Mar 19, 2019

View reviewed changes

cmd/docker/docker_test.go Outdated Show resolved Hide resolved

cli/command/config/create.go Outdated Show resolved Hide resolved

cmd/docker/docker.go Show resolved Hide resolved

thaJeztah added status/2-code-review and removed status/0-triage labels Mar 19, 2019

simonferquel force-pushed the defer-context-initialization branch from 403b94d to 90b2d14 Compare March 21, 2019 16:36

simonferquel force-pushed the defer-context-initialization branch from 90b2d14 to 19c0830 Compare March 21, 2019 17:14

simonferquel added 5 commits March 26, 2019 14:58

Support combination of no-remote and experimental CLI

188fcee

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

Renamed no-remote to local-only

4b2de6c

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

Added a bunch of tests for flags/commands validation

12c9767

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

Rebase-fix: add a lazy-check annotation

4975ead

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

simonferquel force-pushed the defer-context-initialization branch from 19c0830 to 4975ead Compare March 26, 2019 13:58

silvin-lubecki reviewed Mar 28, 2019

View reviewed changes

cli/command/image/build.go Outdated Show resolved Hide resolved

silvin-lubecki approved these changes Mar 28, 2019

View reviewed changes

Fix Import style

d35f48c

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

simonferquel force-pushed the defer-context-initialization branch from b1537da to d35f48c Compare March 28, 2019 14:54

thaJeztah requested changes Apr 2, 2019

View reviewed changes

silvin-lubecki mentioned this pull request Apr 15, 2019

19.03.0-beta1: docker context commands hang if current engine is not reachable #1831

Closed

ijc mentioned this pull request May 13, 2019

add a 10s timeout to the client object. #1872

Merged

thaJeztah mentioned this pull request Apr 7, 2020

cli: perform feature detection lazily #2424

Merged

silvin-lubecki closed this in #2424 Apr 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Defer API version negotiation #1747

Defer API version negotiation #1747

simonferquel commented Mar 18, 2019 •

edited

Loading

thaJeztah commented Mar 18, 2019

silvin-lubecki Mar 18, 2019

thaJeztah Mar 18, 2019

simonferquel Mar 18, 2019

silvin-lubecki Mar 18, 2019

codecov-io commented Mar 18, 2019 •

edited

Loading

simonferquel commented Mar 18, 2019

silvin-lubecki left a comment

simonferquel commented Mar 19, 2019

thaJeztah left a comment

thaJeztah commented Mar 21, 2019

simonferquel commented Mar 21, 2019

simonferquel commented Mar 27, 2019

silvin-lubecki left a comment

thaJeztah commented Apr 2, 2019

thaJeztah left a comment

thaJeztah Apr 2, 2019

thaJeztah Apr 2, 2019

thaJeztah Apr 2, 2019

simonferquel commented Apr 3, 2019

gtardif commented Apr 7, 2020

thaJeztah commented Apr 7, 2020

thaJeztah commented Apr 7, 2020

	func EvaluateFlagLazyChacks(flag *pflag.Flag, clientInfo command.ClientInfo, serverInfo command.ServerInfo, clientVersion string) error {
	func EvaluateFlagLazyChecks(flag *pflag.Flag, clientInfo command.ClientInfo, serverInfo command.ServerInfo, clientVersion string) error {

	// EvaluateFlagLazyChacks evaluates the lazy checks associated with a flag depending on client/server info
	// EvaluateFlagLazyChecks evaluates the lazy checks associated with a flag depending on client/server info

Defer API version negotiation #1747

Defer API version negotiation #1747

Conversation

simonferquel commented Mar 18, 2019 • edited Loading

thaJeztah commented Mar 18, 2019

silvin-lubecki Mar 18, 2019

Choose a reason for hiding this comment

thaJeztah Mar 18, 2019

Choose a reason for hiding this comment

simonferquel Mar 18, 2019

Choose a reason for hiding this comment

silvin-lubecki Mar 18, 2019

Choose a reason for hiding this comment

codecov-io commented Mar 18, 2019 • edited Loading

Codecov Report

simonferquel commented Mar 18, 2019

silvin-lubecki left a comment

Choose a reason for hiding this comment

simonferquel commented Mar 19, 2019

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Mar 21, 2019

simonferquel commented Mar 21, 2019

simonferquel commented Mar 27, 2019

silvin-lubecki left a comment

Choose a reason for hiding this comment

thaJeztah commented Apr 2, 2019

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Apr 2, 2019

Choose a reason for hiding this comment

thaJeztah Apr 2, 2019

Choose a reason for hiding this comment

thaJeztah Apr 2, 2019

Choose a reason for hiding this comment

simonferquel commented Apr 3, 2019

gtardif commented Apr 7, 2020

thaJeztah commented Apr 7, 2020

thaJeztah commented Apr 7, 2020

simonferquel commented Mar 18, 2019 •

edited

Loading

codecov-io commented Mar 18, 2019 •

edited

Loading