Add adaptive retry logic in RCI call for non-terminal errors #4499
Conversation
tshan2001
force-pushed
the
master
branch
2 times, most recently
from
99cedbd to
34b63af
Compare
| defer cancel() | ||
| err := retry.RetryWithBackoffCtx(ctx, backoff, | ||
| func() error { | ||
| containerInstanceARN, availabilityZone, errFromRCI = client.registerContainerInstance( |
There was a problem hiding this comment.
Question: Don't we already use the default retryer from the AWS SDK under the hood in this call? That would mean that we perform additional retries on the actual network call.
Is this intentional?
There was a problem hiding this comment.
Synced offline. Adding a summary here: the aws sdk retry only retries for 3 times and probably under 10 seconds, to address potential network issues. This change is to add more control over the overall initialization workflow. Without this, after the default 3 retries, the agent would exit and restart, and after around 3 seconds it repeats the same process again, we're essentially retrying 3 times every 15 seconds. We want to add more control in this process, where the backoff we add can go up to 3 minutes. This is to help alleviate systematic account-level throttling.
| err := retry.RetryWithBackoffCtx(ctx, client.rciRetryBackoff, | ||
| func() error { | ||
| // Reset the backoff such that retries from past calls won't impact the current call. | ||
| client.rciRetryBackoff.Reset() |
There was a problem hiding this comment.
We want to reset it for every retry? Will we still get exponential backoffs? How?
There was a problem hiding this comment.
Thanks for the catch! Published a new revision to move this outside of the sub-function within the retry loop
Summary
Add retry with exponential backoff when receiving non-terminal error from RCI calls, to prevent retry-storms.
Implementation details
A wrapper
registerContainerInstanceWithRetryis added around the originalregisterContainerInstancemethod. It utilizes theRetryWithBackoffCtxmethod from the retry package. Upon receiving failures from RCI, we will examine the error type to determine if it's a terminal error, if yes, we will break the retry loop, otherwise, we will continue to retry with increased backoff time. The max backoff time is capped at ~3 minutes to ensure we don't wait too long between retries.Testing
A new test
TestRegisterContainerInstanceWithRetryNonTerminalErrorhas been added to test both the happy and unhappy case.New tests cover the changes: yes
Description for the changelog
Add adaptive retry logic in RCI call for non-terminal errors.
Additional Information
Does this PR include breaking model changes? If so, Have you added transformation functions?
Does this PR include the addition of new environment variables in the README?
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.