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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 aborting tasks fails #2449

Merged

Conversation

sanderegg
Copy link
Member

@sanderegg sanderegg commented Jul 28, 2021

What do these changes do?

Cancelling tasks seems to have been failing since a couple of PRs went in.
First, the python 3.8 PR changed how asyncio.loop.call_soon is to used.
Then, when the redis database was splitted in 3 separate DBs (one redis instance with 3 databases) instead of 1. the director-v2 celery client was not fixed accordingly.

This has lead to the following:

  1. Cancelling a pipeline would cancel the tasks AFTER the one currently running correctly. BUT the currently running tasks would still run until completion instead of being stopped.
  2. When a sidecar is shutdown, the currently running task would not be correctly cancelled.

Both of these issues are now fixed.

Related issue/s

How to test

make build-devel up-devel

then start a pipeline with 1sleeper that lasts more than 2 seconds... cancel it. it should instantly be cancelled.

Checklist

@sanderegg sanderegg added a:sidecar issue related with the sidecar worker service a:director-v2 issue related with the director-v2 service labels Jul 28, 2021
@sanderegg sanderegg added this to the Wombat milestone Jul 28, 2021
@sanderegg sanderegg self-assigned this Jul 28, 2021
@codecov
Copy link

codecov bot commented Jul 28, 2021

Codecov Report

Merging #2449 (f8eee49) into master (424d857) will decrease coverage by 0.0%.
The diff coverage is 53.8%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2449     +/-   ##
========================================
- Coverage    76.0%   75.9%   -0.1%     
========================================
  Files         574     574             
  Lines       21658   21667      +9     
  Branches     2078    2082      +4     
========================================
- Hits        16465   16464      -1     
- Misses       4639    4648      +9     
- Partials      554     555      +1     
Flag Coverage 螖
integrationtests 65.5% <47.8%> (-0.2%) 猬囷笍
unittests 69.9% <50.0%> (-0.1%) 猬囷笍

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage 螖
...ices/sidecar/src/simcore_service_sidecar/celery.py 0.0% <0.0%> (酶)
...r/src/simcore_service_sidecar/celery_task_utils.py 66.6% <酶> (+13.7%) 猬嗭笍
...vices/sidecar/src/simcore_service_sidecar/utils.py 64.7% <12.5%> (-15.3%) 猬囷笍
...sidecar/src/simcore_service_sidecar/celery_task.py 38.8% <50.0%> (酶)
...ervices/sidecar/src/simcore_service_sidecar/cli.py 65.8% <50.0%> (-1.7%) 猬囷笍
...es/settings-library/src/settings_library/celery.py 88.8% <83.3%> (+2.2%) 猬嗭笍
...es/sidecar/src/simcore_service_sidecar/rabbitmq.py 88.0% <88.8%> (-2.0%) 猬囷笍
..._director_v2/modules/dynamic_sidecar/docker_api.py 85.6% <0.0%> (+0.6%) 猬嗭笍

@sanderegg sanderegg requested review from GitHK and mguidon July 28, 2021 13:16
@sanderegg sanderegg changed the title 馃悰 fixes aborting tasks failing 馃悰 aborting tasks fails Jul 28, 2021
@sanderegg sanderegg requested a review from odeimaiz July 28, 2021 13:19
@sanderegg sanderegg marked this pull request as ready for review July 28, 2021 13:19
@sanderegg sanderegg force-pushed the bugfix/aborting_tasks_fails branch from 1a196f8 to 2b1fd4a Compare July 28, 2021 13:23
Copy link
Member

@mguidon mguidon left a comment

Choose a reason for hiding this comment

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

Thanks for fixing!

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

Looks OK, just some proposals to be verified.

@sanderegg sanderegg force-pushed the bugfix/aborting_tasks_fails branch from 2b1fd4a to f979547 Compare July 28, 2021 14:54
@sanderegg sanderegg merged commit b56d5b0 into ITISFoundation:master Jul 28, 2021
@sanderegg sanderegg deleted the bugfix/aborting_tasks_fails branch July 28, 2021 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:director-v2 issue related with the director-v2 service a:sidecar issue related with the sidecar worker service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants