diff --git a/plugins/hypervisors/kvm/pom.xml b/plugins/hypervisors/kvm/pom.xml index 8ed960c6bc0..bc7b6903aae 100644 --- a/plugins/hypervisors/kvm/pom.xml +++ b/plugins/hypervisors/kvm/pom.xml @@ -107,15 +107,6 @@ - - org.apache.maven.plugins - maven-surefire-plugin - - - **/QemuImg*.java - - - 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 index 87544cfaa9d..a03daeb197b 100644 --- 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 @@ -1081,7 +1081,7 @@ public class LibvirtStorageAdaptor implements StorageAdaptor { destFile.setSize(size); Map options = new HashMap(); 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)) { diff --git a/plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/utils/qemu/QemuImg.java b/plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/utils/qemu/QemuImg.java index 0a8ea27cd49..1fec561dc89 100644 --- a/plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/utils/qemu/QemuImg.java +++ b/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 getResizeOptionsFromConvertOptions(final Map options) { + if (MapUtils.isEmpty(options)) { + return null; + } + Map 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 options, final List 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,17 +703,29 @@ public class QemuImg { } } - private void addScriptOptionsFromMap(Map options, Script s) { - if (options != null && !options.isEmpty()) { - s.add("-o"); - final StringBuffer optionsBuffer = new StringBuffer(); - for (final Map.Entry option : options.entrySet()) { - optionsBuffer.append(option.getKey()).append('=').append(option.getValue()).append(','); - } - String optionsStr = optionsBuffer.toString(); - optionsStr = optionsStr.replaceAll(",$", ""); - s.add(optionsStr); + protected void addScriptOptionsFromMap(Map options, Script s) { + if (MapUtils.isEmpty(options)) { + return; } + s.add("-o"); + final StringBuffer optionsBuffer = new StringBuffer(); + for (final Map.Entry option : options.entrySet()) { + optionsBuffer.append(option.getKey()).append('=').append(option.getValue()).append(','); + } + String optionsStr = optionsBuffer.toString(); + optionsStr = optionsStr.replaceAll(",$", ""); + s.add(optionsStr); + } + + protected void addScriptResizeOptionsFromMap(Map 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. - * + *

* This method is a facade for 'qemu-img resize'. - * + *

* 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 - * The file to be resized. - * @param size - * The new size. - * @param delta - * Flag to inform if the new size is a delta. + * @param file The file to be resized. + * @param size The new size. + * @param delta Flag to inform if the new size is a delta. + * @param options Script options for resizing. Takes a Map with key value */ - 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 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. - * - * 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. * - * @param file - * The file to be resized. - * @param size - * The new size. + * @param file The file to be resized. + * @param size The new size. + * @param options Script options for resizing. Takes a Map with key value */ - public void resize(final QemuImgFile file, final long size) throws QemuImgException { - this.resize(file, size, false); + public void resize(final QemuImgFile file, final long size, Map options) throws QemuImgException { + this.resize(file, size, false, options); } /** diff --git a/plugins/hypervisors/kvm/src/test/java/org/apache/cloudstack/utils/qemu/QemuImgTest.java b/plugins/hypervisors/kvm/src/test/java/org/apache/cloudstack/utils/qemu/QemuImgTest.java index b0981dde26e..5a027425776 100644 --- a/plugins/hypervisors/kvm/src/test/java/org/apache/cloudstack/utils/qemu/QemuImgTest.java +++ b/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.apache.cloudstack.utils.qemu.QemuImg.PhysicalDiskFormat; +import org.junit.runner.RunWith; +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 = 10995116277760l; + long size = 10 * 1024 * 1024L; QemuImgFile file = new QemuImgFile(filename, size, PhysicalDiskFormat.QCOW2); String preallocation = "metadata"; Map options = new HashMap(); @@ -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 declaredSize = Script.runSimpleBashScript(String.format("ls -alhs %s | awk '{print $6}'", 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}'", 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 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 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 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 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 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 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 options = new HashMap<>(); + + Map result = qemuImg.getResizeOptionsFromConvertOptions(options); + + Assert.assertNull(result); + } + + @Test + public void getResizeOptionsFromConvertOptionsReturnsNullForNullOptions() throws LibvirtException, QemuImgException { + QemuImg qemuImg = new QemuImg(0); + + Map result = qemuImg.getResizeOptionsFromConvertOptions(null); + + Assert.assertNull(result); + } + + @Test + public void getResizeOptionsFromConvertOptionsReturnsPreallocationOption() throws LibvirtException, QemuImgException { + QemuImg qemuImg = new QemuImg(0); + Map options = new HashMap<>(); + options.put(QemuImg.PREALLOCATION, "metadata"); + + Map 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 options = new HashMap<>(); + options.put("unrelatedKey", "unrelatedValue"); + + Map result = qemuImg.getResizeOptionsFromConvertOptions(options); + + Assert.assertTrue(MapUtils.isEmpty(result)); + } + + @Test + public void getResizeOptionsFromConvertOptionsHandlesMixedOptions() throws LibvirtException, QemuImgException { + QemuImg qemuImg = new QemuImg(0); + Map options = new HashMap<>(); + options.put(QemuImg.PREALLOCATION, "full"); + options.put("unrelatedKey", "unrelatedValue"); + + Map 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 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 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 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)); + } }