eni watcher: add backoff-retry #1148

aaithal · 2017-12-21T18:54:25Z

Summary

eni watcher: add backoff-retry

Implementation details

This commit adds back-off retry logic in:

udev watcher: It takes a few milliseconds for the ENI's mac address to
show up on the instance once udev watcher detects that a new device has
been attached
eni state lookup: If for whatever reason, there's a delay in ACS
sending the ENI attachment message to the agent, the agent's state
remains unaware of the ENI

changelog entry

This should lead to improved task start latencies of awsvpc tasks.

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
Manual tests:

Time between AttachTaskNetworkInterfacesMessage and PayloadMessage without this change: 23s
Time between AttachTaskNetworkInterfacesMessage and PayloadMessage with this change: 11s

New tests cover the changes: yes

Description for the changelog

Licensing

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

This commit adds back-off retry logic in: * udev watcher: It takes a few milliseconds for the ENI's mac address to show up on the instance once udev watcher detects that a new device has been attached * eni state lookup: If for whatever reason, there's a delay in ACS sending the ENI attachment message to the agent, the agent's state remains unaware of the ENI This should lead to improved task start latencies of awsvpc tasks.

richardpen

minor comments.

richardpen · 2017-12-22T00:50:24Z

agent/eni/networkutils/utils_linux.go

 	if err != nil {
 		return "", err
 	}
-	return link.Attrs().HardwareAddr.String(), err
+	// RetryWithBackoffCtx returns nil when the context is cancelled. Check if there was


Would it be better to check retriever.macAddress != "", though I don't strongly opposite this.

Hmm, I don't know if it gives us any additional benefits. i like dealing with errors rather than empty strings or nils. So, will keep it this way

richardpen · 2017-12-22T00:58:46Z

agent/eni/watcher/watcher_linux.go

@@ -57,6 +88,17 @@ type UdevWatcher struct {
 	primaryMAC           string
 }

+// unmanagedENIError is used to indicate that the agent found an ENI, but the agent isn't


Can this be moved to another file?

I was thinking we may need a type.go for the struct and const, to split this file into smaller pieces which are easier to read.

i'm not a fan of the type.go model. It's much more easier to navigate/organize the code where the struct and the methods are defined in the same file. We could move this into an errors.go file. But, seeing how it's just one type, I'd rather keep it here than to create a new file for it.

adnxn

code lgtm, just few questions.

adnxn · 2017-12-22T16:45:07Z

agent/eni/networkutils/utils_linux.go

 )

