Skip to content

Commit

Permalink
fix(job_attachments): include file path in error message for upload f…
Browse files Browse the repository at this point in the history 
…ailures

- Added file path in custom error messages when multipart uploads fail due to a ClientError.
- Corrected indentation in existing unit tests for error message verification (in test_download.py).

Signed-off-by: Gahyun Suh <[email protected]>
  • Loading branch information
Gahyun Suh committed Dec 20, 2023
1 parent 4ace7c1 commit 8c2fc6b
Show file tree
Hide file tree
Showing 6 changed files with 108 additions and 54 deletions.
4 changes: 2 additions & 2 deletions scripted_tests/upload_cancel_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@
"To stop the hash/upload process, please hit 'k' key and then 'Enter' key in succession.\n"
)

NUM_SMALL_FILES = 0
NUM_MEDIUM_FILES = 0
NUM_SMALL_FILES = 20
NUM_MEDIUM_FILES = 5
NUM_LARGE_FILES = 1

continue_reporting = True
Expand Down
2 changes: 1 addition & 1 deletion src/deadline/job_attachments/download.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ def download_file(
status_code=status_code,
bucket_name=s3_bucket,
key_or_prefix=s3_key,
message=f"{status_code_guidance.get(status_code, '')} {str(exc)}",
message=f"{status_code_guidance.get(status_code, '')} {str(exc)} (Failed to download the file to {str(local_file_name)})",
) from exc

download_logger.debug(f"Downloaded {file.path} to {str(local_file_name)}")
Expand Down
6 changes: 3 additions & 3 deletions src/deadline/job_attachments/exceptions.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,13 @@ def __init__(
self.key_or_prefix = key_or_prefix

message_parts = [
f"Error {action} in bucket '{bucket_name}'. Target key or prefix: '{key_or_prefix}'.",
f"Error {action} in bucket '{bucket_name}', Target key or prefix: '{key_or_prefix}'",
f"HTTP Status Code: {status_code}",
]
if message:
message_parts.append(message)

super().__init__(" ".join(message_parts))
super().__init__(", ".join(message_parts))


class MissingS3BucketError(JobAttachmentsError):
Expand Down Expand Up @@ -89,7 +89,7 @@ class MissingManifestError(JobAttachmentsError):

class MissingAssetRootError(JobAttachmentsError):
"""
Exception for when trying to retrieve asset root from metatdata (in S3) that doesn't exist.
Exception for when trying to retrieve asset root from metadata (in S3) that doesn't exist.
"""


Expand Down
4 changes: 2 additions & 2 deletions src/deadline/job_attachments/upload.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ def upload_file_to_s3(
self._upload_part,
s3_bucket,
s3_upload_key,
local_path,
str(local_path),
upload_id,
chunk_size,
progress_tracker,
Expand Down Expand Up @@ -397,7 +397,7 @@ def upload_file_to_s3(
status_code=status_code,
bucket_name=s3_bucket,
key_or_prefix=s3_upload_key,
message=status_code_guidance.get(status_code, ""),
message=f"{status_code_guidance.get(status_code, '')} {str(exc)} (Failed to upload {str(local_path)})",
) from exc
except Exception as exc:
if upload_id:
Expand Down
78 changes: 40 additions & 38 deletions test/unit/deadline_job_attachments/test_download.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1650,16 +1650,16 @@ def test_get_asset_root_from_s3_error_message_on_not_found(self):
with stubber, patch(
f"{deadline.__package__}.job_attachments.download.get_s3_client", return_value=s3_client
):
with pytest.raises(JobAttachmentsS3ClientError) as exc:
_get_asset_root_from_s3("not/existed/test.txt", "my-bucket")
assert isinstance(exc.value.__cause__, ClientError)
assert (
exc.value.__cause__.response["ResponseMetadata"]["HTTPStatusCode"] == 404 # type: ignore[attr-defined]
)
assert (
"Error checking if object exists in bucket 'my-bucket'. Target key or prefix: 'not/existed/test.txt'. "
"HTTP Status Code: 404 Not found. "
) in str(exc.value)
with pytest.raises(JobAttachmentsS3ClientError) as err:
_get_asset_root_from_s3("not/existed/test.txt", "test-bucket")
assert isinstance(err.value.__cause__, ClientError)
assert (
err.value.__cause__.response["ResponseMetadata"]["HTTPStatusCode"] == 404 # type: ignore[attr-defined]
)
assert (
"Error checking if object exists in bucket 'test-bucket', Target key or prefix: 'not/existed/test.txt', "
"HTTP Status Code: 404, Not found. "
) in str(err.value)

@mock_sts
def test_get_manifest_from_s3_error_message_on_access_denied(self):
Expand All @@ -1680,15 +1680,15 @@ def test_get_manifest_from_s3_error_message_on_access_denied(self):
f"{deadline.__package__}.job_attachments.download.get_s3_client", return_value=s3_client
):
with pytest.raises(JobAttachmentsS3ClientError) as exc:
get_manifest_from_s3("test-key", "my-bucket")
assert isinstance(exc.value.__cause__, ClientError)
assert (
exc.value.__cause__.response["ResponseMetadata"]["HTTPStatusCode"] == 403 # type: ignore[attr-defined]
)
assert (
"Error downloading binary file in bucket 'my-bucket'. Target key or prefix: 'test-key'. "
"HTTP Status Code: 403 Forbidden or Access denied. "
) in str(exc.value)
get_manifest_from_s3("test-key", "test-bucket")
assert isinstance(exc.value.__cause__, ClientError)
assert (
exc.value.__cause__.response["ResponseMetadata"]["HTTPStatusCode"] == 403 # type: ignore[attr-defined]
)
assert (
"Error downloading binary file in bucket 'test-bucket', Target key or prefix: 'test-key', "
"HTTP Status Code: 403, Forbidden or Access denied. "
) in str(exc.value)

@mock_sts
def test_get_tasks_manifests_keys_from_s3_error_message_on_access_denied(self):
Expand All @@ -1710,17 +1710,17 @@ def test_get_tasks_manifests_keys_from_s3_error_message_on_access_denied(self):
):
with pytest.raises(JobAttachmentsS3ClientError) as exc:
_get_tasks_manifests_keys_from_s3(
"test_prefix",
"my-bucket",
"assetRoot",
"test-bucket",
)
assert isinstance(exc.value.__cause__, ClientError)
assert (
exc.value.__cause__.response["ResponseMetadata"]["HTTPStatusCode"] == 403 # type: ignore[attr-defined]
)
assert (
"Error listing bucket contents in bucket 'my-bucket'. Target key or prefix: 'test_prefix'. "
"HTTP Status Code: 403 Forbidden or Access denied. "
) in str(exc.value)
assert isinstance(exc.value.__cause__, ClientError)
assert (
exc.value.__cause__.response["ResponseMetadata"]["HTTPStatusCode"] == 403 # type: ignore[attr-defined]
)
assert (
"Error listing bucket contents in bucket 'test-bucket', Target key or prefix: 'assetRoot', "
"HTTP Status Code: 403, Forbidden or Access denied. "
) in str(exc.value)

