pull client API version negotiation out of the CLI and into the client by rremer · Pull Request #32779 · moby/moby

rremer · 2017-04-22T19:01:01Z

- What I did

Added some tests for client version discovery, and moved that logic out of the CLI and into the client.

The drive here is that I was using the go docker client, and had to call UpdateVersion with a string I needed to get from the server. I noticed that the CLI never had to do this, and sure enough there was some server version negotiation going on in the CLI. At this commit, the CLI just implements a normal client, and the client owns the logic for server version negotiation (so folks consuming the client library can enjoy that same functionality).

- How I did it

Added a new conditional statement in client/client.go UpdateClientVersion(str). If the string is empty, it will trigger DowngradeClientVersion() and step into some cut/paste logic that already existed in cli/command/cli.go for pinging the server, grabbing the APIVersion string, and updating that.

- How to verify it

docker run --privileged -e DOCKER_GITCOMMIT=eebfdd0 docker hack/make.sh test-unit test-integration-cli test-docker-py

or

cd client
go test

The manual/functional verification for the CLI built against this commit:

start a daemon of an older version
docker info

The test cases cover a few more edges than that:

if the server doesn't return an APIVersion
if the environment of the CLI specifically pins DOCKER_API_VERSION

- Description for the changelog

UpdateClientVersion can auto-downgrade to the server APIVersion

- A picture of a cute animal (not mandatory but encouraged)

vdemeester · 2017-04-26T13:20:52Z

@rremer build error

09:50:33 --- FAIL: TestClientDebugEnabled (0.00s)
09:50:33 	assertions.go:226: 
                          
	Error Trace:	docker_test.go:21
09:50:33 		
	Error:      	Received unexpected error:
09:50:33 		
	            	Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
09:50:33 FAIL
09:50:33 coverage: 29.1% of statements

dnephin

Overall I think this is good, so design LGTM

dnephin · 2017-04-26T14:41:00Z

We don't want to fail here if there is an error. A user might just be running with --help to figure out how to specify the docker host.

This should return nil, and I guess could use a comment about why.

dnephin · 2017-04-26T14:48:19Z

There are two distinct use cases for this function so I don't think it makes sense to group these together.

DockerCLI.Initialize() should call DowngradeClientVersionPing() directly, and leave this functions as it was.

We might be able to just delete UpdateClientVersion() , I don't think it's called from anywhere else.

Indeed there are no other references to UpdateClientVersion that I can find, however removing it would:

potentially break existing consumers of the client library

put update logic into the new negotiate function (although, it is just updating a field)

We don't make any promises about not breaking the API on client/ it's not being released as a versioned package.

dnephin · 2017-04-26T14:53:23Z

I think this needs a more appropriate name, It's not necessarily downgrading, it might just be verifying the version is correct. It's also the "APIVersion", not the client version. Maybe NegotiateAPIVersion() ?

dnephin · 2017-04-26T15:09:54Z

Same comment about the function name, how about UpdateAPIVersionFromPing() ?

The check for if !cli.manualOverride {...} needs to move here. It should be the first line in the function I think:

if cli.manualOverride { return }

dnephin · 2017-04-26T15:13:13Z

assert.Equal(t, expected, client.version)

(same with the other assertions)

I was trying to use the same format as the rest of the tests in this suite. If I understood your suggestion, we want to use an assertion framework, so I reworked the tests here to use the same assertion library found in other packages here. I kept this in a separate commit, since it's unrelated cleanup.

Yes, we are slowly migrating everything to testify assertions.

rremer · 2017-04-28T01:51:04Z

Thanks for the feedback, @dnephin !

dnephin · 2017-04-28T19:28:57Z

This doesn't seem like the right place for this. We also don't want to call ping again. I think this line needs to be removed.

Ah yes, I misunderstood the request to move this to "Initialize". I'm guessing we meant "When we remove UpdateClientVersion, then NegotiateAPIVersion should just take its place".

dnephin · 2017-04-28T19:29:15Z

