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

Simple task to finish inactive builds #3312

Merged
merged 4 commits into from
Dec 14, 2017
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions readthedocs/projects/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

from __future__ import absolute_import

import datetime
import hashlib
import json
import logging
Expand All @@ -20,6 +21,7 @@
from celery.exceptions import SoftTimeLimitExceeded
from django.conf import settings
from django.core.urlresolvers import reverse
from django.db.models import Q
from django.utils.translation import ugettext_lazy as _
from readthedocs_build.config import ConfigError
from slumber.exceptions import HttpClientError
Expand All @@ -41,6 +43,7 @@
from readthedocs.core.symlink import PublicSymlink, PrivateSymlink
from readthedocs.core.utils import send_email, broadcast
from readthedocs.doc_builder.config import load_yaml_config
from readthedocs.doc_builder.constants import DOCKER_LIMITS
from readthedocs.doc_builder.environments import (LocalEnvironment,
DockerEnvironment)
from readthedocs.doc_builder.exceptions import BuildEnvironmentError
Expand Down Expand Up @@ -1005,3 +1008,48 @@ def sync_callback(_, version_pk, commit, *args, **kwargs):
"""
fileify(version_pk, commit=commit)
update_search(version_pk, commit=commit)


@app.task()
def finish_inactive_builds():
"""
Finish inactive builds.

A build is consider inactive if it's not in ``FINISHED`` state and it has been
"running" for more time that the allowed one (``Project.container_time_limit``
or ``DOCKER_LIMITS['time']`` plus a 20% of it).

These inactive builds will be marked as ``success`` and ``FINISHED`` with an
``error`` to be communicated to the user.
"""
time_limit = int(DOCKER_LIMITS['time'] * 1.2)
delta = datetime.timedelta(minutes=time_limit)
query = (~Q(state=BUILD_STATE_FINISHED) &
Q(date__lte=datetime.datetime.now() - delta))

builds_finished = 0
builds = Build.objects.filter(query)[:50]
for build in builds:

if build.project.container_time_limit:
custom_delta = datetime.timedelta(
minutes=int(build.project.container_time_limit))
Copy link
Contributor

Choose a reason for hiding this comment

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

container_time_limit is in seconds, so delta will need to be changed here

if build.date + custom_delta > datetime.datetime.now():
# Do not mark as FINISHED builds with a custom time limit that wasn't
# expired yet (they are still building the project version)
continue

build.success = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's check for the timeout date here, ie - if build.date + build.project.container_time_limit > now: continue.

So the query will surface extra builds, but the custom timeout builds will be an exception to this case. They keep running through this loop until the condition above is true.

Does this seem reasonable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I think this is the easiest and more reasonable thing to do.

"I was overengeering it" as Eric said :)

build.state = BUILD_STATE_FINISHED
build.error = _(
'This build was terminated due to inactivity. If you '
'continue to encounter this error, file a support '
'request with and reference this build id ({0}).'.format(build.pk)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be translated, though I'm not sure if we're equipped to actually translate this yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Equipped? In what sense?

I don't understand how/where the translation are working yet.

build.save()
builds_finished += 1

log.info(
'Builds marked as "Terminated due inactivity": %s',
builds_finished,
)