Build: remove DEDUPLICATE_BUILDS feature

With the introduction of `CANCEL_OLD_BUILDS` we are moving away from the
`DEDUPLICATE_BUILDS` feature since these scenario is managed in a better way by
the new feature.

Closes #7306
Related #9549

Author: humitos

readthedocs/builds/constants.py

@@ -140,10 +140,8 @@
 }
 
 BUILD_STATUS_NORMAL = 'normal'
-BUILD_STATUS_DUPLICATED = 'duplicated'
 BUILD_STATUS_CHOICES = (
     (BUILD_STATUS_NORMAL, 'Normal'),
-    (BUILD_STATUS_DUPLICATED, 'Duplicated'),
 )
 
 

readthedocs/core/utils/__init__.py

@@ -1,12 +1,10 @@
 """Common utility functions."""
 
-import datetime
 import re
 import signal
 
 import structlog
 from django.conf import settings
-from django.utils import timezone
 from django.utils.functional import keep_lazy
 from django.utils.safestring import SafeText, mark_safe
 from django.utils.text import slugify as slugify_base
@@ -19,11 +17,7 @@
     BUILD_STATUS_PENDING,
     EXTERNAL,
 )
-from readthedocs.doc_builder.exceptions import (
-    BuildCancelled,
-    BuildMaxConcurrencyError,
-    DuplicatedBuildError,
-)
+from readthedocs.doc_builder.exceptions import BuildCancelled, BuildMaxConcurrencyError
 from readthedocs.worker import app
 
 log = structlog.get_logger(__name__)
@@ -147,63 +141,6 @@ def prepare_build(
         # we cancel all of them and trigger a new one for the latest commit received.
         for running_build in running_builds:
             cancel_build(running_build)
-    else:
-        # NOTE: de-duplicate builds won't be required if we enable `CANCEL_OLD_BUILDS`,
-        # since canceling a build is more effective.
-        # However, we are keepting `DEDUPLICATE_BUILDS` code around while we test
-        # `CANCEL_OLD_BUILDS` with a few projects and we are happy with the results.
-        # After that, we can remove `DEDUPLICATE_BUILDS` code
-        # and make `CANCEL_OLD_BUILDS` the default behavior.
-        if commit:
-            skip_build = (
-                Build.objects.filter(
-                    project=project,
-                    version=version,
-                    commit=commit,
-                )
-                .exclude(
-                    state__in=BUILD_FINAL_STATES,
-                )
-                .exclude(
-                    pk=build.pk,
-                )
-                .exists()
-            )
-        else:
-            skip_build = (
-                Build.objects.filter(
-                    project=project,
-                    version=version,
-                    state=BUILD_STATE_TRIGGERED,
-                    # By filtering for builds triggered in the previous 5 minutes we
-                    # avoid false positives for builds that failed for any reason and
-                    # didn't update their state, ending up on blocked builds for that
-                    # version (all the builds are marked as DUPLICATED in that case).
-                    # Adding this date condition, we reduce the risk of hitting this
-                    # problem to 5 minutes only.
-                    date__gte=timezone.now() - datetime.timedelta(minutes=5),
-                ).count()
-                > 1
-            )
-
-        if not project.has_feature(Feature.DEDUPLICATE_BUILDS):
-            log.debug(
-                "Skipping deduplication of builds. Feature not enabled.",
-            )
-            skip_build = False
-
-        if skip_build:
-            # TODO: we could mark the old build as duplicated, however we reset our
-            # position in the queue and go back to the end of it --penalization
-            log.warning(
-                "Marking build to be skipped by builder.",
-            )
-            build.error = DuplicatedBuildError.message
-            build.status = DuplicatedBuildError.status
-            build.exit_code = DuplicatedBuildError.exit_code
-            build.success = False
-            build.state = BUILD_STATE_CANCELLED
-            build.save()
 
     # Start the build in X minutes and mark it as limited
     if not skip_build and project.has_feature(Feature.LIMIT_CONCURRENT_BUILDS):

readthedocs/doc_builder/exceptions.py

@@ -2,7 +2,7 @@
 
 from django.utils.translation import gettext_noop
 
-from readthedocs.builds.constants import BUILD_STATE_CANCELLED, BUILD_STATUS_DUPLICATED
+from readthedocs.builds.constants import BUILD_STATE_CANCELLED
 from readthedocs.projects.constants import BUILD_COMMANDS_OUTPUT_PATH_HTML
 
 
@@ -56,13 +56,6 @@ class BuildMaxConcurrencyError(BuildUserError):
     message = gettext_noop('Concurrency limit reached ({limit}), retrying in 5 minutes.')
 
 
-class DuplicatedBuildError(BuildUserError):
-    message = gettext_noop('Duplicated build.')
-    exit_code = 1
-    status = BUILD_STATUS_DUPLICATED
-    state = BUILD_STATE_CANCELLED
-
-
 class BuildCancelled(BuildUserError):
     message = gettext_noop('Build cancelled by user.')
     state = BUILD_STATE_CANCELLED

readthedocs/projects/models.py

@@ -1863,7 +1863,6 @@ def add_features(sender, **kwargs):
     VCS_REMOTE_LISTING = "vcs_remote_listing"
     SPHINX_PARALLEL = "sphinx_parallel"
     USE_SPHINX_BUILDERS = "use_sphinx_builders"
-    DEDUPLICATE_BUILDS = "deduplicate_builds"
     CANCEL_OLD_BUILDS = "cancel_old_builds"
     DONT_CREATE_INDEX = "dont_create_index"
 
@@ -2012,10 +2011,6 @@ def add_features(sender, **kwargs):
             USE_SPHINX_BUILDERS,
             _('Use regular sphinx builders instead of custom RTD builders'),
         ),
