sandeeplocharla commented on code in PR #13897:
URL: https://github.com/apache/cloudstack/pull/13897#discussion_r4322137510
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java:
##########
@@ -457,10 +457,12 @@ public synchronized boolean
deleteStoragePool(StoragePoolType type, String uuid)
public boolean deleteStoragePool(StoragePoolType type, String uuid,
Map<String, String> details) {
StorageAdaptor adaptor = getStorageAdaptor(type);
+ // For NetworkFilesystem, libvirt will take care of unmounting the nfs
mount. If nfs mount has been removed before libvirt's pool
+ // delete, libvirt will throw an error.
+ boolean deleteStatus = adaptor.deleteStoragePool(uuid, details);
Review Comment:
Hi @winterhazel, `_haMonitor.removeStoragePool(uuid)` is only called for
NetworkFilesystem, so that line is not what unmounts a SharedMountPoint pool.
The behaviour change comes from the new
LibvirtStorageAdaptor.deleteStoragePool(String, Map) override added in the file
below
`plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java`.
I see that NetworkFilesystem, SharedMountPoint, Gluster, Filesystem, and RBD
all use LibvirtStorageAdaptor. Before this override, that call used the
StorageAdaptor default, which returns true and does not destroy the libvirt
pool. The override forwards all five types to deleteStoragePool(uuid), so
entering maintenance now destroys the SharedMountPoint libvirt pool too. The
pool stays in the HA monitor, and the next heartbeat unmounts it.
The default is not enough for NetworkFilesystem, because libvirt has to
handle the pool. The real delete can stay limited to NetworkFilesystem, with
the other types left on the old no-op:
`@Override
public boolean deleteStoragePool(String uuid, Map<String, String> details) {
try {
KVMStoragePool pool = getStoragePool(uuid);
if (pool.getType() == StoragePoolType.NetworkFilesystem) {
return deleteStoragePool(uuid);
}
} catch (CloudRuntimeException e) {
logger.debug("Unable to fetch libvirt pool " + uuid + " before
delete: " + e);
}
return StorageAdaptor.super.deleteStoragePool(uuid, details);
}`
OR
`@Override
public boolean deleteStoragePool(String uuid, Map<String, String> details) {
try {
KVMStoragePool pool = getStoragePool(uuid);
if (pool.getType() == StoragePoolType.NetworkFilesystem) {
return deleteStoragePool(uuid);
}
} catch (CloudRuntimeException e) {
logger.debug("Unable to fetch libvirt pool " + uuid + " before
delete: " + e);
}
return true;
}`
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
