Build: ability to cancel a running build from dashboard #8850
Conversation
|
We could also revoke a task that hasn't started yet as noted by @astrojuanlu in #7660 (comment)
|
|
@agjohnson since you are reviewing the PR for the new Celery handlers, it would be good to take a quick look at this one as well to get some feedback about the implementation. |
| # itself to be executed in the `on_failure` handler. | ||
| terminate = True | ||
|
|
||
| app.control.revoke(build.task_id, signal=signal.SIGINT, terminate=terminate) |
There was a problem hiding this comment.
This could be a helper cancel_build that we could use from other places as well.
|
Yay! I'm happy that you are +1 on this. The only thing that I want to test a little more is what happens with supervisord and the celery process itself. I tested it locally and it worked fine, but I'd expect supervisor to restart celery process after a build was canceled because |
We tried to implement this in another opportunities (see #7031) but the build process was really complex and we had to manage the exception in multiple places. After implementing Celery Handlers, we can just raise the exception when attending to the proper signal coming from `app.control.revoke` and handle it properly from `on_failure` task's method. All the initial local tests were great!
humitos
force-pushed
the
humitos/cancel-build
branch
from
d133ddc to
5066e64
Compare
humitos
force-pushed
the
humitos/cancel-build
branch
from
5066e64 to
a3078ee
Compare
|
I QAed ths locally and the I'm targeting this feature for next deploy, on 2022-03-01. |
`task.id` is a mock.Mock object and it's not possible to save it into the database. I've tried using `.return_value` and others but I failed when trying to mock it and return a valid str/int. So, for now, we can keep this hack here and only save the value into the database if it's a str/int.
| # 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() |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
We tried to implement this in another opportunities (see
#7031) but the build process
was really complex and we had to manage the exception in multiple places.
After implementing Celery Handlers, we can just raise the exception when
attending to the proper signal coming from
app.control.revokeand handle itproperly from
on_failuretask's method.All the initial local tests were great!
Closes #7660
Closes #8772