-        (
-            DEDUPLICATE_BUILDS,
-            _('Mark duplicated builds as NOOP to be skipped by builders'),
-        ),
         (
             CANCEL_OLD_BUILDS,
             _(

readthedocs/projects/tasks/builds.py

@@ -44,7 +44,6 @@
     BuildCancelled,
     BuildMaxConcurrencyError,
     BuildUserError,
-    DuplicatedBuildError,
     MkDocsYAMLParseError,
     ProjectBuildsSkippedError,
     YAMLParseError,
@@ -276,7 +275,6 @@ class UpdateDocsTask(SyncRepositoryMixin, Task):
     # All exceptions generated by a user miss-configuration should be listed
     # here. Actually, every subclass of ``BuildUserError``.
     throws = (
-        DuplicatedBuildError,
         ProjectBuildsSkippedError,
         ConfigError,
         YAMLParseError,
@@ -291,14 +289,12 @@ class UpdateDocsTask(SyncRepositoryMixin, Task):
     exceptions_without_notifications = (
         BuildCancelled,
         BuildMaxConcurrencyError,
-        DuplicatedBuildError,
         ProjectBuildsSkippedError,
     )
 
     # Do not send external build status on failure builds for these exceptions.
     exceptions_without_external_build_status = (
         BuildMaxConcurrencyError,
-        DuplicatedBuildError,
     )
 
     acks_late = True
@@ -354,11 +350,6 @@ def _check_concurrency_limit(self):
                 )
             )
 
-    def _check_duplicated_build(self):
-        if self.data.build.get('status') == DuplicatedBuildError.status:
-            log.warning('NOOP: build is marked as duplicated.')
-            raise DuplicatedBuildError
-
     def _check_project_disabled(self):
         if self.data.project.skip:
             log.warning('Project build skipped.')
@@ -412,7 +403,6 @@ def before_start(self, task_id, args, kwargs):
         self._setup_sigterm()
 
         self._check_project_disabled()
-        self._check_duplicated_build()
         self._check_concurrency_limit()
         self._reset_build()
 

readthedocs/projects/views/public.py

@@ -1,12 +1,12 @@
 """Public project views."""
 
 import hashlib
-import structlog
 import mimetypes
 import os
 from collections import OrderedDict
 from urllib.parse import urlparse
 
+import structlog
 from django.conf import settings
 from django.contrib import messages
 from django.db.models import prefetch_related_objects
@@ -22,7 +22,7 @@
 
 from readthedocs.analytics.tasks import analytics_event
 from readthedocs.analytics.utils import get_client_ip
-from readthedocs.builds.constants import BUILD_STATUS_DUPLICATED, LATEST
+from readthedocs.builds.constants import LATEST
 from readthedocs.builds.models import Version
 from readthedocs.builds.views import BuildTriggerMixin
 from readthedocs.core.permissions import AdminPermission
@@ -184,20 +184,6 @@ def get(self, request, project_slug, *args, **kwargs):
                 slug=version_slug,
             ).first()
 
-        if version:
-            last_build = (
-                version.builds
-                .filter(type='html', state='finished')
-                .exclude(status=BUILD_STATUS_DUPLICATED)
-                .order_by('-date')
-                .first()
-            )
-            if last_build:
-                if last_build.success:
-                    status = self.STATUS_PASSING
-                else:
-                    status = self.STATUS_FAILING
-
         return self.serve_badge(request, status)
 
     def get_style(self, request):

readthedocs/rtd_tests/tests/test_api.py

@@ -40,7 +40,6 @@
 from readthedocs.builds.constants import (
     BUILD_STATE_CLONING,
     BUILD_STATE_TRIGGERED,
-    BUILD_STATUS_DUPLICATED,
     EXTERNAL,
     EXTERNAL_VERSION_STATE_CLOSED,
     LATEST,
@@ -105,7 +104,6 @@ def test_reset_build(self):
             project=self.project,
             version=self.version,
             state=BUILD_STATE_CLONING,
-            status=BUILD_STATUS_DUPLICATED,
             success=False,
             output='Output',
             error='Error',

readthedocs/rtd_tests/tests/test_builds.py

@@ -1,5 +1,4 @@
 import datetime
-from unittest import mock
 
 from django.contrib.auth.models import User
 from django.test import TestCase
@@ -14,9 +13,7 @@
     GITLAB_EXTERNAL_VERSION_NAME,
 )
 from readthedocs.builds.models import Build, Version
-from readthedocs.core.utils import trigger_build
-from readthedocs.doc_builder.exceptions import DuplicatedBuildError
-from readthedocs.projects.models import Feature, Project
+from readthedocs.projects.models import Project
 
 
 class BuildModelTests(TestCase):
@@ -482,112 +479,3 @@ def test_can_rebuild_with_old_build(self):
 
         self.assertFalse(old_external_build.can_rebuild)
         self.assertTrue(latest_external_build.can_rebuild)
-
-
-@mock.patch('readthedocs.projects.tasks.builds.update_docs_task')
-class DeDuplicateBuildTests(TestCase):
-
-    def setUp(self):
-        self.project = get(Project)
-        self.version = get(
-            Version,
-            project=self.project,
-            type=BRANCH,
-        )
-
-        get(
-            Feature,
-            feature_id=Feature.DEDUPLICATE_BUILDS,
-            projects=[self.project],
-        )
-
-    def test_trigger_duplicated_build_by_commit(self, update_docs_task):
-        """
-        Trigger a build for the same commit twice.
-
-        The second build should be marked as NOOP.
-        """
-        self.assertEqual(Build.objects.count(), 0)
-        trigger_build(self.project, self.version, commit='a1b2c3')
-        self.assertEqual(Build.objects.count(), 1)
-        build = Build.objects.first()
-        self.assertEqual(build.state, 'triggered')
-
-        trigger_build(self.project, self.version, commit='a1b2c3')
-        self.assertEqual(Build.objects.count(), 2)
-        build = Build.objects.first()
-        self.assertEqual(build.error, DuplicatedBuildError.message)
-        self.assertEqual(build.success, False)
-        self.assertEqual(build.exit_code, DuplicatedBuildError.exit_code)
-        self.assertEqual(build.status, DuplicatedBuildError.status)
-        self.assertEqual(build.state, "cancelled")
-
-    def test_trigger_duplicated_finshed_build_by_commit(self, update_docs_task):
-        """
-        Trigger a build for the same commit twice.
-
-        The second build should not be marked as NOOP if the previous
-        duplicated builds are in 'finished' state.
-        """
-        self.assertEqual(Build.objects.count(), 0)
-        trigger_build(self.project, self.version, commit='a1b2c3')
-        self.assertEqual(Build.objects.count(), 1)
-
-        # Mark the build as finished
-        build = Build.objects.first()
-        build.state = 'finished'
-        build.save()
-        build.refresh_from_db()
-
-        trigger_build(self.project, self.version, commit='a1b2c3')
-        self.assertEqual(Build.objects.count(), 2)
-        build = Build.objects.first()
-        self.assertEqual(build.state, 'triggered')
-        self.assertIsNone(build.status)
-
-    def test_trigger_duplicated_build_by_version(self, update_docs_task):
-        """
-        Trigger a build for the same version.
-
-        The second build should be marked as NOOP if there is already a build
-        for the same project and version on 'triggered' state.
-        """
-        self.assertEqual(Build.objects.count(), 0)
-        trigger_build(self.project, self.version, commit=None)
-        self.assertEqual(Build.objects.count(), 1)
-        build = Build.objects.first()
-        self.assertEqual(build.state, 'triggered')
-
-        trigger_build(self.project, self.version, commit=None)
-        self.assertEqual(Build.objects.count(), 2)
-        build = Build.objects.first()
-        self.assertEqual(build.error, DuplicatedBuildError.message)
-        self.assertEqual(build.success, False)
-        self.assertEqual(build.exit_code, DuplicatedBuildError.exit_code)
-        self.assertEqual(build.status, DuplicatedBuildError.status)
-        self.assertEqual(build.state, "cancelled")
-
-    def test_trigger_duplicated_non_triggered_build_by_version(self, update_docs_task):
-        """
-        Trigger a build for the same version.
-
-        The second build should not be marked as NOOP because the previous build
-        for the same project and version is on 'building' state (any non 'triggered')
-        """
-        self.assertEqual(Build.objects.count(), 0)
-        trigger_build(self.project, self.version, commit=None)
-        self.assertEqual(Build.objects.count(), 1)
-        build = Build.objects.first()
-        self.assertEqual(build.state, 'triggered')
-
-        # Mark the build as building
-        build = Build.objects.first()
-        build.state = 'building'
-        build.save()
-        build.refresh_from_db()
-
-        trigger_build(self.project, self.version, commit=None)
-        self.assertEqual(Build.objects.count(), 2)
-        build = Build.objects.first()
-        self.assertEqual(build.state, 'triggered')
-        self.assertIsNone(build.status)