Add clean up job working directory as celery task #15816
Conversation
lib/galaxy/celery/tasks.py
Outdated
| galaxy_log_dir = "/var/log/galaxy" | ||
| log_file_name = f"jwds_cleanup_{datetime.datetime.now().strftime('%d_%m_%Y-%I_%M_%S')}.log" |
There was a problem hiding this comment.
I'm not sure about this, I think this should just be log.info to the system log. I don't want more files i need to manage and ensure are properly rotated.
There was a problem hiding this comment.
Ok, I will modify that and push a new commit
|
You don't need to merge dev into your branch unless there are conflicts. |
lib/galaxy/celery/tasks.py
Outdated
| @@ -1,7 +1,10 @@ | |||
| import json | |||
| from concurrent.futures import TimeoutError | |||
| import datetime | |||
There was a problem hiding this comment.
make format can fix that for you
There was a problem hiding this comment.
Will do that and push a new commit.
sanjaysrikakulam
force-pushed
the
galaxy_jwd
branch
from
1c82ab4 to
44db84f
Compare
lib/galaxy/celery/tasks.py
Outdated
| log.error(f"Error deleting job working directory: {jwd_path} : {e.strerror}") | ||
|
|
||
| # days should be converted to a config option | ||
| days = 5 |
There was a problem hiding this comment.
Can you move that into the task signature ?
|
Does this supersede #15618 ? should we close that one? |
| def get_failed_jobs(): | ||
| failed_jobs = {} | ||
| jobs = sa_session.query(model.Job).filter( | ||
| model.Job.state == "error", |
There was a problem hiding this comment.
it might make sense to check against all terminal states here (not in ok, queued, running, new), but maybe there's a better way to do that. I know that @natefoo and I occasionally manually fail jobs with a state liked manually_failed and it'd be good to make sure we're getting all terminal jobs.
There was a problem hiding this comment.
I think we can tweak this after the first version is merged.
Yes, I will close that PR now |
|
Wondering what the advantage of this is over |
|
The cron job can not easily know if the job is still running or when it failed in a generic way. |
|
Bumping this to 23.2 |
|
@sanjaysrikakulam can you rebase this PR. What is needed to get this one in? |
Was unable to rebase, so fixed the conflicts manually. |
|
@sanjaysrikakulam can you please rebase this again, or merge manually? |
Co-authored-by: Marius van den Beek <[email protected]>
Co-authored-by: Marius van den Beek <[email protected]>
Co-authored-by: Marius van den Beek <[email protected]>
sanjaysrikakulam
force-pushed
the
galaxy_jwd
branch
from
951b5a1 to
3ebe869
Compare
1. datetime.timedelta takes only float 2. sqlalchemy has .isnot for filtering
…tion Fixes a type error where Optional[int] caused incompatibility with datetime.timedelta(days=...). Removed the Optional annotation from the days parameter since it always defaults to 5, ensuring days is consistently treated as an integer and avoiding MyPy complaints.
I have rebased it again and fixed lint issues and errors. @mvdbeek please review. |
|
Thanks @sanjaysrikakulam! |
|
This PR was merged without a "kind/" label, please correct. |
WHAT:
Adds a new celery task to clean up job working directories for failed jobs that are older than X days
See the initial PR: #15618 for the discussion
How to test the changes?
(Select all options that apply)
License
fixes #15977