Seed zero-value metrics for empty buckets in count-items#378
Conversation
Hello delthas,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development/1 #378 +/- ##
=================================================
+ Coverage 45.47% 45.63% +0.15%
=================================================
Files 88 88
Lines 6529 6550 +21
Branches 1372 1380 +8
=================================================
+ Hits 2969 2989 +20
- Misses 3510 3511 +1
Partials 50 50 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
delthas
changed the title
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
delthas
force-pushed
the
improvement/S3UTILS-224/preseed-empty-bucket-metrics
branch
from
c954e09 to
ef5a150
Compare
delthas
changed the title
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
|
LGTM |
count-items only produced metric documents for buckets that hold objects. An empty bucket yielded no document in __infostore, which makes Scuba's deep health check fail and disables the quota service cluster-wide (ARTESCA-17063). Seed zero-value bucket, account and location entries in getObjectMDStats() when a bucket scan yields no objects. The bucket key is derived from the __usersbucket creation date, exactly like populated buckets (S3UTILS-131), so the seeded document matches the key quota lookups use; if that date is missing the bucket is skipped with a warning rather than written under an unmatchable key. Issue: S3UTILS-224
delthas
force-pushed
the
improvement/S3UTILS-224/preseed-empty-bucket-metrics
branch
from
ef5a150 to
6442f99
Compare
| const retResult = this._handleResults(collRes, isVer); | ||
| retResult.stalled = stalledCount; | ||
|
|
||
| // Empty buckets produce no metric document, which disables quotas |
There was a problem hiding this comment.
A 400 lines function are we are adding more logic, can we found something better ? I'm not sure the comment is really useful to understand the code ? Just explain the code. We can have context with git history if needed ?
| const retResult = this._handleResults(collRes, isVer); | ||
| retResult.stalled = stalledCount; | ||
|
|
||
| // Empty buckets produce no metric document, which disables quotas |
There was a problem hiding this comment.
this is re-implementing what _handleResults already does: every field is destructured with a 0n default, so an empty entry in collRes produces exactly the zero-value document (including the locations maps for bucket and account), there's no need to hand-build the shape we can just seed collRes before calling _handleResults
| } | ||
| } | ||
| } else { | ||
| log.warn('unable to seed zero metrics for empty bucket: no creation date in __usersbucket', { |
There was a problem hiding this comment.
this warn is not conditioned on the bucket being empty: any bucket missing its __usersbucket creation date will log "unable to seed zero metrics for empty bucket" on every count-items round, even when it holds millions of objects (for which seeding is irrelevant they already produce a document, just keyed _NaN).We should either gate it on the bucket actually being empty (Object.keys(collRes.bucket).length === 0), or reword; as written it will flood logs and mislead support
| const owner = bucketInfo.getOwner(); | ||
| if (owner) { | ||
| if (!retResult.dataMetrics.account[owner]) { | ||
| retResult.dataMetrics.account[owner] = zeroDataMetrics(true); | ||
| } | ||
| // so the inflight block below tags it with the owner | ||
| if (!accountBucket) { | ||
| accountBucket = owner; | ||
| } | ||
| } |
There was a problem hiding this comment.
can accountBucket ever be set here? Any processed entry creates the ${bucketName}_${date} bucket resource, so if we reach this block the cursor yielded nothing and accountBucket is necessarily undefined , the if (!accountBucket) check is dead. Same for if (!retResult.dataMetrics.account[owner]) => account entries only come from object entries, which would have created the bucket entry and skipped the block. And if (owner) ,can a BucketInfo really have no owner? If not I think that we can drop the guard; defensive checks on impossible states can hide the actual invariants
| // cluster-wide (ARTESCA-17063); seed zeros so every resource has one. | ||
| // Key off the __usersbucket creation date, like populated buckets (S3UTILS-131). | ||
| if (bucketCreationDate) { | ||
| const bucketResource = `${bucketName}_${new Date(bucketCreationDate).getTime()}`; |
There was a problem hiding this comment.
bucketKey => https://github.com/scality/s3utils/pull/378/changes#diff-3a778b361ddfba6106347ff0cb053bcbb1a84374fa0d7e6e6413e7c6fb6ca5cdR220 is already computed for inflights lookup; computing${bucketName}${date} a second time inline invites the two drifting apart, we should compute the unprefixed resource name once next to bucketKey and derive both.
| @@ -1152,7 +1152,7 @@ describe('S3UtilsMongoClient, tests', () => { | |||
|
|
|||
| const tests = [ | |||
There was a problem hiding this comment.
can we add a case for it (empty bucket, getUsersBucketCreationDate returning nothing → no seeded entry, warn emitted) ?
| // Empty buckets produce no metric document, which disables quotas | ||
| // cluster-wide (ARTESCA-17063); seed zeros so every resource has one. | ||
| // Key off the __usersbucket creation date, like populated buckets (S3UTILS-131). | ||
| if (bucketCreationDate) { |
There was a problem hiding this comment.
getObjectMDStats is also called with isTransient buckets (being deleted). Do we want to seed a zero document for those too, or should seeding be skipped so a bucket in deletion doesn't get a fresh __infostore doc? Probably harmless since the next run drops it, but worth stating in the PR description.
4 participants
count-items only produced metric documents for buckets that hold objects. An empty bucket yielded no document in
__infostore, so Scuba's deep health check (getAnyMetric) found nothing and returned 500, causing CloudServer to disable the quota service cluster-wide (ARTESCA-17063).This seeds zero-value entries for the empty bucket, its account and its location in
getObjectMDStats()when a bucket scan yields no objects, so every known resource always has a document in__infostore.The bucket key is derived from the
__usersbucketcreation date, exactly like populated buckets (S3UTILS-131), so the seeded document matches the key quota lookups use. If that date is missing, the bucket is skipped with a warning rather than written under a key that lookups could never match.Note: this addresses only the empty-bucket case. The new-bucket window (before the first count-items run) and the Scuba deep-health-check / alert behavior are tracked separately.
Issue: S3UTILS-224