refactor: avoid redundant DB calls when checking for root admin account #11390
Conversation
AccountService.isRootAdmin(Long) is currently invoked each time to check if an account or caller is a root admin, leading to repeated DB lookups. In most cases, the Account object is already available and should be used directly. For the caller, the root admin flag is now cached in CallContext to avoid repeated evaluations. Signed-off-by: Abhishek Kumar <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11390 +/- ##
=========================================
Coverage 17.56% 17.57%
- Complexity 15548 15550 +2
=========================================
Files 5913 5913
Lines 529440 529394 -46
Branches 64670 64668 -2
=========================================
+ Hits 93018 93026 +8
+ Misses 425964 425913 -51
+ Partials 10458 10455 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Abhishek Kumar <[email protected]>
|
@blueorangutan package |
|
@shwstppr 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14546 |
Signed-off-by: Abhishek Kumar <[email protected]>
Signed-off-by: Abhishek Kumar <[email protected]>
|
@blueorangutan package |
|
@shwstppr 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14552 |
Signed-off-by: Abhishek Kumar <[email protected]>
Signed-off-by: Abhishek Kumar <[email protected]>
Signed-off-by: Abhishek Kumar <[email protected]>
|
@blueorangutan package |
|
@shwstppr 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14558 |
|
@blueorangutan test |
|
@shwstppr a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-14035)
|
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the AccountService.isRootAdmin(Long) method usage by introducing a new overloaded isRootAdmin(Account) method and caching the root admin flag in CallContext to avoid redundant database lookups. The changes reduce DB calls from 80 to 45 during VM deployment, improving performance by reducing execution time from 4.994s to 3.082s in test environments.
Key Changes
- Added
isRootAdmin(Account)method overload to avoid redundant account lookups - Added
isCallingAccountRootAdmin()method toCallContextfor cached access to caller's root admin status - Refactored existing code to use the new methods where Account objects are already available
Reviewed Changes
Copilot reviewed 82 out of 82 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| AccountManagerImpl.java | Implements new isRootAdmin(Account) method and updates existing logic |
| CallContext interface/impl | Adds isCallingAccountRootAdmin() method for cached root admin checks |
| Various service implementations | Updated to use new methods when Account objects are available |
| Test files | Updated mock configurations and test setup to use new method signatures |
Signed-off-by: Abhishek Kumar <[email protected]>
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
Signed-off-by: Abhishek Kumar <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 82 out of 82 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public boolean isCallingAccountRootAdmin() { | ||
| if (isAccountRootAdmin == null) { | ||
| AccountService accountService; | ||
| try { | ||
| accountService = ComponentContext.getDelegateComponentOfType(AccountService.class); | ||
| } catch (NoSuchBeanDefinitionException e) { | ||
| LOGGER.warn("Falling back to account type check for isRootAdmin for account ID: {} as no AccountService bean found: {}", accountId, e.getMessage()); | ||
| Account caller = getCallingAccount(); | ||
| return caller != null && caller.getType() == Account.Type.ADMIN; | ||
| } | ||
| isAccountRootAdmin = accountService.isRootAdmin(getCallingAccount()); | ||
| } | ||
| return Boolean.TRUE.equals(isAccountRootAdmin); | ||
| } |
There was a problem hiding this comment.
The fallback logic at line 149 returns the result directly without caching it in isAccountRootAdmin. This means if AccountService is unavailable, subsequent calls will repeat the check instead of using the cached value. Either cache the result before returning, or document this as intentional behavior.
| LOGGER.warn("Falling back to account type check for isRootAdmin for account ID: {} as no AccountService bean found: {}", accountId, e.getMessage()); | ||
| Account caller = getCallingAccount(); | ||
| return caller != null && caller.getType() == Account.Type.ADMIN; |
There was a problem hiding this comment.
The fallback check caller.getType() == Account.Type.ADMIN is inconsistent with the actual root admin check performed by AccountService, which uses security checkers. This could lead to different behavior when AccountService is unavailable. Consider removing the fallback entirely or throwing an exception to fail fast when AccountService is not available.
| LOGGER.warn("Falling back to account type check for isRootAdmin for account ID: {} as no AccountService bean found: {}", accountId, e.getMessage()); | |
| Account caller = getCallingAccount(); | |
| return caller != null && caller.getType() == Account.Type.ADMIN; | |
| throw new CloudRuntimeException("AccountService bean not found, cannot determine if calling account is root admin for account ID: " + accountId, e); |
2 participants
Description
AccountService.isRootAdmin(Long)is currently invoked each time to check if an account or caller is a root admin, leading to repeated DB lookups. In most cases, the Account object is already available and should be used directly. For the caller, the root admin flag is now cached in CallContext to avoid repeated evaluations.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Without change, 80 calls for AccountDaoImpl.findById during deploy VM and it took 4.994s in a local simulator env,
After the change, AccountDaoImpl.findById calls reduced to 45 during deploy VM, and it took 3.082s in a local simulator env,
The considerable time difference (0m4.994s vs 0m3.082s) could be intermittent and may need more testing/profiling. Reduction in DB queries is certainly real.
How did you try to break this feature and the system with this change?