-
Notifications
You must be signed in to change notification settings - Fork 32
🐛 LRT redis cleanup ⚠️🚨 #8539
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
🐛 LRT redis cleanup ⚠️🚨 #8539
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug in the long-running task (LRT) redis cleanup mechanism. The changes ensure proper cleanup of fire-and-forget tasks and correct identification of sidecar resources using service run IDs instead of node IDs.
Key Changes:
- Updated sidecar cleanup to use
service_run_idinstead ofnode_idfor proper namespace identification - Added tracking mechanism for fire-and-forget tasks to be removed after a timeout period once completed
- Introduced
detected_as_done_atfield to track when fire-and-forget tasks finish
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| services/director-v2/src/simcore_service_director_v2/modules/dynamic_sidecar/scheduler/_core/_events_utils.py | Updated cleanup function to use service_run_id instead of node_id for correct namespace identification |
| packages/service-library/src/servicelib/long_running_tasks/task.py | Implemented fire-and-forget task cleanup logic with timeout-based removal |
| packages/service-library/src/servicelib/long_running_tasks/models.py | Added detected_as_done_at field to track completion time of fire-and-forget tasks |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/service-library/src/servicelib/long_running_tasks/task.py
Outdated
Show resolved
Hide resolved
packages/service-library/src/servicelib/long_running_tasks/task.py
Outdated
Show resolved
Hide resolved
packages/service-library/src/servicelib/long_running_tasks/task.py
Outdated
Show resolved
Hide resolved
packages/service-library/src/servicelib/long_running_tasks/models.py
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8539 +/- ##
==========================================
+ Coverage 87.04% 87.37% +0.33%
==========================================
Files 2011 1578 -433
Lines 78751 65758 -12993
Branches 1365 684 -681
==========================================
- Hits 68545 57453 -11092
+ Misses 9803 8065 -1738
+ Partials 403 240 -163
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
🧪 CI InsightsHere's what we observed from your CI run for 234208f. ✅ Passed Jobs With Interesting Signals
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx. A test coverning this case (i.e. tasks not being deleted ) would also be a plus!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
Will you execute the clean up scripts? Shall I ping you during next staging and production release?
…nning-redis-cleanup
…HK/osparc-simcore-forked into pr-osparc-long-running-redis-cleanup
|



What do these changes do?
dynamic-sidecarwhere it was cleaning up the entries in Redis using thenode_idinstead of theservice_run_id.fire_and_forget=Truelong running tasks, which never had a removal procedureRelated issue/s
How to test
Dev-ops⚠️
When releasing this to any deployment, do the following:
rootuser (you have to type root since it's empty) to theredis-commandercontainer6Run the following cleanup