Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

small refactors on account manager #11784

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
DaanHoogland wants to merge 1 commit into apache:main
base: main
Choose a base branch
Loading
from DaanHoogland:accountManagerRefactors

Conversation

@DaanHoogland
Copy link
Contributor

@DaanHoogland DaanHoogland commented Oct 3, 2025

Description

This PR does some small refactors encountered during trouble shooting. ...

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

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

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

@DaanHoogland DaanHoogland modified the milestones: 4.23, 4.20.3 Oct 3, 2025
Copy link

codecov bot commented Oct 3, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 0% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 17.89%. Comparing base (a7178ee) to head (7718dd8).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...c/main/java/com/cloud/user/AccountManagerImpl.java 0.00% 14 Missing ⚠️
Additional details and impacted files 
@@ Coverage Diff @@
## main #11784 +/- ##
============================================
- Coverage 17.89% 17.89% -0.01% 
+ Complexity 16084 16082 -2 
============================================
 Files 5936 5936 
 Lines 532635 532634 -1 
 Branches 65148 65148 
============================================
- Hits 95298 95289 -9 
- Misses 426667 426674 +7 
- Partials 10670 10671 +1
Flag Coverage Δ
uitests 3.67% <ø> (ø)
unittests 18.99% <0.00%> (-0.01%) ⬇️

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.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR performs small refactoring improvements to the AccountManagerImpl class to enhance code readability and maintainability. The changes include simplifying conditional logic, extracting reusable methods, and updating logging patterns.

  • Simplified multiple conditional statements to use direct return expressions
  • Extracted domain ID calculation logic into a reusable private method
  • Updated logging to use parameterized format and modern charset handling

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor Author

@blueorangutan package

Copy link

@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.

@apache apache deleted a comment from blueorangutan Dec 8, 2025
@apache apache deleted a comment from blueorangutan Dec 8, 2025
@apache apache deleted a comment from blueorangutan Dec 8, 2025
@apache apache deleted a comment from blueorangutan Dec 8, 2025
@apache apache deleted a comment from blueorangutan Dec 8, 2025
@apache apache deleted a comment from blueorangutan Dec 8, 2025
@apache apache deleted a comment from blueorangutan Dec 8, 2025
@apache apache deleted a comment from blueorangutan Dec 8, 2025
@apache apache deleted a comment from blueorangutan Dec 9, 2025
Copy link

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


}

private static long getDomainIdFor(ControlledEntity entity) {
Copy link
Collaborator

@hsato03 hsato03 Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private staticlong getDomainIdFor(ControlledEntity entity) {
private long getDomainIdFor(ControlledEntity entity) {

Can static be removed?

Copy link
Contributor Author

@DaanHoogland DaanHoogland Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I think it can. Any pressing reason? It is a bit of a utility method and has no bearing on the manager internals.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the keyword is useful here, but it's just a nitpicking. ;)

Copy link
Collaborator

@hsato03 hsato03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clgtm

Copy link

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

Test Result Time (s) Test File
test_01_primary_storage_nfs Error 0.20 test_primary_storage.py
ContextSuite context=TestStorageTags>:setup Error 0.31 test_primary_storage.py
test_01_primary_storage_scope_change Error 0.12 test_primary_storage_scope.py
test_01_secure_vm_migration Error 212.85 test_vm_life_cycle.py
test_01_secure_vm_migration Error 212.86 test_vm_life_cycle.py
test_02_unsecure_vm_migration Error 0.02 test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration Error 0.01 test_vm_life_cycle.py
test_04_nonsecured_to_secured_vm_migration Error 0.01 test_vm_life_cycle.py
test_08_migrate_vm Error 7.43 test_vm_life_cycle.py
test_01_migrate_vm_strict_tags_success Error 64.00 test_vm_strict_host_tags.py
test_02_migrate_vm_strict_tags_failure Error 11.74 test_vm_strict_host_tags.py
test_01_restore_vm_strict_tags_success Error 11.72 test_vm_strict_host_tags.py
test_02_restore_vm_strict_tags_failure Error 11.77 test_vm_strict_host_tags.py
test_01_scale_vm_strict_tags_success Error 11.77 test_vm_strict_host_tags.py
test_02_scale_vm_strict_tags_failure Error 12.83 test_vm_strict_host_tags.py
test_01_deploy_vm_on_specific_host_without_strict_tags Error 11.71 test_vm_strict_host_tags.py
test_02_deploy_vm_on_any_host_without_strict_tags Error 17.85 test_vm_strict_host_tags.py
test_03_deploy_vm_on_specific_host_with_strict_tags_success Error 12.76 test_vm_strict_host_tags.py
test_04_deploy_vm_on_any_host_with_strict_tags_success Error 16.88 test_vm_strict_host_tags.py
test_01_verify_ipv6_vpc Error 51.38 test_vpc_ipv6.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL Error 111.80 test_vpc_redundant.py
test_02_redundant_VPC_default_routes Error 111.61 test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers Error 55.18 test_vpc_redundant.py
test_04_rvpc_network_garbage_collector_nics Error 112.83 test_vpc_redundant.py
test_05_rvpc_multi_tiers Error 58.48 test_vpc_redundant.py
test_02_VPC_default_routes Error 64.55 test_vpc_router_nics.py
test_01_redundant_vpc_site2site_vpn Failure 51.40 test_vpc_vpn.py
test_01_vpc_site2site_vpn_multiple_options Failure 51.42 test_vpc_vpn.py

@DaanHoogland DaanHoogland marked this pull request as draft December 11, 2025 07:22
Copy link

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

Test Result Time (s) Test File
test_uservm_host_control_state Failure 16.95 test_host_control_state.py
ContextSuite context=TestHostControlState>:teardown Error 32.52 test_host_control_state.py
test_01_vpn_usage Error 1.08 test_usage.py
test_01_migrate_VM_and_root_volume Error 146.94 test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks Error 31.81 test_vm_life_cycle.py
test_01_secure_vm_migration Error 241.43 test_vm_life_cycle.py
test_hostha_enable_ha_when_host_disabled Error 2.78 test_hostha_kvm.py
test_hostha_enable_ha_when_host_in_maintenance Error 305.93 test_hostha_kvm.py

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

// Block when is not in the list of allowed IPs
if (!NetUtils.isIpInCidrList(loginIpAddress, accessAllowedCidrs.split(","))) {
logger.warn("Request by account '" + account.toString() + "' was denied since " + loginIpAddress.toString().replace("/", "") + " does not match " + accessAllowedCidrs);
logger.warn("Request by account '{}' was denied since {} does not match {}", account , loginIpAddress.toString().replace("/", ""), accessAllowedCidrs);
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an extra space before the comma after the first placeholder. The code has "account ," but it should be "account," for consistent formatting.

Suggested change
logger.warn("Request by account '{}' was denied since {} does not match {}", account, loginIpAddress.toString().replace("/", ""), accessAllowedCidrs);
logger.warn("Request by account '{}' was denied since {} does not match {}", account, loginIpAddress.toString().replace("/", ""), accessAllowedCidrs);

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@borisstoyanov borisstoyanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, on code

Copy link
Contributor

@blueorangutan package

Copy link

@borisstoyanov a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

Copy link

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

Copy link
Contributor

@blueorangutan test

Copy link

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

Copy link

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

Test Result Time (s) Test File
ContextSuite context=TestListIdsParams>:teardown Error 1.11 test_list_ids_parameter.py
test_01_snapshot_root_disk Error 4.80 test_snapshots.py
test_02_list_snapshots_with_removed_data_store Error 46.32 test_snapshots.py
test_02_list_snapshots_with_removed_data_store Error 46.33 test_snapshots.py
ContextSuite context=TestSnapshotStandaloneBackup>:teardown Error 26.13 test_snapshots.py
test_01_snapshot_usage Error 23.61 test_usage.py
test_01_vpn_usage Error 1.12 test_usage.py

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

Reviewers

Copilot code review Copilot Copilot left review comments

@borisstoyanov borisstoyanov borisstoyanov approved these changes

@sureshanaparti sureshanaparti Awaiting requested review from sureshanaparti

+1 more reviewer

@hsato03 hsato03 hsato03 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.23.0

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /