Skip to content

Storage pool monitor disconnect improvements #12398

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.

Sign up for GitHub

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
sureshanaparti wants to merge 1 commit into
base: 4.20
Choose a base branch
from
+22 −6
Open

Storage pool monitor disconnect improvements #12398

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 22 additions & 6 deletions server/src/main/java/com/cloud/storage/listener/StoragePoolMonitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import javax.inject.Inject;

import com.cloud.storage.StorageManager;
import com.cloud.utils.Profiler;
import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreProvider;
import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreProviderManager;
import org.apache.cloudstack.engine.subsystem.api.storage.HypervisorHostListener;
Expand Down Expand Up @@ -144,51 +145,66 @@ public void processConnect(Host host, StartupCommand cmd, boolean forRebalance)
}

@Override
public synchronized boolean processDisconnect(long agentId, Status state) {
public boolean processDisconnect(long agentId, Status state) {
return processDisconnect(agentId, null, null, state);
}

@Override
public synchronized boolean processDisconnect(long agentId, String uuid, String name, Status state) {
public boolean processDisconnect(long agentId, String uuid, String name, Status state) {
logger.debug("Starting disconnect for Agent [id: {}, uuid: {}, name: {}]", agentId, uuid, name);
Host host = _storageManager.getHost(agentId);
if (host == null) {
logger.warn("Agent [id: {}, uuid: {}, name: {}] not found, not disconnecting pools", agentId, uuid, name);
return false;
}

if (host.getType() != Host.Type.Routing) {
logger.debug("Host [id: {}, uuid: {}, name: {}] is not of type {}, skip", agentId, uuid, name, Host.Type.Routing);
return false;
}

logger.debug("Looking for connected Storage Pools for Host [id: {}, uuid: {}, name: {}]", agentId, uuid, name);
List<StoragePoolHostVO> storagePoolHosts = _storageManager.findStoragePoolsConnectedToHost(host.getId());
if (storagePoolHosts == null) {
if (logger.isTraceEnabled()) {
logger.trace("No pools to disconnect for host: {}", host);
}
logger.debug("No pools to disconnect for host: {}", host);
return true;
}

logger.debug("Found {} pools to disconnect for host: {}", storagePoolHosts.size(), host);
boolean disconnectResult = true;
for (StoragePoolHostVO storagePoolHost : storagePoolHosts) {
int storagePoolHostsSize = storagePoolHosts.size();
for (int i = 0; i < storagePoolHostsSize; i++) {
StoragePoolHostVO storagePoolHost = storagePoolHosts.get(i);
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);
Comment on lines +206 to +207
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
Comment on lines +178 to +207
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
}
Comment on lines 198 to 208
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
}

Expand Down
Loading