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

Add basic support for lower priority PR builds #6921

Merged
merged 8 commits into from Apr 27, 2020
9 changes: 9 additions & 0 deletions readthedocs/core/utils/__init__.py
Expand Up @@ -18,6 +18,7 @@
EXTERNAL,
)
from readthedocs.doc_builder.constants import DOCKER_LIMITS
from readthedocs.projects.constants import CELERY_LOW, CELERY_MEDIUM, CELERY_HIGH
from readthedocs.doc_builder.exceptions import BuildMaxConcurrencyError


Expand Down Expand Up @@ -150,6 +151,14 @@ def prepare_build(
# Send Webhook notification for build triggered.
send_notifications.delay(version.pk, build_pk=build.pk, email=False)

options['priority'] = CELERY_HIGH
if project.main_language_project:
# Translations should be medium priority
options['priority'] = CELERY_MEDIUM
if version.type == EXTERNAL:
Copy link
Member

Choose a reason for hiding this comment

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

Want we check for being a translation here before checking if external and assigning CELERY_MEDIUM?

# External builds should be lower priority.
options['priority'] = CELERY_LOW

# Start the build in X minutes and mark it as limited
if project.has_feature(Feature.LIMIT_CONCURRENT_BUILDS):
running_builds = (
Expand Down
6 changes: 6 additions & 0 deletions readthedocs/projects/constants.py
Expand Up @@ -374,3 +374,9 @@
# Git provider names
GITHUB_BRAND = 'GitHub'
GITLAB_BRAND = 'GitLab'

# Set 3 priorities, [low, medium, high] -- default is medium
# Leave some space on each side of the set to expand if needed
CELERY_LOW = 3
CELERY_MEDIUM = 5
CELERY_HIGH = 7
Comment on lines +378 to +382
Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe these fit better in builds.contants?

57 changes: 56 additions & 1 deletion readthedocs/rtd_tests/tests/test_core_utils.py
Expand Up @@ -13,14 +13,15 @@
from readthedocs.core.utils import slugify, trigger_build, prepare_build
from readthedocs.core.utils.general import wipe_version_via_slugs
from readthedocs.doc_builder.exceptions import BuildMaxConcurrencyError
from readthedocs.projects.constants import CELERY_LOW, CELERY_MEDIUM, CELERY_HIGH
from readthedocs.projects.models import Project, Feature
from readthedocs.projects.tasks import remove_dirs


class CoreUtilTests(TestCase):

def setUp(self):
self.project = get(Project, container_time_limit=None)
self.project = get(Project, container_time_limit=None, main_language_project=None)
self.version = get(Version, project=self.project)

@mock.patch('readthedocs.projects.tasks.update_docs_task')
Expand Down Expand Up @@ -101,6 +102,7 @@ def test_trigger_custom_queue(self, update_docs):
'queue': 'build03',
'time_limit': 720,
'soft_time_limit': 600,
'priority': CELERY_HIGH,
}
update_docs.signature.assert_called_with(
args=(self.version.pk,),
Expand All @@ -123,6 +125,7 @@ def test_trigger_build_time_limit(self, update_docs):
'queue': mock.ANY,
'time_limit': 720,
'soft_time_limit': 600,
'priority': CELERY_HIGH,
}
update_docs.signature.assert_called_with(
args=(self.version.pk,),
Expand All @@ -146,6 +149,7 @@ def test_trigger_build_invalid_time_limit(self, update_docs):
'queue': mock.ANY,
'time_limit': 720,
'soft_time_limit': 600,
'priority': CELERY_HIGH,
}
update_docs.signature.assert_called_with(
args=(self.version.pk,),
Expand All @@ -169,6 +173,7 @@ def test_trigger_build_rounded_time_limit(self, update_docs):
'queue': mock.ANY,
'time_limit': 3,
'soft_time_limit': 3,
'priority': CELERY_HIGH,
Copy link
Member

Choose a reason for hiding this comment

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

We should modify/add a test that triggers a PR and check it's CELERY_LOW.

}
update_docs.signature.assert_called_with(
args=(self.version.pk,),
Expand All @@ -177,6 +182,7 @@ def test_trigger_build_rounded_time_limit(self, update_docs):
immutable=True,
)


@mock.patch('readthedocs.projects.tasks.update_docs_task')
def test_trigger_max_concurrency_reached(self, update_docs):
get(
Expand Down Expand Up @@ -208,6 +214,7 @@ def test_trigger_max_concurrency_reached(self, update_docs):
'soft_time_limit': 600,
'countdown': 5 * 60,
'max_retries': 25,
'priority': CELERY_HIGH,
}
update_docs.signature.assert_called_with(
args=(self.version.pk,),
Expand All @@ -218,6 +225,54 @@ def test_trigger_max_concurrency_reached(self, update_docs):
build = self.project.builds.first()
self.assertEqual(build.error, BuildMaxConcurrencyError.message.format(limit=max_concurrent_builds))

@mock.patch('readthedocs.projects.tasks.update_docs_task')
def test_trigger_external_build_low_priority(self, update_docs):
"""Time limit should round down."""
self.version.type = 'external'
trigger_build(project=self.project, version=self.version)
kwargs = {
'record': True,
'force': False,
'build_pk': mock.ANY,
'commit': None
}
options = {
'queue': mock.ANY,
'time_limit': mock.ANY,
'soft_time_limit': mock.ANY,
'priority': CELERY_LOW,
}
update_docs.signature.assert_called_with(
args=(self.version.pk,),
kwargs=kwargs,
options=options,
immutable=True,
)

@mock.patch('readthedocs.projects.tasks.update_docs_task')
def test_trigger_build_translation_medium_priority(self, update_docs):
"""Time limit should round down."""
self.project.main_language_project = get(Project, slug='main')
trigger_build(project=self.project, version=self.version)
kwargs = {
'record': True,
'force': False,
'build_pk': mock.ANY,
'commit': None
}
options = {
'queue': mock.ANY,
'time_limit': mock.ANY,
'soft_time_limit': mock.ANY,
'priority': CELERY_MEDIUM,
}
update_docs.signature.assert_called_with(
args=(self.version.pk,),
kwargs=kwargs,
options=options,
immutable=True,
)

def test_slugify(self):
"""Test additional slugify."""
self.assertEqual(
Expand Down
7 changes: 7 additions & 0 deletions readthedocs/settings/base.py
Expand Up @@ -6,6 +6,7 @@
from celery.schedules import crontab

from readthedocs.core.settings import Settings
from readthedocs.projects.constants import CELERY_LOW, CELERY_MEDIUM, CELERY_HIGH


try:
Expand Down Expand Up @@ -332,6 +333,12 @@ def USE_PROMOS(self): # noqa
CELERYD_PREFETCH_MULTIPLIER = 1
CELERY_CREATE_MISSING_QUEUES = True


BROKER_TRANSPORT_OPTIONS = {
'queue_order_strategy': 'priority',
'priority_steps': [CELERY_LOW, CELERY_MEDIUM, CELERY_HIGH],
}

CELERY_DEFAULT_QUEUE = 'celery'
CELERYBEAT_SCHEDULE = {
# Ran every hour on minute 30
Expand Down