engine,dockerclient: per container start/stop timeouts #1849

adnxn · 2019-02-13T21:34:28Z

Summary

start timeout is governed by the agent and is the context timeout for
the StartContainer api call
stop timeout is the parameter passed to StopContainer and is time the
docker daemon waits after a StopContainer call to issue a SIGKILL to the
container

Implementation details

Testing

New tests cover the changes: yes
unit test:

TestPerContainerTimeouts
TestPostUnmarshalPerContainerTimeouts

integ test:

TestPerContainerStopTimeout

Description for the changelog

Feature - Support for granular start and stop timeouts per container

Licensing

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

adnxn · 2019-02-15T20:01:05Z

will squash to single commit before unWIP

yhlee-aws · 2019-02-15T22:20:52Z

agent/dockerclient/dockerapi/docker_client.go

@@ -550,8 +552,8 @@ func (dg *dockerGoClient) createContainer(ctx context.Context,
 	return dg.containerMetadata(ctx, dockerContainer.ID)
 }

-func (dg *dockerGoClient) StartContainer(ctx context.Context, id string, timeout time.Duration) DockerContainerMetadata {
-	ctx, cancel := context.WithTimeout(ctx, timeout)
+func (dg *dockerGoClient) StartContainer(ctx context.Context, id string, ctxTimeout time.Duration) DockerContainerMetadata {


can you add similar comment for ctxTimeout here as well? is there valid range of values for this like it does for stop timeout?

no, this is just what we chose for the start container timeout. we have some sane defaults here #1321, but since we're allowing it to be configurable from the task def we don't have a range check at this layer. the service is enforcing a lower bound of 2s and no upper bound.

the docker daemon's start container api doesnt enforce timeouts.

yhlee-aws · 2019-02-15T22:28:59Z

agent/dockerclient/dockerapi/docker_client.go

 	defer cancel()
 	defer metrics.MetricsEngineGlobal.RecordDockerMetric("STOP_CONTAINER")()
 	// Buffered channel so in the case of timeout it takes one write, never gets
 	// read, and can still be GC'd
 	response := make(chan DockerContainerMetadata, 1)
-	go func() { response <- dg.stopContainer(ctx, dockerID) }()
+	go func() { response <- dg.stopContainer(ctx, dockerID, timeout) }()


should this be passing ctxTimeout? What is the difference between timeout and ctxTimeout?

For my understanding, why isn't startContainer handled the same with timeout?

What is the difference between timeout and ctxTimeout?

so i have some information in the comment above this code and also the PR description.

but also - ctxTimeout is for the timeout for the context the agent uses for the actual call to docker, where as timeout is a parameter for the api call to stop container call. the timeout parameter is how long the docker daemon will wait after a docker stop call to sigkill the container. we want our context timeout to be greater than the daemon's sigkill timeout.

adnxn

❗️ reminder: ~~need to bump state file version for container fields~~

* start timeout is governed by the agent and is the context timeout for the StartContainer api call * stop timeout is the parameter passed to StopContainer and is time the docker daemon waits after a StopContainer call to issue a SIGKILL to the container

adnxn · 2019-02-18T19:17:27Z

cherry-picked changes on to container-ordering-feature branch and resolved merge conflicts.

sharanyad · 2019-02-18T19:13:28Z

agent/acs/model/ecsacs/api.go

@@ -236,6 +236,10 @@ type Container struct {

 	Secrets []*Secret `locationName:"secrets" type:"list"`

+	StartTimeout *int64 `locationName:"startTimeout" type:"integer"`


Can we make sure ECS service treats it as int64 as well?

we're treating this field as we do with cpu/mem

sharanyad · 2019-02-18T19:22:23Z

agent/statemanager/state_manager.go

@@ -78,7 +78,10 @@ const (
 	//   a) Add 'Associations' field to 'api.task.task'
 	//   b) Add 'GPUIDs' field to 'apicontainer.Container'
 	//   c) Add 'NvidiaRuntime' field to 'api.task.task'
-	// 20) Add 'DependsOn' field to 'apicontainer.Container'
+	// 20)
+	//	 a) Add 'DependsOn' field to 'apicontainer.Container'


nit: indentation

sharanyad · 2019-02-18T19:22:54Z

agent/statemanager/state_manager.go

-	// 20) Add 'DependsOn' field to 'apicontainer.Container'
+	// 20)
+	//	 a) Add 'DependsOn' field to 'apicontainer.Container'
+	//   b) Add 'StartTime' field to 'api.container.Container'


could you please add a test for the state file version bump in the state manager package?

good catch, will add state manager unit tests for this.

sharanyad · 2019-02-18T19:24:22Z

agent/engine/engine_unix_integ_test.go

@@ -1447,3 +1447,42 @@ func TestGPUAssociationTask(t *testing.T) {
 	go taskEngine.AddTask(&taskUpdate)
 	verifyTaskIsStopped(stateChangeEvents, testTask)
 }
+
+func TestPerContainerStopTimeout(t *testing.T) {


is there any reason why this is under unix?

adding TODO for windows test.

sharanyad · 2019-02-18T19:25:50Z

agent/engine/docker_task_engine.go

-	timeout := engine.cfg.DockerStopTimeout + dockerclient.StopContainerTimeout
-	return engine.client.StopContainer(engine.ctx, dockerContainer.DockerID, timeout)
+
+	apiTimeoutStopContainer := container.GetStopTimeout()


how about calling this stopContainerAPITimeout?

i dont have a strong feeling either way tbh

adnxn · 2019-02-18T23:38:06Z

ecs/windows/unit_test failing intermittently here due to #1864 but passing in travis.

sharanyad

code lgtm.
just that ECS_CONTAINER_STOP_TIMEOUT and stopTimeout have different meanings and might confuse customers :(

adnxn · 2019-02-19T00:18:46Z

just that ECS_CONTAINER_STOP_TIMEOUT and stopTimeout have different meanings and might confuse customers :(

since this is a communication problem, ill update the README entry for ECS_CONTAINER_STOP_TIMEOUT and ECS_CONTAINER_START_TIMEOUT to specify that they have instance wide scope. we'll need to make sure the docs for the new container fields differentiate the two as well (task/container def field vs ecs.config variable)

adnxn changed the title ~~engine,dockerclient: per container start/stop timeouts~~ [WIP] engine,dockerclient: per container start/stop timeouts Feb 13, 2019

adnxn added this to the 1.26.0 milestone Feb 13, 2019

adnxn requested a review from petderek February 13, 2019 21:35

adnxn added bot/test and removed bot/test labels Feb 14, 2019

adnxn force-pushed the dev-granular-timeouts branch 2 times, most recently from 38b4496 to 057f252 Compare February 15, 2019 21:30

adnxn changed the title ~~[WIP] engine,dockerclient: per container start/stop timeouts~~ engine,dockerclient: per container start/stop timeouts Feb 15, 2019

adnxn requested review from a team and yhlee-aws February 15, 2019 21:46

yhlee-aws reviewed Feb 15, 2019

View reviewed changes

adnxn commented Feb 15, 2019

View reviewed changes

petderek approved these changes Feb 18, 2019

View reviewed changes

adnxn changed the base branch from dev to container-ordering-feature February 18, 2019 16:26

adnxn force-pushed the dev-granular-timeouts branch from 057f252 to a7b34b2 Compare February 18, 2019 19:16

sharanyad reviewed Feb 18, 2019

View reviewed changes

statemanager: add container start/stop statemanger

3265bf9

adnxn force-pushed the dev-granular-timeouts branch from a7b34b2 to 3265bf9 Compare February 18, 2019 20:52

adnxn mentioned this pull request Feb 18, 2019

testing: TestPullImageInactivityTimeout failing intermittently on windows #1864

Closed

sharanyad approved these changes Feb 19, 2019

View reviewed changes

adnxn merged commit 3265bf9 into aws:container-ordering-feature Feb 19, 2019

adnxn added a commit that referenced this pull request Feb 19, 2019

Merge pull request #1849 from adnxn/dev-granular-timeouts

f4da625

adnxn mentioned this pull request Feb 19, 2019

readme: update config level container timeouts #1870

Merged

adnxn mentioned this pull request Mar 6, 2019

Tune SIGKILL timeout on a per ECS Task/Container Definition basis #1241

Closed

adnxn deleted the dev-granular-timeouts branch October 21, 2019 20:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

engine,dockerclient: per container start/stop timeouts #1849

engine,dockerclient: per container start/stop timeouts #1849

adnxn commented Feb 13, 2019 •

edited

Loading

adnxn commented Feb 15, 2019

yhlee-aws Feb 15, 2019

adnxn Feb 15, 2019

yhlee-aws Feb 15, 2019

adnxn Feb 15, 2019 •

edited

Loading

adnxn left a comment •

edited

Loading

adnxn commented Feb 18, 2019

sharanyad Feb 18, 2019

adnxn Feb 18, 2019

sharanyad Feb 18, 2019

sharanyad Feb 18, 2019

adnxn Feb 18, 2019

sharanyad Feb 18, 2019

adnxn Feb 18, 2019

sharanyad Feb 18, 2019

adnxn Feb 18, 2019

adnxn commented Feb 18, 2019

sharanyad left a comment

adnxn commented Feb 19, 2019

		@@ -236,6 +236,10 @@ type Container struct {

		Secrets []*Secret `locationName:"secrets" type:"list"`

		StartTimeout *int64 `locationName:"startTimeout" type:"integer"`

engine,dockerclient: per container start/stop timeouts #1849

engine,dockerclient: per container start/stop timeouts #1849

Conversation

adnxn commented Feb 13, 2019 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

adnxn commented Feb 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adnxn Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

adnxn left a comment • edited Loading

Choose a reason for hiding this comment

adnxn commented Feb 18, 2019

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

adnxn commented Feb 18, 2019

sharanyad left a comment

Choose a reason for hiding this comment

adnxn commented Feb 19, 2019

adnxn commented Feb 13, 2019 •

edited

Loading

adnxn Feb 15, 2019 •

edited

Loading

adnxn left a comment •

edited

Loading