Pull retries and configurable routes for `awsvpc` network mode #975

samuelkarp · 2017-09-08T22:32:17Z

Summary

This series of commits adds retries for failed pulls (compensating for transient network issues) and allows additional routes to be configured for tasks launched with the awsvpc network mode.

Implementation details

Retries are implemented with the new utils.RetryNWithBackoffCtx (based on existing utils.RetryNWithBackoff) inside the dockerGoClient and excludes one expected non-transient error (missing IAM permissions to call ecr:GetAuthorizationToken). Other non-transient errors could be excluded from retries by extending CannotPullContainerError to implement utils.Retrier, but has not been done yet (I have not assembled a list of expected non-transient error codes from Docker).

Configurable routes were added by depending on the types.IPNet type from github.com/containernetworking/cni/pkg/types, which wraps net.IPNet so it can be serialized and deserialized in CIDR notation.

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

Ran in an environment with manually configured additional routes and with transient network failures.

New tests cover the changes: yes

Description for the changelog

Enhancement - Retry failed container image pull operations.

Licensing

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

This change enables a static IPv4 address to be assigned to tasks with the `awsvpc` network mode and also configures a return route for traffic between the task and the host.

richardpen · 2017-09-11T19:45:35Z

agent/engine/engine_mocks.go

@@ -17,6 +17,7 @@
 package engine

 import (
+	context0 "context"


Why was this added?

I'm using the context package in my code.

I have no idea why golang.org/x/net/context is still imported here...

Looks like the golang.org/x/net/context is used by the task_engine.Init(), I think that's why the reason it's imported. I asked because I saw two context imported, we can't get rid of this until go docker client updated this.

richardpen · 2017-09-11T19:50:56Z

agent/ecscni/plugin.go

 	"github.com/pkg/errors"
 )

 // CNIClient defines the method of setting/cleaning up container namespace
 type CNIClient interface {
 	Version(string) (string, error)
 	Capabilities(string) ([]string, error)
-	SetupNS(*Config) error
-	CleanupNS(*Config) error
+	SetupNS(*Config, []cnitypes.IPNet) error


Instead of passing an extra parameter, would it be better to add a new field in the Config struct?

I could do that if you feel strongly about it.

richardpen · 2017-09-11T20:14:57Z

agent/engine/docker_container_engine.go


 	response := make(chan DockerContainerMetadata, 1)
-	go func() { response <- dg.pullImage(image, authData) }()
+	go func() {


Should we distinguish the errors generated by the agent from the errors generated by docker, and we only retry for errors from docker? For the errors generated by the agent like CannotPullECRContainerError and CreateEmptyVolumeError probably won't recover with retry.

CannotPullECRContainerError is already excluded from retries. I can extend CreateEmptyVolumeError to exclude it from retries as well. The other errors wrapped by CannotPullContainerError are not excluded right now; I only covered ECR because it was easy.

richardpen · 2017-09-11T20:31:33Z

CHANGELOG.md

@@ -2,6 +2,7 @@

 ## UNRELEASED
 * Feature - Support for provisioning Tasks with ENIs
+* Enhancement - Retry failed container image pull operations.


This also added another feature that add customized route in the container namespace for task using awsvpc. If I read the code correctly, this only can be configured by the config file, would it be more convenient to use environment variable. Also we may need to check the route before adding it, as the task traffic inside the container namespace can be changed by adding some route?

You're right; I had intended to make it configurable by environment variable as well but apparently never wrote the code. I'll do that.

I don't think we need to check the route. This is a sufficiently advanced, optional feature that is used in fairly narrow use-cases.

nmeyerhans · 2017-09-11T21:24:22Z

agent/engine/docker_task_engine.go

+			return nil
+		}
+		if imagePullError, ok := metadata.Error.(CannotPullContainerError); ok {
+			seelog.Warnf("When pulling %s: %s", container.Image, pullError)


pullError and imagePullError aren't supposed to be distinct names. Can we fix this commit before the PR is merged? Either name is fine to use.

(I realize that this code doesn't exist as written in the final PR, but I do want bisects, etc, to work and we shouldn't have unbuildable code in the repo.)

Yeah, I'll fix this.

petderek · 2017-09-11T22:45:50Z

agent/ecs_client/model/ecs/service.go

@@ -70,8 +70,8 @@ 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 = ServiceName
+	if len(signingName) == 0 {


Why this way?

So, I didn't make this change...this is what happened when I ran make gogenerate.

But the logic here is actually a little better; even though the zero-value of a string is "", this also would handle a *string where the zero-value is nil with no changes.

petderek · 2017-09-11T23:31:34Z

agent/engine/docker_task_engine.go

 }

 func (engine *DockerTaskEngine) buildCNIConfigFromTaskContainer(task *api.Task, container *api.Container) (*ecscni.Config, error) {
 	cfg, err := task.BuildCNIConfig()
 	if err != nil {
 		return nil, errors.Wrapf(err, "engine: build cni configuration from taskfailed")
 	}
+
+	if engine.cfg.OverrideAWSVPCLocalIPv4Address != nil &&


[nit]
consider moving this expression to a smaller function. ie, hasValidAWSVPCLocalOverrides(cfg *Config)

I think I'm going to leave this as-is for now unless you feel strongly about it.

petderek · 2017-09-11T23:56:23Z

agent/utils/utils.go

+		select {
+		case <-ctx.Done():
+			return nil
+		default:


Neat!!

Why keep the default block empty? Would it make sense to put err = fn().. etc there?

Seemed clearer to me to have it out. This way it reads that we early return if the context is done, but otherwise keep going.

samuelkarp · 2017-09-13T00:35:15Z

@richardpen @nmeyerhans I've addressed your comments, can you take a look?

richardpen

Also have you manually tested the route add part?

richardpen · 2017-09-13T16:50:54Z

agent/api/errors.go

@@ -46,7 +46,7 @@ type NamedError interface {
 	ErrorName() string
 }

-// NamedError is a wrapper type for 'error' which adds an optional name and provides a symetric marshal/unmarshal
+// NamedError is a wrapper type for 'error' which adds an optional name and provides a symmetric marshal/unmarshal


Looks like this comment should be moved above to the NamedError.

I'll change it to talking about DefaultNamedError, since that's the implementation that actually provides the marshal/unmarshal behavior.

richardpen · 2017-09-13T16:51:44Z

agent/config/config.go

@@ -218,7 +219,8 @@ func fileConfig() (Config, error) {

 	err = json.Unmarshal(data, &cfg)
 	if err != nil {
-		seelog.Errorf("Error reading cfg json data, err %v", err)
+		seelog.Criticalf("Error reading cfg json data, err %v", err)


Is it worth to logging the string(data) here?

Yeah, not a bad idea.

After further thought, I'm going to leave this out for now. I'm concerned about the case where a customer adds credentials (EngineAuthData) to the file and then we have a parse error; that would cause credentials to be written out to the log.

richardpen · 2017-09-13T16:55:13Z

agent/config/config.go

+	if additionalLocalRoutesEnv != "" {
+		err := json.Unmarshal([]byte(additionalLocalRoutesEnv), &additionalLocalRoutes)
+		if err != nil {
+			seelog.Warnf("Invalid format for ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES, expected a json array of CIDRs: %v", err)


same here, should we log the additionalLocalRoutesEnv?

Yes, can do.

I'm also going to leave this out. We don't log the read environment variable for any of the other ones that we parse. I'd rather leave this consistent for now and then revisit it later when we can change all of them.

richardpen · 2017-09-13T16:57:32Z

agent/config/config_test.go

 	os.Setenv("ECS_ENABLE_TASK_ENI", "true")
+	defer os.Unsetenv("ECS_ENABLE_TASK_ENI")
+	additionalLocalRoutesJSON := `["1.2.3.4/22","5.6.7.8/32"]`
+	os.Setenv("ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES", additionalLocalRoutesJSON)


you missed defer here

richardpen · 2017-09-13T16:59:59Z

agent/config/config_unix_test.go

+	assert.Equal(t, map[string]string{"attribute1": "value1"}, cfg.InstanceAttributes)
+}
+
+// TestDockerAuthMergeFromFile tests docker auth read from file correctly after merge


the comment is incorrect.

richardpen · 2017-09-13T17:00:30Z

agent/config/config_windows_test.go

@@ -25,6 +25,8 @@ import (
 )

 func TestConfigDefault(t *testing.T) {
+	os.Setenv("AWS_DEFAULT_REGION", "foo-bar-1")
+	defer os.Unsetenv("AWS_DEFAULT")


nit: AWS_DEFAULT_REGION

richardpen · 2017-09-13T17:01:05Z

agent/config/config_windows_test.go

@@ -64,10 +66,12 @@ func TestConfigDefault(t *testing.T) {
 }

 func TestConfigIAMTaskRolesReserves80(t *testing.T) {
+	os.Setenv("AWS_DEFAULT_REGION", "foo-bar-1")
+	defer os.Unsetenv("AWS_DEFAULT")


same as above.

samuelkarp · 2017-09-13T17:16:02Z

Also have you manually tested the route add part?

Yes, but in an earlier revision. I will re-run my tests with the latest changes.

nmeyerhans · 2017-09-13T17:20:24Z

README.md

+| `ECS_ENABLE_TASK_ENI` | `false` | Whether to enable task networking for task to be launched with its own network interface | `false` | Not applicable |
+| `ECS_CNI_PLUGINS_PATH` | `/ecs/cni` | The path where the cni binary file is located | `/amazon-ecs-cni-plugins` | Not applicable |
+| `ECS_AWSVPC_BLOCK_IMDS` | `true` | Whether to block access to [Instance Metdata](http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-metadata.html) for Tasks started with `awsvpc` network mode | `false` | Not applicable |
+| `ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES` | `["10.0.15.0/24"]` | Additional routes that cause traffic to travel by the default ENI | `[]` | Not applicable |


Although the name strongly implies it, the description should be explicit that this is used in awsvpc mode.

It's also not entirely accurate to say that traffic to these routes goes out via the default ENI; it may never leave the host at all. I might rewrite the description as

In awsvpc network mode, traffic to these prefixes will be routed via the host bridge instead of the task ENI

or something like that.

I'll use your description.

nmeyerhans · 2017-09-13T17:24:02Z

agent/config/config.go

+	if additionalLocalRoutesEnv != "" {
+		err := json.Unmarshal([]byte(additionalLocalRoutesEnv), &additionalLocalRoutes)
+		if err != nil {
+			seelog.Warnf("Invalid format for ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES, expected a json array of CIDRs: %v", err)


Why is warn the right severity here?

We're using warn for all of them right now. I'd be happy to change all of them to error if you'd like.

Actually, we're using warn for both non-fatal conditions (actually should be warnings) and for parsing errors (should be errors). I'll change the ones that actually cause the agent to exit to error.

Thanks, that's pretty much what I was getting at. If NewConfig() returns an error, agent panics. We should probably log the condition that cause the panic at an appropriately high severity.

samuelkarp · 2017-09-13T17:58:23Z

@richardpen @nmeyerhans I believe I have addressed your comments, including additional manual testing. Can you take a look?

aaithal

lgtm. I've got minor questions comments.

aaithal · 2017-09-13T18:04:51Z

agent/config/config.go

+	err = json.Unmarshal([]byte(instanceAttributesEnv), &instanceAttributes)
+	if instanceAttributesEnv != "" {
+		if err != nil {
+			err := fmt.Errorf("Invalid format for ECS_INSTANCE_ATTRIBUTES. Expected a json hash")


Can we log/wrap the original error as well?

aaithal · 2017-09-13T18:08:58Z

agent/ecscni/plugin_test.go

@@ -26,7 +27,11 @@ func TestSetupNS(t *testing.T) {
 		mockResult.EXPECT().String().Return(""),
 	)

-	err := ecscniClient.SetupNS(&Config{})
+	additionalRoutesJson := `["169.254.172.1/32", "10.11.12.13/32"]`
+	additionalRoutes := &[]cnitypes.IPNet{}


nit: this reads weird to me. Can it be just var AdditionalLocalRoutes []cnitypes.IPNet{} here instead and you can unmarshal it as &additionalRoutes?

aaithal · 2017-09-13T18:17:20Z

agent/utils/utils.go

+
+		retriableErr, isRetriableErr := err.(Retriable)
+
+		if err == nil || isRetriableErr && !retriableErr.Retry() {


Can you use parenthesis here to illustrate/enforce the operator precedence? Otherwise, readers like me will always have to look up the operator precedence documentation for how this is evaluated.

It's just short-circuiting evaluation; any time a left-condition evaluates false, nothing proceeds.

Are you asking me to change this to err == nil || (isRetriableErr && !retriableErr.Retry())? That's logically equivalent, but (to me) harder to read.

Are you asking me to change this to err == nil || (isRetriableErr && !retriableErr.Retry())? That's logically equivalent, but (to me) harder to read.

Yeah, I prefer that. Because, then I wouldn't have to wonder if isRetriableErr is associated with the || or && operator.

aaithal · 2017-09-13T18:20:06Z

agent/config/types.go

+
+	// OverrideAWSVPCLocalIPv4Address overrides the local IPv4 address chosen
+	// for a task using the `awsvpc` networking mode. Using this configuration
+	// will limit you to running one `awsvpc` task at a time. IPv4 addresses


Using this configuration will limit you to running one awsvpc task

Since we don't enforce this anywhere in the code (iiuc), may be we should add a TODO so that we don't lose sight of this? Even an issue would be fine.

Do you want a TODO even if we don't have any short-term plans to change it? If you statically-assign an IP address, you'll always be limited to one or else you'll have IP address conflicts and things won't work right.

Hmm, may be the better thing to do here is to update the description for the ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES field in the README file with some of what you have here as well?

ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES won't limit you to running one awsvpc task; it just adds routes. It's specifically the static IP assignment that is limiting here.

So, OverrideAWSVPCLocalIPv4Address gets set just from the JSON config and nowhere else? If that's the case, it's probably fine to leave it be. I'm just looking for ways to highlight this behavior so that it doesn't lead to post start-task failures.

Exactly. The intent here is that this is meant to serve a very limited set of use-cases and is not really a generally useful feature, so it's excluded from the more-common way to configure the agent (environment variables).

nmeyerhans

LGTM

We're going to need an IPv6 equivalent of ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES at some point...

samuelkarp · 2017-09-13T18:44:03Z

@aaithal I've addressed your comments, except for the one about OverrideAWSVPCLocalIPv4Address. Let me know if you want me to make any additional changes.

awsvpc: enable static IPv4 config + return route

a109a8c

This change enables a static IPv4 address to be assigned to tasks with the `awsvpc` network mode and also configures a return route for traffic between the task and the host.

samuelkarp requested review from nmeyerhans, aaithal and richardpen September 8, 2017 22:32

samuelkarp force-pushed the retries-and-configurable-routes branch from 21c19d9 to e45a20b Compare September 8, 2017 22:44

richardpen reviewed Sep 11, 2017

View reviewed changes

nmeyerhans reviewed Sep 11, 2017

View reviewed changes

petderek reviewed Sep 11, 2017

View reviewed changes

Noah Meyerhans added 2 commits September 12, 2017 15:34

Add retries to docker pull operations

9b70685

Allow specification of additional routes via the config file

6ba0923

samuelkarp force-pushed the retries-and-configurable-routes branch from e45a20b to 557658d Compare September 13, 2017 00:34

samuelkarp force-pushed the retries-and-configurable-routes branch from 557658d to cf49171 Compare September 13, 2017 00:40

richardpen reviewed Sep 13, 2017

View reviewed changes

nmeyerhans reviewed Sep 13, 2017

View reviewed changes

samuelkarp force-pushed the retries-and-configurable-routes branch from cf49171 to 2bc5d66 Compare September 13, 2017 17:57

richardpen approved these changes Sep 13, 2017

View reviewed changes

aaithal reviewed Sep 13, 2017

View reviewed changes

samuelkarp added 5 commits September 13, 2017 11:33

eni: use types.IPNet instead of string

65c40dc

utils: Add context to retries

4e6b589

engine: move pull retries inside dockerGoClient

68ce465

engine: exclude CreateEmptyVolumeError from retry

6b1f314

awsvpc: configure local routes with env var

9241ed4

nmeyerhans approved these changes Sep 13, 2017

View reviewed changes

config: clean up ECS_INSTANCE_ATTRIBUTES parsing

4ebfefe

samuelkarp force-pushed the retries-and-configurable-routes branch from 2bc5d66 to 4ebfefe Compare September 13, 2017 18:43

aaithal approved these changes Sep 13, 2017

View reviewed changes

samuelkarp merged commit 4ebfefe into aws:dev Sep 13, 2017

jhaynes mentioned this pull request Sep 30, 2017

V1.14.5 release #999

Merged


		retriableErr, isRetriableErr := err.(Retriable)

		if err == nil \|\| isRetriableErr && !retriableErr.Retry() {

Pull retries and configurable routes for awsvpc network mode #975

Pull retries and configurable routes for awsvpc network mode #975

Conversation

samuelkarp commented Sep 8, 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

samuelkarp commented Sep 13, 2017

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

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

samuelkarp commented Sep 13, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelkarp commented Sep 13, 2017

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

nmeyerhans left a comment

Choose a reason for hiding this comment

samuelkarp commented Sep 13, 2017

Pull retries and configurable routes for `awsvpc` network mode #975

Pull retries and configurable routes for `awsvpc` network mode #975

samuelkarp commented Sep 8, 2017 •

edited

Loading