Fix AWS credential creation error handling #2321

suquark · 2023-07-28T23:40:03Z

Address the credential fetching issue mentioned in #2288

Tested (run the relevant ones):

Code formatting: bash format.sh
Any manual or new tests for this PR (please specify below)
All smoke tests: pytest tests/test_smoke.py
Relevant individual smoke tests: pytest tests/test_smoke.py --aws
Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

concretevitamin

Quick look. Thanks for the in-depth investigation @suquark. Some questions inlined.

Just to make sure my understanding of our plan is right:

We need to make session()/resource() thread-local since they are not thread-safe. We also want to cache it once for each thread's lifetime.
We can in principle reuse client() across threads. However, if it's created by calling session() first, that func is already made thread-local, so client() is going to be made thread-local too?

sky/adaptors/aws.py

concretevitamin · 2023-07-29T00:50:50Z

sky/adaptors/aws.py

+        try:
+            with _session_creation_lock:
+                return boto3.session.Session()
+        except (botocore_exceptions().CredentialRetrievalError,


Actually, I'm not too sure whether the error reported by the user in #1988 was thrown at session create time. There was no stacktrace. Spot controller log:

I 05-29 16:23:00 spot_utils.py:69] === Checking the job status... === I 05-29 16:23:01 spot_utils.py:75] Job status: JobStatus.RUNNING I 05-29 16:23:01 spot_utils.py:78] ================================== sky.exceptions.ClusterStatusFetchingError: Failed to query AWS cluster 'train-xxxxxxx-16' status: Unable to locate credentials. You can configure credentials by running "aws configure". Unexpected error occurred: [sky.exceptions.ClusterStatusFetchingError] Failed to query AWS cluster 'train-xxxxxxx-16' status: Unable to locate credentials. You can configure credentials by running "aws configure". I 05-29 16:23:06 spot_state.py:292] Unexpected error occurred: [sky.exceptions.ClusterStatusFetchingError] Failed to query AWS cluster 'train-xxxxxxx-16' status: Unable to locate credentials. You can configure credentials by running "aws configure".

Guessing from the newer stacktrace in #2301, is it possible that NoCredentialsError is thrown not at creation time by at request time?

Another possibility: #2301 happened because the current session/resource handling are not thread-safe.

hmmm, if it happens at request time, then I do not have good approaches to handle this (seems impossible to distinguish between a real error or flaky env issue). But this may not be an issue, since we add default retries here:

skypilot/sky/provision/aws/instance.py

Lines 16 to 21 in ca2a092

def _default_ec2_resource(region: str) -> Any:

return aws.resource(

'ec2',

region_name=region,

config=config.Config(retries={'max_attempts': BOTO_MAX_RETRIES}))

concretevitamin

Nice, thanks a bunch @suquark! Should I run smoke tests?

sky/adaptors/aws.py

concretevitamin · 2023-08-05T02:02:09Z

sky/adaptors/aws.py

+        _local.resource = {}
+
+    # Using service name and kwargs as key
+    sorted_kwargs = tuple(sorted(kwargs.items(), key=lambda x: x[0]))


key=lambda x: x[0] seems omittable?

well, kwargs.items() returns a list of pair of (key: str, value: Any), where we only want to compare against the key, since values may not be comparable.

Sg. Ref I found: https://github.com/python/cpython/blob/main/Lib/functools.py#L448

concretevitamin · 2023-08-05T02:02:50Z

sky/adaptors/aws.py

+        _local.client = {}
+
+    # Using service name and kwargs as key
+    sorted_kwargs = tuple(sorted(kwargs.items(), key=lambda x: x[0]))


(same above)

suquark · 2023-08-05T07:24:59Z

I have run the smoke tests. Only these tests fail due to lack of quotas with my account:

FAILED tests/test_smoke.py::test_aws_image_id_dict - Exception: test failed: less /var/folders/4m/37zr5qys6yxbhs49_gwr1vb80000gn/T/aws_image_id_dict-s5sbybpz.log
FAILED tests/test_smoke.py::test_image_no_conda - Exception: test failed: less /var/folders/4m/37zr5qys6yxbhs49_gwr1vb80000gn/T/image_no_conda-1kklvlpb.log
FAILED tests/test_smoke.py::test_aws_image_id_dict_zone - Exception: test failed: less /var/folders/4m/37zr5qys6yxbhs49_gwr1vb80000gn/T/aws_image_id_dict_zone-8qfi2j7n.log
FAILED tests/test_smoke.py::test_aws_image_id_dict_region - Exception: test failed: less /var/folders/4m/37zr5qys6yxbhs49_gwr1vb80000gn/T/aws_image_id_dict_region-px_098q0.log
FAILED tests/test_smoke.py::test_spot_recovery_aws - Exception: test failed: less /var/folders/4m/37zr5qys6yxbhs49_gwr1vb80000gn/T/spot_recovery_aws-en7297qe.log
FAILED tests/test_smoke.py::test_custom_image - Exception: test failed: less /var/folders/4m/37zr5qys6yxbhs49_gwr1vb80000gn/T/test-custom-image-_8fus_yg.log

