Skip to content

Conversation

@pcrespov
Copy link
Member

@pcrespov pcrespov commented Jun 24, 2025

What do these changes do?

This PR unifies how the peridoic background tasks are setup in the garbage-collector domain of the web-server

Highlights

  • create a safe function template for the cleanup_ctx event in tasks_utils.py
  • All background tasks within the garbage-collector are executed using exclusive periodic decorators ( servicelib.baground_tasks.exclusive_periodic )

Follow ups

Possible follow-ups (to discuss with @sanderegg ) in order to resolve #7931

  • split "core" task into independent background tasks?
  • increase test coverage
  • refactor tests
  • refactor garbage-collector domain skeleton
  • add a gc stand-alone boot app

Related issue/s

How to test

These are the (few) tests we have for the GC

  • services/web/server/tests/unit/isolated/test_garbage_collector_core.py
  • services/web/server/tests/integration/01/test_garbage_collection.py
  • services/web/server/tests/unit/with_dbs/04/garbage_collector/test_resource_manager.py
make build-devel
make up-devel
  • open redis commander
    image
  • tests
    • increase number of replicas
    • pause one and observe how the other takes over the background tasks

Dev-ops

None

@pcrespov pcrespov self-assigned this Jun 24, 2025
@codecov
Copy link

codecov bot commented Jun 24, 2025

Codecov Report

Attention: Patch coverage is 94.56522% with 5 lines in your changes missing coverage. Please review.

Project coverage is 87.76%. Comparing base (7f10771) to head (b4a644c).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7970      +/-   ##
==========================================
+ Coverage   87.61%   87.76%   +0.15%     
==========================================
  Files        1804     1424     -380     
  Lines       68935    59171    -9764     
  Branches     1231      627     -604     
