Consider Quota settings during processing

[julien-vaz]

Consider Quota settings during processing

Description

Currently, when using the Quota plugin, it is possible to enable/disable the plugin specifically in the account settings. However, even if the plugin is disabled for the account, the setting is not considered because the Quota manager is failing to get its value, so that the account's Quota balance is always processed. This PR fixes this behavior.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)
• build/CI
• test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

Screenshots (if appropriate):

How Has This Been Tested?

1. A account was created and, before the Usage job was run, the Quota plugin was disabled for the account. It was attested via GUI and logs that the account didn't appear in Quota and it wasn't even processed.
2. Another account was created with the Quota plugin enabled. After the Usage job was run once and the Quota was processed for the account, the Quota plugin was disabled and on the second run it was attested that the credit was not decremented.

[codecov]

Codecov Report

❌ Patch coverage is 4.00000% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.57%. Comparing base (0934fb1) to head (617d232).
⚠️ Report is 603 commits behind head on main.

Files with missing lines	Patch %	Lines
.../org/apache/cloudstack/quota/QuotaManagerImpl.java	0.00%	24 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #10892      +/-   ##
============================================
+ Coverage     16.30%   16.57%   +0.26%     
- Complexity    13450    13867     +417     
============================================
  Files          5675     5719      +44     
  Lines        499249   507216    +7967     
  Branches      60377    61577    +1200     
============================================
+ Hits          81425    84088    +2663     
- Misses       408753   413709    +4956     
- Partials       9071     9419     +348     

Flag	Coverage Δ
uitests	3.96% <ø> (-0.03%)	⬇️
unittests	17.45% <4.00%> (+0.28%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[JoaoJandre]

@blueorangutan package

[blueorangutan]

@JoaoJandre a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13585

[Copilot]

Pull Request Overview

This PR fixes the Quota plugin to respect per-account and per-domain enable/disable settings during usage processing and email alerts.

• Introduce findConfigurationValue to read boolean config keys from account or domain details, falling back to defaults.
• Update shouldCalculateUsageRecord and isQuotaEmailTypeEnabledForAccount to use the new lookup.
• Wire account/domain DAOs into Spring and adjust tests for the new method.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
framework/quota/src/test/java/org/apache/cloudstack/quota/QuotaAlertManagerImplTest.java	Remove old VO mock, update test setup to mock the new configuration lookup.
framework/quota/src/main/java/org/apache/cloudstack/quota/QuotaManagerImpl.java	Add findConfigurationValue helper, inject AccountDetailsDao and DomainDetailsDao, and update usage logic.
framework/quota/src/main/java/org/apache/cloudstack/quota/QuotaManager.java	Declare new interface method findConfigurationValue.
framework/quota/src/main/java/org/apache/cloudstack/quota/QuotaAlertManagerImpl.java	Replace direct config lookup with _quotaManager.findConfigurationValue.
engine/schema/src/main/resources/META-INF/cloudstack/core/spring-engine-schema-core-common-daos-between-management-and-usage-context.xml	Register beans for accountDetailsDaoImpl, domainDaoImpl, and domainDetailsDaoImpl.

Comments suppressed due to low confidence (3)

framework/quota/src/test/java/org/apache/cloudstack/quota/QuotaAlertManagerImplTest.java:117

• The first test no longer mocks the new findConfigurationValue call, so it falls back to the default instead of expectedValue. Add Mockito.when(quotaManagerMock.findConfigurationValue(accountMock, QuotaConfig.QuotaEnableEmails)).thenReturn(expectedValue) before invoking isQuotaEmailTypeEnabledForAccount.

boolean expectedValue = !QuotaConfig.QuotaEnableEmails.value();

framework/quota/src/main/java/org/apache/cloudstack/quota/QuotaManagerImpl.java:362

• Missing import for AccountVO. Add import com.cloud.user.AccountVO; so the method signature compiles against the interface.

public boolean findConfigurationValue(AccountVO accountVO, ConfigKey<Boolean> key) {

engine/schema/src/main/resources/META-INF/cloudstack/core/spring-engine-schema-core-common-daos-between-management-and-usage-context.xml:32

• [nitpick] The domainDaoImpl bean is registered but not injected or used in the new code. Consider removing it to keep the Spring context lean.

<bean id="domainDaoImpl" class="com.cloud.domain.dao.DomainDaoImpl" />

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14203

[DaanHoogland]

@blueorangutan test

[blueorangutan]

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[blueorangutan]

[SF] Trillian test result (tid-13798)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 54368 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10892-t13798-kvm-ol8.zip
Smoke tests completed. 142 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

[shwstppr]

As far as I understand this might be a generic problem, ie,
ConfigKey.valueIn(accountId) not returning domain-level value when the config enable.account.settings.for.domain is set to true.
And a change should be added at the ConfigKey level itself.
Also, it will be worth checking if the behaviour has been fixed by 2a4a1f7 as Domain is considered PArent scope for Account scope now.

[shwstppr]

key.value()

What is the difference?

[harikrishna-patnala]

As far as I understand this might be a generic problem, ie, ConfigKey.valueIn(accountId) not returning domain-level value when the config enable.account.settings.for.domain is set to true. And a change should be added at the ConfigKey level itself. Also, it will be worth checking if the behaviour has been fixed by 2a4a1f7 as Domain is considered PArent scope for Account scope now.

if we call valueinscope() method, will it work @shwstppr ?

[github-actions]

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.