From 4e82435455e1ffbe378f33606b35b7b730546908 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 21 Jan 2025 23:06:02 +0530 Subject: [PATCH 1/4] Delete local storage properties in agent.properties during delete pool --- .../resource/LibvirtComputingResource.java | 2 +- ...ibvirtDeleteStoragePoolCommandWrapper.java | 36 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index f9d56f8301d5..fd5ac365c3fd 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -431,7 +431,7 @@ public class LibvirtComputingResource extends ServerResourceBase implements Serv protected static final String LOCAL_STORAGE_PATH = "local.storage.path"; protected static final String LOCAL_STORAGE_UUID = "local.storage.uuid"; - protected static final String DEFAULT_LOCAL_STORAGE_PATH = "/var/lib/libvirt/images/"; + public static final String DEFAULT_LOCAL_STORAGE_PATH = "/var/lib/libvirt/images"; protected List localStoragePaths = new ArrayList<>(); protected List localStorageUUIDs = new ArrayList<>(); diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteStoragePoolCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteStoragePoolCommandWrapper.java index 716df4789f8b..99b398426ea3 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteStoragePoolCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteStoragePoolCommandWrapper.java @@ -22,12 +22,20 @@ import com.cloud.agent.api.Answer; import com.cloud.agent.api.DeleteStoragePoolCommand; import com.cloud.agent.api.to.StorageFilerTO; +import com.cloud.agent.dao.impl.PropertiesStorage; +import com.cloud.agent.properties.AgentProperties; +import com.cloud.agent.properties.AgentPropertiesFileHandler; import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; import com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager; import com.cloud.resource.CommandWrapper; import com.cloud.resource.ResourceWrapper; +import com.cloud.storage.Storage; import com.cloud.utils.exception.CloudRuntimeException; +import java.util.Arrays; +import java.util.HashMap; +import java.util.stream.Collectors; + @ResourceWrapper(handles = DeleteStoragePoolCommand.class) public final class LibvirtDeleteStoragePoolCommandWrapper extends CommandWrapper { @Override @@ -39,6 +47,34 @@ public Answer execute(final DeleteStoragePoolCommand command, final LibvirtCompu final KVMStoragePoolManager storagePoolMgr = libvirtComputingResource.getStoragePoolMgr(); storagePoolMgr.deleteStoragePool(pool.getType(), pool.getUuid()); + if (Storage.StoragePoolType.Filesystem.equals(pool.getType()) && !libvirtComputingResource.DEFAULT_LOCAL_STORAGE_PATH.equals(pool.getPath())) { + String localStoragePath = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.LOCAL_STORAGE_PATH); + String localStorageUuid = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.LOCAL_STORAGE_UUID); + + String uuidToRemove = pool.getUuid(); + String pathToRemove = pool.getPath(); + + if (localStorageUuid != null && uuidToRemove != null) { + localStorageUuid = Arrays.stream(localStorageUuid.split(",")) + .filter(uuid -> !uuid.equals(uuidToRemove)) + .collect(Collectors.joining(",")); + } + + if (localStoragePath != null && pathToRemove != null) { + localStoragePath = Arrays.stream(localStoragePath.split(",")) + .filter(path -> !path.equals(pathToRemove)) + .collect(Collectors.joining(",")); + } + + PropertiesStorage agentProperties = new PropertiesStorage(); + agentProperties.configure("AgentProperties", new HashMap()); + if (localStorageUuid != null) { + agentProperties.persist(AgentProperties.LOCAL_STORAGE_UUID.getName(), localStorageUuid); + } + if (localStoragePath != null) { + agentProperties.persist(AgentProperties.LOCAL_STORAGE_PATH.getName(), localStoragePath); + } + } } return new Answer(command); From 7e4860a63b8ebd0c6d03561b751352015eef8cde Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 21 Jan 2025 23:22:51 +0530 Subject: [PATCH 2/4] Fix stale entry when add local storage failed --- .../java/com/cloud/storage/StorageManagerImpl.java | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java index 36e0f582df8a..0a45fd448ad9 100644 --- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java @@ -803,7 +803,9 @@ public DataStore createLocalStorage(Host host, StoragePoolInfo pInfo) throws Con if (!(dc.isLocalStorageEnabled() || useLocalStorageForSystemVM)) { return null; } - DataStore store; + DataStore store = null; + DataStoreProvider provider = _dataStoreProviderMgr.getDefaultPrimaryDataStoreProvider(); + DataStoreLifeCycle lifeCycle = provider.getDataStoreLifeCycle(); try { String hostAddress = pInfo.getHost(); if (host.getHypervisorType() == Hypervisor.HypervisorType.VMware) { @@ -829,8 +831,6 @@ public DataStore createLocalStorage(Host host, StoragePoolInfo pInfo) throws Con } } - DataStoreProvider provider = _dataStoreProviderMgr.getDefaultPrimaryDataStoreProvider(); - DataStoreLifeCycle lifeCycle = provider.getDataStoreLifeCycle(); if (pool == null) { Map params = new HashMap(); String name = pInfo.getName() != null ? pInfo.getName() : createLocalStoragePoolName(host, pInfo); @@ -860,6 +860,14 @@ public DataStore createLocalStorage(Host host, StoragePoolInfo pInfo) throws Con } catch (Exception e) { s_logger.warn("Unable to setup the local storage pool for " + host, e); + try { + if (store != null) { + s_logger.debug(String.format("Trying to delete storage pool entry if exists %s", store)); + lifeCycle.deleteDataStore(store); + } + } catch (Exception ex) { + s_logger.debug(String.format("Failed to clean up local storage pool: %s", ex.getMessage())); + } throw new ConnectionException(true, "Unable to setup the local storage pool for " + host, e); } From 4246ab32884c57146e2be52893bea145f31cd9d8 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Wed, 22 Jan 2025 17:36:20 +0530 Subject: [PATCH 3/4] Smaller methods --- ...ibvirtDeleteStoragePoolCommandWrapper.java | 77 +++++++++++-------- 1 file changed, 45 insertions(+), 32 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteStoragePoolCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteStoragePoolCommandWrapper.java index 99b398426ea3..d3ff34aa6f0f 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteStoragePoolCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteStoragePoolCommandWrapper.java @@ -41,45 +41,58 @@ public final class LibvirtDeleteStoragePoolCommandWrapper extends CommandWrapper @Override public Answer execute(final DeleteStoragePoolCommand command, final LibvirtComputingResource libvirtComputingResource) { try { - // if getRemoveDatastore() is true, then we are dealing with managed storage and can skip the delete logic here if (!command.getRemoveDatastore()) { - final StorageFilerTO pool = command.getPool(); - final KVMStoragePoolManager storagePoolMgr = libvirtComputingResource.getStoragePoolMgr(); + handleStoragePoolDeletion(command, libvirtComputingResource); + } + return new Answer(command); + } catch (final CloudRuntimeException e) { + return new Answer(command, false, e.toString()); + } + } - storagePoolMgr.deleteStoragePool(pool.getType(), pool.getUuid()); - if (Storage.StoragePoolType.Filesystem.equals(pool.getType()) && !libvirtComputingResource.DEFAULT_LOCAL_STORAGE_PATH.equals(pool.getPath())) { - String localStoragePath = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.LOCAL_STORAGE_PATH); - String localStorageUuid = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.LOCAL_STORAGE_UUID); + private void handleStoragePoolDeletion(final DeleteStoragePoolCommand command, final LibvirtComputingResource libvirtComputingResource) { + final StorageFilerTO pool = command.getPool(); + final KVMStoragePoolManager storagePoolMgr = libvirtComputingResource.getStoragePoolMgr(); + storagePoolMgr.deleteStoragePool(pool.getType(), pool.getUuid()); - String uuidToRemove = pool.getUuid(); - String pathToRemove = pool.getPath(); + if (isLocalStorageAndNotHavingDefaultPath(pool, libvirtComputingResource)) { + updateLocalStorageProperties(pool); + } + } - if (localStorageUuid != null && uuidToRemove != null) { - localStorageUuid = Arrays.stream(localStorageUuid.split(",")) - .filter(uuid -> !uuid.equals(uuidToRemove)) - .collect(Collectors.joining(",")); - } + private boolean isLocalStorageAndNotHavingDefaultPath(final StorageFilerTO pool, final LibvirtComputingResource libvirtComputingResource) { + return Storage.StoragePoolType.Filesystem.equals(pool.getType()) + && !libvirtComputingResource.DEFAULT_LOCAL_STORAGE_PATH.equals(pool.getPath()); + } - if (localStoragePath != null && pathToRemove != null) { - localStoragePath = Arrays.stream(localStoragePath.split(",")) - .filter(path -> !path.equals(pathToRemove)) - .collect(Collectors.joining(",")); - } + private void updateLocalStorageProperties(final StorageFilerTO pool) { + String localStoragePath = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.LOCAL_STORAGE_PATH); + String localStorageUuid = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.LOCAL_STORAGE_UUID); - PropertiesStorage agentProperties = new PropertiesStorage(); - agentProperties.configure("AgentProperties", new HashMap()); - if (localStorageUuid != null) { - agentProperties.persist(AgentProperties.LOCAL_STORAGE_UUID.getName(), localStorageUuid); - } - if (localStoragePath != null) { - agentProperties.persist(AgentProperties.LOCAL_STORAGE_PATH.getName(), localStoragePath); - } - } - } + String uuidToRemove = pool.getUuid(); + String pathToRemove = pool.getPath(); - return new Answer(command); - } catch (final CloudRuntimeException e) { - return new Answer(command, false, e.toString()); + if (localStorageUuid != null && uuidToRemove != null) { + localStorageUuid = Arrays.stream(localStorageUuid.split(",")) + .filter(uuid -> !uuid.equals(uuidToRemove)) + .collect(Collectors.joining(",")); + } + + if (localStoragePath != null && pathToRemove != null) { + localStoragePath = Arrays.stream(localStoragePath.split(",")) + .filter(path -> !path.equals(pathToRemove)) + .collect(Collectors.joining(",")); + } + + PropertiesStorage agentProperties = new PropertiesStorage(); + agentProperties.configure("AgentProperties", new HashMap()); + + if (localStorageUuid != null) { + agentProperties.persist(AgentProperties.LOCAL_STORAGE_UUID.getName(), localStorageUuid); + } + + if (localStoragePath != null) { + agentProperties.persist(AgentProperties.LOCAL_STORAGE_PATH.getName(), localStoragePath); } } } From dcea74e6e2d3820cca6f968866881f592fe34614 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Wed, 22 Jan 2025 18:19:22 +0530 Subject: [PATCH 4/4] Comment added --- .../resource/wrapper/LibvirtDeleteStoragePoolCommandWrapper.java | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteStoragePoolCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteStoragePoolCommandWrapper.java index d3ff34aa6f0f..ad3ba80253ed 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteStoragePoolCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteStoragePoolCommandWrapper.java @@ -41,6 +41,7 @@ public final class LibvirtDeleteStoragePoolCommandWrapper extends CommandWrapper @Override public Answer execute(final DeleteStoragePoolCommand command, final LibvirtComputingResource libvirtComputingResource) { try { + // if getRemoveDatastore() is true, then we are dealing with managed storage and can skip the delete logic here if (!command.getRemoveDatastore()) { handleStoragePoolDeletion(command, libvirtComputingResource); }