+const (


how exactly did we determine the durations for these backoff times? same with the eni ones below.

with experiments. It took an average of 10ms for the ENI's mac address to show up on the instance. The one for attachment message was based on my conversations with the ECS backend team.

adnxn · 2017-12-22T17:15:39Z

agent/eni/networkutils/utils_linux.go

 	splitDevLink := strings.SplitN(devicePath, "devices/", 2)
 	if len(splitDevLink) != 2 {
-		log.Warnf("Cannot determine device validity: %s", devicePath)
+		seelog.Warnf("Cannot determine device validity: %s", devicePath)


its not clear to me why len(splitDevLink) != 2 means we cannot determine device validity. can we add some documentation? should we add the splitDevLink to log output as well? although, maybe i'm just missing context.

The example entries in the comment above are meant to illustrate that:

eth1 -> /devices/pci0000:00/0000:00:05.0/net/eth1

This means that we expect it to be of "devices/some-string" format. Is that not clear enough? It's just additional validation

adnxn · 2017-12-22T17:25:43Z

agent/eni/watcher/watcher_linux.go

+
+// Error returns the error string for the unmanagedENIError type
+func (err *unmanagedENIError) Error() string {
+	return fmt.Sprintf("udev watcher send ENI state change: eni not managed by ecs: %s", err.mac)


it's not clear to me what we mean by udev watcher send ENI state change. the second part of the error message is fine though.

adnxn · 2017-12-22T17:44:37Z

agent/utils/errors.go

@@ -1,4 +1,4 @@
-// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved.


thanks for documenting all these =]

nmeyerhans · 2017-12-22T18:24:49Z

agent/eni/networkutils/utils_linux.go

+	dev := filepath.Base(retriever.dev)
+	link, err := retriever.netlinkClient.LinkByName(dev)
+	if err != nil {
+		return utils.NewRetriableError(utils.NewRetriable(false), err)


Are you sure that we should never retry in this case?

yeah, an error means a syscall failed for describing the device. at this point of time, it doesn't make sense to retry it as it really should be visible on the instance

samuelkarp · 2017-12-22T22:24:29Z

CHANGELOG.md

@@ -4,6 +4,7 @@
 * Feature - Support a HTTP endpoint for `awsvpc` tasks to query metadata
 * 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 - Fixed a bug where the agent could miss sending an ENI attachment to ECS because of address propagation delays [#1148](https://github.com/aws/amazon-ecs-agent/pull/1148)


Can you wrap this at 80 characters?

samuelkarp · 2017-12-22T22:25:50Z

agent/eni/networkutils/utils_linux.go

+
+	// macAddressBackoffJitter specifies the jitter multiple percentage when
+	// looking for an ENI's mac address on the host
+	macAddressBackoffJitter = 0.2


I don't think jitter is really necessary here since you're not making remote calls or accessing a resource with high concurrency. On the other hand, I don't think it really hurts either except to make the timing a bit more unpredictable.

samuelkarp · 2017-12-22T22:28:04Z

agent/eni/networkutils/utils_linux.go

 	if err != nil {
 		return "", err
 	}
-	return link.Attrs().HardwareAddr.String(), err
+	// RetryWithBackoffCtx returns nil when the context is cancelled. Check if there was
+	// a timeout here. TODO: Fix RetryWithBackoffCtx to return ctx.Err() on context Done()


oops, I think that's on me...

aaithal · 2018-01-05T19:12:03Z

closing this as this is already merged to the master branch with 1.16.1 release

aaithal requested review from nmeyerhans and richardpen December 21, 2017 18:54

aaithal added 2 commits December 21, 2017 13:05

changelog entry for pr 1148

64377a9

aaithal force-pushed the eniAttachmentLatencyImprovements branch from 1f3855f to 64377a9 Compare December 21, 2017 21:06

richardpen reviewed Dec 22, 2017

View reviewed changes

adnxn reviewed Dec 22, 2017

View reviewed changes

nmeyerhans reviewed Dec 22, 2017

View reviewed changes

richardpen approved these changes Dec 22, 2017

View reviewed changes

samuelkarp approved these changes Dec 22, 2017

View reviewed changes

adnxn approved these changes Dec 22, 2017

View reviewed changes

nmeyerhans approved these changes Dec 26, 2017

View reviewed changes

nmeyerhans mentioned this pull request Dec 27, 2017

Pr 1148 for 1.16.x #1152

Merged

8 tasks

aaithal closed this Jan 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eni watcher: add backoff-retry #1148

eni watcher: add backoff-retry #1148

aaithal commented Dec 21, 2017 •

edited

Loading

richardpen left a comment

richardpen Dec 22, 2017

aaithal Dec 22, 2017

richardpen Dec 22, 2017

aaithal Dec 22, 2017

richardpen Dec 22, 2017

aaithal Dec 22, 2017

adnxn left a comment

adnxn Dec 22, 2017

aaithal Dec 22, 2017

adnxn Dec 22, 2017

aaithal Dec 22, 2017 •

edited

Loading

adnxn Dec 22, 2017

adnxn Dec 22, 2017

nmeyerhans Dec 22, 2017

aaithal Dec 22, 2017

samuelkarp Dec 22, 2017

samuelkarp Dec 22, 2017

samuelkarp Dec 22, 2017

aaithal commented Jan 5, 2018

		@@ -1,4 +1,4 @@
		// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved.
		// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved.

		)

		const (

eni watcher: add backoff-retry #1148

eni watcher: add backoff-retry #1148

Conversation

aaithal commented Dec 21, 2017 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

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

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

aaithal Dec 22, 2017 • 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

aaithal commented Jan 5, 2018

aaithal commented Dec 21, 2017 •

edited

Loading

aaithal Dec 22, 2017 •

edited

Loading