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

Limit concurrency in translations #6969

Merged
merged 10 commits into from
Jun 8, 2020
17 changes: 10 additions & 7 deletions readthedocs/api/v2/views/model_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import logging

from allauth.socialaccount.models import SocialAccount
from django.db.models import Q
from django.shortcuts import get_object_or_404
from django.template.loader import render_to_string
from rest_framework import decorators, permissions, status, viewsets
Expand Down Expand Up @@ -287,14 +288,16 @@ class BuildViewSetBase(UserSelectViewSet):
permission_classes=[permissions.IsAdminUser],
methods=['get'],
)
def running(self, request, **kwargs):
def concurrent(self, request, **kwargs):
project_slug = request.GET.get('project__slug')
queryset = (
self.get_queryset()
.filter(project__slug=project_slug)
.exclude(state__in=[BUILD_STATE_TRIGGERED, BUILD_STATE_FINISHED])
)
return Response({'count': queryset.count()})
project = get_object_or_404(Project, slug=project_slug)
humitos marked this conversation as resolved.
Show resolved Hide resolved
limit_reached, concurrent, max_concurrent = Build.objects.concurrent(project)
data = {
'limit_reached': limit_reached,
'concurrent': concurrent,
'max_concurrent': max_concurrent,
}
return Response(data)


class BuildViewSet(SettingsOverrideObject):
Expand Down
47 changes: 47 additions & 0 deletions readthedocs/builds/querysets.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,18 @@
"""Build and Version QuerySet classes."""
import logging

from django.conf import settings
from django.db import models
from django.db.models import Q

from readthedocs.core.utils.extend import SettingsOverrideObject
from readthedocs.projects import constants

from .constants import BUILD_STATE_TRIGGERED, BUILD_STATE_FINISHED


log = logging.getLogger(__name__)


__all__ = ['VersionQuerySet', 'BuildQuerySet', 'RelatedBuildQuerySet']

Expand Down Expand Up @@ -113,6 +121,45 @@ def api(self, user=None, detail=True):
queryset = self._add_user_repos(queryset, user)
return queryset.distinct()

def concurrent(self, project):
"""
Check if the max build concurrency for this project was reached.

- regular project: counts concurrent builds

- translation: concurrent builds of all the translations + builds of main project

:rtype: tuple
:returns: limit_reached, number of concurrent builds, number of max concurrent
"""
limit_reached = False
query = Q(project__slug=project.slug)

if project.main_language_project:
# Project is a translation, counts all builds of all the translations
query |= Q(project__main_language_project=project.main_language_project)
query |= Q(project__slug=project.main_language_project.slug)

elif project.translations.exists():
# The project has translations, counts their builds as well
query |= Q(project__in=project.translations.all())

concurrent = (
self.filter(query)
.exclude(state__in=[BUILD_STATE_TRIGGERED, BUILD_STATE_FINISHED])
).count()

max_concurrent = project.max_concurrent_builds or settings.RTD_MAX_CONCURRENT_BUILDS
log.info(
'Concurrent builds. project=%s running=%s max=%s',
project.slug,
concurrent,
max_concurrent,
)
if concurrent >= max_concurrent:
limit_reached = True
return (limit_reached, concurrent, max_concurrent)


class BuildQuerySet(SettingsOverrideObject):
_default_class = BuildQuerySetBase
Expand Down
75 changes: 75 additions & 0 deletions readthedocs/builds/tests/test_build_queryset.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import pytest

import django_dynamic_fixture as fixture
from django.conf import settings

from readthedocs.builds.querysets import BuildQuerySet
from readthedocs.builds.models import Build, Version
from readthedocs.projects.models import Project, Feature


@pytest.mark.django_db
class TestBuildQuerySet:

def test_concurrent_builds(self):
project = fixture.get(
Project,
max_concurrent_builds=None,
main_language_project=None,
)
for state in ('triggered', 'building', 'cloning', 'finished'):
fixture.get(
Build,
project=project,
state=state,
)
assert (False, 2, 4) == Build.objects.concurrent(project)
for state in ('building', 'cloning'):
fixture.get(
Build,
project=project,
state=state,
)
assert (True, 4, 4) == Build.objects.concurrent(project)

def test_concurrent_builds_project_limited(self):
project = fixture.get(
Project,
max_concurrent_builds=2,
main_language_project=None,
)
for state in ('triggered', 'building', 'cloning', 'finished'):
fixture.get(
Build,
project=project,
state=state,
)
assert (True, 2, 2) == Build.objects.concurrent(project)

def test_concurrent_builds_translations(self):
project = fixture.get(
Project,
max_concurrent_builds=None,
main_language_project=None,
)
translation = fixture.get(
Project,
max_concurrent_builds=None,
main_language_project=project,
)
for state in ('triggered', 'building', 'cloning', 'finished'):
fixture.get(
Build,
project=project,
state=state,
)
assert (False, 2, 4) == Build.objects.concurrent(translation)

