-
Notifications
You must be signed in to change notification settings - Fork 23
Replace backbeat client with cloudserver client and migration aws sdk to v3 #2679
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
base: development/9.1
Are you sure you want to change the base?
Conversation
Hello sylvainsenechal,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
6398439 to
269de9b
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 10 files with indirect coverage changes
@@ Coverage Diff @@
## development/9.1 #2679 +/- ##
===================================================
- Coverage 74.81% 74.24% -0.58%
===================================================
Files 201 200 -1
Lines 13579 13564 -15
===================================================
- Hits 10159 10070 -89
- Misses 3410 3484 +74
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
d06be27 to
11d7667
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
5c4933f to
616c43b
Compare
| return done(err); | ||
| } | ||
|
|
||
| const backbeatClient = this.getBackbeatClient(accountId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note : There are a lot of variables, function names and possibly file names that would benefit from a rename, from backbeatClient to cloudserverClient etc.
I don't wanna do it now because it will add a lot of noise to that PR, and it's not even that straightforward to do with this silly programming language
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to make it more palatable, think of it as Backbeat(Routes)Client for now...
616c43b to
dcd1111
Compare
| Key: destEntry.getObjectKey(), | ||
| CanonicalID: destEntry.getOwnerId(), | ||
| // TODO : missing content length | ||
| ContentLength: partSize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed content-length. Smithy doesn't allow us to have a content length variable that we defined ourselves to be used as the header, because it's calculating the header itself.
Only thing I'm worried about is that the new ContentLength from Smithy might not be equal to the previous one : I think the new ones uses the whole request, when our old content length was partObj.getObjSize()
404c946 to
897ee3e
Compare
| // eslint-disable-next-line no-param-reassign | ||
| err.origin = 'target'; | ||
| if (err.ObjNotFound || err.code === 'ObjNotFound') { | ||
| if (err.ObjNotFound || err.code === 'ObjNotFound' || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't super nice error checking 🫤
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we cannot guaarantee the type of error there? is it always an arsenal error, or an sdk one? if sdk you should be able to use instanceof, no?
Side note: can't it also be a NoSuchVersion in this case, if the versionID is not empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I double checked, we can use err.name === 'ObjNotFound' (or err.code too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the side note, I wasnt able to trigger any NoSuchVersion, but I was able to trigger NoSuchBucket.
In any case, nto very important imo : the error will always be thrown, this if statement is only there to not log the error in this specific situation
| .then(data => { | ||
| sourceEntry.setReplicationSiteDataStoreVersionId(this.site, | ||
| data.versionId); | ||
| // TODO : review metadata metrics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@francoisferrand I think using JSON.stringify will work, I see that the function .getSerialized() in Arsenal also returns a string from JSON.stringify that is then used in this same _publishMetadataWriteMetrics, but I'm not sure that "command.input" is equivalent to the old "destReq.httpRequest.body" data we used to call it with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few occurences where this _publishMetadataWriteMetrics is an issue for me.
Actually, we can add a middleware to extract the request body like this, and pass it to the metrics just as before.
Although, I have found that here it would likely be useless, because as you can see in the command above, there is Body in the command, so the length would always be 0.
I have only found one occurence where we the request has a body
command.middlewareStack.add(
next => async args => {
const request = args.request as any;
console.log("content-length:", request?.headers?.['content-length'])
console.log("len", Buffer.byteLength(request.body as any))
return next(args);
},
{ step: 'finalizeRequest' }
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thought : With the old SDK, we were using destReq.httpRequest.body, but if you look at the command, there is no proper body parameter, and with the new v3 client, I tried to get the body with the middleware and it is undefined...
Anyways, probably not worth it to overthink this problem, we just need to clearly state what is the metric we want to compute first (I believe that here it's the Tags, since the api is putTagging), so we probably just want to do something like
this._publishMetadataWriteMetrics(JSON.stringify(command.input.Tags), writeStartTime)
3f6b0a3 to
fb1beba
Compare
| } | ||
| if (!aborted) { | ||
|
|
||
| return this.backbeatClient.send(getObjectCommand) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the kind of changes that's a little bit tricky :
With this new GetObject, we cannot have the same "incomeMsg", readableStream, as we used to have before.
The body returned is of type "StreamingBlobPayloadOutputTypes", there are a few methods that we can call on it (transformToString, transformToByteArray, transformToWebStream).
Here, I choose to directly pass the returned body from getObject to _sendMultipleBackendPutObject, because the MultiBackendPutObject function should be directly capable of using the response.Body of that getObject, simplifying the code
| (err.origin === 'source' && | ||
| (err.NoSuchEntity || err.code === 'NoSuchEntity' || | ||
| err.AccessDenied || err.code === 'AccessDenied'))) { | ||
| (err.NoSuchEntity || err.code === 'NoSuchEntity' || err.name === 'NoSuchEntity' || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is horrible but I'll call it defensive code
e6f1071 to
2a4168a
Compare
143378d to
8cacb15
Compare
8cacb15 to
2d1ec8f
Compare
| accountId = res.Account; | ||
| } catch (err) { | ||
| // Workaround a Vault issue on 8.3 branch | ||
| // https://scality.atlassian.net/browse/VAULT-238 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this still relevant? Please create ticket to look at it, and possibly clean it eventually.,...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😂 I was checking the same. We already have the ticket I guess...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to the checklist
francoisferrand
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(still reviewing, but aleady some findings)
| if (err.code === 'ObjNotFound' || err.code === 'NoSuchBucket') { | ||
| return done(err, { committable: true }); | ||
| if (err.code === 'ObjNotFound' || err.code === 'NoSuchBucket' || | ||
| err.name === 'ObjNotFound' || err.name === 'NoSuchBucket') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no actual object type, as the errors are not "parsed"?
(should be added in CloudserverClient eventually, I guess)
We should create a ticket, and already "flag" the places which would benefit for this in backbeat with something like TODO: use error types once supported in CLDSRVCLT-5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I didn't pay much attention to proper smithy error generation, I'm adding this to my checklist
| // because it means the object has been deleted by other means and we don't need to retry | ||
| if (err.code === 'ObjNotFound' || err.code === 'NoSuchBucket') { | ||
| return done(err, { committable: true }); | ||
| if (err.code === 'ObjNotFound' || err.code === 'NoSuchBucket' || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to check both code and name?
esp. since errors later check only name, does not seem consistent (either we need to check everywhere, or we can check nane only) ?
→ please review and align behavior where/if relevant
|
|
||
| const s3 = this.clientManager.getS3Client(accountId); | ||
| if (!s3) { | ||
| const s3Client = this.clientManager.getS3Client(accountId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: renaming from s3 to s3client was not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to have a proper name for clients, I probably did this because results from ctrl-f "s3" returns to many irrelevant results
| logFields: { params }, | ||
| actionFunc: done => s3.getBucketLifecycleConfiguration(params, done), | ||
| actionFunc: done => { | ||
| const command = new GetBucketLifecycleConfigurationCommand(params); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we not attach request id here?
(maybe cleanup for later, but best flag it already with a TODO and ticket)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We talked about requestID earlier.
I think what we can do is leave it as it is with this pr (I kept request id where it was used, but didn't add it where it wasn't used).
Then follow up ticket to check if some apis should use it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but ew can already add comments pointing to this followup in the code: since it is relatively easy to identify those spots in the diff (as it is relatively small vs the whole codebase). It does not change the current PR behavior at all, but will give a starting point to look at when working on that followup ticket.
extensions/lifecycle/management.js
Outdated
|
|
||
| (async () => { | ||
| try { | ||
| const command = new DeleteBucketLifecycleCommand(params); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing request id
extensions/replication/management.js
Outdated
|
|
||
| (async () => { | ||
| try { | ||
| const command = new PutBucketVersioningCommand(params); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing requestId
extensions/replication/management.js
Outdated
| try { | ||
| const command = new PutBucketVersioningCommand(params); | ||
| await getS3Client(endpoint).send(command); | ||
| cb(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you must NEVER invoke the continuation callback from within the try block !
this creates hard to debug issues when the "followup" code (in the callback) throws an exception....and thus invokes the callback as well!
either we "fully" migrate to async/await (probably too large a change to do along with SDK), or we need to stick with promise.then()... or use utils.callbackify(), which can make this migration even simpler as it does the combining of success and failures callbacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I fixed it, and found the pattern in a few other file that I'll also fix
| accountId = res.Account; | ||
| } catch (err) { | ||
| // Workaround a Vault issue on 8.3 branch | ||
| // https://scality.atlassian.net/browse/VAULT-238 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😂 I was checking the same. We already have the ticket I guess...
francoisferrand
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments, but to sumup "major" issues:
- In quite a few places, code now calls the continuation callback from within the
tryblock. This is very dangerous (leading to duplicate callback code & hard to debug issues) and must be not be done : use callbackify() or Promise.then() instead - Some code can be dedup/simplified using callbackify() (e.g. common log or metrics in resolve & reject)
- “duplicated” CloudserverClient / S3Clent / IACClient setup, with lots of options & extra middleware : should be factorized with helper functions?
- Errors checked with both
err.codeanderr.name: seems weird, is this really needed? - RequestUID is not consistently passed ; not sure it can/should always be added, or how
getSerializedLogwill behave when not called from an API : but should be flagged to ease followup work - I still need to review the
AbortControllerand assocaited uses (CopyLocationTask/ReplicateObject). Now that we have fixed "underlying" issues with streaming, are these "deep" rework/refactor needed? Maybe worth checking, to minimize risk of breaking stuff by rewriting if not needed...
| } | ||
| return cb(null, roles[0], roles[1]); | ||
| }) | ||
| .catch(err => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: using utils.callbackify() instead of promise.then() allows to keep the same exact structure as before... or you can at least put the .catch block before .then
| return this.backbeatSource.getMetadata(params, (err, blob) => { | ||
| if (err) { | ||
| return this.backbeatSource.send(new GetMetadataCommand(params)) | ||
| .then(data => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, .catch().then() make it easier to review
| }; | ||
| return this.backbeatSource.getMetadata(params, (err, blob) => { | ||
| if (err) { | ||
| return this.backbeatSource.send(new GetMetadataCommand(params)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing request id
| return doneOnce(err); | ||
| }); | ||
| const incomingMsg = sourceReq.createReadStream(); | ||
| attachReqUids(command, log); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replaces with a attribute in command param?
|
Should probably create a follow up ticket (edit : created : https://scality.atlassian.net/jira/software/c/projects/OS/boards/268?selectedIssue=BB-730)
Other things to check now for this ticket :
|
77722a8 to
561e37e
Compare
561e37e to
19e0184
Compare
Issue: BB-706
sdk v3 migration + backbeat client migration