From acae5c5b9ebf2961b05f4b092e8b6aaa7cca9fb4 Mon Sep 17 00:00:00 2001 From: Harikrishna Date: Tue, 11 Jun 2024 14:44:46 +0530 Subject: [PATCH] kvm: Update the java doc for the method disconnectPhysicalDiskByPath (#9210) This PR addresses the issue #8789 The original issue is disconnectPhysicalDiskByPath() implementation in FibreChannelAdaptor always returns true irrespective of the success of the operation. This was already fixed in the PR #8889 . Ideally this method has to be called after choosing the right adapter based on the storage pool type of the volume path, but currently it is just called in a loop. https://github.com/shapeblue/cloudstack/blob/05b9b6e2e77b9b633f0fe22aff5f9f3c047b2303/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java#L200-L212 while trying to fix the case of running into the loop of all adapters by somehow passing the storage pool type to that caller cleanup() method but this is touching all over the code (which I fear it creates other regressions), instead I feel we can keep it the current way only since Fibrechannel adapter has already fixed. In this PR I've added the java doc explaining the method and situation. --- .../hypervisor/kvm/storage/StorageAdaptor.java | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/StorageAdaptor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/StorageAdaptor.java index ecf8691c6ed..efb2821b9d9 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/StorageAdaptor.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/StorageAdaptor.java @@ -50,8 +50,17 @@ public interface StorageAdaptor { public boolean disconnectPhysicalDisk(Map volumeToDisconnect); - // given local path to file/device (per Libvirt XML), 1) check that device is - // handled by your adaptor, return false if not. 2) clean up device, return true + /** + * Given local path to file/device (per Libvirt XML), + * 1) Make sure to check that device is handled by your adaptor, return false if not. + * 2) clean up device, return true + * 3) if clean up fails, then return false + * + * If the method wrongly returns true, then there are chances that disconnect will not reach the right storage adapter + * + * @param localPath path for the file/device from the disk definition per Libvirt XML. + * @return true if the operation is successful; false if the operation fails or the adapter fails to handle the path. + */ public boolean disconnectPhysicalDiskByPath(String localPath); public boolean deletePhysicalDisk(String uuid, KVMStoragePool pool, Storage.ImageFormat format);