@mock_sts
def test_download_file_error_message_on_access_denied(self):
Expand All @@ -1746,16 +1746,18 @@ def test_download_file_error_message_on_access_denied(self):
), patch(f"{deadline.__package__}.job_attachments.download.Path.mkdir"):
with pytest.raises(JobAttachmentsS3ClientError) as exc:
download_file(
file_path, "/home/username/assets", "my-bucket", "rootPrefix/Data", s3_client
file_path, "/home/username/assets", "test-bucket", "rootPrefix/Data", s3_client
)
assert isinstance(exc.value.__cause__, ClientError)
assert (
exc.value.__cause__.response["ResponseMetadata"]["HTTPStatusCode"] == 403 # type: ignore[attr-defined]
)
assert (
"Error downloading file in bucket 'my-bucket'. Target key or prefix: 'rootPrefix/Data/relative_file_path'. "
"HTTP Status Code: 403 Forbidden or Access denied. "
) in str(exc.value)
assert isinstance(exc.value.__cause__, ClientError)
assert (
exc.value.__cause__.response["ResponseMetadata"]["HTTPStatusCode"] == 403 # type: ignore[attr-defined]
)
assert (
"Error downloading file in bucket 'test-bucket', Target key or prefix: 'rootPrefix/Data/input1', "
"HTTP Status Code: 403, Forbidden or Access denied. "
) in str(exc.value)
failed_file_path = Path("/home/username/assets/inputs/input1.txt")
assert (f"(Failed to download the file to {str(failed_file_path)})") in str(exc.value)


