fix issue1043 #1217

aaithal · 2018-01-25T21:28:57Z

Summary

Implementation details

Testing

Description for the changelog

Licensing

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

richardpen · 2018-01-25T22:38:04Z

agent/engine/task_manager.go

+			// a stop
+			seelog.Warnf("Managed task [%s]: Forcing container [%s] to stop",
+				mtask.Arn, container.Name)
+			go mtask.engine.transitionContainer(mtask.Task, container, api.ContainerStopped)


Do we need to check the KnownStatus of the container here? In case it has been stopped or already running, eg:

StartContainer -> inspect timeout -> stop the container StartEvent -> inspect timeout -> stop the container

yhlee-aws · 2018-01-26T17:43:44Z

agent/engine/task_manager.go

@@ -267,6 +274,13 @@ func (mtask *managedTask) waitSteady() {
 	}
 }

+// steadyState returns if the task is in a steady state. Steady state is when task's desired
+// and known status are both RUNNING
+func (mtask *managedTask) steadyState() bool {


nit, and possibly my java-brain speak here: can this be named "isSteadyState" or "isInSteadyState"?

Hmm, don't have strong opinions here. if task.steadyState() conveys the 'is task in steady state?' meaning in my brain. Golang prefers less verbosity in names (though we don't always stick to that convention). If you feel strongly about it, I can change it.

Fixed an issue where ECS agent could lose track of running containers if there were timeouts strating the container or inspecting the started container.

This should hopefully fix failure seen in https://ci.appveyor.com/project/AmazonECS/amazon-ecs-agent/build/1620 This change ensures that the message is sent on the stopDone channel before marking the waitgroup as done

richardpen

LGTM, one minor comment, also please make sure the header was updated to 2018 for the modified file.

richardpen · 2018-01-26T19:50:45Z

CHANGELOG.md

