Storage pool monitor disconnect improvements #12398

sureshanaparti · 2026-01-09T11:59:10Z

Description

The PRs improves the following in storage pool monitor disconnect process.

Removes unwanted synchronize as it blocks disconnect for all hosts on management server
Improves logs for the disconnect process

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?

- Removes unwanted synchronize as it blocks disconnect for all hosts on management server - Improves logs for the disconnect process

sureshanaparti · 2026-01-09T12:00:49Z

@blueorangutan package

blueorangutan · 2026-01-09T12:02:04Z

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

codecov · 2026-01-09T12:06:59Z

Codecov Report

❌ Patch coverage is 0% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.23%. Comparing base (9a38e75) to head (050f47d).
⚠️ Report is 1 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
...com/cloud/storage/listener/StoragePoolMonitor.java	0.00%	20 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.20   #12398      +/-   ##
============================================
- Coverage     16.23%   16.23%   -0.01%     
- Complexity    13375    13376       +1     
============================================
  Files          5657     5657              
  Lines        498933   498946      +13     
  Branches      60553    60552       -1     
============================================
+ Hits          81012    81014       +2     
- Misses       408889   408901      +12     
+ Partials       9032     9031       -1

Flag	Coverage Δ
uitests	`4.00% <ø> (ø)`
unittests	`17.09% <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.

blueorangutan · 2026-01-09T13:21:09Z

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

vishesh92

clgtm

Copilot

Pull request overview

This PR enhances the storage pool monitor disconnect process by removing synchronization that blocks disconnects for all hosts and adding comprehensive logging and profiling.

Changes:

Removed synchronized keyword from processDisconnect methods to prevent blocking
Added detailed debug logging throughout the disconnect flow to track each step
Implemented profiling to measure disconnect operation duration

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

Copilot · 2026-01-12T06:52:21Z

server/src/main/java/com/cloud/storage/listener/StoragePoolMonitor.java

+                long disconnectDuration = disconnectProfiler.getDurationInMillis() / 1000;
+                logger.debug("Finished disconnect with result {} from Storage Pool {} ({}) ({} of {}) for host: {}, duration: {} secs", disconnectResult, pool.getName(), pool.getUuid(), i, storagePoolHostsSize, host, disconnectDuration);


The profiling duration calculation uses integer division which will truncate to 0 for operations taking less than 1000 milliseconds. This makes the timing information less useful since most disconnect operations likely complete in under a second. Consider using getDurationInMillis() directly and logging in milliseconds, or convert to a double/float for fractional seconds.

Suggested change

long disconnectDuration = disconnectProfiler.getDurationInMillis() / 1000;

logger.debug("Finished disconnect with result {} from Storage Pool {} ({}) ({} of {}) for host: {}, duration: {} secs", disconnectResult, pool.getName(), pool.getUuid(), i, storagePoolHostsSize, host, disconnectDuration);

long disconnectDurationMillis = disconnectProfiler.getDurationInMillis();

logger.debug("Finished disconnect with result {} from Storage Pool {} ({}) ({} of {}) for host: {}, duration: {} ms", disconnectResult, pool.getName(), pool.getUuid(), i, storagePoolHostsSize, host, disconnectDurationMillis);

Copilot · 2026-01-12T06:52:21Z

server/src/main/java/com/cloud/storage/listener/StoragePoolMonitor.java

+            logger.debug("Processing disconnect from Storage Pool {} ({} of {}) for host: {}", storagePoolHost.getPoolId(), i, storagePoolHostsSize, host);
            StoragePoolVO pool = _poolDao.findById(storagePoolHost.getPoolId());
            if (pool == null) {
+                logger.debug("No Storage Pool found with id {} ({} of {}) for host: {}", storagePoolHost.getPoolId(), i, storagePoolHostsSize, host);
                continue;
            }

            if (!pool.isShared()) {
+                logger.debug("Storage Pool {} ({}) ({} of {}) is not shared for host: {}, ignore disconnect", pool.getName(), pool.getUuid(), i, storagePoolHostsSize, host);
                continue;
            }

            // Handle only PowerFlex pool for now, not to impact other pools behavior
            if (pool.getPoolType() != StoragePoolType.PowerFlex) {
+                logger.debug("Storage Pool {} ({}) ({} of {}) is not of type {} for host: {}, ignore disconnect", pool.getName(), pool.getUuid(), i, storagePoolHostsSize, pool.getPoolType(), host);
                continue;
            }

+            logger.debug("Sending disconnect to Storage Pool {} ({}) ({} of {}) for host: {}", pool.getName(), pool.getUuid(), i, storagePoolHostsSize, host);
+            Profiler disconnectProfiler = new Profiler();
            try {
+                disconnectProfiler.start();
                _storageManager.disconnectHostFromSharedPool(host, pool);
            } catch (Exception e) {
                logger.error("Unable to disconnect host {} from storage pool {} due to {}", host, pool, e.toString());
                disconnectResult = false;
+            } finally {
+                disconnectProfiler.stop();
+                long disconnectDuration = disconnectProfiler.getDurationInMillis() / 1000;
+                logger.debug("Finished disconnect with result {} from Storage Pool {} ({}) ({} of {}) for host: {}, duration: {} secs", disconnectResult, pool.getName(), pool.getUuid(), i, storagePoolHostsSize, host, disconnectDuration);


The loop index message format is inconsistent. The message shows "i of storagePoolHostsSize" which displays as "0 of 5", "1 of 5", etc. This is confusing because the index is 0-based while the total count is 1-based. Consider using "i+1" to display "1 of 5", "2 of 5" for better readability in log messages.

Copilot · 2026-01-12T06:52:21Z

server/src/main/java/com/cloud/storage/listener/StoragePoolMonitor.java

            try {
+                disconnectProfiler.start();
                _storageManager.disconnectHostFromSharedPool(host, pool);
            } catch (Exception e) {
                logger.error("Unable to disconnect host {} from storage pool {} due to {}", host, pool, e.toString());
                disconnectResult = false;
+            } finally {
+                disconnectProfiler.stop();
+                long disconnectDuration = disconnectProfiler.getDurationInMillis() / 1000;
+                logger.debug("Finished disconnect with result {} from Storage Pool {} ({}) ({} of {}) for host: {}, duration: {} secs", disconnectResult, pool.getName(), pool.getUuid(), i, storagePoolHostsSize, host, disconnectDuration);
            }


The disconnectResult variable logged in the finally block at line 207 does not accurately represent the result of the current disconnect operation. The variable is set to false only when an exception occurs, but it retains its previous value (from previous iterations or the initial true value) when the current operation succeeds. This makes the log message misleading. Consider using a local boolean variable to track the result of the current disconnect operation specifically.

Suggested change

try {

disconnectProfiler.start();

_storageManager.disconnectHostFromSharedPool(host, pool);

} catch (Exception e) {

logger.error("Unable to disconnect host {} from storage pool {} due to {}", host, pool, e.toString());

disconnectResult = false;

} finally {

disconnectProfiler.stop();

long disconnectDuration = disconnectProfiler.getDurationInMillis() / 1000;

logger.debug("Finished disconnect with result {} from Storage Pool {} ({}) ({} of {}) for host: {}, duration: {} secs", disconnectResult, pool.getName(), pool.getUuid(), i, storagePoolHostsSize, host, disconnectDuration);

}

boolean currentDisconnectResult = true;

try {

disconnectProfiler.start();

_storageManager.disconnectHostFromSharedPool(host, pool);

} catch (Exception e) {

logger.error("Unable to disconnect host {} from storage pool {} due to {}", host, pool, e.toString());

currentDisconnectResult = false;

} finally {

disconnectProfiler.stop();

long disconnectDuration = disconnectProfiler.getDurationInMillis() / 1000;

logger.debug("Finished disconnect with result {} from Storage Pool {} ({}) ({} of {}) for host: {}, duration: {} secs", currentDisconnectResult, pool.getName(), pool.getUuid(), i, storagePoolHostsSize, host, disconnectDuration);

}

if (!currentDisconnectResult) {

disconnectResult = false;

}

Copilot · 2026-01-12T06:52:21Z

server/src/main/java/com/cloud/storage/listener/StoragePoolMonitor.java


            // Handle only PowerFlex pool for now, not to impact other pools behavior
            if (pool.getPoolType() != StoragePoolType.PowerFlex) {
+                logger.debug("Storage Pool {} ({}) ({} of {}) is not of type {} for host: {}, ignore disconnect", pool.getName(), pool.getUuid(), i, storagePoolHostsSize, pool.getPoolType(), host);


The log message has an incorrect argument order. The message template shows "Storage Pool {} ({}) ({} of {}) is not of type {} for host: {}", but the arguments provided are: pool.getName(), pool.getUuid(), i, storagePoolHostsSize, pool.getPoolType(), host. The fifth placeholder expects the pool type that the pool is NOT, but pool.getPoolType() returns the actual type of the pool. The expected type (StoragePoolType.PowerFlex) should be included in the log message instead.

Suggested change

logger.debug("Storage Pool {} ({}) ({} of {}) is not of type {} for host: {}, ignore disconnect", pool.getName(), pool.getUuid(), i, storagePoolHostsSize, pool.getPoolType(), host);

logger.debug("Storage Pool {} ({}) ({} of {}) is not of type {} for host: {}, ignore disconnect", pool.getName(), pool.getUuid(), i, storagePoolHostsSize, StoragePoolType.PowerFlex, host);

Storage pool monitor disconnect improvements

050f47d

- Removes unwanted synchronize as it blocks disconnect for all hosts on management server - Improves logs for the disconnect process

boring-cyborg bot added the component:storage label Jan 9, 2026

sureshanaparti added this to the 4.20.3 milestone Jan 9, 2026

sureshanaparti requested review from kiranchavala and nvazquez January 9, 2026 11:59

sureshanaparti requested a review from vishesh92 January 12, 2026 06:31

vishesh92 requested a review from Copilot January 12, 2026 06:42

Copilot started reviewing on behalf of vishesh92 January 12, 2026 06:42 View session

vishesh92 approved these changes Jan 12, 2026

View reviewed changes

Copilot AI reviewed Jan 12, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Storage pool monitor disconnect improvements #12398

Storage pool monitor disconnect improvements #12398

sureshanaparti commented Jan 9, 2026

Uh oh!

sureshanaparti commented Jan 9, 2026

Uh oh!

blueorangutan commented Jan 9, 2026

Uh oh!

codecov bot commented Jan 9, 2026 •

edited

Loading

Uh oh!

blueorangutan commented Jan 9, 2026

Uh oh!

vishesh92 left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 12, 2026

Uh oh!

Copilot AI Jan 12, 2026

Uh oh!

Copilot AI Jan 12, 2026

Uh oh!

Copilot AI Jan 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		long disconnectDuration = disconnectProfiler.getDurationInMillis() / 1000;
		logger.debug("Finished disconnect with result {} from Storage Pool {} ({}) ({} of {}) for host: {}, duration: {} secs", disconnectResult, pool.getName(), pool.getUuid(), i, storagePoolHostsSize, host, disconnectDuration);

	logger.debug("Storage Pool {} ({}) ({} of {}) is not of type {} for host: {}, ignore disconnect", pool.getName(), pool.getUuid(), i, storagePoolHostsSize, pool.getPoolType(), host);
	logger.debug("Storage Pool {} ({}) ({} of {}) is not of type {} for host: {}, ignore disconnect", pool.getName(), pool.getUuid(), i, storagePoolHostsSize, StoragePoolType.PowerFlex, host);

Storage pool monitor disconnect improvements #12398

Are you sure you want to change the base?

Storage pool monitor disconnect improvements #12398

Conversation

sureshanaparti commented Jan 9, 2026

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!

sureshanaparti commented Jan 9, 2026

Uh oh!

blueorangutan commented Jan 9, 2026

Uh oh!

codecov bot commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blueorangutan commented Jan 9, 2026

Uh oh!

vishesh92 left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jan 9, 2026 •

edited

Loading