batch container state change events #867

adnxn · 2017-06-27T19:41:49Z

Summary

This change updates the model for the ecs client and includes the corresponding code changes to SubmitTaskStateChange in the api package. Secondly we've added the batching logic to eventhandler.

Implementation details

Two main points of interest:
In agent/eventhandler/task_handler.go, as ContainerStateChange events are propagated up, they are collected and bucketed by task arn. Then on a task state transition, we attach the the collection of ContainerStateChange events to the TaskStateChange payload that is sent to the the backend.

In agent/api/ecsclient/client.go, we made the changes required to reflect the model changes and also added logic to attach the container state change message payloads to task state change message.

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

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

richardpen · 2017-06-27T21:21:31Z

agent/api/ecsclient/client.go

+			trimmed := change.Reason[0:ecsMaxReasonLength]
+			statechange.Reason = &trimmed
+		} else {
+			statechange.Reason = &change.Reason


Please use aws.String instead.

richardpen · 2017-06-27T21:25:36Z

agent/api/ecsclient/client.go

+		}
+	}
+	stat := change.Status.String()
+	if stat == "DEAD" {


Do we really need this?

i'm not sure tbh. the check was in the original SubmitContainerStateChange code. although api.ContainerStatus doesnt have a "DEAD" state, so we may be good to remove this. @aaithal any ideas?

richardpen · 2017-06-27T21:27:36Z

agent/api/ecsclient/client.go

+		stat = "STOPPED"
+	}
+
+	if stat != "STOPPED" && stat != "RUNNING" {


you can compare directly with api.ContainerStopped and api.ContainerRunning without converting the status to string.

richardpen · 2017-06-27T21:28:23Z

agent/api/ecsclient/client.go

+		log.Info("Not submitting not supported upstream container state", "state", stat)
+		return nil
+	}
+	statechange.Status = &stat


same as above, please use aws.String instead.

richardpen · 2017-06-27T21:32:06Z

agent/api/ecsclient/client.go