for state in ('building', 'cloning'):
fixture.get(
Build,
project=translation,
state=state,
)
assert (True, 4, 4) == Build.objects.concurrent(translation)
assert (True, 4, 4) == Build.objects.concurrent(project)
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
from readthedocs.projects.models import Project, Feature



class TaskRouterTests(TestCase):

def setUp(self):
Expand Down
9 changes: 2 additions & 7 deletions readthedocs/core/utils/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,13 +152,8 @@ def prepare_build(

# Start the build in X minutes and mark it as limited
if project.has_feature(Feature.LIMIT_CONCURRENT_BUILDS):
running_builds = (
Build.objects
.filter(project__slug=project.slug)
.exclude(state__in=[BUILD_STATE_TRIGGERED, BUILD_STATE_FINISHED])
)
max_concurrent_builds = project.max_concurrent_builds or settings.RTD_MAX_CONCURRENT_BUILDS
if running_builds.count() >= max_concurrent_builds:
limit_reached, _, max_concurrent_builds = Build.objects.concurrent(project)
if limit_reached:
log.warning(
'Delaying tasks at trigger step due to concurrency limit. project=%s version=%s',
project.slug,
Expand Down
3 changes: 2 additions & 1 deletion readthedocs/doc_builder/environments.py
Original file line number Diff line number Diff line change
Expand Up @@ -1062,8 +1062,9 @@ def create_container(self):
client = self.get_client()
try:
log.info(
'Creating Docker container: image=%s',
'Creating Docker container: image=%s id=%s',
self.container_image,
self.container_id,
)
self.container = client.create_container(
image=self.container_image,
Expand Down
27 changes: 13 additions & 14 deletions readthedocs/projects/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -500,19 +500,18 @@ def run(
self.config = None

if self.project.has_feature(Feature.LIMIT_CONCURRENT_BUILDS):
response = api_v2.build.running.get(project__slug=self.project.slug)
builds_running = response.get('count', 0)
max_concurrent_builds = (
self.project.max_concurrent_builds or
settings.RTD_MAX_CONCURRENT_BUILDS
)
log.info(
'Concurrent builds: max=%s running=%s project=%s',
max_concurrent_builds,
builds_running,
self.project.slug,
)
if builds_running >= max_concurrent_builds:
try:
response = api_v2.build.concurrent_limit.get(project__slug=self.project.slug)
concurrency_limit_reached = response.get('limit_reached', False)
max_concurrent_builds = response.get(
'max_concurrent',
settings.RTD_MAX_CONCURRENT_BUILDS,
)
except Exception:
concurrency_limit_reached = False
humitos marked this conversation as resolved.
Show resolved Hide resolved
max_concurrent_builds = settings.RTD_MAX_CONCURRENT_BUILDS

if concurrency_limit_reached:
log.warning(
'Delaying tasks due to concurrency limit. project=%s version=%s',
self.project.slug,
Expand Down Expand Up @@ -2198,7 +2197,7 @@ def send_build_status(build_pk, commit, status):
)
except RemoteRepository.DoesNotExist:
log.warning(
'Project does not have a RemoteRepository. project= %s',
'Project does not have a RemoteRepository. project=%s',
build.project.slug,
)

Expand Down
26 changes: 26 additions & 0 deletions readthedocs/rtd_tests/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -705,6 +705,32 @@ def test_init_api_project(self):
self.assertFalse(api_project.show_advertising)
self.assertEqual(api_project.environment_variables, {'TOKEN': 'a1b2c3'})

def test_concurrent_builds(self):
expected = {
'limit_reached': False,
'concurrent': 2,
'max_concurrent': 4,
}
user = get(User, is_staff=True)
project = get(
Project,
max_concurrent_builds=None,
main_language_project=None,
)
for state in ('triggered', 'building', 'cloning', 'finished'):
get(
Build,
project=project,
state=state,
)

client = APIClient()
client.force_authenticate(user=user)

resp = client.get(f'/api/v2/build/concurrent/', data={'project__slug': project.slug})
self.assertEqual(resp.status_code, 200)
self.assertDictEqual(expected, resp.data)


class APIImportTests(TestCase):

Expand Down
2 changes: 1 addition & 1 deletion readthedocs/rtd_tests/tests/test_privacy_urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ def setUp(self):
'api_webhook': {'integration_pk': self.integration.pk},
}
self.response_data = {
'build-running': {'status_code': 403},
'build-concurrent': {'status_code': 403},
'project-sync-versions': {'status_code': 403},
'project-token': {'status_code': 403},
'emailhook-list': {'status_code': 403},
Expand Down