Seed zero-value quota metric on bucket create / quota enable - #6221
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}
❌ 1 Tests Failed:
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
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
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: |
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
b060edd to
96ee768
Compare
|
The MR is ready for me, if all OK feel free to /approve / merge without me. |
96ee768 to
5fd7a50
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
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
5fd7a50 to
79c7731
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: |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
|
/approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
This pull request did not target the following hotfix branch(es) so they
Please check the status of the associated issue CLDSRV-949. Goodbye delthas. The following options are set: approve |
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