==========================================
- Hits        60400    51934    -8466     
+ Misses       8179     7026    -1153     
+ Partials      356      211     -145     
Flag Coverage Δ
integrationtests 64.30% <94.38%> (+0.01%) ⬆️
unittests 86.08% <94.56%> (-0.10%) ⬇️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library ∅ <ø> (∅)
pkg_celery_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library 71.63% <100.00%> (+<0.01%) ⬆️
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 85.05% <ø> (ø)
agent 96.29% <ø> (ø)
api_server 92.64% <ø> (ø)
autoscaling 96.03% <ø> (∅)
catalog 92.29% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 91.79% <ø> (ø)
datcore_adapter 97.94% <ø> (ø)
director 76.73% <ø> (ø)
director_v2 91.11% <ø> (-0.02%) ⬇️
dynamic_scheduler 96.69% <ø> (ø)
dynamic_sidecar 90.09% <ø> (ø)
efs_guardian 89.65% <ø> (ø)
invitations 93.60% <ø> (ø)
payments 92.57% <ø> (ø)
resource_usage_tracker 89.00% <ø> (+0.21%) ⬆️
storage 86.23% <ø> (-0.09%) ⬇️
webclient ∅ <ø> (∅)
webserver 87.65% <94.38%> (+0.03%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f10771...b4a644c. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pcrespov pcrespov force-pushed the is7961/upgrade-gc-periodic-task branch from ba8a7c6 to f31912f Compare June 24, 2025 14:47
@pcrespov pcrespov added this to the Engage milestone Jun 24, 2025
@pcrespov pcrespov changed the title Is7961/upgrade gc periodic task ♻️ web-server: Upgrade GC periodic tasks to new servicelib.background_task Jun 24, 2025
@pcrespov pcrespov added the a:webserver webserver's codebase. Assigning the area is particularly useful for bugs label Jun 24, 2025
@pcrespov pcrespov marked this pull request as ready for review June 24, 2025 18:37
@pcrespov pcrespov changed the title ♻️ web-server: Upgrade GC periodic tasks to new servicelib.background_task ♻️ web-server: Upgrade GC periodic tasks to new servicelib.background_task Jun 24, 2025
Copy link
Contributor

Copilot AI left a 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 upgrades the garbage collector’s periodic background tasks by refactoring the task creation and management logic to use the new servicelib.background_task utilities. Key changes include:

  • Replacing the old run_background_task function with create_background_task_for_garbage_collection throughout the garbage collector.
  • Updating the tests and patch targets to align with the new background task pattern and redis-based exclusive periodic decorators.
  • Refactoring related task modules (_tasks_users, _tasks_trash, _tasks_api_keys) to use the new setup_periodic_task utility.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
services/web/server/tests/unit/isolated/test_garbage_collector_core.py Updated model example retrieval method.
services/web/server/tests/integration/01/test_garbage_collection.py Patching and status code changes to use new task creation function and servicelib.aiohttp status constants.
services/web/server/src/simcore_service_webserver/garbage_collector/plugin.py Adjusted cleanup_ctx registration with the new background task creator.
services/web/server/src/simcore_service_webserver/garbage_collector/_tasks_utils.py Introduced a generic setup_periodic_task utility for background tasks.
services/web/server/src/simcore_service_webserver/garbage_collector/_tasks_users.py Updated the expired users task with the exclusive_periodic decorator and refactored task creation.
services/web/server/src/simcore_service_webserver/garbage_collector/_tasks_trash.py Refactored pruning task to use setup_periodic_task and exclusive_periodic.
services/web/server/src/simcore_service_webserver/garbage_collector/_tasks_core.py Migrated the garbage collection loop to the setup_periodic_task framework with exclusive_periodic.
services/web/server/src/simcore_service_webserver/garbage_collector/_tasks_api_keys.py Updated API keys pruning task with exclusive_periodic and refactored task management.
packages/service-library/src/servicelib/background_task_utils.py Minor update to use f-string for error messaging.
packages/service-library/src/servicelib/background_task.py Updated docstring and parameter formatting for clarity.
Comments suppressed due to low confidence (2)

services/web/server/tests/integration/01/test_garbage_collection.py:203

  • Ensure that the updated patch target 'create_background_task_for_garbage_collection' properly replaces the old 'run_background_task' and that the tests accurately reflect the new background task lifecycle.
        "simcore_service_webserver.garbage_collector.plugin._tasks_core.create_background_task_for_garbage_collection",

services/web/server/src/simcore_service_webserver/garbage_collector/_tasks_users.py:70

  • [nitpick] Consider renaming 'create_background_task_for_trial_accounts' to better indicate that the task updates expired users if that is its primary function.
def create_background_task_for_trial_accounts(wait_s: float) -> CleanupContextFunc:

@pcrespov pcrespov force-pushed the is7961/upgrade-gc-periodic-task branch from 991a087 to de52738 Compare June 24, 2025 18:42
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

thanks

@pcrespov pcrespov enabled auto-merge (squash) June 26, 2025 07:25
@pcrespov
Copy link
Member Author

@mergify queue

@pcrespov pcrespov added the 🤖-automerge marks PR as ready to be merged for Mergify label Jun 26, 2025
@mergify
Copy link
Contributor

mergify bot commented Jun 26, 2025

queue

🛑 The pull request has been merged manually

The pull request has been merged manually at 6041cec

Copy link
Contributor

@bisgaard-itis bisgaard-itis left a comment

Choose a reason for hiding this comment

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

Thanks. Looks cool. Just one suggestion from my side.

@sonarqubecloud
Copy link

@pcrespov pcrespov merged commit 6041cec into ITISFoundation:master Jun 26, 2025
97 of 98 checks passed
@pcrespov pcrespov deleted the is7961/upgrade-gc-periodic-task branch June 26, 2025 11:46
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Aug 5, 2025
88 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖-automerge marks PR as ready to be merged for Mergify a:webserver webserver's codebase. Assigning the area is particularly useful for bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GC gets stuck and has to be rebooted Upgrade GC periodic tasks to new servicelib.background_task

5 participants