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

388 race condition occurs when adding folder to shared items in shares and errors out #392

Conversation

dlpzx
Copy link
Contributor

@dlpzx dlpzx commented Mar 30, 2023

Feature or Bugfix

  • Bugfix

Detail

  • The creation of S3 access points is asynchronous and can take more than 5 seconds to complete. When the share managers tries attaching the policy to the access points it fails in certain cases. This PR replaces the waiting time of 5 seconds for a while loop that checks that the access points has been created and if not it waits for 30s

Relates

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dlpzx dlpzx self-assigned this Mar 30, 2023
@dlpzx dlpzx requested a review from nikpodsh March 30, 2023 06:19
@@ -196,7 +196,11 @@ def manage_access_point_and_policy(self):
)
access_point_arn = S3.create_bucket_access_point(self.source_account_id, self.source_environment.region, self.bucket_name, self.access_point_name)
# Access point creation is slow
time.sleep(5)
while not S3.get_bucket_access_point_arn(self.source_account_id, self.source_environment.region, self.access_point_name):
Copy link
Contributor

Choose a reason for hiding this comment

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

We potentially can stuck here forever if there is an error and we can't create a access point. Should we think about task execution limit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeeep, I always overlook while loops! Thank you

logger.info(
'Waiting 30s for access point creation to complete..'
)
time.sleep(30)
existing_policy = S3.get_access_point_policy(self.source_account_id, self.source_environment.region, self.access_point_name)
Copy link
Contributor

@nikpodsh nikpodsh Mar 30, 2023

Choose a reason for hiding this comment

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

Not to request information twice we could do:

    access_point_arn = S3.create_bucket_access_point(self.source_account_id, self.source_environment.region, self.bucket_name, self.access_point_name)
existing_policy = None
num_retries = ...
while not existing_policy and num_retries > 0:
        num_retries -= 1
        waiting....
        existing_policy = S3.get_access_point_policy....

Copy link
Contributor Author

@dlpzx dlpzx Mar 30, 2023

Choose a reason for hiding this comment

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

The policy should not exist even after we create the access point. So checking the policy existence is not going to solve the issue. The error arises when we try to attach a new policy to a still-creating access point. But open to ideas on how to make it better, I coded it a bit fast

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification!

@dlpzx dlpzx merged commit 9057116 into main Mar 30, 2023
@dlpzx dlpzx deleted the 388-race-condition-occurs-when-adding-folder-to-shared-items-in-shares-and-errors-out branch April 4, 2023 07:14
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.

Race condition occurs when adding folder to shared items in shares and errors out
2 participants