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 4 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}
![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.01% +0.02%
===================================================
Files 212 212
Lines 14415 14451 +36
===================================================
+ Hits 12396 12430 +34
- Misses 2019 2021 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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); |
7930f2c to
815f61c
Compare
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
049c059 to
b0c3fc9
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
b0c3fc9 to
7e7a71a
Compare
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: |
7e7a71a to
3bd38a8
Compare
a3571b1 to
68a1f28
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
68a1f28 to
e360f5b
Compare
|
|
||
| async function freshStartCreateBucket(bucket, canonicalID, log, callback) { | ||
| if (callback) { | ||
| return freshStartCreateBucket(bucket, canonicalID, log).then(() => callback(null), callback); |
| function cleanUpBucket(bucketMD, canonicalID, log, callback) { | ||
| async function cleanUpBucket(bucketMD, canonicalID, log, callback) { | ||
| if (callback) { | ||
| return cleanUpBucket(bucketMD, canonicalID, log).then(() => callback(null), callback); |
| return bucketUpdateQuota(authInfo, request, log).then( | ||
| corsHeaders => callback(null, corsHeaders), | ||
| err => callback(err, err.code, err.additionalResHeaders), | ||
| ); |
There was a problem hiding this comment.
I think that the current ordering still allows usage to be silently under-reported =>
- the quota-enable path has a check-then-act race between the empty listing, metric initialization, and quota persistence;
- bucket creation clears the transient/deleted flag before initializing the metric, exposing a similar race;
The callback trampolines also leave the new CodeQL.then()findings open; async helpers should be promise-only, with callback adaptation at their callers IMO
| } | ||
| const bucketName = bucket.getName(); | ||
| try { | ||
| const list = await listObjects(bucketName, { maxKeys: 1, listingType: 'DelimiterVersions' }, log); |
There was a problem hiding this comment.
this is check-then-act: an object can be written after this empty listing and before the quota is persisted. Since quota tracking is still disabled, that object is not reflected in Scuba, while $setOnInsert leaves the zero metric in place. This silently under-enforces the newly enabled quota until count-items runs — the same problem this PR is meant to fix. How do we prevent this race?
There was a problem hiding this comment.
You're right that it's check-then-act, and there's no atomic fix here without a bucket lock, since the bucket is already live (unlike the creation path, which has the transient-flag gate we can seed behind).
We're accepting this deliberately: the seed is a best-effort floor, not an authoritative count — count-items is the source of truth and reconciles at its next run. The failure mode is one bucket briefly under-enforced until then, which is far narrower than the bug this PR fixes (Scuba's deep-health failing → quotas disabled cluster-wide, indefinitely). I've documented it inline at the seed call.
If you'd want a stronger guarantee it'd need a lock; otherwise I think accept-and-self-heal is the right tradeoff. Happy to discuss — leaving this open for your call.
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: |
e360f5b to
b060edd
Compare
Pull in MetadataWrapper.initializeBucketCapacity (ARSN-610) for the empty-bucket quota metric seeding. 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 bucketUpdateQuota and the bucket-creation seeding helpers (seedBucketQuotaCapacity, freshStartCreateBucket, cleanUpBucket) to async/await, using a callback trampoline so existing callers keep working. Seed the zero metric before persisting the quota to close the fail-open window, probe the MPU shadow bucket so a bucket with only in-progress uploads is not seeded to zero, log seeding failures as non-fatal warnings, and declare the promisified metadata helpers once at module scope. Issue: CLDSRV-949
b060edd to
96ee768
Compare
|
The MR is ready for me, if all OK feel free to /approve / merge without me. |
| }); | ||
| return callback(null, corsHeaders); | ||
| monitoring.promMetrics('PUT', bucketName, err.code, 'updateBucketQuota'); | ||
| err.additionalResHeaders = err.additionalResHeaders || corsHeaders; |
There was a problem hiding this comment.
arsenal errors coming from metadata are shared singletons (only customizeDescription() clones), so this pins the first request's CORS headers on the instance and replays them on every later failure with the same error, for any bucket/origin. Can we carry the headers out without mutating err (e.g. a small result carrier shared with the callback shim)?
ConflictThere is a conflict between your branch Please resolve the conflict on the feature branch ( git fetch && \
git checkout origin/improvement/CLDSRV-949/seed-empty-bucket-quota-metric && \
git merge origin/development/9.4Resolve merge conflicts and commit git push origin HEAD:improvement/CLDSRV-949/seed-empty-bucket-quota-metric |
| async function parseRequestBody(requestBody, contentType) { | ||
| if (contentType === 'application/xml') { | ||
| try { | ||
| return await promisify(parseString)(requestBody, { explicitArray: false }); |
There was a problem hiding this comment.
nit: promisify(parseString) is rebuilt on every XML request , we can hoist it to module scope next to the other promisified helpers.
| return removeTransientOrDeletedLabel(bucket, log, callback); | ||
| }); | ||
| }); | ||
| await createBucketMD(bucketName, bucket, log); |
There was a problem hiding this comment.
the old log.debug('error from metadata', ...) on createBucket failure is gone is that intentional? the caller still gets the error, just checking we don't lose the trace.
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