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

Enhance quota exceeded logging for admins #37581

Conversation

joshtrichards
Copy link
Member

@joshtrichards joshtrichards commented Apr 5, 2023

Summary

Enhances quota exceeded server-side logging for administrators by adding:

  • target folder owner name (i.e. when shared)
  • indicator of user quota (vs disk space) issue

Context: The existing Insufficient space error logging doesn't indicate the folder owner (which is important since the sharee path is what's logged). In addition, quota issues are addressed differently than, say, disk space issues so administrators benefit from quickly noting the type of Insufficient space issue occurring.

Applies to file/folder operations: uploads, moves, copies

Also:

  • improves - indirectly - the user-facing messaging when uploading (only*)
  • Doesn't include the owner when it's useless (i.e. not shared)

Addresses backend portion of #37519

*Currently working on independent (but related) PRs to unify frontend messaging with same target folder owner inclusion (when applicable)

TODO

  • Review code

Checklist

@joshtrichards
Copy link
Member Author

For the record...

Existing log entry for all quota issues:

Insufficient space in /Project/Videos/lightonflux, 27136230 required, 343725 available

New (where owner is applicable):

Quota exceeded in /Project/Videos/lightonflux (owner: sarah), 27136230 required, 343725 available

New (where owner is inapplicable but there's still a quota issue):

Quota exceeded in /QuotaTest, 341471232 required, 301045000 available

@szaimen szaimen added the 3. to review Waiting for reviews label Apr 5, 2023
@szaimen szaimen added this to the Nextcloud 27 milestone Apr 5, 2023
@szaimen szaimen requested review from nickvergessen, a team, ArtificialOwl, icewind1991 and blizzz and removed request for a team April 5, 2023 11:59
@nickvergessen nickvergessen removed their request for review April 5, 2023 17:25
This was referenced May 3, 2023
@joshtrichards joshtrichards requested a review from blizzz May 9, 2023 16:02
@szaimen
Copy link
Contributor

szaimen commented May 16, 2023

/rebase

@szaimen
Copy link
Contributor

szaimen commented May 16, 2023

tests failing...

@szaimen szaimen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels May 16, 2023
auto-merge was automatically disabled May 16, 2023 16:20

Head branch was pushed to by a user without write access

@szaimen szaimen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 16, 2023
@joshtrichards
Copy link
Member Author

joshtrichards commented May 16, 2023

Fixed (at least for me... we'll have to see what the tests say).

In the quota plugin, depending on which before* operation is being called, sometimes checkQuota() gets handed a $path that includes the basename of the URI and other times not so much.

That's current behavior so I'm leaving it be, but I added a check for non-chunked uploads in checkQuota() now just before the getOwner() call in this PR (and then I adjust the $path to exclude the basename). That way we always gets a workable $path for non-chunked uploads (and, more importantly, the same as we'd have gotten if it had been a chunked upload)

The main reason non-chunked uploads triggered this is because the PUT operation never finishes (since the quota is exceeded) so there's really no file to check ownership of - all we have is the intended folder.

All my manual testing was apparently based on doing >10 MB uploads. And the way that chunking-v2 uploads work, the MOVE was giving us what we expected for $path. But <10 MB uploads just do a straight PUT and we receive the full path with the filename intact for the quota check. The integration tests failing all seem to be <10 MB (non-chunked).

@joshtrichards
Copy link
Member Author

Incidentally this also fixes a "cosmetic" bug as a side effect versus current master behavior where hitting quota with uploads in the root folder were being mishandled in the logging/notifications:

Before:
NC Files - Quota Full Handling - Screenshot 2023-05-16 130845
After:
NC Files - Quota Full Handling After - Screenshot 2023-05-16 131039

I'd noted this during various bits of testing but hadn't gotten around to checking for prior reports of or root causes for - so that's a nice benefit to this change even if it wasn't the target of (or caused by) this PR. Woohoo!

@szaimen szaimen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 18, 2023
@szaimen szaimen disabled auto-merge July 18, 2023 21:41
@szaimen szaimen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jul 18, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
This was referenced Mar 12, 2024
This was referenced Mar 20, 2024
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@skjnldsv
Copy link
Member

What's the status here @joshtrichards :)

@skjnldsv skjnldsv added the stale Ticket or PR with no recent activity label May 30, 2024
This was referenced Jul 30, 2024
This was referenced Aug 5, 2024
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@skjnldsv skjnldsv closed this Aug 14, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress stale Ticket or PR with no recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Insufficient Storage Error does not name user (nextcloud.log, android, desktop)
5 participants