add FlareCleanupTask #947

nora-codecov · 2024-12-06T07:36:37Z

New strategy for managing flare storage

When a pull is no longer OPEN, leave the flare in 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 flare field but also won't slow down other pull actions by checking or clearing the field 🎉

github-actions · 2024-12-06T07:36:53Z

This PR includes changes to shared. Please review them here: codecov/shared@45252f7...9f5c4fd

codecov · 2024-12-06T07:42:30Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
1769	1	1768	1

View the top 1 failed tests by shortest run time

tasks/tests/unit/test_flare_cleanup.py::TestFlareCleanupTask::test_successful_run

Stack Traces | 0.309s run time

self = <worker.tasks.tests.unit.test_flare_cleanup.TestFlareCleanupTask object at 0x7f4e421ca210>
transactional_db = None
mocker = <pytest_mock.plugin.MockFixture object at 0x7f4e3be52510>

    def test_successful_run(self, transactional_db, mocker):
        mock_logs = mocker.patch("logging.Logger.info")
        mock_archive_service = mocker.patch(
            "shared.django_apps.utils.model_utils.ArchiveService"
        )
        archive_value_for_flare = {"some": "data"}
        mock_archive_service.return_value.read_file.return_value = json.dumps(
            archive_value_for_flare
        )
        mock_path = ".../to/written/object"
        mock_archive_service.return_value.write_json_data_to_storage.return_value = (
            mock_path
        )
        mock_archive_service_in_task = mocker.patch(
            "tasks.flare_cleanup.ArchiveService"
        )
        mock_archive_service_in_task.return_value.delete_file.return_value = None
    
        local_value_for_flare = {"test": "test"}
        open_pull_with_local_flare = PullFactory(
            state=PullStates.OPEN.value,
            _flare=local_value_for_flare,
            repository=RepositoryFactory(),
        )
>       assert open_pull_with_local_flare.flare == local_value_for_flare
E       AssertionError: assert {} == {'test': 'test'}
E         
E         Right contains 1 more item:
E         {'test': 'test'}
E         Use -v to get more diff

.../tests/unit/test_flare_cleanup.py:42: AssertionError

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

codecov-notifications · 2024-12-06T07:42:39Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
1769	1	1768	1

View the top 1 failed tests by shortest run time

tasks/tests/unit/test_flare_cleanup.py::TestFlareCleanupTask::test_successful_run

Stack Traces | 0.309s run time

self = <worker.tasks.tests.unit.test_flare_cleanup.TestFlareCleanupTask object at 0x7f4e421ca210>
transactional_db = None
mocker = <pytest_mock.plugin.MockFixture object at 0x7f4e3be52510>

    def test_successful_run(self, transactional_db, mocker):
        mock_logs = mocker.patch("logging.Logger.info")
        mock_archive_service = mocker.patch(
            "shared.django_apps.utils.model_utils.ArchiveService"
        )
        archive_value_for_flare = {"some": "data"}
        mock_archive_service.return_value.read_file.return_value = json.dumps(
            archive_value_for_flare
        )
        mock_path = ".../to/written/object"
        mock_archive_service.return_value.write_json_data_to_storage.return_value = (
            mock_path
        )
        mock_archive_service_in_task = mocker.patch(
            "tasks.flare_cleanup.ArchiveService"
        )
        mock_archive_service_in_task.return_value.delete_file.return_value = None
    
        local_value_for_flare = {"test": "test"}
        open_pull_with_local_flare = PullFactory(
            state=PullStates.OPEN.value,
            _flare=local_value_for_flare,
            repository=RepositoryFactory(),
        )
>       assert open_pull_with_local_flare.flare == local_value_for_flare
E       AssertionError: assert {} == {'test': 'test'}
E         
E         Right contains 1 more item:
E         {'test': 'test'}
E         Use -v to get more diff

.../tests/unit/test_flare_cleanup.py:42: AssertionError

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

github-actions · 2024-12-06T07:42:56Z

✅ All tests successful. No failed tests were found.

📣 Thoughts on this report? Let Codecov know! | Powered by Codecov

Swatinem

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_storage fixture, you could assert the actual side effect of files being removed from the storage backend, instead of manually mocking very specific ArchiveService methods

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 the db_session argument, 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.

Swatinem · 2024-12-06T08:29:42Z

tasks/flare_cleanup.py

+        # 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:


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.

Swatinem · 2024-12-06T08:31:11Z

tasks/flare_cleanup.py

+            archive_service = ArchiveService(repository=pull.repository)
+            archive_service.delete_file(pull._flare_storage_path)


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.

Swatinem · 2024-12-06T08:32:14Z

tasks/flare_cleanup.py

+            _flare_storage_path__isnull=False
+        ).select_related("repository")
+        log.info(
+            f"FlareCleanupTask will clear {non_open_pulls_with_flare_in_archive.count()} Archive flares"


I believe this .count() is a query on its own, which seems a bit unreasonable just for a log line?

Swatinem · 2024-12-06T08:34:20Z

tasks/tests/unit/test_flare_cleanup.py

+        mock_archive_service = mocker.patch(
+            "shared.django_apps.utils.model_utils.ArchiveService"
+        )


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.

github-actions · 2024-12-28T01:58:32Z

This PR includes changes to shared. Please review them here: codecov/shared@2674ae9...741151b

codecov-qa · 2024-12-28T02:33:57Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
1769	1	1768	1

View the top 1 failed tests by shortest run time

tasks/tests/unit/test_flare_cleanup.py::TestFlareCleanupTask::test_successful_run

Stack Traces | 0.309s run time

self = <worker.tasks.tests.unit.test_flare_cleanup.TestFlareCleanupTask object at 0x7f4e421ca210>
transactional_db = None
mocker = <pytest_mock.plugin.MockFixture object at 0x7f4e3be52510>

    def test_successful_run(self, transactional_db, mocker):
        mock_logs = mocker.patch("logging.Logger.info")
        mock_archive_service = mocker.patch(
            "shared.django_apps.utils.model_utils.ArchiveService"
        )
        archive_value_for_flare = {"some": "data"}
        mock_archive_service.return_value.read_file.return_value = json.dumps(
            archive_value_for_flare
        )
        mock_path = ".../to/written/object"
        mock_archive_service.return_value.write_json_data_to_storage.return_value = (
            mock_path
        )
        mock_archive_service_in_task = mocker.patch(
            "tasks.flare_cleanup.ArchiveService"
        )
        mock_archive_service_in_task.return_value.delete_file.return_value = None
    
        local_value_for_flare = {"test": "test"}
        open_pull_with_local_flare = PullFactory(
            state=PullStates.OPEN.value,
            _flare=local_value_for_flare,
            repository=RepositoryFactory(),
        )
>       assert open_pull_with_local_flare.flare == local_value_for_flare
E       AssertionError: assert {} == {'test': 'test'}
E         
E         Right contains 1 more item:
E         {'test': 'test'}
E         Use -v to get more diff

.../tests/unit/test_flare_cleanup.py:42: AssertionError

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

codecov-public-qa · 2024-12-28T02:34:05Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
1769	1	1768	1

View the top 1 failed tests by shortest run time

tasks/tests/unit/test_flare_cleanup.py::TestFlareCleanupTask::test_successful_run

Stack Traces | 0.309s run time

self = <worker.tasks.tests.unit.test_flare_cleanup.TestFlareCleanupTask object at 0x7f4e421ca210>
transactional_db = None
mocker = <pytest_mock.plugin.MockFixture object at 0x7f4e3be52510>

    def test_successful_run(self, transactional_db, mocker):
        mock_logs = mocker.patch("logging.Logger.info")
        mock_archive_service = mocker.patch(
            "shared.django_apps.utils.model_utils.ArchiveService"
        )
        archive_value_for_flare = {"some": "data"}
        mock_archive_service.return_value.read_file.return_value = json.dumps(
            archive_value_for_flare
        )
        mock_path = ".../to/written/object"
        mock_archive_service.return_value.write_json_data_to_storage.return_value = (
            mock_path
        )
        mock_archive_service_in_task = mocker.patch(
            "tasks.flare_cleanup.ArchiveService"
        )
        mock_archive_service_in_task.return_value.delete_file.return_value = None
    
        local_value_for_flare = {"test": "test"}
        open_pull_with_local_flare = PullFactory(
            state=PullStates.OPEN.value,
            _flare=local_value_for_flare,
            repository=RepositoryFactory(),
        )
>       assert open_pull_with_local_flare.flare == local_value_for_flare
E       AssertionError: assert {} == {'test': 'test'}
E         
E         Right contains 1 more item:
E         {'test': 'test'}
E         Use -v to get more diff

.../tests/unit/test_flare_cleanup.py:42: AssertionError

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

nora-codecov requested a review from Swatinem December 6, 2024 07:36

nora-codecov requested a review from a team December 6, 2024 07:36

nora-codecov mentioned this pull request Dec 6, 2024

Remove pulls trigger codecov/shared#450

Open

Swatinem reviewed Dec 6, 2024

View reviewed changes

nora-codecov force-pushed the nora/1029 branch from 8b57acf to 5ac629e Compare December 28, 2024 01:58

nora-codecov added 2 commits December 27, 2024 18:24

add FlareCleanupTask

5f2f571

make flare cleanup task safer, conver to manual task for testing

044d770

nora-codecov force-pushed the nora/1029 branch from 5ac629e to 044d770 Compare December 28, 2024 02:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add FlareCleanupTask #947

add FlareCleanupTask #947

nora-codecov commented Dec 6, 2024

github-actions bot commented Dec 6, 2024

codecov bot commented Dec 6, 2024 •

edited

Loading

codecov-notifications bot commented Dec 6, 2024 •

edited by codecov-staging bot

Loading

github-actions bot commented Dec 6, 2024

Swatinem left a comment

Swatinem Dec 6, 2024

Swatinem Dec 6, 2024

Swatinem Dec 6, 2024

Swatinem Dec 6, 2024

github-actions bot commented Dec 28, 2024 •

edited

Loading

codecov-qa bot commented Dec 28, 2024

codecov-public-qa bot commented Dec 28, 2024

		# 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:

		archive_service = ArchiveService(repository=pull.repository)
		archive_service.delete_file(pull._flare_storage_path)

add FlareCleanupTask #947

Are you sure you want to change the base?

add FlareCleanupTask #947

Conversation

nora-codecov commented Dec 6, 2024

github-actions bot commented Dec 6, 2024

codecov bot commented Dec 6, 2024 • edited Loading

❌ 1 Tests Failed:

codecov-notifications bot commented Dec 6, 2024 • edited by codecov-staging bot Loading

❌ 1 Tests Failed:

github-actions bot commented Dec 6, 2024

Swatinem left a comment

Choose a reason for hiding this comment

Swatinem Dec 6, 2024

Choose a reason for hiding this comment

Swatinem Dec 6, 2024

Choose a reason for hiding this comment

Swatinem Dec 6, 2024

Choose a reason for hiding this comment

Swatinem Dec 6, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 28, 2024 • edited Loading

codecov-qa bot commented Dec 28, 2024

❌ 1 Tests Failed:

codecov-public-qa bot commented Dec 28, 2024

❌ 1 Tests Failed:

codecov bot commented Dec 6, 2024 •

edited

Loading

codecov-notifications bot commented Dec 6, 2024 •

edited by codecov-staging bot

Loading

github-actions bot commented Dec 28, 2024 •

edited

Loading