Skip to content

Fix: Convert volume to another directory instead of copying it while taking volume snapshots on KVM #8041

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DaanHoogland merged 3 commits into from
Oct 6, 2023
Merged

Fix: Convert volume to another directory instead of copying it while taking volume snapshots on KVM #8041

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4dedf49
Convert volume to another directory instead of copying it while takin…
GutoVeronezi Oct 4, 2023
af83477
Fix unit tests
GutoVeronezi Oct 4, 2023
315568a
Rename variable
GutoVeronezi Oct 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 27 additions & 16 deletions ...s/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1715,11 +1715,11 @@ public Answer createSnapshot(final CreateObjectCommand cmd) {
snapshotPath = getSnapshotPathInPrimaryStorage(primaryPool.getLocalPath(), snapshotName);

String diskLabel = takeVolumeSnapshot(resource.getDisks(conn, vmName), snapshotName, diskPath, vm);
String copyResult = copySnapshotToPrimaryStorageDir(primaryPool, diskPath, snapshotPath, volume);
String convertResult = convertBaseFileToSnapshotFileInPrimaryStorageDir(primaryPool, diskPath, snapshotPath, volume, cmd.getWait());

mergeSnapshotIntoBaseFile(vm, diskLabel, diskPath, snapshotName, volume, conn);

validateCopyResult(copyResult, snapshotPath);
validateConvertResult(convertResult, snapshotPath);
} catch (LibvirtException e) {
if (!e.getMessage().contains(LIBVIRT_OPERATION_NOT_SUPPORTED_MESSAGE)) {
throw e;
Expand Down Expand Up @@ -1784,8 +1784,8 @@ public Answer createSnapshot(final CreateObjectCommand cmd) {
}
} else {
snapshotPath = getSnapshotPathInPrimaryStorage(primaryPool.getLocalPath(), snapshotName);
String copyResult = copySnapshotToPrimaryStorageDir(primaryPool, diskPath, snapshotPath, volume);
validateCopyResult(copyResult, snapshotPath);
String convertResult = convertBaseFileToSnapshotFileInPrimaryStorageDir(primaryPool, diskPath, snapshotPath, volume, cmd.getWait());
validateConvertResult(convertResult, snapshotPath);
}
}

Expand Down Expand Up @@ -1838,13 +1838,13 @@ protected void takeFullVmSnapshotForBinariesThatDoesNotSupportLiveDiskSnapshot(D
s_logger.debug(String.format("Full VM Snapshot [%s] of VM [%s] took [%s] seconds to finish.", snapshotName, vmName, (System.currentTimeMillis() - start)/1000));
}

