Add logs for storage pools reordering #10419
Conversation
|
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)
|
Codecov Report❌ Patch coverage is
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
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:
|
...rage/src/main/java/org/apache/cloudstack/storage/allocator/AbstractStoragePoolAllocator.java
Outdated
Show resolved
Hide resolved
...rage/src/main/java/org/apache/cloudstack/storage/allocator/AbstractStoragePoolAllocator.java
Outdated
Show resolved
Hide resolved
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
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)
|
Alright, I've just changed the level of logs on reordering methods, @DaanHoogland |
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
makes mostly sense, just one info i'd like to question.
...rage/src/main/java/org/apache/cloudstack/storage/allocator/AbstractStoragePoolAllocator.java
Outdated
Show resolved
Hide resolved
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12691 |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
@julien-vaz could you fix the conflicts? |
Sure! |
Substitui por nova sintaxe do Log4j
…ocator/AbstractStoragePoolAllocator.java Co-authored-by: dahn <daan.hoogland@gmail.com>
julien-vaz
force-pushed
the
add-logs-storage-pool-reordering
branch
from
53c59bf to
04e69aa
Compare
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✖️ debian ✔️ suse15. SL-JID 13617 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-13481)
|
|
@sureshanaparti , do you think we need anymore testing on this? (/me not convinced) |
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14773 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-14157)
|
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14793 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-14164)
|
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
@julien-vaz can you please fix the conflicts, lets try to put this in 4.22 |
|
@julien-vaz Since this is for the 4.22.1 release, could you retarget the PR to the 4.22 branch? |
borisstoyanov
left a comment
borisstoyanov
left a comment
There was a problem hiding this comment.
LGTM, based on code review and BO tests. @sureshanaparti could you check the branch and merge please
There was a problem hiding this comment.
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 throughoutAbstractStoragePoolAllocator. - 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); |
There was a problem hiding this comment.
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.
| 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); |
| 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()); |
There was a problem hiding this comment.
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.
|
@julien-vaz can you please resolve the conflicts |
11 participants
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
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
How Has This Been Tested?