add FlareCleanupTask #947
add FlareCleanupTask #947
Conversation
|
This PR includes changes to |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #947 +/- ##
===========================================
- Coverage 97.99% 26.14% -71.85%
===========================================
Files 445 447 +2
Lines 35754 35838 +84
===========================================
- Hits 35036 9371 -25665
- Misses 718 26467 +25749
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
|
✅ All tests successful. No failed tests were found. 📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
There was a problem hiding this comment.
A couple of suggestions:
- We should definitely limit / batch the query. This could yield millions of entries on the first run for all we know 🤷🏻♂️
- Using the
mock_storagefixture, you could assert the actual side effect of files being removed from the storage backend, instead of manually mocking very specificArchiveServicemethods
Otherwise this looks good, and the following are just "personal preference" suggestions:
- IMO, we should move core business logic out of the celery framework code. In this case this would mean having some free functions instead of all the logic being in
FlareCleanupTask.run_cron_task. This means you don’t even need thedb_sessionargument, as the code does not use sqlalchemy models, and it should make testing less awkward. - I’m personally not a fan of asserting log calls, but rather just the side effects we are aiming for: files being removed from storage, fields being cleared from the DB.
| # single query, loads all pulls and repos in qset into memory, deletes file in Archive 1 by 1 | ||
| for pull in non_open_pulls_with_flare_in_archive: |
There was a problem hiding this comment.
In particular, the "load all pulls … into memory" part can be potentially dangerous as it might overwhelm the task.
This should use a limit query and work in batches.
| archive_service = ArchiveService(repository=pull.repository) | ||
| archive_service.delete_file(pull._flare_storage_path) |
There was a problem hiding this comment.
I believe it might be worth grouping these by repository(.id) on the python side, to deduplicate creation of per-repo ArchiveService. Plus, I believe there is a delete_files method to do batch deletions as well. Though I believe that depending on the storage backend, that will still delete each file individually.
| _flare_storage_path__isnull=False | ||
| ).select_related("repository") | ||
| log.info( | ||
| f"FlareCleanupTask will clear {non_open_pulls_with_flare_in_archive.count()} Archive flares" |
There was a problem hiding this comment.
I believe this .count() is a query on its own, which seems a bit unreasonable just for a log line?
| mock_archive_service = mocker.patch( | ||
| "shared.django_apps.utils.model_utils.ArchiveService" | ||
| ) |
There was a problem hiding this comment.
Instead of mocking the ArchiveService, you could use the mock_storage fixture (I believe that should be hooked up to the shared archive service).
That way, you can actually store files (in memory), and assert that those files are truely being deleted from the storage.
New strategy for managing
flarestoragedepends on codecov/shared#450
When a
pullis no longerOPEN, leave theflarein order to reduce locks and wait time.During low traffic hours (4am UTC?), run this task to clear it out of our database and Archive storage.
So we won't pay for maintaining the
flarefield but also won't slow down otherpullactions by checking or clearing the field 🎉