kvm: use preallocation option for fat disk resize (#11986)

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

plugins/hypervisors/kvm/pom.xml

@@ -107,15 +107,6 @@
                     </execution>
                 </executions>
             </plugin>
-            <plugin>
-                <groupId>org.apache.maven.plugins</groupId>
-                <artifactId>maven-surefire-plugin</artifactId>
-                <configuration>
-                    <excludes>
-                        <exclude>**/QemuImg*.java</exclude>
-                    </excludes>
-                </configuration>
-            </plugin>
         </plugins>
     </build>
     <profiles>

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java

@@ -1081,7 +1081,7 @@ public class LibvirtStorageAdaptor implements StorageAdaptor {
         destFile.setSize(size);
         Map<String, String> options = new HashMap<String, String>();
         if (List.of(StoragePoolType.NetworkFilesystem, StoragePoolType.Filesystem).contains(pool.getType())) {
-            options.put("preallocation", QemuImg.PreallocationType.getPreallocationType(provisioningType).toString());
+            options.put(QemuImg.PREALLOCATION, QemuImg.PreallocationType.getPreallocationType(provisioningType).toString());
         }
 
         try (KeyFile keyFile = new KeyFile(passphrase)) {

plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/utils/qemu/QemuImg.java

@@ -25,6 +25,7 @@ import java.util.Map;
 import java.util.regex.Pattern;
 
 import org.apache.cloudstack.storage.formatinspector.Qcow2Inspector;
+import org.apache.commons.collections.MapUtils;
 import org.apache.commons.lang.NotImplementedException;
 import org.apache.commons.lang3.StringUtils;
 import org.libvirt.LibvirtException;
@@ -51,6 +52,7 @@ public class QemuImg {
     public static final String ENCRYPT_FORMAT = "encrypt.format";
     public static final String ENCRYPT_KEY_SECRET = "encrypt.key-secret";
     public static final String TARGET_ZERO_FLAG = "--target-is-zero";
+    public static final String PREALLOCATION = "preallocation";
     public static final long QEMU_2_10 = 2010000;
     public static final long QEMU_5_10 = 5010000;
 
@@ -393,6 +395,17 @@ public class QemuImg {
         convert(srcFile, destFile, options, qemuObjects, srcImageOpts, snapshotName, forceSourceFormat, false);
     }
 
+    protected Map<String, String> getResizeOptionsFromConvertOptions(final Map<String, String> options) {
+        if (MapUtils.isEmpty(options)) {
+            return null;
+        }
+        Map<String, String> resizeOpts = new HashMap<>();
+        if (options.containsKey(PREALLOCATION)) {
+            resizeOpts.put(PREALLOCATION, options.get(PREALLOCATION));
+        }
+        return resizeOpts;
+    }
+
     /**
      * Converts an image from source to destination.
      *
@@ -420,7 +433,6 @@ public class QemuImg {
     public void convert(final QemuImgFile srcFile, final QemuImgFile destFile,
                         final Map<String, String> options, final List<QemuObject> qemuObjects, final QemuImageOptions srcImageOpts, final String snapshotName, final boolean forceSourceFormat,
                         boolean keepBitmaps) throws QemuImgException {
-
         Script script = new Script(_qemuImgPath, timeout);
         if (StringUtils.isNotBlank(snapshotName)) {
             String qemuPath = Script.runSimpleBashScript(getQemuImgPathScript);
@@ -484,7 +496,7 @@ public class QemuImg {
         }
 
         if (srcFile.getSize() < destFile.getSize()) {
-            this.resize(destFile, destFile.getSize());
+            this.resize(destFile, destFile.getSize(), getResizeOptionsFromConvertOptions(options));
         }
     }
 
@@ -691,8 +703,10 @@ public class QemuImg {
         }
     }
 
-    private void addScriptOptionsFromMap(Map<String, String> options, Script s) {
+    protected void addScriptOptionsFromMap(Map<String, String> options, Script s) {
-        if (options != null && !options.isEmpty()) {
+        if (MapUtils.isEmpty(options)) {
+            return;
+        }
         s.add("-o");
         final StringBuffer optionsBuffer = new StringBuffer();
         for (final Map.Entry<String, String> option : options.entrySet()) {
@@ -702,6 +716,16 @@ public class QemuImg {
         optionsStr = optionsStr.replaceAll(",$", "");
         s.add(optionsStr);
     }
+
+    protected void addScriptResizeOptionsFromMap(Map<String, String> options, Script s) {
+        if (MapUtils.isEmpty(options)) {
+            return;
+        }
+        if (options.containsKey(PREALLOCATION)) {
+            s.add(String.format("--%s=%s", PREALLOCATION, options.get(PREALLOCATION)));
+            options.remove(PREALLOCATION);
+        }
+        addScriptOptionsFromMap(options, s);
     }
 
     /**
@@ -747,19 +771,17 @@ public class QemuImg {
 
     /**
      * Resizes an image.
-     *
+     * <p>
      * This method is a facade for 'qemu-img resize'.
-     *
+     * <p>
      * A negative size value will get prefixed with '-' and a positive with '+'. Sizes are in bytes and will be passed on that way.
      *
-     * @param file
+     * @param file      The file to be resized.
-     *            The file to be resized.
+     * @param size      The new size.
-     * @param size
+     * @param delta     Flag to inform if the new size is a delta.
-     *            The new size.
+     * @param options   Script options for resizing. Takes a Map<String, String> with key value
-     * @param delta
-     *            Flag to inform if the new size is a delta.
      */
-    public void resize(final QemuImgFile file, final long size, final boolean delta) throws QemuImgException {
+    public void resize(final QemuImgFile file, final long size, final boolean delta, Map<String, String> options) throws QemuImgException {
         String newSize = null;
 
         if (size == 0) {
@@ -781,6 +803,7 @@ public class QemuImg {
 
         final Script s = new Script(_qemuImgPath);
         s.add("resize");
+        addScriptResizeOptionsFromMap(options, s);
         s.add(file.getFileName());
         s.add(newSize);
         s.execute();
@@ -789,7 +812,7 @@ public class QemuImg {
     /**
      * Resizes an image.
      *
-     * This method is a facade for {@link QemuImg#resize(QemuImgFile, long, boolean)}.
+     * This method is a facade for {@link QemuImg#resize(QemuImgFile, long, boolean, Map)}.
      *
      * A negative size value will get prefixed with - and a positive with +. Sizes are in bytes and will be passed on that way.
      *
@@ -818,18 +841,17 @@ public class QemuImg {
 
     /**
      * Resizes an image.
-     *
+     * <p>
-     * This method is a facade for {@link QemuImg#resize(QemuImgFile, long, boolean)}.
+     * This method is a facade for {@link QemuImg#resize(QemuImgFile, long, boolean, Map)}.
-     *
+     * <p>
      * A negative size value will get prefixed with - and a positive with +. Sizes are in bytes and will be passed on that way.
      *
-     * @param file
+     * @param file      The file to be resized.
-     *            The file to be resized.
+     * @param size      The new size.
-     * @param size
+     * @param options   Script options for resizing. Takes a Map<String, String> with key value
-     *            The new size.
      */
-    public void resize(final QemuImgFile file, final long size) throws QemuImgException {
+    public void resize(final QemuImgFile file, final long size, Map<String, String> options) throws QemuImgException {
-        this.resize(file, size, false);
+        this.resize(file, size, false, options);
     }
 
     /**

plugins/hypervisors/kvm/src/test/java/org/apache/cloudstack/utils/qemu/QemuImgTest.java

@@ -22,26 +22,45 @@ import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 import java.io.File;
-import com.cloud.utils.script.Script;
-
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.UUID;
 
+import org.apache.cloudstack.utils.qemu.QemuImg.PhysicalDiskFormat;
+import org.apache.commons.collections.MapUtils;
 import org.junit.Assert;
+import org.junit.Assume;
+import org.junit.BeforeClass;
 import org.junit.Ignore;
 import org.junit.Test;
-
+import org.junit.runner.RunWith;
-import org.apache.cloudstack.utils.qemu.QemuImg.PhysicalDiskFormat;
+import org.libvirt.Connect;
 import org.libvirt.LibvirtException;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
 
-@Ignore
+import com.cloud.utils.script.Script;
+
+@RunWith(MockitoJUnitRunner.class)
 public class QemuImgTest {
 
+    @BeforeClass
+    public static void setUp() {
+        Assume.assumeTrue("qemu-img not found", Script.runSimpleBashScript("command -v qemu-img") != null);
+        boolean libVirtAvailable = false;
+        try {
+            Connect conn = new Connect("qemu:///system", false);
+            conn.getVersion();
+            libVirtAvailable = true;
+        } catch (LibvirtException ignored) {}
+        Assume.assumeTrue("libvirt not available", libVirtAvailable);
+    }
+
     @Test
     public void testCreateAndInfo() throws QemuImgException, LibvirtException {
         String filename = "/tmp/" + UUID.randomUUID() + ".qcow2";
@@ -130,8 +149,7 @@ public class QemuImgTest {
     public void testCreateSparseVolume() throws QemuImgException, LibvirtException {
         String filename = "/tmp/" + UUID.randomUUID() + ".qcow2";
 
-        /* 10TB virtual_size */
+        long size = 10 * 1024 * 1024L;
-        long size = 10995116277760l;
         QemuImgFile file = new QemuImgFile(filename, size, PhysicalDiskFormat.QCOW2);
         String preallocation = "metadata";
         Map<String, String> options = new HashMap<String, String>();
@@ -141,8 +159,8 @@ public class QemuImgTest {
         QemuImg qemu = new QemuImg(0);
         qemu.create(file, options);
 
-        String allocatedSize = Script.runSimpleBashScript(String.format("ls -alhs %s | awk '{print $1}'", file));
+        String allocatedSize = Script.runSimpleBashScript(String.format("ls -alhs %s | awk '{print $1}'", filename));
-        String declaredSize  = Script.runSimpleBashScript(String.format("ls -alhs %s | awk '{print $6}'", file));
+        String declaredSize  = Script.runSimpleBashScript(String.format("ls -alhs %s | awk '{print $6}'", filename));
 
         assertFalse(allocatedSize.equals(declaredSize));
 
@@ -162,7 +180,7 @@ public class QemuImgTest {
         try {
             QemuImg qemu = new QemuImg(0);
             qemu.create(file);
-            qemu.resize(file, endSize);
+            qemu.resize(file, endSize, null);
             Map<String, String> info = qemu.info(file);
 
             if (info == null) {
@@ -191,7 +209,7 @@ public class QemuImgTest {
         try {
             QemuImg qemu = new QemuImg(0);
             qemu.create(file);
-            qemu.resize(file, increment, true);
+            qemu.resize(file, increment, true, null);
             Map<String, String> info = qemu.info(file);
 
             if (info == null) {
@@ -208,6 +226,9 @@ public class QemuImgTest {
         f.delete();
     }
 
+    // This test is failing and needs changes in QemuImg.resize to support shrinking images with delta sizes.
+    // Earlier whole test suite was ignored, now only this test is ignored to allow other tests to run.
+    @Ignore
     @Test
     public void testCreateAndResizeDeltaNegative() throws QemuImgException, LibvirtException {
         String filename = "/tmp/" + UUID.randomUUID() + ".qcow2";
@@ -219,7 +240,7 @@ public class QemuImgTest {
         try {
             QemuImg qemu = new QemuImg(0);
             qemu.create(file);
-            qemu.resize(file, increment, true);
+            qemu.resize(file, increment, true, null);
             Map<String, String> info = qemu.info(file);
 
             if (info == null) {
@@ -249,7 +270,7 @@ public class QemuImgTest {
         QemuImg qemu = new QemuImg(0);
         try {
             qemu.create(file);
-            qemu.resize(file, endSize);
+            qemu.resize(file, endSize, null);
         } finally {
             File f = new File(filename);
             f.delete();
@@ -265,7 +286,7 @@ public class QemuImgTest {
 
         QemuImg qemu = new QemuImg(0);
         qemu.create(file);
-        qemu.resize(file, 0);
+        qemu.resize(file, 0, null);
 
         File f = new File(filename);
         f.delete();
@@ -377,7 +398,7 @@ public class QemuImgTest {
 
         try {
             QemuImg qemu = new QemuImg(0);
-            qemu.checkAndRepair(file, null, null, null);
+            qemu.checkAndRepair(file, new QemuImageOptions(Collections.emptyMap()), Collections.emptyList(), null);
         } catch (QemuImgException e) {
             fail(e.getMessage());
         }
@@ -385,4 +406,160 @@ public class QemuImgTest {
         File f = new File(filename);
         f.delete();
     }
+
+    @Test
+    public void addScriptOptionsFromMapAddsValidOptions() throws LibvirtException, QemuImgException {
+        Script script = Mockito.mock(Script.class);
+        Map<String, String> options = new HashMap<>();
+        options.put("key1", "value1");
+        options.put("key2", "value2");
+
+        QemuImg qemu = new QemuImg(0);
+        qemu.addScriptOptionsFromMap(options, script);
+
+        Mockito.verify(script, Mockito.times(1)).add("-o");
+        Mockito.verify(script, Mockito.times(1)).add("key1=value1,key2=value2");
+    }
+
+    @Test
+    public void addScriptOptionsFromMapHandlesEmptyOptions() throws LibvirtException, QemuImgException {
+        Script script = Mockito.mock(Script.class);
+        Map<String, String> options = new HashMap<>();
+
+        QemuImg qemu = new QemuImg(0);
+        qemu.addScriptOptionsFromMap(options, script);
+
+        Mockito.verify(script, Mockito.never()).add(Mockito.anyString());
+    }
+
+    @Test
+    public void addScriptOptionsFromMapHandlesNullOptions() throws LibvirtException, QemuImgException {
+        Script script = Mockito.mock(Script.class);
+
+        QemuImg qemu = new QemuImg(0);
+        qemu.addScriptOptionsFromMap(null, script);
+
+        Mockito.verify(script, Mockito.never()).add(Mockito.anyString());
+    }
+
+    @Test
+    public void addScriptOptionsFromMapHandlesTrailingComma() throws LibvirtException, QemuImgException {
+        Script script = Mockito.mock(Script.class);
+        Map<String, String> options = new HashMap<>();
+        options.put("key1", "value1");
+
+        QemuImg qemu = new QemuImg(0);
+        qemu.addScriptOptionsFromMap(options, script);
+
+        Mockito.verify(script, Mockito.times(1)).add("-o");
+        Mockito.verify(script, Mockito.times(1)).add("key1=value1");
+    }
+
+    @Test
+    public void getResizeOptionsFromConvertOptionsReturnsNullForEmptyOptions() throws LibvirtException, QemuImgException {
+        QemuImg qemuImg = new QemuImg(0);
+        Map<String, String> options = new HashMap<>();
+
+        Map<String, String> result = qemuImg.getResizeOptionsFromConvertOptions(options);
+
+        Assert.assertNull(result);
+    }
+
+    @Test
+    public void getResizeOptionsFromConvertOptionsReturnsNullForNullOptions() throws LibvirtException, QemuImgException {
+        QemuImg qemuImg = new QemuImg(0);
+
+        Map<String, String> result = qemuImg.getResizeOptionsFromConvertOptions(null);
+
+        Assert.assertNull(result);
+    }
+
+    @Test
+    public void getResizeOptionsFromConvertOptionsReturnsPreallocationOption() throws LibvirtException, QemuImgException {
+        QemuImg qemuImg = new QemuImg(0);
+        Map<String, String> options = new HashMap<>();
+        options.put(QemuImg.PREALLOCATION, "metadata");
+
+        Map<String, String> result = qemuImg.getResizeOptionsFromConvertOptions(options);
+
+        Assert.assertNotNull(result);
+        assertEquals(1, result.size());
+        assertEquals("metadata", result.get(QemuImg.PREALLOCATION));
+    }
+
+    @Test
+    public void getResizeOptionsFromConvertOptionsIgnoresUnrelatedOptions() throws LibvirtException, QemuImgException {
+        QemuImg qemuImg = new QemuImg(0);
+        Map<String, String> options = new HashMap<>();
+        options.put("unrelatedKey", "unrelatedValue");
+
+        Map<String, String> result = qemuImg.getResizeOptionsFromConvertOptions(options);
+
+        Assert.assertTrue(MapUtils.isEmpty(result));
+    }
+
+    @Test
+    public void getResizeOptionsFromConvertOptionsHandlesMixedOptions() throws LibvirtException, QemuImgException {
+        QemuImg qemuImg = new QemuImg(0);
+        Map<String, String> options = new HashMap<>();
+        options.put(QemuImg.PREALLOCATION, "full");
+        options.put("unrelatedKey", "unrelatedValue");
+
+        Map<String, String> result = qemuImg.getResizeOptionsFromConvertOptions(options);
+
+        Assert.assertNotNull(result);
+        assertEquals(1, result.size());
+        assertEquals("full", result.get(QemuImg.PREALLOCATION));
+    }
+
+    @Test
+    public void addScriptResizeOptionsFromMapAddsPreallocationOption() throws LibvirtException, QemuImgException {
+        Script script = Mockito.mock(Script.class);
+        Map<String, String> options = new HashMap<>();
+        options.put(QemuImg.PREALLOCATION, "metadata");
+
+        QemuImg qemuImg = new QemuImg(0);
+        qemuImg.addScriptResizeOptionsFromMap(options, script);
+
+        Mockito.verify(script, Mockito.times(1)).add("--preallocation=metadata");
+        Mockito.verify(script, Mockito.never()).add("-o");
+        assertTrue(options.isEmpty());
+    }
+
+    @Test
+    public void addScriptResizeOptionsFromMapHandlesEmptyOptions() throws LibvirtException, QemuImgException {
+        Script script = Mockito.mock(Script.class);
+        Map<String, String> options = new HashMap<>();
+
+        QemuImg qemuImg = new QemuImg(0);
+        qemuImg.addScriptResizeOptionsFromMap(options, script);
+
+        Mockito.verify(script, Mockito.never()).add(Mockito.anyString());
+    }
+
+    @Test
+    public void addScriptResizeOptionsFromMapHandlesNullOptions() throws LibvirtException, QemuImgException {
+        Script script = Mockito.mock(Script.class);
+
+        QemuImg qemuImg = new QemuImg(0);
+        qemuImg.addScriptResizeOptionsFromMap(null, script);
+
+        Mockito.verify(script, Mockito.never()).add(Mockito.anyString());
+    }
+
+    @Test
+    public void addScriptResizeOptionsFromMapHandlesMixedOptions() throws LibvirtException, QemuImgException {
+        Script script = Mockito.mock(Script.class);
+        Map<String, String> options = new HashMap<>();
+        options.put(QemuImg.PREALLOCATION, "full");
+        options.put("key", "value");
+
+        QemuImg qemuImg = new QemuImg(0);
+        qemuImg.addScriptResizeOptionsFromMap(options, script);
+
+        Mockito.verify(script, Mockito.times(1)).add("--preallocation=full");
+        Mockito.verify(script, Mockito.times(1)).add("-o");
+        Mockito.verify(script, Mockito.times(1)).add("key=value");
+        assertFalse(options.containsKey(QemuImg.PREALLOCATION));
+    }
 }