suquark · 2023-08-05T07:32:52Z

Thanks for the suggestions! I updated the code and comments.

concretevitamin

LGTM @suquark. All these tests passed for me:

FAILED tests/test_smoke.py::test_aws_image_id_dict - Exception: test failed: less /var/folders/4m/37zr5qys6yxbhs49_gwr1vb80000gn/T/aws_image_id_dict-s5sbybpz.log
FAILED tests/test_smoke.py::test_image_no_conda - Exception: test failed: less /var/folders/4m/37zr5qys6yxbhs49_gwr1vb80000gn/T/image_no_conda-1kklvlpb.log
FAILED tests/test_smoke.py::test_aws_image_id_dict_zone - Exception: test failed: less /var/folders/4m/37zr5qys6yxbhs49_gwr1vb80000gn/T/aws_image_id_dict_zone-8qfi2j7n.log
FAILED tests/test_smoke.py::test_aws_image_id_dict_region - Exception: test failed: less /var/folders/4m/37zr5qys6yxbhs49_gwr1vb80000gn/T/aws_image_id_dict_region-px_098q0.log
FAILED tests/test_smoke.py::test_spot_recovery_aws - Exception: test failed: less /var/folders/4m/37zr5qys6yxbhs49_gwr1vb80000gn/T/spot_recovery_aws-en7297qe.log
FAILED tests/test_smoke.py::test_custom_image - Exception: test failed: less /var/folders/4m/37zr5qys6yxbhs49_gwr1vb80000gn/T/test-custom-image-_8fus_yg.log

fix

06acbf2

concretevitamin reviewed Jul 29, 2023

View reviewed changes

fix

5dd4cda

suquark requested a review from concretevitamin August 4, 2023 22:37

use thread-local for aws objects

83e3415

concretevitamin approved these changes Aug 5, 2023

View reviewed changes

update comments

4b72eca

suquark requested a review from concretevitamin August 5, 2023 07:32

concretevitamin approved these changes Aug 6, 2023

View reviewed changes

suquark merged commit 2b2a158 into master Aug 6, 2023

suquark deleted the retry_on_credential_error branch August 6, 2023 06:44

Michaelvll mentioned this pull request Oct 5, 2023

[Storage/S3] Retry with new session/client on 'Unable to locate credentials' error. #2314

Closed

2 tasks

landscapepainter mentioned this pull request Nov 27, 2023

[storage] S3 client should auto-retry NoCredentialsError #2301

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix AWS credential creation error handling #2321

Fix AWS credential creation error handling #2321

suquark commented Jul 28, 2023 •

edited

Loading

concretevitamin left a comment

concretevitamin Jul 29, 2023

suquark Aug 4, 2023

concretevitamin left a comment

concretevitamin Aug 5, 2023

suquark Aug 5, 2023

concretevitamin Aug 6, 2023

concretevitamin Aug 5, 2023

suquark Aug 5, 2023

suquark commented Aug 5, 2023

suquark commented Aug 5, 2023

concretevitamin left a comment

	def _default_ec2_resource(region: str) -> Any:
	return aws.resource(
	'ec2',
	region_name=region,
	config=config.Config(retries={'max_attempts': BOTO_MAX_RETRIES}))

Fix AWS credential creation error handling #2321

Fix AWS credential creation error handling #2321

Conversation

suquark commented Jul 28, 2023 • edited Loading

concretevitamin left a comment

Choose a reason for hiding this comment

concretevitamin Jul 29, 2023

Choose a reason for hiding this comment

suquark Aug 4, 2023

Choose a reason for hiding this comment

concretevitamin left a comment

Choose a reason for hiding this comment

concretevitamin Aug 5, 2023

Choose a reason for hiding this comment

suquark Aug 5, 2023

Choose a reason for hiding this comment

concretevitamin Aug 6, 2023

Choose a reason for hiding this comment

concretevitamin Aug 5, 2023

Choose a reason for hiding this comment

suquark Aug 5, 2023

Choose a reason for hiding this comment

suquark commented Aug 5, 2023

suquark commented Aug 5, 2023

concretevitamin left a comment

Choose a reason for hiding this comment

suquark commented Jul 28, 2023 •

edited

Loading