@pytest.mark.parametrize("manifest_version", [ManifestVersion.v2023_03_03])
Expand Down
68 changes: 60 additions & 8 deletions test/unit/deadline_job_attachments/test_upload.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1231,8 +1231,8 @@ def test_file_already_uploaded_bucket_in_different_account(self):
err.value.__cause__.response["ResponseMetadata"]["HTTPStatusCode"] == 403 # type: ignore[attr-defined]
)
assert (
"Error checking if object exists in bucket 'test-bucket'. Target key or prefix: 'test_key'. "
"HTTP Status Code: 403 Access denied. Ensure that the bucket is in the account 123456789012, "
"Error checking if object exists in bucket 'test-bucket', Target key or prefix: 'test_key', "
"HTTP Status Code: 403, Access denied. Ensure that the bucket is in the account 123456789012, "
"and your AWS IAM Role or User has the 's3:ListBucket' permission for this bucket."
) in str(err.value)

Expand Down Expand Up @@ -1265,8 +1265,8 @@ def test_filter_objects_to_upload_bucket_in_different_account(self):
err.value.__cause__.response["ResponseMetadata"]["HTTPStatusCode"] == 403 # type: ignore[attr-defined]
)
assert (
"Error listing bucket contents in bucket 'test-bucket'. Target key or prefix: 'test_prefix'. "
"HTTP Status Code: 403 Forbidden or Access denied. "
"Error listing bucket contents in bucket 'test-bucket', Target key or prefix: 'test_prefix', "
"HTTP Status Code: 403, Forbidden or Access denied. "
) in str(err.value)

@mock_sts
Expand Down Expand Up @@ -1300,10 +1300,61 @@ def test_upload_bytes_to_s3_bucket_in_different_account(self):
err.value.__cause__.response["ResponseMetadata"]["HTTPStatusCode"] == 403 # type: ignore[attr-defined]
)
assert (
"Error uploading binary file in bucket 'test-bucket'. Target key or prefix: 'test_key'. "
"HTTP Status Code: 403 Forbidden or Access denied. "
"Error uploading binary file in bucket 'test-bucket', Target key or prefix: 'test_key', "
"HTTP Status Code: 403, Forbidden or Access denied. "
) in str(err.value)

@mock_sts
def test_upload_file_to_s3_upload_part_failure(self, tmp_path: Path):
"""
Test that the error message correctly includes the failed file path when a multipart upload
(upload_part method) fails with a 400 error code.
"""
bucket_name = self.job_attachment_s3_settings.s3BucketName
s3 = boto3.client("s3")
stubber = Stubber(s3)
stubber.add_response(
"create_multipart_upload",
service_response={"UploadId": "test_upload_id"},
expected_params={
"Bucket": bucket_name,
"Key": "test_key",
"ExpectedBucketOwner": "123456789012",
},
)
stubber.add_client_error(
"upload_part",
service_error_code="InvalidPart",
service_message="Invalid Part",
http_status_code=400,
)
stubber.add_response(
"abort_multipart_upload",
service_response={},
expected_params={
"Bucket": bucket_name,
"Key": "test_key",
"UploadId": "test_upload_id",
"ExpectedBucketOwner": "123456789012",
},
)

uploader = S3AssetUploader()
uploader._s3 = s3

file = tmp_path / "test_file.txt"
file.write_text("test file")

with stubber:
with pytest.raises(JobAttachmentsS3ClientError) as err:
uploader.upload_file_to_s3(file, bucket_name, "test_key")
assert isinstance(err.value.__cause__, ClientError)
assert err.value.__cause__.response["ResponseMetadata"]["HTTPStatusCode"] == 400
assert (
"Error uploading file in bucket 'test-bucket', Target key or prefix: 'test_key', HTTP Status Code: 400"
) in str(err.value)
assert (f"(Failed to upload {str(file)})") in str(err.value)

@mock_sts
def test_upload_file_to_s3_bucket_in_different_account(self, tmp_path: Path):
"""
Expand Down Expand Up @@ -1338,9 +1389,10 @@ def test_upload_file_to_s3_bucket_in_different_account(self, tmp_path: Path):
err.value.__cause__.response["ResponseMetadata"]["HTTPStatusCode"] == 403 # type: ignore[attr-defined]
)
assert (
"Error uploading file in bucket 'test-bucket'. Target key or prefix: 'test_key'. "
"HTTP Status Code: 403 Forbidden or Access denied. "
"Error uploading file in bucket 'test-bucket', Target key or prefix: 'test_key', "
"HTTP Status Code: 403, Forbidden or Access denied. "
) in str(err.value)
assert (f"(Failed to upload {str(file)})") in str(err.value)

@pytest.mark.parametrize(
"manifest_version",
Expand Down

0 comments on commit 8c2fc6b

Please sign in to comment.