linstor: grow cloned volumes inside the clone to fix resize race with BalanceAfterClone

plugins/storage/volume/linstor/src/main/java/org/apache/cloudstack/storage/datastore/driver/LinstorPrimaryDataStoreDriverImpl.java

@@ -251,7 +251,7 @@ private void deleteSnapshot(@Nonnull DataStore dataStore, @Nonnull String rscDef
 
         try
         {
-            ApiCallRcList answers = linstorApi.resourceSnapshotDelete(rscDefName, snapshotName, Collections.emptyList());
+            ApiCallRcList answers = linstorApi.resourceSnapshotDelete(rscDefName, snapshotName, Collections.emptyList(), null);
             if (answers.hasError())
             {
                 for (ApiCallRc answer : answers)
@@ -403,6 +403,29 @@ private void deleteTemplateForProps(
         }
     }
 
+    /**
+     * Request the target size as part of the clone if the controller supports it.
+     *
+     * A clone reports COMPLETE as soon as every replica can access UpToDate data, which with
+     * Clone/BalanceAfterClone is while the additional replica is still syncing. A resize issued at that
+     * point is rejected with "Cannot resize volume, because we have a non-UpToDate DRBD device".
+     * Controllers with REST API 1.29.1+ take the size in the clone request and grow the volume inside the
+     * clone before the balance placement, so the race cannot occur. Older controllers keep the
+     * clone-then-resize sequence.
+     *
+     * @return true if the caller still has to resize the resource after the clone finished
+     */
+    static boolean applyCloneSize(DevelopersApi api, ResourceDefinitionCloneRequest cloneRequest, Long sizeByte) {
+        if (sizeByte == null || sizeByte <= 0) {
+            return false;
+        }
+        if (LinstorUtil.supportsCloneVolumeSizes(api)) {
+            cloneRequest.setVolumeSizes(Collections.singletonList(sizeByte / 1024));
+            return false;
+        }
+        return true;
+    }
+
     private String cloneResource(long csCloneId, VolumeInfo volumeInfo, StoragePoolVO storagePoolVO) {
         // get the cached template on this storage
         VMTemplateStoragePoolVO tmplPoolRef = _vmTemplatePoolDao.findByPoolTemplate(
@@ -436,6 +459,7 @@ private String cloneResource(long csCloneId, VolumeInfo volumeInfo, StoragePoolV
                         cloneRequest.setVolumePassphrases(Collections.singletonList(utf8Passphrase));
                     }
                 }
+                final boolean resizeAfterClone = applyCloneSize(linstorApi, cloneRequest, volumeInfo.getSize());
                 ResourceDefinitionCloneStarted cloneStarted = linstorApi.resourceDefinitionClone(
                     cloneRes, cloneRequest);
 
@@ -447,7 +471,7 @@ private String cloneResource(long csCloneId, VolumeInfo volumeInfo, StoragePoolV
 
                 logger.info("Clone resource definition " + cloneRes + " to " + rscName + " finished");
 
-                if (volumeInfo.getSize() != null && volumeInfo.getSize() > 0) {
+                if (resizeAfterClone) {
                     resizeResource(linstorApi, rscName, volumeInfo.getSize());
                 }
 

plugins/storage/volume/linstor/src/main/java/org/apache/cloudstack/storage/datastore/util/LinstorUtil.java

@@ -101,6 +101,50 @@ public static DevelopersApi getLinstorAPI(String linstorUrl, String apiToken, bo
         return new DevelopersApi(client);
     }
 
+    /**
+     * The REST API version of the connected controller, e.g. "1.29.1", or null if it could not be queried.
+     */
+    @Nullable
+    public static String getRestApiVersion(DevelopersApi api) {
+        try {
+            return api.controllerVersion().getRestApiVersion();
+        } catch (ApiException apiExc) {
+            LOGGER.warn("Unable to query controller API version: {}", apiExc.getBestMessage());
+            return null;
+        }
+    }
+
+    /**
+     * Check if the connected controller accepts volume_sizes on a resource-definition clone request
+     * (REST API 1.29.1, LINSTOR 1.35.0). With it the grow happens inside the clone, before an optional
+     * Clone/BalanceAfterClone placement, so it cannot race the balance replica's sync.
+     */
+    public static boolean supportsCloneVolumeSizes(DevelopersApi api) {
+        return isVersionAtLeast(getRestApiVersion(api), 1, 29, 1);
+    }
+
+    static boolean isVersionAtLeast(String version, int major, int minor, int patch) {
+        if (version == null || version.isEmpty()) {
+            return false;
+        }
+        String[] parts = version.split("\\.");
+        try {
+            int maj = Integer.parseInt(parts[0]);
+            int min = parts.length > 1 ? Integer.parseInt(parts[1]) : 0;
+            int pat = parts.length > 2 ? Integer.parseInt(parts[2]) : 0;
+            if (maj != major) {
+                return maj > major;
+            }
+            if (min != minor) {
+                return min > minor;
+            }
+            return pat >= patch;
+        } catch (NumberFormatException nfExc) {
+            LOGGER.warn("Unable to parse controller API version '{}'", version);
+            return false;
+        }
+    }
+
     public static String getBestErrorMessage(ApiCallRcList answers) {
         return answers != null && !answers.isEmpty() ?
             answers.stream()

plugins/storage/volume/linstor/src/main/java/org/apache/cloudstack/storage/snapshot/LinstorVMSnapshotStrategy.java

@@ -214,7 +214,7 @@ private StrategyPriority allVolumesOnLinstor(Long vmId) {
     private String linstorDeleteSnapshot(final DevelopersApi api, final String rscName, final String snapshotName) {
         String resultMsg = null;
         try {
-            ApiCallRcList answers = api.resourceSnapshotDelete(rscName, snapshotName, Collections.emptyList());
+            ApiCallRcList answers = api.resourceSnapshotDelete(rscName, snapshotName, Collections.emptyList(), null);
             if (answers.hasError()) {
                 resultMsg = LinstorUtil.getBestErrorMessage(answers);
             }

plugins/storage/volume/linstor/src/test/java/org/apache/cloudstack/storage/datastore/driver/LinstorPrimaryDataStoreDriverImplTest.java

@@ -19,7 +19,9 @@
 import com.linbit.linstor.api.ApiException;
 import com.linbit.linstor.api.DevelopersApi;
 import com.linbit.linstor.api.model.AutoSelectFilter;
+import com.linbit.linstor.api.model.ControllerVersion;
 import com.linbit.linstor.api.model.LayerType;
+import com.linbit.linstor.api.model.ResourceDefinitionCloneRequest;
 import com.linbit.linstor.api.model.ResourceGroup;
 
 import java.util.Arrays;
@@ -35,6 +37,8 @@
 import org.mockito.junit.MockitoJUnitRunner;
 
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 @RunWith(MockitoJUnitRunner.class)
@@ -85,4 +89,45 @@ public void testGetEncryptedLayerList() throws ApiException  {
         layers = LinstorUtil.getEncryptedLayerList(api, "EncryptedGrp");
         Assert.assertEquals(Arrays.asList(LayerType.DRBD, LayerType.LUKS, LayerType.STORAGE), layers);
     }
+
+    private DevelopersApi mockApiWithRestVersion(String restApiVersion) throws ApiException {
+        DevelopersApi apiMock = mock(DevelopersApi.class);
+        ControllerVersion version = new ControllerVersion();
+        version.setRestApiVersion(restApiVersion);
+        when(apiMock.controllerVersion()).thenReturn(version);
+        return apiMock;
+    }
+
+    @Test
+    public void testApplyCloneSizeNewController() throws ApiException {
+        DevelopersApi newCtrl = mockApiWithRestVersion("1.29.1");
+        ResourceDefinitionCloneRequest req = new ResourceDefinitionCloneRequest();
+
+        boolean resizeAfter = LinstorPrimaryDataStoreDriverImpl.applyCloneSize(newCtrl, req, 40L * 1024 * 1024 * 1024);
+
+        Assert.assertFalse(resizeAfter);
+        Assert.assertEquals(Collections.singletonList(40L * 1024 * 1024), req.getVolumeSizes());
+    }
+
+    @Test
+    public void testApplyCloneSizeOldController() throws ApiException {
+        DevelopersApi oldCtrl = mockApiWithRestVersion("1.28.0");
+        ResourceDefinitionCloneRequest req = new ResourceDefinitionCloneRequest();
+
+        boolean resizeAfter = LinstorPrimaryDataStoreDriverImpl.applyCloneSize(oldCtrl, req, 40L * 1024 * 1024 * 1024);
+
+        Assert.assertTrue(resizeAfter);
+        Assert.assertNull(req.getVolumeSizes());
+    }
+
+    @Test
+    public void testApplyCloneSizeWithoutSize() throws ApiException {
+        DevelopersApi newCtrl = mockApiWithRestVersion("1.29.1");
+        ResourceDefinitionCloneRequest req = new ResourceDefinitionCloneRequest();
+
+        Assert.assertFalse(LinstorPrimaryDataStoreDriverImpl.applyCloneSize(newCtrl, req, null));
+        Assert.assertFalse(LinstorPrimaryDataStoreDriverImpl.applyCloneSize(newCtrl, req, 0L));
+        Assert.assertNull(req.getVolumeSizes());
+        verify(newCtrl, never()).controllerVersion();
+    }
 }

plugins/storage/volume/linstor/src/test/java/org/apache/cloudstack/storage/datastore/util/LinstorUtilTest.java

@@ -19,6 +19,7 @@
 import com.linbit.linstor.api.ApiException;
 import com.linbit.linstor.api.DevelopersApi;
 import com.linbit.linstor.api.model.AutoSelectFilter;
+import com.linbit.linstor.api.model.ControllerVersion;
 import com.linbit.linstor.api.model.Node;
 import com.linbit.linstor.api.model.Properties;
 import com.linbit.linstor.api.model.ProviderKind;
@@ -124,4 +125,57 @@ public void testGetRscGroupStoragePools() throws ApiException {
                 .collect(Collectors.toList());
         Assert.assertEquals(names, Arrays.asList("nodeA::thinpool", "nodeB::thinpool", "nodeC::thinpool"));
     }
+
+    @Test
+    public void testIsVersionAtLeast() {
+        Assert.assertTrue(LinstorUtil.isVersionAtLeast("1.29.1", 1, 29, 1));
+        Assert.assertTrue(LinstorUtil.isVersionAtLeast("1.29.2", 1, 29, 1));
+        Assert.assertTrue(LinstorUtil.isVersionAtLeast("1.30.0", 1, 29, 1));
+        Assert.assertTrue(LinstorUtil.isVersionAtLeast("2.0.0", 1, 29, 1));
+        Assert.assertTrue(LinstorUtil.isVersionAtLeast("1.30", 1, 29, 1));
+
+        Assert.assertFalse(LinstorUtil.isVersionAtLeast("1.29.0", 1, 29, 1));
+        Assert.assertFalse(LinstorUtil.isVersionAtLeast("1.29", 1, 29, 1));
+        Assert.assertFalse(LinstorUtil.isVersionAtLeast("1.28.5", 1, 29, 1));
+        Assert.assertFalse(LinstorUtil.isVersionAtLeast("0.99.9", 1, 29, 1));
+        Assert.assertFalse(LinstorUtil.isVersionAtLeast(null, 1, 29, 1));
+        Assert.assertFalse(LinstorUtil.isVersionAtLeast("", 1, 29, 1));
+        Assert.assertFalse(LinstorUtil.isVersionAtLeast("garbage", 1, 29, 1));
+    }
+
+    private DevelopersApi mockApi() {
+        return mock(DevelopersApi.class);
+    }
+
+    private ControllerVersion controllerVersion(String restApiVersion) {
+        ControllerVersion version = new ControllerVersion();
+        version.setRestApiVersion(restApiVersion);
+        return version;
+    }
+
+    @Test
+    public void testGetRestApiVersion() throws ApiException {
+        DevelopersApi ctrl = mockApi();
+        when(ctrl.controllerVersion()).thenReturn(controllerVersion("1.29.1"));
+        Assert.assertEquals("1.29.1", LinstorUtil.getRestApiVersion(ctrl));
+
+        DevelopersApi down = mockApi();
+        when(down.controllerVersion()).thenThrow(new ApiException(503, "unavailable"));
+        Assert.assertNull(LinstorUtil.getRestApiVersion(down));
+    }
+
+    @Test
+    public void testSupportsCloneVolumeSizes() throws ApiException {
+        DevelopersApi newCtrl = mockApi();
+        when(newCtrl.controllerVersion()).thenReturn(controllerVersion("1.29.1"));
+        Assert.assertTrue(LinstorUtil.supportsCloneVolumeSizes(newCtrl));
+
+        DevelopersApi oldCtrl = mockApi();
+        when(oldCtrl.controllerVersion()).thenReturn(controllerVersion("1.29.0"));
+        Assert.assertFalse(LinstorUtil.supportsCloneVolumeSizes(oldCtrl));
+
+        DevelopersApi unreachable = mockApi();
+        when(unreachable.controllerVersion()).thenThrow(new ApiException(503, "unavailable"));
+        Assert.assertFalse(LinstorUtil.supportsCloneVolumeSizes(unreachable));
+    }
 }

pom.xml

@@ -173,7 +173,7 @@
         <cs.nitro.version>10.1</cs.nitro.version>
         <cs.opensaml.version>2.6.6</cs.opensaml.version>
         <cs.rados-java.version>0.6.0</cs.rados-java.version>
-        <cs.java-linstor.version>0.7.0</cs.java-linstor.version>
+        <cs.java-linstor.version>0.8.1</cs.java-linstor.version>
         <cs.reflections.version>0.10.2</cs.reflections.version>
         <cs.servicemix.version>3.4.4_1</cs.servicemix.version>
         <cs.servlet.version>4.0.1</cs.servlet.version>

test/integration/plugins/linstor/test_linstor_volumes.py

@@ -857,7 +857,10 @@ def test_07_detach_volume_reboot_vm(self):
         # STEP 3: Reboot VM with detached vol #
         #######################################
 
-        self.virtual_machine.reboot(self.apiClient)
+        # via the helper, which waits for the guest to come back: CloudStack reports
+        # Running as soon as the domain exists, so leaving it un-awaited lets the next
+        # test attach and detach a volume against a guest that has not enumerated it yet
+        TestLinstorVolumes._reboot_vm(self.virtual_machine)
 
         vm = self._get_vm(self.virtual_machine.id)