From 061066a21e80e05e553d7e18a84dcbf0ee67157b Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 8 May 2019 16:16:11 -0500 Subject: [PATCH 01/22] Fix tests Mock new tasks --- readthedocs/rtd_tests/tests/test_privacy.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_privacy.py b/readthedocs/rtd_tests/tests/test_privacy.py index bac259253ae..e0c3565f721 100644 --- a/readthedocs/rtd_tests/tests/test_privacy.py +++ b/readthedocs/rtd_tests/tests/test_privacy.py @@ -1,5 +1,3 @@ -# -*- coding: utf-8 -*- -import json import logging import mock @@ -9,7 +7,6 @@ from readthedocs.builds.constants import LATEST from readthedocs.builds.models import Build, Version -from readthedocs.projects import tasks from readthedocs.projects.forms import UpdateProjectForm from readthedocs.projects.models import Project @@ -17,6 +14,8 @@ log = logging.getLogger(__name__) +@mock.patch('readthedocs.projects.tasks.clean_build_task.signature', new=mock.MagicMock) +@mock.patch('readthedocs.projects.tasks.update_docs_task.signature', new=mock.MagicMock) class PrivacyTests(TestCase): def setUp(self): @@ -28,8 +27,6 @@ def setUp(self): self.tester.set_password('test') self.tester.save() - tasks.update_docs_task.delay = mock.Mock() - def _create_kong( self, privacy_level='private', version_privacy_level='private', From 11debc3b224dad60084bf8db063f6ce4b377c789 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 8 May 2019 16:17:47 -0500 Subject: [PATCH 02/22] Add task to remove files after build --- readthedocs/core/utils/__init__.py | 23 ++++++++++++++--------- readthedocs/projects/tasks.py | 14 ++++++++++++++ 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index d6b15a602f2..06039bd2a03 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -2,22 +2,21 @@ """Common utilty functions.""" -from __future__ import absolute_import - import errno import logging import os import re +from celery import chain, chord, group from django.conf import settings from django.utils.functional import keep_lazy from django.utils.safestring import SafeText, mark_safe from django.utils.text import slugify as slugify_base -from celery import group, chord from readthedocs.builds.constants import BUILD_STATE_TRIGGERED from readthedocs.doc_builder.constants import DOCKER_LIMITS + log = logging.getLogger(__name__) SYNC_USER = settings.SYNC_USER @@ -83,7 +82,7 @@ def prepare_build( # Avoid circular import from readthedocs.builds.models import Build from readthedocs.projects.models import Project - from readthedocs.projects.tasks import update_docs_task + from readthedocs.projects.tasks import clean_build_task, update_docs_task build = None @@ -132,11 +131,17 @@ def prepare_build( options['time_limit'] = int(time_limit * 1.2) return ( - update_docs_task.signature( - args=(project.pk,), - kwargs=kwargs, - options=options, - immutable=True, + chain( + update_docs_task.signature( + args=(project.pk,), + kwargs=kwargs, + options=options, + immutable=True, + ), + clean_build_task.signature( + args=(version.pk,), + inmutable=True, + ) ), build, ) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index a240770f3fb..4e6c7b54612 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -1310,6 +1310,20 @@ def warn(self, msg): delete_queryset.delete() +@app.task(max_retries=5, default_retry_delay=7 * 60) +def clean_build_task(version_id): + """Clean the build artifacts of the given version.""" + version = Version.objects.get_object_or_log(pk=version_id) + if not version: + return + del_dirs = [ + os.path.join(version.project.doc_path, dir_, version.slug) + for dir_ in ('checkout', 'envs', 'conda') + ] + for del_dir in del_dirs: + broadcast(type='build', task=remove_dirs, args=[(del_dir,)]) + + def _manage_imported_files(version, path, commit): """ Update imported files for version. From 0a6445edb9621256f28f673e1a9ae8d4a3831704 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 8 May 2019 16:18:57 -0500 Subject: [PATCH 03/22] Fix more tests Mock new task --- .../rtd_tests/tests/test_core_utils.py | 79 ++++++++++++++----- 1 file changed, 61 insertions(+), 18 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_core_utils.py b/readthedocs/rtd_tests/tests/test_core_utils.py index d69dad3b20f..57516576d60 100644 --- a/readthedocs/rtd_tests/tests/test_core_utils.py +++ b/readthedocs/rtd_tests/tests/test_core_utils.py @@ -1,20 +1,19 @@ -# -*- coding: utf-8 -*- """Test core util functions.""" import os -import mock -from mock import call +import mock from django.http import Http404 from django.test import TestCase from django_dynamic_fixture import get +from mock import call +from readthedocs.builds.constants import LATEST from readthedocs.builds.models import Version -from readthedocs.core.utils.general import wipe_version_via_slugs -from readthedocs.projects.tasks import remove_dirs from readthedocs.core.utils import slugify, trigger_build +from readthedocs.core.utils.general import wipe_version_via_slugs from readthedocs.projects.models import Project -from readthedocs.builds.constants import LATEST +from readthedocs.projects.tasks import remove_dirs class CoreUtilTests(TestCase): @@ -64,7 +63,7 @@ def test_trigger_build_when_version_not_provided_default_version_exist(self, upd immutable=True, ), ]) - + @mock.patch('readthedocs.projects.tasks.update_docs_task') def test_trigger_build_when_version_not_provided_default_version_doesnt_exist(self, update_docs_task): @@ -90,8 +89,10 @@ def test_trigger_build_when_version_not_provided_default_version_doesnt_exist(se ), ]) + @mock.patch('readthedocs.projects.tasks.clean_build_task') @mock.patch('readthedocs.projects.tasks.update_docs_task') - def test_trigger_custom_queue(self, update_docs): + @mock.patch('readthedocs.core.utils.chain') + def test_trigger_custom_queue(self, chain, update_docs, clean_build_task): """Use a custom queue when routing the task.""" self.project.build_queue = 'build03' trigger_build(project=self.project, version=self.version) @@ -106,18 +107,23 @@ def test_trigger_custom_queue(self, update_docs): 'time_limit': 720, 'soft_time_limit': 600, } - update_docs.signature.assert_has_calls([ - mock.call( + chain.assert_called_with( + update_docs.signature( args=(self.project.pk,), kwargs=kwargs, options=options, immutable=True, ), - ]) - update_docs.signature().apply_async.assert_called() + clean_build_task.signature( + args=(self.version.pk,), + immutable=True, + ), + ) + @mock.patch('readthedocs.projects.tasks.clean_build_task') @mock.patch('readthedocs.projects.tasks.update_docs_task') - def test_trigger_build_time_limit(self, update_docs): + @mock.patch('readthedocs.core.utils.chain') + def test_trigger_build_time_limit(self, chain, update_docs, clean_build_task): """Pass of time limit.""" trigger_build(project=self.project, version=self.version) kwargs = { @@ -139,10 +145,23 @@ def test_trigger_build_time_limit(self, update_docs): immutable=True, ), ]) - update_docs.signature().apply_async.assert_called() + chain.assert_called_with( + update_docs.signature( + args=(self.project.pk,), + kwargs=kwargs, + options=options, + immutable=True, + ), + clean_build_task.signature( + args=(self.version.pk,), + immutable=True, + ), + ) + @mock.patch('readthedocs.projects.tasks.clean_build_task') @mock.patch('readthedocs.projects.tasks.update_docs_task') - def test_trigger_build_invalid_time_limit(self, update_docs): + @mock.patch('readthedocs.core.utils.chain') + def test_trigger_build_invalid_time_limit(self, chain, update_docs, clean_build_task): """Time limit as string.""" self.project.container_time_limit = '200s' trigger_build(project=self.project, version=self.version) @@ -165,10 +184,23 @@ def test_trigger_build_invalid_time_limit(self, update_docs): immutable=True, ), ]) - update_docs.signature().apply_async.assert_called() + chain.assert_called_with( + update_docs.signature( + args=(self.project.pk,), + kwargs=kwargs, + options=options, + immutable=True, + ), + clean_build_task.signature( + args=(self.version.pk,), + immutable=True, + ), + ) + @mock.patch('readthedocs.projects.tasks.clean_build_task') @mock.patch('readthedocs.projects.tasks.update_docs_task') - def test_trigger_build_rounded_time_limit(self, update_docs): + @mock.patch('readthedocs.core.utils.chain') + def test_trigger_build_rounded_time_limit(self, chain, update_docs, clean_build_task): """Time limit should round down.""" self.project.container_time_limit = 3 trigger_build(project=self.project, version=self.version) @@ -191,7 +223,18 @@ def test_trigger_build_rounded_time_limit(self, update_docs): immutable=True, ), ]) - update_docs.signature().apply_async.assert_called() + chain.assert_called_with( + update_docs.signature( + args=(self.project.pk,), + kwargs=kwargs, + options=options, + immutable=True, + ), + clean_build_task.signature( + args=(self.version.pk,), + immutable=True, + ), + ) def test_slugify(self): """Test additional slugify.""" From 96ef7138eda916c23988aba227b094b1b8d8e32b Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 9 May 2019 16:15:29 -0500 Subject: [PATCH 04/22] Fix docstring --- readthedocs/projects/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index a2955f2b0b0..db03a845170 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -1317,7 +1317,7 @@ def warn(self, msg): @app.task(max_retries=5, default_retry_delay=7 * 60) def clean_build_task(version_id): - """Clean the build artifacts of the given version.""" + """Clean the files used in the build of the given version.""" version = Version.objects.get_object_or_log(pk=version_id) if not version: return From b3c8d636e9e589805305e14483ab5e4b64a6741c Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 9 May 2019 17:26:19 -0500 Subject: [PATCH 05/22] Fix typos --- readthedocs/core/utils/__init__.py | 2 +- readthedocs/projects/tasks.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index 42f822e3b07..7f597914511 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -138,7 +138,7 @@ def prepare_build( ), clean_build_task.signature( args=(version.pk,), - inmutable=True, + immutable=True, ) ), build, diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index db03a845170..4b7a618ea88 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -1323,7 +1323,7 @@ def clean_build_task(version_id): return del_dirs = [ os.path.join(version.project.doc_path, dir_, version.slug) - for dir_ in ('checkout', 'envs', 'conda') + for dir_ in ('checkouts', 'envs', 'conda') ] for del_dir in del_dirs: broadcast(type='build', task=remove_dirs, args=[(del_dir,)]) From 9bde8d3c62574ca2e715c03eb8ecaf7f7a213144 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 13 May 2019 12:21:51 -0500 Subject: [PATCH 06/22] Only remove files from the current builder --- readthedocs/projects/tasks.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 4b7a618ea88..db43ee9fb78 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -1325,8 +1325,7 @@ def clean_build_task(version_id): os.path.join(version.project.doc_path, dir_, version.slug) for dir_ in ('checkouts', 'envs', 'conda') ] - for del_dir in del_dirs: - broadcast(type='build', task=remove_dirs, args=[(del_dir,)]) + remove_dirs(del_dirs) def _manage_imported_files(version, path, commit): From 47f2b67f39c2a01adfd9dc4419cd04961af1d9d7 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 14 May 2019 13:24:13 -0500 Subject: [PATCH 07/22] Update commands --- readthedocs/core/management/commands/pull.py | 1 + readthedocs/core/management/commands/update_repos.py | 6 ++++++ readthedocs/core/management/commands/update_versions.py | 3 ++- 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/readthedocs/core/management/commands/pull.py b/readthedocs/core/management/commands/pull.py index ae62f9da10c..68c78004f16 100644 --- a/readthedocs/core/management/commands/pull.py +++ b/readthedocs/core/management/commands/pull.py @@ -22,3 +22,4 @@ def handle(self, *args, **options): for slug in args: version = utils.version_from_slug(slug, LATEST) tasks.sync_repository_task(version.pk) + tasks.clean_build_task(version.pk) diff --git a/readthedocs/core/management/commands/update_repos.py b/readthedocs/core/management/commands/update_repos.py index 17d0951909e..148de292a9d 100644 --- a/readthedocs/core/management/commands/update_repos.py +++ b/readthedocs/core/management/commands/update_repos.py @@ -77,6 +77,7 @@ def handle(self, *args, **options): build_pk=build.pk, version_pk=version.pk, ) + tasks.clean_build_task(version.pk) else: p = Project.all_objects.get(slug=slug) log.info('Building %s', p) @@ -94,6 +95,7 @@ def handle(self, *args, **options): force=force, version_pk=version.pk, ) + tasks.clean_build_task(version.pk) else: log.info('Updating all docs') for project in Project.objects.all(): @@ -102,3 +104,7 @@ def handle(self, *args, **options): project.pk, force=force, ) + version = project.versions.get( + slug=project.get_default_version() + ) + tasks.clean_build_task(version.pk) diff --git a/readthedocs/core/management/commands/update_versions.py b/readthedocs/core/management/commands/update_versions.py index aacb47635a1..1803f798106 100644 --- a/readthedocs/core/management/commands/update_versions.py +++ b/readthedocs/core/management/commands/update_versions.py @@ -5,7 +5,7 @@ from django.core.management.base import BaseCommand from readthedocs.builds.models import Version -from readthedocs.projects.tasks import update_docs_task +from readthedocs.projects.tasks import clean_build_task, update_docs_task class Command(BaseCommand): @@ -20,3 +20,4 @@ def handle(self, *args, **options): record=False, version_pk=version.pk, ) + clean_build_task(version.pk) From c4258696bf114fe3218b6e5dcf1ee7c4b1755140 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 14 May 2019 13:24:41 -0500 Subject: [PATCH 08/22] Clean files after sync versions via webhooks --- readthedocs/core/views/hooks.py | 15 ++- readthedocs/rtd_tests/tests/test_api.py | 124 +++++++++++++++++++++--- 2 files changed, 121 insertions(+), 18 deletions(-) diff --git a/readthedocs/core/views/hooks.py b/readthedocs/core/views/hooks.py index 1a77c99fa69..a316d88cece 100644 --- a/readthedocs/core/views/hooks.py +++ b/readthedocs/core/views/hooks.py @@ -4,6 +4,7 @@ import logging import re +from celery import chain from django.http import HttpResponse, HttpResponseNotFound from django.shortcuts import redirect from django.views.decorators.csrf import csrf_exempt @@ -12,7 +13,7 @@ from readthedocs.core.utils import trigger_build from readthedocs.projects import constants from readthedocs.projects.models import Feature, Project -from readthedocs.projects.tasks import sync_repository_task +from readthedocs.projects.tasks import clean_build_task, sync_repository_task log = logging.getLogger(__name__) @@ -100,7 +101,17 @@ def sync_versions(project): if not version: log.info('Unable to sync from %s version', version_identifier) return None - sync_repository_task.delay(version.pk) + task = chain( + sync_repository_task.signature( + args=(version.pk,), + immutable=True, + ), + clean_build_task.signature( + args=(version.pk,), + immutable=True, + ), + ) + task.delay() return version.slug except Exception: log.exception('Unknown sync versions exception') diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index 3ee7820d24d..026c3783196 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/readthedocs/rtd_tests/tests/test_api.py @@ -873,8 +873,12 @@ def test_github_webhook_for_tags(self, trigger_build): [mock.call(force=True, version=self.version_tag, project=self.project)], ) + @mock.patch('readthedocs.core.views.hooks.clean_build_task') @mock.patch('readthedocs.core.views.hooks.sync_repository_task') - def test_github_create_event(self, sync_repository_task, trigger_build): + @mock.patch('readthedocs.core.views.hooks.chain') + def test_github_create_event( + self, chain, sync_repository_task, clean_build_task, trigger_build + ): client = APIClient() headers = {GITHUB_EVENT_HEADER: GITHUB_CREATE} @@ -890,10 +894,23 @@ def test_github_create_event(self, sync_repository_task, trigger_build): self.assertEqual(resp.data['versions'], [LATEST]) trigger_build.assert_not_called() latest_version = self.project.versions.get(slug=LATEST) - sync_repository_task.delay.assert_called_with(latest_version.pk) + chain.assert_called_with( + sync_repository_task.signature( + args=(latest_version.pk,), + immutable=True, + ), + clean_build_task.signature( + args=(latest_version.pk,), + immutable=True, + ), + ) + @mock.patch('readthedocs.core.views.hooks.clean_build_task') @mock.patch('readthedocs.core.views.hooks.sync_repository_task') - def test_github_delete_event(self, sync_repository_task, trigger_build): + @mock.patch('readthedocs.core.views.hooks.chain') + def test_github_delete_event( + self, chain, sync_repository_task, clean_build_task, trigger_build + ): client = APIClient() headers = {GITHUB_EVENT_HEADER: GITHUB_DELETE} @@ -909,7 +926,16 @@ def test_github_delete_event(self, sync_repository_task, trigger_build): self.assertEqual(resp.data['versions'], [LATEST]) trigger_build.assert_not_called() latest_version = self.project.versions.get(slug=LATEST) - sync_repository_task.delay.assert_called_with(latest_version.pk) + chain.assert_called_with( + sync_repository_task.signature( + args=(latest_version.pk,), + immutable=True, + ), + clean_build_task.signature( + args=(latest_version.pk,), + immutable=True, + ), + ) def test_github_parse_ref(self, trigger_build): wh = GitHubWebhookView() @@ -1097,9 +1123,11 @@ def test_gitlab_webhook_for_tags(self, trigger_build): ) trigger_build.assert_not_called() + @mock.patch('readthedocs.core.views.hooks.clean_build_task') @mock.patch('readthedocs.core.views.hooks.sync_repository_task') + @mock.patch('readthedocs.core.views.hooks.chain') def test_gitlab_push_hook_creation( - self, sync_repository_task, trigger_build, + self, chain, sync_repository_task, clean_build_task, trigger_build, ): client = APIClient() self.gitlab_payload.update( @@ -1117,11 +1145,22 @@ def test_gitlab_push_hook_creation( self.assertEqual(resp.data['versions'], [LATEST]) trigger_build.assert_not_called() latest_version = self.project.versions.get(slug=LATEST) - sync_repository_task.delay.assert_called_with(latest_version.pk) + chain.assert_called_with( + sync_repository_task.signature( + args=(latest_version.pk,), + immutable=True, + ), + clean_build_task.signature( + args=(latest_version.pk,), + immutable=True, + ), + ) + @mock.patch('readthedocs.core.views.hooks.clean_build_task') @mock.patch('readthedocs.core.views.hooks.sync_repository_task') + @mock.patch('readthedocs.core.views.hooks.chain') def test_gitlab_push_hook_deletion( - self, sync_repository_task, trigger_build, + self, chain, sync_repository_task, clean_build_task, trigger_build, ): client = APIClient() self.gitlab_payload.update( @@ -1139,11 +1178,22 @@ def test_gitlab_push_hook_deletion( self.assertEqual(resp.data['versions'], [LATEST]) trigger_build.assert_not_called() latest_version = self.project.versions.get(slug=LATEST) - sync_repository_task.delay.assert_called_with(latest_version.pk) + chain.assert_called_with( + sync_repository_task.signature( + args=(latest_version.pk,), + immutable=True, + ), + clean_build_task.signature( + args=(latest_version.pk,), + immutable=True, + ), + ) + @mock.patch('readthedocs.core.views.hooks.clean_build_task') @mock.patch('readthedocs.core.views.hooks.sync_repository_task') + @mock.patch('readthedocs.core.views.hooks.chain') def test_gitlab_tag_push_hook_creation( - self, sync_repository_task, trigger_build, + self, chain, sync_repository_task, clean_build_task, trigger_build, ): client = APIClient() self.gitlab_payload.update( @@ -1162,11 +1212,22 @@ def test_gitlab_tag_push_hook_creation( self.assertEqual(resp.data['versions'], [LATEST]) trigger_build.assert_not_called() latest_version = self.project.versions.get(slug=LATEST) - sync_repository_task.delay.assert_called_with(latest_version.pk) + chain.assert_called_with( + sync_repository_task.signature( + args=(latest_version.pk,), + immutable=True, + ), + clean_build_task.signature( + args=(latest_version.pk,), + immutable=True, + ), + ) + @mock.patch('readthedocs.core.views.hooks.clean_build_task') @mock.patch('readthedocs.core.views.hooks.sync_repository_task') + @mock.patch('readthedocs.core.views.hooks.chain') def test_gitlab_tag_push_hook_deletion( - self, sync_repository_task, trigger_build, + self, chain, sync_repository_task, clean_build_task, trigger_build, ): client = APIClient() self.gitlab_payload.update( @@ -1185,7 +1246,16 @@ def test_gitlab_tag_push_hook_deletion( self.assertEqual(resp.data['versions'], [LATEST]) trigger_build.assert_not_called() latest_version = self.project.versions.get(slug=LATEST) - sync_repository_task.delay.assert_called_with(latest_version.pk) + chain.assert_called_with( + sync_repository_task.signature( + args=(latest_version.pk,), + immutable=True, + ), + clean_build_task.signature( + args=(latest_version.pk,), + immutable=True, + ), + ) def test_gitlab_invalid_webhook(self, trigger_build): """GitLab webhook unhandled event.""" @@ -1335,9 +1405,11 @@ def test_bitbucket_webhook(self, trigger_build): ) self.assertEqual(trigger_build_call_count, trigger_build.call_count) + @mock.patch('readthedocs.core.views.hooks.clean_build_task') @mock.patch('readthedocs.core.views.hooks.sync_repository_task') + @mock.patch('readthedocs.core.views.hooks.chain') def test_bitbucket_push_hook_creation( - self, sync_repository_task, trigger_build, + self, chain, sync_repository_task, clean_build_task, trigger_build, ): client = APIClient() self.bitbucket_payload['push']['changes'][0]['old'] = None @@ -1352,11 +1424,22 @@ def test_bitbucket_push_hook_creation( self.assertEqual(resp.data['versions'], [LATEST]) trigger_build.assert_not_called() latest_version = self.project.versions.get(slug=LATEST) - sync_repository_task.delay.assert_called_with(latest_version.pk) + chain.assert_called_with( + sync_repository_task.signature( + args=(latest_version.pk,), + immutable=True, + ), + clean_build_task.signature( + args=(latest_version.pk,), + immutable=True, + ), + ) + @mock.patch('readthedocs.core.views.hooks.clean_build_task') @mock.patch('readthedocs.core.views.hooks.sync_repository_task') + @mock.patch('readthedocs.core.views.hooks.chain') def test_bitbucket_push_hook_deletion( - self, sync_repository_task, trigger_build, + self, chain, sync_repository_task, clean_build_task, trigger_build, ): client = APIClient() self.bitbucket_payload['push']['changes'][0]['new'] = None @@ -1371,7 +1454,16 @@ def test_bitbucket_push_hook_deletion( self.assertEqual(resp.data['versions'], [LATEST]) trigger_build.assert_not_called() latest_version = self.project.versions.get(slug=LATEST) - sync_repository_task.delay.assert_called_with(latest_version.pk) + chain.assert_called_with( + sync_repository_task.signature( + args=(latest_version.pk,), + immutable=True, + ), + clean_build_task.signature( + args=(latest_version.pk,), + immutable=True, + ), + ) def test_bitbucket_invalid_webhook(self, trigger_build): """Bitbucket webhook unhandled event.""" From 3f6095d64f487ac824a46b9be1d0d7a6cd509b44 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 14 May 2019 17:39:57 -0500 Subject: [PATCH 09/22] Add feature flag and lock --- readthedocs/projects/models.py | 7 ++++++- readthedocs/projects/tasks.py | 17 +++++++++++++---- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 04e092e7f10..eff1a2b37bd 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -1360,6 +1360,7 @@ def add_features(sender, **kwargs): SHARE_SPHINX_DOCTREE = 'share_sphinx_doctree' USE_PDF_LATEXMK = 'use_pdf_latexmk' DEFAULT_TO_MKDOCS_0_17_3 = 'default_to_mkdocs_0_17_3' + CLEAN_AFTER_BUILD = 'clean_after_build' FEATURES = ( (USE_SPHINX_LATEST, _('Use latest version of Sphinx')), @@ -1395,7 +1396,11 @@ def add_features(sender, **kwargs): ), ( DEFAULT_TO_MKDOCS_0_17_3, - _('Install mkdocs 0.17.3 by default') + _('Install mkdocs 0.17.3 by default'), + ), + ( + CLEAN_AFTER_BUILD, + _('Clean all files used in the build process'), ), ) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index b0d6a11c4eb..bc318929987 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -60,7 +60,7 @@ ) from readthedocs.doc_builder.loader import get_builder_class from readthedocs.doc_builder.python_environments import Conda, Virtualenv -from readthedocs.projects.models import APIProject +from readthedocs.projects.models import APIProject, Feature from readthedocs.sphinx_domains.models import SphinxDomain from readthedocs.vcs_support import utils as vcs_support_utils from readthedocs.worker import app @@ -1417,17 +1417,26 @@ def warn(self, msg): delete_queryset.delete() -@app.task(max_retries=5, default_retry_delay=7 * 60) +@app.task(max_retries=3, default_retry_delay=7 * 60) def clean_build_task(version_id): """Clean the files used in the build of the given version.""" version = Version.objects.get_object_or_log(pk=version_id) - if not version: + if ( + not version or + not version.project.has_feature(Feature.CLEAN_AFTER_BUILD) + ): return del_dirs = [ os.path.join(version.project.doc_path, dir_, version.slug) for dir_ in ('checkouts', 'envs', 'conda') ] - remove_dirs(del_dirs) + # TODO: the max_lock_age can be lower than the default + try: + with version.project.repo_nonblockinglock(version): + log.info('Removing: %s', del_dirs) + remove_dirs(del_dirs) + except vcs_support_utils.LockTimeout: + log.info('Another task is running. Not removing: %s', del_dirs) def _manage_imported_files(version, path, commit): From 2bf07e559ba2744eb0117a51569839e7230baf26 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 14 May 2019 17:59:42 -0500 Subject: [PATCH 10/22] Remove comment --- readthedocs/projects/tasks.py | 1 - 1 file changed, 1 deletion(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index bc318929987..1f88f3cae63 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -1430,7 +1430,6 @@ def clean_build_task(version_id): os.path.join(version.project.doc_path, dir_, version.slug) for dir_ in ('checkouts', 'envs', 'conda') ] - # TODO: the max_lock_age can be lower than the default try: with version.project.repo_nonblockinglock(version): log.info('Removing: %s', del_dirs) From b613bbb98f99fb507fbe84826d8fa90de2db4358 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 14 May 2019 18:22:05 -0500 Subject: [PATCH 11/22] Remove artifacts too Artifacts are copied to rtd_builds after build --- readthedocs/projects/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 1f88f3cae63..8e443816e0d 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -1428,7 +1428,7 @@ def clean_build_task(version_id): return del_dirs = [ os.path.join(version.project.doc_path, dir_, version.slug) - for dir_ in ('checkouts', 'envs', 'conda') + for dir_ in ('checkouts', 'artifacts', 'envs', 'conda') ] try: with version.project.repo_nonblockinglock(version): From 9ba356c01b83fcb73df4f6458f6c886aa4a76b61 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 16 May 2019 19:49:05 -0500 Subject: [PATCH 12/22] Move lock to top level tasks --- readthedocs/projects/tasks.py | 57 +++++++++++++++-------------------- 1 file changed, 25 insertions(+), 32 deletions(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index b76089e6197..85795b5249d 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -133,28 +133,27 @@ def sync_repo(self): ), ) - with self.project.repo_nonblockinglock(version=self.version): - # Get the actual code on disk - try: - before_vcs.send(sender=self.version) - msg = 'Checking out version {slug}: {identifier}'.format( - slug=self.version.slug, - identifier=self.version.identifier, - ) - log.info( - LOG_TEMPLATE, - { - 'project': self.project.slug, - 'version': self.version.slug, - 'msg': msg, - } - ) - version_repo = self.get_vcs_repo() - version_repo.update() - self.sync_versions(version_repo) - version_repo.checkout(self.version.identifier) - finally: - after_vcs.send(sender=self.version) + # Get the actual code on disk + try: + before_vcs.send(sender=self.version) + msg = 'Checking out version {slug}: {identifier}'.format( + slug=self.version.slug, + identifier=self.version.identifier, + ) + log.info( + LOG_TEMPLATE, + { + 'project': self.project.slug, + 'version': self.version.slug, + 'msg': msg, + } + ) + version_repo = self.get_vcs_repo() + version_repo.update() + self.sync_versions(version_repo) + version_repo.checkout(self.version.identifier) + finally: + after_vcs.send(sender=self.version) def sync_versions(self, version_repo): """ @@ -242,7 +241,8 @@ def run(self, version_pk): # pylint: disable=arguments-differ try: self.version = self.get_version(version_pk=version_pk) self.project = self.version.project - self.sync_repo() + with self.project.repo_nonblockinglock(version=self.version): + self.sync_repo() return True except RepositoryError: # Do not log as ERROR handled exceptions @@ -462,7 +462,8 @@ def run_setup(self, record=True): if self.project.skip: raise ProjectBuildsSkippedError try: - self.setup_vcs() + with self.project.repo_nonblockinglock(version=self.version): + self.setup_vcs() except vcs_support_utils.LockTimeout as e: self.task.retry(exc=e, throw=False) raise VersionLockedError @@ -648,14 +649,6 @@ def setup_vcs(self): log.warning('There was an error with the repository', exc_info=True) # Re raise the exception to stop the build at this point raise - except vcs_support_utils.LockTimeout: - log.info( - 'Lock still active: project=%s version=%s', - self.project.slug, - self.version.slug, - ) - # Raise the proper exception (won't be sent to Sentry) - raise VersionLockedError except Exception: # Catch unhandled errors when syncing log.exception( From ce446985bc02ebeaade76fdb2a81230e14e34a70 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 30 May 2019 18:38:44 -0500 Subject: [PATCH 13/22] Put log.info --- readthedocs/projects/tasks.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 50edd7b672a..2b5ca69fef2 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -1397,7 +1397,11 @@ def clean_build_task(version_id): not version or not version.project.has_feature(Feature.CLEAN_AFTER_BUILD) ): - return + log.info( + 'Skipping build files deletetion for version: %s', + version_id, + ) + return False del_dirs = [ os.path.join(version.project.doc_path, dir_, version.slug) for dir_ in ('checkouts', 'artifacts', 'envs', 'conda') @@ -1408,6 +1412,8 @@ def clean_build_task(version_id): remove_dirs(del_dirs) except vcs_support_utils.LockTimeout: log.info('Another task is running. Not removing: %s', del_dirs) + else: + return True def _manage_imported_files(version, path, commit): From cd350c2ac0e17f03ab8c3c0651e67133a8604eb7 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 4 Jun 2019 00:31:48 -0500 Subject: [PATCH 14/22] Use try/finally blocks to run clean_build_files in the same server --- readthedocs/core/management/commands/pull.py | 1 - .../core/management/commands/update_repos.py | 6 - .../management/commands/update_versions.py | 3 +- readthedocs/core/utils/__init__.py | 20 +-- readthedocs/core/views/hooks.py | 14 +- readthedocs/projects/tasks.py | 14 +- readthedocs/rtd_tests/tests/test_api.py | 120 +++--------------- .../rtd_tests/tests/test_core_utils.py | 80 ++++-------- 8 files changed, 60 insertions(+), 198 deletions(-) diff --git a/readthedocs/core/management/commands/pull.py b/readthedocs/core/management/commands/pull.py index 68c78004f16..ae62f9da10c 100644 --- a/readthedocs/core/management/commands/pull.py +++ b/readthedocs/core/management/commands/pull.py @@ -22,4 +22,3 @@ def handle(self, *args, **options): for slug in args: version = utils.version_from_slug(slug, LATEST) tasks.sync_repository_task(version.pk) - tasks.clean_build_task(version.pk) diff --git a/readthedocs/core/management/commands/update_repos.py b/readthedocs/core/management/commands/update_repos.py index 148de292a9d..17d0951909e 100644 --- a/readthedocs/core/management/commands/update_repos.py +++ b/readthedocs/core/management/commands/update_repos.py @@ -77,7 +77,6 @@ def handle(self, *args, **options): build_pk=build.pk, version_pk=version.pk, ) - tasks.clean_build_task(version.pk) else: p = Project.all_objects.get(slug=slug) log.info('Building %s', p) @@ -95,7 +94,6 @@ def handle(self, *args, **options): force=force, version_pk=version.pk, ) - tasks.clean_build_task(version.pk) else: log.info('Updating all docs') for project in Project.objects.all(): @@ -104,7 +102,3 @@ def handle(self, *args, **options): project.pk, force=force, ) - version = project.versions.get( - slug=project.get_default_version() - ) - tasks.clean_build_task(version.pk) diff --git a/readthedocs/core/management/commands/update_versions.py b/readthedocs/core/management/commands/update_versions.py index 1803f798106..aacb47635a1 100644 --- a/readthedocs/core/management/commands/update_versions.py +++ b/readthedocs/core/management/commands/update_versions.py @@ -5,7 +5,7 @@ from django.core.management.base import BaseCommand from readthedocs.builds.models import Version -from readthedocs.projects.tasks import clean_build_task, update_docs_task +from readthedocs.projects.tasks import update_docs_task class Command(BaseCommand): @@ -20,4 +20,3 @@ def handle(self, *args, **options): record=False, version_pk=version.pk, ) - clean_build_task(version.pk) diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py index 7f597914511..1fa41b3c8f0 100644 --- a/readthedocs/core/utils/__init__.py +++ b/readthedocs/core/utils/__init__.py @@ -7,7 +7,7 @@ import os import re -from celery import chain, chord, group +from celery import chord, group from django.conf import settings from django.utils.functional import keep_lazy from django.utils.safestring import SafeText, mark_safe @@ -80,7 +80,7 @@ def prepare_build( # Avoid circular import from readthedocs.builds.models import Build from readthedocs.projects.models import Project - from readthedocs.projects.tasks import clean_build_task, update_docs_task + from readthedocs.projects.tasks import update_docs_task build = None @@ -129,17 +129,11 @@ def prepare_build( options['time_limit'] = int(time_limit * 1.2) return ( - chain( - update_docs_task.signature( - args=(project.pk,), - kwargs=kwargs, - options=options, - immutable=True, - ), - clean_build_task.signature( - args=(version.pk,), - immutable=True, - ) + update_docs_task.signature( + args=(project.pk,), + kwargs=kwargs, + options=options, + immutable=True, ), build, ) diff --git a/readthedocs/core/views/hooks.py b/readthedocs/core/views/hooks.py index a316d88cece..1201e386b0a 100644 --- a/readthedocs/core/views/hooks.py +++ b/readthedocs/core/views/hooks.py @@ -13,7 +13,7 @@ from readthedocs.core.utils import trigger_build from readthedocs.projects import constants from readthedocs.projects.models import Feature, Project -from readthedocs.projects.tasks import clean_build_task, sync_repository_task +from readthedocs.projects.tasks import sync_repository_task log = logging.getLogger(__name__) @@ -101,17 +101,7 @@ def sync_versions(project): if not version: log.info('Unable to sync from %s version', version_identifier) return None - task = chain( - sync_repository_task.signature( - args=(version.pk,), - immutable=True, - ), - clean_build_task.signature( - args=(version.pk,), - immutable=True, - ), - ) - task.delay() + sync_repository_task.delay(version.pk) return version.slug except Exception: log.exception('Unknown sync versions exception') diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 2b5ca69fef2..45e83579f97 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -212,8 +212,11 @@ def validate_duplicate_reserved_versions(self, data): @app.task(max_retries=5, default_retry_delay=7 * 60) def sync_repository_task(version_pk): """Celery task to trigger VCS version sync.""" - step = SyncRepositoryTaskStep() - return step.run(version_pk) + try: + step = SyncRepositoryTaskStep() + return step.run(version_pk) + finally: + clean_build_task(version_pk) class SyncRepositoryTaskStep(SyncRepositoryMixin): @@ -289,8 +292,11 @@ def run(self, version_pk): # pylint: disable=arguments-differ ), ) def update_docs_task(self, project_id, *args, **kwargs): - step = UpdateDocsTaskStep(task=self) - return step.run(project_id, *args, **kwargs) + try: + step = UpdateDocsTaskStep(task=self) + return step.run(project_id, *args, **kwargs) + finally: + clean_build_task(kwargs.get('version_pk')) class UpdateDocsTaskStep(SyncRepositoryMixin): diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index 026c3783196..af0aabf3be4 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/readthedocs/rtd_tests/tests/test_api.py @@ -873,11 +873,9 @@ def test_github_webhook_for_tags(self, trigger_build): [mock.call(force=True, version=self.version_tag, project=self.project)], ) - @mock.patch('readthedocs.core.views.hooks.clean_build_task') @mock.patch('readthedocs.core.views.hooks.sync_repository_task') - @mock.patch('readthedocs.core.views.hooks.chain') def test_github_create_event( - self, chain, sync_repository_task, clean_build_task, trigger_build + self, sync_repository_task, trigger_build ): client = APIClient() @@ -894,22 +892,11 @@ def test_github_create_event( self.assertEqual(resp.data['versions'], [LATEST]) trigger_build.assert_not_called() latest_version = self.project.versions.get(slug=LATEST) - chain.assert_called_with( - sync_repository_task.signature( - args=(latest_version.pk,), - immutable=True, - ), - clean_build_task.signature( - args=(latest_version.pk,), - immutable=True, - ), - ) + sync_repository_task.delay.assert_called_with(latest_version.pk) - @mock.patch('readthedocs.core.views.hooks.clean_build_task') @mock.patch('readthedocs.core.views.hooks.sync_repository_task') - @mock.patch('readthedocs.core.views.hooks.chain') def test_github_delete_event( - self, chain, sync_repository_task, clean_build_task, trigger_build + self, sync_repository_task, trigger_build ): client = APIClient() @@ -926,16 +913,7 @@ def test_github_delete_event( self.assertEqual(resp.data['versions'], [LATEST]) trigger_build.assert_not_called() latest_version = self.project.versions.get(slug=LATEST) - chain.assert_called_with( - sync_repository_task.signature( - args=(latest_version.pk,), - immutable=True, - ), - clean_build_task.signature( - args=(latest_version.pk,), - immutable=True, - ), - ) + sync_repository_task.delay.assert_called_with(latest_version.pk) def test_github_parse_ref(self, trigger_build): wh = GitHubWebhookView() @@ -1123,11 +1101,9 @@ def test_gitlab_webhook_for_tags(self, trigger_build): ) trigger_build.assert_not_called() - @mock.patch('readthedocs.core.views.hooks.clean_build_task') @mock.patch('readthedocs.core.views.hooks.sync_repository_task') - @mock.patch('readthedocs.core.views.hooks.chain') def test_gitlab_push_hook_creation( - self, chain, sync_repository_task, clean_build_task, trigger_build, + self, sync_repository_task, trigger_build, ): client = APIClient() self.gitlab_payload.update( @@ -1145,22 +1121,11 @@ def test_gitlab_push_hook_creation( self.assertEqual(resp.data['versions'], [LATEST]) trigger_build.assert_not_called() latest_version = self.project.versions.get(slug=LATEST) - chain.assert_called_with( - sync_repository_task.signature( - args=(latest_version.pk,), - immutable=True, - ), - clean_build_task.signature( - args=(latest_version.pk,), - immutable=True, - ), - ) + sync_repository_task.delay.assert_called_with(latest_version.pk) - @mock.patch('readthedocs.core.views.hooks.clean_build_task') @mock.patch('readthedocs.core.views.hooks.sync_repository_task') - @mock.patch('readthedocs.core.views.hooks.chain') def test_gitlab_push_hook_deletion( - self, chain, sync_repository_task, clean_build_task, trigger_build, + self, sync_repository_task, trigger_build, ): client = APIClient() self.gitlab_payload.update( @@ -1178,22 +1143,11 @@ def test_gitlab_push_hook_deletion( self.assertEqual(resp.data['versions'], [LATEST]) trigger_build.assert_not_called() latest_version = self.project.versions.get(slug=LATEST) - chain.assert_called_with( - sync_repository_task.signature( - args=(latest_version.pk,), - immutable=True, - ), - clean_build_task.signature( - args=(latest_version.pk,), - immutable=True, - ), - ) + sync_repository_task.delay.assert_called_with(latest_version.pk) - @mock.patch('readthedocs.core.views.hooks.clean_build_task') @mock.patch('readthedocs.core.views.hooks.sync_repository_task') - @mock.patch('readthedocs.core.views.hooks.chain') def test_gitlab_tag_push_hook_creation( - self, chain, sync_repository_task, clean_build_task, trigger_build, + self, sync_repository_task, trigger_build, ): client = APIClient() self.gitlab_payload.update( @@ -1212,22 +1166,11 @@ def test_gitlab_tag_push_hook_creation( self.assertEqual(resp.data['versions'], [LATEST]) trigger_build.assert_not_called() latest_version = self.project.versions.get(slug=LATEST) - chain.assert_called_with( - sync_repository_task.signature( - args=(latest_version.pk,), - immutable=True, - ), - clean_build_task.signature( - args=(latest_version.pk,), - immutable=True, - ), - ) + sync_repository_task.delay.assert_called_with(latest_version.pk) - @mock.patch('readthedocs.core.views.hooks.clean_build_task') @mock.patch('readthedocs.core.views.hooks.sync_repository_task') - @mock.patch('readthedocs.core.views.hooks.chain') def test_gitlab_tag_push_hook_deletion( - self, chain, sync_repository_task, clean_build_task, trigger_build, + self, sync_repository_task, trigger_build, ): client = APIClient() self.gitlab_payload.update( @@ -1246,16 +1189,7 @@ def test_gitlab_tag_push_hook_deletion( self.assertEqual(resp.data['versions'], [LATEST]) trigger_build.assert_not_called() latest_version = self.project.versions.get(slug=LATEST) - chain.assert_called_with( - sync_repository_task.signature( - args=(latest_version.pk,), - immutable=True, - ), - clean_build_task.signature( - args=(latest_version.pk,), - immutable=True, - ), - ) + sync_repository_task.delay.assert_called_with(latest_version.pk) def test_gitlab_invalid_webhook(self, trigger_build): """GitLab webhook unhandled event.""" @@ -1405,11 +1339,9 @@ def test_bitbucket_webhook(self, trigger_build): ) self.assertEqual(trigger_build_call_count, trigger_build.call_count) - @mock.patch('readthedocs.core.views.hooks.clean_build_task') @mock.patch('readthedocs.core.views.hooks.sync_repository_task') - @mock.patch('readthedocs.core.views.hooks.chain') def test_bitbucket_push_hook_creation( - self, chain, sync_repository_task, clean_build_task, trigger_build, + self, sync_repository_task, trigger_build, ): client = APIClient() self.bitbucket_payload['push']['changes'][0]['old'] = None @@ -1424,22 +1356,11 @@ def test_bitbucket_push_hook_creation( self.assertEqual(resp.data['versions'], [LATEST]) trigger_build.assert_not_called() latest_version = self.project.versions.get(slug=LATEST) - chain.assert_called_with( - sync_repository_task.signature( - args=(latest_version.pk,), - immutable=True, - ), - clean_build_task.signature( - args=(latest_version.pk,), - immutable=True, - ), - ) + sync_repository_task.delay.assert_called_with(latest_version.pk) - @mock.patch('readthedocs.core.views.hooks.clean_build_task') @mock.patch('readthedocs.core.views.hooks.sync_repository_task') - @mock.patch('readthedocs.core.views.hooks.chain') def test_bitbucket_push_hook_deletion( - self, chain, sync_repository_task, clean_build_task, trigger_build, + self, sync_repository_task, trigger_build, ): client = APIClient() self.bitbucket_payload['push']['changes'][0]['new'] = None @@ -1454,16 +1375,7 @@ def test_bitbucket_push_hook_deletion( self.assertEqual(resp.data['versions'], [LATEST]) trigger_build.assert_not_called() latest_version = self.project.versions.get(slug=LATEST) - chain.assert_called_with( - sync_repository_task.signature( - args=(latest_version.pk,), - immutable=True, - ), - clean_build_task.signature( - args=(latest_version.pk,), - immutable=True, - ), - ) + sync_repository_task.delay.assert_called_with(latest_version.pk) def test_bitbucket_invalid_webhook(self, trigger_build): """Bitbucket webhook unhandled event.""" diff --git a/readthedocs/rtd_tests/tests/test_core_utils.py b/readthedocs/rtd_tests/tests/test_core_utils.py index 57516576d60..ff92de56fb5 100644 --- a/readthedocs/rtd_tests/tests/test_core_utils.py +++ b/readthedocs/rtd_tests/tests/test_core_utils.py @@ -89,10 +89,8 @@ def test_trigger_build_when_version_not_provided_default_version_doesnt_exist(se ), ]) - @mock.patch('readthedocs.projects.tasks.clean_build_task') @mock.patch('readthedocs.projects.tasks.update_docs_task') - @mock.patch('readthedocs.core.utils.chain') - def test_trigger_custom_queue(self, chain, update_docs, clean_build_task): + def test_trigger_custom_queue(self, update_docs): """Use a custom queue when routing the task.""" self.project.build_queue = 'build03' trigger_build(project=self.project, version=self.version) @@ -107,23 +105,15 @@ def test_trigger_custom_queue(self, chain, update_docs, clean_build_task): 'time_limit': 720, 'soft_time_limit': 600, } - chain.assert_called_with( - update_docs.signature( - args=(self.project.pk,), - kwargs=kwargs, - options=options, - immutable=True, - ), - clean_build_task.signature( - args=(self.version.pk,), - immutable=True, - ), + update_docs.signature.assert_called_with( + args=(self.project.pk,), + kwargs=kwargs, + options=options, + immutable=True, ) - @mock.patch('readthedocs.projects.tasks.clean_build_task') @mock.patch('readthedocs.projects.tasks.update_docs_task') - @mock.patch('readthedocs.core.utils.chain') - def test_trigger_build_time_limit(self, chain, update_docs, clean_build_task): + def test_trigger_build_time_limit(self, update_docs): """Pass of time limit.""" trigger_build(project=self.project, version=self.version) kwargs = { @@ -145,23 +135,15 @@ def test_trigger_build_time_limit(self, chain, update_docs, clean_build_task): immutable=True, ), ]) - chain.assert_called_with( - update_docs.signature( - args=(self.project.pk,), - kwargs=kwargs, - options=options, - immutable=True, - ), - clean_build_task.signature( - args=(self.version.pk,), - immutable=True, - ), + update_docs.signature.assert_called_with( + args=(self.project.pk,), + kwargs=kwargs, + options=options, + immutable=True, ) - @mock.patch('readthedocs.projects.tasks.clean_build_task') @mock.patch('readthedocs.projects.tasks.update_docs_task') - @mock.patch('readthedocs.core.utils.chain') - def test_trigger_build_invalid_time_limit(self, chain, update_docs, clean_build_task): + def test_trigger_build_invalid_time_limit(self, update_docs): """Time limit as string.""" self.project.container_time_limit = '200s' trigger_build(project=self.project, version=self.version) @@ -184,23 +166,15 @@ def test_trigger_build_invalid_time_limit(self, chain, update_docs, clean_build_ immutable=True, ), ]) - chain.assert_called_with( - update_docs.signature( - args=(self.project.pk,), - kwargs=kwargs, - options=options, - immutable=True, - ), - clean_build_task.signature( - args=(self.version.pk,), - immutable=True, - ), + update_docs.signature.assert_called_with( + args=(self.project.pk,), + kwargs=kwargs, + options=options, + immutable=True, ) - @mock.patch('readthedocs.projects.tasks.clean_build_task') @mock.patch('readthedocs.projects.tasks.update_docs_task') - @mock.patch('readthedocs.core.utils.chain') - def test_trigger_build_rounded_time_limit(self, chain, update_docs, clean_build_task): + def test_trigger_build_rounded_time_limit(self, update_docs): """Time limit should round down.""" self.project.container_time_limit = 3 trigger_build(project=self.project, version=self.version) @@ -223,17 +197,11 @@ def test_trigger_build_rounded_time_limit(self, chain, update_docs, clean_build_ immutable=True, ), ]) - chain.assert_called_with( - update_docs.signature( - args=(self.project.pk,), - kwargs=kwargs, - options=options, - immutable=True, - ), - clean_build_task.signature( - args=(self.version.pk,), - immutable=True, - ), + update_docs.signature.assert_called_with( + args=(self.project.pk,), + kwargs=kwargs, + options=options, + immutable=True, ) def test_slugify(self): From 8558c20995f67971aa9bed660c6d55ffa0f48ac9 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 4 Jun 2019 00:32:24 -0500 Subject: [PATCH 15/22] Revert lock Making this in another PR --- readthedocs/projects/tasks.py | 57 ++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 45e83579f97..e242bc363e3 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -133,27 +133,28 @@ def sync_repo(self): ), ) - # Get the actual code on disk - try: - before_vcs.send(sender=self.version) - msg = 'Checking out version {slug}: {identifier}'.format( - slug=self.version.slug, - identifier=self.version.identifier, - ) - log.info( - LOG_TEMPLATE, - { - 'project': self.project.slug, - 'version': self.version.slug, - 'msg': msg, - } - ) - version_repo = self.get_vcs_repo() - version_repo.update() - self.sync_versions(version_repo) - version_repo.checkout(self.version.identifier) - finally: - after_vcs.send(sender=self.version) + with self.project.repo_nonblockinglock(version=self.version): + # Get the actual code on disk + try: + before_vcs.send(sender=self.version) + msg = 'Checking out version {slug}: {identifier}'.format( + slug=self.version.slug, + identifier=self.version.identifier, + ) + log.info( + LOG_TEMPLATE, + { + 'project': self.project.slug, + 'version': self.version.slug, + 'msg': msg, + } + ) + version_repo = self.get_vcs_repo() + version_repo.update() + self.sync_versions(version_repo) + version_repo.checkout(self.version.identifier) + finally: + after_vcs.send(sender=self.version) def sync_versions(self, version_repo): """ @@ -244,8 +245,7 @@ def run(self, version_pk): # pylint: disable=arguments-differ try: self.version = self.get_version(version_pk=version_pk) self.project = self.version.project - with self.project.repo_nonblockinglock(version=self.version): - self.sync_repo() + self.sync_repo() return True except RepositoryError: # Do not log as ERROR handled exceptions @@ -448,8 +448,7 @@ def run_setup(self, record=True): if self.project.skip: raise ProjectBuildsSkippedError try: - with self.project.repo_nonblockinglock(version=self.version): - self.setup_vcs() + self.setup_vcs() except vcs_support_utils.LockTimeout as e: self.task.retry(exc=e, throw=False) raise VersionLockedError @@ -635,6 +634,14 @@ def setup_vcs(self): log.warning('There was an error with the repository', exc_info=True) # Re raise the exception to stop the build at this point raise + except vcs_support_utils.LockTimeout: + log.info( + 'Lock still active: project=%s version=%s', + self.project.slug, + self.version.slug, + ) + # Raise the proper exception (won't be sent to Sentry) + raise VersionLockedError except Exception: # Catch unhandled errors when syncing log.exception( From caeb3abf19bbd02d4a6fc6adf95f1a0407e7122f Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 4 Jun 2019 00:40:51 -0500 Subject: [PATCH 16/22] Untouch files --- readthedocs/core/views/hooks.py | 1 - readthedocs/rtd_tests/tests/test_api.py | 8 ++------ 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/readthedocs/core/views/hooks.py b/readthedocs/core/views/hooks.py index 1201e386b0a..1a77c99fa69 100644 --- a/readthedocs/core/views/hooks.py +++ b/readthedocs/core/views/hooks.py @@ -4,7 +4,6 @@ import logging import re -from celery import chain from django.http import HttpResponse, HttpResponseNotFound from django.shortcuts import redirect from django.views.decorators.csrf import csrf_exempt diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index af0aabf3be4..3ee7820d24d 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/readthedocs/rtd_tests/tests/test_api.py @@ -874,9 +874,7 @@ def test_github_webhook_for_tags(self, trigger_build): ) @mock.patch('readthedocs.core.views.hooks.sync_repository_task') - def test_github_create_event( - self, sync_repository_task, trigger_build - ): + def test_github_create_event(self, sync_repository_task, trigger_build): client = APIClient() headers = {GITHUB_EVENT_HEADER: GITHUB_CREATE} @@ -895,9 +893,7 @@ def test_github_create_event( sync_repository_task.delay.assert_called_with(latest_version.pk) @mock.patch('readthedocs.core.views.hooks.sync_repository_task') - def test_github_delete_event( - self, sync_repository_task, trigger_build - ): + def test_github_delete_event(self, sync_repository_task, trigger_build): client = APIClient() headers = {GITHUB_EVENT_HEADER: GITHUB_DELETE} From a65dc3855be15a9f5a425f640b296d9356734447 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 6 Jun 2019 15:35:29 -0500 Subject: [PATCH 17/22] Refactor --- readthedocs/projects/tasks.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 451ab785598..61c4d910b1a 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -207,7 +207,7 @@ def sync_repository_task(version_pk): step = SyncRepositoryTaskStep() return step.run(version_pk) finally: - clean_build_task(version_pk) + clean_build(version_pk) class SyncRepositoryTaskStep(SyncRepositoryMixin): @@ -282,12 +282,12 @@ def run(self, version_pk): # pylint: disable=arguments-differ MkDocsYAMLParseError, ), ) -def update_docs_task(self, project_id, *args, **kwargs): +def update_docs_task(self, version_pk, *args, **kwargs): try: step = UpdateDocsTaskStep(task=self) return step.run(version_pk, *args, **kwargs) finally: - clean_build_task(kwargs.get('version_pk')) + clean_build(version_pk) class UpdateDocsTaskStep(SyncRepositoryMixin): @@ -1385,17 +1385,16 @@ def warn(self, msg): delete_queryset.delete() -@app.task(max_retries=3, default_retry_delay=7 * 60) -def clean_build_task(version_id): +def clean_build(version_pk): """Clean the files used in the build of the given version.""" - version = Version.objects.get_object_or_log(pk=version_id) + version = Version.objects.get_object_or_log(pk=version_pk) if ( not version or not version.project.has_feature(Feature.CLEAN_AFTER_BUILD) ): log.info( 'Skipping build files deletetion for version: %s', - version_id, + version_pk, ) return False del_dirs = [ From 167dcbfe1a04d9bb85bf33b251a289fc47ada582 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 6 Jun 2019 17:03:03 -0500 Subject: [PATCH 18/22] Fix tests --- .../rtd_tests/tests/test_core_utils.py | 58 +++++-------------- 1 file changed, 15 insertions(+), 43 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_core_utils.py b/readthedocs/rtd_tests/tests/test_core_utils.py index 64f36572ab3..26205fbd160 100644 --- a/readthedocs/rtd_tests/tests/test_core_utils.py +++ b/readthedocs/rtd_tests/tests/test_core_utils.py @@ -54,14 +54,12 @@ def test_trigger_build_when_version_not_provided_default_version_exist(self, upd 'build_pk': mock.ANY, } - update_docs_task.signature.assert_has_calls([ - mock.call( - args=(version_1.pk,), - kwargs=kwargs, - options=mock.ANY, - immutable=True, - ), - ]) + update_docs_task.signature.assert_called_with( + args=(version_1.pk,), + kwargs=kwargs, + options=mock.ANY, + immutable=True, + ) @mock.patch('readthedocs.projects.tasks.update_docs_task') def test_trigger_build_when_version_not_provided_default_version_doesnt_exist(self, update_docs_task): @@ -78,14 +76,12 @@ def test_trigger_build_when_version_not_provided_default_version_doesnt_exist(se 'build_pk': mock.ANY, } - update_docs_task.signature.assert_has_calls([ - mock.call( - args=(version.pk,), - kwargs=kwargs, - options=mock.ANY, - immutable=True, - ), - ]) + update_docs_task.signature.assert_called_with( + args=(version.pk,), + kwargs=kwargs, + options=mock.ANY, + immutable=True, + ) @mock.patch('readthedocs.projects.tasks.update_docs_task') def test_trigger_custom_queue(self, update_docs): @@ -123,16 +119,8 @@ def test_trigger_build_time_limit(self, update_docs): 'time_limit': 720, 'soft_time_limit': 600, } - update_docs.signature.assert_has_calls([ - mock.call( - args=(self.version.pk,), - kwargs=kwargs, - options=options, - immutable=True, - ), - ]) update_docs.signature.assert_called_with( - args=(self.project.pk,), + args=(self.version.pk,), kwargs=kwargs, options=options, immutable=True, @@ -153,16 +141,8 @@ def test_trigger_build_invalid_time_limit(self, update_docs): 'time_limit': 720, 'soft_time_limit': 600, } - update_docs.signature.assert_has_calls([ - mock.call( - args=(self.version.pk,), - kwargs=kwargs, - options=options, - immutable=True, - ), - ]) update_docs.signature.assert_called_with( - args=(self.project.pk,), + args=(self.version.pk,), kwargs=kwargs, options=options, immutable=True, @@ -183,16 +163,8 @@ def test_trigger_build_rounded_time_limit(self, update_docs): 'time_limit': 3, 'soft_time_limit': 3, } - update_docs.signature.assert_has_calls([ - mock.call( - args=(self.version.pk,), - kwargs=kwargs, - options=options, - immutable=True, - ), - ]) update_docs.signature.assert_called_with( - args=(self.project.pk,), + args=(self.version.pk,), kwargs=kwargs, options=options, immutable=True, From 478935d2a71039b25c5b50c622cdbaee566a3150 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 6 Jun 2019 17:03:18 -0500 Subject: [PATCH 19/22] Fix mock --- readthedocs/rtd_tests/tests/test_privacy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/rtd_tests/tests/test_privacy.py b/readthedocs/rtd_tests/tests/test_privacy.py index e0c3565f721..7c75f14ff10 100644 --- a/readthedocs/rtd_tests/tests/test_privacy.py +++ b/readthedocs/rtd_tests/tests/test_privacy.py @@ -14,7 +14,7 @@ log = logging.getLogger(__name__) -@mock.patch('readthedocs.projects.tasks.clean_build_task.signature', new=mock.MagicMock) +@mock.patch('readthedocs.projects.tasks.clean_build', new=mock.MagicMock) @mock.patch('readthedocs.projects.tasks.update_docs_task.signature', new=mock.MagicMock) class PrivacyTests(TestCase): From 9ca1107436588340d5ed3105bda9296520c6601f Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 6 Jun 2019 17:03:27 -0500 Subject: [PATCH 20/22] Add tests --- readthedocs/rtd_tests/tests/test_celery.py | 56 ++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/readthedocs/rtd_tests/tests/test_celery.py b/readthedocs/rtd_tests/tests/test_celery.py index a7f122a7d1e..543519bfebe 100644 --- a/readthedocs/rtd_tests/tests/test_celery.py +++ b/readthedocs/rtd_tests/tests/test_celery.py @@ -159,12 +159,68 @@ def test_no_notification_on_version_locked_error(self, mock_setup_vcs, mock_send mock_send_notifications.assert_not_called() self.assertTrue(result.successful()) + @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.setup_python_environment', new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.setup_vcs', new=MagicMock) + @patch('readthedocs.doc_builder.environments.BuildEnvironment.update_build', new=MagicMock) + @patch('readthedocs.projects.tasks.clean_build') + @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.build_docs') + def test_clean_build_after_update_docs(self, build_docs, clean_build): + version = self.project.versions.first() + build = get( + Build, project=self.project, + version=version, + ) + with mock_api(self.repo) as mapi: + result = tasks.update_docs_task.delay( + version.pk, + build_pk=build.pk, + record=False, + intersphinx=False, + ) + self.assertTrue(result.successful()) + clean_build.assert_called_with(version.pk) + + @patch('readthedocs.projects.tasks.clean_build') + @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.run_setup') + def test_clean_build_after_failure_in_update_docs(self, run_setup, clean_build): + run_setup.side_effect = Exception() + version = self.project.versions.first() + build = get( + Build, project=self.project, + version=version, + ) + with mock_api(self.repo): + result = tasks.update_docs_task.delay( + version.pk, + build_pk=build.pk, + record=False, + intersphinx=False, + ) + clean_build.assert_called_with(version.pk) + def test_sync_repository(self): version = self.project.versions.get(slug=LATEST) with mock_api(self.repo): result = tasks.sync_repository_task.delay(version.pk) self.assertTrue(result.successful()) + @patch('readthedocs.projects.tasks.clean_build') + def test_clean_build_after_sync_repository(self, clean_build): + version = self.project.versions.get(slug=LATEST) + with mock_api(self.repo): + result = tasks.sync_repository_task.delay(version.pk) + self.assertTrue(result.successful()) + clean_build.assert_called_with(version.pk) + + @patch('readthedocs.projects.tasks.SyncRepositoryTaskStep.run') + @patch('readthedocs.projects.tasks.clean_build') + def test_clean_build_after_failure_in_sync_repository(self, clean_build, run_syn_repository): + run_syn_repository.side_effect = Exception() + version = self.project.versions.get(slug=LATEST) + with mock_api(self.repo): + result = tasks.sync_repository_task.delay(version.pk) + clean_build.assert_called_with(version.pk) + @patch('readthedocs.projects.tasks.api_v2') @patch('readthedocs.projects.models.Project.checkout_path') def test_check_duplicate_reserved_version_latest(self, checkout_path, api_v2): From 3e3a532abb65bec49984da87a018e36a824783da Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 6 Jun 2019 18:49:42 -0500 Subject: [PATCH 21/22] Ignore artifacts path --- readthedocs/projects/tasks.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 61c4d910b1a..74ac741c058 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -1397,9 +1397,11 @@ def clean_build(version_pk): version_pk, ) return False + # NOTE: we are skipping the deltetion of the `artifacts` dir + # because we are syncing the servers with an async task. del_dirs = [ os.path.join(version.project.doc_path, dir_, version.slug) - for dir_ in ('checkouts', 'artifacts', 'envs', 'conda') + for dir_ in ('checkouts', 'envs', 'conda') ] try: with version.project.repo_nonblockinglock(version): From 2d045fae2919f181cd67ea50200270255110c1c4 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Fri, 7 Jun 2019 10:45:51 -0500 Subject: [PATCH 22/22] Typo --- readthedocs/projects/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 74ac741c058..39fc4407043 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -1397,7 +1397,7 @@ def clean_build(version_pk): version_pk, ) return False - # NOTE: we are skipping the deltetion of the `artifacts` dir + # NOTE: we are skipping the deletion of the `artifacts` dir # because we are syncing the servers with an async task. del_dirs = [ os.path.join(version.project.doc_path, dir_, version.slug)