protected void validateCopyResult(String copyResult, String snapshotPath) throws CloudRuntimeException, IOException {
if (copyResult == null) {
protected void validateConvertResult(String convertResult, String snapshotPath) throws CloudRuntimeException, IOException {
if (convertResult == null) {
return;
}

Files.deleteIfExists(Paths.get(snapshotPath));
throw new CloudRuntimeException(copyResult);
throw new CloudRuntimeException(convertResult);
}

/**
Expand Down Expand Up @@ -1901,20 +1901,31 @@ protected void manuallyDeleteUnusedSnapshotFile(boolean isLibvirtSupportingFlagD
}

/**
* Creates the snapshot directory in the primary storage, if it does not exist; then copies the base file (VM's old writing file) to the snapshot dir..
* Creates the snapshot directory in the primary storage, if it does not exist; then, converts the base file (VM's old writing file) to the snapshot directory.
* @param primaryPool Storage to create folder, if not exists;
* @param baseFile Base file of VM, which will be copied;
* @param snapshotPath Path to copy the base file;
* @return null if copies successfully or a error message.
* @param baseFile Base file of VM, which will be converted;
* @param snapshotPath Path to convert the base file;
* @return null if the conversion occurs successfully or an error message that must be handled.
*/
protected String copySnapshotToPrimaryStorageDir(KVMStoragePool primaryPool, String baseFile, String snapshotPath, VolumeObjectTO volume) {
protected String convertBaseFileToSnapshotFileInPrimaryStorageDir(KVMStoragePool primaryPool, String baseFile, String snapshotPath, VolumeObjectTO volume, int wait) {
try {
s_logger.debug(String.format("Trying to convert volume [%s] (%s) to snapshot [%s].", volume, baseFile, snapshotPath));

primaryPool.createFolder(TemplateConstants.DEFAULT_SNAPSHOT_ROOT_DIR);
Files.copy(Paths.get(baseFile), Paths.get(snapshotPath));
s_logger.debug(String.format("Copied %s snapshot from [%s] to [%s].", volume, baseFile, snapshotPath));

QemuImgFile srcFile = new QemuImgFile(baseFile);
srcFile.setFormat(PhysicalDiskFormat.QCOW2);

QemuImgFile destFile = new QemuImgFile(snapshotPath);
destFile.setFormat(PhysicalDiskFormat.QCOW2);

QemuImg q = new QemuImg(wait);
q.convert(srcFile, destFile);

s_logger.debug(String.format("Converted volume [%s] (from path \"%s\") to snapshot [%s].", volume, baseFile, snapshotPath));
return null;
} catch (IOException ex) {
return String.format("Unable to copy %s snapshot [%s] to [%s] due to [%s].", volume, baseFile, snapshotPath, ex.getMessage());
} catch (QemuImgException | LibvirtException ex) {
return String.format("Failed to convert %s snapshot of volume [%s] to [%s] due to [%s].", volume, baseFile, snapshotPath, ex.getMessage());
Copy link
Contributor

@DaanHoogland DaanHoogland Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't it be convenient to throw the CloudRuntimeException(convertResult) instead of in the validate method?

Copy link
Contributor Author

@GutoVeronezi GutoVeronezi Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaanHoogland I will check if we can refactor this part.

Copy link
Contributor Author

@GutoVeronezi GutoVeronezi Oct 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaanHoogland
Here we return a String instead of throwing an exception so we can handle the merge of the delta before throwing the exception. We could refactor KVMStorageProcessor#createSnapshot a little bit to work with a try-catch-finally. Though, I think we could do it afterward, so we focus on the issue fixing and reduce the scope of changes in this PR. What do you think?

}
}

Expand Down
51 changes: 39 additions & 12 deletions ...pervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@
import java.util.Set;
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.Assert;
import org.junit.Before;
import org.junit.Test;
Expand Down Expand Up @@ -91,6 +94,9 @@ public class KVMStorageProcessorTest {
@Mock
Connect connectMock;

@Mock
QemuImg qemuImgMock;

@Mock
LibvirtDomainXMLParser libvirtDomainXMLParserMock;
@Mock
Expand Down Expand Up @@ -251,32 +257,53 @@ public void validateTakeVolumeSnapshotSuccessReturnDiskLabel() throws LibvirtExc

@Test
@PrepareForTest(KVMStorageProcessor.class)
public void validateCopySnapshotToPrimaryStorageDirFailToCopyReturnErrorMessage() throws Exception {
public void convertBaseFileToSnapshotFileInPrimaryStorageDirTestFailToConvertWithQemuImgExceptionReturnErrorMessage() throws Exception {
String baseFile = "baseFile";
String snapshotPath = "snapshotPath";
String errorMessage = "error";
String expectedResult = String.format("Unable to copy %s snapshot [%s] to [%s] due to [%s].", volumeObjectToMock, baseFile, snapshotPath, errorMessage);
String expectedResult = String.format("Failed to convert %s snapshot of volume [%s] to [%s] due to [%s].", volumeObjectToMock, baseFile, snapshotPath, errorMessage);

Mockito.doReturn(true).when(kvmStoragePoolMock).createFolder(Mockito.anyString());
PowerMockito.mockStatic(Files.class);
PowerMockito.when(Files.copy(Mockito.any(Path.class), Mockito.any(Path.class), Mockito.any())).thenThrow(new IOException(errorMessage));

String result = storageProcessorSpy.copySnapshotToPrimaryStorageDir(kvmStoragePoolMock, baseFile, snapshotPath, volumeObjectToMock);
PowerMockito.whenNew(QemuImg.class).withArguments(Mockito.anyInt()).thenReturn(qemuImgMock);
Mockito.doThrow(new QemuImgException(errorMessage)).when(qemuImgMock).convert(Mockito.any(QemuImgFile.class), Mockito.any(QemuImgFile.class));

String result = storageProcessorSpy.convertBaseFileToSnapshotFileInPrimaryStorageDir(kvmStoragePoolMock, baseFile, snapshotPath, volumeObjectToMock, 1);

Assert.assertEquals(expectedResult, result);
}

@Test
@PrepareForTest(KVMStorageProcessor.class)
public void validateCopySnapshotToPrimaryStorageDirCopySuccessReturnNull() throws Exception {
public void convertBaseFileToSnapshotFileInPrimaryStorageDirTestFailToConvertWithLibvirtExceptionReturnErrorMessage() throws Exception {
String baseFile = "baseFile";
String snapshotPath = "snapshotPath";
String errorMessage = "null";
String expectedResult = String.format("Failed to convert %s snapshot of volume [%s] to [%s] due to [%s].", volumeObjectToMock, baseFile, snapshotPath, errorMessage);

Mockito.doReturn(true).when(kvmStoragePoolMock).createFolder(Mockito.anyString());
PowerMockito.mockStatic(Files.class);
PowerMockito.when(Files.copy(Mockito.any(Path.class), Mockito.any(Path.class), Mockito.any())).thenReturn(null);

String result = storageProcessorSpy.copySnapshotToPrimaryStorageDir(kvmStoragePoolMock, baseFile, snapshotPath, volumeObjectToMock);
PowerMockito.whenNew(QemuImg.class).withArguments(Mockito.anyInt()).thenReturn(qemuImgMock);
Mockito.doThrow(LibvirtException.class).when(qemuImgMock).convert(Mockito.any(QemuImgFile.class), Mockito.any(QemuImgFile.class));

String result = storageProcessorSpy.convertBaseFileToSnapshotFileInPrimaryStorageDir(kvmStoragePoolMock, baseFile, snapshotPath, volumeObjectToMock, 1);

Assert.assertEquals(expectedResult, result);
}


@Test
@PrepareForTest(KVMStorageProcessor.class)
public void convertBaseFileToSnapshotFileInPrimaryStorageDirTestConvertSuccessReturnNull() throws Exception {
String baseFile = "baseFile";
String snapshotPath = "snapshotPath";

Mockito.doReturn(true).when(kvmStoragePoolMock).createFolder(Mockito.anyString());

PowerMockito.whenNew(QemuImg.class).withArguments(Mockito.anyInt()).thenReturn(qemuImgMock);
Mockito.doNothing().when(qemuImgMock).convert(Mockito.any(QemuImgFile.class), Mockito.any(QemuImgFile.class));

String result = storageProcessorSpy.convertBaseFileToSnapshotFileInPrimaryStorageDir(kvmStoragePoolMock, baseFile, snapshotPath, volumeObjectToMock, 1);

Assert.assertNull(result);
}
Expand Down Expand Up @@ -321,22 +348,22 @@ public void validateIsAvailablePoolSizeDividedByDiskSizeLesserThanMinRate(){

@Test
public void validateValidateCopyResultResultIsNullReturn() throws CloudRuntimeException, IOException{
storageProcessorSpy.validateCopyResult(null, "");
storageProcessorSpy.validateConvertResult(null, "");
}

@Test (expected = IOException.class)
public void validateValidateCopyResultFailToDeleteThrowIOException() throws CloudRuntimeException, IOException{
PowerMockito.mockStatic(Files.class);
PowerMockito.when(Files.deleteIfExists(Mockito.any())).thenThrow(new IOException(""));
storageProcessorSpy.validateCopyResult("", "");
storageProcessorSpy.validateConvertResult("", "");
}

@Test (expected = CloudRuntimeException.class)
@PrepareForTest(KVMStorageProcessor.class)
public void validateValidateCopyResulResultNotNullThrowCloudRuntimeException() throws CloudRuntimeException, IOException{
PowerMockito.mockStatic(Files.class);
PowerMockito.when(Files.deleteIfExists(Mockito.any())).thenReturn(true);
storageProcessorSpy.validateCopyResult("", "");
storageProcessorSpy.validateConvertResult("", "");
}

@Test (expected = CloudRuntimeException.class)
Expand Down