Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Add rate limit handling for GitHub client #4926

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat: Add rate limit handling for GitHub client #4926

wants to merge 5 commits into from
+64 −3

Conversation

itays333
Copy link

@itays333 itays333 commented Sep 16, 2024 •
edited by nitrocode
Loading

What

  • Add retry mechanism for GitHub API secondary rate limit
  • Implement a retry mechanism for GitHub client API calls when encountering secondary rate limit responses.

Why

  • In large-scale deployments, a single PR can affect hundreds of projects.
  • When attempting to plan/apply for a non-mergeable PR, Atlantis may try to set the PR status hundreds of times in a short period, triggering GitHub's secondary API rate limit.
  • This results in Atlantis appearing to stop mid-operation: some projects are marked as failed, and no comments are posted.
  • This issue can occur in other scenarios where Atlantis is used at scale.

Implementation

  • Utilize the go-github-ratelimit library, as recommended in the official go-github README:

    "You can use go-github-ratelimit to handle secondary rate limit sleep-and-retry for you."

Testing

  • Added a test that verifies an API request completes successfully after responding with API secondary rate limit errors multiple times.

References

@itays333 itays333 requested review from a team as code owners September 16, 2024 08:39
@itays333 itays333 requested review from jamengual, lukemassa and nitrocode and removed request for a team September 16, 2024 08:39
@github-actions github-actions bot added dependencies PRs that update a dependency file go Pull requests that update Go code provider/github labels Sep 16, 2024
itays333
itays333 commented Sep 16, 2024
View reviewed changes
server/events/vcs/github_client.go
@@ -121,23 +122,28 @@ func NewGithubClient(hostname string, credentials GithubCredentials, config Gith
return nil, errors.Wrap(err, "error initializing github authentication transport")
}

transportWithRateLimit, err := github_ratelimit.NewRateLimitWaiterClient(transport.Transport, github_ratelimit.WithTotalSleepLimit(time.Minute, nil))
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regarding WithTotalSleepLimit(time.Minute, nil) - not sure how long we want to keep retry, and if this should be configurable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are trying to limit as much as possible adding more flags, so maybe we can add this to the repo.Yaml instead.

@chenrui333

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is something like this acceptable? open to suggestions

...
git:
  github:
     secondary_rate_limit_duration: 60

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, repo config would be a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I suggest starting with the hard-coded 1 minute value that you currently have, so that we can get this PR finished. It can always be refined later to make it configurable.

X-Guardian
X-Guardian reviewed Nov 2, 2024
View reviewed changes
server/events/vcs/github_client.go
@@ -121,23 +122,28 @@ func NewGithubClient(hostname string, credentials GithubCredentials, config Gith
return nil, errors.Wrap(err, "error initializing github authentication transport")
}

transportWithRateLimit, err := github_ratelimit.NewRateLimitWaiterClient(transport.Transport, github_ratelimit.WithTotalSleepLimit(time.Minute, nil))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding a callback to these that log an info message so that we can see when they are being triggered?

server/events/vcs/github_client.go
@@ -121,23 +122,28 @@ func NewGithubClient(hostname string, credentials GithubCredentials, config Gith
return nil, errors.Wrap(err, "error initializing github authentication transport")
}

transportWithRateLimit, err := github_ratelimit.NewRateLimitWaiterClient(transport.Transport, github_ratelimit.WithTotalSleepLimit(time.Minute, nil))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I suggest starting with the hard-coded 1 minute value that you currently have, so that we can get this PR finished. It can always be refined later to make it configurable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chenrui333 chenrui333 chenrui333 left review comments

@X-Guardian X-Guardian X-Guardian left review comments

@jamengual jamengual Awaiting requested review from jamengual jamengual is a code owner automatically assigned from runatlantis/maintainers

@lukemassa lukemassa Awaiting requested review from lukemassa lukemassa is a code owner automatically assigned from runatlantis/core-contributors

@nitrocode nitrocode Awaiting requested review from nitrocode nitrocode is a code owner automatically assigned from runatlantis/core-contributors

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
dependencies PRs that update a dependency file go Pull requests that update Go code provider/github
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@itays333 @chenrui333 @jamengual @X-Guardian