@@ -5,6 +5,8 @@
 * Feature - Support Docker health check.
 * Bug - Fixed a bug where `-version` fails due to its dependency on docker client. [#1118](https://github.com/aws/amazon-ecs-agent/pull/1118)
 * Bug - Persist container exit code in agent state file. [#1125](https://github.com/aws/amazon-ecs-agent/pull/1125)
+* Bug - Persist container exit code in agent state file [#1125](https://github.com/aws/amazon-ecs-agent/pull/1125)


We should be consistent in the changelog, whether the . is needed at the end.

I think convention has been to not add .'s at the end. I'll modify that.

adnxn · 2018-01-26T21:04:57Z

agent/engine/docker_client.go

@@ -518,6 +518,7 @@ func (dg *dockerGoClient) createContainer(ctx context.Context,
 	if err != nil {
 		return DockerContainerMetadata{Error: CannotCreateContainerError{err}}
 	}
+


nit: blank line

adnxn · 2018-01-26T21:07:02Z

agent/engine/docker_client.go

+		switch event.Status {
+		case "create":
+			status = api.ContainerCreated
+			// TODO no need to inspect containers here


im probably missing context, but what do you mean by "no need to inspect containers here"?

There's no need to inspect containers after they are created once we adopt Docker's volume APIs. Today, that's the only information we need from the inspect API. Once we start injecting that ourselves, there's no need to inspect containers on Create anymore. This will save us a lot of inspect calls in the future. I can update the comment to have this context

adnxn · 2018-01-26T21:10:09Z

agent/engine/docker_client.go

+		default:
+			// Because docker emits new events even when you use an old event api
+			// version, it's not that big a deal
+			seelog.Debugf("DockerGoClient: unknown status event from docker: %s", event.Status)


nit: should we a tie an event.ID to this debug line?

Probably. I'll add more details here.

adnxn · 2018-01-26T21:27:07Z

agent/engine/task_manager.go

+		return
+	}
+	if containerKnownStatus != api.ContainerStopped {
+		// Container's known status is STOPPED. Nothing to do here.


did you mean to put container's known status is NOT stopped here?

yes. thanks for catching that.

adnxn · 2018-01-26T21:31:27Z

agent/engine/task_manager.go

-	taskKnownStatus := mtask.GetKnownStatus()
-	return taskKnownStatus == api.TaskRunning && taskKnownStatus >= mtask.GetDesiredStatus()
+// handleContainerStoppedTransitionError handles an error when transitioning a container to
+// STOPPED. It returns a boolean indicating whethere the taks can continue with updating its


typo: whether, tasks

sharanyad · 2018-01-26T00:53:49Z

agent/engine/task_manager.go

+	for !mtask.waitEvent(othersStopped) {
+		if mtask.GetDesiredStatus().Terminal() {
+			// If we end up here, that means we received a start then stop for this
+			// task before a task that was expected to stop before it could


this comment is not readable. Can you please refactor?

:) it's a right of initiation for anyone who's relatively new to this code base to read this and wonder what in the world is the meaning of this sentence. Since I've already merged this PR, we can pick this next time we touch this file

Maybe this will help anyone who stumbles here: We want to process stop events (a task changes to desired=STOPPED) prior to start events (desired=RUNNING) so that any resources that are consumed by the desired=STOPPED task can be freed for the desired=RUNNING task. This logic helps to enforce the ordering.

sharanyad · 2018-01-26T00:55:14Z

agent/engine/task_manager.go

@@ -418,7 +445,7 @@ func (mtask *managedTask) emitTaskEvent(task *api.Task, reason string) {
 func (mtask *managedTask) emitContainerEvent(task *api.Task, cont *api.Container, reason string) {
 	event, err := api.NewContainerStateChangeEvent(task, cont, reason)
 	if err != nil {
-		seelog.Infof("Managed task [%s]: unable to create state change event for container [%s]: %v",
+		seelog.Debugf("Managed task [%s]: unable to create state change event for container [%s]: %v",


Why not Infof ?

valid internal state change change events like NONE and CREATED end up resulting in an error here. Hence, reverted back to Debug

sharanyad · 2018-01-26T22:37:47Z

agent/engine/task_manager.go

+		container.SetKnownStatus(currentKnownStatus)
+		container.SetDesiredStatus(api.ContainerStopped)
+		errorName := event.Error.ErrorName()
+		if errorName == dockerTimeoutErrorName || errorName == cannotInspectContainerErrorName {


Are there other inspect error types? Is the error name check necessary?

We can either inspect types or use error names. Error names are easier as there's no type-casting involved. Instead of casting a wide net here, I'd prefer to selectively add error types as that impact to limit any potential unwanted impact of this

aaithal force-pushed the issue1043 branch from 54244e0 to e0db582 Compare January 25, 2018 22:40

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

changelog for aws#1217

e0db582

richardpen reviewed Jan 25, 2018

View reviewed changes

aaithal changed the title ~~[wip] fix issue1043~~ fix issue1043 Jan 26, 2018

aaithal added this to the 1.17.0 milestone Jan 26, 2018

aaithal requested review from adnxn, samuelkarp, sharanyad, richardpen and yhlee-aws January 26, 2018 00:33

aaithal mentioned this pull request Jan 26, 2018

Fix container metadata information missing during agent restart #1183

Merged

12 tasks

yhlee-aws reviewed Jan 26, 2018

View reviewed changes

aaithal added 2 commits January 26, 2018 11:42

fix issue aws#1043

75e172a

Fixed an issue where ECS agent could lose track of running containers if there were timeouts strating the container or inspecting the started container.

changelog for aws#1217

9b34e25

aaithal force-pushed the issue1043 branch from e0db582 to 9b34e25 Compare January 26, 2018 19:43

aaithal added 2 commits January 26, 2018 12:04

TaskWaitForHostResourceOnRestart: reorder stop wg

ceeedee

This should hopefully fix failure seen in https://ci.appveyor.com/project/AmazonECS/amazon-ecs-agent/build/1620 This change ensures that the message is sent on the stopDone channel before marking the waitgroup as done

appveyor: use 1.9.3 golang

4810e7c

richardpen approved these changes Jan 26, 2018

View reviewed changes

CHANGELOG: remove periods for unreleased entries

a6701b7

adnxn reviewed Jan 26, 2018

View reviewed changes

update copyright headers for aws#1217

2b1f36d

aaithal mentioned this pull request Jan 26, 2018

Adding container health information into task metadata #1218

Merged

8 tasks

aaithal merged commit 9f40dcf into aws:dev Jan 26, 2018

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

changelog for #1217

53bbf5c

sharanyad reviewed Jan 26, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix issue1043 #1217

fix issue1043 #1217

aaithal commented Jan 25, 2018 •

edited

Loading

richardpen Jan 25, 2018

yhlee-aws Jan 26, 2018

aaithal Jan 26, 2018

richardpen left a comment

richardpen Jan 26, 2018

aaithal Jan 26, 2018

adnxn Jan 26, 2018

adnxn Jan 26, 2018

aaithal Jan 26, 2018 •

edited

Loading

adnxn Jan 26, 2018

aaithal Jan 26, 2018

adnxn Jan 26, 2018

aaithal Jan 26, 2018

adnxn Jan 26, 2018

sharanyad Jan 26, 2018

aaithal Jan 26, 2018

samuelkarp Jan 27, 2018

sharanyad Jan 26, 2018

aaithal Jan 26, 2018

sharanyad Jan 26, 2018

aaithal Jan 26, 2018

fix issue1043 #1217

fix issue1043 #1217

Conversation

aaithal commented Jan 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

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

Choose a reason for hiding this comment

aaithal Jan 26, 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

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

aaithal commented Jan 25, 2018 •

edited

Loading

aaithal Jan 26, 2018 •

edited

Loading