Add test, and propagate the modification of URL of primary store to the hosts only for nfs and gluster

Pearl Dsilva · Pearl Dsilva · commit eaffcd7be8a5 · 2026-05-20T16:34:37.000-04:00
diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/storage/UpdateStoragePoolCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/storage/UpdateStoragePoolCmd.java
@@ -73,7 +73,12 @@ public class UpdateStoragePoolCmd extends BaseCmd {
     @Parameter(name = ApiConstants.URL,
                             type = CommandType.STRING,
                             required = false,
-                            description = "the URL of the storage pool",
+                            description = "the URL of the storage pool. Supported only for NFS and Gluster pool type." +
+                                    "The pool must be in Maintenance mode before changing the URL. WARNING: use this parameter" +
+                                    "with caution. It is intended for failover scenarios where the storage content is already " +
+                                    "fully mirrored at the new location. Pointing to a new location without ensuring complete " +
+                                    "data parity will result in data loss or corruption. After the URL is updated, the new mount" +
+                                    "is applied to all connected hosts or restart the Management server",
                             since = "4.19.0")
     private String url;
 
diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java
@@ -766,6 +766,39 @@ public KVMStoragePool createStoragePool(String name, String host, int port, Stri
             path = path.substring(0, path.length() - 1);
         }
 
+        // For NFS and Gluster pools, if the existing pool's source host or path has changed
+        // (e.g. after a storage URL update), destroy and undefine it so it gets recreated
+        // with the new source below. Restricted to NFS/Gluster only.
+        if (sp != null && (type == StoragePoolType.NetworkFilesystem || type == StoragePoolType.Gluster)) {
+            try {
+                LibvirtStoragePoolDef existingDef = getStoragePoolDef(conn, sp);
+                if (existingDef != null) {
+                    String existingSourceHost = existingDef.getSourceHost();
+                    String existingSourceDir = existingDef.getSourceDir();
+                    boolean hostChanged = host != null && existingSourceHost != null && !existingSourceHost.equals(host);
+                    boolean pathChanged = path != null && existingSourceDir != null && !existingSourceDir.equals(path);
+                    if (hostChanged || pathChanged) {
+                        logger.info("Storage pool {} source has changed (host: {} -> {}, path: {} -> {}); destroying and redefining.",
+                                name, existingSourceHost, host, existingSourceDir, path);
+                        try {
+                            if (sp.isPersistent() == 1) {
+                                sp.destroy();
+                                sp.undefine();
+                            } else {
+                                sp.destroy();
+                            }
+                            sp.free();
+                            sp = null;
+                        } catch (LibvirtException destroyEx) {
+                            logger.error("Failed to destroy storage pool {} with changed source; will attempt to reuse existing pool: {}", name, destroyEx.getMessage());
+                        }
+                    }
+                }
+            } catch (LibvirtException e) {
+                logger.warn("Failed to check existing storage pool {} source path, will attempt to reuse it: {}", name, e.getMessage());
+            }
+        }
+
         if (sp == null) {
             // see if any existing pool by another name is using our storage path.
             // if anyone is, undefine the pool so we can define it as requested.
diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptorTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptorTest.java
@@ -42,9 +42,13 @@
 import com.cloud.hypervisor.kvm.resource.LibvirtConnection;
 import com.cloud.hypervisor.kvm.resource.LibvirtStoragePoolDef;
 import com.cloud.storage.Storage;
+import com.cloud.storage.StorageLayer;
 import com.cloud.utils.Pair;
 import com.cloud.utils.exception.CloudRuntimeException;
 import com.cloud.utils.script.Script;
+import org.libvirt.LibvirtException;
+
+import org.springframework.test.util.ReflectionTestUtils;
 
 @RunWith(MockitoJUnitRunner.class)
 public class LibvirtStorageAdaptorTest {
@@ -56,6 +60,9 @@ public class LibvirtStorageAdaptorTest {
     @Mock
     LibvirtStoragePool mockPool;
 
+    @Mock
+    StorageLayer storageLayer;
+
     MockedStatic<Script> mockScript;
 
     @Spy
@@ -67,6 +74,7 @@ public void initMocks() {
         libvirtConnectionMockedStatic = Mockito.mockStatic(LibvirtConnection.class);
         Mockito.reset(mockPool);
         mockScript = Mockito.mockStatic(Script.class);
+        ReflectionTestUtils.setField(libvirtStorageAdaptor, "_storageLayer", storageLayer);
     }
 
     @After
@@ -176,4 +184,171 @@ public void testUpdateLocalPoolIops_NullResultFromScript() {
 
         Mockito.verify(mockPool, never()).setUsedIops(anyLong());
     }
+
+    /**
+     * Creates a {@link LibvirtException} via reflection since its only constructor is package-private.
+     */
+    private static LibvirtException createLibvirtException(String message) throws Exception {
+        org.libvirt.Error virError = Mockito.mock(org.libvirt.Error.class);
+        Mockito.when(virError.getMessage()).thenReturn(message);
+        java.lang.reflect.Constructor<LibvirtException> ctor =
+                LibvirtException.class.getDeclaredConstructor(org.libvirt.Error.class);
+        ctor.setAccessible(true);
+        return ctor.newInstance(virError);
+    }
+
+    private String buildNfsPoolXml(String uuid, String host, String dir, String targetPath) {
+        return "<pool type='netfs'>\n" +
+                "<name>" + uuid + "</name>\n<uuid>" + uuid + "</uuid>\n" +
+                "<source>\n<host name='" + host + "'/>\n<dir path='" + dir + "'/>\n</source>\n" +
+                "<target>\n<path>" + targetPath + "</path>\n</target>\n</pool>\n";
+    }
+
+    /**
+     * NFS pool exists with same host and path: should reuse it without destroying.
+     */
+    @Test
+    public void testCreateStoragePool_NFS_SameHostAndPath_ReusesPool() throws Exception {
+        String uuid = UUID.randomUUID().toString();
+        String host = "10.0.0.1";
+        String path = "/export/primary";
+        String targetPath = "/mnt/" + uuid;
+        String poolXml = buildNfsPoolXml(uuid, host, path, targetPath);
+
+        Connect conn = Mockito.mock(Connect.class);
+        StoragePool sp = Mockito.mock(StoragePool.class);
+        Mockito.when(LibvirtConnection.getConnection()).thenReturn(conn);
+        Mockito.when(conn.storagePoolLookupByUUIDString(uuid)).thenReturn(sp);
+        Mockito.when(sp.isActive()).thenReturn(1);
+        Mockito.when(sp.getXMLDesc(0)).thenReturn(poolXml);
+        Mockito.when(Script.runSimpleBashScriptForExitValue(anyString())).thenReturn(0);
+        Mockito.doReturn(Mockito.mock(KVMStoragePool.class)).when(libvirtStorageAdaptor).getStoragePool(uuid);
+
+        libvirtStorageAdaptor.createStoragePool(uuid, host, 0, path, null,
+                Storage.StoragePoolType.NetworkFilesystem, null, true);
+
+        // pool was not destroyed since source didn't change
+        Mockito.verify(sp, Mockito.never()).destroy();
+        Mockito.verify(sp, Mockito.never()).undefine();
+    }
+
+    /**
+     * NFS pool exists but path has changed: should destroy and recreate.
+     */
+    @Test
+    public void testCreateStoragePool_NFS_PathChanged_DestroysAndRecreates() throws Exception {
+        String uuid = UUID.randomUUID().toString();
+        String host = "10.0.0.1";
+        String oldPath = "/export/old";
+        String newPath = "/export/new";
+        String targetPath = "/mnt/" + uuid;
+        String poolXml = buildNfsPoolXml(uuid, host, oldPath, targetPath);
+
+        Connect conn = Mockito.mock(Connect.class);
+        StoragePool sp = Mockito.mock(StoragePool.class);
+        Mockito.when(LibvirtConnection.getConnection()).thenReturn(conn);
+        Mockito.when(conn.storagePoolLookupByUUIDString(uuid)).thenReturn(sp);
+        Mockito.when(sp.isActive()).thenReturn(1);
+        Mockito.when(sp.getXMLDesc(0)).thenReturn(poolXml);
+        Mockito.when(sp.isPersistent()).thenReturn(1);
+        Mockito.when(conn.listStoragePools()).thenReturn(new String[]{});
+        StoragePool newSp = Mockito.mock(StoragePool.class);
+        Mockito.when(conn.storagePoolCreateXML(anyString(), Mockito.eq(0))).thenReturn(newSp);
+        Mockito.when(Script.runSimpleBashScriptForExitValue(anyString())).thenReturn(0);
+        Mockito.doReturn(Mockito.mock(KVMStoragePool.class)).when(libvirtStorageAdaptor).getStoragePool(uuid);
+
+        libvirtStorageAdaptor.createStoragePool(uuid, host, 0, newPath, null,
+                Storage.StoragePoolType.NetworkFilesystem, null, true);
+
+        Mockito.verify(sp).destroy();
+        Mockito.verify(sp).undefine();
+        Mockito.verify(sp).free();
+    }
+
+    /**
+     * NFS pool exists but host has changed: should destroy and recreate.
+     */
+    @Test
+    public void testCreateStoragePool_NFS_HostChanged_DestroysAndRecreates() throws Exception {
+        String uuid = UUID.randomUUID().toString();
+        String oldHost = "10.0.0.1";
+        String newHost = "10.0.0.2";
+        String path = "/export/primary";
+        String targetPath = "/mnt/" + uuid;
+        String poolXml = buildNfsPoolXml(uuid, oldHost, path, targetPath);
+
+        Connect conn = Mockito.mock(Connect.class);
+        StoragePool sp = Mockito.mock(StoragePool.class);
+        Mockito.when(LibvirtConnection.getConnection()).thenReturn(conn);
+        Mockito.when(conn.storagePoolLookupByUUIDString(uuid)).thenReturn(sp);
+        Mockito.when(sp.isActive()).thenReturn(1);
+        Mockito.when(sp.getXMLDesc(0)).thenReturn(poolXml);
+        Mockito.when(sp.isPersistent()).thenReturn(1);
+        Mockito.when(conn.listStoragePools()).thenReturn(new String[]{});
+        StoragePool newSp = Mockito.mock(StoragePool.class);
+        Mockito.when(conn.storagePoolCreateXML(anyString(), Mockito.eq(0))).thenReturn(newSp);
+        Mockito.when(Script.runSimpleBashScriptForExitValue(anyString())).thenReturn(0);
+        Mockito.doReturn(Mockito.mock(KVMStoragePool.class)).when(libvirtStorageAdaptor).getStoragePool(uuid);
+
+        libvirtStorageAdaptor.createStoragePool(uuid, newHost, 0, path, null,
+                Storage.StoragePoolType.NetworkFilesystem, null, true);
+
+        Mockito.verify(sp).destroy();
+        Mockito.verify(sp).undefine();
+        Mockito.verify(sp).free();
+    }
+
+    /**
+     * RBD pool exists with different source — should NOT destroy (RBD is excluded from this logic).
+     */
+    @Test
+    public void testCreateStoragePool_RBD_SourceChanged_DoesNotDestroy() throws Exception {
+        String uuid = UUID.randomUUID().toString();
+        String rbdPoolXml = "<pool type='rbd'>\n<name>" + uuid + "</name>\n<uuid>" + uuid + "</uuid>\n" +
+                "<source>\n<host name='10.0.0.1'/>\n<name>oldpool</name>\n</source>\n</pool>\n";
+
+        Connect conn = Mockito.mock(Connect.class);
+        StoragePool sp = Mockito.mock(StoragePool.class);
+        Mockito.when(LibvirtConnection.getConnection()).thenReturn(conn);
+        Mockito.when(conn.storagePoolLookupByUUIDString(uuid)).thenReturn(sp);
+        Mockito.when(sp.isActive()).thenReturn(1);
+        Mockito.doReturn(Mockito.mock(KVMStoragePool.class)).when(libvirtStorageAdaptor).getStoragePool(uuid);
+
+        libvirtStorageAdaptor.createStoragePool(uuid, "10.0.0.2", 6789, "newpool", "user:secret",
+                Storage.StoragePoolType.RBD, null, true);
+
+        Mockito.verify(sp, Mockito.never()).destroy();
+        Mockito.verify(sp, Mockito.never()).undefine();
+    }
+
+    /**
+     * NFS pool exists, destroy fails — pool should be reused (not crash), sp.free() not called.
+     */
+    @Test
+    public void testCreateStoragePool_NFS_DestroyFails_ReusesExistingPool() throws Exception {
+        String uuid = UUID.randomUUID().toString();
+        String host = "10.0.0.1";
+        String oldPath = "/export/old";
+        String newPath = "/export/new";
+        String targetPath = "/mnt/" + uuid;
+        String poolXml = buildNfsPoolXml(uuid, host, oldPath, targetPath);
+
+        Connect conn = Mockito.mock(Connect.class);
+        StoragePool sp = Mockito.mock(StoragePool.class);
+        Mockito.when(LibvirtConnection.getConnection()).thenReturn(conn);
+        Mockito.when(conn.storagePoolLookupByUUIDString(uuid)).thenReturn(sp);
+        Mockito.when(sp.isActive()).thenReturn(1);
+        Mockito.when(sp.getXMLDesc(0)).thenReturn(poolXml);
+        Mockito.when(sp.isPersistent()).thenReturn(1);
+        LibvirtException libvirtException = createLibvirtException("pool is busy");
+        Mockito.doThrow(libvirtException).when(sp).destroy();
+        Mockito.when(Script.runSimpleBashScriptForExitValue(anyString())).thenReturn(0);
+        Mockito.doReturn(Mockito.mock(KVMStoragePool.class)).when(libvirtStorageAdaptor).getStoragePool(uuid);
+
+        libvirtStorageAdaptor.createStoragePool(uuid, host, 0, newPath, null,
+                Storage.StoragePoolType.NetworkFilesystem, null, true);
+
+        Mockito.verify(sp).destroy();
+        Mockito.verify(sp, Mockito.never()).free();
+    }
 }
diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java
@@ -1304,31 +1304,90 @@ public PrimaryDataStoreInfo updateStoragePool(UpdateStoragePoolCmd cmd) throws I
                     _storagePoolDao.updateCapacityIops(id, updatedCapacityIops);
                 }
                 if (cmd.getUrl() != null) {
-                    if (!storagePool.isInMaintenance()) {
-                        throw new InvalidParameterValueException("Storage pool must be in Maintenance state before its URL can be changed. " +
-                                "Please put the pool into maintenance first.");
-                    }
-                    URI newUri;
-                    try {
-                        newUri = new URI(cmd.getUrl());
-                    } catch (URISyntaxException e) {
-                        throw new InvalidParameterValueException("Invalid URL format: " + cmd.getUrl());
-                    }
-                    storagePool.setHostAddress(newUri.getHost());
-                    storagePool.setPath(newUri.getPath());
-                    if (newUri.getPort() != -1) {
-                        storagePool.setPort(newUri.getPort());
-                    }
                     details.put("url", cmd.getUrl());
+
+                    // Updating host/path/port and propagating the remount to agents is only
+                    // supported for NFS and Gluster pools. For other types of storage pools, the URL is just informational and won't be used for actual connection, so we don't need to parse and propagate it.
+                    StoragePoolType poolType = storagePool.getPoolType();
+                    if (poolType == StoragePoolType.NetworkFilesystem || poolType == StoragePoolType.Gluster) {
+                        if (!storagePool.isInMaintenance()) {
+                            throw new InvalidParameterValueException("Storage pool must be in Maintenance state before its URL can be changed. " +
+                                    "Please put the pool into maintenance first.");
+                        }
+                        URI newUri;
+                        try {
+                            newUri = new URI(cmd.getUrl());
+                        } catch (URISyntaxException e) {
+                            throw new InvalidParameterValueException("Invalid URL format: " + cmd.getUrl());
+                        }
+                        storagePool.setHostAddress(newUri.getHost());
+                        storagePool.setPath(newUri.getPath());
+                        if (newUri.getPort() != -1) {
+                            storagePool.setPort(newUri.getPort());
+                        }
+                    }
                 }
                 _storagePoolDao.update(id, storagePool);
                 _storagePoolDao.updateDetails(id, details);
+                ((PrimaryDataStoreLifeCycle) dataStoreLifeCycle).updateStoragePool(storagePool, details);
+                StoragePoolType poolType = storagePool.getPoolType();
+                if (cmd.getUrl() != null &&
+                        (poolType == StoragePoolType.NetworkFilesystem || poolType == StoragePoolType.Gluster)) {
+                    propagateStoragePoolUrlChangeToHosts(storagePool);
+                }
             }
         }
 
         return (PrimaryDataStoreInfo)_dataStoreMgr.getDataStore(pool.getId(), DataStoreRole.Primary);
     }
 
