From 9e59f70b03eb333f8b510c0c143b30af1b9cd020 Mon Sep 17 00:00:00 2001 From: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com> Date: Fri, 2 Oct 2026 15:07:18 +0530 Subject: [PATCH] KVM: abort disk-only VM snapshot merges cleanly instead of leaving volumes stale Deleting a disk-only VM snapshot merged each volume on its own and reported a single result, and the host kept merging after the management server had stopped waiting, so a timeout or a failure on one volume left already-merged volumes pointing to deleted files. The host now holds active commits at the ready state and pivots them only once every volume is ready, cancels every job (leaving each backing chain unchanged) when one fails or the command's wait is about to run out, and reports which volumes were merged. The management server sends its wait to the host, records exactly the merged volumes, marks the VM snapshot as Error on failure, and resumes the remaining volumes when the deletion is retried. Fixes #14030 --- .../MergeDiskOnlyVmSnapshotAnswer.java | 44 ++ ...KvmFileBasedStorageVmSnapshotStrategy.java | 188 +++++-- ...ileBasedStorageVmSnapshotStrategyTest.java | 182 ++++++ .../resource/LibvirtComputingResource.java | 4 + ...MergeDiskOnlyVMSnapshotCommandWrapper.java | 525 ++++++++++++++++-- ...eDiskOnlyVMSnapshotCommandWrapperTest.java | 465 ++++++++++++++++ 6 files changed, 1322 insertions(+), 86 deletions(-) create mode 100644 core/src/main/java/com/cloud/agent/api/storage/MergeDiskOnlyVmSnapshotAnswer.java create mode 100644 plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMergeDiskOnlyVMSnapshotCommandWrapperTest.java diff --git a/core/src/main/java/com/cloud/agent/api/storage/MergeDiskOnlyVmSnapshotAnswer.java b/core/src/main/java/com/cloud/agent/api/storage/MergeDiskOnlyVmSnapshotAnswer.java new file mode 100644 index 000000000000..f79846f44fc9 --- /dev/null +++ b/core/src/main/java/com/cloud/agent/api/storage/MergeDiskOnlyVmSnapshotAnswer.java @@ -0,0 +1,44 @@ +/* + * 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.agent.api.storage; + +import com.cloud.agent.api.Answer; +import com.cloud.agent.api.Command; + +import java.util.ArrayList; +import java.util.List; + +/** + * Answer to {@link MergeDiskOnlyVmSnapshotCommand}. Besides the overall result, it reports which volumes had their merge applied on the host, so that the + * management server can update its metadata for exactly those volumes, even when the merge of other volumes failed or was aborted. The volumes not listed + * were left with their backing chain unchanged. + */ +public class MergeDiskOnlyVmSnapshotAnswer extends Answer { + + private List mergedVolumeUuids; + + public MergeDiskOnlyVmSnapshotAnswer(Command command, boolean success, String details, List mergedVolumeUuids) { + super(command, success, details); + this.mergedVolumeUuids = mergedVolumeUuids == null ? new ArrayList<>() : mergedVolumeUuids; + } + + public List getMergedVolumeUuids() { + return mergedVolumeUuids; + } +} diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java index 79e2a9722039..571e87a8f88b 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java @@ -18,11 +18,13 @@ */ package org.apache.cloudstack.storage.vmsnapshot; +import com.cloud.agent.AgentManager; import com.cloud.agent.api.Answer; import com.cloud.agent.api.VMSnapshotTO; import com.cloud.agent.api.storage.CreateDiskOnlyVmSnapshotAnswer; import com.cloud.agent.api.storage.CreateDiskOnlyVmSnapshotCommand; import com.cloud.agent.api.storage.DeleteDiskOnlyVmSnapshotCommand; +import com.cloud.agent.api.storage.MergeDiskOnlyVmSnapshotAnswer; import com.cloud.agent.api.storage.MergeDiskOnlyVmSnapshotCommand; import com.cloud.agent.api.storage.RevertDiskOnlyVmSnapshotAnswer; import com.cloud.agent.api.storage.RevertDiskOnlyVmSnapshotCommand; @@ -76,10 +78,12 @@ import javax.inject.Inject; import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.NoSuchElementException; import java.util.Objects; +import java.util.Set; import java.util.stream.Collectors; public class KvmFileBasedStorageVmSnapshotStrategy extends StorageVMSnapshotStrategy { @@ -137,11 +141,22 @@ public boolean deleteVMSnapshot(VMSnapshot vmSnapshot) { VMSnapshotVO vmSnapshotBeingDeleted = (VMSnapshotVO) vmSnapshot; Long hostId = pickHostForNvramSidecarCleanup(vmSnapshotBeingDeleted, userVm, "delete"); validateHostSupportsNvramSidecarCleanup(vmSnapshotBeingDeleted, hostId, "delete"); - long virtualSize = 0; boolean isCurrent = vmSnapshotBeingDeleted.getCurrent(); transitStateWithoutThrow(vmSnapshotBeingDeleted, VMSnapshot.Event.ExpungeRequested); + try { + return deleteVMSnapshotAfterExpungeRequested(vmSnapshotBeingDeleted, userVm, hostId, isCurrent); + } catch (RuntimeException ex) { + logger.error("Failed to delete VM snapshot [{}]; it will be marked as [{}]. The volumes that were merged before the failure had their metadata updated;" + + " the others were left unchanged on storage, and deleting the VM snapshot again will retry their merge.", vmSnapshot.getUuid(), VMSnapshot.State.Error, ex); + transitStateWithoutThrow(vmSnapshotBeingDeleted, VMSnapshot.Event.OperationFailed); + throw ex; + } + } + + private boolean deleteVMSnapshotAfterExpungeRequested(VMSnapshotVO vmSnapshotBeingDeleted, UserVmVO userVm, Long hostId, boolean isCurrent) { + long virtualSize = 0; List volumeTOs = vmSnapshotHelper.getVolumeTOList(vmSnapshotBeingDeleted.getVmId()); List snapshotChildren = vmSnapshotDao.listByParentAndStateIn(vmSnapshotBeingDeleted.getId(), VMSnapshot.State.Ready, VMSnapshot.State.Hidden); PrimaryDataStoreTO nvramPrimaryDataStore = getPrimaryDataStoreForNvramCleanup(vmSnapshotBeingDeleted, volumeTOs); @@ -153,18 +168,21 @@ public boolean deleteVMSnapshot(VMSnapshot vmSnapshot) { if (isCurrent && numberOfChildren == 0) { volumeSnapshotVos = mergeCurrentDeltaOnSnapshot(vmSnapshotBeingDeleted, userVm, hostId, volumeTOs); } else if (numberOfChildren == 0) { - logger.debug("Deleting VM snapshot [{}] as no snapshots/volumes depend on it.", vmSnapshot.getUuid()); + logger.debug("Deleting VM snapshot [{}] as no snapshots/volumes depend on it.", vmSnapshotBeingDeleted.getUuid()); volumeSnapshotVos = deleteSnapshot(vmSnapshotBeingDeleted, hostId); - mergeOldSiblingWithOldParentIfOldParentIsDead(vmSnapshotDao.findByIdIncludingRemoved(vmSnapshotBeingDeleted.getParent()), userVm, hostId, volumeTOs); + VMSnapshotVO oldParent = vmSnapshotDao.findByIdIncludingRemoved(vmSnapshotBeingDeleted.getParent()); + try { + mergeOldSiblingWithOldParentIfOldParentIsDead(oldParent, userVm, hostId, volumeTOs); + } catch (CloudRuntimeException ex) { + logger.warn("Deleted VM snapshot [{}], but failed to merge its hidden parent [{}] with its remaining child; the parent will be kept hidden on storage.", + vmSnapshotBeingDeleted.getUuid(), oldParent.getUuid(), ex); + } } else if (!isCurrent && numberOfChildren == 1) { VMSnapshotVO childSnapshot = snapshotChildren.get(0); volumeSnapshotVos = mergeSnapshots(vmSnapshotBeingDeleted, childSnapshot, userVm, volumeTOs, hostId); } - for (SnapshotVO snapshotVO : volumeSnapshotVos) { - snapshotVO.setState(Snapshot.State.Destroyed); - snapshotDao.update(snapshotVO.getId(), snapshotVO); - } + markVolumeSnapshotsAsDestroyed(volumeSnapshotVos); for (VolumeObjectTO volumeTo : volumeTOs) { publishUsageEvent(EventTypes.EVENT_VM_SNAPSHOT_DELETE, vmSnapshotBeingDeleted, userVm, volumeTo); @@ -306,10 +324,7 @@ private void mergeOldSiblingWithOldParentIfOldParentIsDead(VMSnapshotVO oldParen snapshotVos = mergeSnapshots(oldParent, oldSibling, userVm, volumeTOs, hostId); } - for (SnapshotVO snapshotVO : snapshotVos) { - snapshotVO.setState(Snapshot.State.Destroyed); - snapshotDao.update(snapshotVO.getId(), snapshotVO); - } + markVolumeSnapshotsAsDestroyed(snapshotVos); deleteNvramSnapshotIfNeeded(oldParent, hostId, nvramPrimaryDataStore); @@ -436,29 +451,39 @@ private List mergeSnapshots(VMSnapshotVO vmSnapshotVO, VMSnapshotVO for (VolumeObjectTO volumeObjectTO : volumeObjectTOS) { snapshotMergeTreeToList.stream().filter(snapshotTree -> Objects.equals(((SnapshotObjectTO) snapshotTree.getParent()).getVolume().getId(), volumeObjectTO.getId())) .findFirst() - .orElseThrow(() -> new CloudRuntimeException(String.format("Failed to find volume snapshot for volume [%s].", volumeObjectTO.getUuid()))) - .addGrandChild(volumeObjectTO); + .ifPresentOrElse(snapshotTree -> snapshotTree.addGrandChild(volumeObjectTO), + () -> logger.debug("Volume [{}] has no snapshot left to merge in VM snapshot [{}]; it was merged by a previous attempt.", + volumeObjectTO.getUuid(), vmSnapshotVO.getUuid())); } } - MergeDiskOnlyVmSnapshotCommand mergeDiskOnlyVMSnapshotCommand = new MergeDiskOnlyVmSnapshotCommand(snapshotMergeTreeToList, userVm.getState(), userVm.getName()); - Answer answer = agentMgr.easySend(hostId, mergeDiskOnlyVMSnapshotCommand); - if (answer == null || !answer.getResult()) { - throw new CloudRuntimeException(String.format("Failed to merge VM snapshot [%s] due to %s.", vmSnapshotVO.getUuid(), answer != null ? answer.getDetails() : "Communication failure")); - } - - logger.debug("Updating metadata of VM snapshot [{}] and its child [{}].", vmSnapshotVO.getUuid(), childSnapshot.getUuid()); List snapshotVOList = new ArrayList<>(); - for (SnapshotMergeTreeTO snapshotMergeTreeTO : snapshotMergeTreeToList) { - SnapshotObjectTO childTO = (SnapshotObjectTO) snapshotMergeTreeTO.getChild(); - SnapshotObjectTO parentTO = (SnapshotObjectTO) snapshotMergeTreeTO.getParent(); + if (snapshotMergeTreeToList.isEmpty()) { + logger.debug("Every volume of VM snapshot [{}] was merged with its child [{}] by a previous attempt.", vmSnapshotVO.getUuid(), childSnapshot.getUuid()); + } else { + MergeDiskOnlyVmSnapshotCommand mergeDiskOnlyVMSnapshotCommand = new MergeDiskOnlyVmSnapshotCommand(snapshotMergeTreeToList, userVm.getState(), userVm.getName()); + Answer answer = sendMergeCommand(hostId, mergeDiskOnlyVMSnapshotCommand); + Set mergedVolumeUuids = getMergedVolumeUuids(answer, snapshotMergeTreeToList); + + logger.debug("Updating metadata of VM snapshot [{}] and its child [{}] for the merged volumes [{}].", vmSnapshotVO.getUuid(), childSnapshot.getUuid(), + mergedVolumeUuids); + for (SnapshotMergeTreeTO snapshotMergeTreeTO : snapshotMergeTreeToList) { + SnapshotObjectTO childTO = (SnapshotObjectTO) snapshotMergeTreeTO.getChild(); + SnapshotObjectTO parentTO = (SnapshotObjectTO) snapshotMergeTreeTO.getParent(); + if (!mergedVolumeUuids.contains(parentTO.getVolume().getUuid())) { + continue; + } - SnapshotDataStoreVO childSnapshotDataStoreVO = snapshotDataStoreDao.findBySnapshotIdInAnyState(childTO.getId(), DataStoreRole.Primary); - childSnapshotDataStoreVO.setInstallPath(parentTO.getPath()); - snapshotDataStoreDao.update(childSnapshotDataStoreVO.getId(), childSnapshotDataStoreVO); + SnapshotDataStoreVO childSnapshotDataStoreVO = snapshotDataStoreDao.findBySnapshotIdInAnyState(childTO.getId(), DataStoreRole.Primary); + childSnapshotDataStoreVO.setInstallPath(parentTO.getPath()); + snapshotDataStoreDao.update(childSnapshotDataStoreVO.getId(), childSnapshotDataStoreVO); - snapshotDataStoreDao.expungeReferenceBySnapshotIdAndDataStoreRole(parentTO.getId(), childSnapshotDataStoreVO.getDataStoreId(), DataStoreRole.Primary); - snapshotVOList.add(snapshotDao.findById(parentTO.getId())); + snapshotDataStoreDao.expungeReferenceBySnapshotIdAndDataStoreRole(parentTO.getId(), childSnapshotDataStoreVO.getDataStoreId(), DataStoreRole.Primary); + removeVolumeSnapshotFromVmSnapshot(vmSnapshotVO, parentTO.getId()); + snapshotVOList.add(snapshotDao.findById(parentTO.getId())); + } + + failIfNotEveryVolumeWasMerged(vmSnapshotVO, answer, snapshotMergeTreeToList, mergedVolumeUuids, snapshotVOList); } childSnapshot.setParent(vmSnapshotVO.getParent()); @@ -474,31 +499,42 @@ private List mergeCurrentDeltaOnSnapshot(VMSnapshotVO vmSnapshotVo, for (VolumeObjectTO volumeObjectTO : volumeObjectTOS) { SnapshotDataStoreVO volumeParentSnapshot = volumeSnapshots.stream().filter(snapshot -> Objects.equals(snapshot.getVolumeId(), volumeObjectTO.getId())) - .findFirst() - .orElseThrow(() -> new CloudRuntimeException(String.format("Failed to find volume snapshot for volume [%s].", volumeObjectTO.getUuid()))); + .findFirst().orElse(null); + if (volumeParentSnapshot == null) { + logger.debug("Volume [{}] has no snapshot left to merge in VM snapshot [{}]; it was merged by a previous attempt.", volumeObjectTO.getUuid(), + vmSnapshotVo.getUuid()); + continue; + } DataTO parentSnapshot = snapshotDataFactory.getSnapshot(volumeParentSnapshot.getSnapshotId(), volumeParentSnapshot.getDataStoreId(), DataStoreRole.Primary).getTO(); snapshotMergeTreeTOList.add(new SnapshotMergeTreeTO(parentSnapshot, volumeObjectTO, new ArrayList<>())); } - MergeDiskOnlyVmSnapshotCommand mergeDiskOnlyVMSnapshotCommand = new MergeDiskOnlyVmSnapshotCommand(snapshotMergeTreeTOList, userVmVO.getState(), userVmVO.getName()); - - Answer answer = agentMgr.easySend(hostId, mergeDiskOnlyVMSnapshotCommand); - if (answer == null || !answer.getResult()) { - throw new CloudRuntimeException(String.format("Failed to delete VM snapshot [%s] due to %s.", vmSnapshotVo.getUuid(), answer != null ? answer.getDetails() : "Communication failure")); - } - - logger.debug("Updating metadata of VM snapshot [{}].", vmSnapshotVo.getUuid()); List snapshotVOList = new ArrayList<>(); - for (SnapshotMergeTreeTO snapshotMergeTreeTO : snapshotMergeTreeTOList) { - VolumeObjectTO volumeObjectTO = (VolumeObjectTO) snapshotMergeTreeTO.getChild(); - SnapshotObjectTO parentTO = (SnapshotObjectTO) snapshotMergeTreeTO.getParent(); + if (snapshotMergeTreeTOList.isEmpty()) { + logger.debug("Every volume of VM snapshot [{}] was merged with the current delta by a previous attempt.", vmSnapshotVo.getUuid()); + } else { + MergeDiskOnlyVmSnapshotCommand mergeDiskOnlyVMSnapshotCommand = new MergeDiskOnlyVmSnapshotCommand(snapshotMergeTreeTOList, userVmVO.getState(), userVmVO.getName()); + Answer answer = sendMergeCommand(hostId, mergeDiskOnlyVMSnapshotCommand); + Set mergedVolumeUuids = getMergedVolumeUuids(answer, snapshotMergeTreeTOList); + + logger.debug("Updating metadata of VM snapshot [{}] for the merged volumes [{}].", vmSnapshotVo.getUuid(), mergedVolumeUuids); + for (SnapshotMergeTreeTO snapshotMergeTreeTO : snapshotMergeTreeTOList) { + VolumeObjectTO volumeObjectTO = (VolumeObjectTO) snapshotMergeTreeTO.getChild(); + SnapshotObjectTO parentTO = (SnapshotObjectTO) snapshotMergeTreeTO.getParent(); + if (!mergedVolumeUuids.contains(parentTO.getVolume().getUuid())) { + continue; + } - VolumeVO volumeVO = volumeDao.findById(volumeObjectTO.getId()); - volumeVO.setPath(parentTO.getPath()); - volumeDao.update(volumeVO.getId(), volumeVO); + VolumeVO volumeVO = volumeDao.findById(volumeObjectTO.getId()); + volumeVO.setPath(parentTO.getPath()); + volumeDao.update(volumeVO.getId(), volumeVO); - snapshotDataStoreDao.expungeReferenceBySnapshotIdAndDataStoreRole(parentTO.getId(), volumeVO.getPoolId(), DataStoreRole.Primary); - snapshotVOList.add(snapshotDao.findById(parentTO.getId())); + snapshotDataStoreDao.expungeReferenceBySnapshotIdAndDataStoreRole(parentTO.getId(), volumeVO.getPoolId(), DataStoreRole.Primary); + removeVolumeSnapshotFromVmSnapshot(vmSnapshotVo, parentTO.getId()); + snapshotVOList.add(snapshotDao.findById(parentTO.getId())); + } + + failIfNotEveryVolumeWasMerged(vmSnapshotVo, answer, snapshotMergeTreeTOList, mergedVolumeUuids, snapshotVOList); } vmSnapshotVo.setCurrent(false); @@ -511,6 +547,66 @@ private List mergeCurrentDeltaOnSnapshot(VMSnapshotVO vmSnapshotVo, return snapshotVOList; } + /** + * Sends the merge command with the wait that the management server will use for it, so that the host aborts the merge before the management server + * stops waiting for the answer, instead of finishing it after the management server has already given up. + */ + protected Answer sendMergeCommand(Long hostId, MergeDiskOnlyVmSnapshotCommand mergeDiskOnlyVmSnapshotCommand) { + if (mergeDiskOnlyVmSnapshotCommand.getWait() <= 0) { + mergeDiskOnlyVmSnapshotCommand.setWait(AgentManager.Wait.value()); + } + return agentMgr.easySend(hostId, mergeDiskOnlyVmSnapshotCommand); + } + + /** + * Returns the UUIDs of the volumes that the host reports as merged. Hosts that do not report the volumes individually either merged all of them or none. + */ + protected Set getMergedVolumeUuids(Answer answer, List snapshotMergeTreeTOList) { + if (answer instanceof MergeDiskOnlyVmSnapshotAnswer) { + return new HashSet<>(((MergeDiskOnlyVmSnapshotAnswer) answer).getMergedVolumeUuids()); + } + if (answer != null && answer.getResult()) { + return snapshotMergeTreeTOList.stream().map(this::getVolumeUuid).collect(Collectors.toSet()); + } + return new HashSet<>(); + } + + private void failIfNotEveryVolumeWasMerged(VMSnapshotVO vmSnapshotVO, Answer answer, List snapshotMergeTreeTOList, Set mergedVolumeUuids, + List mergedVolumeSnapshots) { + List notMergedVolumeUuids = snapshotMergeTreeTOList.stream().map(this::getVolumeUuid) + .filter(volumeUuid -> !mergedVolumeUuids.contains(volumeUuid)) + .collect(Collectors.toList()); + if (notMergedVolumeUuids.isEmpty()) { + return; + } + + markVolumeSnapshotsAsDestroyed(mergedVolumeSnapshots); + throw new CloudRuntimeException(String.format("Failed to merge volumes [%s] of VM snapshot [%s] due to %s.", StringUtils.join(notMergedVolumeUuids, ", "), + vmSnapshotVO.getUuid(), answer != null ? answer.getDetails() : "Communication failure")); + } + + private String getVolumeUuid(SnapshotMergeTreeTO snapshotMergeTreeTO) { + return ((SnapshotObjectTO) snapshotMergeTreeTO.getParent()).getVolume().getUuid(); + } + + /** + * Removes a merged volume snapshot from the VM snapshot, so that a later attempt to delete the VM snapshot only merges the volumes that are left. + */ + private void removeVolumeSnapshotFromVmSnapshot(VMSnapshotVO vmSnapshotVO, long volumeSnapshotId) { + for (VMSnapshotDetailsVO vmSnapshotDetailsVO : vmSnapshotDetailsDao.findDetails(vmSnapshotVO.getId(), KVM_FILE_BASED_STORAGE_SNAPSHOT)) { + if (String.valueOf(volumeSnapshotId).equals(vmSnapshotDetailsVO.getValue())) { + vmSnapshotDetailsDao.remove(vmSnapshotDetailsVO.getId()); + } + } + } + + private void markVolumeSnapshotsAsDestroyed(List snapshotVOs) { + for (SnapshotVO snapshotVO : snapshotVOs) { + snapshotVO.setState(Snapshot.State.Destroyed); + snapshotDao.update(snapshotVO.getId(), snapshotVO); + } + } + /** * Takes a disk-only VM snapshot, exceptions thrown will be caught deeper in the stack and treated there. * @param vmSnapshot the definition of the VM Snapshot that will be created. diff --git a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java index 35b1f9ff65bf..dbf3c9e4a5c6 100644 --- a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java +++ b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java @@ -21,6 +21,8 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyString; @@ -30,12 +32,16 @@ import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; +import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.Set; import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory; import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo; @@ -57,8 +63,12 @@ import com.cloud.agent.api.Answer; import com.cloud.agent.api.storage.CreateDiskOnlyVmSnapshotAnswer; import com.cloud.agent.api.storage.DeleteDiskOnlyVmSnapshotCommand; +import com.cloud.agent.api.storage.MergeDiskOnlyVmSnapshotAnswer; +import com.cloud.agent.api.storage.MergeDiskOnlyVmSnapshotCommand; import com.cloud.agent.api.storage.RevertDiskOnlyVmSnapshotAnswer; import com.cloud.agent.api.storage.RevertDiskOnlyVmSnapshotCommand; +import com.cloud.agent.api.storage.SnapshotMergeTreeTO; +import com.cloud.agent.api.to.DataTO; import com.cloud.alert.AlertManager; import com.cloud.host.DetailVO; import com.cloud.host.Host; @@ -66,8 +76,11 @@ import com.cloud.host.dao.HostDao; import com.cloud.host.dao.HostDetailsDao; import com.cloud.storage.DataStoreRole; +import com.cloud.storage.Snapshot; +import com.cloud.storage.SnapshotVO; import com.cloud.storage.Volume; import com.cloud.storage.VolumeVO; +import com.cloud.storage.dao.SnapshotDao; import com.cloud.storage.dao.VolumeDao; import com.cloud.user.ResourceLimitService; import com.cloud.uservm.UserVm; @@ -547,4 +560,173 @@ private void mockCandidateHostScope(long vmId, long hostId) { when(host.getId()).thenReturn(hostId); when(hostDao.listAllUpAndEnabledNonHAHosts(Host.Type.Routing, clusterId, podId, dataCenterId, null)).thenReturn(List.of(host)); } + + @Test + public void testGetMergedVolumeUuidsUsesPerVolumeResultsAndFallsBackForLegacyAnswers() { + List trees = List.of(mergeTree(volumeTo(1L, "vol-a"), "snap-a", 101L), mergeTree(volumeTo(2L, "vol-b"), "snap-b", 102L)); + + assertEquals(Set.of("vol-a"), strategy.getMergedVolumeUuids(new MergeDiskOnlyVmSnapshotAnswer(null, false, "aborted", List.of("vol-a")), trees)); + assertEquals(Set.of("vol-a", "vol-b"), strategy.getMergedVolumeUuids(new Answer(null, true, null), trees)); + assertTrue(strategy.getMergedVolumeUuids(new Answer(null, false, "failed"), trees).isEmpty()); + assertTrue(strategy.getMergedVolumeUuids(null, trees).isEmpty()); + } + + @Test + public void testSendMergeCommandExposesTheManagementServerWaitToTheHost() { + MergeDiskOnlyVmSnapshotCommand command = new MergeDiskOnlyVmSnapshotCommand(new ArrayList<>(), VirtualMachine.State.Running, "vm"); + + strategy.sendMergeCommand(40L, command); + assertEquals(AgentManager.Wait.value().intValue(), command.getWait()); + + command.setWait(20); + strategy.sendMergeCommand(40L, command); + assertEquals(20, command.getWait()); + verify(agentMgr, times(2)).easySend(40L, command); + } + + @Test + public void testMergeCurrentDeltaOnSnapshotOnlyUpdatesTheVolumesThatWereMerged() throws Exception { + VMSnapshotVO vmSnapshot = mock(VMSnapshotVO.class); + VolumeObjectTO volumeA = volumeTo(1L, "vol-a"); + VolumeObjectTO volumeB = volumeTo(2L, "vol-b"); + VMSnapshotDetailsVO detailA = volumeSnapshotDetail(11L, 101L); + VMSnapshotDetailsVO detailB = volumeSnapshotDetail(12L, 102L); + VolumeVO volumeVoA = mock(VolumeVO.class); + SnapshotVO snapshotVoA = mock(SnapshotVO.class); + strategy.snapshotDao = mock(SnapshotDao.class); + + when(vmSnapshot.getId()).thenReturn(20L); + when(vmSnapshot.getUuid()).thenReturn("vm-snapshot"); + when(vmSnapshotDetailsDao.findDetails(eq(20L), anyString())).thenReturn(List.of(detailA, detailB)); + mockVolumeSnapshot(volumeA, 101L, "snap-a"); + mockVolumeSnapshot(volumeB, 102L, "snap-b"); + when(strategy.volumeDao.findById(1L)).thenReturn(volumeVoA); + when(volumeVoA.getId()).thenReturn(1L); + when(volumeVoA.getPoolId()).thenReturn(5L); + when(snapshotVoA.getId()).thenReturn(101L); + when(strategy.snapshotDao.findById(101L)).thenReturn(snapshotVoA); + when(agentMgr.easySend(eq(40L), any(MergeDiskOnlyVmSnapshotCommand.class))) + .thenReturn(new MergeDiskOnlyVmSnapshotAnswer(null, false, "the block commits did not finish in time", List.of("vol-a"))); + + try { + invokeMergeCurrentDeltaOnSnapshot(vmSnapshot, List.of(volumeA, volumeB)); + fail("The merge should fail as volume [vol-b] was not merged."); + } catch (CloudRuntimeException ex) { + assertTrue(ex.getMessage(), ex.getMessage().contains("vol-b")); + } + + verify(volumeVoA).setPath("snap-a"); + verify(strategy.volumeDao).update(1L, volumeVoA); + verify(strategy.volumeDao, never()).findById(2L); + verify(snapshotDataStoreDao).expungeReferenceBySnapshotIdAndDataStoreRole(101L, 5L, DataStoreRole.Primary); + verify(snapshotDataStoreDao, never()).expungeReferenceBySnapshotIdAndDataStoreRole(eq(102L), anyLong(), any()); + verify(vmSnapshotDetailsDao).remove(11L); + verify(vmSnapshotDetailsDao, never()).remove(12L); + verify(snapshotVoA).setState(Snapshot.State.Destroyed); + verify(vmSnapshot, never()).setCurrent(Mockito.anyBoolean()); + } + + @Test + public void testMergeCurrentDeltaOnSnapshotOnlyMergesTheVolumesLeftByAPreviousAttempt() throws Exception { + VMSnapshotVO vmSnapshot = mock(VMSnapshotVO.class); + VolumeObjectTO volumeA = volumeTo(1L, "vol-a"); + VolumeObjectTO volumeB = volumeTo(2L, "vol-b"); + VolumeVO volumeVoB = mock(VolumeVO.class); + VMSnapshotDetailsVO detailB = volumeSnapshotDetail(12L, 102L); + strategy.snapshotDao = mock(SnapshotDao.class); + + when(vmSnapshot.getId()).thenReturn(20L); + when(vmSnapshot.getUuid()).thenReturn("vm-snapshot"); + when(vmSnapshot.getParent()).thenReturn(null); + when(vmSnapshotDetailsDao.findDetails(eq(20L), anyString())).thenReturn(List.of(detailB)); + mockVolumeSnapshot(volumeB, 102L, "snap-b"); + when(strategy.volumeDao.findById(2L)).thenReturn(volumeVoB); + when(volumeVoB.getId()).thenReturn(2L); + when(agentMgr.easySend(eq(40L), any(MergeDiskOnlyVmSnapshotCommand.class))) + .thenReturn(new MergeDiskOnlyVmSnapshotAnswer(null, true, null, List.of("vol-b"))); + + invokeMergeCurrentDeltaOnSnapshot(vmSnapshot, List.of(volumeA, volumeB)); + + ArgumentCaptor commandCaptor = ArgumentCaptor.forClass(MergeDiskOnlyVmSnapshotCommand.class); + verify(agentMgr).easySend(eq(40L), commandCaptor.capture()); + assertEquals(1, commandCaptor.getValue().getSnapshotMergeTreeToList().size()); + assertSame(volumeB, commandCaptor.getValue().getSnapshotMergeTreeToList().get(0).getChild()); + assertEquals(AgentManager.Wait.value().intValue(), commandCaptor.getValue().getWait()); + verify(volumeVoB).setPath("snap-b"); + verify(vmSnapshot).setCurrent(false); + } + + @Test(expected = CloudRuntimeException.class) + public void testDeleteVMSnapshotMarksSnapshotAsErrorWhenTheDeletionFails() throws Exception { + long vmId = 10L; + UserVmVO userVm = mock(UserVmVO.class); + VMSnapshotVO vmSnapshot = mock(VMSnapshotVO.class); + + when(vmSnapshot.getVmId()).thenReturn(vmId); + when(vmSnapshot.getId()).thenReturn(20L); + when(vmSnapshot.getUuid()).thenReturn("vm-snapshot"); + when(userVm.getState()).thenReturn(VirtualMachine.State.Running); + when(strategy.userVmDao.findById(vmId)).thenReturn(userVm); + when(vmSnapshotHelper.pickRunningHost(vmId)).thenReturn(40L); + when(vmSnapshotHelper.getVolumeTOList(vmId)).thenThrow(new CloudRuntimeException("Failed to merge volumes [vol-b] of VM snapshot [vm-snapshot].")); + + try { + strategy.deleteVMSnapshot(vmSnapshot); + } finally { + InOrder inOrder = inOrder(vmSnapshotHelper); + inOrder.verify(vmSnapshotHelper).vmSnapshotStateTransitTo(vmSnapshot, VMSnapshot.Event.ExpungeRequested); + inOrder.verify(vmSnapshotHelper).vmSnapshotStateTransitTo(vmSnapshot, VMSnapshot.Event.OperationFailed); + } + } + + private void invokeMergeCurrentDeltaOnSnapshot(VMSnapshotVO vmSnapshot, List volumeTOs) throws Exception { + UserVmVO userVm = mock(UserVmVO.class); + when(userVm.getState()).thenReturn(VirtualMachine.State.Running); + when(userVm.getName()).thenReturn("vm"); + + Method method = KvmFileBasedStorageVmSnapshotStrategy.class.getDeclaredMethod("mergeCurrentDeltaOnSnapshot", VMSnapshotVO.class, UserVmVO.class, Long.class, + List.class); + method.setAccessible(true); + try { + method.invoke(strategy, vmSnapshot, userVm, 40L, volumeTOs); + } catch (InvocationTargetException ex) { + throw (Exception) ex.getCause(); + } + } + + private VolumeObjectTO volumeTo(long id, String uuid) { + VolumeObjectTO volumeTO = mock(VolumeObjectTO.class); + when(volumeTO.getId()).thenReturn(id); + when(volumeTO.getUuid()).thenReturn(uuid); + return volumeTO; + } + + private SnapshotMergeTreeTO mergeTree(VolumeObjectTO volumeTO, String parentPath, long parentId) { + SnapshotObjectTO parent = mock(SnapshotObjectTO.class); + when(parent.getVolume()).thenReturn(volumeTO); + when(parent.getPath()).thenReturn(parentPath); + when(parent.getId()).thenReturn(parentId); + return new SnapshotMergeTreeTO(parent, volumeTO, new ArrayList<>()); + } + + private VMSnapshotDetailsVO volumeSnapshotDetail(long detailId, long volumeSnapshotId) { + VMSnapshotDetailsVO detail = mock(VMSnapshotDetailsVO.class); + when(detail.getId()).thenReturn(detailId); + when(detail.getValue()).thenReturn(String.valueOf(volumeSnapshotId)); + return detail; + } + + private void mockVolumeSnapshot(VolumeObjectTO volumeTO, long volumeSnapshotId, String path) { + SnapshotDataStoreVO snapshotDataStoreVO = mock(SnapshotDataStoreVO.class); + SnapshotInfo snapshotInfo = mock(SnapshotInfo.class); + DataTO parent = mergeTree(volumeTO, path, volumeSnapshotId).getParent(); + long volumeId = volumeTO.getId(); + + when(snapshotDataStoreVO.getSnapshotId()).thenReturn(volumeSnapshotId); + when(snapshotDataStoreVO.getDataStoreId()).thenReturn(5L); + when(snapshotDataStoreVO.getVolumeId()).thenReturn(volumeId); + when(snapshotDataStoreDao.findOneBySnapshotAndDatastoreRole(volumeSnapshotId, DataStoreRole.Primary)).thenReturn(snapshotDataStoreVO); + when(strategy.snapshotDataFactory.getSnapshot(volumeSnapshotId, 5L, DataStoreRole.Primary)).thenReturn(snapshotInfo); + when(snapshotInfo.getTO()).thenReturn(parent); + } } diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index a9ed0c62cab8..623d6e749e9a 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -796,6 +796,10 @@ public int getCmdsTimeout() { return cmdsTimeout; } + public int getQcow2DeltaMergeTimeout() { + return qcow2DeltaMergeTimeout; + } + public String manageSnapshotPath() { return manageSnapshotPath; } diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMergeDiskOnlyVMSnapshotCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMergeDiskOnlyVMSnapshotCommandWrapper.java index ad0678080ed7..2ac0f63fb094 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMergeDiskOnlyVMSnapshotCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMergeDiskOnlyVMSnapshotCommandWrapper.java @@ -20,6 +20,7 @@ import com.cloud.agent.api.Answer; +import com.cloud.agent.api.storage.MergeDiskOnlyVmSnapshotAnswer; import com.cloud.agent.api.storage.MergeDiskOnlyVmSnapshotCommand; import com.cloud.agent.api.storage.SnapshotMergeTreeTO; import com.cloud.agent.api.to.DataObjectType; @@ -38,19 +39,49 @@ import org.apache.cloudstack.utils.qemu.QemuImg; import org.apache.cloudstack.utils.qemu.QemuImgException; import org.apache.cloudstack.utils.qemu.QemuImgFile; +import org.apache.cloudstack.utils.security.ParserUtils; +import org.apache.commons.lang3.StringUtils; import org.libvirt.Connect; import org.libvirt.Domain; +import org.libvirt.DomainBlockJobInfo; import org.libvirt.LibvirtException; +import org.w3c.dom.Document; +import org.w3c.dom.Element; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; +import org.xml.sax.InputSource; import java.io.IOException; +import java.io.StringReader; import java.nio.file.Files; import java.nio.file.Path; +import java.util.ArrayList; +import java.util.HashMap; import java.util.List; +import java.util.Map; +import java.util.Objects; import java.util.stream.Collectors; +/** + * Merges the deltas of disk-only VM snapshots when one of them is deleted.

