eni watcher: retry in first reconciliation loop #2581
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shubham2892
approved these changes
Aug 19, 2020
yhlee-aws
reviewed
Aug 19, 2020
agent/eni/watcher/watcher_linux.go
Outdated
| // This can happen in two scenarios: (1) the ENI is indeed not managed by ECS (i.e. attached manually | ||
| // by customer); (2) this is an ENI attached by ECS but we have not yet received its information from | ||
| // ACS. | ||
| log.Debugf("Not sending state change because the ENI is not managed by ECS: %v", sendErr) |
There was a problem hiding this comment.
nit: is "ENI is not managed by ECS" message valid if it's the second case? i think it might create customer confusion.
There was a problem hiding this comment.
i updated it to "Not sending state change because we don't know about the ENI". not sure if that sounds better?
yhlee-aws
approved these changes
Aug 19, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Let eni watcher retry acknowledging ENI in its first reconciliation.
Under certain scenario, the ENI is attached before agent starts and connects to ACS, and as a result, when the watcher detects the ENI in the first reconciliation loop we did not acknowledge it. This causes us to wait for the next loop, causing 25s+ delay in acknowledging the ENI. Adding retry fixes the delay.
Implementation details
Update
reconcileOncemethod to take an argument on whether to retry or not, and retry in the first reconciliation.Testing
Added unit test. Manual test has been performed to make sure the delay of confirming ENI attachment is addressed by the change.
New tests cover the changes: yes
Description for the changelog
N/A
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.