This should be `NegotiateAPIVersionPing(ping)

dnephin

LGTM

cpuguy83 · 2017-06-09T01:48:41Z

This one needs a rebase because the CLI has been removed from this repo (though the client still exists here).

rremer · 2017-06-13T01:28:55Z

Woah, I leave the country for a bit and the whole project disappears! I've rebased, and once it's merged/released we can update the cli to take advantage of NegotiateAPIVersion() @cpuguy83

Incidentally, since the project restructure, moby/moby:master has a failing test in client_test.go: TestClientRedirect, happy to investigate that, just wanted to point out that it's unrelated to this change.

dnephin · 2017-06-13T17:45:49Z

I guess context should be an argument to this function. the caller can decide if they have a context they'd like to re-use, or if they want to start a new background context.

dnephin

LGTM

rremer · 2017-06-16T03:22:47Z

It appears that node azure-windows-rs1-5 may require daemon/logger/adapter_test.go:137 to be more than 10 milliseconds. Slow shared tenant windows vms aside, we probably don't want to consider log reading to be a failure if it takes more than 10 milliseconds . Maybe >1s is a failure?

dnephin

LGTM

And yes, that timeout should be increased. I've seen it fail a few times on windowsRS1

rremer · 2017-06-19T01:59:59Z

FYI, I standardized the timeouts in daemon logger tests in #33727, and implemented the same assertion framework there @dnephin ... so hopefully you won't have to rebuild as much :p

cpuguy83 · 2017-06-20T16:55:59Z

This should not be exported.

cpuguy83 · 2017-06-20T16:56:03Z

This should not be exported.

sure, done.

Signed-off-by: Royce Remer <[email protected]>

cpuguy83 · 2017-06-21T14:12:56Z

+
+// NegotiateAPIVersionPing updates the version string associated with this
+// instance of the Client to match the latest version the server supports
+func (cli *Client) NegotiateAPIVersionPing(p types.Ping) {


Does this need to be exposed on the interface?

I believe it does. This is what we'll call from docker/cli because we already have a Ping object, which we need to store HasExperimental and OSType.

The other one is a "convenience" for other consumers of this library.

cpuguy83

LGTM

GordonTheTurtle added the status/0-triage label Apr 22, 2017

AkihiroSuda added area/cli Client status/failing-ci Indicates that the PR in its current state fails the test suite labels Apr 25, 2017

vdemeester added status/1-design-review and removed status/0-triage labels Apr 26, 2017

vdemeester requested review from dnephin and vieux April 26, 2017 13:20

dnephin reviewed Apr 26, 2017

View reviewed changes

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Apr 28, 2017

dnephin reviewed Apr 28, 2017

View reviewed changes

rremer force-pushed the auto-update-client branch from 0cb5794 to e298205 Compare April 30, 2017 23:46

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Apr 30, 2017

dnephin added rebuild/powerpc and removed status/failing-ci Indicates that the PR in its current state fails the test suite labels May 1, 2017

GordonTheTurtle removed rebuild/powerpc labels May 1, 2017

dnephin added status/2-code-review and removed status/1-design-review labels May 1, 2017

dnephin approved these changes May 1, 2017

View reviewed changes

thaJeztah added the impact/cli label May 5, 2017

GordonTheTurtle assigned cpuguy83 May 7, 2017

rremer force-pushed the auto-update-client branch from e298205 to fe396be Compare June 13, 2017 01:21

dnephin reviewed Jun 13, 2017

View reviewed changes

moby deleted a comment from GordonTheTurtle Jun 13, 2017

thaJeztah mentioned this pull request Jun 15, 2017

Stop using deprecated SockRequest #33534

Merged

rremer force-pushed the auto-update-client branch 2 times, most recently from 174200d to 75b5de5 Compare June 16, 2017 00:02

dnephin approved these changes Jun 16, 2017

View reviewed changes

dnephin added the rebuild/windowsRS1 label Jun 16, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Jun 16, 2017

dnephin approved these changes Jun 16, 2017

View reviewed changes

cpuguy83 requested changes Jun 20, 2017

View reviewed changes

pull client API version negotiation out of the CLI and into the client

5f1d94e

Signed-off-by: Royce Remer <[email protected]>

rremer force-pushed the auto-update-client branch from 75b5de5 to 5f1d94e Compare June 21, 2017 05:58

cpuguy83 reviewed Jun 21, 2017

View reviewed changes

cpuguy83 approved these changes Jun 22, 2017

View reviewed changes

cpuguy83 merged commit 8a672ba into moby:master Jun 22, 2017

Conversation

rremer commented Apr 22, 2017

Uh oh!

vdemeester commented Apr 26, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rremer commented Apr 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Jun 9, 2017

Uh oh!

rremer commented Jun 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

rremer commented Jun 16, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

rremer commented Jun 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants