5228-query-instead-of-signals #5469

devincowan · 2024-04-29T14:25:21Z

Partially resolves #5228 by directly querying resource file size at runtime instead of caching used storage size in the UserQuota model

This PR alters are quota system to remove reliance on the iRods microservices.
More info is available in
https://github.com/hydroshare/hydroshare/blob/5228-query-instead-of-signals/quota-info.md

Resolves #5228 in conjunction with #5229
Relies on #5443 in order to get accurate file sizes from irods storage
Resolves #5418 by entirely removing the need for irods microservice in the dataZone
Resolves #5235 because removing the irods microservice will vastly improve performance of filesystem operations
Resolves #5441 because quota will be refreshed before sending emails and we can be confident in correct values
Resolves #5384 because we don't need a microservice anymore
Resolves #5195 because we don't need a microservice anymore
Resolves #5286 because we don't need a microservice anymore

Pull Request Checklist:

Positive Test Case Written by Dev

Automated Testing

Sufficient User and Developer Documentation

Passing Jenkins Build

Peer Code review and approval

Positive Test Case

Suggest that you open a private window and login as admin, then open the userQuotas model in the django admin panel
Login as non-admin user in the non-private window
With that user, conduct the following operations

uploading a file to a resource
creating a resource
copying a resource
versioning a resource
transferring ownership of a resource to "user"
ingesting a bag from the userZone
zipping a file without removing the original
unzipping a file
creating aggregations
moving files into subdirectories

In all cases, verify that the user's quota (viewed in the private browser, admin panel) adjusts in the expected way, either increasing, decreasing, or remaining the same.

pkdash

Looks good. Left few comments. The main one is the new model field quota_holder that needs change.

pkdash · 2024-04-30T18:23:45Z

hs_core/models.py

@@ -2178,6 +2178,9 @@ class MyResourceContentType(pages.Page, hs_core.AbstractResource):
    # allow resource that contains spam_patterns to be discoverable/public
    spam_allowlisted = models.BooleanField(default=False)

+    quota_holder = models.ForeignKey(User, on_delete=models.CASCADE, null=True, blank=True,


Do we want to delete all resources when a user is deleted (on_delete=models.CASCADE)? We probably want to use on_delete=models.SET_NULL

thank you for this Pabitra! I'm thinking maybe we use SET()
https://docs.djangoproject.com/en/3.2/ref/models/fields/#django.db.models.SET
added that, let me know what you think

Nevermind, I realized that SET() doesn't seem to take any args so I will use SET_NULL as you suggested

What if you make this function as an instance method of the AbstractResource class?.
81c5e6b#diff-3bc7182059367a2c6cf9e96428d08a0ce66f709baec5d00090abd19683582891R2112

Thanks @pkdash! I tried like this:

class AbstractResource(ResourcePermissionsMixin, ResourceIRODSMixin): quota_holder = models.ForeignKey(User, on_delete=models.SET(self.get_new_quota_holder), null=True, blank=True, related_name='quota_holder') def get_new_quota_holder(self): """Get a new quota holder for a resource.""" if self.raccess.owners.count() > 0: return self.raccess.owners.first() else: return None

But results:

hydroshare | File "/hydroshare/hs_core/models.py", line 2181, in AbstractResource
hydroshare | quota_holder = models.ForeignKey(User, on_delete=models.SET(self.get_new_quota_holder), null=True, blank=True,
hydroshare | NameError: name 'self' is not defined

It looks like SET() needs a callable, and there is a good mention of the fact that the callable can't run any queries when models.py gets imported:
https://docs.djangoproject.com/en/3.2/ref/models/fields/#django.db.models.SET

Set the ForeignKey to the value passed to SET(), or if a callable is passed in, the result of calling it. In most cases, passing a callable will be necessary to avoid executing queries at the time your models.py is imported:

...maybe there is another way?

Yeah I have never used models.SET() before. Seems like it has quite a few restrictions. I guess we can set it to None and for now and deal with it later to update it using management command. Do we ever delete users? What about a user being inactive? If we really want to set another user as quota holder upon user delete, maybe we can use pre delete signal for user?

ooh pre-delete signal is a good call. AFAIK we don't ever delete users, we make them inactive. I think it's okay for an inactive user to retain their quota.

I'll add a pre-delete signal

added signal in 0747594

I think it might not even matter because in my testing, deleting user also deletes all of their resources.
I don't see this cascade in the code...maybe it is
https://github.com/hydroshare/hydroshare/blob/master/hs_core/models.py#L2119-L2124

pkdash · 2024-04-30T18:48:51Z

hs_core/hydroshare/resource.py

+    return uqDataZoneSize if uqDataZoneSize is not None else 0
+
+
+def get_user_zone_usage(username):


This function is not returning the quota size.

I wish we didn't had to depend on the iRODS AVU to get this value.

good catch thank you!
In this PR we get rid of iRods AVUs for the Datazone.
Similar to you, I wish we would get rid of the AVUs for the UserZone as well.
It would certainly be possible, but since the userzone is (:fingers-crossed) going away after our cloud migration, I decided not to invest that effort at this time.

hs_core/hydroshare/resource.py

theme/models.py

pkdash · 2024-04-30T19:24:40Z

theme/models.py

+        from hs_core.hydroshare.resource import get_quota_usage
+
+        if refresh:
+            # TODO get_quota_usage will run the dz query, but then it gets run again below...


Planning to fix this TODO?

I believe that is resolved. Removed the TODO

pkdash · 2024-04-30T19:32:08Z

theme/management/commands/dump_user_quota.py

@@ -98,7 +102,7 @@ def handle(self, *args, **options):
                owned_resources = user.uaccess.owned_resources
                total_size = 0
                for res in owned_resources:
-                    if res.get_quota_holder() == user:
+                    if res.quota_holder == user:


Since quota_holder is a model field now, we can filter on it - no need for the if check

good call thanks! added that filter

pkdash · 2024-04-30T19:48:48Z

hs_core/tasks.py

@@ -509,6 +509,12 @@ def send_over_quota_emails():
        if uq:
            used_percent = uq.used_percent
            if used_percent >= qmsg.soft_limit_percent:
+                # first update the quota just to be sure that we are using the latest figures
+                update_quota_usage(u.username)


This function update_quota_usage() also sends email notifications. Would we end up sending duplicate emails?

I added a param notify_user so that we can suppress messages to the users. Thanks!

theme/management/commands/migrate_quota_holder_from_irods.py

hs_core/tests/test_update_quota_usage.py

…foreign key

…-signals

This commit adds a new management command `reset_quota.py` that allows for resetting the userzone quota by forcing the quota iRODS microservices to recalculate the quota for all users. This command is useful in scenarios where the quota needs to be recalculated due to changes in the system.

…-signals

… in expected quota calculation

pkdash · 2024-05-13T15:21:13Z

@devincowan Will try to finish reviewing by tomorrow.

hs_core/migrations/0082_merge_20240509_1646.py

pkdash · 2024-05-13T17:40:57Z

hs_core/receivers.py

+    user = instance
+    for res in BaseResource.objects.filter(quota_holder=user):
+        other_owners = None
+        if hasattr(res, 'raccess') and res.raccess is not None:


Is this check necessary?

Since we are not deleting user, I am not considering making suggestions to optimize queries.

I guess maybe that check isn't necessary. I'm not remembering now why I added that... removed

Added it back in. See test failures here:
http://ci.hydroshare.org:8080/job/hydroshare-pull-requests/7611/

It is fine to have that check. However, the reason I think the tests failed when you removed that check is because of the delete order here - the ResourceAccess records are deleted before User records:
https://github.com/hydroshare/hydroshare/blob/develop/hs_access_control/tests/utilities.py#L35
https://github.com/hydroshare/hydroshare/blob/develop/hs_access_control/tests/utilities.py#L36

If you swap the delete order the tests are likely to pass. Again, I am not suggesting you need to make any changes for this PR.

right you are! Thanks Pabitra!

The migrations `0080_baseresource_quota_holder.py`, `0080_resourcefile_filesize_cache_updated.py`, `0081_alter_baseresource_quota_holder.py`, and `0082_merge_20240509_1646.py` are no longer needed and have been deleted. These migrations were related to the `baseresource` model's `quota_holder` field and the `resourcefile` model's `filesize_cache_updated` field. The changes made in these migrations have been consolidated into a new migration `0080_baseresource_quota_holder_squashed_0082_merge_20240509_1646.py`, which adds both fields to their respective models.

…ners" This reverts commit 6091a85.

devincowan added 3 commits April 29, 2024 09:20

[#5228] store quota_holder in django db

afd32c1

[#5228] add tests for updating quota

b5ae0b5

[#5228] query ResrourceFile objects to get datazone usage

4b285cb

devincowan requested a review from pkdash April 29, 2024 14:27

devincowan mentioned this pull request Apr 29, 2024

[#5228] Quota use django signals instead of irods microservice and AVUs #5461

Closed

5 tasks

devincowan added 5 commits April 29, 2024 10:58

[#5228] fix units mismatch

697e0b3

[#5228] account for published resources

0920ae7

[#5228] fix debug_resource view

c123ea1

documentation for datazone quota calculations

808a3d5

add some additional assertions in quota tests

d9740a1

devincowan force-pushed the 5228-query-instead-of-signals branch from a5abc34 to d9740a1 Compare April 29, 2024 16:38

pkdash requested changes Apr 30, 2024

View reviewed changes

devincowan added 6 commits May 1, 2024 10:16

respond to review suggestions

81c5e6b

Modify update_quota_usage function to include notify_user parameter

970cf9d

Refactor models.py to use on_delete=models.SET_NULL for quota_holder …

a686730

…foreign key

[#5228] set user_zone_value in tests instead of data zone value

6d27745

[#5228] Update receivers.py to include pre_delete signal for User

0747594

check for raccess exist in pre_delete signal

e22ae70

devincowan force-pushed the 5228-query-instead-of-signals branch 3 times, most recently from 5f31c99 to e22ae70 Compare May 6, 2024 13:57

devincowan added 7 commits May 6, 2024 12:53

[#5228] userZone tests ensure cleanup if other tests are irresponsible

3e0c8df

fix typo in quota email

f38a065

Merge branch 'develop' into 5228-query-instead-of-signals

9a65f63

Create merge migration

837a734

Merge branch '5228-hard-quota-enforcement' into 5228-query-instead-of…

bc0686b

…-signals

Merge branch '5228-hard-quota-enforcement' into 5228-query-instead-of…

43f05a6

…-signals

Merge branch '5228-hard-quota-enforcement' into 5228-query-instead-of…

7c89614

…-signals

devincowan force-pushed the 5228-hard-quota-enforcement branch from 1bbae82 to 8f54dc1 Compare May 9, 2024 20:38

devincowan force-pushed the 5228-query-instead-of-signals branch from 48871d4 to 7c89614 Compare May 9, 2024 20:39

Merge branch '5228-hard-quota-enforcement' into 5228-query-instead-of…

8ddcf61

…-signals

devincowan linked an issue May 9, 2024 that may be closed by this pull request

Enforce file quota #5228

Closed

38 tasks

devincowan removed a link to an issue May 9, 2024

Enforce file quota #5228

Closed

38 tasks

devincowan added 2 commits May 9, 2024 16:50

Merge branch '5228-hard-quota-enforcement' into 5228-query-instead-of…

d03378e

…-signals

devincowan force-pushed the 5228-hard-quota-enforcement branch from 87d37f9 to f4b85b8 Compare May 9, 2024 22:51

Merge branch '5228-hard-quota-enforcement' into 5228-query-instead-of…

a21d785

…-signals

devincowan force-pushed the 5228-query-instead-of-signals branch from 636297c to a21d785 Compare May 9, 2024 22:51

[#5228] Update test_update_quota_usage.py to include size of zip file…

1bb7bf8

… in expected quota calculation

pkdash approved these changes May 13, 2024

View reviewed changes

devincowan added 3 commits May 13, 2024 16:37

pre_delete_user_handler remove unnecessary check for other_owners

6091a85

Revert "pre_delete_user_handler remove unnecessary check for other_ow…

bbf624a

…ners" This reverts commit 6091a85.

devincowan merged commit 22e9bc5 into 5228-hard-quota-enforcement May 14, 2024
1 check passed

devincowan deleted the 5228-query-instead-of-signals branch May 14, 2024 12:46

devincowan mentioned this pull request May 14, 2024

[#5228] hard quota enforcement #5229

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

5228-query-instead-of-signals #5469

5228-query-instead-of-signals #5469

devincowan commented Apr 29, 2024 •

edited

pkdash left a comment

pkdash Apr 30, 2024

devincowan May 1, 2024

devincowan May 1, 2024

pkdash May 2, 2024

devincowan May 3, 2024 •

edited

pkdash May 3, 2024

devincowan May 3, 2024

devincowan May 3, 2024

devincowan May 3, 2024

pkdash Apr 30, 2024

pkdash Apr 30, 2024

devincowan May 1, 2024

pkdash Apr 30, 2024

devincowan May 1, 2024

pkdash Apr 30, 2024

devincowan May 1, 2024 •

edited

pkdash Apr 30, 2024

devincowan May 1, 2024

pkdash commented May 13, 2024

pkdash May 13, 2024

pkdash May 13, 2024 •

edited

devincowan May 13, 2024 •

edited

devincowan May 13, 2024

pkdash May 14, 2024

devincowan May 14, 2024

		return uqDataZoneSize if uqDataZoneSize is not None else 0


		def get_user_zone_usage(username):

5228-query-instead-of-signals #5469

5228-query-instead-of-signals #5469

Conversation

devincowan commented Apr 29, 2024 • edited

Pull Request Checklist:

Positive Test Case

pkdash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devincowan May 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devincowan May 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkdash commented May 13, 2024

Choose a reason for hiding this comment

pkdash May 13, 2024 • edited

Choose a reason for hiding this comment

devincowan May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devincowan commented Apr 29, 2024 •

edited

devincowan May 3, 2024 •

edited

devincowan May 1, 2024 •

edited

pkdash May 13, 2024 •

edited

devincowan May 13, 2024 •

edited