SDK: Adjust client RetryMax to not exceed timeout #1072
Conversation
github-actions
bot
added
the
release-once-merged
There was a problem hiding this comment.
Hi @magodo, thanks for looking at this, we can certainly improve our handling here to accommodate a wider range of deadlines. I've plotted the resulting times and they match what one would generally expect to see - minus network delays.
Network delay is the first issue with trying to extrapolate the time available, perhaps we should be slightly more conservative with these timings for safety? That won't account for things like DNS delays though, which can add seconds to the roundtrip.
The other problem is that the exponential backoff is only a fallback - in many (most?) cases, the API returns a Retry-After header, and all these timing go right out the window.
I am wondering if the only way to avoid the context expiring is to catch it before invoking the next retried request. We could extend RetryFunc to receive the context from the configured CheckRetry, then it can determine whether the next retry should take place?
I still think we can merge this, but I don't consider this fully solved just yet 🙂
This PR modifies the logic about how the
client.RetryMaxis deduced. The main motivation is to avoid the retry exceeds the context dead line (if any).This is useful for
terraform-provider-azurermusers to get a useful error message instead of a useless "context deadline exceeded" error message, especially when doing aterraform plan, given all resources' read timeout is defined as 5min.Reference
hashicorp/terraform-provider-azurerm#21464
Example
Given a storage account with PE correctly setup, while running in an errorneous network environment, that failed to resolve the storage blob data plane domain name. With this implementation, the retry log is as below: