From 1c1eee6a64cfda496d62dee0234440b2adf57513 Mon Sep 17 00:00:00 2001 From: Jacob Pierce Date: Fri, 15 Sep 2023 15:17:11 -0700 Subject: [PATCH 1/6] move incomplete downloads calculation logic into StorageCalculator class --- kolibri/core/content/utils/content_request.py | 75 +++++++++++++++---- 1 file changed, 59 insertions(+), 16 deletions(-) diff --git a/kolibri/core/content/utils/content_request.py b/kolibri/core/content/utils/content_request.py index cec567500f4..2d1206c5b1d 100644 --- a/kolibri/core/content/utils/content_request.py +++ b/kolibri/core/content/utils/content_request.py @@ -486,6 +486,7 @@ def process_content_requests(): LearnerDeviceStatus.clear_statuses() except InsufficientStorage as e: logger.warning(str(e)) + LearnerDeviceStatus.save_statuses(DeviceStatus.InsufficientStorage) except NoPeerAvailable as e: logger.warning(str(e)) @@ -649,19 +650,14 @@ def _process_content_requests(incomplete_downloads): """ Processes content requests, both for downloading and removing content """ - incomplete_downloads_with_metadata = incomplete_downloads.filter(has_metadata=True) - # obtain the incomplete removals, that do not have an associated download - incomplete_removals = incomplete_removals_queryset() - incomplete_sync_removals = incomplete_removals.filter( - reason=ContentRequestReason.SyncInitiated - ) - incomplete_user_removals = incomplete_removals.filter( - reason=ContentRequestReason.UserInitiated - ) - complete_user_downloads = ContentDownloadRequest.objects.filter( - status=ContentRequestStatus.Completed, reason=ContentRequestReason.UserInitiated + calc = StorageCalculator(incomplete_downloads) + + incomplete_downloads_with_metadata = calc.incomplete_downloads.filter( + has_metadata=True ) + + # obtain the incomplete removals, that do not have an associated download # track failed so we can exclude them from the loop failed_ids = [] has_processed_sync_removals = False @@ -688,19 +684,28 @@ def _process_content_requests(incomplete_downloads): free_space ) ) - if not has_processed_sync_removals and incomplete_sync_removals.exists(): + if ( + not has_processed_sync_removals + and calc.incomplete_sync_removals.exists() + ): # process, then repeat has_processed_sync_removals = True logger.info("Processing sync-initiated content removal requests") - process_content_removal_requests(incomplete_sync_removals) + process_content_removal_requests(calc.incomplete_sync_removals) continue - if not has_processed_user_removals and incomplete_user_removals.exists(): + if ( + not has_processed_user_removals + and calc.incomplete_user_removals.exists() + ): # process, then repeat has_processed_user_removals = True logger.info("Processing user-initiated content removal requests") - process_content_removal_requests(incomplete_user_removals) + process_content_removal_requests(calc.incomplete_user_removals) continue - if not has_processed_user_downloads and complete_user_downloads.exists(): + if ( + not has_processed_user_downloads + and calc.complete_user_downloads.exists() + ): # process, then repeat has_processed_user_downloads = True process_user_downloads_for_removal() @@ -939,3 +944,41 @@ def process_content_removal_requests(queryset): remaining_pending = queryset.filter(status=ContentRequestStatus.Pending) _remove_corresponding_download_requests(remaining_pending) remaining_pending.update(status=ContentRequestStatus.Completed) + + +class StorageCalculator: + def __init__(self, incomplete_downloads_queryset): + incomplete_removals = incomplete_removals_queryset() + + self.incomplete_downloads = incomplete_downloads_queryset + self.incomplete_sync_removals = incomplete_removals.filter( + reason=ContentRequestReason.SyncInitiated + ).annotate( + total_size=_total_size_annotation(available=True), + ) + + self.incomplete_user_removals = incomplete_removals.filter( + reason=ContentRequestReason.UserInitiated + ).annotate( + total_size=_total_size_annotation(available=True), + ) + + self.complete_user_downloads = ContentDownloadRequest.objects.filter( + status=ContentRequestStatus.Completed, + reason=ContentRequestReason.UserInitiated, + ).annotate( + total_size=_total_size_annotation(available=True), + ) + + self._calculate_space_available() + + def _calculate_space_available(self): + free_space = get_free_space_for_downloads( + completed_size=_total_size(completed_downloads_queryset()) + ) + free_space -= _total_size(self.incomplete_downloads) + free_space += _total_size(self.incomplete_sync_removals) + free_space += _total_size(self.incomplete_user_removals) + free_space += _total_size(self.complete_user_downloads) + + self.free_space = free_space From 92eb2841cdc3d54ce3d95aad1be4511c3b690eb2 Mon Sep 17 00:00:00 2001 From: Jacob Pierce Date: Fri, 15 Sep 2023 15:17:49 -0700 Subject: [PATCH 2/6] ContentSyncHook.post_transfer sets InsufficientStorage status --- kolibri/core/content/kolibri_plugin.py | 10 +++- .../content/test/test_content_sync_hook.py | 48 +++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) create mode 100644 kolibri/core/content/test/test_content_sync_hook.py diff --git a/kolibri/core/content/kolibri_plugin.py b/kolibri/core/content/kolibri_plugin.py index aee3a8792df..2390f6e004e 100644 --- a/kolibri/core/content/kolibri_plugin.py +++ b/kolibri/core/content/kolibri_plugin.py @@ -8,7 +8,10 @@ from kolibri.core.content.tasks import enqueue_automatic_resource_import_if_needed from kolibri.core.content.utils.content_request import incomplete_downloads_queryset from kolibri.core.content.utils.content_request import process_metadata_import +from kolibri.core.content.utils.content_request import StorageCalculator from kolibri.core.content.utils.content_request import synchronize_content_requests +from kolibri.core.device.models import DeviceStatus +from kolibri.core.device.models import LearnerDeviceStatus from kolibri.core.device.utils import get_device_setting from kolibri.core.discovery.hooks import NetworkLocationDiscoveryHook from kolibri.plugins.hooks import register_hook @@ -80,8 +83,11 @@ def post_transfer( if incomplete_downloads_without_metadata.exists(): process_metadata_import(incomplete_downloads_without_metadata) - # TODO: we need determine total space for new downloads and if there isn't sufficient space - # save the `LearnerDeviceStatus` with the insufficient storage status + calc = StorageCalculator(incomplete_downloads) + if calc.free_space < sum([dl.total_size for dl in incomplete_downloads]): + LearnerDeviceStatus.save_learner_status( + single_user_id, DeviceStatus.InsufficientStorage + ) enqueue_automatic_resource_import_if_needed() diff --git a/kolibri/core/content/test/test_content_sync_hook.py b/kolibri/core/content/test/test_content_sync_hook.py new file mode 100644 index 00000000000..a65b2014812 --- /dev/null +++ b/kolibri/core/content/test/test_content_sync_hook.py @@ -0,0 +1,48 @@ +import mock +from morango.sync.context import SessionContext + +from kolibri.core.auth.sync_operations import KolibriSyncOperations +from kolibri.core.content.kolibri_plugin import ContentSyncHook +from kolibri.core.content.test.utils.test_content_request import ( + IncompleteDownloadsQuerysetTestCase, +) +from kolibri.core.device.models import DeviceStatus + + +class KolibriContentSyncHookTestCase(IncompleteDownloadsQuerysetTestCase): + def setUp(self): + super(KolibriContentSyncHookTestCase, self).setUp() + self.operation = KolibriSyncOperations() + self.context = mock.Mock(spec_set=SessionContext)() + + @mock.patch("kolibri.core.device.models.LearnerDeviceStatus.save_learner_status") + @mock.patch("kolibri.core.content.utils.content_request.StorageCalculator") + @mock.patch("kolibri.core.content.kolibri_plugin.ContentSyncHook") + def test_post_transfer_sets_insufficient_storage( + self, + mock_hook, + mock_calc, + mock_save_learner_status, + ): + with mock.patch( + "kolibri.core.content.utils.settings.automatic_download_enabled", + return_value=True, + ): + with mock.patch( + "kolibri.core.content.utils.content_request.get_free_space_for_downloads", + return_value=0, + ): + self._create_resources(self.admin_request.contentnode_id) + mock_hook.post_transfer = lambda *args: ContentSyncHook.post_transfer( + mock_hook, *args + ) + mock_hook.post_transfer( + self.facility.dataset_id, + True, + True, + self.learner.id, + self.context, + ) + mock_save_learner_status.assert_called_with( + self.learner.id, DeviceStatus.InsufficientStorage + ) From c69a9015e9da2a2af16ad443ef9dfd0622faff56 Mon Sep 17 00:00:00 2001 From: Jacob Pierce Date: Mon, 18 Sep 2023 10:01:57 -0700 Subject: [PATCH 3/6] move space comparison to StorageCalculator; use _total_size helper --- kolibri/core/content/kolibri_plugin.py | 2 +- kolibri/core/content/utils/content_request.py | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/kolibri/core/content/kolibri_plugin.py b/kolibri/core/content/kolibri_plugin.py index 2390f6e004e..6960ca13730 100644 --- a/kolibri/core/content/kolibri_plugin.py +++ b/kolibri/core/content/kolibri_plugin.py @@ -84,7 +84,7 @@ def post_transfer( process_metadata_import(incomplete_downloads_without_metadata) calc = StorageCalculator(incomplete_downloads) - if calc.free_space < sum([dl.total_size for dl in incomplete_downloads]): + if not calc.is_space_sufficient(): LearnerDeviceStatus.save_learner_status( single_user_id, DeviceStatus.InsufficientStorage ) diff --git a/kolibri/core/content/utils/content_request.py b/kolibri/core/content/utils/content_request.py index 2d1206c5b1d..11550681305 100644 --- a/kolibri/core/content/utils/content_request.py +++ b/kolibri/core/content/utils/content_request.py @@ -982,3 +982,6 @@ def _calculate_space_available(self): free_space += _total_size(self.complete_user_downloads) self.free_space = free_space + + def is_space_sufficient(self): + return self.free_space > _total_size(self.incomplete_downloads) From 42bd3bfc54746ddeeaf75e6778b563b98dcd42cb Mon Sep 17 00:00:00 2001 From: Jacob Pierce Date: Mon, 18 Sep 2023 10:15:01 -0700 Subject: [PATCH 4/6] don't calculate space available until needed --- kolibri/core/content/utils/content_request.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kolibri/core/content/utils/content_request.py b/kolibri/core/content/utils/content_request.py index 11550681305..857ec5cfa4d 100644 --- a/kolibri/core/content/utils/content_request.py +++ b/kolibri/core/content/utils/content_request.py @@ -969,8 +969,7 @@ def __init__(self, incomplete_downloads_queryset): ).annotate( total_size=_total_size_annotation(available=True), ) - - self._calculate_space_available() + self.free_space = 0 def _calculate_space_available(self): free_space = get_free_space_for_downloads( @@ -984,4 +983,5 @@ def _calculate_space_available(self): self.free_space = free_space def is_space_sufficient(self): + self._calculate_space_available() return self.free_space > _total_size(self.incomplete_downloads) From 61aac196f45d950171e726f4fa345ed75347fab8 Mon Sep 17 00:00:00 2001 From: Jacob Pierce Date: Mon, 18 Sep 2023 10:58:57 -0700 Subject: [PATCH 5/6] only set insufficient storage device status when local_is_single_user --- kolibri/core/content/kolibri_plugin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kolibri/core/content/kolibri_plugin.py b/kolibri/core/content/kolibri_plugin.py index 6960ca13730..8f5442c4ad7 100644 --- a/kolibri/core/content/kolibri_plugin.py +++ b/kolibri/core/content/kolibri_plugin.py @@ -84,7 +84,7 @@ def post_transfer( process_metadata_import(incomplete_downloads_without_metadata) calc = StorageCalculator(incomplete_downloads) - if not calc.is_space_sufficient(): + if local_is_single_user and not calc.is_space_sufficient(): LearnerDeviceStatus.save_learner_status( single_user_id, DeviceStatus.InsufficientStorage ) From 9513ebf46c026e0c8897a840c94b92b91e4cdfb6 Mon Sep 17 00:00:00 2001 From: Jacob Pierce Date: Mon, 18 Sep 2023 12:03:52 -0700 Subject: [PATCH 6/6] no need to mock the hook --- kolibri/core/content/test/test_content_sync_hook.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/kolibri/core/content/test/test_content_sync_hook.py b/kolibri/core/content/test/test_content_sync_hook.py index a65b2014812..480b7e90c2d 100644 --- a/kolibri/core/content/test/test_content_sync_hook.py +++ b/kolibri/core/content/test/test_content_sync_hook.py @@ -17,10 +17,8 @@ def setUp(self): @mock.patch("kolibri.core.device.models.LearnerDeviceStatus.save_learner_status") @mock.patch("kolibri.core.content.utils.content_request.StorageCalculator") - @mock.patch("kolibri.core.content.kolibri_plugin.ContentSyncHook") def test_post_transfer_sets_insufficient_storage( self, - mock_hook, mock_calc, mock_save_learner_status, ): @@ -33,10 +31,8 @@ def test_post_transfer_sets_insufficient_storage( return_value=0, ): self._create_resources(self.admin_request.contentnode_id) - mock_hook.post_transfer = lambda *args: ContentSyncHook.post_transfer( - mock_hook, *args - ) - mock_hook.post_transfer( + hook = ContentSyncHook() + hook.post_transfer( self.facility.dataset_id, True, True,