Skip to content

Add SharedMountPoint to KVMs supported storage pool types #4780

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
nvazquez merged 2 commits into from
Aug 16, 2021
Merged

Add SharedMountPoint to KVMs supported storage pool types #4780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5bf1d73
Add SharedMountPoint to KVMs supported storage pool types
Mar 8, 2021
2f6b9cb
Fix live migration to iSCSI and improve logs
Jun 14, 2021
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
11 changes: 8 additions & 3 deletions ...ain/java/org/apache/cloudstack/storage/motion/KvmNonManagedStorageDataMotionStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ protected StrategyPriority internalCanHandle(Map<VolumeInfo, DataStore> volumeMa

for (VolumeInfo volumeInfo : volumeInfoSet) {
StoragePoolVO storagePoolVO = _storagePoolDao.findById(volumeInfo.getPoolId());
if (storagePoolVO.getPoolType() != StoragePoolType.Filesystem && storagePoolVO.getPoolType() != StoragePoolType.NetworkFilesystem) {

if (!supportStoragePoolType(storagePoolVO.getPoolType())) {
return StrategyPriority.CANT_HANDLE;
}
}
Expand Down Expand Up @@ -187,7 +188,7 @@ protected void setVolumePath(VolumeVO volume) {
*/
@Override
protected boolean shouldMigrateVolume(StoragePoolVO sourceStoragePool, Host destHost, StoragePoolVO destStoragePool) {
return sourceStoragePool.getPoolType() == StoragePoolType.Filesystem || sourceStoragePool.getPoolType() == StoragePoolType.NetworkFilesystem;
return supportStoragePoolType(sourceStoragePool.getPoolType());
}

/**
Expand All @@ -201,7 +202,7 @@ protected void copyTemplateToTargetFilesystemStorageIfNeeded(VolumeInfo srcVolum
}

VMTemplateStoragePoolVO sourceVolumeTemplateStoragePoolVO = vmTemplatePoolDao.findByPoolTemplate(destStoragePool.getId(), srcVolumeInfo.getTemplateId(), null);
if (sourceVolumeTemplateStoragePoolVO == null && destStoragePool.getPoolType() == StoragePoolType.Filesystem) {
if (sourceVolumeTemplateStoragePoolVO == null && (isStoragePoolTypeInList(destStoragePool.getPoolType(), StoragePoolType.Filesystem, StoragePoolType.SharedMountPoint))) {
DataStore sourceTemplateDataStore = dataStoreManagerImpl.getRandomImageStore(srcVolumeInfo.getDataCenterId());
if (sourceTemplateDataStore != null) {
TemplateInfo sourceTemplateInfo = templateDataFactory.getTemplate(srcVolumeInfo.getTemplateId(), sourceTemplateDataStore);
Expand Down Expand Up @@ -270,4 +271,8 @@ protected void logInCaseOfTemplateCopyFailure(Answer copyCommandAnswer, Template
LOGGER.error(generateFailToCopyTemplateMessage(sourceTemplate, destDataStore) + failureDetails);
}
}

protected Boolean supportStoragePoolType(StoragePoolType storagePoolType) {
return super.supportStoragePoolType(storagePoolType, StoragePoolType.Filesystem);
}
}
31 changes: 28 additions & 3 deletions ...n/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,10 @@
import com.cloud.vm.VirtualMachineManager;
import com.cloud.vm.dao.VMInstanceDao;
import com.google.common.base.Preconditions;
import java.util.Arrays;
import java.util.HashSet;
import java.util.stream.Collectors;
import org.apache.commons.collections.CollectionUtils;

public class StorageSystemDataMotionStrategy implements DataMotionStrategy {
private static final Logger LOGGER = Logger.getLogger(StorageSystemDataMotionStrategy.class);
Expand Down Expand Up @@ -1861,9 +1864,8 @@ public void copyAsync(Map<VolumeInfo, DataStore> volumeDataStoreMap, VirtualMach

MigrateCommand.MigrateDiskInfo migrateDiskInfo;

boolean isNonManagedNfsToNfs = sourceStoragePool.getPoolType() == StoragePoolType.NetworkFilesystem
&& destStoragePool.getPoolType() == StoragePoolType.NetworkFilesystem && !managedStorageDestination;
if (isNonManagedNfsToNfs) {
boolean isNonManagedNfsToNfsOrSharedMountPointToNfs = supportStoragePoolType(sourceStoragePool.getPoolType()) && destStoragePool.getPoolType() == StoragePoolType.NetworkFilesystem && !managedStorageDestination;
if (isNonManagedNfsToNfsOrSharedMountPointToNfs) {
migrateDiskInfo = new MigrateCommand.MigrateDiskInfo(srcVolumeInfo.getPath(),
MigrateCommand.MigrateDiskInfo.DiskType.FILE,
MigrateCommand.MigrateDiskInfo.DriverType.QCOW2,
Expand Down Expand Up @@ -2897,4 +2899,27 @@ private CopyCmdAnswer performCopyOfVdi(VolumeInfo volumeInfo, SnapshotInfo snaps

return copyCmdAnswer;
}

protected Boolean supportStoragePoolType(StoragePoolType storagePoolTypeToValidate, StoragePoolType... extraAcceptedValues) {
List<StoragePoolType> values = new ArrayList<>();

values.add(StoragePoolType.NetworkFilesystem);
values.add(StoragePoolType.SharedMountPoint);
Copy link
Contributor

@DaanHoogland DaanHoogland Mar 11, 2021

Choose a reason for hiding this comment

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

is this true for all hypervisors/storage backends? I know this is a new method only called from the KVMNonManaged, but that is not reflected in the name or protected by encapsulation. (genuine question)

Copy link
Contributor Author

@GutoVeronezi GutoVeronezi Mar 11, 2021

Choose a reason for hiding this comment

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

Actually KVM is the only hypervisor that enables SharedMountPoint as an option, so it is true to all hypervisors that has it enabled.


if (extraAcceptedValues != null) {
CollectionUtils.addAll(values, extraAcceptedValues);
}

return isStoragePoolTypeInList(storagePoolTypeToValidate, values.toArray(new StoragePoolType[values.size()]));
}

protected Boolean isStoragePoolTypeInList(StoragePoolType storagePoolTypeToValidate, StoragePoolType... acceptedValues){
Set<StoragePoolType> supportedTypes = new HashSet<>();

if (acceptedValues != null) {
supportedTypes.addAll(Arrays.asList(acceptedValues));
}

return supportedTypes.contains(storagePoolTypeToValidate);
};
}
36 changes: 34 additions & 2 deletions ...t/java/org/apache/cloudstack/storage/motion/KvmNonManagedStorageSystemDataMotionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@
import com.cloud.storage.dao.VMTemplatePoolDao;
import com.cloud.utils.exception.CloudRuntimeException;
import com.cloud.vm.VirtualMachineManager;
import java.util.HashSet;
import java.util.Set;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

@RunWith(MockitoJUnitRunner.class)
public class KvmNonManagedStorageSystemDataMotionTest {
Expand Down Expand Up @@ -159,13 +163,23 @@ private void canHandleExpectCannotHandle(HypervisorType hypervisorType, int time
Assert.assertEquals(expectedStrategyPriority, strategyPriority);
}

public Boolean supportStoragePoolType(StoragePoolType storagePoolType) {
Set<StoragePoolType> supportedTypes = new HashSet<>();
supportedTypes.add(StoragePoolType.Filesystem);
supportedTypes.add(StoragePoolType.NetworkFilesystem);
supportedTypes.add(StoragePoolType.SharedMountPoint);

return supportedTypes.contains(storagePoolType);
}

@Test
public void internalCanHandleTestNonManaged() {
StoragePoolType[] storagePoolTypeArray = StoragePoolType.values();
for (int i = 0; i < storagePoolTypeArray.length; i++) {
Map<VolumeInfo, DataStore> volumeMap = configureTestInternalCanHandle(false, storagePoolTypeArray[i]);
StrategyPriority strategyPriority = kvmNonManagedStorageDataMotionStrategy.internalCanHandle(volumeMap, new HostVO("sourceHostUuid"), new HostVO("destHostUuid"));
if (storagePoolTypeArray[i] == StoragePoolType.Filesystem || storagePoolTypeArray[i] == StoragePoolType.NetworkFilesystem) {

if (supportStoragePoolType(storagePoolTypeArray[i])) {
Assert.assertEquals(StrategyPriority.HYPERVISOR, strategyPriority);
} else {
Assert.assertEquals(StrategyPriority.CANT_HANDLE, strategyPriority);
Expand Down Expand Up @@ -243,7 +257,7 @@ public void shouldMigrateVolumeTest() {
for (int i = 0; i < storagePoolTypes.length; i++) {
Mockito.doReturn(storagePoolTypes[i]).when(sourceStoragePool).getPoolType();
boolean result = kvmNonManagedStorageDataMotionStrategy.shouldMigrateVolume(sourceStoragePool, destHost, destStoragePool);
if (storagePoolTypes[i] == StoragePoolType.Filesystem || storagePoolTypes[i] == StoragePoolType.NetworkFilesystem) {
if (supportStoragePoolType(storagePoolTypes[i])) {
Assert.assertTrue(result);
} else {
Assert.assertFalse(result);
Expand Down Expand Up @@ -472,4 +486,22 @@ public void testVerifyLiveMigrationMapForKVMMixedManagedUnmagedStorage() {
lenient().when(pool2.isManaged()).thenReturn(false);
kvmNonManagedStorageDataMotionStrategy.verifyLiveMigrationForKVM(migrationMap, host2);
}

@Test
public void validateSupportStoragePoolType() {
Set<StoragePoolType> supportedTypes = new HashSet<>();
supportedTypes.add(StoragePoolType.Filesystem);
supportedTypes.add(StoragePoolType.NetworkFilesystem);
supportedTypes.add(StoragePoolType.SharedMountPoint);

for (StoragePoolType poolType : StoragePoolType.values()) {
boolean isSupported = kvmNonManagedStorageDataMotionStrategy.supportStoragePoolType(poolType);
if (supportedTypes.contains(poolType)) {
assertTrue(isSupported);
} else {
assertFalse(isSupported);
}
}
}

}
57 changes: 57 additions & 0 deletions ...c/test/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.apache.cloudstack.storage.motion;

import static org.junit.Assert.assertTrue;
import static org.junit.Assert.assertFalse;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.lenient;
import static org.mockito.Mockito.mock;
Expand Down Expand Up @@ -56,6 +57,8 @@
import com.cloud.storage.Volume;
import com.cloud.storage.VolumeVO;
import java.util.AbstractMap;
import java.util.HashSet;
import java.util.Set;

@RunWith(MockitoJUnitRunner.class)
public class StorageSystemDataMotionStrategyTest {
Expand Down Expand Up @@ -288,4 +291,58 @@ public void formatEntryOfVolumesAndStoragesAsJsonToDisplayOnLogValidateFormat(){

Assert.assertEquals(String.format("{volume: \"%s\", from: \"%s\", to:\"%s\"}", volume, from, to), strategy.formatEntryOfVolumesAndStoragesAsJsonToDisplayOnLog(new AbstractMap.SimpleEntry<>(volumeInfo, dataStore)));
}

@Test
public void validateSupportStoragePoolTypeDefaultValues() {
Set<StoragePoolType> supportedTypes = new HashSet<>();
supportedTypes.add(StoragePoolType.NetworkFilesystem);
supportedTypes.add(StoragePoolType.SharedMountPoint);

for (StoragePoolType poolType : StoragePoolType.values()) {
boolean isSupported = strategy.supportStoragePoolType(poolType);
if (supportedTypes.contains(poolType)) {
assertTrue(isSupported);
} else {
assertFalse(isSupported);
}
}
}

@Test
public void validateSupportStoragePoolTypeExtraValues() {
Set<StoragePoolType> supportedTypes = new HashSet<>();
supportedTypes.add(StoragePoolType.NetworkFilesystem);
supportedTypes.add(StoragePoolType.SharedMountPoint);
supportedTypes.add(StoragePoolType.Iscsi);
supportedTypes.add(StoragePoolType.CLVM);

for (StoragePoolType poolType : StoragePoolType.values()) {
boolean isSupported = strategy.supportStoragePoolType(poolType, StoragePoolType.Iscsi, StoragePoolType.CLVM);
if (supportedTypes.contains(poolType)) {
assertTrue(isSupported);
} else {
assertFalse(isSupported);
}
}
}

@Test
public void validateIsStoragePoolTypeInListReturnsTrue() {
StoragePoolType[] listTypes = new StoragePoolType[3];
listTypes[0] = StoragePoolType.LVM;
listTypes[1] = StoragePoolType.NetworkFilesystem;
listTypes[2] = StoragePoolType.SharedMountPoint;

assertTrue(strategy.isStoragePoolTypeInList(StoragePoolType.SharedMountPoint, listTypes));
}

@Test
public void validateIsStoragePoolTypeInListReturnsFalse() {
StoragePoolType[] listTypes = new StoragePoolType[3];
listTypes[0] = StoragePoolType.LVM;
listTypes[1] = StoragePoolType.NetworkFilesystem;
listTypes[2] = StoragePoolType.RBD;

assertFalse(strategy.isStoragePoolTypeInList(StoragePoolType.SharedMountPoint, listTypes));
}
}
53 changes: 34 additions & 19 deletions ...hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@
import com.cloud.utils.script.Script;

import static com.cloud.utils.NumbersUtil.toHumanReadableSize;
import java.util.Arrays;
import java.util.HashSet;
import java.util.Set;
import java.util.stream.Collectors;

public class LibvirtStorageAdaptor implements StorageAdaptor {
private static final Logger s_logger = Logger.getLogger(LibvirtStorageAdaptor.class);
Expand All @@ -80,6 +84,9 @@ public class LibvirtStorageAdaptor implements StorageAdaptor {
private int rbdFeatures = RBD_FEATURE_LAYERING + RBD_FEATURE_EXCLUSIVE_LOCK + RBD_FEATURE_OBJECT_MAP + RBD_FEATURE_FAST_DIFF + RBD_FEATURE_DEEP_FLATTEN;
private int rbdOrder = 0; /* Order 0 means 4MB blocks (the default) */

private static final Set<StoragePoolType> poolTypesThatEnableCreateDiskFromTemplateBacking = new HashSet<>(Arrays.asList(StoragePoolType.NetworkFilesystem,
StoragePoolType.Filesystem));

public LibvirtStorageAdaptor(StorageLayer storage) {
_storageLayer = storage;
_manageSnapshotPath = Script.findScript("scripts/storage/qcow2/", "managesnapshot.sh");
Expand All @@ -98,28 +105,36 @@ public boolean createFolder(String uuid, String path) {
@Override
public KVMPhysicalDisk createDiskFromTemplateBacking(KVMPhysicalDisk template, String name, PhysicalDiskFormat format, long size,
KVMStoragePool destPool, int timeout) {
s_logger.info("Creating volume " + name + " with template backing " + template.getName() + " in pool " + destPool.getUuid() +
" (" + destPool.getType().toString() + ") with size " + size);
String volumeDesc = String.format("volume [%s], with template backing [%s], in pool [%s] (%s), with size [%s]", name, template.getName(), destPool.getUuid(),
destPool.getType(), size);

KVMPhysicalDisk disk = null;
String destPath = destPool.getLocalPath().endsWith("/") ?
destPool.getLocalPath() + name :
destPool.getLocalPath() + "/" + name;
if (!poolTypesThatEnableCreateDiskFromTemplateBacking.contains(destPool.getType())) {
s_logger.info(String.format("Skipping creation of %s due to pool type is none of the following types %s.", volumeDesc, poolTypesThatEnableCreateDiskFromTemplateBacking.stream()
.map(type -> type.toString()).collect(Collectors.joining(", "))));

if (destPool.getType() == StoragePoolType.NetworkFilesystem) {
try {
if (format == PhysicalDiskFormat.QCOW2) {
QemuImg qemu = new QemuImg(timeout);
QemuImgFile destFile = new QemuImgFile(destPath, format);
destFile.setSize(size);
QemuImgFile backingFile = new QemuImgFile(template.getPath(), template.getFormat());
qemu.create(destFile, backingFile);
}
} catch (QemuImgException e) {
s_logger.error("Failed to create " + destPath + " due to a failed executing of qemu-img: " + e.getMessage());
}
return null;
}
return disk;

if (format != PhysicalDiskFormat.QCOW2) {
s_logger.info(String.format("Skipping creation of %s due to format [%s] is not [%s].", volumeDesc, format, PhysicalDiskFormat.QCOW2));
return null;
}

s_logger.info(String.format("Creating %s.", volumeDesc));

String destPoolLocalPath = destPool.getLocalPath();
String destPath = String.format("%s%s%s", destPoolLocalPath, destPoolLocalPath.endsWith("/") ? "" : "/", name);

try {
QemuImgFile destFile = new QemuImgFile(destPath, format);
destFile.setSize(size);
QemuImgFile backingFile = new QemuImgFile(template.getPath(), template.getFormat());
new QemuImg(timeout).create(destFile, backingFile);
} catch (QemuImgException e) {
s_logger.error(String.format("Failed to create %s in [%s] due to [%s].", volumeDesc, destPath, e.getMessage()), e);
}

return null;
}

/**
Expand Down