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

Build: ability to cancel a running build from dashboard #8850

Merged
merged 10 commits into from
Mar 1, 2022
18 changes: 18 additions & 0 deletions readthedocs/builds/migrations/0041_track_task_id.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Generated by Django 3.2.11 on 2022-01-26 20:10

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('builds', '0040_remove_old_jsonfields'),
]

operations = [
migrations.AddField(
model_name='build',
name='task_id',
field=models.CharField(blank=True, max_length=36, null=True, verbose_name='Celery task id'),
),
]
7 changes: 7 additions & 0 deletions readthedocs/builds/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -682,6 +682,13 @@ class Build(models.Model):
help_text='Build steps stored outside the database.',
)

task_id = models.CharField(
_('Celery task id'),
max_length=36,
null=True,
blank=True,
)

# Managers
objects = BuildQuerySet.as_manager()
# Only include BRANCH, TAG, UNKNOWN type Version builds.
Expand Down
47 changes: 46 additions & 1 deletion readthedocs/builds/views.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
"""Views for builds app."""

import signal

import structlog
import textwrap
from urllib.parse import urlparse
Expand All @@ -14,12 +16,15 @@
from django.views.generic import DetailView, ListView
from requests.utils import quote

from readthedocs.builds.constants import BUILD_STATE_TRIGGERED, BUILD_STATE_FINISHED
from readthedocs.builds.filters import BuildListFilter
from readthedocs.builds.models import Build, Version
from readthedocs.core.permissions import AdminPermission
from readthedocs.core.utils import trigger_build
from readthedocs.doc_builder.exceptions import BuildAppError
from readthedocs.doc_builder.exceptions import BuildAppError, BuildCancelled
from readthedocs.projects.models import Project
from readthedocs.worker import app


log = structlog.get_logger(__name__)

Expand Down Expand Up @@ -148,6 +153,46 @@ class BuildDetail(BuildBase, DetailView):

pk_url_kwarg = 'build_pk'

@method_decorator(login_required)
def post(self, request, project_slug, build_pk):
project = get_object_or_404(Project, slug=project_slug)
build = get_object_or_404(Build, pk=build_pk)

if not AdminPermission.is_admin(request.user, project):
return HttpResponseForbidden()

# NOTE: `terminate=True` is required for the child to attend our call
# immediately when it's running the build. Otherwise, it finishes the
# task. However, to revoke a task that has not started yet, we don't
# need it.
if build.state == BUILD_STATE_TRIGGERED:
# Since the task won't be executed at all, we need to update the
# Build object here.
terminate = False
build.state = BUILD_STATE_FINISHED
build.success = False
build.error = BuildCancelled.message
build.length = 0
build.save()
else:
# In this case, we left the update of the Build object to the task
# itself to be executed in the `on_failure` handler.
terminate = True

log.warning(
'Canceling build.',
project_slug=project.slug,
version_slug=build.version.slug,
build_id=build.pk,
build_task_id=build.task_id,
terminate=terminate,
)
app.control.revoke(build.task_id, signal=signal.SIGINT, terminate=terminate)
Copy link
Member Author

Choose a reason for hiding this comment

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

This could be a helper cancel_build that we could use from other places as well.


return HttpResponseRedirect(
reverse('builds_detail', args=[project.slug, build.pk]),
)

def get_context_data(self, **kwargs):
context = super().get_context_data(**kwargs)
context['project'] = self.project
Expand Down
13 changes: 12 additions & 1 deletion readthedocs/core/utils/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,18 @@ def trigger_build(project, version=None, commit=None):
# Build was skipped
return (None, None)

return (update_docs_task.apply_async(), build)
task = update_docs_task.apply_async()

# FIXME: I'm using `isinstance` here because I wasn't able to mock this
# properly when running tests and it fails when trying to save a
# `mock.Mock` object in the database.
#
# Store the task_id in the build object to be able to cancel it later.
if isinstance(task.id, (str, int)):
build.task_id = task.id
build.save()
Comment on lines +234 to +241
Copy link
Member Author

Choose a reason for hiding this comment

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

@stsewd do you have an idea how we can mock this? At this point update_docs_task and task are mock.Mock objects. It would be good if we can make task.id to return an integer or a string. I've tried using mock.return_value and others and I wasn't able to make it work.

Copy link
Member

Choose a reason for hiding this comment

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

Which test is running this code? Something like update_docs_task_mock.apply_async.return_value = MagicMock(id=1) should do it.

maybe update_docs_task_mock.apply_async().id = 1 could work as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Which test is running this code?

Most of the tests from readthedocs/projects/tests/test_build_tasks.py. For example, test_successful_build

The solution that you suggested is what I've tried and I wasn't able to mock them properly 😞

I'd appreciate it if you find out the way to mock it. Otherwise, I think I will commit the code with this if for now.

Copy link
Member

Choose a reason for hiding this comment

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

All tests from that file pass for me. But some tests from rtd_tests/tests/test_builds.py fail, but with a mock like this update_docs_task.signature().apply_async().id = 1 they pass

Copy link
Member Author

Choose a reason for hiding this comment

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

OK! I'm merging it for now because I want to deploy this change today. I'll come back to this in the following days. If you have the diff required at hand, can you open a PR with the changes?


return task, build


def send_email(
Expand Down
4 changes: 4 additions & 0 deletions readthedocs/doc_builder/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ class DuplicatedBuildError(BuildUserError):
status = BUILD_STATUS_DUPLICATED


class BuildCancelled(BuildUserError):
message = gettext_noop('Build cancelled by user.')


class MkDocsYAMLParseError(BuildUserError):
GENERIC_WITH_PARSE_EXCEPTION = gettext_noop(
'Problem parsing MkDocs YAML configuration. {exception}',
Expand Down
9 changes: 9 additions & 0 deletions readthedocs/projects/tasks/builds.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
BuildUserError,
BuildMaxConcurrencyError,
DuplicatedBuildError,
BuildCancelled,
ProjectBuildsSkippedError,
YAMLParseError,
MkDocsYAMLParseError,
Expand Down Expand Up @@ -240,6 +241,7 @@ class UpdateDocsTask(SyncRepositoryMixin, Task):
ProjectBuildsSkippedError,
ConfigError,
YAMLParseError,
BuildCancelled,
BuildUserError,
RepositoryError,
MkDocsYAMLParseError,
Expand All @@ -248,6 +250,7 @@ class UpdateDocsTask(SyncRepositoryMixin, Task):

# Do not send notifications on failure builds for these exceptions.
exceptions_without_notifications = (
BuildCancelled,
DuplicatedBuildError,
ProjectBuildsSkippedError,
)
Expand All @@ -265,10 +268,16 @@ def _setup_sigterm(self):
def sigterm_received(*args, **kwargs):
log.warning('SIGTERM received. Waiting for build to stop gracefully after it finishes.')

def sigint_received(*args, **kwargs):
log.warning('SIGINT received. Canceling the build running.')
raise BuildCancelled

# Do not send the SIGTERM signal to children (pip is automatically killed when
# receives SIGTERM and make the build to fail one command and stop build)
signal.signal(signal.SIGTERM, sigterm_received)

signal.signal(signal.SIGINT, sigint_received)

def _check_concurrency_limit(self):
try:
response = api_v2.build.concurrent.get(project__slug=self.data.project.slug)
Expand Down
7 changes: 7 additions & 0 deletions readthedocs/templates/builds/build_detail.html
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@
{% block content %}
<div class="build build-detail" id="build-detail">

{% if build.state != "finished" and request.user|is_admin:project %}
<form method="post" action="{% url "builds_detail" build.version.project.slug build.pk %}">
{% csrf_token %}
<input type="submit" value="{% trans "Cancel build" %}">
</form>
{% endif %}

<!-- Build meta data -->
<ul class="build-meta">
<div data-bind="visible: finished()">
Expand Down