kvm: clone encrypted roots from a dense base image, not from the sparse template

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

@@ -101,6 +101,9 @@ public class LibvirtStorageAdaptor implements StorageAdaptor {
        usable (decrypted) size still matches the requested volume size. */
     private static final long LUKS2_HEADER_RESERVE_BYTES = 16L << 20; // 16 MiB
 
+    /** Suffix of the per-template base image encrypted roots are cloned from. */
+    private static final String LUKS_CLONE_BASE_SUFFIX = "-luks";
+
     private static final Set<StoragePoolType> QEMU_IMG_MANAGED_POOL_TYPES = Set.of(StoragePoolType.NetworkFilesystem, StoragePoolType.Filesystem, StoragePoolType.SharedMountPoint);
 
     public LibvirtStorageAdaptor(StorageLayer storage) {
@@ -1200,6 +1203,7 @@ public boolean deletePhysicalDisk(String uuid, KVMStoragePool pool, Storage.Imag
                     }
                     logger.info("Successfully unprotected and removed any remaining snapshots (" + snaps.size() + ") of "
                         + pool.getSourceDir() + "/" + uuid + " Continuing to remove the RBD image");
+                    removeLuksCloneBaseIfPresent(rbd, uuid);
                 } catch (RbdException e) {
                     logger.error("Failed to remove snapshot with exception: " + e.toString() +
                         ", RBD error: " + ErrorCode.getErrorMessage(e.getReturnValue()));
@@ -1526,54 +1530,35 @@ private KVMPhysicalDisk createDiskFromTemplateOnRBD(KVMPhysicalDisk template,
 
     /**
      * Option A (thin CoW encrypted root), used when the template already lives on the same RBD cluster
-     * as the destination pool. Per the Ceph "Image Encryption" clone recipe: grow the template base to
-     * reserve LUKS2-header space, snapshot+protect that grown state, clone from it, apply a LUKS2 header,
-     * then resize the clone to the requested size. The inherited (plaintext) template data stays readable
-     * through the clone's encryption, and the clone is a thin CoW image (only the header is written).
+     * as the destination pool: clone a per-template base image that carries LUKS2-header room, apply a
+     * LUKS2 header to the clone, then grow it to the requested size. The base is a dense copy of the
+     * template, not the template itself, because a hole in the parent of an encrypted clone reads as
+     * garbage once a write copies up the object around it. It is built once per template per pool.
      *
      * @return the encrypted CoW clone, or {@code null} if the Ceph operations failed
      */
     private KVMPhysicalDisk createEncryptedRootCoWClone(KVMPhysicalDisk template, KVMStoragePool destPool,
             String newUuid, KVMPhysicalDisk disk, byte[] passphrase) {
-        String luksReservedSnapshotName = rbdTemplateSnapName + "-luks";
+        final String cloneBaseImage = template.getName() + LUKS_CLONE_BASE_SUFFIX;
         Rados radosConnection = null;
         IoCTX ioContext = null;
-        Rbd rbdClient = null;
-        RbdImage templateImage = null;
         try {
             radosConnection = new Rados(destPool.getAuthUserName());
             radosConnection.confSet("mon_host", destPool.getSourceHost() + ":" + destPool.getSourcePort());
             radosConnection.confSet("key", destPool.getAuthSecret());
             radosConnection.confSet("client_mount_timeout", "30");
             radosConnection.connect();
             ioContext = radosConnection.ioCtxCreate(destPool.getSourceDir());
-            rbdClient = new Rbd(ioContext);
-            templateImage = rbdClient.open(template.getName());
-            boolean luksSnapshotExists = false;
-            for (RbdSnapInfo snapshotInfo : templateImage.snapList()) {
-                if (luksReservedSnapshotName.equals(snapshotInfo.name)) {
-                    luksSnapshotExists = true;
-                    break;
-                }
-            }
-            if (!luksSnapshotExists) {
-                templateImage.resize(template.getVirtualSize() + LUKS2_HEADER_RESERVE_BYTES);
-                templateImage.snapCreate(luksReservedSnapshotName);
-                templateImage.snapProtect(luksReservedSnapshotName);
-                logger.debug("Prepared LUKS-reserved template snapshot {}@{}", template.getName(), luksReservedSnapshotName);
+            Rbd rbdClient = new Rbd(ioContext);
+            if (!isLuksCloneBaseReady(rbdClient, cloneBaseImage)) {
+                buildLuksCloneBase(rbdClient, template, destPool, cloneBaseImage);
             }
-            rbdClient.clone(template.getName(), luksReservedSnapshotName, ioContext, newUuid, RBD_FEATURES, rbdOrder);
+            rbdClient.clone(cloneBaseImage, rbdTemplateSnapName, ioContext, newUuid, RBD_FEATURES, rbdOrder);
+            logger.debug("Cloned {}/{}@{} to {}", destPool.getSourceDir(), cloneBaseImage, rbdTemplateSnapName, newUuid);
         } catch (RadosException | RbdException e) {
             logger.error("Failed to create encrypted CoW clone {}: {}", newUuid, e.getMessage());
             return null;
         } finally {
-            if (rbdClient != null && templateImage != null) {
-                try {
-                    rbdClient.close(templateImage);
-                } catch (RbdException ignored) {
-                    // best-effort close of the template handle
-                }
-            }
             if (radosConnection != null && ioContext != null) {
                 radosConnection.ioCtxDestroy(ioContext);
             }
@@ -1589,6 +1574,114 @@ private KVMPhysicalDisk createEncryptedRootCoWClone(KVMPhysicalDisk template, KV
         return disk;
     }
 
+    /** Does {@code cloneBaseImage} exist with its protected snapshot? It gets its name only once complete. */
+    private boolean isLuksCloneBaseReady(Rbd rbdClient, String cloneBaseImage) {
+        RbdImage image = null;
+        try {
+            image = rbdClient.open(cloneBaseImage);
+            for (RbdSnapInfo snapshotInfo : image.snapList()) {
+                if (rbdTemplateSnapName.equals(snapshotInfo.name)) {
+                    return image.snapIsProtected(snapshotInfo.name);
+                }
+            }
+            return false;
+        } catch (RbdException e) {
+            // not there yet, or not readable: it gets built
+            return false;
+        } finally {
+            if (image != null) {
+                try {
+                    rbdClient.close(image);
+                } catch (RbdException ignored) {
+                    // best-effort close of the base image handle
+                }
+            }
+        }
+    }
+
+    /**
+     * Build the base image: template size plus LUKS2-header room, a dense copy of the template, and a
+     * protected snapshot. It is built under a staging name and renamed at the end, so a half-written
+     * base is never cloned; a host that loses the rename to another one discards its copy.
+     */
+    private void buildLuksCloneBase(Rbd rbdClient, KVMPhysicalDisk template, KVMStoragePool destPool,
+            String cloneBaseImage) throws RbdException {
+        final String stagingImage = cloneBaseImage + "." + UUID.randomUUID();
+        logger.info("Building dense LUKS clone base {}/{} from template {} as {}", destPool.getSourceDir(),
+                cloneBaseImage, template.getName(), stagingImage);
+        rbdClient.create(stagingImage, template.getVirtualSize() + LUKS2_HEADER_RESERVE_BYTES, RBD_FEATURES, rbdOrder);
+        boolean renamed = false;
+        try {
+            new RbdEncryption().copyTemplateDense(destPool.getSourceHost(), destPool.getSourcePort(),
+                    destPool.getAuthUserName(), destPool.getAuthSecret(), destPool.getSourceDir(),
+                    template.getName(), stagingImage);
+            RbdImage stagedImage = rbdClient.open(stagingImage);
+            try {
+                stagedImage.snapCreate(rbdTemplateSnapName);
+                stagedImage.snapProtect(rbdTemplateSnapName);
+            } finally {
+                rbdClient.close(stagedImage);
+            }
+            try {
+                rbdClient.rename(stagingImage, cloneBaseImage);
+                renamed = true;
+                logger.info("Prepared dense LUKS clone base {}/{}", destPool.getSourceDir(), cloneBaseImage);
+            } catch (RbdException e) {
+                logger.info("Another host prepared LUKS clone base {}/{} first; discarding {}",
+                        destPool.getSourceDir(), cloneBaseImage, stagingImage);
+            }
+        } finally {
+            if (!renamed) {
+                removeRbdImageQuietly(rbdClient, stagingImage);
+            }
+        }
+        if (!renamed && !isLuksCloneBaseReady(rbdClient, cloneBaseImage)) {
+            throw new RbdException("LUKS clone base " + destPool.getSourceDir() + "/" + cloneBaseImage
+                    + " could not be prepared and no other host has prepared it");
+        }
+    }
+
+    /** Remove a template's LUKS clone base image along with the template, if it has one. */
+    private void removeLuksCloneBaseIfPresent(Rbd rbdClient, String volumeName) {
+        final String cloneBaseImage = volumeName + LUKS_CLONE_BASE_SUFFIX;
+        RbdImage image;
+        try {
+            image = rbdClient.open(cloneBaseImage);
+        } catch (RbdException e) {
+            // everything that is not a template of an encrypted root has no base image
+            return;
+        }
+        try {
+            rbdClient.close(image);
+        } catch (RbdException ignored) {
+            // best-effort close of the probe handle
+        }
+        logger.info("Removing LUKS clone base image {} along with {}", cloneBaseImage, volumeName);
+        removeRbdImageQuietly(rbdClient, cloneBaseImage);
+    }
+
+    /** Remove an RBD image and its snapshots, logging instead of failing. */
+    private void removeRbdImageQuietly(Rbd rbdClient, String imageName) {
+        try {
+            RbdImage image = rbdClient.open(imageName);
+            try {
+                for (RbdSnapInfo snapshotInfo : image.snapList()) {
+                    if (image.snapIsProtected(snapshotInfo.name)) {
+                        image.snapUnprotect(snapshotInfo.name);
+                    }
+                    image.snapRemove(snapshotInfo.name);
+                }
+            } finally {
+                rbdClient.close(image);
+            }
+            rbdClient.remove(imageName);
+            logger.debug("Removed RBD image {}", imageName);
+        } catch (RbdException e) {
+            // e.g. a base image still has clones of live encrypted roots
+            logger.warn("Could not remove RBD image {}: {}", imageName, e.getMessage());
+        }
+    }
+
     /**
      * Option B (full-copy encrypted root), used when the template is not on the same RBD cluster (e.g. first
      * use from secondary storage). Create an empty image, apply a LUKS2 header, then import the template

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,40 @@ public void importTemplate(String srcRbdPool, String srcRbdImage,
     /**
      * Seam for unit tests; {@link QemuImg} probes the qemu version through libvirt on construction.
      */
+    /**
+     * Copy a plaintext RBD image onto an existing image in the same pool, writing out its zero ranges
+     * too ({@code -S 0}), so the copy has no holes. Used for the base image 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 srcImage, String destImage) {
+        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 + "/" + srcImage, QemuImg.PhysicalDiskFormat.RAW);
+            QemuImageOptions srcImageOpts = new QemuImageOptions(rbdImageOptions(cephPool, srcImage, conf.toString(), authUser));
+            srcImageOpts.setImageOptsFlag(true);
+            QemuImageOptions destImageOpts = new QemuImageOptions(rbdImageOptions(cephPool, destImage, conf.toString(), authUser));
+
+            QemuImg qemu = createQemuImg();
+            qemu.setWriteZeroRanges(true);
+            qemu.convertIntoExistingTarget(srcQemuFile, null, null, srcImageOpts, destImageOpts, false);
+            logger.debug("Copied RBD image {}/{} into {} without holes", cephPool, srcImage, destImage);
+        } catch (IOException | QemuImgException | LibvirtException ex) {
+            throw new CloudRuntimeException(String.format("Failed to copy RBD image %s/%s into %s", cephPool, srcImage, destImage), 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", "tmpl-luks");
+
+        // The whole point of this copy: without -S 0 it inherits the template's holes, and a hole in
+        // the parent of an encrypted clone is read through the crypto layer - as garbage - as soon as
+        // anything writes into the same 4 MiB object.
+        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"));
+
+        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-luks"));
+        // the base image is a plaintext copy; the LUKS header belongs to the clone, not to the base
+        Assert.assertFalse(dest, dest.contains("encrypt."));
+    }
+
     @Test
     public void formatRejectsEmptyPassphrase() {
         Assert.assertThrows(CloudRuntimeException.class, () -> rbdEncryption.format(