Seed zero-value quota metric on bucket create / quota enable#6221
Open
delthas wants to merge 5 commits into
Open
Seed zero-value quota metric on bucket create / quota enable#6221delthas wants to merge 5 commits into
delthas wants to merge 5 commits into
Conversation
Contributor
Hello delthas,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Contributor
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.
Suggested change
| "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 3 files with indirect coverage changes @@ Coverage Diff @@
## development/9.4 #6221 +/- ##
===================================================
- Coverage 85.99% 85.96% -0.03%
===================================================
Files 212 212
Lines 14415 14453 +38
===================================================
+ Hits 12396 12425 +29
- Misses 2019 2028 +9
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
Comment on lines
+270
to
-242
| if (err) { | ||
| return callback(err); | ||
| } | ||
| return callback(null, data); | ||
| }); | ||
| }, | ||
| }, | ||
| }, | ||
| // Function to run upon finishing both parallel requests | ||
| (err, results) => { | ||
| if (err) { | ||
| return cb(err); |
Comment on lines
+78
to
+79
| 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
Contributor
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
DarkIsDude
requested changes
Jul 8, 2026
| "@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", |
Contributor
Author
There was a problem hiding this comment.
Yes, known and flagged by Claude already, keeping open until the Arsenal part is merged
| } | ||
| const bucketName = bucket.getName(); | ||
| try { | ||
| const list = await promisify(metadata.listObject.bind(metadata))( |
Contributor
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 ?
Contributor
Author
There was a problem hiding this comment.
promisify.custom is really for callbacks whose shape isn't the standard (err, result) (multiple results / a custom resolve object) — these metadata methods are standard-shaped, so it wouldn't add anything, and attaching it would mean mutating arsenal's shared MetadataWrapper methods. What the comment is really after — not calling promisify() on every invocation — I did by declaring the promisified wrappers once at module scope (arrow form so this is preserved and sinon stubs still resolve), matching the existing convention in lib/routes/veeam/utils.js (const getBucket = promisify((...args) => metadata.getBucket(...args))). Happy to switch to promisify.custom on the wrapper if you'd still prefer it.
Contributor
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: |
benzekrimaha
reviewed
Jul 8, 2026
benzekrimaha
reviewed
Jul 8, 2026
benzekrimaha
reviewed
Jul 8, 2026
benzekrimaha
reviewed
Jul 8, 2026
benzekrimaha
reviewed
Jul 8, 2026
benzekrimaha
reviewed
Jul 8, 2026
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
Comment on lines
+109
to
+110
| return freshStartCreateBucket(bucket, canonicalID, log) | ||
| .then(() => callback(null)) |
Comment on lines
+136
to
+137
| 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:
Suggested change
| 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(); | |
| }); |
- 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
delthas
force-pushed
the
improvement/CLDSRV-949/seed-empty-bucket-quota-metric
branch
from
9e549e4 to
1ba9440
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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