Fix update resource count failure for domains by Pearl1594 · Pull Request #11138 · apache/cloudstack

Pearl1594 · 2025-07-03T14:22:36Z

Description

This PR fixes: #11100

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

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

codecov · 2025-07-03T14:29:59Z

Codecov Report

Attention: Patch coverage is 14.58333% with 41 lines in your changes missing coverage. Please review.

Project coverage is 16.23%. Comparing base (ed7bd5e) to head (84577e8).
Report is 9 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
.../cloud/resourcelimit/ResourceLimitManagerImpl.java	14.58%	41 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.20   #11138      +/-   ##
============================================
+ Coverage     16.16%   16.23%   +0.07%     
- Complexity    13278    13328      +50     
============================================
  Files          5657     5657              
  Lines        497969   498663     +694     
  Branches      60389    60650     +261     
============================================
+ Hits          80472    80945     +473     
- Misses       408531   408726     +195     
- Partials       8966     8992      +26

Flag	Coverage Δ
uitests	`4.00% <ø> (ø)`
unittests	`17.08% <14.58%> (+0.07%)`	⬆️

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.

DaanHoogland · 2025-07-04T10:53:48Z

@blueorangutan package

blueorangutan · 2025-07-04T10:56:05Z

@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 · 2025-07-04T13:01:30Z

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

DaanHoogland · 2025-07-05T13:14:10Z

@blueorangutan ui

blueorangutan · 2025-07-05T13:15:04Z

@DaanHoogland a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

blueorangutan · 2025-07-05T13:32:18Z

UI build: ✔️
Live QA URL: https://qa.cloudstack.cloud/simulator/pr/11138 (QA-JID-660)

DaanHoogland · 2025-07-05T13:36:37Z

when updating resource count for a child domain it fails. I think this is not what solves the issue at hand/needs more investigation.

hsato03 · 2025-07-07T16:03:28Z

