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

fix(job_attachments)!: use correct profile for GetStorageProfileForQueue API #296

Conversation

gahyusuh
Copy link
Contributor

@gahyusuh gahyusuh commented Apr 12, 2024

What was the problem/requirement? (What/Why)

When submitting a job with a Storage Profile ID, the GetStorageProfileForQueue API was using the default AWS profile instead of the Deadline Cloud submitter's configured profile. If the two profiles pointed to different regions, the API couldn't find the storage profile resource, resulting in a job submission failure

What was the solution? (How)

Modified the code to use the correct boto3 session (using the Deadline Cloud submitter's profile) when calling the GetStorageProfileForQueue API. The API is no longer called from src/deadline/job_attachments/upload.py. Instead, the API is called from the below places:

  1. src/deadline/client/api/_submit_job_bundle.py
  2. src/deadline/client/ui/dialogs/submit_job_to_deadline_dialog.py
    and the prepare_paths_for_upload() function now receives storage_profile object itself instead of storage_profile_id.

What is the impact of this change?

Fixes the job submission issue when involving a Storage Profile in a different region than the default AWS profile.

How was this change tested?

  1. I created AWS CLI's [default] profile and Deadline Cloud default profile, where each profile is pointing to different regions.
  2. Before the change, job submission to a queue with a Storage Profile failed with a "storage profile does not exist" error.
  3. After the change, job submission to the same queue succeeded, and the job could be created.
  4. A new job was submitted to a Queue with a Storage Profile, and I executed a worker (CMF) to verify that asset sync and output downloads worked as expected.

Was this change documented?

No.

Is this a breaking change?

Yes, there are changes in the public interface (S3AssetManager's prepare_paths_for_upload()).


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@gahyusuh gahyusuh force-pushed the gahyusuh/get_storage_profile_missing_session branch from 0167a9c to b698da6 Compare April 15, 2024 14:48
@gahyusuh gahyusuh marked this pull request as ready for review April 15, 2024 17:37
@gahyusuh gahyusuh requested a review from a team as a code owner April 15, 2024 17:37
marofke
marofke previously approved these changes Apr 16, 2024
if storage_profile_id:
create_job_args["storageProfileId"] = storage_profile_id
storage_profile_response = deadline.get_storage_profile_for_queue(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: since we're doing this in two places, we might want to make a function wrapper like what we've done in other places, like queue params https://github.com/aws-deadline/deadline-cloud/blob/mainline/src/deadline/client/api/_queue_parameters.py

Not blocking though, since we didn't hit the rule of 3 ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the advise. I have dded a function wrapper get_storage_profile_for_queue in client/api/_get_storage_profile_for_queue.py.

chocecil
chocecil previously approved these changes Apr 16, 2024
@@ -1333,8 +1333,9 @@ def test_upload_bucket_wrong_account(external_bucket: str, job_attachment_test:

# WHEN
with pytest.raises(
AssetSyncError, match=f"Error checking if object exists in bucket '{external_bucket}'"
AssetSyncError, match=f"Error uploading binary file in bucket '{external_bucket}'"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about the exception. When I ran this test, I received an "access denied" error. Was the related logic updated in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I also saw the "access denied" error message. There were no updates related to this in this PR. I found it a bit strange that this integ test was failing... The error message that's currently showing up seems correct though - it's expected that PutObject would be denied if we try to upload a manifest file to a bucket that we don't have access to.

@gahyusuh gahyusuh dismissed stale reviews from chocecil and marofke via 2ebad98 April 16, 2024 21:16
@gahyusuh gahyusuh force-pushed the gahyusuh/get_storage_profile_missing_session branch 2 times, most recently from 2ebad98 to 6bd5c17 Compare April 16, 2024 21:18
@gahyusuh gahyusuh force-pushed the gahyusuh/get_storage_profile_missing_session branch from 6bd5c17 to 6cc7596 Compare April 17, 2024 01:29
@gahyusuh gahyusuh merged commit a8de5f6 into aws-deadline:mainline Apr 17, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants