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

Add logs for storage pools reordering #10419

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

Open
julien-vaz wants to merge 4 commits into apache:4.22
base: 4.22
Choose a base branch
Loading
from scclouds:add-logs-storage-pool-reordering

Conversation

@julien-vaz
Copy link
Contributor

@julien-vaz julien-vaz commented Feb 18, 2025

Description

Since the storage pool reordering process was lacking logs for troubleshooting, some debug level log messages were added and trace level log messages were changed to the debug level. Also, the log messages were rewritten according to the new Log4j2 syntax.

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

How Has This Been Tested?

  1. A volume was created;
  2. Its disk offering was changed;
  3. It was verified that the log messages were properly being displayed.

Copy link

boring-cyborg bot commented Feb 18, 2025

Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:

Copy link

codecov bot commented Feb 18, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 20.00000% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.57%. Comparing base (fa85a75) to head (b69581a).
⚠️ Report is 332 commits behind head on main.

Files with missing lines Patch % Lines
...torage/allocator/AbstractStoragePoolAllocator.java 20.00% 24 Missing ⚠️
Additional details and impacted files 
@@ Coverage Diff @@
## main #10419 +/- ##
============================================
- Coverage 16.58% 16.57% -0.01% 
- Complexity 13870 13987 +117 
============================================
 Files 5719 5745 +26 
 Lines 507194 510845 +3651 
 Branches 61573 62136 +563 
============================================
+ Hits 84093 84697 +604 
- Misses 413681 416676 +2995 
- Partials 9420 9472 +52
Flag Coverage Δ
uitests 3.91% <ø> (-0.06%) ⬇️
unittests 17.47% <20.00%> (+0.02%) ⬆️

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

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

one general remark on these changes; I see only trace and debug logs, but if axtra logs are really needed I would expect at least hight level logging to be added (info and higher) were considerations made to do this? (DEBUG should not be on in production environments in principle, unless trouble shooting is going on)

julien-vaz reacted with thumbs up emoji
Copy link
Contributor Author

Alright, I've just changed the level of logs on reordering methods, @DaanHoogland

DaanHoogland reacted with thumbs up emoji

Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

makes mostly sense, just one info i'd like to question.

Copy link
Contributor Author

@blueorangutan package

Copy link

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

Copy link

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

Copy link
Contributor

@DaanHoogland DaanHoogland 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
Member

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

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

Copy link
Contributor

@julien-vaz could you fix the conflicts?

Copy link
Contributor Author

@julien-vaz could you fix the conflicts?

Sure!

Julien Hervot de Mattos Vaz and others added 3 commits June 4, 2025 18:17 
Substitui por nova sintaxe do Log4j
...ocator/AbstractStoragePoolAllocator.java
Co-authored-by: dahn <daan.hoogland@gmail.com>
Copy link
Contributor

@blueorangutan package

Copy link

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

Copy link

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

Copy link
Contributor

@blueorangutan test

Copy link

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

Copy link

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

Test Result Time (s) Test File

Copy link
Contributor

@sureshanaparti , do you think we need anymore testing on this? (/me not convinced)

Copy link
Contributor

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

Copy link

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

Copy link
Contributor

@blueorangutan test

Copy link

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

Copy link

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

