[ENG-2247] Creates a Node quota_status v1 endpoint for WB to enforce storage limits#9494
Conversation
Johnetordoff
left a comment
Johnetordoff
left a comment
There was a problem hiding this comment.
Nothing that's really a blocker, but a few questions here and there, still some WB work to be done, but fundamentally a good implementation, might want to include a few more docs about what WB is doing. Kind of curious why this is getting it's own callback and not just piggybacking on get_auth.
| 'over_quota': False | ||
| } | ||
| # Storage calculation for the target has been accepted and will run asynchronously | ||
| if target.storage_usage is None: |
There was a problem hiding this comment.
What does WB do if it gets a 202? Do we assume it's under quota?
There was a problem hiding this comment.
I think it should be retried. @felliott is working on the logic responding to this endpoint. What do you think, Fitz?
There was a problem hiding this comment.
That makes sense, we can't always guarantee the calculation will be done before WB retries, but it usually only takes a few ms to do, so it's a safe bet.
There was a problem hiding this comment.
Hmmm, I hadn't considered this, but it should be doable. If I don't get an answer, though, we might need to let it go through.
There was a problem hiding this comment.
That should be fine. We're trying to stop continued abuse, and the occasional file to get through shouldn't be a problem. If we have to, we'll eventually modify our caching invalidation strategy to make this less of a problem.
| return self.get_node() | ||
|
|
||
| def retrieve(self, request, *args, **kwargs): | ||
| instance = self.get_object() |
There was a problem hiding this comment.
can we just call get_node here and cut out the get_object definition?
There was a problem hiding this comment.
It ends up breaking the NodeStorage embed which relies on the get_object definition. This change would be viable if we didn't need to embed it though!
|
Drive-by: Did you mean to include your other API PR in with this one? |
|
I rebased this PR upon the other API PR because of some of the |
|
@Johnetordoff Some of your feedback is actually for #9483 (which this PR is based upon). I'll incorporate those changes there. We'll talk about why a distinct endpoint is necessary for WB tomorrow and I can write up a Notion doc detailing why - the gist is that get_auth is sent on an operation basis rather than a user intent basis (i.e. A user-facing move is two file operations, an upload and delete). For certain moves/copies within the same OSFStorage instance, they should be allowed regardless of storage caps because they are internal and not increasing the storage used. Permitting these operations through |
|
Excellent work! just address Fitz's comments and we good. |
brianjgeiger
left a comment
brianjgeiger
left a comment
There was a problem hiding this comment.
Handle quickfiles, plus Fitz's comments. Otherwise looks great.
…hecks should be in place to prevent issues.
UdayVarkhedkar
force-pushed
the
feature/wb-node-storage-endpoint
branch
from
93ce1da to
187eecd
Compare
…which now rely on storage usage calculations
…which now rely on storage usage calculations
…which now rely on storage usage calculations
…se 0 should evaluate to True
5 participants
Purpose
Provides Waterbutler with an endpoint to check if a node has exceeded storage limits prior to conducting file operations
Changes
api/v1/<guid>/osfstorage/quota_status/) for WB to check theover_quotastatus of a nodeQA Notes
Dev Tested (Endpoint can only be accessed by WB)
Documentation
N/A
Side Effects
N/A
Ticket
JIRA Ticket