From 0148241b5d023682daeed25277d843e06996393d Mon Sep 17 00:00:00 2001 From: Slavka Peleva Date: Fri, 13 Nov 2026 18:50:49 +0200 Subject: [PATCH] StorPool: keep the LUKS header allowance when resizing encrypted volumes The StorPool volume of an encrypted volume is created 2 MiB bigger than the requested size for the LUKS header, but resize and its rollback set it to the bare CloudStack size, so the guest lost the header space. - add the LUKS header allowance on resize and on rollback - always roll back when the agent fails or can not be reached - update the pool capacity only after a successful operation - delete the StorPool volume when encryption fails and return the encryption error from the agent - detach the template snapshot and the volume even on failures - add unit tests and smoke tests for live, offline and ROOT resize, and clean up all objects created by the smoke tests Fixes #14695 --- .../StorPoolResizeVolumeCommandWrapper.java | 7 +- ...PoolSetVolumeEncryptionCommandWrapper.java | 22 +- .../StorPoolPrimaryDataStoreDriver.java | 83 ++++-- ...torPoolResizeVolumeCommandWrapperTest.java | 118 ++++++++ ...SetVolumeEncryptionCommandWrapperTest.java | 135 +++++++++ .../StorPoolPrimaryDataStoreDriverTest.java | 275 ++++++++++++++++++ .../plugins/storpool/TestEncryptedVolumes.py | 225 +++++++++++++- test/integration/plugins/storpool/sp_util.py | 10 + 8 files changed, 829 insertions(+), 46 deletions(-) create mode 100644 plugins/storage/volume/storpool/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/StorPoolResizeVolumeCommandWrapperTest.java create mode 100644 plugins/storage/volume/storpool/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/StorPoolSetVolumeEncryptionCommandWrapperTest.java diff --git a/plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/StorPoolResizeVolumeCommandWrapper.java b/plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/StorPoolResizeVolumeCommandWrapper.java index 8fc6b6bbcb8f..585a0bdc9f91 100644 --- a/plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/StorPoolResizeVolumeCommandWrapper.java +++ b/plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/StorPoolResizeVolumeCommandWrapper.java @@ -89,7 +89,12 @@ public ResizeVolumeAnswer execute(final StorPoolResizeVolumeCommand command, fin return new ResizeVolumeAnswer(command, false, error); } finally { if (!command.isAttached() && volPath != null) { - StorPoolStorageAdaptor.attachOrDetachVolume("detach", "volume", volPath); + // a failed detach must not replace the answer of the resize + try { + StorPoolStorageAdaptor.attachOrDetachVolume("detach", "volume", volPath); + } catch (final Exception e) { + logger.warn("Could not detach volume " + volPath + " after resize due to: " + e.getMessage()); + } } } } diff --git a/plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/StorPoolSetVolumeEncryptionCommandWrapper.java b/plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/StorPoolSetVolumeEncryptionCommandWrapper.java index 6efc118cf7e3..62f85a7e1555 100644 --- a/plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/StorPoolSetVolumeEncryptionCommandWrapper.java +++ b/plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/StorPoolSetVolumeEncryptionCommandWrapper.java @@ -38,7 +38,6 @@ import org.apache.commons.lang3.StringUtils; import org.libvirt.LibvirtException; -import com.cloud.agent.api.Answer; import com.cloud.agent.api.storage.StorPoolSetVolumeEncryptionAnswer; import com.cloud.agent.api.storage.StorPoolSetVolumeEncryptionCommand; import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; @@ -72,7 +71,9 @@ public StorPoolSetVolumeEncryptionAnswer execute(StorPoolSetVolumeEncryptionComm } logger.debug(String.format("StorPoolSetVolumeEncryptionCommandWrapper disk=%s", disk)); } catch (Exception e) { - new Answer(command, e); + String err = String.format("Could not encrypt volume [%s] due to: %s", volume.getPath(), e.getMessage()); + logger.error(err, e); + return new StorPoolSetVolumeEncryptionAnswer(command, false, err); } finally { StorPoolStorageAdaptor.attachOrDetachVolume("detach", "volume", volume.getPath()); volume.clearPassphrase(); @@ -83,13 +84,16 @@ public StorPoolSetVolumeEncryptionAnswer execute(StorPoolSetVolumeEncryptionComm private KVMPhysicalDisk encryptRootDisk(StorPoolSetVolumeEncryptionCommand command, VolumeObjectTO volume, String srcVolumeName, KVMStoragePool pool, KVMPhysicalDisk disk) { StorPoolStorageAdaptor.attachOrDetachVolume("attach", "snapshot", srcVolumeName); - KVMPhysicalDisk srcVolume = pool.getPhysicalDisk(srcVolumeName); - disk = copyPhysicalDisk(srcVolume, disk, command.getWait() * 1000, null, volume.getPassphrase()); - disk.setQemuEncryptFormat(QemuObject.EncryptFormat.LUKS); - disk.setFormat(QemuImg.PhysicalDiskFormat.RAW); - volume.setEncryptFormat(disk.getQemuEncryptFormat().toString()); - StorPoolStorageAdaptor.attachOrDetachVolume("detach", "snapshot", srcVolumeName); - return disk; + try { + KVMPhysicalDisk srcVolume = pool.getPhysicalDisk(srcVolumeName); + disk = copyPhysicalDisk(srcVolume, disk, command.getWait() * 1000, null, volume.getPassphrase()); + disk.setQemuEncryptFormat(QemuObject.EncryptFormat.LUKS); + disk.setFormat(QemuImg.PhysicalDiskFormat.RAW); + volume.setEncryptFormat(disk.getQemuEncryptFormat().toString()); + return disk; + } finally { + StorPoolStorageAdaptor.attachOrDetachVolume("detach", "snapshot", srcVolumeName); + } } private void encryptDataDisk(VolumeObjectTO volume, KVMPhysicalDisk disk) throws CryptSetupException { diff --git a/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/driver/StorPoolPrimaryDataStoreDriver.java b/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/driver/StorPoolPrimaryDataStoreDriver.java index 9a41ff76c66b..a8e3f3f5801a 100644 --- a/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/driver/StorPoolPrimaryDataStoreDriver.java +++ b/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/driver/StorPoolPrimaryDataStoreDriver.java @@ -93,7 +93,6 @@ import org.apache.commons.lang3.StringUtils; import com.cloud.agent.api.Answer; -import com.cloud.agent.api.storage.ResizeVolumeAnswer; import com.cloud.agent.api.storage.StorPoolBackupSnapshotCommand; import com.cloud.agent.api.storage.StorPoolBackupTemplateFromSnapshotCommand; import com.cloud.agent.api.storage.StorPoolCopyVolumeToSecondaryCommand; @@ -107,7 +106,6 @@ import com.cloud.agent.api.to.DataTO; import com.cloud.agent.api.to.StorageFilerTO; import com.cloud.dc.dao.ClusterDao; -import com.cloud.exception.StorageUnavailableException; import com.cloud.host.Host; import com.cloud.host.HostVO; import com.cloud.host.dao.HostDao; @@ -142,6 +140,9 @@ public class StorPoolPrimaryDataStoreDriver implements PrimaryDataStoreDriver { + // space reserved on the StorPool volume for the LUKS header of encrypted volumes + static final long LUKS_HEADER_SIZE = 2097152; + protected Logger logger = LogManager.getLogger(getClass()); @Inject @@ -299,7 +300,7 @@ public void createAsync(DataStore dataStore, DataObject data, AsyncCompletionCal try { VolumeInfo vinfo = (VolumeInfo)data; String name = vinfo.getUuid(); - Long size = vinfo.getPassphraseId() == null ? vinfo.getSize() : vinfo.getSize() + 2097152; + Long size = getStorPoolVolumeSize(vinfo, vinfo.getSize()); Long vmId = vinfo.getInstanceId(); SpConnectionDesc conn = StorPoolUtil.getSpConnection(dataStore.getUuid(), dataStore.getId(), storagePoolDetailsDao, primaryStoreDao); @@ -320,11 +321,13 @@ public void createAsync(DataStore dataStore, DataObject data, AsyncCompletionCal if (vinfo.getPassphraseId() != null) { VolumeObjectTO volume = updateVolumeObjectTO(vinfo, resp); - answer = createEncryptedVolume(dataStore, data, vinfo, size, volume, null, true); + answer = createEncryptedVolume(data, vinfo, volume, null, true, conn); } else { answer = new Answer(null, true, null); } - updateStoragePool(dataStore.getId(), size); + if (answer.getResult()) { + updateStoragePool(dataStore.getId(), size); + } StorPoolUtil.spLog("StorpoolPrimaryDataStoreDriver.createAsync volume: name=%s, uuid=%s, isAttached=%s vm=%s, payload=%s, template: %s", volumeName, vinfo.getUuid(), vinfo.isAttachedVM(), vinfo.getAttachedVmName(), vinfo.getpayload(), conn.getTemplateName()); } else { answer = new Answer(null, false, String.format("Could not create StorPool volume %s. Error: %s", name, resp.getError())); @@ -366,7 +369,8 @@ private void updateVolume(DataStore dataStore, String path, VolumeInfo vinfo) { volumeDao.update(volume.getId(), volume); } - private StorPoolSetVolumeEncryptionAnswer createEncryptedVolume(DataStore dataStore, DataObject data, VolumeInfo vinfo, Long size, VolumeObjectTO volume, String parentName, boolean isDataDisk) { + private StorPoolSetVolumeEncryptionAnswer createEncryptedVolume(DataObject data, VolumeInfo vinfo, VolumeObjectTO volume, String parentName, boolean isDataDisk, + SpConnectionDesc conn) { StorPoolSetVolumeEncryptionAnswer ans; EndPoint ep = null; if (parentName == null) { @@ -384,14 +388,28 @@ private StorPoolSetVolumeEncryptionAnswer createEncryptedVolume(DataStore dataSt ans = new StorPoolSetVolumeEncryptionAnswer(null, false, "Could not find a host with volume encryption"); } else { StorPoolSetVolumeEncryptionCommand cmd = new StorPoolSetVolumeEncryptionCommand(volume, parentName, isDataDisk); - ans = (StorPoolSetVolumeEncryptionAnswer) ep.sendMessage(cmd); - if (ans.getResult()) { - updateStoragePool(dataStore.getId(), size); + Answer answer = ep.sendMessage(cmd); + if (answer instanceof StorPoolSetVolumeEncryptionAnswer) { + ans = (StorPoolSetVolumeEncryptionAnswer) answer; + } else { + ans = new StorPoolSetVolumeEncryptionAnswer(cmd, false, answer != null ? answer.getDetails() : "Could not encrypt the volume, a null answer was returned"); } } + if (!ans.getResult()) { + deleteVolumeAfterFailedEncryption(volume, conn); + } return ans; } + private void deleteVolumeAfterFailedEncryption(VolumeObjectTO volume, SpConnectionDesc conn) { + String name = StorPoolStorageAdaptor.getVolumeNameFromPath(volume.getPath(), true); + StorPoolUtil.spLog("Deleting StorPool volume %s, because it could not be encrypted", name); + SpApiResponse resp = StorPoolUtil.volumeDelete(name, conn); + if (resp.getError() != null) { + logger.warn("Could not delete StorPool volume {} after a failed encryption due to {}", name, resp.getError()); + } + } + @Override public void resize(DataObject data, AsyncCompletionCallback callback) { String path = null; @@ -429,7 +447,7 @@ private String resizeVolume(DataObject data, String path, VolumeObject vol) { } if (err != null) { // try restoring volume to its initial size - SpApiResponse response = StorPoolUtil.volumeUpdate(name, oldSize, true, oldMaxIops, conn); + SpApiResponse response = StorPoolUtil.volumeUpdate(name, getStorPoolVolumeSize(vol, oldSize), true, oldMaxIops, conn); if (response.getError() != null) { logger.debug(String.format("Could not resize StorPool volume %s back to its original size. Error: %s", name, response.getError())); } @@ -444,6 +462,7 @@ private String resizeVolume(DataObject data, String path, VolumeObject vol) { } private void updateVolumeWithTheNewSize(VolumeObject vol, ResizeVolumePayload payload) { + long oldSize = vol.getSize(); vol.setSize(payload.newSize); vol.update(); if (payload.newMaxIops != null) { @@ -451,16 +470,21 @@ private void updateVolumeWithTheNewSize(VolumeObject vol, ResizeVolumePayload pa volume.setMaxIops(payload.newMaxIops); volumeDao.update(volume.getId(), volume); } - updateStoragePool(vol.getPoolId(), payload.newSize - vol.getSize()); + updateStoragePool(vol.getPoolId(), payload.newSize - oldSize); } - private String notifyQemuForTheNewSize(DataObject data, String err, VolumeObject vol, ResizeVolumePayload payload) - throws StorageUnavailableException { + private String notifyQemuForTheNewSize(DataObject data, String err, VolumeObject vol, ResizeVolumePayload payload) { StoragePool pool = (StoragePool)data.getDataStore(); StorPoolResizeVolumeCommand resizeCmd = new StorPoolResizeVolumeCommand(vol.getPath(), new StorageFilerTO(pool), vol.getSize(), payload.newSize, payload.shrinkOk, payload.instanceName, payload.hosts == null ? false : true); - ResizeVolumeAnswer answer = (ResizeVolumeAnswer) storageMgr.sendToPool(pool, payload.hosts, resizeCmd); + // any failure is returned as an error, so the StorPool volume is restored to its initial size + Answer answer; + try { + answer = storageMgr.sendToPool(pool, payload.hosts, resizeCmd); + } catch (Exception e) { + return String.format("Could not send the resize command for volume [%s] due to: %s", vol.getPath(), e.getMessage()); + } if (answer == null || !answer.getResult()) { err = answer != null ? answer.getDetails() : "return a null answer, resize failed for unknown reason"; @@ -482,7 +506,7 @@ private String updateStorPoolVolume(VolumeObject vol, ResizeVolumePayload payloa } SpApiResponse resp = new SpApiResponse(); if (tier != null || template != null) { - resp = updateVolumeByStorPoolQoS(payload, conn, name, tier, template); + resp = updateVolumeByStorPoolQoS(vol, payload, conn, name, tier, template); } else { resp = updateVolumeByOffering(vol, payload, conn, name); } @@ -492,24 +516,32 @@ private String updateStorPoolVolume(VolumeObject vol, ResizeVolumePayload payloa return err; } - private static SpApiResponse updateVolumeByStorPoolQoS(ResizeVolumePayload payload, SpConnectionDesc conn, String name, String tier, String template) { + private static SpApiResponse updateVolumeByStorPoolQoS(VolumeObject vol, ResizeVolumePayload payload, SpConnectionDesc conn, String name, String tier, String template) { Map tags = StorPoolHelper.addStorPoolTags(null, null, null, null, tier); - StorPoolVolumeDef spVolume = new StorPoolVolumeDef(name, payload.newSize, tags, null, null, template, null, null, + StorPoolVolumeDef spVolume = new StorPoolVolumeDef(name, getStorPoolVolumeSize(vol, payload.newSize), tags, null, null, template, null, null, payload.shrinkOk); return StorPoolUtil.volumeUpdate(spVolume, conn); } private static SpApiResponse updateVolumeByOffering(VolumeObject vol, ResizeVolumePayload payload, SpConnectionDesc conn, String name) { long maxIops = payload.newMaxIops == null ? Long.valueOf(0) : payload.newMaxIops; + Long newSize = getStorPoolVolumeSize(vol, payload.newSize); - StorPoolVolumeDef spVolume = new StorPoolVolumeDef(name, payload.newSize, null, null, maxIops, null, null, null, + StorPoolVolumeDef spVolume = new StorPoolVolumeDef(name, newSize, null, null, maxIops, null, null, null, payload.shrinkOk); StorPoolUtil.spLog( "StorpoolPrimaryDataStoreDriverImpl.resize: name=%s, uuid=%s, oldSize=%d, newSize=%s, shrinkOk=%s, maxIops=%s", - name, vol.getUuid(), vol.getSize(), payload.newSize, payload.shrinkOk, maxIops); + name, vol.getUuid(), vol.getSize(), newSize, payload.shrinkOk, maxIops); return StorPoolUtil.volumeUpdate(spVolume, conn); } + static Long getStorPoolVolumeSize(VolumeInfo vol, Long size) { + if (size == null || vol.getPassphraseId() == null) { + return size; + } + return size + LUKS_HEADER_SIZE; + } + @Override public void deleteAsync(DataStore dataStore, DataObject data, AsyncCompletionCallback callback) { String err = null; @@ -872,11 +904,10 @@ public void copyAsync(DataObject srcData, DataObject dstData, AsyncCompletionCal vinfo.getDataStore().getId(), storagePoolDetailsDao, primaryStoreDao); Long snapshotSize = templStoragePoolVO.getTemplateSize(); - boolean withoutEncryption = vinfo.getPassphraseId() == null; - long size = withoutEncryption ? vinfo.getSize() : vinfo.getSize() + 2097152; + long size = getStorPoolVolumeSize(vinfo, vinfo.getSize()); if (snapshotSize != null && size < snapshotSize) { StorPoolUtil.spLog(String.format("provided size is too small for snapshot. Provided %d, snapshot %d. Using snapshot size", size, snapshotSize)); - size = withoutEncryption ? snapshotSize : snapshotSize + 2097152; + size = getStorPoolVolumeSize(vinfo, snapshotSize); } StorPoolUtil.spLog(String.format("volume size is: %d", size)); Long vmId = vinfo.getInstanceId(); @@ -909,20 +940,22 @@ public void copyAsync(DataObject srcData, DataObject dstData, AsyncCompletionCal resp = StorPoolUtil.volumeCreate(volumeDef, conn); if (resp.getError() == null) { - updateStoragePool(dstData.getDataStore().getId(), vinfo.getSize()); updateVolumePoolType(vinfo); - if (withoutEncryption) { + if (vinfo.getPassphraseId() == null) { VolumeObjectTO to = updateVolumeObjectTO(vinfo, resp); answer = new CopyCmdAnswer(to); } else { VolumeObjectTO volume = updateVolumeObjectTO(vinfo, resp); String snapshotPath = StorPoolUtil.devPath(parentName.split("~")[1]); - answer = createEncryptedVolume(dstData.getDataStore(), dstData, vinfo, size, volume, snapshotPath, false); + answer = createEncryptedVolume(dstData, vinfo, volume, snapshotPath, false, conn); if (answer.getResult()) { answer = new CopyCmdAnswer(((StorPoolSetVolumeEncryptionAnswer) answer).getVolume()); } } + if (answer.getResult()) { + updateStoragePool(dstData.getDataStore().getId(), vinfo.getSize()); + } } else { err = String.format("Could not create Storpool volume %s. Error: %s", name, resp.getError()); } diff --git a/plugins/storage/volume/storpool/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/StorPoolResizeVolumeCommandWrapperTest.java b/plugins/storage/volume/storpool/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/StorPoolResizeVolumeCommandWrapperTest.java new file mode 100644 index 000000000000..9934ac2f13e3 --- /dev/null +++ b/plugins/storage/volume/storpool/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/StorPoolResizeVolumeCommandWrapperTest.java @@ -0,0 +1,118 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package com.cloud.hypervisor.kvm.resource.wrapper; + +import com.cloud.agent.api.storage.ResizeVolumeAnswer; +import com.cloud.agent.api.storage.StorPoolResizeVolumeCommand; +import com.cloud.agent.api.to.StorageFilerTO; +import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; +import com.cloud.hypervisor.kvm.storage.KVMPhysicalDisk; +import com.cloud.hypervisor.kvm.storage.KVMStoragePool; +import com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager; +import com.cloud.hypervisor.kvm.storage.StorPoolStorageAdaptor; +import com.cloud.storage.Storage.StoragePoolType; +import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.utils.script.Script; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.MockedConstruction; +import org.mockito.MockedStatic; +import org.mockito.Mockito; +import org.mockito.junit.MockitoJUnitRunner; + +import static org.mockito.Mockito.when; + +@RunWith(MockitoJUnitRunner.class) +public class StorPoolResizeVolumeCommandWrapperTest { + + private static final String VOLUME_PATH = "/dev/storpool-byid/t.t.t"; + private static final long GiB = 1024L * 1024 * 1024; + + @Mock + private LibvirtComputingResource libvirtComputingResource; + @Mock + private KVMStoragePoolManager storagePoolMgr; + @Mock + private KVMStoragePool pool; + @Mock + private StorageFilerTO storageFilerTO; + + private MockedStatic storPoolStorageAdaptor; + private final StorPoolResizeVolumeCommandWrapper wrapper = new StorPoolResizeVolumeCommandWrapper(); + + @Before + public void setUp() { + storPoolStorageAdaptor = Mockito.mockStatic(StorPoolStorageAdaptor.class); + + KVMPhysicalDisk disk = new KVMPhysicalDisk(VOLUME_PATH, "t.t.t", pool); + disk.setVirtualSize(20 * GiB); + + when(storageFilerTO.getType()).thenReturn(StoragePoolType.StorPool); + when(storageFilerTO.getUuid()).thenReturn("pool-uuid"); + when(libvirtComputingResource.getStoragePoolMgr()).thenReturn(storagePoolMgr); + when(storagePoolMgr.getStoragePool(StoragePoolType.StorPool, "pool-uuid")).thenReturn(pool); + when(pool.getPhysicalDisk(VOLUME_PATH)).thenReturn(disk); + Mockito.lenient().when(libvirtComputingResource.getResizeVolumePath()).thenReturn("resizevolume.sh"); + Mockito.lenient().when(libvirtComputingResource.getCmdsTimeout()).thenReturn(60); + } + + @After + public void tearDown() { + storPoolStorageAdaptor.close(); + } + + @Test + public void testResizeSucceedsWhenDetachFails() { + mockDetachFailure(); + + try (MockedConstruction