Test Result Time (s) Test File
all_test_2fa Skipped --- test_2fa.py
all_test_account_access Skipped --- test_account_access.py
all_test_accounts Skipped --- test_accounts.py
all_test_affinity_groups_projects Skipped --- test_affinity_groups_projects.py
all_test_affinity_groups Skipped --- test_affinity_groups.py
all_test_annotations Skipped --- test_annotations.py
all_test_async_job Skipped --- test_async_job.py
all_test_attach_multiple_volumes Skipped --- test_attach_multiple_volumes.py
all_test_backup_recovery_dummy Skipped --- test_backup_recovery_dummy.py
all_test_backup_recovery_nas Skipped --- test_backup_recovery_nas.py
all_test_backup_recovery_veeam Skipped --- test_backup_recovery_veeam.py
all_test_bucket Skipped --- test_bucket.py
all_test_certauthority_root Skipped --- test_certauthority_root.py
all_test_cluster_drs Skipped --- test_cluster_drs.py
all_test_console_endpoint Skipped --- test_console_endpoint.py
all_test_create_list_domain_account_project Skipped --- test_create_list_domain_account_project.py
all_test_create_network Skipped --- test_create_network.py
all_test_deploy_vgpu_enabled_vm Skipped --- test_deploy_vgpu_enabled_vm.py
all_test_deploy_virtio_scsi_vm Skipped --- test_deploy_virtio_scsi_vm.py
all_test_deploy_vm_extra_config_data Skipped --- test_deploy_vm_extra_config_data.py
all_test_deploy_vm_iso Skipped --- test_deploy_vm_iso.py
all_test_deploy_vm_iso_uefi Skipped --- test_deploy_vm_iso_uefi.py
all_test_deploy_vm_root_resize Skipped --- test_deploy_vm_root_resize.py
all_test_deploy_vms_in_parallel Skipped --- test_deploy_vms_in_parallel.py
all_test_deploy_vms_with_varied_deploymentplanners Skipped --- test_deploy_vms_with_varied_deploymentplanners.py
all_test_deploy_vm_with_userdata Skipped --- test_deploy_vm_with_userdata.py
all_test_diagnostics Skipped --- test_diagnostics.py
all_test_direct_download Skipped --- test_direct_download.py
all_test_disk_offerings Skipped --- test_disk_offerings.py
all_test_disk_provisioning_types Skipped --- test_disk_provisioning_types.py
all_test_domain_disk_offerings Skipped --- test_domain_disk_offerings.py
all_test_domain_network_offerings Skipped --- test_domain_network_offerings.py
all_test_domain_service_offerings Skipped --- test_domain_service_offerings.py
all_test_domain_vpc_offerings Skipped --- test_domain_vpc_offerings.py
all_test_dynamicroles Skipped --- test_dynamicroles.py
all_test_enable_account_settings_for_domain Skipped --- test_enable_account_settings_for_domain.py
all_test_enable_role_based_users_in_projects Skipped --- test_enable_role_based_users_in_projects.py
all_test_events_resource Skipped --- test_events_resource.py
all_test_extension_custom_action_lifecycle Skipped --- test_extension_custom_action_lifecycle.py
all_test_extension_custom Skipped --- test_extension_custom.py
all_test_extension_lifecycle Skipped --- test_extension_lifecycle.py
all_test_gateway_on_shared_networks Skipped --- test_gateway_on_shared_networks.py
all_test_global_acls Skipped --- test_global_acls.py
all_test_global_settings Skipped --- test_global_settings.py
all_test_guest_os Skipped --- test_guest_os.py
all_test_guest_vlan_range Skipped --- test_guest_vlan_range.py
all_test_host_control_state Skipped --- test_host_control_state.py
all_test_hostha_simulator Skipped --- test_hostha_simulator.py
all_test_host_ping Skipped --- test_host_ping.py
all_test_host_tags Skipped --- test_host_tags.py
all_test_human_readable_logs Skipped --- test_human_readable_logs.py
all_test_image_store_object_migration Skipped --- test_image_store_object_migration.py
all_test_import_unmanage_volumes Skipped --- test_import_unmanage_volumes.py
all_test_internal_lb Skipped --- test_internal_lb.py
all_test_ipv4_routing Skipped --- test_ipv4_routing.py
all_test_ipv6_infra Skipped --- test_ipv6_infra.py
all_test_iso Skipped --- test_iso.py
all_test_kubernetes_clusters Skipped --- test_kubernetes_clusters.py
all_test_kubernetes_supported_versions Skipped --- test_kubernetes_supported_versions.py
all_test_list_accounts Skipped --- test_list_accounts.py
all_test_list_disk_offerings Skipped --- test_list_disk_offerings.py
all_test_list_domains Skipped --- test_list_domains.py
all_test_list_hosts Skipped --- test_list_hosts.py
all_test_list_ids_parameter Skipped --- test_list_ids_parameter.py
all_test_list_service_offerings Skipped --- test_list_service_offerings.py
all_test_list_storage_pools Skipped --- test_list_storage_pools.py
all_test_list_volumes Skipped --- test_list_volumes.py
all_test_loadbalance Skipped --- test_loadbalance.py
all_test_login Skipped --- test_login.py
all_test_metrics_api Skipped --- test_metrics_api.py
all_test_migration Skipped --- test_migration.py
all_test_ms_maintenance_and_safe_shutdown Skipped --- test_ms_maintenance_and_safe_shutdown.py
all_test_multipleips_per_nic Skipped --- test_multipleips_per_nic.py
all_test_nested_virtualization Skipped --- test_nested_virtualization.py
all_test_network_acl Skipped --- test_network_acl.py
all_test_network_ipv6 Skipped --- test_network_ipv6.py
all_test_network_permissions Skipped --- test_network_permissions.py
all_test_network Skipped --- test_network.py
all_test_nic_adapter_type Skipped --- test_nic_adapter_type.py
all_test_nic Skipped --- test_nic.py
all_test_non_contigiousvlan Skipped --- test_non_contigiousvlan.py
all_test_nonstrict_affinity_group Skipped --- test_nonstrict_affinity_group.py
all_test_object_stores Skipped --- test_object_stores.py
all_test_outofbandmanagement_nestedplugin Skipped --- test_outofbandmanagement_nestedplugin.py
all_test_outofbandmanagement Skipped --- test_outofbandmanagement.py
all_test_over_provisioning Skipped --- test_over_provisioning.py
all_test_password_server Skipped --- test_password_server.py
all_test_persistent_network Skipped --- test_persistent_network.py
all_test_portable_publicip Skipped --- test_portable_publicip.py
all_test_portforwardingrules Skipped --- test_portforwardingrules.py
all_test_primary_storage Skipped --- test_primary_storage.py
all_test_primary_storage_scope Skipped --- test_primary_storage_scope.py
all_test_privategw_acl_ovs_gre Skipped --- test_privategw_acl_ovs_gre.py
all_test_privategw_acl Skipped --- test_privategw_acl.py
all_test_private_roles Skipped --- test_private_roles.py
all_test_projects Skipped --- test_projects.py
all_test_public_ip_range Skipped --- test_public_ip_range.py
all_test_purge_expunged_vms Skipped --- test_purge_expunged_vms.py
all_test_pvlan Skipped --- test_pvlan.py
all_test_quarantined_ips Skipped --- test_quarantined_ips.py
all_test_regions Skipped --- test_regions.py
all_test_register_userdata Skipped --- test_register_userdata.py
all_test_reset_configuration_settings Skipped --- test_reset_configuration_settings.py
all_test_reset_vm_on_reboot Skipped --- test_reset_vm_on_reboot.py
all_test_resource_accounting Skipped --- test_resource_accounting.py
all_test_resource_detail Skipped --- test_resource_detail.py
all_test_resource_names Skipped --- test_resource_names.py
all_test_restore_vm Skipped --- test_restore_vm.py
all_test_router_dhcphosts Skipped --- test_router_dhcphosts.py
all_test_router_dns Skipped --- test_router_dns.py
all_test_router_dnsservice Skipped --- test_router_dnsservice.py
all_test_routers_iptables_default_policy Skipped --- test_routers_iptables_default_policy.py
all_test_routers_network_ops Skipped --- test_routers_network_ops.py
all_test_routers Skipped --- test_routers.py
all_test_scale_vm Skipped --- test_scale_vm.py
all_test_secondary_storage Skipped --- test_secondary_storage.py
all_test_service_offerings Skipped --- test_service_offerings.py
all_test_set_sourcenat Skipped --- test_set_sourcenat.py
all_test_sharedfs_lifecycle Skipped --- test_sharedfs_lifecycle.py
all_test_snapshots Skipped --- test_snapshots.py
all_test_ssvm Skipped --- test_ssvm.py
all_test_staticroles Skipped --- test_staticroles.py
all_test_storage_policy Skipped --- test_storage_policy.py
all_test_templates Skipped --- test_templates.py
all_test_update_security_group Skipped --- test_update_security_group.py
all_test_usage_events Skipped --- test_usage_events.py
all_test_usage Skipped --- test_usage.py
all_test_vm_autoscaling Skipped --- test_vm_autoscaling.py
all_test_vm_deployment_planner Skipped --- test_vm_deployment_planner.py
all_test_vm_life_cycle Skipped --- test_vm_life_cycle.py
all_test_vm_lifecycle_unmanage_import Skipped --- test_vm_lifecycle_unmanage_import.py
all_test_vm_lifecycle_with_snapshot_or_volume Skipped --- test_vm_lifecycle_with_snapshot_or_volume.py
all_test_vm_schedule Skipped --- test_vm_schedule.py
all_test_vm_snapshot_kvm Skipped --- test_vm_snapshot_kvm.py
all_test_vm_snapshots Skipped --- test_vm_snapshots.py
all_test_vm_strict_host_tags Skipped --- test_vm_strict_host_tags.py
all_test_vnf_templates Skipped --- test_vnf_templates.py
all_test_volumes Skipped --- test_volumes.py
all_test_vpc_ipv6 Skipped --- test_vpc_ipv6.py
all_test_vpc_redundant Skipped --- test_vpc_redundant.py
all_test_vpc_router_nics Skipped --- test_vpc_router_nics.py
all_test_vpc_vpn Skipped --- test_vpc_vpn.py
all_test_webhook_delivery Skipped --- test_webhook_delivery.py
all_test_webhook_lifecycle Skipped --- test_webhook_lifecycle.py
all_test_host_maintenance Skipped --- test_host_maintenance.py
all_test_hostha_kvm Skipped --- test_hostha_kvm.py
DaanHoogland reacted with eyes emoji

