Repository navigation
Conversation
(cherry picked from commit 5184ec2) Dropped monitoring, as it appeared only in 7.70 and not 7.10 This goes with scality/Arsenal#2720 For RD-2378
Hello bourgoismickael,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
BourgoisMickael
changed the title
BourgoisMickael
changed the title
| return callback(err); | ||
| // Check KMS Key access and usability before checking data | ||
| // diff with AWS: for empty object (no dataLocator) KMS not checked | ||
| return async.each(dataLocator || [], |
There was a problem hiding this comment.
nit/question: async.each fires one KMS call per location entry, all in parallel. For an MPU object with thousands of parts, that is thousands of concurrent requests to the KMS on a single GET. Could we use async.eachLimit with a small cap (5 or 10) so one big object cannot flood the KMS?
There was a problem hiding this comment.
Right, and this has been shipped like that since more than 1 year in 9.5.0. This needs to be fixed in all versions.
I could even dedup the keys to avoid querying the same key multiple times as for MPU it's likely all parts use the same master key.
There was a problem hiding this comment.
Created ticket https://scality.atlassian.net/browse/CLDSRV-1020 for that.
| }); | ||
| }, | ||
| err => { | ||
| if (err) { |
There was a problem hiding this comment.
Question: if one part's createDecipherBundle fails here, or data.head fails just below, what happens to the decipher streams already attached to the other parts? Same question if the client goes away before streaming starts. As far as I can tell they are never consumed or destroyed. Is that harmless, or should we destroy any objectGetInfo.decipherStream already set on these error paths? It feels like the same kind of lifecycle gap the arsenal fix covers, but I may be missing where they get cleaned up.
Also, would it make sense to add a cloudserver unit test for "KMS error is returned before any data is read", for example with a stubbed kms.createDecipherBundle that returns an error? That seems to be the main behavior this backport is for.
There was a problem hiding this comment.
The current fix is related to an HTTP request keeping a socket to sproxyd.
The decipher stream has no socket to external service, it's a stream.Transform from crypto
Line 124 in 17977b1
https://nodejs.org/api/crypto.html#cryptocreatedecipherivalgorithm-key-iv-options
It's in memory Transform with no socket involved. So it gets garbage collected.
JS memory is very light on Transform but that crypto does hold for OpenSSL some C++ native memory. So it would be better to clean them ASAP instead of waiting for GC or it might create native memory OOM in some conditions.
I'll likely open another PR / ticket to fix those 2 issues present in all versions of cloudserver out of this backport.
And I'll add some tests as well if they don't exist
bb3935e to
a5b4c80
Compare
|
/create_integration_branches |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: git fetch
git checkout -B w/7.70/improvement/CLDSRV-1012-bump-arsenal-datawrapper-7.10 origin/development/7.70
git merge origin/improvement/CLDSRV-1012-bump-arsenal-datawrapper-7.10
# <intense conflict resolution>
git commit
git push -u origin w/7.70/improvement/CLDSRV-1012-bump-arsenal-datawrapper-7.10The following options are set: create_integration_branches |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: git fetch
git checkout -B w/8.8/improvement/CLDSRV-1012-bump-arsenal-datawrapper-7.10 origin/development/8.8
git merge origin/w/7.70/improvement/CLDSRV-1012-bump-arsenal-datawrapper-7.10
# <intense conflict resolution>
git commit
git push -u origin w/8.8/improvement/CLDSRV-1012-bump-arsenal-datawrapper-7.10The following options are set: create_integration_branches |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: git fetch
git checkout -B w/9.4/improvement/CLDSRV-1012-bump-arsenal-datawrapper-7.10 origin/development/9.4
git merge origin/w/9.3/improvement/CLDSRV-1012-bump-arsenal-datawrapper-7.10
# <intense conflict resolution>
git commit
git push -u origin w/9.4/improvement/CLDSRV-1012-bump-arsenal-datawrapper-7.10The following options are set: create_integration_branches |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option The following options are set: create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
|
/approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
This pull request does not target the following hotfix branch(es) so they
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve, create_integration_branches |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue CLDSRV-1012. Goodbye bourgoismickael. |
5 participants
Cherry picked 5184ec2 from CLDSRV-629: KMIP error management #5780 to handle KMS before data to return error (Dropped monitoring, as it appeared only in 7.70 and not 7.10)
Bump arsenal for handling socket leak with KMS or backend error: ARSN-652: Backport DataWrapper error improvement & more Arsenal#2720
For RD-2378
Integration test passing: https://github.com/scality/Integration/actions/runs/37098044524
(arsenal is not bumped in integration branches)