+	statechange.Status = &stat
+
+	if change.ExitCode != nil {
+		exitCode := int64(*change.ExitCode)


same as above, you can use aws.IntValue instead.

richardpen · 2017-06-27T21:34:07Z

agent/api/ecsclient/client.go

+		bindIP := binding.BindIP
+		protocol := binding.Protocol.String()
+		networkBindings[i] = &ecs.NetworkBinding{
+			BindIP:        &bindIP,


same as above, use aws.String or aws.Int64 instead.

richardpen · 2017-06-27T21:41:11Z

agent/eventhandler/task_handler.go

@@ -47,13 +47,16 @@ type TaskHandler struct {
 	tasksToEvents map[string]*eventList
 	// tasksToEventsLock for locking the map
 	tasksToEventsLock sync.RWMutex
+
+	batchMap map[string][]api.ContainerStateChange


can you add a comment here?

richardpen · 2017-06-27T21:42:39Z

agent/eventhandler/task_handler.go

 		return nil

 	default:
 		return errors.New("eventhandler: unable to determine event type from state change event")
 	}
 }

+// batchContainerEvent collects container state change events for a given task arn
+func (handler *TaskHandler) batchContainerEvent(event api.ContainerStateChange) {
+	handler.batchMap[event.TaskArn] = append(handler.batchMap[event.TaskArn], event)


This may need a lock?

i'm not sure if this needs a lock. the batchMap data is not shared across goroutines.

I don't believe we guarantee that handler.AddStateChangeEvent is not called across goroutines.

hrm. that's true. i'll make this change.

richardpen · 2017-06-27T21:43:40Z

agent/eventhandler/task_handler.go

+// flushBatch attaches the task arn's container events to TaskStateChange event that
+// is being submittied to the backend
+func (handler *TaskHandler) flushBatch(event *api.TaskStateChange) {
+	event.Containers = append(event.Containers, handler.batchMap[event.TaskArn]...)


same as above, this needs a lock.

same as above.

samuelkarp · 2017-06-28T21:46:20Z

agent/api/ecsclient/client.go

@@ -279,6 +279,51 @@ func (client *APIECSClient) getCustomAttributes() []*ecs.Attribute {
 	return attributes
 }

+func (client *APIECSClient) buildContainerStateChangePayload(change api.ContainerStateChange) *ecs.ContainerStateChange {


nit: Can you move this function below SubitTaskStateChange? As a general rule, I try to organize the code such that caller is above callee.

sounds good.

samuelkarp · 2017-06-29T00:08:16Z

agent/eventhandler/task_handler.go

 		return nil

 	default:
 		return errors.New("eventhandler: unable to determine event type from state change event")
 	}
 }

+// batchContainerEvent collects container state change events for a given task arn
+func (handler *TaskHandler) batchContainerEvent(event api.ContainerStateChange) {
+	handler.batchMap[event.TaskArn] = append(handler.batchMap[event.TaskArn], event)


I don't believe we guarantee that handler.AddStateChangeEvent is not called across goroutines.

aaithal

mostly looks good. I have a bunch of minor comments.

aaithal · 2017-06-29T22:45:17Z

agent/api/ecsclient/client.go

+	status := change.Status
+
+	if status != api.ContainerStopped && status != api.ContainerRunning {
+		log.Info("Not submitting not supported upstream container state", "state", status)


Can you use seelog instead here?

aaithal · 2017-06-29T23:00:16Z

agent/api/ecsclient/client.go

+	statechange.Status = aws.String(status.String())
+
+	if change.ExitCode != nil {
+		exitCode := int64(*change.ExitCode)


please use aws.Int64Val instead

aaithal · 2017-06-29T23:01:49Z

agent/ecs_client/model/ecs/service.go

@@ -70,6 +70,9 @@ func New(p client.ConfigProvider, cfgs ...*aws.Config) *ECS {

 // newClient creates, initializes and returns a new service client instance.
 func newClient(cfg aws.Config, handlers request.Handlers, endpoint, signingRegion, signingName string) *ECS {
+	if len(signingName) == 0 {


Let's stick to signingName == "" since this is a string type.

what about the nil case?

aaithal · 2017-06-29T23:03:09Z

agent/eventhandler/task_handler.go

@@ -47,13 +47,18 @@ type TaskHandler struct {
 	tasksToEvents map[string]*eventList
 	// tasksToEventsLock for locking the map
 	tasksToEventsLock sync.RWMutex
+	// batchMap is used to collect container events
+	// between task transitions
+	batchMap     map[string][]api.ContainerStateChange


Can you rename this as tasksToContainerStates ? If you do that, you can rename the lock as well

aaithal · 2017-06-29T23:04:23Z

agent/eventhandler/task_handler.go

 		return nil

 	default:
 		return errors.New("eventhandler: unable to determine event type from state change event")
 	}
 }

+// batchContainerEvent collects container state change events for a given task arn
+func (handler *TaskHandler) batchContainerEvent(event api.ContainerStateChange) {
+	handler.batchMapLock.Lock()


please use defer for unlocking:

handler.batchMapLock.Lock() defer handler.batchMapLock.Unlock() handler.batchMap[event.TaskArn] = append(handler.batchMap[event.TaskArn], event)

aaithal · 2017-06-29T23:04:41Z

agent/eventhandler/task_handler.go

+// flushBatch attaches the task arn's container events to TaskStateChange event that
+// is being submittied to the backend
+func (handler *TaskHandler) flushBatch(event *api.TaskStateChange) {
+	handler.batchMapLock.Lock()


Same as above, use defer for unlocking

aaithal · 2017-06-29T23:05:36Z

agent/api/ecsclient/client.go

 	if err != nil {
 		log.Warn("Could not submit a task state change", "err", err)
 		return err
 	}
+
+	// WIP LOG payload


When are you getting rid of this?

petderek · 2017-06-29T23:17:41Z

agent/api/ecsclient/client.go

+		ContainerName: aws.String(change.ContainerName),
+	}
+
+	if change.Reason != "" {


What about the nil case?

so change api.ContainerStateChange is a struct value and can't be nil. at worst it may have its fields zeroed if no value is specified.

petderek · 2017-06-29T23:20:23Z

agent/ecs_client/model/ecs/service.go

@@ -70,6 +70,9 @@ func New(p client.ConfigProvider, cfgs ...*aws.Config) *ECS {

 // newClient creates, initializes and returns a new service client instance.
 func newClient(cfg aws.Config, handlers request.Handlers, endpoint, signingRegion, signingName string) *ECS {
+	if len(signingName) == 0 {


what about the nil case?

This change updates the model for the ecs client and includes the corresponding code changes to SubmitTaskStateChange. Secondly we've added the batching logic to eventhandler, which collects container state change events as the generated and then attaches the collection of events to the SubmitTaskStateChange payload as dictated but the model changes.

This change updates the eventhandler tests to reflect the batching changes.

This change updates existing test coverage. We also add a lock around the map that aggregates container state change events. There is also a minor fix to a test failure that was causing a nil pointer panic in the agent/stats package.

This commit includes several minor changes to address reviewer comments

aaithal · 2017-07-26T22:49:55Z

agent/api/ecsclient/client.go

+	status := change.Status
+
+	if status != api.ContainerStopped && status != api.ContainerRunning {
+		seelog.Info("Not submitting not supported upstream container state", "state", status)


Should this be logged as a warning?

aaithal · 2017-07-26T22:52:47Z

agent/eventhandler/task_handler.go

+	// tasksToContainerStates is used to collect container events
+	// between task transitions
+	tasksToContainerStates     map[string][]api.ContainerStateChange
+	tasksToContainerStatesLock sync.RWMutex


Can you switch to using a single lock for this struct? Want to avoid the repeat of "deadly embrace" because of multiple locks

will do. and yup 😅

richardpen · 2017-07-27T21:28:09Z

agent/api/ecsclient/client.go

+		containerEvents[i] = client.buildContainerStateChangePayload(containerEvent)
+	}
+
+	req.Containers = containerEvents


Since we combined several event into one, it may be good to log the actual events here, so that we can know which statechange was actually been submitted.

agreed. i've added info level logging in agent/eventhandler/task_handler.go to capture this info. we log other information about which events are submitted there as well.

aaithal · 2017-07-28T17:51:25Z

agent/api/ecsclient/client.go

+	status := change.Status
+
+	if status != api.ContainerStopped && status != api.ContainerRunning {
+		seelog.Warn("Not submitting unsupported upstream container state", "state", status)


please use seelog.Warnf here.

out of curiosity - why one over the other? the existing code seems to use seelog.Warn

I prefer the formatted string kind because it gives you control over the output format and not having to deal with whatever golang thinks the stringified version of an object should look like.

aaithal · 2017-07-28T17:52:09Z

agent/eventhandler/handler_test.go

-	client.EXPECT().SubmitContainerStateChange(sortaRedundantCont.(api.ContainerStateChange)).Do(func(interface{}) { wait.Done() })
-	client.EXPECT().SubmitTaskStateChange(notReplacedTask.(api.TaskStateChange)).Do(func(interface{}) { wait.Done() })
-	client.EXPECT().SubmitTaskStateChange(sortaRedundantTask.(api.TaskStateChange)).Do(func(interface{}) { wait.Done() })
+	time.Sleep(1 * time.Millisecond)


Why the sleep here?

i was using it to ensure ordering. ill change it to use waitgroups.

aaithal · 2017-07-31T15:35:30Z

agent/api/ecsclient/client.go

+	status := change.Status
+
+	if status != api.ContainerStopped && status != api.ContainerRunning {
+		seelog.Warnf("Not submitting unsupported upstream container state: %v", status)


Can you also log details of the task here? It's not much use when debugging if that's not logged. Also, please use ".. : %s", status.String()

fixed formatting. also - we are capturing details about both task and container events a layer above in agent/eventhandler/task_handler.go as they are passed to the client. did you mean we should capture debug logs here as well?

yeah, when you're printing a warning/error, it's useful to have as much info as possible about what led to that in the message itself (because earlier lines might get lost because of other async routines getting in the way)

aaithal · 2017-07-31T15:36:28Z

agent/ecs_client/model/ecs/service.go

@@ -70,6 +70,9 @@ func New(p client.ConfigProvider, cfgs ...*aws.Config) *ECS {

 // newClient creates, initializes and returns a new service client instance.
 func newClient(cfg aws.Config, handlers request.Handlers, endpoint, signingRegion, signingName string) *ECS {
+	if signingName == "" {
+		signingName = "ecs"


This should be a const (or, you can reuse ServiceName)

aaithal · 2017-07-31T15:39:15Z

agent/eventhandler/task_handler.go

+	handler.taskHandlerLock.Lock()
+	defer handler.taskHandlerLock.Unlock()
+
+	seelog.Info("TaskHandler, batching container event :", event)


Please use Infof here.

This commit also contains changes to log formatting to address code review comments.

richardpen

LGTM, after this you should work on fix the leak in #798.

This implements the changes required to batch container state change events and include them with the task state change payload

This reverts commit 8083c13, reversing changes made to af6e51e.

adnxn requested review from aaithal, petderek and richardpen June 27, 2017 19:41

richardpen reviewed Jun 27, 2017

View reviewed changes

adnxn force-pushed the batch-state-change-events branch from 7dade26 to 9750ac7 Compare June 28, 2017 20:53

samuelkarp reviewed Jun 29, 2017

View reviewed changes

aaithal reviewed Jun 29, 2017

View reviewed changes

petderek reviewed Jun 30, 2017

View reviewed changes

adnxn added 3 commits July 24, 2017 14:37

eventhandler: updated tests

f0bc74e

This change updates the eventhandler tests to reflect the batching changes.

eventhandler: updated tests, batch locks

b4579f1

This change updates existing test coverage. We also add a lock around the map that aggregates container state change events. There is also a minor fix to a test failure that was causing a nil pointer panic in the agent/stats package.

adnxn force-pushed the batch-state-change-events branch 3 times, most recently from a474c3b to 900e02c Compare July 25, 2017 20:03

eventhandler: naming changes

19b2238

This commit includes several minor changes to address reviewer comments

adnxn force-pushed the batch-state-change-events branch from 900e02c to 19b2238 Compare July 25, 2017 20:43

adnxn changed the title ~~batch container state change events - WIP~~ batch container state change events Jul 26, 2017

aaithal reviewed Jul 26, 2017

View reviewed changes

richardpen reviewed Jul 27, 2017

View reviewed changes

adnxn force-pushed the batch-state-change-events branch from d8d0646 to 6a096a4 Compare July 28, 2017 14:11

eventhandler: consolidate TaskHandler locks

26478c1

adnxn force-pushed the batch-state-change-events branch from 6a096a4 to 26478c1 Compare July 28, 2017 14:27

aaithal requested changes Jul 28, 2017

View reviewed changes

adnxn force-pushed the batch-state-change-events branch from 3fe6f58 to 38d5a99 Compare July 28, 2017 22:46

aaithal reviewed Jul 31, 2017

View reviewed changes

adnxn force-pushed the batch-state-change-events branch from 38d5a99 to a41ff4f Compare August 3, 2017 16:51

eventhandler: update test to use waitgroup

211d4ba

This commit also contains changes to log formatting to address code review comments.

adnxn force-pushed the batch-state-change-events branch from a41ff4f to 211d4ba Compare August 3, 2017 17:07

aaithal approved these changes Aug 7, 2017

View reviewed changes

richardpen approved these changes Aug 9, 2017

View reviewed changes

adnxn merged commit 211d4ba into aws:dev Aug 9, 2017

adnxn added a commit that referenced this pull request Aug 9, 2017

Merge pull request #867 from adnxn/batch-state-change-events

8083c13

This implements the changes required to batch container state change events and include them with the task state change payload

jahkeup added a commit to jahkeup/amazon-ecs-agent that referenced this pull request Aug 11, 2017

Revert "Merge pull request aws#867 from adnxn/batch-state-change-events"

1a7cb0b

This reverts commit 8083c13, reversing changes made to af6e51e.

jhaynes mentioned this pull request Aug 22, 2017

V1.14.4 release #936

Merged

samuelkarp added this to the 1.14.4 milestone Aug 22, 2017

adnxn deleted the batch-state-change-events branch March 14, 2018 17:46

batch container state change events #867

batch container state change events #867

Conversation

adnxn commented Jun 27, 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

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

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

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

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

adnxn commented Jun 27, 2017 •

edited

Loading