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

🐛 Attempt to fix 400 RequestTimeout when uploading to S3 ⚠️ #4996

Merged
merged 34 commits into from
Nov 10, 2023

Conversation

GitHK
Copy link
Contributor

@GitHK GitHK commented Nov 8, 2023

What do these changes do?

This is an enhancement based on #4483

Attempts to fix a long running issue when uploading files to S3, sometimes the AWS S3 backend will reply with 400 ReuestTimeout.

When receiving such a reply the entire multipart upload is stopped and another one will be started once more. New multipart upload links will be requested.

The retry is only applied to the above mentioned case. A specific error was created and isolated to better be propagated and handled.
To disable this feature set DIRECTOR_V2_NODE_PORTS_400_REQUEST_TIMEOUT_ATTEMPTS to 0. No retry will happen and the error will just bubble up as it previously did, it will now show as AWSS3400RequestTimeOutError.

Devops ⚠️

No actions are required from devops. Be aware about the following:

  • added new env var DIRECTOR_V2_NODE_PORTS_400_REQUEST_TIMEOUT_ATTEMPTS to the director-v2 which will be sent to the dynamic-sidecar. Inside it, nodeports captures the env var and uses it for the retry.

Related issue/s

How to test

DevOps Checklist

@GitHK GitHK self-assigned this Nov 8, 2023
@GitHK GitHK added this to the 7peaks milestone Nov 8, 2023
@GitHK GitHK force-pushed the pr-osparc-404-bad-request branch from 4b2f5da to cbf45d6 Compare November 8, 2023 09:50
@GitHK GitHK changed the title 🐛 Attempt to fix 404 ReuestTimeout when uploading to S3 🐛 Attempt to fix 400 ReuestTimeout when uploading to S3 Nov 8, 2023
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Merging #4996 (4fcb191) into master (910c1a1) will decrease coverage by 0.1%.
The diff coverage is 97.8%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4996      +/-   ##
=========================================
- Coverage    87.1%   87.0%    -0.1%     
=========================================
  Files        1243     907     -336     
  Lines       51132   40099   -11033     
  Branches     1081     198     -883     
=========================================
- Hits        44549   34921    -9628     
+ Misses       6345    5129    -1216     
+ Partials      238      49     -189     
Flag Coverage Δ
integrationtests 63.6% <91.4%> (-0.1%) ⬇️
unittests 84.4% <72.3%> (-0.6%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...dk/src/simcore_sdk/node_ports_common/exceptions.py 69.8% <100.0%> (+1.2%) ⬆️
...k/src/simcore_sdk/node_ports_common/filemanager.py 84.6% <100.0%> (+1.1%) ⬆️
...-sdk/src/simcore_sdk/node_ports_common/settings.py 100.0% <100.0%> (ø)
...2/src/simcore_service_director_v2/core/settings.py 98.8% <100.0%> (+<0.1%) ⬆️
...es/dynamic_sidecar/docker_service_specs/sidecar.py 88.1% <ø> (ø)
...src/simcore_sdk/node_ports_common/file_io_utils.py 88.4% <96.2%> (-0.2%) ⬇️

... and 341 files with indirect coverage changes

@GitHK GitHK force-pushed the pr-osparc-404-bad-request branch from 9fb4502 to 746ca49 Compare November 8, 2023 10:50
@GitHK GitHK marked this pull request as ready for review November 9, 2023 06:40
@GitHK GitHK requested a review from bisgaard-itis November 9, 2023 06:40
@pcrespov pcrespov changed the title 🐛 Attempt to fix 400 ReuestTimeout when uploading to S3 🐛 Attempt to fix 400 RequestTimeout when uploading to S3 Nov 9, 2023
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

Please check how you raise your exception.. there is dead code

Copy link
Contributor

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

thanks! just think of updating the description of your PR

@GitHK GitHK changed the title 🐛 Attempt to fix 400 RequestTimeout when uploading to S3 🐛 Attempt to fix 400 RequestTimeout when uploading to S3 ⚠️ Nov 10, 2023
@GitHK GitHK enabled auto-merge (squash) November 10, 2023 09:11
Copy link

codeclimate bot commented Nov 10, 2023

Code Climate has analyzed commit 4fcb191 and detected 0 issues on this pull request.

View more on Code Climate.

Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.1% 0.1% Duplication

@GitHK GitHK merged commit d1f2dc6 into ITISFoundation:master Nov 10, 2023
@GitHK GitHK deleted the pr-osparc-404-bad-request branch November 10, 2023 10:02
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Nov 23, 2023
29 tasks
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.

4 participants