Add timeout for setting up pause container #1358

richardpen · 2018-04-23T21:24:05Z

Summary

Added timeout for SetupNS.

Implementation details

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

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

sharanyad

how did we come up with a timeout value of of 1 minute?

sharanyad · 2018-04-23T21:47:12Z

agent/ecscni/plugin.go

+
+	select {
+	case <-ctx.Done():
+		return nil, errors.Errorf("cni setup: setup the container namespace timedout")


this can just be errors.New?
also, message can be "cni: container namespace setup timed out"

sharanyad · 2018-04-23T21:49:13Z

agent/engine/docker_task_engine.go

@@ -61,6 +61,7 @@ const (
 	labelTaskDefinitionFamily    = labelPrefix + "task-definition-family"
 	labelTaskDefinitionVersion   = labelPrefix + "task-definition-version"
 	labelCluster                 = labelPrefix + "cluster"
+	cniSetupTimeoutDuration      = 1 * time.Minute


nit: this can just be cniSetupTimeout

richardpen · 2018-04-23T21:56:46Z

Based on the test, it seems this process is really fast on different instance <5s. But in the cni plugin itself, there are some retry and timeout(maximum is 30s) for retrieving instance metadata and ipv6 gateway, so I doubled it to 1mins.

adnxn

changes lgtm, just minor edit to error msg wording as @sharanyad called out.

aaithal · 2018-04-25T20:01:37Z

agent/ecscni/plugin.go

@@ -69,7 +70,24 @@ func NewClient(cfg *Config) CNIClient {

 // SetupNS will set up the namespace of container, including create the bridge
 // and the veth pair, move the eni to container namespace, setup the routes
-func (client *cniClient) SetupNS(cfg *Config) (*current.Result, error) {
+func (client *cniClient) SetupNS(cfg *Config, ctx context.Context) (*current.Result, error) {


Please make sure that ctx is the first argument here.

aaithal · 2018-04-25T20:02:44Z

agent/ecscni/plugin.go

@@ -38,7 +39,7 @@ type CNIClient interface {
 	// Capabilities returns the capabilities supported by a plugin
 	Capabilities(string) ([]string, error)
 	// SetupNS sets up the namespace of container
-	SetupNS(*Config) (*current.Result, error)
+	SetupNS(*Config, context.Context) (*current.Result, error)


Can you make sure that CleanupNS() also has a context wired?

aaithal · 2018-04-25T20:03:50Z

agent/ecscni/plugin.go

+	var err error
+	setupDone := make(chan struct{})
+	go func() {
+		result, err = client.setupNS(cfg)


What happens if this actually times out to the plugin execution path? Will they still be invoked in an orphan process? Can you please add details about how you tested this? What the behavior is? Essentially, how safe is it to do this?

If the timeout happens, agent will move the task to stopped but the plugin was still running until it wasn't stuck. Ideally we should stop the process, but we don't know the plugin process id, I have opened the issue in libcni for adding the functionality to stop the plugin process after a timeout. The test I did is having two awsvpc task, the first was timedout during cni setup, and the second task can still progress successfully.

Should we try to invoke CleanupNS() in that scenario then (if SetupNS() times out invoke CleanupNS())?

The CleanupNS is already called after the setupNS timeout: when setupNS timeout, the task desired status was moved to stop, and agent will try to stop pause container at which time the CleanupNS is called.

Can you please create an issue in the agent repo to track this as well (and link the cni issue to that)? It doesn't seem super optimal to just leave a process hanging if we decide to give up early on it.

aaithal · 2018-04-26T16:20:09Z

agent/ecscni/plugin.go

+	var err error
+	setupDone := make(chan struct{})
+	go func() {
+		result, err = client.setupNS(cfg)


Even though it's not today, it seems like this could get racy in the future. Instead of this semi-global var pattern, can you instead pass an intermediate struct back in setupDone channel?

aaithal · 2018-04-26T16:20:45Z

agent/ecscni/plugin.go

+
+	select {
+	case <-ctx.Done():
+		return nil, errors.New("cni setup: container namespace setup timed out")


you should also check for ctx.Err() here and wrap that if non nil instead of always assuming that context timed out.

aaithal · 2018-04-26T16:21:09Z

agent/ecscni/plugin.go

+
+	select {
+	case <-ctx.Done():
+		return errors.New("cni cleanup: container namespace cleanup timed out")


same comment as above regarding ctx.Err()

aaithal · 2018-04-26T19:41:30Z

CHANGELOG.md

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

+## 1.17.4-dev
+* Bug - Fixed a bug where task stuck in pending due to the cni setup process hangs [#1358](https://github.com/aws/amazon-ecs-agent/pull/1358)


how about Fixed a bug where tasks could get stuck waiting for execution of CNI scripts?

aaithal · 2018-04-26T19:44:13Z

agent/ecscni/plugin.go

+
+	select {
+	case <-ctx.Done():
+		return nil, errors.Wrap(ctx.Err(), "cni setup: container namespace setup failed")


I think you have to check if ctx.Err() is non nil. A much better model would be to create the derived context within this method and handle ctx.Err() == nil case as well.

You essentially do

derivedCtx, cancel := context.WithTimeout(ctx, timeout) defer cancel() go func(...) { } select { case <-ctx.Done(): if ctc.Err() != nil { return nil, errors.Wrap() } else { return nil, errors.New() } ... }

Based on the document here, I don't think check ctx.Err()!=nil is required.

// If Done is not yet closed, Err returns nil. // If Done is closed, Err returns a non-nil error explaining why: // Canceled if the context was canceled // or DeadlineExceeded if the context's deadline passed. // After Err returns a non-nil error, successive calls to Err return the same error.

A much better model would be to create the derived context within this method

Yes, a timeout instead should be passed in instead of passing the context, will modify that.

Based on the document here, I don't think check ctx.Err()!=nil is required.

Thanks! make sense.

aaithal · 2018-04-26T19:49:05Z

agent/ecscni/plugin.go

@@ -110,7 +133,21 @@ func (client *cniClient) SetupNS(cfg *Config) (*current.Result, error) {

 // CleanupNS will clean up the container namespace, including remove the veth
 // pair and stop the dhclient
-func (client *cniClient) CleanupNS(cfg *Config) error {
+func (client *cniClient) CleanupNS(ctx context.Context, cfg *Config) error {


Same comment about accepting timeout as a param and dealing with ctx.Err() as above.

richardpen requested review from adnxn, aaithal and a team April 23, 2018 21:24

sharanyad reviewed Apr 23, 2018

View reviewed changes

richardpen added this to the 1.17.3 milestone Apr 24, 2018

adnxn approved these changes Apr 24, 2018

View reviewed changes

adnxn modified the milestones: 1.17.3, 1.17.4 Apr 25, 2018

richardpen force-pushed the cni-timeout branch from 69b186a to 2079cb3 Compare April 25, 2018 18:18

aaithal reviewed Apr 25, 2018

View reviewed changes

richardpen force-pushed the cni-timeout branch from 1bac2a2 to 40f3da9 Compare April 25, 2018 21:28

aaithal reviewed Apr 26, 2018

View reviewed changes

richardpen force-pushed the cni-timeout branch from 40f3da9 to ce134c8 Compare April 26, 2018 17:28

aaithal reviewed Apr 26, 2018

View reviewed changes

richardpen force-pushed the cni-timeout branch from ce134c8 to 9bf3d9b Compare April 26, 2018 20:26

Peng Yin added 2 commits April 26, 2018 20:40

engine: add timeout for setting up pause container

180ab59

engine: add timeout for cleaning up pause container

7f07da3

richardpen force-pushed the cni-timeout branch from 9bf3d9b to 7f07da3 Compare April 26, 2018 20:42

aaithal approved these changes Apr 26, 2018

View reviewed changes

richardpen merged commit 6f3cf0f into aws:dev May 1, 2018

aaithal modified the milestones: 1.17.4, 1.18.0 May 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add timeout for setting up pause container #1358

Add timeout for setting up pause container #1358

richardpen commented Apr 23, 2018 •

edited

Loading

sharanyad left a comment

sharanyad Apr 23, 2018

sharanyad Apr 23, 2018

richardpen commented Apr 23, 2018

adnxn left a comment

aaithal Apr 25, 2018

aaithal Apr 25, 2018

aaithal Apr 25, 2018

richardpen Apr 25, 2018

aaithal Apr 25, 2018

richardpen Apr 25, 2018

aaithal Apr 26, 2018

aaithal Apr 26, 2018

aaithal Apr 26, 2018

aaithal Apr 26, 2018

aaithal Apr 26, 2018

aaithal Apr 26, 2018 •

edited

Loading

richardpen Apr 26, 2018

aaithal Apr 26, 2018

aaithal Apr 26, 2018

Add timeout for setting up pause container #1358

Add timeout for setting up pause container #1358

Conversation

richardpen commented Apr 23, 2018 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

sharanyad 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 Apr 23, 2018

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

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

richardpen commented Apr 23, 2018 •

edited

Loading

aaithal Apr 26, 2018 •

edited

Loading