kvm: refresh the RBD pool when resizing a volume libvirt has not cached

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

@@ -120,7 +120,7 @@ public Answer execute(final ResizeVolumeCommand command, final LibvirtComputingR
                     final LibvirtUtilitiesHelper libvirtUtilitiesHelper = libvirtComputingResource.getLibvirtUtilitiesHelper();
 
                     final Connect conn = libvirtUtilitiesHelper.getConnection();
-                    final StorageVol v = conn.storageVolLookupByPath(path);
+                    final StorageVol v = libvirtUtilitiesHelper.getStorageVolumeByPath(conn, pool, path);
                     int flags = 0;
 
                     if (conn.getLibVirVersion() > 1001000 && vol.getFormat() == PhysicalDiskFormat.RAW && pool.getType() != StoragePoolType.RBD) {

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

@@ -30,11 +30,14 @@
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.LogManager;
 import org.libvirt.Connect;
+import org.libvirt.Error.ErrorNumber;
 import org.libvirt.LibvirtException;
+import org.libvirt.StorageVol;
 
 import com.cloud.agent.api.VMSnapshotTO;
 import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
 import com.cloud.hypervisor.kvm.resource.LibvirtConnection;
+import com.cloud.storage.Storage.StoragePoolType;
 import com.cloud.storage.StorageLayer;
 import com.cloud.storage.template.Processor;
 import com.cloud.storage.template.QCOW2Processor;
@@ -156,4 +159,31 @@ public KVMStoragePool getPrimaryPoolFromDataTo(DataTO dataTO, KVMStoragePoolMana
         return storageManager.getStoragePool(primaryDataStoreTO.getPoolType(), primaryDataStoreTO.getUuid());
     }
 
+    /**
+     * Looks a volume up through libvirt by its path.
+     *
+     * An RBD volume is created through librbd, and KVMStoragePool.getPhysicalDisk() looks RBD
+     * volumes up through librbd too, so nothing may have made libvirt refresh the pool since the
+     * volume was created, and the volume may be missing from libvirt's cache of the pool on this
+     * host. For an RBD pool, a lookup that fails because the volume is not found refreshes the
+     * pool and looks the volume up once more. Any other failure, and a failure on any other pool
+     * type, is thrown to the caller.
+     */
+    public StorageVol getStorageVolumeByPath(final Connect conn, final KVMStoragePool pool, final String path) throws LibvirtException {
+        try {
+            return conn.storageVolLookupByPath(path);
+        } catch (final LibvirtException e) {
+            if (pool.getType() != StoragePoolType.RBD || e.getError() == null
+                    || e.getError().getCode() != ErrorNumber.VIR_ERR_NO_STORAGE_VOL) {
+                throw e;
+            }
+            LOGGER.info("Volume " + path + " is not in libvirt's cache of RBD pool " + pool.getUuid() + " (" + e.getMessage()
+                    + "). Refreshing the pool and looking the volume up again.");
+            if (!pool.refresh()) {
+                LOGGER.warn("Failed to refresh RBD pool " + pool.getUuid() + ". Looking volume " + path + " up again anyway.");
+            }
+            return conn.storageVolLookupByPath(path);
+        }
+    }
+
 }

plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java

@@ -92,6 +92,7 @@
 import org.libvirt.DomainInfo;
 import org.libvirt.DomainInfo.DomainState;
 import org.libvirt.DomainInterfaceStats;
+import org.libvirt.Error.ErrorNumber;
 import org.libvirt.LibvirtException;
 import org.libvirt.MemoryStatistic;
 import org.libvirt.NodeInfo;
@@ -101,6 +102,7 @@
 import org.libvirt.jna.virDomainMemoryStats;
 import org.mockito.ArgumentCaptor;
 import org.mockito.BDDMockito;
+import org.mockito.InOrder;
 import org.mockito.Mock;
 import org.mockito.MockedConstruction;
 import org.mockito.MockedStatic;
@@ -175,6 +177,7 @@
 import com.cloud.agent.api.storage.CreateCommand;
 import com.cloud.agent.api.storage.DestroyCommand;
 import com.cloud.agent.api.storage.PrimaryStorageDownloadCommand;
+import com.cloud.agent.api.storage.ResizeVolumeAnswer;
 import com.cloud.agent.api.storage.ResizeVolumeCommand;
 import com.cloud.agent.api.to.DataStoreTO;
 import com.cloud.agent.api.to.DiskTO;
@@ -4803,7 +4806,7 @@ public void testResizeVolumeCommand() {
         when(libvirtComputingResourceMock.getLibvirtUtilitiesHelper()).thenReturn(libvirtUtilitiesHelper);
         try {
             when(libvirtUtilitiesHelper.getConnection()).thenReturn(conn);
-            when(conn.storageVolLookupByPath(path)).thenReturn(v);
+            when(libvirtUtilitiesHelper.getStorageVolumeByPath(conn, storagePool, path)).thenReturn(v);
             when(libvirtUtilitiesHelper.getConnectionByVmName(vmInstance)).thenReturn(conn);
             when(conn.domainLookupByName(vmInstance)).thenReturn(vm);
             when(vm.getInfo()).thenReturn(info);
@@ -4830,6 +4833,103 @@ public void testResizeVolumeCommand() {
         }
     }
 
+    private static final String RBD_RESIZE_PATH = "cloudstack/0d3c2bd6-cb39-4c8b-a1d9-0e7fbd3b8a5b";
+
+    /**
+     * Sets up a resize of a RAW volume at {@link #RBD_RESIZE_PATH} on an RBD pool, up to the point
+     * where the wrapper looks the volume up through {@code libvirtUtilitiesHelper} on {@code conn}.
+     */
+    private ResizeVolumeCommand prepareRbdResizeLookup(final KVMStoragePool storagePool, final KVMPhysicalDisk vol,
+            final LibvirtUtilitiesHelper libvirtUtilitiesHelper, final Connect conn) throws LibvirtException {
+        final StorageFilerTO pool = Mockito.mock(StorageFilerTO.class);
+        final KVMStoragePoolManager storagePoolMgr = Mockito.mock(KVMStoragePoolManager.class);
+
+        when(pool.getType()).thenReturn(StoragePoolType.RBD);
+        when(libvirtComputingResourceMock.getStoragePoolMgr()).thenReturn(storagePoolMgr);
+        when(storagePoolMgr.getStoragePool(pool.getType(), pool.getUuid())).thenReturn(storagePool);
+        when(storagePool.getPhysicalDisk(RBD_RESIZE_PATH)).thenReturn(vol);
+        when(storagePool.getType()).thenReturn(StoragePoolType.RBD);
+        when(vol.getPath()).thenReturn(RBD_RESIZE_PATH);
+        when(vol.getFormat()).thenReturn(PhysicalDiskFormat.RAW);
+        when(libvirtComputingResourceMock.getLibvirtUtilitiesHelper()).thenReturn(libvirtUtilitiesHelper);
+        Mockito.doReturn(conn).when(libvirtUtilitiesHelper).getConnection();
+        // Only reached when the resize gets as far as telling the VM about the new size.
+        Mockito.lenient().doReturn(conn).when(libvirtUtilitiesHelper).getConnectionByVmName("Test");
+
+        return new ResizeVolumeCommand(RBD_RESIZE_PATH, pool, 100L, 200L, false, "Test", null);
+    }
+
+    /**
+     * An RBD volume created through librbd is not in libvirt's cache of the pool until the pool is
+     * refreshed, so the first lookup by path misses. The resize looks the volume up through
+     * LibvirtUtilitiesHelper.getStorageVolumeByPath(), which refreshes the pool and finds it. The
+     * resize must then resize the volume and tell the running VM about the new size.
+     */
+    @Test
+    public void testResizeVolumeCommandRbdVolumeNotInLibvirtPoolCache() throws LibvirtException {
+        final KVMStoragePool storagePool = Mockito.mock(KVMStoragePool.class);
+        final KVMPhysicalDisk vol = Mockito.mock(KVMPhysicalDisk.class);
+        final LibvirtUtilitiesHelper libvirtUtilitiesHelper = Mockito.spy(LibvirtUtilitiesHelper.class);
+        final Connect conn = Mockito.mock(Connect.class);
+        final StorageVol v = Mockito.mock(StorageVol.class);
+        final Domain vm = Mockito.mock(Domain.class);
+        final DomainInfo info = Mockito.mock(DomainInfo.class);
+        info.state = DomainInfo.DomainState.VIR_DOMAIN_RUNNING;
+
+        final ResizeVolumeCommand command = prepareRbdResizeLookup(storagePool, vol, libvirtUtilitiesHelper, conn);
+        final org.libvirt.Error error = Mockito.mock(org.libvirt.Error.class);
+        when(error.getCode()).thenReturn(ErrorNumber.VIR_ERR_NO_STORAGE_VOL);
+        final LibvirtException notFound = Mockito.mock(LibvirtException.class);
+        when(notFound.getError()).thenReturn(error);
+        when(conn.storageVolLookupByPath(RBD_RESIZE_PATH)).thenThrow(notFound).thenReturn(v);
+        when(storagePool.refresh()).thenReturn(true);
+        when(conn.getLibVirVersion()).thenReturn(10010L);
+        when(conn.domainLookupByName("Test")).thenReturn(vm);
+        when(vm.getInfo()).thenReturn(info);
+        when(vol.getVirtualSize()).thenReturn(200L);
+
+        // The resize script succeeds (a mocked Script.execute() returns null), so the answer comes from the size read back after the resize.
+        try (MockedConstruction<Script> scripts = Mockito.mockConstruction(Script.class)) {
+            final Answer answer = LibvirtRequestWrapper.getInstance().execute(command, libvirtComputingResourceMock);
+            assertTrue(answer.getResult());
+            assertEquals("success", answer.getDetails());
+            assertEquals(200L, ((ResizeVolumeAnswer) answer).getNewSize());
+
+            assertEquals(1, scripts.constructed().size());
+            final Script script = scripts.constructed().get(0);
+            final InOrder inOrder = Mockito.inOrder(libvirtUtilitiesHelper, conn, storagePool, v, script);
+            inOrder.verify(libvirtUtilitiesHelper).getStorageVolumeByPath(conn, storagePool, RBD_RESIZE_PATH);
+            inOrder.verify(conn).storageVolLookupByPath(RBD_RESIZE_PATH);
+            inOrder.verify(storagePool).refresh();
+            inOrder.verify(conn).storageVolLookupByPath(RBD_RESIZE_PATH);
+            inOrder.verify(v).resize(200L, 0);
+            inOrder.verify(script).add("-t", LibvirtComputingResource.RESIZE_NOTIFY_ONLY);
+            inOrder.verify(script).execute();
+        }
+    }
+
+    /**
+     * If the volume cannot be looked up through libvirt, the resize fails without running the
+     * resize script.
+     */
+    @Test
+    public void testResizeVolumeCommandVolumeLookupFails() throws LibvirtException {
+        final KVMStoragePool storagePool = Mockito.mock(KVMStoragePool.class);
+        final KVMPhysicalDisk vol = Mockito.mock(KVMPhysicalDisk.class);
+        final LibvirtUtilitiesHelper libvirtUtilitiesHelper = Mockito.mock(LibvirtUtilitiesHelper.class);
+        final Connect conn = Mockito.mock(Connect.class);
+
+        final ResizeVolumeCommand command = prepareRbdResizeLookup(storagePool, vol, libvirtUtilitiesHelper, conn);
+        when(libvirtUtilitiesHelper.getStorageVolumeByPath(conn, storagePool, RBD_RESIZE_PATH)).thenThrow(LibvirtException.class);
+
+        try (MockedConstruction<Script> scripts = Mockito.mockConstruction(Script.class)) {
+            final Answer answer = LibvirtRequestWrapper.getInstance().execute(command, libvirtComputingResourceMock);
+            assertFalse(answer.getResult());
+            assertTrue(scripts.constructed().isEmpty());
+        }
+        verify(libvirtUtilitiesHelper, times(1)).getStorageVolumeByPath(conn, storagePool, RBD_RESIZE_PATH);
+    }
+
     @Test
     public void testResizeVolumeCommandLinstorNotifyOnly() {
         final String path = "/dev/drbd1000";

plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtUtilitiesHelperTest.java

@@ -20,12 +20,17 @@
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.libvirt.Connect;
+import org.libvirt.Error.ErrorNumber;
 import org.libvirt.LibvirtException;
+import org.libvirt.StorageVol;
+import org.mockito.InOrder;
 import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.junit.MockitoJUnitRunner;
 
 import com.cloud.agent.api.VMSnapshotTO;
+import com.cloud.hypervisor.kvm.storage.KVMStoragePool;
+import com.cloud.storage.Storage.StoragePoolType;
 import com.cloud.utils.Pair;
 
 import junit.framework.TestCase;
@@ -38,6 +43,12 @@ public class LibvirtUtilitiesHelperTest extends TestCase {
     @Mock
     Connect connectMock;
 
+    @Mock
+    KVMStoragePool storagePoolMock;
+
+    @Mock
+    StorageVol storageVolMock;
+
     @Test
     public void validateIsLibvirtVersionEqualOrHigherThanVersionInParameterExceptionOnRetrievingLibvirtVersionReturnsFalse() throws LibvirtException {
         Mockito.doThrow(LibvirtException.class).when(connectMock).getLibVirVersion();
@@ -102,4 +113,123 @@ public void generateVMSnapshotXMLEscapesParentSnapshotName() {
         Assert.assertTrue(xml.contains("<parent><name>parent&lt;/name&gt;&lt;disks/&gt;</name></parent>"));
         Assert.assertFalse(xml.contains("<disks/>"));
     }
+
+    private static final String RBD_VOLUME_PATH = "cloudstack/0d3c2bd6-cb39-4c8b-a1d9-0e7fbd3b8a5b";
+
+    // Not a spy: a spy rewrites the stack trace of exceptions passing through it, which a mocked LibvirtException does not have.
+    private final LibvirtUtilitiesHelper libvirtUtilitiesHelper = new LibvirtUtilitiesHelper();
+
+    private static LibvirtException libvirtException(final ErrorNumber code) {
+        final org.libvirt.Error error = Mockito.mock(org.libvirt.Error.class);
+        Mockito.when(error.getCode()).thenReturn(code);
+        final LibvirtException e = Mockito.mock(LibvirtException.class);
+        Mockito.when(e.getError()).thenReturn(error);
+        return e;
+    }
+
+    @Test
+    public void getStorageVolumeByPathVolumeInLibvirtPoolCacheDoesNotRefresh() throws LibvirtException {
+        Mockito.when(connectMock.storageVolLookupByPath(RBD_VOLUME_PATH)).thenReturn(storageVolMock);
+
+        Assert.assertSame(storageVolMock, libvirtUtilitiesHelper.getStorageVolumeByPath(connectMock, storagePoolMock, RBD_VOLUME_PATH));
+
+        Mockito.verify(connectMock, Mockito.times(1)).storageVolLookupByPath(RBD_VOLUME_PATH);
+        Mockito.verify(storagePoolMock, Mockito.never()).refresh();
+    }
+
+    /**
+     * An RBD volume created through librbd is not in libvirt's cache of the pool until the pool is
+     * refreshed, so the first lookup by path misses. The pool is refreshed and the volume is
+     * looked up once more.
+     */
+    @Test
+    public void getStorageVolumeByPathRbdVolumeNotInLibvirtPoolCacheRefreshesPool() throws LibvirtException {
+        final LibvirtException notFound = libvirtException(ErrorNumber.VIR_ERR_NO_STORAGE_VOL);
+        Mockito.when(storagePoolMock.getType()).thenReturn(StoragePoolType.RBD);
+        Mockito.when(connectMock.storageVolLookupByPath(RBD_VOLUME_PATH)).thenThrow(notFound).thenReturn(storageVolMock);
+        Mockito.when(storagePoolMock.refresh()).thenReturn(true);
+
+        Assert.assertSame(storageVolMock, libvirtUtilitiesHelper.getStorageVolumeByPath(connectMock, storagePoolMock, RBD_VOLUME_PATH));
+
+        InOrder inOrder = Mockito.inOrder(connectMock, storagePoolMock);
+        inOrder.verify(connectMock).storageVolLookupByPath(RBD_VOLUME_PATH);
+        inOrder.verify(storagePoolMock).refresh();
+        inOrder.verify(connectMock).storageVolLookupByPath(RBD_VOLUME_PATH);
+    }
+
+    /**
+     * If the RBD volume is still missing after the pool is refreshed, the lookup fails after a
+     * single refresh and a single retry.
+     */
+    @Test
+    public void getStorageVolumeByPathRbdVolumeNotFoundAfterRefreshThrows() throws LibvirtException {
+        assertRbdLookupRetriedOnce(true);
+    }
+
+    /**
+     * A refresh that fails does not stop the retry.
+     */
+    @Test
+    public void getStorageVolumeByPathRbdVolumeRetriedAfterFailedRefresh() throws LibvirtException {
+        assertRbdLookupRetriedOnce(false);
+    }
+
+    private void assertRbdLookupRetriedOnce(final boolean refreshSucceeds) throws LibvirtException {
+        final LibvirtException notFound = libvirtException(ErrorNumber.VIR_ERR_NO_STORAGE_VOL);
+        Mockito.when(storagePoolMock.getType()).thenReturn(StoragePoolType.RBD);
+        Mockito.when(connectMock.storageVolLookupByPath(RBD_VOLUME_PATH)).thenThrow(notFound);
+        Mockito.when(storagePoolMock.refresh()).thenReturn(refreshSucceeds);
+
+        try {
+            libvirtUtilitiesHelper.getStorageVolumeByPath(connectMock, storagePoolMock, RBD_VOLUME_PATH);
+            Assert.fail("Expected the lookup to fail");
+        } catch (final LibvirtException e) {
+            Assert.assertSame(notFound, e);
+        }
+
+        Mockito.verify(connectMock, Mockito.times(2)).storageVolLookupByPath(RBD_VOLUME_PATH);
+        Mockito.verify(storagePoolMock, Mockito.times(1)).refresh();
+    }
+
+    /**
+     * A lookup on an RBD pool that fails for another reason than the volume not being found
+     * fails without refreshing the pool.
+     */
+    @Test
+    public void getStorageVolumeByPathRbdOtherLookupErrorDoesNotRefresh() throws LibvirtException {
+        assertLookupFailsWithoutRefresh(StoragePoolType.RBD, RBD_VOLUME_PATH, libvirtException(ErrorNumber.VIR_ERR_INVALID_CONN));
+    }
+
+    /**
+     * A LibvirtException without an error attached is not taken for a missing volume.
+     */
+    @Test
+    public void getStorageVolumeByPathRbdLookupErrorWithoutCodeDoesNotRefresh() throws LibvirtException {
+        assertLookupFailsWithoutRefresh(StoragePoolType.RBD, RBD_VOLUME_PATH, Mockito.mock(LibvirtException.class));
+    }
+
+    /**
+     * A missing volume on a pool that is not RBD fails without refreshing the pool.
+     */
+    @Test
+    public void getStorageVolumeByPathNonRbdVolumeNotFoundDoesNotRefresh() throws LibvirtException {
+        assertLookupFailsWithoutRefresh(StoragePoolType.NetworkFilesystem, "/mnt/0d3c2bd6-cb39-4c8b-a1d9-0e7fbd3b8a5b/volume",
+                libvirtException(ErrorNumber.VIR_ERR_NO_STORAGE_VOL));
+    }
+
+    private void assertLookupFailsWithoutRefresh(final StoragePoolType poolType, final String path, final LibvirtException lookupError)
+            throws LibvirtException {
+        Mockito.when(storagePoolMock.getType()).thenReturn(poolType);
+        Mockito.when(connectMock.storageVolLookupByPath(path)).thenThrow(lookupError);
+
+        try {
+            libvirtUtilitiesHelper.getStorageVolumeByPath(connectMock, storagePoolMock, path);
+            Assert.fail("Expected the lookup to fail");
+        } catch (final LibvirtException e) {
+            Assert.assertSame(lookupError, e);
+        }
+
+        Mockito.verify(connectMock, Mockito.times(1)).storageVolLookupByPath(path);
+        Mockito.verify(storagePoolMock, Mockito.never()).refresh();
+    }
 }