The regression was introduced by commit a0080a0 as the accountId (a0080a0#diff-d2867d8cb1ef1dd180f2cf4a9c8c69086ffcd4153b7f3c7c2211b0d49920ac62R857) parameter is null while updating domains resource count, which caused the moveDomain API to stop working as well.

Perhaps ensuring that accountId is not null before using it would be enough to fix the issue.

DaanHoogland · 2025-07-08T16:27:19Z

The regression was introduced by commit a0080a0 as the accountId (a0080a0#diff-d2867d8cb1ef1dd180f2cf4a9c8c69086ffcd4153b7f3c7c2211b0d49920ac62R857) parameter is null while updating domains resource count, which caused the moveDomain API to stop working as well.

Perhaps ensuring that accountId is not null before using it would be enough to fix the issue.

no, I think an accountid should not be obligatory when updating limit counts on a domain. But a null check (and making the code you point to not be called if it is null) would seem in order.

hsato03 · 2025-07-08T17:28:13Z

The regression was introduced by commit a0080a0 as the accountId (a0080a0#diff-d2867d8cb1ef1dd180f2cf4a9c8c69086ffcd4153b7f3c7c2211b0d49920ac62R857) parameter is null while updating domains resource count, which caused the moveDomain API to stop working as well.
Perhaps ensuring that accountId is not null before using it would be enough to fix the issue.

no, I think an accountid should not be obligatory when updating limit counts on a domain. But a null check (and making the code you point to not be called if it is null) would seem in order.

Yes, that's what I meant. My last comment was a bit ambiguous, sorry 😁

…arnings)

This reverts commit 504cdb8.

DaanHoogland · 2025-07-15T10:18:40Z

server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java

+        if (accountId != null) {
+            Account account = _entityMgr.findById(Account.class, accountId);
+            if (account == null) {
+                throw new InvalidParameterValueException("Unable to find account " + accountId);
+            }
+            _accountMgr.verifyCallerPrivilegeForUserOrAccountOperations(account);
        }
-        _accountMgr.verifyCallerPrivilegeForUserOrAccountOperations(account);
-


this is the essential change. @Pearl1594 @hsato03 , can you have a look?

sonarqubecloud · 2025-07-15T11:14:11Z

Quality Gate failed

Failed conditions
7.5% Coverage on New Code (required ≥ 40%)

See analysis details on SonarQube Cloud

Pearl1594

lgtm. Tested fix. Works as expected. Thanks @DaanHoogland

hsato03

CLGTM

daviftorres · 2025-09-09T17:49:43Z

hey @Pearl1594 , I believe I found a bug that is related to this one. Are you able to confirm if this fix resolves the issue below:

On our NON-Prod environment environments that are on ACS 4.21.0, the following command works.

cmk moveDomain domainid=<ID_TO_MOVE> parentdomainid=<DESTINATION_ID>

But on our Prod environment, that is on ACS 4.20.1 the same command fails with the following output.

🙈 Error: (HTTP 431, error code 4350) Unable to find account null

Pearl1594 · 2025-09-09T20:20:43Z

hey @Pearl1594 , I believe I found a bug that is related to this one. Are you able to confirm if this fix resolves the issue below:

On our NON-Prod environment environments that are on ACS 4.21.0, the following command works.
cmk moveDomain domainid=<ID_TO_MOVE> parentdomainid=<DESTINATION_ID>
But on our Prod environment, that is on ACS 4.20.1 the same command fails with the following output.
🙈 Error: (HTTP 431, error code 4350) Unable to find account null

Thanks @daviftorres - will check it out. This wasn't tested with the moveDomain API - something may have been overlooked.

Fix update resource count failure for domains

504cdb8

Pearl1594 requested a review from DaanHoogland July 3, 2025 14:22

boring-cyborg bot added the component:UI label Jul 3, 2025

sureshanaparti added this to the 4.20.2 milestone Jul 4, 2025

weizhouapache linked an issue Jul 10, 2025 that may be closed by this pull request

Update Resource Count fails in 4.20.1 with “Unable to find account null” #11100

Closed

Daan Hoogland added 2 commits July 15, 2025 12:17

make sure account is only considered when given (and deal with some w…

76b9a75

…arnings)

Revert "Fix update resource count failure for domains"

84577e8

This reverts commit 504cdb8.

DaanHoogland force-pushed the ghi11100-resource-count branch from 9d01e3b to 84577e8 Compare July 15, 2025 10:17

DaanHoogland reviewed Jul 15, 2025

View reviewed changes

DaanHoogland mentioned this pull request Jul 18, 2025

Update Resource Count fails in 4.20.1 with “Unable to find account null” #11100

Closed

Pearl1594 commented Jul 18, 2025

View reviewed changes

hsato03 approved these changes Jul 18, 2025

View reviewed changes

DaanHoogland merged commit 23de6c7 into 4.20 Jul 18, 2025
48 of 50 checks passed

DaanHoogland deleted the ghi11100-resource-count branch July 18, 2025 20:35

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Aug 6, 2025

Fix update resource count failure for domains (apache#11138)

7a5f760

Conversation

Pearl1594 commented Jul 3, 2025

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Uh oh!

codecov bot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DaanHoogland commented Jul 4, 2025

Uh oh!

blueorangutan commented Jul 4, 2025

Uh oh!

blueorangutan commented Jul 4, 2025

Uh oh!

DaanHoogland commented Jul 5, 2025

Uh oh!

blueorangutan commented Jul 5, 2025

Uh oh!

blueorangutan commented Jul 5, 2025

Uh oh!

DaanHoogland commented Jul 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hsato03 commented Jul 7, 2025

Uh oh!

DaanHoogland commented Jul 8, 2025

Uh oh!

hsato03 commented Jul 8, 2025

Uh oh!

DaanHoogland Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jul 15, 2025

Quality Gate failed

Uh oh!

Pearl1594 left a comment

Choose a reason for hiding this comment

Uh oh!

hsato03 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

daviftorres commented Sep 9, 2025

Uh oh!

Pearl1594 commented Sep 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Jul 3, 2025 •

edited

Loading

DaanHoogland commented Jul 5, 2025 •

edited

Loading