-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Limit concurrent builds #6847
Merged
Merged
Limit concurrent builds #6847
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
4313e49
Limit concurrent builds
humitos 2562d30
Update readthedocs/doc_builder/exceptions.py
humitos 36a669a
Lint
humitos 01d7a03
Test
humitos 5c2ca2e
Merge branch 'humitos/max-builds-concurrency' of github.com:readthedo…
humitos 301f35c
Remove BUILD_STATE_QUEUED
humitos 8015674
Increase max_retries for builds reached concurrency limit
humitos File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
# Generated by Django 2.2.11 on 2020-04-01 20:44 | ||
|
||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
('builds', '0015_uploading_build_state'), | ||
] | ||
|
||
operations = [ | ||
migrations.AlterField( | ||
model_name='build', | ||
name='state', | ||
field=models.CharField(choices=[('triggered', 'Triggered'), ('queued', 'Queued'), ('cloning', 'Cloning'), ('installing', 'Installing'), ('building', 'Building'), ('uploading', 'Uploading'), ('finished', 'Finished')], default='finished', max_length=55, verbose_name='State'), | ||
), | ||
] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,7 @@ | |
|
||
from readthedocs.api.v2.client import api as api_v2 | ||
from readthedocs.builds.constants import ( | ||
BUILD_STATE_QUEUED, | ||
BUILD_STATE_BUILDING, | ||
BUILD_STATE_CLONING, | ||
BUILD_STATE_FINISHED, | ||
|
@@ -57,6 +58,7 @@ | |
from readthedocs.doc_builder.exceptions import ( | ||
BuildEnvironmentError, | ||
BuildEnvironmentWarning, | ||
BuildMaxConcurrencyError, | ||
BuildTimeoutError, | ||
MkDocsYAMLParseError, | ||
ProjectBuildsSkippedError, | ||
|
@@ -510,6 +512,26 @@ def run( | |
self.commit = commit | ||
self.config = None | ||
|
||
if self.project.has_feature(Feature.LIMIT_CONCURRENT_BUILDS): | ||
response = api_v2.build.running.get(project__slug=self.project.slug) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could use APIv3 here (https://docs.readthedocs.io/en/stable/api/v3.html#builds-listing) if we add a new permission class to allow build user to list builds. As this will require extra work and we are only using APIv2 from builders, I didn't want to mix them here. |
||
if response.get('count', 0) >= settings.RTD_MAX_CONCURRENT_BUILDS: | ||
log.warning( | ||
'Delaying tasks due to concurrency limit. project=%s version=%s', | ||
self.project.slug, | ||
self.version.slug, | ||
) | ||
|
||
# This is done automatically on the environment context, but | ||
# we are executing this code before creating one | ||
api_v2.build(self.build['id']).patch({ | ||
'error': BuildMaxConcurrencyError.message.format( | ||
limit=settings.RTD_MAX_CONCURRENT_BUILDS, | ||
), | ||
'state': BUILD_STATE_QUEUED, | ||
}) | ||
self.task.retry(exc=BuildMaxConcurrencyError, throw=False) | ||
humitos marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return False | ||
|
||
# Build process starts here | ||
setup_successful = self.run_setup(record=record) | ||
if not setup_successful: | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This case is where I had issues with overloading the build state to communicate build step to the user in the UI. There are likely other queries/methods we need to update whenever we add build states like this -- for instance, anything checking just for
BUILD_STATE_TRIGGERED
and should be also checking for our new state,BUILD_STATE_QUEUED
?This code is correct, but it's hard to know what fallout could be without some thorough testing. Queries like these should probably be QuerySet methods so that we're not continually reproducing logic that we need to update.
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 don't have a strong opinion here, but a slight preference to add a new state. I just wanted to mention why I thought it could be useful.
The difference in UI is that going to the Build list page, you can have a big picture about what's happening: Triggered, Queued, Building, Passed, Failed. Where Triggered means that you are waiting for your turn to build, while Queued means that you have reached the concurrency limit: "those builds are waiting "because of you" (in some way), not us.
Having too many builds in Triggered could cause the user to contact us because they think we are not processing their builds: "the waiting time is too high", where in fact, they have reached the limit.
On the other hand, we use
Build.is_stale
to show a small "Warning" icon in the build list if the build has been in Triggered state for more than 5 minutes. I'd not show this icon for Queued state.Finally, I did a quick grep trying to find filters via
BUILD_STATE_TRIGGERED
and I didn't find anything important. Although, forBUILD_STATE_FINISHED
I found this,readthedocs.org/readthedocs/api/v3/filters.py
Lines 41 to 45 in ee403c5
I agree we could move them to a QuerySet and avoid repetition, though.
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 say lets keep it Triggered for now, and we can adjust as we go.
There are ways to change the UX for the user without using the state, if needed, or audit all the queries. We need to do additional work before we ship it for users anyway.
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.
Good points! Some of this felt like core team opinions, so I got feedback on the UI changes. What it came back to was that builds should be in "Queued" state if the user has to wait for the build -- regardless of whether we introduce a delay due to concurrency limits, or the build is queued because of build backup. "Triggered" doesn't communicate the same thing as "Queued".
So, I think my opinion is that adding "Queued" is not a problem, but all builds should become "Queued" state when they are put into the build queue. This might leave no room for "Triggered" state if so, and so probably back to my original point, should be dropped in favor of "Queued".
For now, I'd agree we can leave it "Triggered" and move more deliberately to clearer language for the users. We can add more UI later and guide our UI decisions based on our technical implementation.
Here are the feedback notes:
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've updated this PR and removed the QUEUED state for now. We can come back to this discussion later if needed.