+ * The merge is done so that every volume either has its merge fully applied, or is left with its backing chain exactly as it was, and the answer reports + * which volumes are in which case. It is also bounded by the command's wait, so that the host never keeps merging after the management server has given + * up on the answer, which would leave the management server's metadata pointing to files that no longer exist.

+ * For a running VM, all block commits are started together. Active commits (the current delta being merged into the snapshot) are held in the ready + * state and are only pivoted once every disk is ready; if any disk fails or the deadline is reached, every job is cancelled, which leaves every disk on + * its original active image. Non-active commits have no ready phase and finish on their own, thus each of them is reported individually.

+ * For a stopped VM, every delta is first committed into its parent without changing any backing chain; the chains are only changed once all the commits + * succeeded.

+ * In every case, once the copy of a delta into its parent has started, the parent no longer holds the point-in-time state of the snapshot being deleted. + * Retrying the merge reproduces the same result. + */ @ResourceWrapper(handles = MergeDiskOnlyVmSnapshotCommand.class) public class LibvirtMergeDiskOnlyVMSnapshotCommandWrapper extends CommandWrapper { + protected static final int MIN_SAFETY_MARGIN_IN_SECONDS = 5; + protected static final int MAX_SAFETY_MARGIN_IN_SECONDS = 60; + protected static final long POLL_INTERVAL_IN_MILLIS = 1000; + @Override public Answer execute(MergeDiskOnlyVmSnapshotCommand command, LibvirtComputingResource serverResource) { VirtualMachine.State vmState = command.getVmState(); @@ -60,51 +91,103 @@ public Answer execute(MergeDiskOnlyVmSnapshotCommand command, LibvirtComputingRe return mergeDiskOnlySnapshotsForRunningVM(command, serverResource); } return mergeDiskOnlySnapshotsForStoppedVM(command, serverResource); - } catch (LibvirtException | QemuImgException | CloudRuntimeException ex) { - return new Answer(command, ex); + } catch (LibvirtException | CloudRuntimeException ex) { + logger.error("Failed to merge disk-only snapshots of VM [{}] before any volume was merged.", command.getVmName(), ex); + return new MergeDiskOnlyVmSnapshotAnswer(command, false, ex.getMessage(), new ArrayList<>()); } } - protected Answer mergeDiskOnlySnapshotsForStoppedVM(MergeDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResource resource) throws QemuImgException, LibvirtException { - QemuImg qemuImg = new QemuImg(resource.getCmdsTimeout()); - KVMStoragePoolManager storageManager = resource.getStoragePoolMgr(); + /** + * Returns how long, in seconds, the host may spend merging. When the management server informs the command's wait, a safety margin is subtracted + * from it so that the jobs can be cancelled and the answer delivered before the management server stops waiting. Otherwise, the given fallback is used. + */ + protected int getMergeTimeoutInSeconds(MergeDiskOnlyVmSnapshotCommand cmd, int fallbackInSeconds) { + int wait = cmd.getWait(); + if (wait <= 0) { + return fallbackInSeconds; + } + + int timeout = wait - getSafetyMarginInSeconds(wait); + if (timeout <= 0) { + timeout = Math.max(1, wait / 2); + } + return fallbackInSeconds > 0 ? Math.min(timeout, fallbackInSeconds) : timeout; + } + + protected int getSafetyMarginInSeconds(int wait) { + return Math.min(MAX_SAFETY_MARGIN_IN_SECONDS, Math.max(MIN_SAFETY_MARGIN_IN_SECONDS, wait / 10)); + } + protected Answer mergeDiskOnlySnapshotsForStoppedVM(MergeDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResource resource) { + KVMStoragePoolManager storageManager = resource.getStoragePoolMgr(); List snapshotMergeTreeTOList = cmd.getSnapshotMergeTreeToList(); + long deadline = System.currentTimeMillis() + getMergeTimeoutInSeconds(cmd, resource.getCmdsTimeout() / 1000) * 1000L; logger.debug("Merging disk-only snapshots for stopped VM [{}] using the following Snapshot Merge Trees [{}].", cmd.getVmName(), snapshotMergeTreeTOList); + List diskMerges = new ArrayList<>(); for (SnapshotMergeTreeTO snapshotMergeTreeTO : snapshotMergeTreeTOList) { DataTO parentTo = snapshotMergeTreeTO.getParent(); PrimaryDataStoreTO primaryDataStoreTO = (PrimaryDataStoreTO) parentTo.getDataStore(); KVMStoragePool storagePool = storageManager.getStoragePool(primaryDataStoreTO.getPoolType(), primaryDataStoreTO.getUuid()); - String childLocalPath = storagePool.getLocalPathFor(snapshotMergeTreeTO.getChild().getPath()); - - QemuImgFile parent = new QemuImgFile(storagePool.getLocalPathFor(parentTo.getPath()), QemuImg.PhysicalDiskFormat.QCOW2); - QemuImgFile child = new QemuImgFile(childLocalPath, QemuImg.PhysicalDiskFormat.QCOW2); - - logger.debug("Committing child delta [{}] into parent snapshot [{}].", parentTo, snapshotMergeTreeTO.getChild()); - qemuImg.commit(child, parent, true); + diskMerges.add(new StoppedVmDiskMerge(getVolumeUuid(snapshotMergeTreeTO), storagePool.getLocalPathFor(parentTo.getPath()), + storagePool.getLocalPathFor(snapshotMergeTreeTO.getChild().getPath()), + snapshotMergeTreeTO.getGrandChildren().stream().map(grandChild -> storagePool.getLocalPathFor(grandChild.getPath())).collect(Collectors.toList()))); + } - List grandChildren = snapshotMergeTreeTO.getGrandChildren().stream() - .map(snapshotTo -> new QemuImgFile(storagePool.getLocalPathFor(snapshotTo.getPath()), QemuImg.PhysicalDiskFormat.QCOW2)) - .collect(Collectors.toList()); + for (StoppedVmDiskMerge diskMerge : diskMerges) { + long remaining = deadline - System.currentTimeMillis(); + if (remaining <= 0) { + return abortStoppedVmMerge(cmd, String.format("the merge did not finish within the time allowed by the command's wait of [%s] seconds", cmd.getWait())); + } - logger.debug("Rebasing grandChildren [{}] into parent at [{}].", grandChildren, parent.getFileName()); - for (QemuImgFile grandChild : grandChildren) { - qemuImg.rebase(grandChild, parent, parent.getFormat().toString(), false); + QemuImgFile parent = new QemuImgFile(diskMerge.parentPath, QemuImg.PhysicalDiskFormat.QCOW2); + QemuImgFile child = new QemuImgFile(diskMerge.childPath, QemuImg.PhysicalDiskFormat.QCOW2); + logger.debug("Committing child delta [{}] into parent snapshot [{}] of volume [{}].", diskMerge.childPath, diskMerge.parentPath, diskMerge.volumeUuid); + try { + getQemuImg(remaining).commit(child, parent, true); + } catch (QemuImgException | LibvirtException ex) { + return abortStoppedVmMerge(cmd, String.format("committing [%s] into [%s] failed due to [%s]", diskMerge.childPath, diskMerge.parentPath, ex.getMessage())); } + } - logger.debug("Deleting child at [{}] as it is useless.", childLocalPath); + List mergedVolumeUuids = new ArrayList<>(); + List failures = new ArrayList<>(); + for (StoppedVmDiskMerge diskMerge : diskMerges) { try { - Files.deleteIfExists(Path.of(childLocalPath)); - } catch (IOException e) { - return new Answer(cmd, e); + if (!diskMerge.grandChildPaths.isEmpty()) { + QemuImg qemuImg = getQemuImg(resource.getCmdsTimeout()); + QemuImgFile parent = new QemuImgFile(diskMerge.parentPath, QemuImg.PhysicalDiskFormat.QCOW2); + logger.debug("Rebasing grandChildren [{}] into parent at [{}].", diskMerge.grandChildPaths, diskMerge.parentPath); + for (String grandChildPath : diskMerge.grandChildPaths) { + qemuImg.rebase(new QemuImgFile(grandChildPath, QemuImg.PhysicalDiskFormat.QCOW2), parent, parent.getFormat().toString(), false); + } + } + + logger.debug("Deleting child at [{}] as it is useless.", diskMerge.childPath); + Files.deleteIfExists(Path.of(diskMerge.childPath)); + mergedVolumeUuids.add(diskMerge.volumeUuid); + } catch (QemuImgException | LibvirtException | IOException ex) { + logger.error("Failed to finish the merge of volume [{}]; its child [{}] was kept, thus its chain is still consistent.", diskMerge.volumeUuid, + diskMerge.childPath, ex); + failures.add(String.format("volume [%s]: %s", diskMerge.volumeUuid, ex.getMessage())); } } - return new Answer(cmd, true, null); + + if (failures.isEmpty()) { + return new MergeDiskOnlyVmSnapshotAnswer(cmd, true, null, mergedVolumeUuids); + } + return new MergeDiskOnlyVmSnapshotAnswer(cmd, false, String.format("Failed to finish the merge of some volumes of VM [%s]: %s.", cmd.getVmName(), + StringUtils.join(failures, "; ")), mergedVolumeUuids); + } + + private Answer abortStoppedVmMerge(MergeDiskOnlyVmSnapshotCommand cmd, String reason) { + String details = String.format("Aborted the merge of disk-only snapshots of stopped VM [%s] because %s. No backing chain was changed.", cmd.getVmName(), reason); + logger.error(details); + return new MergeDiskOnlyVmSnapshotAnswer(cmd, false, details, new ArrayList<>()); } - protected Answer mergeDiskOnlySnapshotsForRunningVM(MergeDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResource resource) throws LibvirtException, QemuImgException { + protected Answer mergeDiskOnlySnapshotsForRunningVM(MergeDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResource resource) throws LibvirtException { String vmName = cmd.getVmName(); List snapshotMergeTreeTOList = cmd.getSnapshotMergeTreeToList(); @@ -113,38 +196,400 @@ protected Answer mergeDiskOnlySnapshotsForRunningVM(MergeDiskOnlyVmSnapshotComma Domain domain = resource.getDomain(conn, vmName); List disks = resource.getDisks(conn, vmName); KVMStoragePoolManager storageManager = resource.getStoragePoolMgr(); - QemuImg qemuImg = new QemuImg(resource.getCmdsTimeout()); + int timeout = getMergeTimeoutInSeconds(cmd, resource.getQcow2DeltaMergeTimeout()); + long deadline = System.currentTimeMillis() + timeout * 1000L; - logger.debug("Merging disk-only snapshots for running VM [{}] using the following Snapshot Merge Trees [{}].", vmName, snapshotMergeTreeTOList); + logger.debug("Merging disk-only snapshots for running VM [{}] within [{}] seconds using the following Snapshot Merge Trees [{}].", vmName, timeout, + snapshotMergeTreeTOList); + List diskMerges = new ArrayList<>(); for (SnapshotMergeTreeTO mergeTreeTO : snapshotMergeTreeTOList) { DataTO childTO = mergeTreeTO.getChild(); SnapshotObjectTO parentSnapshotTO = (SnapshotObjectTO) mergeTreeTO.getParent(); VolumeObjectTO volumeObjectTO = parentSnapshotTO.getVolume(); KVMStoragePool storagePool = libvirtUtilitiesHelper.getPrimaryPoolFromDataTo(volumeObjectTO, storageManager); - boolean active = DataObjectType.VOLUME.equals(childTO.getObjectType()); String label = resource.getDiskWithPathOfVolumeObjectTO(disks, volumeObjectTO).getDiskLabel(); - String parentSnapshotLocalPath = storagePool.getLocalPathFor(parentSnapshotTO.getPath()); - String childDeltaPath = storagePool.getLocalPathFor(childTO.getPath()); + diskMerges.add(new RunningVmDiskMerge(volumeObjectTO.getUuid(), label, storagePool.getLocalPathFor(parentSnapshotTO.getPath()), + storagePool.getLocalPathFor(childTO.getPath()), DataObjectType.VOLUME.equals(childTO.getObjectType()), + mergeTreeTO.getGrandChildren().stream().map(grandChild -> storagePool.getLocalPathFor(grandChild.getPath())).collect(Collectors.toList()))); + } + + String failure; + try { + failure = startBlockCommits(domain, vmName, diskMerges); + if (failure == null) { + failure = waitForBlockCommits(domain, vmName, diskMerges, deadline, timeout); + } + if (failure == null) { + failure = pivotActiveBlockCommits(domain, vmName, diskMerges); + } + } catch (RuntimeException ex) { + logger.error("Unexpected error while merging the disk-only snapshots of VM [{}].", vmName, ex); + failure = String.format("an unexpected error occurred [%s]", ex.getMessage()); + } + + cancelUnfinishedBlockCommits(domain, vmName, diskMerges, System.currentTimeMillis() + getSafetyMarginInSeconds(Math.max(cmd.getWait(), 0)) * 1000L); + + return finishRunningVmMerge(cmd, domain, diskMerges, failure, resource.getCmdsTimeout()); + } + + /** + * Starts the block commit of every disk. Active commits are started without pivoting, so that the pivot of every disk can be done only once all of them + * are ready. The merged-away files are not deleted by Libvirt, as they are only deleted once the merge of their disk is known to be applied. + * @return the reason of the failure, or null if every block commit was started. + */ + protected String startBlockCommits(Domain domain, String vmName, List diskMerges) { + for (RunningVmDiskMerge diskMerge : diskMerges) { + int flags = diskMerge.active ? Domain.BlockCommitFlags.ACTIVE : 0; + logger.info("Starting block commit of top file [{}] into base file [{}] on disk [{}] of VM [{}] (active: [{}]).", diskMerge.topPath, diskMerge.basePath, + diskMerge.label, vmName, diskMerge.active); + try { + domain.blockCommit(diskMerge.label, diskMerge.basePath, diskMerge.topPath, 0, flags); + } catch (LibvirtException ex) { + return String.format("starting the block commit of disk [%s] failed due to [%s]", diskMerge.label, ex.getMessage()); + } + } + return null; + } + + /** + * Waits until every active commit is ready to be pivoted and every non-active commit has finished. + * @return the reason of the failure, or null if every disk got there before the deadline. + */ + protected String waitForBlockCommits(Domain domain, String vmName, List diskMerges, long deadline, int timeout) { + while (true) { + Map diskChains; + try { + diskChains = getDiskChains(domain); + } catch (LibvirtException | CloudRuntimeException ex) { + return String.format("reading the disks of the VM failed due to [%s]", ex.getMessage()); + } + + boolean allDone = true; + for (RunningVmDiskMerge diskMerge : diskMerges) { + DiskChain diskChain = diskChains.get(diskMerge.label); + if (diskMerge.active && diskChain != null && diskChain.isActiveCommitReady()) { + continue; + } + + boolean jobRunning; + try { + jobRunning = isBlockJobRunning(domain, diskMerge.label); + } catch (LibvirtException ex) { + return String.format("reading the block job of disk [%s] failed due to [%s]", diskMerge.label, ex.getMessage()); + } + + if (jobRunning) { + allDone = false; + continue; + } + if (diskMerge.active || diskChain == null || diskChain.contains(diskMerge.topPath)) { + return String.format("the block commit of disk [%s] ended before being %s", diskMerge.label, diskMerge.active ? "ready to pivot" : "completed"); + } + diskMerge.observedApplied = true; + } + + if (allDone) { + logger.debug("Every block commit of VM [{}] is ready to be finished.", vmName); + return null; + } + if (System.currentTimeMillis() >= deadline) { + return String.format("the block commits did not finish within [%s] seconds", timeout); + } + + try { + Thread.sleep(getPollIntervalInMillis()); + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + return "the thread waiting for the block commits was interrupted"; + } + } + } + + /** + * Pivots every active commit. It is only called once all of them are ready, thus the pivots are quick; it stops at the first failure so that the + * remaining disks keep their original active image. + * @return the reason of the failure, or null if every active commit was pivoted. + */ + protected String pivotActiveBlockCommits(Domain domain, String vmName, List diskMerges) { + for (RunningVmDiskMerge diskMerge : diskMerges) { + if (!diskMerge.active) { + continue; + } + logger.debug("Pivoting disk [{}] of VM [{}] to [{}].", diskMerge.label, vmName, diskMerge.basePath); + try { + domain.blockJobAbort(diskMerge.label, Domain.BlockJobAbortFlags.PIVOT); + diskMerge.observedApplied = true; + } catch (LibvirtException ex) { + return String.format("pivoting disk [%s] failed due to [%s]", diskMerge.label, ex.getMessage()); + } + } + return null; + } + + /** + * Cancels every block job that is still running, without pivoting. Cancelling a block commit leaves the disk's backing chain unchanged. Then, waits + * until the given deadline for the jobs to be gone. + */ + protected void cancelUnfinishedBlockCommits(Domain domain, String vmName, List diskMerges, long deadline) { + List unfinished = new ArrayList<>(diskMerges); + while (!unfinished.isEmpty()) { + for (RunningVmDiskMerge diskMerge : new ArrayList<>(unfinished)) { + try { + if (!isBlockJobRunning(domain, diskMerge.label)) { + unfinished.remove(diskMerge); + continue; + } + logger.warn("Cancelling the block commit of disk [{}] of VM [{}]; its backing chain will be kept as it was.", diskMerge.label, vmName); + domain.blockJobAbort(diskMerge.label, 0); + } catch (LibvirtException ex) { + logger.warn("Failed to cancel the block commit of disk [{}] of VM [{}] due to [{}].", diskMerge.label, vmName, ex.getMessage()); + } + } + + if (unfinished.isEmpty() || System.currentTimeMillis() >= deadline) { + break; + } + try { + Thread.sleep(getPollIntervalInMillis()); + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + break; + } + } + + if (!unfinished.isEmpty()) { + logger.error("The block jobs of disks [{}] of VM [{}] could not be cancelled; they will be reported as not merged.", + unfinished.stream().map(diskMerge -> diskMerge.label).collect(Collectors.toList()), vmName); + } + } + + /** + * Reads the backing chain of every disk from Libvirt to know which merges were applied, then finishes those: off-chain grandchildren are rebased into + * the base file and the merged-away file is deleted. + */ + protected Answer finishRunningVmMerge(MergeDiskOnlyVmSnapshotCommand cmd, Domain domain, List diskMerges, String failure, int cmdsTimeout) { + String vmName = cmd.getVmName(); + Map diskChains = null; + try { + diskChains = getDiskChains(domain); + } catch (LibvirtException | CloudRuntimeException ex) { + logger.error("Failed to read the disks of VM [{}] after merging; only the pivots and completions observed while merging will be reported as merged.", + vmName, ex); + failure = StringUtils.defaultIfBlank(failure, String.format("reading the disks of the VM failed due to [%s]", ex.getMessage())); + } + + List mergedVolumeUuids = new ArrayList<>(); + List notMergedVolumeUuids = new ArrayList<>(); + for (RunningVmDiskMerge diskMerge : diskMerges) { + DiskChain diskChain = diskChains == null ? null : diskChains.get(diskMerge.label); + boolean applied = diskChains == null ? diskMerge.observedApplied : isMergeApplied(diskMerge, diskChain); + if (diskChains != null && applied != diskMerge.observedApplied) { + logger.warn("Libvirt reports the merge of volume [{}] of VM [{}] as {}applied, which differs from what was observed while merging; using Libvirt's view.", + diskMerge.volumeUuid, vmName, applied ? "" : "not "); + } + if (!applied) { + logger.info("The merge of volume [{}] of VM [{}] was not applied; its backing chain is [{}].", diskMerge.volumeUuid, vmName, diskChain); + notMergedVolumeUuids.add(diskMerge.volumeUuid); + continue; + } + + mergedVolumeUuids.add(diskMerge.volumeUuid); + finishAppliedMerge(vmName, diskMerge, diskChain, cmdsTimeout); + } + + if (failure == null && notMergedVolumeUuids.isEmpty()) { + logger.info("Merged the disk-only snapshots of every volume of VM [{}].", vmName); + return new MergeDiskOnlyVmSnapshotAnswer(cmd, true, null, mergedVolumeUuids); + } + + String details = String.format("Aborted the merge of disk-only snapshots of VM [%s] because %s. Volumes [%s] were left with their backing chain unchanged;" + + " volumes [%s] were merged.", vmName, StringUtils.defaultIfBlank(failure, "not every merge was applied"), StringUtils.join(notMergedVolumeUuids, ", "), + StringUtils.join(mergedVolumeUuids, ", ")); + logger.error(details); + return new MergeDiskOnlyVmSnapshotAnswer(cmd, false, details, mergedVolumeUuids); + } + + protected boolean isMergeApplied(RunningVmDiskMerge diskMerge, DiskChain diskChain) { + if (diskChain == null || diskChain.mirrorJob != null) { + return false; + } + if (diskMerge.active) { + return samePath(diskChain.activeSource, diskMerge.basePath); + } + return !diskChain.contains(diskMerge.topPath) && diskChain.contains(diskMerge.basePath); + } + + private void finishAppliedMerge(String vmName, RunningVmDiskMerge diskMerge, DiskChain diskChain, int cmdsTimeout) { + QemuImgFile base = new QemuImgFile(diskMerge.basePath, QemuImg.PhysicalDiskFormat.QCOW2); + boolean allGrandChildrenRebased = true; + for (String grandChildPath : diskMerge.grandChildPaths) { + if (diskChain != null && diskChain.contains(grandChildPath)) { + logger.debug("Grandchild [{}] is in the backing chain of VM [{}], thus Libvirt has already rebased it.", grandChildPath, vmName); + continue; + } + try { + getQemuImg(cmdsTimeout).rebase(new QemuImgFile(grandChildPath, QemuImg.PhysicalDiskFormat.QCOW2), base, base.getFormat().toString(), false); + } catch (QemuImgException | LibvirtException ex) { + allGrandChildrenRebased = false; + logger.error("Failed to rebase grandchild [{}] into [{}]; [{}] will be kept as the grandchild still depends on it.", grandChildPath, + diskMerge.basePath, diskMerge.topPath, ex); + } + } - logger.debug("Found label [{}] for [{}]. Will merge delta at [{}] into delta at [{}].", label, volumeObjectTO, parentSnapshotLocalPath, childDeltaPath); + if (!allGrandChildrenRebased) { + return; + } + try { + logger.debug("Deleting [{}] as it was merged into [{}].", diskMerge.topPath, diskMerge.basePath); + Files.deleteIfExists(Path.of(diskMerge.topPath)); + } catch (IOException ex) { + logger.warn("Failed to delete [{}], which is no longer used by volume [{}].", diskMerge.topPath, diskMerge.volumeUuid, ex); + } + } + + protected boolean isBlockJobRunning(Domain domain, String label) throws LibvirtException { + DomainBlockJobInfo blockJobInfo = domain.getBlockJobInfo(label, 0); + return blockJobInfo != null && !(blockJobInfo.type == 0 && blockJobInfo.end == 0 && blockJobInfo.cur == 0); + } + + /** + * Reads, from the domain's live XML, the active image, the backing files and the mirror job (if any) of every disk, indexed by the disk's label. + */ + protected Map getDiskChains(Domain domain) throws LibvirtException { + String xml = domain.getXMLDesc(0); + Map diskChains = new HashMap<>(); + try { + Document document = ParserUtils.getSaferDocumentBuilderFactory().newDocumentBuilder().parse(new InputSource(new StringReader(xml))); + NodeList diskNodes = document.getElementsByTagName("disk"); + for (int i = 0; i < diskNodes.getLength(); i++) { + Element disk = (Element) diskNodes.item(i); + Element target = getChildElement(disk, "target"); + if (target == null) { + continue; + } - resource.mergeSnapshotIntoBaseFile(domain, label, parentSnapshotLocalPath, childDeltaPath, active, childTO.getPath(), - volumeObjectTO, conn); + List backingFiles = new ArrayList<>(); + Element backingStore = getChildElement(disk, "backingStore"); + while (backingStore != null) { + String backingFile = getSourcePath(getChildElement(backingStore, "source")); + if (backingFile == null) { + break; + } + backingFiles.add(backingFile); + backingStore = getChildElement(backingStore, "backingStore"); + } - QemuImgFile parent = new QemuImgFile(parentSnapshotLocalPath, QemuImg.PhysicalDiskFormat.QCOW2); + Element mirror = getChildElement(disk, "mirror"); + diskChains.put(target.getAttribute("dev"), new DiskChain(getSourcePath(getChildElement(disk, "source")), backingFiles, + mirror == null ? null : mirror.getAttribute("job"), mirror != null && "yes".equals(mirror.getAttribute("ready")))); + } + } catch (Exception ex) { + throw new CloudRuntimeException(String.format("Failed to parse the XML of VM [%s] due to [%s].", domain.getName(), ex.getMessage()), ex); + } + return diskChains; + } - List grandChildren = mergeTreeTO.getGrandChildren().stream() - .map(snapshotTo -> new QemuImgFile(storagePool.getLocalPathFor(snapshotTo.getPath()), QemuImg.PhysicalDiskFormat.QCOW2)) - .collect(Collectors.toList()); + private Element getChildElement(Element parent, String name) { + NodeList children = parent.getChildNodes(); + for (int i = 0; i < children.getLength(); i++) { + Node child = children.item(i); + if (child.getNodeType() == Node.ELEMENT_NODE && name.equals(child.getNodeName())) { + return (Element) child; + } + } + return null; + } - logger.debug("Rebasing grandChildren [{}] into parent at [{}].", grandChildren, parentSnapshotLocalPath); - for (QemuImgFile grandChild : grandChildren) { - qemuImg.rebase(grandChild, parent, parent.getFormat().toString(), false); + private String getSourcePath(Element source) { + if (source == null) { + return null; + } + for (String attribute : List.of("file", "dev", "name")) { + if (StringUtils.isNotBlank(source.getAttribute(attribute))) { + return source.getAttribute(attribute); } } + return null; + } + + private static boolean samePath(String path, String otherPath) { + return path != null && otherPath != null && Path.of(path).normalize().equals(Path.of(otherPath).normalize()); + } + + private String getVolumeUuid(SnapshotMergeTreeTO snapshotMergeTreeTO) { + return ((SnapshotObjectTO) snapshotMergeTreeTO.getParent()).getVolume().getUuid(); + } + + protected QemuImg getQemuImg(long timeoutInMillis) throws LibvirtException, QemuImgException { + return new QemuImg(timeoutInMillis); + } + + protected long getPollIntervalInMillis() { + return POLL_INTERVAL_IN_MILLIS; + } + + protected static class RunningVmDiskMerge { + protected final String volumeUuid; + protected final String label; + protected final String basePath; + protected final String topPath; + protected final boolean active; + protected final List grandChildPaths; + protected boolean observedApplied; + + protected RunningVmDiskMerge(String volumeUuid, String label, String basePath, String topPath, boolean active, List grandChildPaths) { + this.volumeUuid = volumeUuid; + this.label = label; + this.basePath = basePath; + this.topPath = topPath; + this.active = active; + this.grandChildPaths = grandChildPaths; + } + } + + protected static class StoppedVmDiskMerge { + protected final String volumeUuid; + protected final String parentPath; + protected final String childPath; + protected final List grandChildPaths; + + protected StoppedVmDiskMerge(String volumeUuid, String parentPath, String childPath, List grandChildPaths) { + this.volumeUuid = volumeUuid; + this.parentPath = parentPath; + this.childPath = childPath; + this.grandChildPaths = grandChildPaths; + } + } + + protected static class DiskChain { + protected final String activeSource; + protected final List backingFiles; + protected final String mirrorJob; + protected final boolean mirrorReady; + + protected DiskChain(String activeSource, List backingFiles, String mirrorJob, boolean mirrorReady) { + this.activeSource = activeSource; + this.backingFiles = backingFiles; + this.mirrorJob = StringUtils.isBlank(mirrorJob) ? null : mirrorJob; + this.mirrorReady = mirrorReady; + } + + protected boolean isActiveCommitReady() { + return Objects.equals(mirrorJob, "active-commit") && mirrorReady; + } + + protected boolean contains(String path) { + return samePath(activeSource, path) || backingFiles.stream().anyMatch(backingFile -> samePath(backingFile, path)); + } - return new Answer(cmd, true, null); + @Override + public String toString() { + List chain = new ArrayList<>(); + chain.add(activeSource); + chain.addAll(backingFiles); + return StringUtils.join(chain, " -> ") + (mirrorJob == null ? "" : String.format(" (job [%s], ready [%s])", mirrorJob, mirrorReady)); + } } } diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMergeDiskOnlyVMSnapshotCommandWrapperTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMergeDiskOnlyVMSnapshotCommandWrapperTest.java new file mode 100644 index 000000000000..95e8a506229f --- /dev/null +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMergeDiskOnlyVMSnapshotCommandWrapperTest.java @@ -0,0 +1,465 @@ +/* + * 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 static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +import org.apache.cloudstack.storage.to.PrimaryDataStoreTO; +import org.apache.cloudstack.storage.to.SnapshotObjectTO; +import org.apache.cloudstack.storage.to.VolumeObjectTO; +import org.apache.cloudstack.utils.qemu.QemuImg; +import org.apache.cloudstack.utils.qemu.QemuImgException; +import org.apache.cloudstack.utils.qemu.QemuImgFile; +import org.junit.Before; +import org.junit.Test; +import org.libvirt.Connect; +import org.libvirt.Domain; +import org.libvirt.DomainBlockJobInfo; +import org.libvirt.LibvirtException; + +import com.cloud.agent.api.Answer; +import com.cloud.agent.api.storage.MergeDiskOnlyVmSnapshotAnswer; +import com.cloud.agent.api.storage.MergeDiskOnlyVmSnapshotCommand; +import com.cloud.agent.api.storage.SnapshotMergeTreeTO; +import com.cloud.agent.api.to.DataObjectType; +import com.cloud.agent.api.to.DataTO; +import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; +import com.cloud.hypervisor.kvm.resource.LibvirtVMDef; +import com.cloud.hypervisor.kvm.storage.KVMStoragePool; +import com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager; +import com.cloud.storage.Storage; +import com.cloud.vm.VirtualMachine; + +public class LibvirtMergeDiskOnlyVMSnapshotCommandWrapperTest { + + private static final String VM_NAME = "i-2-10-VM"; + + private QemuImg qemuImg; + private LibvirtMergeDiskOnlyVMSnapshotCommandWrapper wrapper; + private LibvirtComputingResource resource; + private LibvirtUtilitiesHelper libvirtUtilitiesHelper; + private KVMStoragePoolManager storagePoolManager; + private KVMStoragePool storagePool; + private Domain domain; + private FakeBlockJobs fakeBlockJobs; + private Path poolDirectory; + + @Before + public void setup() throws Exception { + qemuImg = mock(QemuImg.class); + wrapper = new LibvirtMergeDiskOnlyVMSnapshotCommandWrapper() { + @Override + protected QemuImg getQemuImg(long timeoutInMillis) { + return qemuImg; + } + + @Override + protected long getPollIntervalInMillis() { + return 10; + } + }; + + resource = mock(LibvirtComputingResource.class); + libvirtUtilitiesHelper = mock(LibvirtUtilitiesHelper.class); + storagePoolManager = mock(KVMStoragePoolManager.class); + storagePool = mock(KVMStoragePool.class); + domain = mock(Domain.class); + Connect connect = mock(Connect.class); + poolDirectory = Files.createTempDirectory("merge-pool-"); + fakeBlockJobs = new FakeBlockJobs(poolDirectory); + + when(resource.getLibvirtUtilitiesHelper()).thenReturn(libvirtUtilitiesHelper); + when(libvirtUtilitiesHelper.getConnection()).thenReturn(connect); + when(resource.getDomain(connect, VM_NAME)).thenReturn(domain); + when(resource.getDisks(connect, VM_NAME)).thenReturn(new ArrayList<>()); + when(resource.getStoragePoolMgr()).thenReturn(storagePoolManager); + when(resource.getQcow2DeltaMergeTimeout()).thenReturn(259200); + when(resource.getCmdsTimeout()).thenReturn(7200000); + when(libvirtUtilitiesHelper.getPrimaryPoolFromDataTo(any(), any())).thenReturn(storagePool); + when(storagePoolManager.getStoragePool(any(), anyString())).thenReturn(storagePool); + when(storagePool.getLocalPathFor(anyString())).thenAnswer(invocation -> poolDirectory.resolve(invocation.getArgument(0, String.class)).toString()); + when(domain.getName()).thenReturn(VM_NAME); + fakeBlockJobs.attachTo(domain); + } + + @Test + public void testGetMergeTimeoutInSecondsLeavesAMarginBeforeTheCommandWait() { + assertEquals(259200, wrapper.getMergeTimeoutInSeconds(commandWithWait(0), 259200)); + assertEquals(15, wrapper.getMergeTimeoutInSeconds(commandWithWait(20), 259200)); + assertEquals(270, wrapper.getMergeTimeoutInSeconds(commandWithWait(300), 259200)); + assertEquals(1740, wrapper.getMergeTimeoutInSeconds(commandWithWait(1800), 259200)); + assertEquals(1, wrapper.getMergeTimeoutInSeconds(commandWithWait(3), 259200)); + assertEquals(100, wrapper.getMergeTimeoutInSeconds(commandWithWait(1800), 100)); + } + + @Test + public void testGetDiskChainsReadsActiveImageBackingChainAndMirror() throws Exception { + fakeBlockJobs.addDisk("vda", "top-a", "snap-a", "base-a"); + fakeBlockJobs.disk("vda").job = "active-commit"; + fakeBlockJobs.disk("vda").ready = true; + + LibvirtMergeDiskOnlyVMSnapshotCommandWrapper.DiskChain diskChain = wrapper.getDiskChains(domain).get("vda"); + + assertNotNull(diskChain); + assertEquals(fakeBlockJobs.path("top-a"), diskChain.activeSource); + assertEquals(List.of(fakeBlockJobs.path("snap-a"), fakeBlockJobs.path("base-a")), diskChain.backingFiles); + assertTrue(diskChain.isActiveCommitReady()); + assertTrue(diskChain.contains(fakeBlockJobs.path("snap-a"))); + assertFalse(diskChain.contains(fakeBlockJobs.path("other"))); + } + + @Test + public void testRunningVmMergePivotsDisksOnlyOnceEveryDiskIsReady() throws Exception { + fakeBlockJobs.addDisk("vda", "top-a", "snap-a", "base-a"); + fakeBlockJobs.addDisk("vdb", "top-b", "snap-b", "base-b"); + fakeBlockJobs.disk("vdb").pollsUntilFinished = 3; + MergeDiskOnlyVmSnapshotCommand command = runningCommand(1800, activeTree("vol-a", "vda", "top-a", "snap-a"), activeTree("vol-b", "vdb", "top-b", "snap-b")); + + Answer answer = wrapper.execute(command, resource); + + assertTrue(answer.getDetails(), answer.getResult()); + assertEquals(List.of("vol-a", "vol-b"), ((MergeDiskOnlyVmSnapshotAnswer) answer).getMergedVolumeUuids()); + assertFalse("A disk was pivoted while another one was not ready yet.", fakeBlockJobs.pivotedBeforeEveryDiskWasReady); + assertEquals(fakeBlockJobs.path("snap-a"), fakeBlockJobs.disk("vda").chain.get(0)); + assertEquals(fakeBlockJobs.path("snap-b"), fakeBlockJobs.disk("vdb").chain.get(0)); + assertFalse(Files.exists(Path.of(fakeBlockJobs.path("top-a")))); + assertFalse(Files.exists(Path.of(fakeBlockJobs.path("top-b")))); + verify(domain, never()).blockJobAbort(anyString(), eq(0)); + } + + @Test + public void testRunningVmMergeCancelsEveryDiskWhenOneIsNotReadyInTime() throws Exception { + fakeBlockJobs.addDisk("vda", "top-a", "snap-a", "base-a"); + fakeBlockJobs.addDisk("vdb", "top-b", "snap-b", "base-b"); + fakeBlockJobs.disk("vdb").pollsUntilFinished = Integer.MAX_VALUE; + MergeDiskOnlyVmSnapshotCommand command = runningCommand(6, activeTree("vol-a", "vda", "top-a", "snap-a"), activeTree("vol-b", "vdb", "top-b", "snap-b")); + + Answer answer = wrapper.execute(command, resource); + + assertFalse(answer.getResult()); + assertTrue(((MergeDiskOnlyVmSnapshotAnswer) answer).getMergedVolumeUuids().isEmpty()); + verify(domain, never()).blockJobAbort(anyString(), eq(Domain.BlockJobAbortFlags.PIVOT)); + verify(domain).blockJobAbort("vda", 0); + verify(domain).blockJobAbort("vdb", 0); + assertEquals(List.of(fakeBlockJobs.path("top-a"), fakeBlockJobs.path("snap-a"), fakeBlockJobs.path("base-a")), fakeBlockJobs.disk("vda").chain); + assertEquals(List.of(fakeBlockJobs.path("top-b"), fakeBlockJobs.path("snap-b"), fakeBlockJobs.path("base-b")), fakeBlockJobs.disk("vdb").chain); + assertTrue(Files.exists(Path.of(fakeBlockJobs.path("top-a")))); + assertTrue(Files.exists(Path.of(fakeBlockJobs.path("top-b")))); + } + + @Test + public void testRunningVmMergeReportsPivotedDisksWhenALaterPivotFails() throws Exception { + fakeBlockJobs.addDisk("vda", "top-a", "snap-a", "base-a"); + fakeBlockJobs.addDisk("vdb", "top-b", "snap-b", "base-b"); + fakeBlockJobs.disk("vdb").pivotFails = true; + MergeDiskOnlyVmSnapshotCommand command = runningCommand(1800, activeTree("vol-a", "vda", "top-a", "snap-a"), activeTree("vol-b", "vdb", "top-b", "snap-b")); + + Answer answer = wrapper.execute(command, resource); + + assertFalse(answer.getResult()); + assertEquals(List.of("vol-a"), ((MergeDiskOnlyVmSnapshotAnswer) answer).getMergedVolumeUuids()); + verify(domain).blockJobAbort("vdb", 0); + assertFalse(Files.exists(Path.of(fakeBlockJobs.path("top-a")))); + assertTrue(Files.exists(Path.of(fakeBlockJobs.path("top-b")))); + assertEquals(fakeBlockJobs.path("top-b"), fakeBlockJobs.disk("vdb").chain.get(0)); + } + + @Test + public void testRunningVmMergeReportsOnlyTheNonActiveCommitsThatCompleted() throws Exception { + fakeBlockJobs.addDisk("vda", "top-a", "child-a", "parent-a", "base-a"); + fakeBlockJobs.addDisk("vdb", "top-b", "child-b", "parent-b", "base-b"); + fakeBlockJobs.disk("vdb").commitFails = true; + Files.createFile(Path.of(fakeBlockJobs.path("grandchild-a"))); + MergeDiskOnlyVmSnapshotCommand command = runningCommand(1800, nonActiveTree("vol-a", "vda", "child-a", "parent-a", "grandchild-a"), + nonActiveTree("vol-b", "vdb", "child-b", "parent-b")); + + Answer answer = wrapper.execute(command, resource); + + assertFalse(answer.getResult()); + assertEquals(List.of("vol-a"), ((MergeDiskOnlyVmSnapshotAnswer) answer).getMergedVolumeUuids()); + verify(domain, never()).blockJobAbort(anyString(), eq(Domain.BlockJobAbortFlags.PIVOT)); + verify(qemuImg).rebase(argThat(file -> file.getFileName().equals(fakeBlockJobs.path("grandchild-a"))), + argThat(file -> file.getFileName().equals(fakeBlockJobs.path("parent-a"))), anyString(), eq(false)); + assertFalse(Files.exists(Path.of(fakeBlockJobs.path("child-a")))); + assertTrue(Files.exists(Path.of(fakeBlockJobs.path("child-b")))); + assertEquals(List.of(fakeBlockJobs.path("top-b"), fakeBlockJobs.path("child-b"), fakeBlockJobs.path("parent-b"), fakeBlockJobs.path("base-b")), + fakeBlockJobs.disk("vdb").chain); + } + + @Test + public void testStoppedVmMergeDoesNotChangeAnyChainWhenACommitFails() throws Exception { + Files.createFile(poolDirectory.resolve("child-a")); + Files.createFile(poolDirectory.resolve("child-b")); + doThrow(new QemuImgException("commit failed")).when(qemuImg).commit(argThat(file -> file.getFileName().endsWith("child-b")), any(), eq(true)); + MergeDiskOnlyVmSnapshotCommand command = stoppedCommand(stoppedTree("vol-a", "child-a", "parent-a", "grandchild-a"), + stoppedTree("vol-b", "child-b", "parent-b")); + + Answer answer = wrapper.execute(command, resource); + + assertFalse(answer.getResult()); + assertTrue(((MergeDiskOnlyVmSnapshotAnswer) answer).getMergedVolumeUuids().isEmpty()); + verify(qemuImg, never()).rebase(any(QemuImgFile.class), any(QemuImgFile.class), anyString(), eq(false)); + assertTrue(Files.exists(poolDirectory.resolve("child-a"))); + assertTrue(Files.exists(poolDirectory.resolve("child-b"))); + } + + @Test + public void testStoppedVmMergeChangesTheChainsOnlyAfterEveryCommitSucceeded() throws Exception { + Files.createFile(poolDirectory.resolve("child-a")); + Files.createFile(poolDirectory.resolve("child-b")); + MergeDiskOnlyVmSnapshotCommand command = stoppedCommand(stoppedTree("vol-a", "child-a", "parent-a", "grandchild-a"), + stoppedTree("vol-b", "child-b", "parent-b")); + + Answer answer = wrapper.execute(command, resource); + + assertTrue(answer.getDetails(), answer.getResult()); + assertEquals(List.of("vol-a", "vol-b"), ((MergeDiskOnlyVmSnapshotAnswer) answer).getMergedVolumeUuids()); + verify(qemuImg).rebase(argThat(file -> file.getFileName().endsWith("grandchild-a")), argThat(file -> file.getFileName().endsWith("parent-a")), + anyString(), eq(false)); + assertFalse(Files.exists(poolDirectory.resolve("child-a"))); + assertFalse(Files.exists(poolDirectory.resolve("child-b"))); + } + + @Test + public void testStoppedVmMergeAbortsWithoutCommittingOnceTheDeadlineIsReached() throws Exception { + MergeDiskOnlyVmSnapshotCommand command = stoppedCommand(stoppedTree("vol-a", "child-a", "parent-a"), stoppedTree("vol-b", "child-b", "parent-b")); + command.setWait(6); + doAnswer(invocation -> { + Thread.sleep(1100); + return null; + }).when(qemuImg).commit(argThat(file -> file.getFileName().endsWith("child-a")), any(), eq(true)); + + Answer answer = wrapper.execute(command, resource); + + assertFalse(answer.getResult()); + assertTrue(((MergeDiskOnlyVmSnapshotAnswer) answer).getMergedVolumeUuids().isEmpty()); + verify(qemuImg, never()).commit(argThat(file -> file.getFileName().endsWith("child-b")), any(), eq(true)); + } + + private MergeDiskOnlyVmSnapshotCommand commandWithWait(int wait) { + MergeDiskOnlyVmSnapshotCommand command = new MergeDiskOnlyVmSnapshotCommand(new ArrayList<>(), VirtualMachine.State.Running, VM_NAME); + command.setWait(wait); + return command; + } + + private MergeDiskOnlyVmSnapshotCommand runningCommand(int wait, SnapshotMergeTreeTO... trees) { + MergeDiskOnlyVmSnapshotCommand command = new MergeDiskOnlyVmSnapshotCommand(List.of(trees), VirtualMachine.State.Running, VM_NAME); + command.setWait(wait); + return command; + } + + private MergeDiskOnlyVmSnapshotCommand stoppedCommand(SnapshotMergeTreeTO... trees) { + MergeDiskOnlyVmSnapshotCommand command = new MergeDiskOnlyVmSnapshotCommand(List.of(trees), VirtualMachine.State.Stopped, VM_NAME); + command.setWait(1800); + return command; + } + + private SnapshotMergeTreeTO activeTree(String volumeUuid, String label, String topPath, String basePath) { + VolumeObjectTO volume = volume(volumeUuid, label, topPath); + when(volume.getObjectType()).thenReturn(DataObjectType.VOLUME); + return new SnapshotMergeTreeTO(snapshot(basePath, volume), volume, new ArrayList<>()); + } + + private SnapshotMergeTreeTO nonActiveTree(String volumeUuid, String label, String topPath, String basePath, String... grandChildren) { + VolumeObjectTO volume = volume(volumeUuid, label, "top-" + label); + SnapshotObjectTO child = snapshot(topPath, volume); + when(child.getObjectType()).thenReturn(DataObjectType.SNAPSHOT); + return new SnapshotMergeTreeTO(snapshot(basePath, volume), child, dataTos(volume, grandChildren)); + } + + private SnapshotMergeTreeTO stoppedTree(String volumeUuid, String childPath, String parentPath, String... grandChildren) { + VolumeObjectTO volume = volume(volumeUuid, null, childPath); + SnapshotObjectTO parent = snapshot(parentPath, volume); + PrimaryDataStoreTO dataStore = mock(PrimaryDataStoreTO.class); + when(dataStore.getPoolType()).thenReturn(Storage.StoragePoolType.NetworkFilesystem); + when(dataStore.getUuid()).thenReturn("pool"); + when(parent.getDataStore()).thenReturn(dataStore); + return new SnapshotMergeTreeTO(parent, snapshot(childPath, volume), dataTos(volume, grandChildren)); + } + + private List dataTos(VolumeObjectTO volume, String... paths) { + List dataTos = new ArrayList<>(); + for (String path : paths) { + dataTos.add(snapshot(path, volume)); + } + return dataTos; + } + + private VolumeObjectTO volume(String uuid, String label, String path) { + VolumeObjectTO volume = mock(VolumeObjectTO.class); + when(volume.getUuid()).thenReturn(uuid); + when(volume.getPath()).thenReturn(path); + if (label != null) { + LibvirtVMDef.DiskDef diskDef = mock(LibvirtVMDef.DiskDef.class); + when(diskDef.getDiskLabel()).thenReturn(label); + when(resource.getDiskWithPathOfVolumeObjectTO(any(), eq(volume))).thenReturn(diskDef); + } + return volume; + } + + private SnapshotObjectTO snapshot(String path, VolumeObjectTO volume) { + SnapshotObjectTO snapshot = mock(SnapshotObjectTO.class); + when(snapshot.getPath()).thenReturn(path); + when(snapshot.getVolume()).thenReturn(volume); + return snapshot; + } + + /** + * Imitates how Libvirt handles block commits: active commits stay ready until pivoted or cancelled, non-active commits finish on their own, and + * cancelling any of them leaves the backing chain unchanged. + */ + private static class FakeBlockJobs { + private final Path poolDirectory; + private final Map disks = new LinkedHashMap<>(); + private boolean pivotedBeforeEveryDiskWasReady; + + private FakeBlockJobs(Path poolDirectory) { + this.poolDirectory = poolDirectory; + } + + private String path(String name) { + return poolDirectory.resolve(name).toString(); + } + + private void addDisk(String label, String... chain) throws Exception { + FakeDisk disk = new FakeDisk(); + for (String name : chain) { + disk.chain.add(path(name)); + Files.createFile(Path.of(path(name))); + } + disks.put(label, disk); + } + + private FakeDisk disk(String label) { + return disks.get(label); + } + + private void attachTo(Domain domain) throws LibvirtException { + when(domain.getXMLDesc(0)).thenAnswer(invocation -> { + disks.values().forEach(FakeDisk::advance); + return xml(); + }); + when(domain.getBlockJobInfo(anyString(), eq(0))).thenAnswer(invocation -> { + DomainBlockJobInfo blockJobInfo = new DomainBlockJobInfo(); + FakeDisk disk = disks.get(invocation.getArgument(0, String.class)); + if (disk != null && disk.job != null) { + blockJobInfo.type = 3; + blockJobInfo.end = 100; + blockJobInfo.cur = disk.ready ? 100 : 50; + } + return blockJobInfo; + }); + doAnswer(invocation -> { + FakeDisk disk = disks.get(invocation.getArgument(0, String.class)); + disk.base = invocation.getArgument(1, String.class); + disk.top = invocation.getArgument(2, String.class); + disk.job = (invocation.getArgument(4, Integer.class) & Domain.BlockCommitFlags.ACTIVE) != 0 ? "active-commit" : "commit"; + return null; + }).when(domain).blockCommit(anyString(), anyString(), anyString(), anyLong(), anyInt()); + doAnswer(invocation -> { + FakeDisk disk = disks.get(invocation.getArgument(0, String.class)); + int flags = invocation.getArgument(1, Integer.class); + if (flags == Domain.BlockJobAbortFlags.PIVOT) { + if (disk.pivotFails || !disk.ready) { + throw mock(LibvirtException.class); + } + pivotedBeforeEveryDiskWasReady |= disks.values().stream().anyMatch(other -> other.job != null && !other.ready); + disk.chain.subList(0, disk.chain.indexOf(disk.base)).clear(); + } + disk.job = null; + disk.ready = false; + return null; + }).when(domain).blockJobAbort(anyString(), anyInt()); + } + + private String xml() { + StringBuilder xml = new StringBuilder("" + VM_NAME + ""); + for (Map.Entry entry : disks.entrySet()) { + FakeDisk disk = entry.getValue(); + xml.append(""); + for (int i = 1; i < disk.chain.size(); i++) { + xml.append(""); + } + xml.append(""); + for (int i = 1; i < disk.chain.size(); i++) { + xml.append(""); + } + if ("active-commit".equals(disk.job)) { + xml.append(""); + } + xml.append(""); + } + return xml.append("").toString(); + } + } + + private static class FakeDisk { + private final List chain = new ArrayList<>(); + private String job; + private boolean ready; + private String base; + private String top; + private int pollsUntilFinished; + private boolean pivotFails; + private boolean commitFails; + + private void advance() { + if (job == null || ready) { + return; + } + if (pollsUntilFinished > 0) { + if (pollsUntilFinished != Integer.MAX_VALUE) { + pollsUntilFinished--; + } + return; + } + if ("active-commit".equals(job)) { + ready = true; + return; + } + if (!commitFails) { + chain.remove(top); + } + job = null; + } + } +}