Fix the issue where agent could create the same container twice #939

richardpen · 2017-08-23T23:33:50Z

Summary

Fix the issue if agent was restarted before the docker create API returned, the agent could create duplicate containers for the task.

Implementation details

Originally the agent stores the generated container name in memory before actually calling CreateContainer API, see here. And this information was only stored in the taskToID struct, due to lacking of docker ID here. But the taskToID struct wasn't stored in the state file by statemanager, so when the agent restart it will lose this information and then it will try to create the container with another name again.

This PR make sure the the generated container name can be restored from agent state file if agent restarted before the CreateContainer API returned. Thus ensure the container won't be created twice. Also the agent would detect the container if it was created during the period of restarting, see here.

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
Manually verified the state when agent was restarted at the create.

New tests cover the changes:
yes

Description for the changelog

Bug - Fix an issue where a container could be created twice.

Licensing

This contribution is under the terms of the Apache 2.0 License:
yes

samuelkarp · 2017-08-24T17:13:53Z

agent/engine/docker_task_engine.go

@@ -236,6 +236,7 @@ func (engine *DockerTaskEngine) synchronizeState() {
 					log.Warn("Could not find matching container for expected", "name", cont.DockerName)
 				} else {
 					cont.DockerID = describedCont.ID
+					cont.Container.SetKnownStatus(api.ContainerCreated)


I think we need to look here and set the state based on what comes back. It could be created, running, or stopped.

If it falls into this condition dockerID=="", the container status shouldn't be > created. Otherwise the dockerID field won't be empty. But have a check here will definitely add some confidence.

samuelkarp · 2017-08-24T17:15:07Z

agent/engine/docker_task_engine.go

@@ -562,9 +563,17 @@ func (engine *DockerTaskEngine) createContainer(task *api.Task, container *api.C
 	// we have to do this in create, not start, because docker no longer handles


Can you move this comment about HostConfig above line 579 instead of here? It's less clear what the comment is applying to now.

aaithal

looks good. please address @samuelkarp's comment.

aaithal · 2017-08-24T23:32:36Z

agent/engine/docker_task_engine.go

+		for _, v := range containerMap {
+			if v.Container.Name == container.Name {
+				dockerContainerName = v.DockerName
+			}


you should/can add a break here as well.

aaithal · 2017-08-31T16:33:17Z

agent/engine/dockerstate/docker_task_engine_state.go

@@ -250,18 +250,25 @@ func (state *DockerTaskEngineState) AddContainer(container *api.DockerContainer,
 	}

 	if container.DockerID != "" {
+		// Update the container id to the state


Is there a unit test covering the if and else if code paths?

jhaynes · 2017-08-31T21:56:51Z

Can you update the CHANGELOG.md also as part of this PR?

aaithal · 2017-09-01T15:00:50Z

CHANGELOG.md

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

+##


Should we leave this as blank? We could consider adding a tentative version string with a * annotation to indicate that this is not yet released (Example: 1.14.5* or 1.15.0*). Or, we can add a placeholder release tag (Example: UNRELEASED). What do you think?

cc @jhaynes

I'd go with a placeholder for now, and we'll fill in the actual version as part of the release process. "UNRELEASED", "IN-PROGRESS", or "PENDING" would be my suggestions.

I vote "UNRELEASED"

aaithal · 2017-09-01T15:02:26Z

agent/engine/dockerstate/dockerstate_test.go

+
+// TestAddContainer tests first add container with docker name and
+// then add the container with dockerID
+func TestAddContainer(t *testing.T) {


Can you rename the test to be more descriptive? You can use what you have written in the comments as well. Example:TestAddContainerNameAndID.

aaithal

Please address the minor comments I made before merging this.

aaithal · 2017-09-07T23:05:51Z

CHANGELOG.md

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

+## UNRELEASED
+* Bug - Fix a race condition where a container can be created twice when agent restart. [#939](https://github.com/aws/amazon-ecs-agent/pull/939)


nit: restarts

aaithal · 2017-09-07T23:06:37Z

agent/engine/docker_task_engine.go

+		// we die before 'createContainer' returns because we can inspect by
+		// name
+		engine.state.AddContainer(&api.DockerContainer{DockerName: dockerContainerName, Container: container}, task)
+		seelog.Infof("Created container name mapping for task %s - %s -> %s", task, container, dockerContainerName)


Can you change task to task.Arn? We don't need the whole task to be logged here.

Save the generated docker container name and when agent restart restore the agent state, when agent try to create the container it will reuse the name so that no duplicate container will be created.

richardpen requested review from samuelkarp, jahkeup, aaithal and petderek August 23, 2017 23:34

samuelkarp reviewed Aug 24, 2017

View reviewed changes

aaithal reviewed Aug 24, 2017

View reviewed changes

richardpen force-pushed the fix-duplicate-create branch 4 times, most recently from 7bb0ffa to 1bec8f9 Compare August 29, 2017 20:50

aaithal reviewed Aug 31, 2017

View reviewed changes

richardpen force-pushed the fix-duplicate-create branch from 1bec8f9 to bd3883a Compare August 31, 2017 23:26

aaithal reviewed Sep 1, 2017

View reviewed changes

richardpen force-pushed the fix-duplicate-create branch 2 times, most recently from 0a62385 to b18e674 Compare September 1, 2017 17:52

aaithal approved these changes Sep 7, 2017

View reviewed changes

engine: Make the agent reuse the generated container name on restart

49c36c7

Save the generated docker container name and when agent restart restore the agent state, when agent try to create the container it will reuse the name so that no duplicate container will be created.

richardpen force-pushed the fix-duplicate-create branch from b18e674 to 49c36c7 Compare September 11, 2017 18:41

samuelkarp approved these changes Sep 13, 2017

View reviewed changes

richardpen merged commit 49c36c7 into aws:dev Sep 13, 2017

jhaynes mentioned this pull request Sep 30, 2017

V1.14.5 release #999

Merged

richardpen deleted the fix-duplicate-create branch November 21, 2017 01:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the issue where agent could create the same container twice #939

Fix the issue where agent could create the same container twice #939

richardpen commented Aug 23, 2017 •

edited

Loading

samuelkarp Aug 24, 2017

richardpen Aug 28, 2017

samuelkarp Aug 24, 2017

aaithal left a comment

aaithal Aug 24, 2017

aaithal Aug 31, 2017

richardpen Aug 31, 2017

jhaynes commented Aug 31, 2017

aaithal Sep 1, 2017

nmeyerhans Sep 1, 2017

aaithal Sep 1, 2017

aaithal Sep 1, 2017

aaithal left a comment

aaithal Sep 7, 2017

aaithal Sep 7, 2017

		@@ -562,9 +563,17 @@ func (engine DockerTaskEngine) createContainer(task api.Task, container *api.C
		// we have to do this in create, not start, because docker no longer handles

Fix the issue where agent could create the same container twice #939

Fix the issue where agent could create the same container twice #939

Conversation

richardpen commented Aug 23, 2017 • 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

aaithal 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

jhaynes commented Aug 31, 2017

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardpen commented Aug 23, 2017 •

edited

Loading