-
Notifications
You must be signed in to change notification settings - Fork 254
Cloudserver adaptation post arsenal refacto #6044
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.3
Are you sure you want to change the base?
Conversation
Hello benzekrimaha,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
5baf746 to
ea9cb65
Compare
❌ 2 Tests Failed:
View the full list of 2 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
| return gcpClient.send(command) | ||
| .then(data => callback(null, data)) | ||
| .catch(err => { | ||
| if (err && err.$metadata && err.$metadata.httpStatusCode && |
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 need to check if err exists here. I think err.$metadata.httpstatus code will always be defined.
Probably same for statusCode, gotta call the api once to verify but I feel like we shouldn't have to check if its defined or not
| before(done => { | ||
| config = getRealAwsConfig(credentialOne); | ||
| gcpClient = new GCP(config); | ||
| gcpClient.listObjects = (params, callback) => { |
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.
Just to understand : this was available in Arsenal GcpService but you just removed it and now you are redefining it in all these files, why not keeping it in Arsenal ? I think the GcpService already has several functions that are used for tests only 🤔
Or is it because you want to reassign the statusCode ?
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.
Upon looking : consider updating the gcpClientRetry function : check err.$metadata.statuscode == 429 on top of the existing check err.statusCode == 429.
This way, can keep listObjects in Arsenal. (unless there are other reason I didn't see)
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.
even if we added the $metadata.httpStatusCode check inside gcpClientRetry, we still need bespoke error munging in these raw-node tests. Rather than keeping a half-working helper in Arsenal plus a bunch of local overrides, I’d rather keep the shared GcpService minimal and let the tests own whatever extra plumbing they require.
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.
- in tests, we should not need a generic wrapper to remap errors : each test is a single scenario, we can update that test to check the excepted result (both error or success)
- even if we really absolutely need a helper, just introduce a regular function but do not modify the gcpClient object
- can't we migrate each test instead of keeping this yet another AWS sdk v2 lookalike api?
SylvainSenechal
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.
Don't forget to use real arsenal version in package json
Just one comment to address about the gcpClient.listObjects, maybe can consider a utility function at least, or even better keep it in Arsenal
b7e25d5 to
bd529bb
Compare
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.
in addition to the comments overall, there is something weird in these test: we use GCP client from arsenal, yet at the same time there is a re-implementation of GCP without the sdk (c.f. makeGcpRequest, retryGcpRequest...)
this should probably be changed, to do things consistently: either with GCP client (to maximize code reuse), or with a custom implementation, or directly with the GCP sdk (if we want to ensure the test code is independent form the production code, and failure would thus only happen due to change in production code)
| gcpClient.listObjects = (params, callback) => { | ||
| const command = new ListObjectsCommand(params); | ||
| return gcpClient.send(command) | ||
| .then(data => callback(null, data)) | ||
| .catch(err => { | ||
| if ( err.statusCode === undefined) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| err.statusCode = err.$metadata.httpStatusCode; | ||
| } | ||
| return callback(err); | ||
| }); | ||
| }; | ||
|
|
||
| gcpClient.getBucket = (params, callback) => | ||
| gcpClient.headBucket(params, (err, res) => { | ||
| if (err) { | ||
| if (err.statusCode === undefined) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| err.statusCode = err.$metadata.httpStatusCode; | ||
| } | ||
| if (err.$metadata && err.$metadata.httpStatusCode === 404) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| err.name = 'NoSuchBucket'; | ||
| } | ||
| return callback(err); | ||
| } | ||
| return callback(null, res); | ||
| }); | ||
|
|
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 keep these "legacy" functions, and not migrate the actual tests to the new SDK api?
since this PR is focused, it does not seem like a large change...
| config = getRealAwsConfig(credentialOne); | ||
| gcpClient = new GCP(config); | ||
| gcpClient.listObjects = (params, callback) => { | ||
| const command = new ListObjectsCommand(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.
same as above, should migrate each test? (and avoid duplicating this piece of code)
c3be2d7 to
9c34aa1
Compare
| process.stdout.write(`err in creating bucket ${err}\n`); | ||
| } | ||
| return done(err); | ||
| const command = new CreateBucketCommand({ |
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 used to be a gcpRequestRetry() : no problem to stop using it, but don't we need to setup retry somehow? or is it useless and handled in the client now?
| before(done => { | ||
| gcpRequestRetry({ | ||
| method: 'PUT', | ||
| bucket: bucketName, | ||
| authCredentials: config.credentials, | ||
| }, 0, err => { | ||
| if (err) { | ||
| process.stdout.write(`err in creating bucket ${err}\n`); | ||
| } | ||
| return done(err); | ||
| const command = new CreateBucketCommand({ | ||
| Bucket: bucketName, | ||
| }); | ||
| gcpClient.send(command) | ||
| .then(() => done()) | ||
| .catch(err => { | ||
| process.stdout.write(`err in creating bucket ${err}\n`); | ||
| return done(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.
before(async () => {
const command = new CreateBucketCommand({Collapse commentComment on line
Bucket: bucketName,
});
await gcpClient.send(command);
});
| after(done => { | ||
| gcpRequestRetry({ | ||
| method: 'DELETE', | ||
| bucket: bucketName, | ||
| authCredentials: config.credentials, | ||
| }, 0, err => { | ||
| if (err) { | ||
| process.stdout.write(`err in deleting bucket ${err}\n`); | ||
| } | ||
| return done(err); | ||
| const command = new DeleteBucketCommand({ | ||
| Bucket: bucketName, | ||
| }); | ||
| gcpClient.send(command) | ||
| .then(() => done()) | ||
| .catch(err => { | ||
| process.stdout.write(`err in deleting bucket ${err}\n`); | ||
| return done(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.
after(async () => {
const command = new DeleteBucketCommand({
Bucket: bucketName,
});
await gcpClient.send(command)
});
| const config = getRealAwsConfig(credentialOne); | ||
| const gcpClient = new GCP(config); | ||
|
|
||
| beforeEach(function beforeFn(done) { |
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.
make function async and use await when sending command (same for afterEach)
| it('should verify bucket versioning is enabled', function testFn(done) { | ||
| return async.waterfall([ | ||
| next => makeGcpRequest({ | ||
| method: 'PUT', | ||
| bucket: this.test.bucketName, | ||
| authCredentials: config.credentials, | ||
| queryObj: { versioning: '' }, | ||
| requestBody: xmlEnable, | ||
| }, err => { | ||
| if (err) { | ||
| process.stdout.write(`err in setting versioning ${err.code}`); | ||
| } | ||
| return next(err); | ||
| }), | ||
| // Enable versioning using the official SDK client | ||
| next => { | ||
| gcpClient.getBucketVersioning({ | ||
| const command = new PutBucketVersioningCommand({ | ||
| Bucket: this.test.bucketName, | ||
| }, (err, res) => { | ||
| assert.equal(err, null, | ||
| `Expected success, but got err ${err}`); | ||
| assert.deepStrictEqual(res.Status, verEnabledObj); | ||
| return next(); | ||
| VersioningConfiguration: { Status: 'Enabled' }, | ||
| }); | ||
| return gcpClient.send(command) | ||
| .then(() => next()) | ||
| .catch(err => next(err)); | ||
| }, | ||
| // Verify using GetBucketVersioningCommand | ||
| next => { | ||
| const command = new GetBucketVersioningCommand({ | ||
| Bucket: this.test.bucketName, | ||
| }); | ||
| return gcpClient.send(command) | ||
| .then(res => { | ||
| assert.deepStrictEqual(res.Status, verEnabledObj); | ||
| return next(); | ||
| }) | ||
| .catch(err => next(err)); | ||
| }, | ||
| ], err => done(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.
almost every line is changed, best use async and shorten the test:
it('should verify bucket versioning is enabled', async () => {
await gcpClient.send(new PutBucketVersioningCommand({
Bucket: this.test.bucketName,
VersioningConfiguration: { Status: 'Enabled' },
}));
const res = gcpClient.send(new GetBucketVersioningCommand({
Bucket: this.test.bucketName,
}));
assert.deepStrictEqual(res.Status, verEnabledObj);
});
| }, | ||
| err => { | ||
| if (err && (err.name === 'SlowDown' | ||
| || err.$metadata?.httpStatusCode === 429) |
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.
bad indent, this is not at the same indent level as 'err' (nested expression)
| || err.$metadata?.httpStatusCode === 429) | |
| || err.$metadata?.httpStatusCode === 429) |
or better yet just keep it on previous line, the line does not seem that long?
| function createBucket(attempt) { | ||
| const cmd = new CreateBucketCommand({ Bucket: bucketName }); | ||
| gcpClient.send(cmd) | ||
| .then(() => { | ||
| bucketCreated = true; | ||
| return done(); | ||
| }) | ||
| .catch(err => { | ||
| if ((err.name === 'SlowDown' | ||
| || err.$metadata?.httpStatusCode === 429) | ||
| && attempt < maxAttempts - 1) { | ||
| const delay = Math.pow(2, attempt) * 1000; | ||
| process.stdout.write( | ||
| `SlowDown creating bucket, retrying in ${delay}ms (attempt ${attempt + 1})\n`); | ||
| return setTimeout(() => createBucket(attempt + 1), delay); | ||
| } | ||
| process.stdout.write(`err in creating bucket ${err}`); | ||
| return done(err); | ||
| }); | ||
| } | ||
|
|
||
| createBucket(0); |
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.
can't this be written much more cleanly with async.retry or even just modifying the gcpRequestRetry to take both a command and client and repeatedly send the command if needed... (that function would not really be specific to gcp, but provide a good separation between retry logic and the test)
|
|
||
| const genUniqID = () => uuidv4().replace(/-/g, ''); | ||
|
|
||
| function gcpRequestRetry(params, retry, callback) { |
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 still keep the retry logic isolated in some helper, instead of re-implementing directly in the tests
- helper may need to support both async & callbacks (depending on where it is used)
async function gcpRetry(gcpClient, cmd, retry, cb) {
if (cb) {
return callbackify(() => gcpRetry(gcpClient, cmd, retry))(cb);
}
[...]
return await gcpCient.send(cmd)
}
| return callback(null, res); | ||
| }); | ||
| } | ||
|
|
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.
if we really need helper (listObjects, ...) they should be added here
package.json
Outdated
| "@hapi/joi": "^17.1.1", | ||
| "@smithy/node-http-handler": "^3.0.0", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#8.3.0-preview.1", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#aed443f02efebeb3f5c2e74786baadb5c19eaded", |
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 be updated with arsenal 8.3.0 now that it is released
2e581a8 to
cfe0d6a
Compare
cfe0d6a to
884e1da
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
This commit removes unused functions and updates object tests accordingly. These changes help reduce the dependency on gcpService only used functions in cloudserver tests, and also enables us to remove wrappers around GCP SDK calls, making the tests more straightforward and easier to maintain. Issue: CLDSRV-826
Issue: CLDSRV-826
f8e33fe to
671e7c6
Compare
DarkIsDude
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.
Not a lot to say, code is clean with some async/await, congrats 🙏
| async.mapLimit( | ||
| createdObjects, | ||
| 10, | ||
| (object, moveOn) => { |
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: async module supports async functions, so the same can be written with async/await:
async.mapLimit(
createdObjects,
10,
async (object) => await gcClient.send(new PutObjectCommand(...)),
callback,
);
| const cmd = typeof makeCommand === 'function' ? | ||
| makeCommand() : makeCommand; |
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 support a function? There does not seem to be any case where the command is dynamically built.
can't we always receive the actual command to send, and just use it?
(and if we really needed to use a function, no point supporting both a function and a command)
| before(async () => { | ||
| await gcpRetry( | ||
| gcpClient, | ||
| () => new CreateBucketCommand({ |
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.
what is the point of passing a function instead of just the command object?
can't we keep things simple?
|
|
||
| describe('without existing bucket', () => { | ||
| beforeEach(function beforeFn(done) { | ||
| beforeEach(function beforeFn() { |
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.
not aync and no callback : will never terminate?
| beforeEach(function beforeFn() { | |
| beforeEach(async function beforeFn() { |
| } catch (err) { | ||
| process.stdout | ||
| .write(`err deleting bucket: ${err.code}\n`); | ||
| } |
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.
the log is not needed (handled by Jest already), and more importantly this catch actually "eats" the error : so failure will not cause the test to fail (as it did before)
| } catch (err) { | |
| process.stdout | |
| .write(`err deleting bucket: ${err.code}\n`); | |
| } | |
| await gcpClient.send(cmd); |
| Bucket: bucketName, | ||
| Key: this.currentTest.key, | ||
| }); | ||
| gcpClient.send(cmd) |
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.
await
| Bucket: bucketName, | ||
| Key: this.currentTest.key, | ||
| }); | ||
| gcpClient.send(cmd) |
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.
await
| gcpRetry( | ||
| gcpClient, | ||
| () => new DeleteBucketCommand({ Bucket: bucketName }), | ||
| null, | ||
| err => { | ||
| if (err) { | ||
| process.stdout.write(`err in deleting bucket ${err}`); | ||
| } | ||
| return done(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.
| gcpRetry( | |
| gcpClient, | |
| () => new DeleteBucketCommand({ Bucket: bucketName }), | |
| null, | |
| err => { | |
| if (err) { | |
| process.stdout.write(`err in deleting bucket ${err}`); | |
| } | |
| return done(err); | |
| }, | |
| ); | |
| after(async () => { | |
| await gcpRetry( | |
| gcpClient, | |
| new DeleteBucketCommand({ Bucket: bucketName }), | |
| ); |
| (bucket, next) => gcpRetry( | ||
| gcpClient, | ||
| () => new CreateBucketCommand({ Bucket: bucket.Name }), | ||
| null, | ||
| next, | ||
| ), |
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.
| (bucket, next) => gcpRetry( | |
| gcpClient, | |
| () => new CreateBucketCommand({ Bucket: bucket.Name }), | |
| null, | |
| next, | |
| ), | |
| async (bucket) => await gcpRetry( | |
| gcpClient, | |
| () => new CreateBucketCommand({ Bucket: bucket.Name }), | |
| ), |
| '</CreateBucketConfiguration>'; | ||
| } | ||
|
|
||
| function listBucketObjects(gcpClient, params, 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.
do we really need this helper?
it is really just sending the command, we can migrate the 2 places where it is called, like everywhere else...
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 change was made based on this comment here : #6044 (comment) to avoid having to redefine the functions everytime. switching back to the commands on the tests level.
fix awaiting on before/after
fix awaiting on before/after
fix awaiting on before/after
fix awaiting on before/after
fix awaiting on before/after on completeMPU
fix awaiting on before/after on completeMPU
fix awaiting on before/after on initiateMPU
fix awaiting on before/after on initiateMPU
fix awaiting on before/after on initiateMPU
fix awaiting on before/after on initiateMPU
Issue: CLDSRV-826