Skip to content

Call config.isQuotaEnabled instead of testing the reference - #6253

Open
delthas wants to merge 2 commits into
development/9.4from
improvement/CLDSRV-972/call-isquotaenabled
Open

Call config.isQuotaEnabled instead of testing the reference#6253
delthas wants to merge 2 commits into
development/9.4from
improvement/CLDSRV-972/call-isquotaenabled

Conversation

@delthas

@delthas delthas commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

lib/utilities/monitoringHandler.js gated quota metrics on
config.isQuotaEnabled — the function reference, not a call — at two sites,
so both were always true.

The two bugs cancelled out. Because the registration gate was always true all
six quota metrics always existed, so the always-true gate in crrCacheToProm
never touched an unregistered gauge. Fixing only the registration site would
crash it, and would also crash the metric observation in the scuba wrapper,
which the Veeam capacity route reaches with no quota gating at all
(lib/routes/veeam/list.js, lib/routes/veeam/utils.js) — in a finally
block, so the TypeError would replace the original error. Every metric touch
point is therefore guarded to tolerate a metric that was never registered:
three in the scuba wrapper, three in quotaUtils.js.

This changes an operator-visible surface: the six s3_cloudserver_quota_*
metrics are now absent when quota is disabled, where previously they were
always registered. Anything scraping or alerting on them in a non-quota
deployment will see them disappear.

An alternative, recorded on the ticket, is to drop the dead conditional and
register unconditionally — preserving today's behaviour exactly with no
metric-surface change.

Issue: CLDSRV-972

@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.44444% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.36%. Comparing base (43f7fee) to head (fcefd5a).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
lib/utilities/monitoringHandler.js 93.75% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

Files with missing lines Coverage Δ
lib/api/apiUtils/quotas/quotaUtils.js 100.00% <100.00%> (ø)
lib/utilization/scuba/wrapper.js 100.00% <100.00%> (ø)
lib/utilities/monitoringHandler.js 88.77% <93.75%> (-2.05%) ⬇️
@@                                      Coverage Diff                                      @@
##           improvement/CLDSRV-971/quota-metrics-retrieval-code-label    #6253      +/-   ##
=============================================================================================
- Coverage                                                      86.38%   86.36%   -0.02%     
=============================================================================================
  Files                                                            212      212              
  Lines                                                          14567    14567              
=============================================================================================
- Hits                                                           12583    12581       -2     
- Misses                                                          1984     1986       +2     
Flag Coverage Δ
checksums-disabled-tests 35.29% <75.00%> (-0.05%) ⬇️
file-ft-tests 69.98% <77.77%> (-0.03%) ⬇️
file-ft-tests-null-compat 70.41% <77.77%> (-0.11%) ⬇️
kmip-ft-tests 28.04% <38.88%> (-0.07%) ⬇️
mongo-v0-ft-tests 71.08% <77.77%> (-0.02%) ⬇️
mongo-v1-ft-tests 71.01% <77.77%> (-0.08%) ⬇️
multiple-backend 36.04% <75.00%> (-0.05%) ⬇️
s3c-ft-tests-v0 65.01% <75.00%> (-0.05%) ⬇️
s3c-ft-tests-v0-null-compat 65.07% <75.00%> (-0.05%) ⬇️
s3c-ft-tests-v1 64.99% <75.00%> (-0.05%) ⬇️
sur-tests 36.63% <75.00%> (ø)
sur-tests-inflights 39.43% <72.22%> (ø)
unit 73.83% <94.44%> (+<0.01%) ⬆️
utapi-v2-tests 35.27% <75.00%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@delthas
delthas marked this pull request as ready for review August 12, 2026 14:09
@delthas
delthas requested review from a team, SylvainSenechal and benzekrimaha August 12, 2026 14:09
Comment thread lib/utilities/monitoringHandler.js
@delthas
delthas force-pushed the improvement/CLDSRV-972/call-isquotaenabled branch from b448fe5 to d580a11 Compare August 12, 2026 15:33
@delthas
delthas requested review from francoisferrand and removed request for benzekrimaha August 13, 2026 07:49
@delthas

delthas commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Requested @francoisferrand in place of Maha Benzekri, who is currently on PTO.

Both quota gates in monitoringHandler tested the function reference rather
than calling it, so both were always true. The two bugs cancelled out:
metrics were always registered, so crrCacheToProm never reached an
unregistered gauge.

Fixing only the registration gate would crash it, and would also crash the
metric observation in the scuba wrapper, which the Veeam capacity route
reaches with no quota gating at all -- in a finally block, so the TypeError
would replace the original error. Guard every metric touch point so each
tolerates a metric that was never registered.

Note this changes an operator-visible surface: the six s3_cloudserver_quota_*
metrics are now absent when quota is disabled.

Issue: CLDSRV-972
@delthas
delthas force-pushed the improvement/CLDSRV-972/call-isquotaenabled branch from d580a11 to fcefd5a Compare August 13, 2026 08:10
Base automatically changed from improvement/CLDSRV-971/quota-metrics-retrieval-code-label to development/9.4 August 13, 2026 08:39
@bert-e

bert-e commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Hello delthas,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options
name description privileged authored
/after_pull_request Wait for the given pull request id to be merged before continuing with the current one.
/bypass_author_approval Bypass the pull request author's approval
/bypass_build_status Bypass the build and test status
/bypass_commit_size Bypass the check on the size of the changeset TBA
/bypass_incompatible_branch Bypass the check on the source branch prefix
/bypass_jira_check Bypass the Jira issue check
/bypass_peer_approval Bypass the pull request peers' approval
/bypass_leader_approval Bypass the pull request leaders' approval
/approve Instruct Bert-E that the author has approved the pull request. ✍️
/create_pull_requests Allow the creation of integration pull requests.
/create_integration_branches Allow the creation of integration branches.
/no_octopus Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
/unanimity Change review acceptance criteria from one reviewer at least to all reviewers
/wait Instruct Bert-E not to run until further notice.
Available commands
name description privileged
/help Print Bert-E's manual in the pull request.
/status Print Bert-E's current status in the pull request.
/clear Remove all comments from Bert-E from the history TBA
/retry Re-start a fresh build TBA
/build Re-start a fresh build TBA
/force_reset Delete integration branches & pull requests, and restart merge process from the beginning.
/reset Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

@scality scality deleted a comment from bert-e Aug 13, 2026
@bert-e

bert-e commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • 2 peers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants