Add timeout for Docker Version API #1363

aaithal · 2018-04-25T18:07:05Z

Summary

Add timeout for Docker Version API

Implementation details

b1dae3d dockerclient: add timeout for Version API
This change adds a timeout for Docker client's Version API.
Additionally, the Docker version returned in saved in the task engine.
This helps with avoiding additional unnecessarily calls to the Docker
Version API everytime the agent connects to the backend. This should
help with issues seen in ecs-agent 1.17.2 stop connection to AWS and start reporting expired credential to task #1331

Finally, the dockerclient package has been refactored to extract all
timeout related constants to dockerclient package from the dockerapi
package to avoid circular dependencies.
63f0650 updated go-dickerclient for VersionWithContext
Updated go-dockerclient to get the VersionWithContext() method, which
helps with adding timeouts against the Docker Version() API, which can
occasionally timeout.

Testing

Builds on Linux (make release)
Builds on Windows (go build -out amazon-ecs-agent.exe ./agent)
Unit tests on Linux (make test) pass
Unit tests on Windows (go test -timeout=25s ./agent/...) pass
Integration tests on Linux (make run-integ-tests) pass
Integration tests on Windows (.\scripts\run-integ-tests.ps1) pass
Functional tests on Linux (make run-functional-tests) pass
Functional tests on Windows (.\scripts\run-functional-tests.ps1) pass

New tests cover the changes:

Description for the changelog

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Updated go-dockerclient to get the VersionWithContext() method, which helps with adding timeouts against the Docker Version() API, which can occasionally timeout.

This change adds a timeout for Docker client's Version API. Additionally, the Docker version returned in saved in the task engine. This helps with avoiding additional unnecessarily calls to the Docker Version API everytime the agent connects to the backend. This should help with issues seen in aws#1331 Finally, the dockerclient package has been refactored to extract all timeout related constants to dockerclient package from the dockerapi package to avoid circular dependencies.

richardpen · 2018-04-25T18:30:05Z

agent/Gopkg.lock

