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.

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

Treat resource types as checkable in rate limiter #7211

Merged
merged 1 commit into from Jun 24, 2021

Conversation

taylorsilva
Copy link
Member

@taylorsilva taylorsilva commented Jun 24, 2021

Changes proposed by this PR

Follow-up from #7102
Will be made irrelevant by #7176

check builds are created for resources and custom resource types. The
resource types were not being considered in the rate limit, so as soon
as you have any custom resource types there would always be check builds
that would get stuck by the rate limiter.

check builds are created for resources and custom resource types. The
resource types were not being considered in the rate limit, so as soon
as you have any custom resource types there would always be check builds
that would get stuck by the rate limiter.

Signed-off-by: Taylor Silva <tsilva@pivotal.io>
Co-authored-by: Bohan Chen <bochen@pivotal.io>
@taylorsilva taylorsilva requested a review from a team as a code owner June 24, 2021 16:18
@chenbh chenbh changed the title B: rate limiter treats resource types as checkable Treat resource types as checkable in rate limiter Jun 24, 2021
@aoldershaw aoldershaw self-assigned this Jun 24, 2021
@aoldershaw
Copy link
Contributor

Probably don't need the release note as the bug didn't exist in 7.3

@aoldershaw
Copy link
Contributor

Just based on the duration of testflight, it doesn't look like this totally fixes the issue - notice how before #7102 was merged, testflight took ~20mins (https://ci.concourse-ci.org/teams/main/pipelines/concourse/jobs/testflight/builds/822), and after, it took ~40mins (or timed out) (https://ci.concourse-ci.org/teams/main/pipelines/concourse/jobs/testflight/builds/824).

This testflight run is currently at 40mins and counting

@taylorsilva
Copy link
Member Author

Just noticed the timing too. Seems like we still got a mystery to solve 🤔

@aoldershaw
Copy link
Contributor

The following test took ~15m:

It("will run both jobs only once even with a new custom resource type version", func() {

I've seen the same thing without this fix:
https://ci.concourse-ci.org/teams/main/pipelines/branch/jobs/testflight/builds/17.2?vars.branch=%22resource-causality%22#L60ced7f3:503:509

@taylorsilva taylorsilva merged commit d7346bf into master Jun 24, 2021
@taylorsilva taylorsilva deleted the fix-rate-limiter branch June 24, 2021 17:48
@taylorsilva
Copy link
Member Author

We noticed these tests were taking over 400s to finish before:

var _ = Describe("Configuring a resource type in a pipeline config", func() {

On my machine they could run under 100s. From this PR's checks they took under 400s now but still very slow.

@taylorsilva taylorsilva added the release/no-impact This is an issue that never affected released versions (i.e. a regression caught prior to shipping). label Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug misc release/no-impact This is an issue that never affected released versions (i.e. a regression caught prior to shipping).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants