Seed zero-value quota metric on bucket create / quota enable#6221
Seed zero-value quota metric on bucket create / quota enable#6221delthas wants to merge 5 commits into
Conversation
Hello delthas,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:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| "@opentelemetry/instrumentation-mongodb": "~0.69.0", | ||
| "@smithy/node-http-handler": "^3.0.0", | ||
| "arsenal": "git+https://github.com/scality/arsenal#8.5.4", | ||
| "arsenal": "git+https://github.com/scality/arsenal#05db0fa2268958fac066204ad2a48d2e083311dd", |
There was a problem hiding this comment.
Arsenal is pinned to a commit hash instead of a release tag. Per repo conventions, git-based dependencies must be pinned to a tag (e.g. 8.5.x).
The PR description mentions this is temporary pending ARSN-610 — please ensure this is updated to a released tag before merging.
| "arsenal": "git+https://github.com/scality/arsenal#05db0fa2268958fac066204ad2a48d2e083311dd", | |
| "arsenal": "git+https://github.com/scality/arsenal#8.5.5", |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 2 files with indirect coverage changes @@ Coverage Diff @@
## development/9.4 #6221 +/- ##
===================================================
+ Coverage 85.99% 86.03% +0.03%
===================================================
Files 212 212
Lines 14415 14455 +40
===================================================
+ Hits 12396 12436 +40
Misses 2019 2019
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
delthas
force-pushed
the
improvement/CLDSRV-949/seed-empty-bucket-quota-metric
branch
from
c463226 to
64c60e4
Compare
| if (err) { | ||
| return callback(err); | ||
| } | ||
| return callback(null, data); | ||
| }); | ||
| }, | ||
| }, | ||
| }, | ||
| // Function to run upon finishing both parallel requests | ||
| (err, results) => { | ||
| if (err) { | ||
| return cb(err); |
| } | ||
| const existingBucketMD = results.getAnyExistingBucketInfo; | ||
| if (existingBucketMD instanceof BucketInfo && | ||
| existingBucketMD.getOwner() !== canonicalID && | ||
| !isServiceAccount(canonicalID)) { | ||
| // return existingBucketMD to collect cors headers | ||
| return cb(errors.BucketAlreadyExists, existingBucketMD); | ||
| } | ||
| const newBucketMD = results.prepareNewBucketMD; | ||
| if (existingBucketMD === 'NoBucketYet') { | ||
| const bucketSseConfig = parseBucketEncryptionHeaders(headers); | ||
| // Function to run upon finishing both parallel requests |
| return bucketUpdateQuota(authInfo, request, log) | ||
| .then(corsHeaders => callback(null, corsHeaders)) |
delthas
force-pushed
the
improvement/CLDSRV-949/seed-empty-bucket-quota-metric
branch
2 times, most recently
from
7930f2c to
815f61c
Compare
delthas
force-pushed
the
improvement/CLDSRV-949/seed-empty-bucket-quota-metric
branch
3 times, most recently
from
7d8d376 to
049c059
Compare
Isolates prettier reformatting of files that were already prettier-dirty on development/9.4, so the functional change commits that follow stay prettier-clean. Issue: CLDSRV-949
delthas
force-pushed
the
improvement/CLDSRV-949/seed-empty-bucket-quota-metric
branch
from
049c059 to
b0c3fc9
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
delthas
force-pushed
the
improvement/CLDSRV-949/seed-empty-bucket-quota-metric
branch
from
b0c3fc9 to
7e7a71a
Compare
| "@opentelemetry/instrumentation-mongodb": "~0.69.0", | ||
| "@smithy/node-http-handler": "^3.0.0", | ||
| "arsenal": "git+https://github.com/scality/arsenal#8.5.6", | ||
| "arsenal": "git+https://github.com/scality/arsenal#43a1d0577dffd6e77a712ab813abaff57ace43ae", |
There was a problem hiding this comment.
Yes, known and flagged by Claude already, keeping open until the Arsenal part is merged
| return metadata.updateBucket(bucketName, bucket, log, callback); | ||
| } | ||
|
|
||
| function seedBucketQuotaCapacity(bucket, log, done) { |
There was a problem hiding this comment.
This function is a new one and must use async/await. We agreed to don't do it for prettier one, but this one is a new one 🙏
| return callback(err); | ||
| } | ||
| return removeTransientOrDeletedLabel(bucket, log, callback); | ||
| return removeTransientOrDeletedLabel(bucket, log, labelErr => { |
There was a problem hiding this comment.
Same here, you touch the function, it's not only a formatting 🙏
| } | ||
| return metadata.initializeBucketCapacity(bucket.getName(), bucket.getCreationDate(), log, err => { | ||
| if (err) { | ||
| log.error('error seeding bucket quota capacity metric', { error: err }); |
There was a problem hiding this comment.
Why we just log and don't propagate it ?
| if (labelErr) { | ||
| return callback(labelErr); | ||
| } | ||
| return seedBucketQuotaCapacity(bucket, log, () => callback(null)); |
There was a problem hiding this comment.
Why callback(null) ? Why not callback() ?
| undefined, undefined, undefined, undefined, undefined, undefined, undefined, undefined, undefined, isNFSEnabled, | ||
| undefined, undefined, objectLockEnabled); | ||
| const objectLockEnabled = headerObjectLock && headerObjectLock.toLowerCase() === 'true'; | ||
| const bucket = new BucketInfo( |
There was a problem hiding this comment.
😂 this is where you know that a class is too large 😂
| } | ||
| const bucketName = bucket.getName(); | ||
| try { | ||
| const list = await promisify(metadata.listObject.bind(metadata))( |
There was a problem hiding this comment.
To avoid promisify always, you can do promise.custom. See the confluence page. does it make sense here ?
| } | ||
| await promisify(metadata.initializeBucketCapacity.bind(metadata))(bucketName, bucket.getCreationDate(), log); | ||
| } catch (err) { | ||
| log.error('error seeding bucket quota capacity metric', { error: err }); |
There was a problem hiding this comment.
Why just a log and not a propagate ?
There was a problem hiding this comment.
warn, not error: IMO the failure is best-effort and self-heals at the next count-items run; nothing for a human to do. Also worth a one-line comment stating why it's non-fatal
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following reviewers are expecting changes from the author, or must review again: |
| const quotaValue = validateBucketQuotaProperty(requestBody); | ||
| bucket.setQuota(quotaValue); | ||
| await promisify(metadata.updateBucket.bind(metadata))(bucket.getName(), bucket, log); | ||
| await seedEmptyBucketCapacity(bucket, log); |
There was a problem hiding this comment.
why seed after updateBucket persists the quota? Done in this order there's still a window where the quota is set but no metric exists (fail-open, the exact bug we're fixing here). Listing + seeding first, then enabling the quota, closes the window entirely IMO and the emptiness check can't be invalidated by the quota becoming visible in between
| }, | ||
| log, | ||
| ); | ||
| const isEmpty = |
There was a problem hiding this comment.
a DelimiterVersions listing won't see in-progress MPU parts (shadow bucket), but those hold real storage. If count-items includes MPU parts in the bucket metric, seeding 0 here under-enforces for a bucket that only has uncommitted uploads , the exact case the description says must never happen ("never default to zero for a bucket that may hold uncounted data"). Does count-items count MPU parts? and if yes, shouldn't the emptiness check also probe the MPU shadow bucket?
| const isEmpty = | |
| const isEmpty = (list.Versions?.length ?? 0) + (list.DeleteMarkers?.length ?? 0) === 0; |
| if (labelErr) { | ||
| return callback(labelErr); | ||
| } | ||
| return seedBucketQuotaCapacity(bucketMD, log, () => callback(null)); |
There was a problem hiding this comment.
seeding also fires on the transient/deleted-flag cleanup path (re-creating a bucket left in transient state), which the PR description doesn't mention and no test covers, codecov flags 5 missed lines in this file. Is the creationDate used here guaranteed to be the new incarnation's metastore date that _evaluateQuotas will query? can we add a test for this path (or explain why it can't be hit with quotas enabled).
| bucketPut(authInfo, testRequest, log, err => { | ||
| assert.ifError(err); | ||
| assert(initStub.calledOnce, 'expected initializeBucketCapacity to be called once'); | ||
| assert.strictEqual(initStub.firstCall.args[0], bucketName); |
There was a problem hiding this comment.
the exact-match creationDate keying is the load-bearing part of the design; the test should also pin it:
| assert.strictEqual(initStub.firstCall.args[0], bucketName); | |
| assert.strictEqual(initStub.firstCall.args[0], bucketName); | |
| assert.strictEqual(initStub.firstCall.args[1], md.getCreationDate()); |
| .stub(metadata, 'initializeBucketCapacity') | ||
| .callsFake((name, creationDate, l, cb) => process.nextTick(() => cb(errors.InternalError))); | ||
| return bucketUpdateQuota(authInfo, updateQuotaRequest, log, err => { | ||
| assert.ifError(err); |
There was a problem hiding this comment.
the test only asserts no error we should assert md.getQuota() === 1000 after the call, otherwise "still update the quota" isn't actually verified.
| return; | ||
| } | ||
| await promisify(metadata.initializeBucketCapacity.bind(metadata))(bucketName, bucket.getCreationDate(), log); | ||
| } catch (err) { |
There was a problem hiding this comment.
can we add the test case where listObject errors (seed skipped, request still succeeds).
Temporary pin to the ARSN-610 branch commit so the quota metric seeding can resolve MetadataWrapper.initializeBucketCapacity. To be replaced with a released arsenal 8.5 tag once ARSN-610 is merged. Issue: CLDSRV-949
Seed a zero-value bucket metric document in __infostore when the bucket is known to be empty, so bucket quota checks are enforceable immediately instead of waiting up to 24h for the periodic count-items job (ARTESCA-17063): - on createBucket, the bucket is empty by definition, so seed it; - on bucketUpdateQuota, seed only when the bucket is verifiably empty (a maxKeys:1 DelimiterVersions listing), never defaulting to zero for a bucket that may hold uncounted data. Both paths are gated on config.isQuotaEnabled() and are best-effort: a seeding failure is logged and never fails the request. The write uses the new arsenal MetadataWrapper.initializeBucketCapacity (idempotent $setOnInsert), keyed by the bucket's metastore creationDate to match the enforcement lookup. Issue: CLDSRV-949
Migrate the callback-style bucketUpdateQuota handler to async/await using the dual callback+async trampoline, to preempt the CodeQL "callback-style function" alert triggered by modifying it. The callback contract and behavior are unchanged. Callback-based dependencies are promisified at call time so the wrapper's method identity (and test stubs) are honored. Issue: CLDSRV-949
delthas
force-pushed
the
improvement/CLDSRV-949/seed-empty-bucket-quota-metric
branch
from
7e7a71a to
3bd38a8
Compare
- migrate seedBucketQuotaCapacity / freshStartCreateBucket / cleanUpBucket to async/await (callback trampoline keeps existing callers working) - bucketUpdateQuota: seed the zero metric before persisting the quota to close the fail-open window, and probe the MPU shadow bucket so a bucket with only in-progress uploads is never seeded to zero - log seeding failures at warn (best-effort, self-heals next count-items) - cache promisified metadata methods at module level - tests: transient/deleted cleanup path, exact creationDate match, listing-error and in-progress-MPU cases Issue: CLDSRV-949
| return freshStartCreateBucket(bucket, canonicalID, log) | ||
| .then(() => callback(null)) |
| return cleanUpBucket(bucketMD, canonicalID, log) | ||
| .then(() => callback(null)) |
| return bucketUpdateQuota(authInfo, updateQuotaRequest, log, err => { | ||
| assert.ifError(err); | ||
| assert(initStub.calledOnce, 'expected seeding for an empty bucket'); | ||
| assert.strictEqual(initStub.firstCall.args[0], bucketName); |
There was a problem hiding this comment.
Missing creationDate assertion. The bucketPut test was updated to verify initStub.firstCall.args[1] === md.getCreationDate() (the load-bearing keying for quota lookups), but this test only checks args[0]. Add the same verification here:
| assert.strictEqual(initStub.firstCall.args[0], bucketName); | |
| assert.strictEqual(initStub.firstCall.args[0], bucketName); | |
| return metadata.getBucket(bucketName, log, (getErr, md) => { | |
| assert.ifError(getErr); | |
| assert.strictEqual( | |
| initStub.firstCall.args[1], | |
| md.getCreationDate(), | |
| 'must seed with the metastore creationDate used by quota lookups', | |
| ); | |
| done(); | |
| }); |
5 participants
Context
Bucket/account quota enforcement reads utilization metrics from Scuba, which serves the
__infostorecollection. Those documents are written today only by the s3utilscount-itemscron (every 6–24h). A freshly created bucket — or a bucket that has a quota enabled while empty — has no metric document until the next cron run, sovalidateQuotas→_evaluateQuotasgetsNoSuchEntityfrom Scuba and fails open: the quota is not enforced for up to 24h (ARTESCA-17063).Change
Seed a zero-value bucket metric document in
__infostoreat the moments CloudServer knows the bucket is empty, so the quota is enforceable immediately:createBucket— the bucket is empty by definition, so seed unconditionally.bucketUpdateQuota— seed only when the bucket is verifiably empty (amaxKeys:1DelimiterVersionslisting). Never default to zero for a bucket that may hold uncounted data (that would silently under-enforce).Both paths:
config.isQuotaEnabled();MetadataWrapper.initializeBucketCapacity(idempotent$setOnInsert, so an existing count-items document is never overwritten), keyed by the bucket's metastorecreationDate— exactly what_evaluateQuotasqueries with, so the create-window lookup is an exact match.Account-level metrics are intentionally out of scope: a brand-new bucket's account may hold data in other, not-yet-counted buckets, so seeding
account_<id>=0would under-report. Empty-account coverage stays with count-items (S3UTILS-224).Dependency
Requires ARSN-610 (arsenal#2666). The first commit here temporarily pins arsenal to that branch commit so the new method resolves; it should be replaced with a released arsenal 8.5 tag once ARSN-610 merges.
Tests
New unit tests:
bucketUpdateQuotaseeds on an empty bucket, does not seed on a non-empty bucket, still succeeds when seeding errors, and does not seed when quotas are disabled;bucketPutseeds on creation, stays non-fatal on seed error, and does not seed when quotas are disabled.Issue: CLDSRV-949