Fix local storage deletion cases (apache#10231)

* Delete local storage properties in agent.properties during delete pool

* Fix stale entry when add local storage failed

* Smaller methods

* Comment added

Author: harikrishna-patnala

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

@@ -459,7 +459,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<String> localStoragePaths = new ArrayList<>();
     protected List<String> localStorageUUIDs = new ArrayList<>();

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteStoragePoolCommandWrapper.java

@@ -22,28 +22,78 @@
 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<DeleteStoragePoolCommand, Answer, LibvirtComputingResource> {
     @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();
-
-                storagePoolMgr.deleteStoragePool(pool.getType(), pool.getUuid());
+                handleStoragePoolDeletion(command, libvirtComputingResource);
             }
-
             return new Answer(command);
         } catch (final CloudRuntimeException e) {
             return new Answer(command, false, e.toString());
         }
     }
+
+    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());
+
+        if (isLocalStorageAndNotHavingDefaultPath(pool, libvirtComputingResource)) {
+            updateLocalStorageProperties(pool);
+        }
+    }
+
+    private boolean isLocalStorageAndNotHavingDefaultPath(final StorageFilerTO pool, final LibvirtComputingResource libvirtComputingResource) {
+        return Storage.StoragePoolType.Filesystem.equals(pool.getType())
+                && !libvirtComputingResource.DEFAULT_LOCAL_STORAGE_PATH.equals(pool.getPath());
+    }
+
+    private void updateLocalStorageProperties(final StorageFilerTO pool) {
+        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<String, Object>());
+
+        if (localStorageUuid != null) {
+            agentProperties.persist(AgentProperties.LOCAL_STORAGE_UUID.getName(), localStorageUuid);
+        }
+
+        if (localStoragePath != null) {
+            agentProperties.persist(AgentProperties.LOCAL_STORAGE_PATH.getName(), localStoragePath);
+        }
+    }
 }

server/src/main/java/com/cloud/storage/StorageManagerImpl.java

@@ -808,7 +808,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) {
@@ -834,8 +836,6 @@ public DataStore createLocalStorage(Host host, StoragePoolInfo pInfo) throws Con
                 }
             }
 
-            DataStoreProvider provider = _dataStoreProviderMgr.getDefaultPrimaryDataStoreProvider();
-            DataStoreLifeCycle lifeCycle = provider.getDataStoreLifeCycle();
             if (pool == null) {
                 Map<String, Object> params = new HashMap<>();
                 String name = pInfo.getName() != null ? pInfo.getName() : createLocalStoragePoolName(host, pInfo);
@@ -865,6 +865,14 @@ public DataStore createLocalStorage(Host host, StoragePoolInfo pInfo) throws Con
 
         } catch (Exception e) {
             logger.warn("Unable to setup the local storage pool for " + host, e);
+            try {
+                if (store != null) {
+                    logger.debug(String.format("Trying to delete storage pool entry if exists %s", store));
+                    lifeCycle.deleteDataStore(store);
+                }
+            } catch (Exception ex) {
+                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);
         }