kvm: fill the template's holes before taking its LUKS clone snapshot

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

@@ -1535,7 +1535,8 @@ private KVMPhysicalDisk createDiskFromTemplateOnRBD(KVMPhysicalDisk template,
      */
     private KVMPhysicalDisk createEncryptedRootCoWClone(KVMPhysicalDisk template, KVMStoragePool destPool,
             String newUuid, KVMPhysicalDisk disk, byte[] passphrase) {
-        String luksReservedSnapshotName = rbdTemplateSnapName + "-luks";
+        // not "-luks": templates prepared before carry a snapshot of that name that still has holes
+        String luksReservedSnapshotName = rbdTemplateSnapName + "-luks2";
         Rados radosConnection = null;
         IoCTX ioContext = null;
         Rbd rbdClient = null;
@@ -1558,8 +1559,20 @@ private KVMPhysicalDisk createEncryptedRootCoWClone(KVMPhysicalDisk template, KV
             }
             if (!luksSnapshotExists) {
                 templateImage.resize(template.getVirtualSize() + LUKS2_HEADER_RESERVE_BYTES);
-                templateImage.snapCreate(luksReservedSnapshotName);
-                templateImage.snapProtect(luksReservedSnapshotName);
+                // fill the template's holes first: an encrypted clone reads a hole in its parent as
+                // garbage once a guest write copies up the object around it
+                new RbdEncryption().copyTemplateDense(destPool.getSourceHost(), destPool.getSourcePort(),
+                        destPool.getAuthUserName(), destPool.getAuthSecret(), destPool.getSourceDir(),
+                        template.getName(), rbdTemplateSnapName);
+                try {
+                    templateImage.snapCreate(luksReservedSnapshotName);
+                    templateImage.snapProtect(luksReservedSnapshotName);
+                } catch (RbdException e) {
+                    // fine if another host prepared the same template at the same time
+                    if (!templateImage.snapIsProtected(luksReservedSnapshotName)) {
+                        throw e;
+                    }
+                }
                 logger.debug("Prepared LUKS-reserved template snapshot {}@{}", template.getName(), luksReservedSnapshotName);
             }
             rbdClient.clone(template.getName(), luksReservedSnapshotName, ioContext, newUuid, RBD_FEATURES, rbdOrder);

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

@@ -66,6 +66,7 @@ public class QemuImg {
     private String cloudQemuImgPath = "cloud-qemu-img";
     private long timeout;
     private boolean skipZero = false;
+    private boolean writeZeroRanges = false;
     private boolean skipTargetVolumeCreation = false;
     private boolean noCache = false;
     private long version;
@@ -508,6 +509,11 @@ public void convert(final QemuImgFile srcFile, final QemuImgFile destFile, QemuI
             script.add("-n");
         }
 
+        if (writeZeroRanges) {
+            script.add("-S");
+            script.add("0");
+        }
+
         if (destImageOpts == null) {
             script.add("-O");
             script.add(destFile.getFormat().toString());
@@ -1011,6 +1017,14 @@ public void setSkipZero(boolean skipZero) {
         this.skipZero = skipZero;
     }
 
+    /**
+     * Make convert write the source's zero ranges to the destination ({@code -S 0}) instead of
+     * leaving them unallocated.
+     */
+    public void setWriteZeroRanges(boolean writeZeroRanges) {
+        this.writeZeroRanges = writeZeroRanges;
+    }
+
     public void setSkipTargetVolumeCreation(boolean skipTargetVolumeCreation) {
         this.skipTargetVolumeCreation = skipTargetVolumeCreation;
     }

plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/utils/rbd/RbdEncryption.java

@@ -253,6 +253,42 @@ public void importTemplate(String srcRbdPool, String srcRbdImage,
     /**
      * Seam for unit tests; {@link QemuImg} probes the qemu version through libvirt on construction.
      */
+    /**
+     * Rewrite an RBD image from one of its snapshots, writing out the zero ranges too ({@code -S 0}),
+     * so the image has no holes. Used on a template before taking the snapshot encrypted roots are
+     * cloned from: a hole in that parent reads as garbage through the clone's encryption once a
+     * guest write copies up the object around it.
+     */
+    public void copyTemplateDense(String monHost, int monPort, String authUser, String authSecret,
+                                  String cephPool, String image, String srcSnapshot) {
+        Path conf = null;
+        Path keyring = null;
+        try {
+            final FileAttribute<?> ownerOnly = PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rw-------"));
+            keyring = Files.createTempFile("cs-ceph-", ".keyring", ownerOnly);
+            Files.writeString(keyring, "[client." + authUser + "]\n\tkey = " + authSecret + "\n");
+            conf = Files.createTempFile("cs-ceph-", ".conf", ownerOnly);
+            Files.writeString(conf, "[global]\nmon_host = " + monSpec(monHost, monPort) + "\nkeyring = " + keyring + "\n");
+
+            QemuImgFile srcQemuFile = new QemuImgFile(cephPool + "/" + image + "@" + srcSnapshot, QemuImg.PhysicalDiskFormat.RAW);
+            Map<String, String> srcParams = rbdImageOptions(cephPool, image, conf.toString(), authUser);
+            srcParams.put("snapshot", srcSnapshot);
+            QemuImageOptions srcImageOpts = new QemuImageOptions(srcParams);
+            srcImageOpts.setImageOptsFlag(true);
+            QemuImageOptions destImageOpts = new QemuImageOptions(rbdImageOptions(cephPool, image, conf.toString(), authUser));
+
+            QemuImg qemu = createQemuImg();
+            qemu.setWriteZeroRanges(true);
+            qemu.convertIntoExistingTarget(srcQemuFile, null, null, srcImageOpts, destImageOpts, false);
+            logger.debug("Rewrote RBD image {}/{} from @{} without holes", cephPool, image, srcSnapshot);
+        } catch (IOException | QemuImgException | LibvirtException ex) {
+            throw new CloudRuntimeException(String.format("Failed to rewrite RBD image %s/%s from @%s", cephPool, image, srcSnapshot), ex);
+        } finally {
+            deleteQuietly(conf);
+            deleteQuietly(keyring);
+        }
+    }
+
     protected QemuImg createQemuImg() throws QemuImgException, LibvirtException {
         return new QemuImg(0);
     }

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

@@ -323,6 +323,41 @@ public void testCreateWithBackingFile() throws QemuImgException, LibvirtExceptio
         }
     }
 
+    @Test
+    public void testConvertWriteZeroRangesAllocatesTheWholeDestination() throws QemuImgException, LibvirtException {
+        // qemu-img skips the source's zero ranges by default, so the destination keeps its holes.
+        // That is wrong for an image whose unallocated ranges are not read as zeros - an RBD image
+        // with a librbd LUKS header, cloned, is the case setWriteZeroRanges exists for.
+        String srcPath = "/tmp/" + UUID.randomUUID() + ".raw";
+        String sparsePath = "/tmp/" + UUID.randomUUID() + ".raw";
+        String densePath = "/tmp/" + UUID.randomUUID() + ".raw";
+        long size = 16L * 1024 * 1024;
+
+        QemuImgFile srcFile = new QemuImgFile(srcPath, size, PhysicalDiskFormat.RAW);
+        QemuImg qemu = new QemuImg(0);
+        qemu.create(srcFile);
+        // data in the first MiB only: the remaining 15 MiB of the source are zeros
+        Script.runSimpleBashScript(String.format("dd if=/dev/urandom of=%s bs=1M count=1 conv=notrunc 2>/dev/null", srcPath));
+
+        qemu.convert(srcFile, new QemuImgFile(sparsePath, PhysicalDiskFormat.RAW));
+        qemu.setWriteZeroRanges(true);
+        qemu.convert(srcFile, new QemuImgFile(densePath, PhysicalDiskFormat.RAW));
+
+        long apparent = Long.parseLong(Script.runSimpleBashScript(String.format("stat -c %%s %s", densePath)));
+        long sparseAllocated = Long.parseLong(Script.runSimpleBashScript(String.format("du --block-size=1 %s | cut -f1", sparsePath)));
+        long denseAllocated = Long.parseLong(Script.runSimpleBashScript(String.format("du --block-size=1 %s | cut -f1", densePath)));
+
+        assertEquals(size, apparent);
+        assertTrue("the default convert should have left the zero ranges unallocated, allocated " + sparseAllocated,
+                sparseAllocated < size);
+        assertTrue("-S 0 should have written every zero range, allocated " + denseAllocated,
+                denseAllocated >= size);
+
+        assertTrue(new File(srcPath).delete());
+        assertTrue(new File(sparsePath).delete());
+        assertTrue(new File(densePath).delete());
+    }
+
     @Test
     public void testConvertBasic() throws QemuImgException, LibvirtException {
         long srcSize = 20480;

plugins/hypervisors/kvm/src/test/java/org/apache/cloudstack/utils/rbd/RbdEncryptionTest.java

@@ -143,6 +143,36 @@ public void importTemplateFromFileSourceForcesSourceFormat() throws Exception {
         Assert.assertTrue(src, src.contains("file.filename=/tmp/tmpl.qcow2"));
     }
 
+    @Test
+    public void copyTemplateDenseWritesZeroRangesAndLeavesTheCopyPlaintext() throws Exception {
+        RbdEncryption spy = Mockito.spy(new RbdEncryption());
+        QemuImg qemuImg = Mockito.mock(QemuImg.class);
+        Mockito.doReturn(qemuImg).when(spy).createQemuImg();
+
+        spy.copyTemplateDense("1.2.3.4", 6789, "cloudstack", "secret", "cloudstack", "tmpl", "cloudstack-base-snap");
+
+        // without -S 0 the template keeps its holes
+        Mockito.verify(qemuImg).setWriteZeroRanges(true);
+
+        ArgumentCaptor<QemuImageOptions> srcOpts = ArgumentCaptor.forClass(QemuImageOptions.class);
+        ArgumentCaptor<QemuImageOptions> destOpts = ArgumentCaptor.forClass(QemuImageOptions.class);
+        Mockito.verify(qemuImg).convertIntoExistingTarget(Mockito.any(QemuImgFile.class), Mockito.isNull(),
+                Mockito.isNull(), srcOpts.capture(), destOpts.capture(), Mockito.eq(false));
+
+        String src = String.join(" ", srcOpts.getValue().toCommandFlag());
+        Assert.assertTrue(src, src.startsWith("--image-opts "));
+        Assert.assertTrue(src, src.contains("pool=cloudstack"));
+        Assert.assertTrue(src, src.contains("image=tmpl"));
+        Assert.assertTrue(src, src.contains("snapshot=cloudstack-base-snap"));
+
+        String dest = String.join(" ", destOpts.getValue().toCommandFlag(QemuImg.TARGET_IMAGE_OPTS_FLAG));
+        Assert.assertTrue(dest, dest.startsWith(QemuImg.TARGET_IMAGE_OPTS_FLAG + " "));
+        Assert.assertTrue(dest, dest.contains("image=tmpl"));
+        // the template is written to itself, not to a snapshot, and stays plaintext
+        Assert.assertFalse(dest, dest.contains("snapshot="));
+        Assert.assertFalse(dest, dest.contains("encrypt."));
+    }
+
     @Test
     public void formatRejectsEmptyPassphrase() {
         Assert.assertThrows(CloudRuntimeException.class, () -> rbdEncryption.format(