added time limit to celery tasks #743
Conversation
Signed-off-by: swastik959 <[email protected]>
There was a problem hiding this comment.
Hi @swastik959,
Thank you for your contribution! 🎉
I left a comment for us to elaborate more this PR
…ded them on task level Signed-off-by: swastik959 <[email protected]>
|
@kairoaraujo I have done the relevant changes as suggested by you please review them |
kairoaraujo
changed the title
There was a problem hiding this comment.
I reviewed it and it looks good for repository_service_tuf_worker main tasks.
I just miss the UT for the app.py (see tests/unit/test_app.py)
Please, see my commends about the other tasks
Great work @swastik959
|
|
||
|
|
||
| @app.task(serializer="json", queue="rstuf_internals") | ||
| @app.task(serializer="json", queue="rstuf_internals" , task_time_limit=TASK_TIME_LIMIT , task_soft_time_limit=TASK_SOFT_TIME_LIMIT) |
There was a problem hiding this comment.
I'm not sure if we should put the task_[soft]_limit for the bump_online_role right now, for some reasons:
- It will conflict directly with the
BOR_LOCKwe have to handle - Depending on the number of delegated online roles, it will be a problem
- Using the same task limit time to the add/remove artifacts tasks
I'm in favor here of filing a new specific issue for adding task time limit for bump_online_roles
|
|
||
| @app.task(serializer="json", queue="rstuf_internals") | ||
| @app.task(serializer="json", queue="rstuf_internals", task_time_limit=TASK_TIME_LIMIT , task_soft_time_limit=TASK_SOFT_TIME_LIMIT) |
There was a problem hiding this comment.
(same as _update_online_role)
| @@ -130,56 +138,68 @@ def _end_bor_chain_callback(result, start_time: float): | |||
| return {"result": result, "execution_time_seconds": total_time} | |||
|
|
|||
|
|
|||
| @app.task(serializer="json", queue="rstuf_internals") | |||
| @app.task(serializer="json", queue="rstuf_internals", task_time_limit=TASK_TIME_LIMIT , task_soft_time_limit=TASK_SOFT_TIME_LIMIT) | |||
There was a problem hiding this comment.
this task is a chain task from bump_online_roles.
I think we should use the same config here, see my comment on bump_online_roles -- I think it should part of a specific change from there.
|
@kairoaraujo should I just undo the changes in other functions apart from main one ? |
added timr limit to celery tasks in app.py for around an hour
solves #708