Copy link
Contributor

@blueorangutan package

Copy link

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

Copy link

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

Copy link
Contributor

@blueorangutan test

Copy link

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

Copy link

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

Test Result Time (s) Test File
test_04_rvpc_network_garbage_collector_nics Failure 1452.41 test_vpc_redundant.py

Copy link

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

Copy link
Contributor

@julien-vaz can you please fix the conflicts, lets try to put this in 4.22

Copy link

@julien-vaz Since this is for the 4.22.1 release, could you retarget the PR to the 4.22 branch?

@DaanHoogland DaanHoogland changed the base branch from main to 4.22 January 12, 2026 09:58
@borisstoyanov borisstoyanov marked this pull request as ready for review January 26, 2026 09:09
Copilot AI review requested due to automatic review settings January 26, 2026 09:09
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, based on code review and BO tests. @sureshanaparti could you check the branch and merge please

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 enhances observability of storage pool reordering by adding and updating log messages and migrating them to Log4j2-style parameterized logging.

Changes:

  • Replaced String.format-based logging with Log4j2-style {} placeholders throughout AbstractStoragePoolAllocator.
  • Added and elevated several log statements (including to INFO level) around pool reordering by capacity, number of volumes, and disk provisioning type.
  • Adjusted logging for filtering and compatibility checks on storage pools to provide more structured and detailed context for troubleshooting.

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

