Enforce minimum docker version support #1477

yhlee-aws · 2018-07-27T00:05:26Z

…revious versions

Summary

Deprecate support for docker version older than 1.9.0, by checking for Docker API 1.21 support. Using API version instead of Docker version since it's been more consistent historically.

Implementation details

During agent startup, check for docker api version and return ExitTerminal if 1.21 is not supported. See https://docs.docker.com/develop/sdk/#api-version-matrix (docker 1.9.0 corresponds to api 1.21).

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: yes

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.

petderek · 2018-07-27T20:49:17Z

agent/app/agent.go

@@ -579,3 +588,24 @@ func (agent *ecsAgent) startACSSession(
 	seelog.Critical("ACS Session handler should never exit")
 	return exitcodes.ExitError
 }
+
+// validateRequiredDockerVersion validates docker version to be minimum 1.9.0
+func (agent *ecsAgent) validateRequiredDockerVersion(selector string) error {


Why can't we use a combination of ACS capabilities and API versions like we have done for other features?

talked offline

sharanyad · 2018-07-27T20:58:11Z

agent/app/agent.go

+// validateRequiredDockerVersion validates docker version to be minimum 1.9.0
+func (agent *ecsAgent) validateRequiredDockerVersion(selector string) error {
+	dockerVersion, err := agent.dockerClient.Version(agent.ctx, dockerclient.VersionTimeout)
+	if err != nil {


Should we retry in the case of a timeout and not exit with a terminal code?

it will exit with terminal code, since timeout will result in error.

I was wondering if timeout was a transient issue, it might make sense for init to try and restart agent again to find the docker version. So instead of returning a terminal code, we could exit with a non terminal one. What do you think?

if there's a timeout, we could exit with a non terminal error code imo

petderek · 2018-07-27T21:57:23Z

agent/app/agent.go

+		return errors.New("Could not get docker version")
+	}
+
+	match, err := utils.Version(dockerVersion).Matches(selector)


how does this handle unexpected strings? dev or custom docker builds?

aaithal · 2018-07-27T22:32:05Z

README.md

@@ -14,6 +14,8 @@ The Amazon ECS Container Agent is a component of Amazon Elastic Container Servic
 The best source of information on running this software is the
 [Amazon ECS documentation](http://docs.aws.amazon.com/AmazonECS/latest/developerguide/ECS_agent.html).

+Please note that from Agent version 1.20.0, Docker versions 1.9.0 or older are no longer supported.


Please add a link to our docs page as well where information can be found and add a sentence here to the effect of "For more information, please visit the ECS agent documentation"

aaithal · 2018-07-27T22:33:46Z

agent/app/agent.go

+	dockerVersion, err := agent.dockerClient.Version(agent.ctx, dockerclient.VersionTimeout)
+	if err != nil {
+		seelog.Criticalf("Could not get docker version: %v", err)
+		return errors.New("Could not get docker version")


(multiple occurrences) please follow error string format as per https://github.com/golang/go/wiki/CodeReviewComments#error-strings

aaithal · 2018-07-27T22:35:35Z

agent/app/agent.go

+// validateRequiredDockerVersion validates docker version to be minimum 1.9.0
+func (agent *ecsAgent) validateRequiredDockerVersion(selector string) error {
+	dockerVersion, err := agent.dockerClient.Version(agent.ctx, dockerclient.VersionTimeout)
+	if err != nil {


if there's a timeout, we could exit with a non terminal error code imo

petderek

Thanks for making another pass at this.

petderek · 2018-07-28T06:45:37Z

agent/app/agent.go

+	minVersionMet := false
+	for _, version := range supportedVersions {
+		if version == dockerclient.Version_1_21 {
+			minVersionMet = true


if you return -1, true here, you can get rid of this variable and the next if statement. :)

petderek · 2018-07-28T06:51:17Z

agent/app/agent.go

@@ -213,6 +214,12 @@ func (agent *ecsAgent) doStart(containerChangeEventStream *eventstream.EventStre
 	imageManager engine.ImageManager,
 	client api.ECSClient) int {

+	// check docker version >= 1.9.0, exit agent if older
+	exitcode, valid := agent.validateRequiredVersion()


If you wanted, you could follow this idiom.

if exitcode, valid:= agent.validate(); !valid{ return exitcode }

petderek · 2018-07-28T06:53:20Z

README.md

@@ -14,6 +14,8 @@ The Amazon ECS Container Agent is a component of Amazon Elastic Container Servic
 The best source of information on running this software is the
 [Amazon ECS documentation](http://docs.aws.amazon.com/AmazonECS/latest/developerguide/ECS_agent.html).

+Please note that from Agent version 1.20.0, Minimum required docker version is 1.9.0, corresponding to API version 1.21. For more information, please visit [Amazon ECS Container Agent Versions](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/container_agent_versions.html).


maybe fully qualify "Docker API version 1.21"

adnxn

nits and question about additional unit test case

adnxn · 2018-07-30T16:19:44Z

agent/app/agent.go

@@ -213,6 +214,11 @@ func (agent *ecsAgent) doStart(containerChangeEventStream *eventstream.EventStre
 	imageManager engine.ImageManager,
 	client api.ECSClient) int {

+	// check docker version >= 1.9.0, exit agent if older
+	if exitcode, verified := agent.verifyRequiredVersion(); !verified {


nit, naming: can we use if exitcode, ok := here? would be in line with golang bool returns convention.

adnxn · 2018-07-30T16:22:57Z

agent/app/agent.go

+	}
+
+	// api 1.21 is not supported, docker version is older than 1.9.0
+	seelog.Criticalf("Required minimum docker API verion %s is not supported ",


nit: extra space at end of log line

adnxn · 2018-07-30T16:28:05Z

agent/app/agent_test.go

+	oldAPIVersions := []dockerclient.DockerVersion{
+		dockerclient.Version_1_18,
+		dockerclient.Version_1_19,
+		dockerclient.Version_1_20}


should a unit test cover the happy path as well? checking for Version_1_21 as well?

That usecase is covered by all the other unit tests in this file, using "apiVersions".

sharanyad · 2018-07-30T16:43:45Z

agent/app/agent_test.go

+
+	exitCode := agent.doStart(eventstream.NewEventStream("events", ctx),
+		credentialsManager, state, imageManager, client)
+	assert.Equal(t, exitcodes.ExitTerminal, exitCode)


Could you please add a test case for exitcodes.ExitError as well?

added TestDoStartMinimumSupportedDockerVersionError test case

sharanyad · 2018-07-30T16:44:30Z

agent/app/agent.go

+// validateRequiredVersion validates docker version.
+// Minimum docker version supported is 1.9.0, maps to api version 1.21
+// see https://docs.docker.com/develop/sdk/#api-version-matrix
+func (agent *ecsAgent) verifyRequiredVersion() (int, bool) {


Renaming this to verifyRequiredDockerVersion might be more descriptive.

richardpen

If we are doing this, should we also remove the 1.17, ,1.18, 1.19, 1.20 from the list of supported versions, and also the default version should now be changed to 1.21?

richardpen · 2018-07-30T16:34:56Z

CHANGELOG.md

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

+## 1.20.0-dev
+* Feature - Support Docker volume and driver plugins.


Probably it's better to list them as two items?

Feature - Add support for Docker volume plugins Enhancement - Replace the empty container with Docker local volume

We should add a note about locking out older docker versions -- the changelog would be a reasonable place to put a significant change like that.

I've added "deprecating support for older docker versions" as enhancement.

richardpen · 2018-07-30T16:37:25Z

README.md

@@ -14,6 +14,8 @@ The Amazon ECS Container Agent is a component of Amazon Elastic Container Servic
 The best source of information on running this software is the
 [Amazon ECS documentation](http://docs.aws.amazon.com/AmazonECS/latest/developerguide/ECS_agent.html).

+Please note that from Agent version 1.20.0, Minimum required Docker version is 1.9.0, corresponding to Docker API version 1.21. For more information, please visit [Amazon ECS Container Agent Versions](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/container_agent_versions.html).


Can you ask @nrdlngr to take a look at this?

Ping'd him.

Would it be possible to rephrase a bit?

"Please note: starting from Agent 1.20.0, the minimum supported Docker version is 1.9.0."

Also: can you break lines at 80 or 120 characters? This is a super long single line in a text file now.

richardpen · 2018-07-30T17:50:13Z

agent/app/agent.go

+	supportedVersions := agent.dockerClient.SupportedVersions()
+	if len(supportedVersions) == 0 {
+		seelog.Critical("Could not get supported docker versions.")
+		return exitcodes.ExitError, false


With the new api version change(minimum is 1.21), you probably only need to return exitcodes.ExitTerminal here, and then you can remove the rest of the code.

Undid this change, CP backend might still need >= logic on the api versions. Keeping exitError here. Updated getDefaultVersion to return 1.21 however.

adnxn

lgtm, thanks for making those changes

sharanyad · 2018-07-30T19:24:40Z

CHANGELOG.md

+* Feature - Add support for Docker volume plugins
+* Enhancement - Replace the empty container with Docker local volume
+* Enhancement - Deprecate support for Docker version older than 1.9.0
+


These changelog items need PR links.

sharanyad · 2018-07-30T19:31:41Z

CHANGELOG.md

+## 1.20.0-dev
+* Feature - Add support for Docker volume plugins
+* Enhancement - Replace the empty container with Docker local volume
+* Enhancement - Deprecate support for Docker version older than 1.9.0 [1477](https://github.com/aws/amazon-ecs-agent/pull/1477)


nit: [#1477]

…ing support for any previous versions updated default docker api version to 1.21.

samuelkarp · 2018-07-31T21:17:28Z

README.md

@@ -14,6 +14,8 @@ The Amazon ECS Container Agent is a component of Amazon Elastic Container Servic
 The best source of information on running this software is the
 [Amazon ECS documentation](http://docs.aws.amazon.com/AmazonECS/latest/developerguide/ECS_agent.html).

+Please note that from Agent version 1.20.0, Minimum required Docker version is 1.9.0, corresponding to Docker API version 1.21. For more information, please visit [Amazon ECS Container Agent Versions](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/container_agent_versions.html).


Would it be possible to rephrase a bit?

"Please note: starting from Agent 1.20.0, the minimum supported Docker version is 1.9.0."

Also: can you break lines at 80 or 120 characters? This is a super long single line in a text file now.

samuelkarp · 2018-07-31T21:18:19Z

agent/app/agent.go

+	}
+
+	// api 1.21 is not supported, docker version is older than 1.9.0
+	seelog.Criticalf("Required minimum docker API verion %s is not supported",


"Docker API"

samuelkarp · 2018-07-31T21:20:04Z

agent/app/agent.go

+	// if api version 1.21 is supported, it means docker version is at least 1.9.0
+	for _, version := range supportedVersions {
+		if version == dockerclient.Version_1_21 {
+			return -1, true


What's -1? Can you use one of the exitcodes constants?

yhlee-aws added this to the 1.20.0 milestone Jul 27, 2018

yhlee-aws requested review from adnxn, aaithal and richardpen July 27, 2018 00:05

yhlee-aws force-pushed the enforce_min_docker_ver branch 3 times, most recently from 038f9b0 to 385598c Compare July 27, 2018 04:38

petderek reviewed Jul 27, 2018

View reviewed changes

sharanyad reviewed Jul 27, 2018

View reviewed changes

petderek reviewed Jul 27, 2018

View reviewed changes

aaithal reviewed Jul 27, 2018

View reviewed changes

yhlee-aws force-pushed the enforce_min_docker_ver branch 2 times, most recently from 5662f28 to de4f86c Compare July 28, 2018 05:53

petderek approved these changes Jul 28, 2018

View reviewed changes

yhlee-aws force-pushed the enforce_min_docker_ver branch from 2471cfe to 4b689ff Compare July 28, 2018 17:57

adnxn reviewed Jul 30, 2018

View reviewed changes

sharanyad reviewed Jul 30, 2018

View reviewed changes

richardpen reviewed Jul 30, 2018

View reviewed changes

yhlee-aws force-pushed the enforce_min_docker_ver branch 2 times, most recently from 9b02e28 to b1726c7 Compare July 30, 2018 17:36

richardpen reviewed Jul 30, 2018

View reviewed changes

yhlee-aws force-pushed the enforce_min_docker_ver branch 2 times, most recently from 3327bb6 to 3db6809 Compare July 30, 2018 18:55

adnxn approved these changes Jul 30, 2018

View reviewed changes

sharanyad reviewed Jul 30, 2018

View reviewed changes

yhlee-aws force-pushed the enforce_min_docker_ver branch from 68fedda to f25f0e7 Compare July 30, 2018 19:29

sharanyad approved these changes Jul 30, 2018

View reviewed changes

richardpen approved these changes Jul 30, 2018

View reviewed changes

Enforcing minimum docker version 1.9.0 via API version 1.21, deprecat…

c75c475

…ing support for any previous versions updated default docker api version to 1.21.

yhlee-aws force-pushed the enforce_min_docker_ver branch from 0a79394 to c75c475 Compare July 30, 2018 19:52

yhlee-aws merged commit fe1d543 into aws:dev Jul 30, 2018

yhlee-aws deleted the enforce_min_docker_ver branch July 30, 2018 20:46

samuelkarp reviewed Jul 31, 2018

View reviewed changes

Enforce minimum docker version support #1477

Enforce minimum docker version support #1477

Conversation

yhlee-aws commented Jul 27, 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

petderek 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

adnxn 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

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

petderek Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

yhlee-aws Jul 30, 2018 • edited Loading

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

yhlee-aws Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

adnxn 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhlee-aws commented Jul 27, 2018 •

edited

Loading

petderek Jul 30, 2018 •

edited

Loading

yhlee-aws Jul 30, 2018 •

edited

Loading

yhlee-aws Jul 30, 2018 •

edited

Loading