+    /**
+     * Propagates a storage pool URL change to all currently connected hosts by sending
+     * a {@link ModifyStoragePoolCommand} with the updated connection details.
+     * This is called after the pool URL has been persisted to the DB while the pool is
+     * in Maintenance state (so no VMs are actively using it).
+     *
+     * @param updatedPool the {@link StoragePoolVO} whose hostAddress/path/port have already been updated
+     */
+    private void propagateStoragePoolUrlChangeToHosts(StoragePoolVO updatedPool) {
+        List<Long> poolIds = List.of(updatedPool.getId());
+        List<Long> connectedHostIds = _storagePoolHostDao.findHostsConnectedToPools(poolIds);
+        if (connectedHostIds.isEmpty()) {
+            logger.debug("No connected hosts found for storage pool [{}]; nothing to propagate for URL change.", updatedPool.getName());
+            return;
+        }
+
+        // Fetch fresh DataStore so that StoragePool delegates (getHostAddress, getPath, getPort)
+        // return the newly saved values.
+        StoragePool freshPool = (StoragePool) _dataStoreMgr.getDataStore(updatedPool.getId(), DataStoreRole.Primary);
+
+        Pair<Map<String, String>, Boolean> nfsMountOpts = getStoragePoolNFSMountOpts(freshPool, null);
+        Map<String, String> mountDetails = nfsMountOpts != null ? nfsMountOpts.first() : new java.util.HashMap<>();
+
+        logger.info("Propagating new URL for storage pool [{}] to {} connected host(s).", updatedPool.getName(), connectedHostIds.size());
+
+        for (Long hostId : connectedHostIds) {
+            HostVO host = _hostDao.findById(hostId);
+            if (host == null) {
+                logger.warn("Host [{}] not found; skipping URL propagation for pool [{}].", hostId, updatedPool.getName());
+                continue;
+            }
+
+            ModifyStoragePoolCommand cmd = new ModifyStoragePoolCommand(true, freshPool, mountDetails);
+            Answer answer = _agentMgr.easySend(hostId, cmd);
+
+            if (answer == null || !answer.getResult()) {
+                String detail = (answer == null) ? "no answer returned" : answer.getDetails();
+                logger.warn("Failed to propagate new URL for storage pool [{}] to host [{}]: {}",
+                        updatedPool.getName(), host.getName(), detail);
+            } else {
+                logger.info("Successfully propagated new URL for storage pool [{}] to host [{}].",
+                        updatedPool.getName(), host.getName());
+                updateStoragePoolHostVOAndBytes(freshPool, hostId, (ModifyStoragePoolAnswer) answer);
+            }
+        }
+    }
+
     private void changeStoragePoolScopeToZone(StoragePoolVO primaryStorage) {
         /*
          * For cluster wide primary storage the hypervisor type might not be set.