@@ -292,7 +292,6 @@
  name = "golang.org/x/net"
  packages = [
    "context",
-    "context/ctxhttp",


This is nice, I think go docker client finally moved to builtin context. Out of this PR: probably we can get rid of golang.org/x/ dependency?

Out of this PR: probably we can get rid of golang.org/x/ dependency?

Not sure, there are many sub-packages that we have a dependency on. Which one in particular you want to get rid of?

I think the golang.org/x/net can be removed.

richardpen · 2018-04-25T19:38:14Z

agent/engine/docker_task_engine.go

@@ -1126,7 +1131,18 @@ func (engine *DockerTaskEngine) State() dockerstate.TaskEngineState {

 // Version returns the underlying docker version.
 func (engine *DockerTaskEngine) Version() (string, error) {
-	return engine.client.Version()
+	if engine.dockerVersion != "" {


Do you think we need a lock for this method? Also, do we need a retry for this?

I don't think this needs a retry. Looking at the usage, there's a remote possibility of a race condition. I'll add a scoped down mutex just for this for now. We can expand it's scope to the whole of DockerTaskEngine if needed.

richardpen · 2018-04-25T19:47:17Z

agent/dockerclient/dockerapi/docker_client.go

 	client, err := dg.dockerClient()
 	if err != nil {
 		return "", err
 	}
-	info, err := client.Version()
+	info, err := client.VersionWithContext(derivedCtx)


Why are we not listening to the ctx.Done channel like the other Docker APIs?

Other APIs require special error handling on timeouts. They all require the output from Docker APIs to be massaged in some way and are invoked in an internal async method. Version() doesn't need any such semantics.

richardpen · 2018-04-25T19:53:24Z

agent/dockerclient/clientfactory/dockerclientfactory.go

 	// if the client version returns a MinAPIVersion and APIVersion, then use it to return
 	// all the Docker clients between MinAPIVersion and APIVersion, else try pinging
 	// the clients in getKnownAPIVersions
 	var minAPIVersion, apiVersion string
 	// get a Docker client with the default supported version
 	client, err := newVersionedClient(endpoint, string(minDockerAPIVersion))
 	if err == nil {
-		clientVersion, err := client.Version()
+		derivedCtx, cancel := context.WithTimeout(ctx, dockerclient.VersionTimeout)


Can't just call client.Version here?

Because Version() is defined on dockerGoClient. This one is dockeriface.Client

Why are we not calling dockerGoClient's Version instead?

Because it's not there. dockerGoClient belongs to the engine package and is only used by DockerTaskEngine

sharanyad · 2018-04-25T22:22:39Z

CHANGELOG.md

@@ -1,5 +1,8 @@
 # Changelog

+## 1.17.4-dev
+* Bug - Fixed a bug where Docker Version() API takes never returns by adding a timeout [#1363](https://github.com/aws/amazon-ecs-agent/pull/1363)


nit: API never returns by..

Moved the logic of caching Docker daemon version string out of the DockerTaskEngine to dockerGoClient struct. This makes it much more reusable across the agent. Also, got rid of the unused SetDockerClient() method in the DockerTaskEngine.

richardpen

LGTM

sharanyad · 2018-04-26T19:46:25Z

CHANGELOG.md

@@ -1,5 +1,8 @@
 # Changelog

+## 1.17.4-dev
+* Bug - Fixed a bug where Docker Version() API never returns by adding a timeout [#1363](https://github.com/aws/amazon-ecs-agent/pull/1363)


Should we put Version() in `` ?

I don't think it matters. I can modify it if you really insist (prefer not to 😃 ).

sharanyad · 2018-04-26T19:48:42Z

agent/dockerclient/clientfactory/dockerclientfactory.go

 	// if the client version returns a MinAPIVersion and APIVersion, then use it to return
 	// all the Docker clients between MinAPIVersion and APIVersion, else try pinging
 	// the clients in getKnownAPIVersions
 	var minAPIVersion, apiVersion string
 	// get a Docker client with the default supported version
 	client, err := newVersionedClient(endpoint, string(minDockerAPIVersion))
 	if err == nil {
-		clientVersion, err := client.Version()
+		derivedCtx, cancel := context.WithTimeout(ctx, dockerclient.VersionTimeout)


Why are we not calling dockerGoClient's Version instead?

aaithal added 2 commits April 25, 2018 08:46

updated go-dickerclient for VersionWithContext

63f0650

Updated go-dockerclient to get the VersionWithContext() method, which helps with adding timeouts against the Docker Version() API, which can occasionally timeout.

aaithal added this to the 1.17.4 milestone Apr 25, 2018

aaithal added a commit to aaithal/amazon-ecs-agent that referenced this pull request Apr 25, 2018

changelog: for aws#1363 (timeout for Docker Version API)

b57ed65

aaithal requested review from adnxn, richardpen, yhlee-aws and sharanyad April 25, 2018 19:47

richardpen reviewed Apr 25, 2018

View reviewed changes

sharanyad reviewed Apr 25, 2018

View reviewed changes

aaithal added 2 commits April 26, 2018 08:33

changelog: for aws#1363 (timeout for Docker Version API)

f6873b3

aaithal force-pushed the versionTimeout branch from 7e78571 to 3387f22 Compare April 26, 2018 15:33

richardpen approved these changes Apr 26, 2018

View reviewed changes

yhlee-aws approved these changes Apr 26, 2018

View reviewed changes

aaithal mentioned this pull request Apr 26, 2018

Add timeout for setting up pause container #1358

Merged

8 tasks

sharanyad reviewed Apr 26, 2018

View reviewed changes

sharanyad approved these changes Apr 26, 2018

View reviewed changes

aaithal merged commit 256a7fd into aws:dev Apr 26, 2018

aaithal added a commit that referenced this pull request Apr 26, 2018

changelog: for #1363 (timeout for Docker Version API)

1dc77b5

aaithal modified the milestones: 1.17.4, 1.18.0 May 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add timeout for Docker Version API #1363

Add timeout for Docker Version API #1363

aaithal commented Apr 25, 2018 •

edited

Loading

richardpen Apr 25, 2018

aaithal Apr 26, 2018

richardpen Apr 26, 2018

richardpen Apr 25, 2018

aaithal Apr 26, 2018

richardpen Apr 25, 2018

aaithal Apr 26, 2018

richardpen Apr 25, 2018

aaithal Apr 26, 2018

sharanyad Apr 26, 2018

aaithal Apr 26, 2018

sharanyad Apr 25, 2018

richardpen left a comment

sharanyad Apr 26, 2018

aaithal Apr 26, 2018

sharanyad Apr 26, 2018

Add timeout for Docker Version API #1363

Add timeout for Docker Version API #1363

Conversation

aaithal commented Apr 25, 2018 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardpen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaithal commented Apr 25, 2018 •

edited

Loading