reorderedPools.add(preferredIndex++, pool);
}
}
logger.debug("Reordered list of pools by disk provisioning type [{}]: [{}]", diskProfile.getProvisioningType(), pools);
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

This debug statement logs the original pools list even though the method has just built a reorderedPools list and is about to return it, which makes the message misleading when troubleshooting reordering by disk provisioning type. Consider logging the reordered list instead so the log output reflects the actual ordering that will be used.

Suggested change
logger.debug("Reordered list of pools by disk provisioning type [{}]: [{}]", diskProfile.getProvisioningType(), pools);
logger.debug("Reordered list of pools by disk provisioning type [{}]: [{}]", diskProfile.getProvisioningType(), reorderedPools);

Copilot uses AI. Check for mistakes.
Comment on lines 209 to +213
if (pools == null) {
logger.trace("There are no pools to reorder; returning null.");
logger.info("There are no pools to reorder.");
return null;
}
if (logger.isTraceEnabled()) {
logger.trace(String.format("reordering %d pools", pools.size()));
}
logger.info("Reordering [{}] pools", pools.size());
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

These log statements (and similar ones added below) have been raised to INFO level, which means every pool reordering attempt will now be emitted at INFO instead of DEBUG/TRACE and can significantly increase log volume in busy environments. This also goes beyond the PR description (which mentioned adding DEBUG logs and promoting TRACE to DEBUG), so please confirm that INFO is the intended level for these high-frequency paths or consider keeping them at DEBUG while still adopting the new Log4j2 placeholder syntax.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@julien-vaz can you please resolve the conflicts

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

Reviewers

@DaanHoogland DaanHoogland DaanHoogland approved these changes

Copilot code review Copilot Copilot left review comments

@sureshanaparti sureshanaparti sureshanaparti approved these changes

@borisstoyanov borisstoyanov borisstoyanov approved these changes

@bernardodemarco bernardodemarco bernardodemarco approved these changes

Projects

Status: In Progress

Milestone

4.22.1

Development

Successfully merging this pull request may close these issues.

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