Fix/4285 limit disk on parallel upload #4329
Conversation
…ix/4285-limit-disk-on-parallel-upload
…odalab/codalab-worksheets into fix/4285-limit-disk-on-parallel-upload
… messing up testS
AndrewJGaut
force-pushed
the
fix/4285-limit-disk-on-parallel-upload
branch
from
da91bb6
to
06b0f91
Compare
| raise Exception( | ||
| 'Upload aborted. User disk quota exceeded. ' | ||
| 'To apply for more quota, please visit the following link: ' | ||
| 'https://codalab-worksheets.readthedocs.io/en/latest/FAQ/' |
There was a problem hiding this comment.
can we refactor this link out into a constant?
| @@ -114,7 +114,7 @@ def _make_request( | |||
| raise RestClientException('Invalid JSON: ' + response_data, False) | |||
|
|
|||
| def _upload_with_chunked_encoding( | |||
| self, method, url, query_params, fileobj, progress_callback=None | |||
| self, method, url, query_params, fileobj, pass_self=False, progress_callback=None | |||
There was a problem hiding this comment.
What does pass_self mean? Can you document here and in the other function where you add it
codalab/worker/upload_util.py
Outdated
| @@ -32,6 +33,11 @@ def upload_with_chunked_encoding( | |||
| :param url: String. Location or sub url that indicating where the file object will be uploaded. | |||
| :param need_response: Bool. Whether need to wait for the response. | |||
| :param progress_callback: Function. Callback function indicating upload progress. | |||
| :param json_api_client: JsonApiClient. None when this function is run by the server. | |||
| Used to update disk usage from client. | |||
| :param check_disk: bool. True if the user's disk should be checked during the upload. | |||
There was a problem hiding this comment.
This param doesn't exist anymore
| @@ -61,16 +67,33 @@ def upload_with_chunked_encoding( | |||
|
|
|||
There was a problem hiding this comment.
Is this the exact same fn as the one in upload_manager.py? If so we might want to refactor / unify in a future PR
There was a problem hiding this comment.
Essentially. There are a few minor differences. I agree that it should be refactored.
codalab/worker/upload_util.py
Outdated
| @@ -92,7 +115,10 @@ def upload_with_chunked_encoding( | |||
| logging.debug("Socket timeout, retrying url: %s", url) | |||
| conn.send(b'\0') | |||
| logging.debug("Finished reading the response, url: %s", url) | |||
| print("Finished reading the response, url: %s", url) | |||
There was a problem hiding this comment.
remove these print statements
| USER_READ_ONLY_FIELDS. We make this special function (which only allows | ||
| positive disk increments so that users can't decrement their disk used) to ensure | ||
| that we can safely increment user disk used without introducing a | ||
| security flaw. |
There was a problem hiding this comment.
Add documentation as to what the request format should look like (i.e., the disk_used_increment key)
tests/cli/test_cli.py
Outdated
| _run_command([cl, 'work', worksheet_uuid]) | ||
| # expect to fail when we upload something more than 2k bytes | ||
| check_contains( | ||
| 'Upload aborted. User disk quota exceeded. ' |
There was a problem hiding this comment.
Just check if it contains "User disk quota exceeded" -- this way we don't need to change this test if we slightly modify the error message in the future.
Co-authored-by: Ashwin Ramaswami <aramaswamis@gmail.com>
Reasons for making this change
We want to make sure users can't bypass their disk quota limits by uploading files in parallel
Related issues
Fixes #4285