-
Notifications
You must be signed in to change notification settings - Fork 24
Properly get location in lifecycle metrics #2736
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.5
Are you sure you want to change the base?
Changes from all commits
45ff195
23dcc78
764f982
5f43490
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,6 +59,19 @@ const MAX_RETRIES = 4; | |
| // parallel tasks, so the total delay of retries should about 1m30s. | ||
| const MAX_RETRIES_TOTAL = CONCURRENCY_DEFAULT * MAX_RETRIES * 10; | ||
|
|
||
| function attachArchiveInfoHeader(command) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: we should use cloudserverclient instead of (manually) adding such header → please create followup |
||
| command.middlewareStack.add(next => async args => { | ||
| if (args.request && args.request.headers) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| args.request.headers['x-amz-scal-archive-info'] = 'true'; | ||
| } | ||
| return next(args); | ||
| }, { | ||
| step: 'build', | ||
| name: 'attachArchiveInfoHeader', | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * compare 2 version by their stale dates returning: | ||
| * - LT (-1) if v1 is less than v2 | ||
|
|
@@ -1588,12 +1601,20 @@ class LifecycleTask extends BackbeatTask { | |
| params.VersionId = obj.VersionId; | ||
| } | ||
| const command = new HeadObjectCommand(params); | ||
| attachArchiveInfoHeader(command); | ||
| attachReqUids(command, log.getSerializedUids()); | ||
| return this.s3target.send(command) | ||
| .then(data => { | ||
| LifecycleMetrics.onS3Request(log, 'headObject', 'bucket', null); | ||
| const object = Object.assign({}, obj, | ||
| { LastModified: data.LastModified }); | ||
| { | ||
| LastModified: data.LastModified, | ||
| // With the archive-info header, CloudServer always | ||
| // returns the real storage class (never STANDARD): | ||
| // the preserved cold class for cold/restored objects, | ||
| // the data-store name otherwise. | ||
| StorageClass: data.StorageClass, | ||
| }); | ||
|
|
||
| // There is an order of importance in cases of conflicts | ||
| // Expiration and NoncurrentVersionExpiration should be priority | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| const assert = require('assert'); | ||
|
|
||
| const bucketProcessorPolicy = require('../../../extensions/lifecycle/bucketProcessor/policy.json'); | ||
|
|
||
| describe('LifecycleBucketProcessor policy', () => { | ||
| it('should allow archive info reads for lifecycle metric location resolution', () => { | ||
| const actions = bucketProcessorPolicy.Statement | ||
| .find(statement => statement.Sid === 'LifecycleExpirationBucketProcessor') | ||
| .Action; | ||
|
|
||
| assert(actions.includes('scality:GetObjectArchiveInfo')); | ||
| assert(!actions.includes('s3:GetBucketLocation')); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,8 @@ const LifecycleTask = require( | |
| '../../../extensions/lifecycle/tasks/LifecycleTask'); | ||
| const LifecycleTaskV2 = require( | ||
| '../../../extensions/lifecycle/tasks/LifecycleTaskV2'); | ||
| const ActionQueueEntry = require('../../../lib/models/ActionQueueEntry'); | ||
| const { LifecycleMetrics } = require('../../../extensions/lifecycle/LifecycleMetrics'); | ||
| const fakeLogger = require('../../utils/fakeLogger'); | ||
| const { withActiveSpan } = require('../../utils/withActiveSpan'); | ||
| const { timeOptions } = require('../../functional/lifecycle/configObjects'); | ||
|
|
@@ -2470,6 +2472,121 @@ describe('lifecycle task helper methods', () => { | |
| }); | ||
| }); | ||
| }); | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if these unit tests are enough: the issue was a correlation between different values at different layers (STANDARD or cold class in ListObject/HeadObject, hot class in GetMetadata, the way the kafka message's dataStoreName was computed...):
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I extended the coverage into a scenario matrix at the queue level: expiration of a hot object on the default location (listing says STANDARD, HeadObject returns the real location), a hot object on a non-default location, a cold object, and a restored cold object (cold class preserved in x-amz-storage-class). Each case asserts the archive-info middleware is attached to the HeadObject and that the queued entry's details.dataStoreName is the head-resolved storage class. The execution side (metric emitted from the entry location) is covered in LifecycleDeleteObjectTask.spec.js and LifecycleUpdateExpirationTask.spec.js, including the restored-object case. |
||
| describe('_sendObjectAction', () => { | ||
| it('should emit trigger metrics with the entry location', done => { | ||
| const lifecycleTask = new LifecycleTask(lp); | ||
| const sentEntries = []; | ||
| lifecycleTask.objectTasksTopic = 'object-topic'; | ||
| lifecycleTask.circuitBreakers = { | ||
| tripped: sinon.stub().returns(false), | ||
| }; | ||
| lifecycleTask.producer = { | ||
| sendToTopic: (topic, entries, cb) => { | ||
| sentEntries.push({ topic, entries }); | ||
| cb(); | ||
| }, | ||
| }; | ||
| const triggeredMetric = sinon.stub(LifecycleMetrics, 'onLifecycleTriggered'); | ||
|
|
||
| const entry = ActionQueueEntry.create('deleteObject') | ||
| .setAttribute('target.owner', 'test-owner') | ||
| .setAttribute('target.bucket', 'test-bucket') | ||
| .setAttribute('target.accountId', 'test-account') | ||
| .setAttribute('target.key', 'test-key') | ||
| .setAttribute('details.dataStoreName', 'us-east-1') | ||
| .setAttribute('transitionTime', Date.now() - HOUR); | ||
| lifecycleTask._sendObjectAction(entry, err => { | ||
| assert.ifError(err); | ||
| assert.strictEqual(entry.getAttribute('details.dataStoreName'), 'us-east-1'); | ||
| assert.strictEqual(lifecycleTask.circuitBreakers.tripped.firstCall.args[1], 'us-east-1'); | ||
| assert.strictEqual(triggeredMetric.firstCall.args[3], 'us-east-1'); | ||
| assert.strictEqual(sentEntries.length, 1); | ||
| done(); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('_compareObject location metrics', () => { | ||
| // With the x-amz-scal-archive-info request header, CloudServer's | ||
| // HeadObject always returns the real storage class: the preserved | ||
| // cold class for cold and restored objects, the data-store name | ||
| // otherwise (never the STANDARD placeholder the listing may carry). | ||
| [ | ||
| { | ||
| desc: 'hot object on the default location', | ||
| listedStorageClass: 'STANDARD', | ||
| headStorageClass: 'us-east-1', | ||
| }, | ||
| { | ||
| desc: 'hot object on a non-default location', | ||
| listedStorageClass: 'site-azure', | ||
| headStorageClass: 'site-azure', | ||
| }, | ||
| { | ||
| desc: 'cold object', | ||
| listedStorageClass: 'location-dmf-v1', | ||
| headStorageClass: 'location-dmf-v1', | ||
| }, | ||
| { | ||
| // restored objects keep the cold class in | ||
| // x-amz-storage-class even though the restored copy lives | ||
| // on a warm location | ||
| desc: 'restored cold object', | ||
| listedStorageClass: 'location-dmf-v1', | ||
| headStorageClass: 'location-dmf-v1', | ||
| }, | ||
| ].forEach(({ desc, listedStorageClass, headStorageClass }) => { | ||
| it(`should queue the expiration of a ${desc} with the ` + | ||
| 'archive-info storage class', done => { | ||
| class LifecycleTaskMock extends LifecycleTask { | ||
| _sendObjectAction(entry, cb) { | ||
| this.latestEntry = entry; | ||
| return cb(); | ||
| } | ||
| } | ||
|
|
||
| const lifecycleTask = new LifecycleTaskMock(lp); | ||
| lifecycleTask.s3target = { | ||
| send: sinon.stub().resolves({ | ||
| LastModified: new Date().toISOString(), | ||
| StorageClass: headStorageClass, | ||
| }), | ||
| }; | ||
|
|
||
| const bucketData = { | ||
| target: { | ||
| owner: 'test-owner', | ||
| bucket: 'test-bucket', | ||
| accountId: 'test-account', | ||
| }, | ||
| details: {}, | ||
| }; | ||
| const rules = { | ||
| Expiration: { | ||
| Date: new Date(Date.now() - DAY), | ||
| }, | ||
| }; | ||
| const listedObject = Object.assign({}, OBJECT, { | ||
| StorageClass: listedStorageClass, | ||
| }); | ||
|
|
||
| lifecycleTask._compareObject(bucketData, listedObject, rules, fakeLogger, err => { | ||
| assert.ifError(err); | ||
| assert.strictEqual(lifecycleTask.s3target.send.calledOnce, true); | ||
| const command = lifecycleTask.s3target.send.firstCall.args[0]; | ||
| assert(command.middlewareStack.identify() | ||
| .includes('attachArchiveInfoHeader - build')); | ||
| assert.strictEqual( | ||
| lifecycleTask.latestEntry.getAttribute('details.dataStoreName'), | ||
| headStorageClass | ||
| ); | ||
| done(); | ||
| }); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| }); | ||
|
|
||
| describe('LifecycleTask trace-context propagation', () => { | ||
|
|
||
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.
details.dataStoreNamewas already added to entries before this PR right?→ If not, we should keep a fallback (
entry.getAttribute('details.dataStoreName') || this.objectMD?.dataStoreName) in case the new backbeat processes messages generated by previous version