-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use retry logic for HTTP communication with IMDS #107
Conversation
10f8dae
to
7b58355
Compare
Do we know how much this increases the final build size of azure-init? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how much we gain from using azure sdk vs just implementing our own retry policy here. Can you verify the final code size?
+66%
+49% Tried also turning off default features of Azure SDK crates, but not much difference. |
7b58355
to
b04a3c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on the fence about the SDK - the lack of error codes to retry seems like it might be a deal-breaker. Since we want a fixed retry policy without jitter and so on, the implementation could be a loop with a constant sleep in it, wrapped in https://docs.rs/tokio/latest/tokio/time/fn.timeout.html.
Ok, let me rewrite this PR based on https://docs.rs/tokio/latest/tokio/time/fn.timeout.html or any simpler ones. |
b04a3c2
to
c4d8a9a
Compare
c4d8a9a
to
a4559b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few very minor notes, but it looks good to me. You should also be able to write a unit test for the timeout case, I think, with https://docs.rs/tokio/latest/tokio/time/fn.advance.html although I've not personally done this before so I'm just guessing based on the documentation.
0cf0ba6
to
86085a1
Compare
Added unit tests using |
86085a1
to
7a5e641
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks! I had two little optional notes, but this is good to go from my perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the most part this LGTM, but I do want @anhvoms thoughts on exponentially increasing the retry interval when making repeated calls to IMDS before approving these changes.
let imds_body = response.text().await?; | ||
let metadata: InstanceMetadata = serde_json::from_str(&imds_body)?; | ||
if statuscode.is_success() && statuscode == StatusCode::OK { | ||
return response.error_for_status(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would error_for_status()
return here given that the status code is a success?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, it would just return the inner response of type Response
itself, where the outer response
would be Ok
.
Use retry logic and max duration for HTTP communication with IMDS, creating a retry loop over getting response from IMDS. Doing that, it is possible for azure-init to retry to send HTTP requests, when requests failed for some reason. For now we define a fixed const for timeout, max retries for simplicity. Make imds::query() capable of receiving input URL, so that both main binary and unit tests can send requests to different endpoints.
7a5e641
to
c6f3627
Compare
Use retry logic and max duration for HTTP communication with IMDS, creating a generic wrapper retry_loop.
Doing that, it is possible for azure-init to retry to send HTTP requests, when requests failed for some reason.
For now we define a fixed const for timeout, max retries, for simplicity.
Make imds::query() capable of receiving input URL, so that both main binary and unit tests can send requests to different endpoints.
Fixes #63
Testing done
TBD