-
Notifications
You must be signed in to change notification settings - Fork 41
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
rate-limit brupop agent calls to the Bottlerocket update API #496
Conversation
}, | ||
); | ||
} | ||
match bottlerocket_shadows |
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.
Nice!
agent/src/apiclient.rs
Outdated
pub(super) async fn invoke_apiclient( | ||
args: Vec<String>, | ||
rate_limiter: Option<&SimpleRateLimiter>, | ||
) -> Result<Output> { | ||
let mut attempts: i8 = 0; | ||
// Retry up to 5 times in case the Update API is busy; Waiting 10 seconds between each attempt. | ||
while attempts < MAX_ATTEMPTS { |
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.
Is there a reason not to use Retry::spawn
?
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 suppose not. Looking at this, I also wonder if we should be retrying any error with the API (e.g. what if the apiserver is being restarted).
This hides away the details of the Bottlerocket API in a private module, then renames the public functions to be more descriptive. This commit also `cargo fmt`s the agent crate.
|
Issue number:
#483
Description of changes:
This PR is probably best reviewed commit-by-commit. It appears large, but most of the bigger changes are just simple refactors.
There were reports in #483 of Brupop being unable to update hosts, entering the agent's crash loop. Sometimes the updates would end up succeeding, but sometimes they would become stuck.
Upon inspection of the brupop logs and systemd journal, it seemed like even under conditions where brupop succeeded its updates, it would often be stalled by the apiserver due to the updates lock being held. Brupop would apparently starve itself of this lock by refreshing updates too often and calling the API too quickly.
This change adds ratelimiting to the update-related API calls. It also refactors a few portions of the agent.
Testing done:
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.