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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add task scheduler to backend services (cron jobs) #2147

Draft
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

spwoodcock
Copy link
Member

@spwoodcock spwoodcock commented Feb 4, 2025

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation
  • πŸ§‘β€πŸ’» Refactor
  • βœ… Test
  • πŸ€– Build or CI
  • ❓ Other (please specify)

Related Issue

Describe this PR

  • Using cron in a container without root is actually really difficult. We need root to run the cron daemon.
  • Instead I added supercronic, a go binary specifically built to replace cron in containers.
  • This runs as part of the local development compose stack.
  • As we don't build a specific container for this, it's much more flexible. We can replace the compose service with a Kubernetes CronJob as needed.
  • The python scripts to run are under /src/backend/scheduled/xxx.py
  • TODO:
    • Update the code in the linked PRs so the logic happens entirely in xxx_crud.py modules. The xxx_routes.py code should simply call the logic. This way we can replicate this in the /src/backend/scheduled/xxx.py too.
    • Remove any requirement on the request: Request var from logic, as we can't use this on a schedule (instead we need to use a service account OSM token to send emails).
    • Test thoroughly.
    • Add compose services to remaining compose files + deploy.

Alternative Approaches Considered

  • I tried many ways of using cron. All pretty impossible without root, if we want to receive any kind of logging.
  • I also considered an approach using a simple sleep command: https://gist.github.com/mowings/a32fbf4dd46d9fb9661822056ceda91c, but it's very difficult to work the syntax as part of the compose command: section.

Review Guide

  • For testing, replace the cron schedule in compose.yaml with * * * * * to run every minute.

Checklist before requesting a review

[optional] What gif best describes this PR or how it makes you feel?

@spwoodcock spwoodcock added this to the (v2025.2.0) future release milestone Feb 4, 2025
@github-actions github-actions bot added enhancement New feature or request backend Related to backend code migration Contains a DB migration labels Feb 4, 2025
@spwoodcock
Copy link
Member Author

I won't work any more on this for now - moving onto MdP work.

Everything is in place to continue the work for unlocking inactive users and tasks @Anuj-Gupta4 πŸ™

Although I would say this is one of the lower priority items on our roadmap, to be addressed by anyone when there is some free time.

Copy link
Collaborator

@Anuj-Gupta4 Anuj-Gupta4 left a comment

Choose a reason for hiding this comment

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

This looks great! I'll make a few minor adjustments to refine it for now and come back to it later.

# Task unlocking every 3hrs
echo '* */3 * * * /opt/scheduler/unlock_tasks.py' > ./crontab

# Task unlocking every Sunday 00:00
Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, this should say 'Check inactive users every Sunday 00:00

try:
async with await AsyncConnection.connect(settings.FMTM_DB_URL) as db:
log.info("Starting processing inactive users")
await process_inactive_users(db)
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to be fixed

try:
async with await AsyncConnection.connect(str(settings.FMTM_DB_URL)) as db:
log.info("Starting processing inactive tasks")
await trigger_unlock_tasks(db)
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to be fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Related to backend code enhancement New feature or request migration Contains a DB migration
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants