winterhazel commented on code in PR #13897:
URL: https://github.com/apache/cloudstack/pull/13897#discussion_r4332146114
##########
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](https://github.com/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`.
Yes, I was referring to the changes of #14187 as a whole.
> 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?
Limiting the umount to NFS is the right path, but I think that the actual
issue is KVMHAMonitor performing the umount here. Instead of reordering this
method and having to perform some additional calls to libvirt, what about this
approach?
- Keep the previous order of calls in this method.
- Split the operations in removeStoragePool() of KVMHAMonitor into two
methods, one for unregistering it from the HA map and another one for
unmounting pools that disappeared in runHeartBeat().
- Replace _haMonitor.removeStoragePool here with
_haMonitor.unregisterStoragePool().
- In this same method (deleteStoragePool), if the storage is NFS, call
adaptor.deleteStoragePool(uuid) instead of the no-op
adaptor.deleteStoragePool(uuid, details).
--
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]
