Skip to content

KVM local migration issue #3521 #3533

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
yadvr merged 2 commits into from
Aug 7, 2019
Merged

KVM local migration issue #3521 #3533

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
dd9d530
Fix regression bug that affects KVM local storage migration
Jul 25, 2019
ac2309c
Allow migrating Non Shared Disk for non managed storage.
Jul 30, 2019
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
9 changes: 9 additions & 0 deletions core/src/main/java/com/cloud/agent/api/MigrateCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ public class MigrateCommand extends Command {
private String destIp;
private Map<String, MigrateDiskInfo> migrateStorage;
private boolean migrateStorageManaged;
private boolean migrateNonSharedInc;
private boolean autoConvergence;
private String hostGuid;
private boolean isWindows;
Expand Down Expand Up @@ -75,6 +76,14 @@ public void setMigrateStorageManaged(boolean migrateStorageManaged) {
this.migrateStorageManaged = migrateStorageManaged;
}

public boolean isMigrateNonSharedInc() {
return migrateNonSharedInc;
}

public void setMigrateNonSharedInc(boolean migrateNonSharedInc) {
this.migrateNonSharedInc = migrateNonSharedInc;
}

public void setAutoConvergence(boolean autoConvergence) {
this.autoConvergence = autoConvergence;
}
Expand Down
40 changes: 31 additions & 9 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 @@ -1828,16 +1828,19 @@ public void copyAsync(Map<VolumeInfo, DataStore> volumeDataStoreMap, VirtualMach
String destPath = generateDestPath(destHost, destStoragePool, destVolumeInfo);

MigrateCommand.MigrateDiskInfo migrateDiskInfo;
if (managedStorageDestination) {
migrateDiskInfo = configureMigrateDiskInfo(srcVolumeInfo, destPath);
migrateDiskInfo.setSourceDiskOnStorageFileSystem(isStoragePoolTypeOfFile(sourceStoragePool));
migrateDiskInfoList.add(migrateDiskInfo);
} else {

boolean isNonManagedNfsToNfs = sourceStoragePool.getPoolType() == StoragePoolType.NetworkFilesystem
&& destStoragePool.getPoolType() == StoragePoolType.NetworkFilesystem && !managedStorageDestination;
if (isNonManagedNfsToNfs) {
migrateDiskInfo = new MigrateCommand.MigrateDiskInfo(srcVolumeInfo.getPath(),
MigrateCommand.MigrateDiskInfo.DiskType.FILE,
MigrateCommand.MigrateDiskInfo.DriverType.QCOW2,
MigrateCommand.MigrateDiskInfo.Source.FILE,
connectHostToVolume(destHost, destVolumeInfo.getPoolId(), volumeIdentifier));
} else {
migrateDiskInfo = configureMigrateDiskInfo(srcVolumeInfo, destPath);
migrateDiskInfo.setSourceDiskOnStorageFileSystem(isStoragePoolTypeOfFile(sourceStoragePool));
migrateDiskInfoList.add(migrateDiskInfo);
}

migrateStorage.put(srcVolumeInfo.getPath(), migrateDiskInfo);
Expand All @@ -1864,11 +1867,14 @@ public void copyAsync(Map<VolumeInfo, DataStore> volumeDataStoreMap, VirtualMach
VMInstanceVO vm = _vmDao.findById(vmTO.getId());
boolean isWindows = _guestOsCategoryDao.findById(_guestOsDao.findById(vm.getGuestOSId()).getCategoryId()).getName().equalsIgnoreCase("Windows");

boolean migrateNonSharedInc = isSourceAndDestinationPoolTypeOfNfs(volumeDataStoreMap);

MigrateCommand migrateCommand = new MigrateCommand(vmTO.getName(), destHost.getPrivateIpAddress(), isWindows, vmTO, true);
migrateCommand.setWait(StorageManager.KvmStorageOnlineMigrationWait.value());
migrateCommand.setMigrateStorage(migrateStorage);
migrateCommand.setMigrateDiskInfoList(migrateDiskInfoList);
migrateCommand.setMigrateStorageManaged(managedStorageDestination);
migrateCommand.setMigrateNonSharedInc(migrateNonSharedInc);

boolean kvmAutoConvergence = StorageManager.KvmAutoConvergence.value();
migrateCommand.setAutoConvergence(kvmAutoConvergence);
Expand Down Expand Up @@ -1905,6 +1911,23 @@ public void copyAsync(Map<VolumeInfo, DataStore> volumeDataStoreMap, VirtualMach
}
}

/**
* Returns true if at least one of the entries on the map 'volumeDataStoreMap' has both source and destination storage pools of Network Filesystem (NFS).
*/
protected boolean isSourceAndDestinationPoolTypeOfNfs(Map<VolumeInfo, DataStore> volumeDataStoreMap) {
for (Map.Entry<VolumeInfo, DataStore> entry : volumeDataStoreMap.entrySet()) {
VolumeInfo srcVolumeInfo = entry.getKey();
DataStore destDataStore = entry.getValue();

StoragePoolVO destStoragePool = _storagePoolDao.findById(destDataStore.getId());
StoragePoolVO sourceStoragePool = _storagePoolDao.findById(srcVolumeInfo.getPoolId());
if (sourceStoragePool.getPoolType() == StoragePoolType.NetworkFilesystem && destStoragePool.getPoolType() == StoragePoolType.NetworkFilesystem) {
Copy link
Copy Markdown
Member

@yadvr yadvr Aug 2, 2019

Choose a reason for hiding this comment

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

@GabrielBrascher shouldn't it loop and check for all volumes before returning true? It could be like it can return false if any of the src/dest pools are not nfs

Copy link
Copy Markdown
Member Author

@GabrielBrascher GabrielBrascher Aug 6, 2019

Choose a reason for hiding this comment

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

@rhtyd The idea is that if at least one of the disks is NFS it cannot be migrated on the execution flow of local storage. Taking a second look I see that the method name might need enhancements.

Am I missing something? Please let me know if something doesn't look right. Thanks for the review.

return true;
}
}
return false;
}

/**
* Returns true. This method was implemented considering the classes that extend this {@link StorageSystemDataMotionStrategy} and cannot migrate volumes from certain types of source storage pools and/or to a different kind of destiny storage pool.
*/
Expand Down Expand Up @@ -2157,7 +2180,7 @@ protected void postVolumeCreationActions(VolumeInfo srcVolumeInfo, VolumeInfo de
}
}

/*
/**
* At a high level: The source storage cannot be managed and
* the destination storages can be all managed or all not managed, not mixed.
*/
Expand Down Expand Up @@ -2191,9 +2214,8 @@ protected void verifyLiveMigrationForKVM(Map<VolumeInfo, DataStore> volumeDataSt
throw new CloudRuntimeException("Destination storage pools must be either all managed or all not managed");
}

if (!destStoragePoolVO.isManaged()) {
if (destStoragePoolVO.getPoolType() == StoragePoolType.NetworkFilesystem &&
destStoragePoolVO.getScope() != ScopeType.CLUSTER) {
if (!destStoragePoolVO.isManaged() && destStoragePoolVO.getPoolType() == StoragePoolType.NetworkFilesystem) {
if (destStoragePoolVO.getScope() != ScopeType.CLUSTER) {
throw new CloudRuntimeException("KVM live storage migrations currently support cluster-wide " +
"not managed NFS destination storage");
}
Expand Down
60 changes: 60 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 @@ -205,4 +205,64 @@ public void shouldMigrateVolumeTest() {
}
}

@Test
public void isSourceAndDestinationPoolTypeOfNfsTestNfsNfs() {
configureAndVerifyIsSourceAndDestinationPoolTypeOfNfs(StoragePoolType.NetworkFilesystem, StoragePoolType.NetworkFilesystem, true);
}

@Test
public void isSourceAndDestinationPoolTypeOfNfsTestNfsAny() {
StoragePoolType[] storagePoolTypeArray = StoragePoolType.values();
for (int i = 0; i < storagePoolTypeArray.length - 1; i++) {
if (storagePoolTypeArray[i] != StoragePoolType.NetworkFilesystem) {
configureAndVerifyIsSourceAndDestinationPoolTypeOfNfs(StoragePoolType.NetworkFilesystem, storagePoolTypeArray[i], false);
}
}
}

@Test
public void isSourceAndDestinationPoolTypeOfNfsTestAnyNfs() {
StoragePoolType[] storagePoolTypeArray = StoragePoolType.values();
for (int i = 0; i < storagePoolTypeArray.length - 1; i++) {
if (storagePoolTypeArray[i] != StoragePoolType.NetworkFilesystem) {
configureAndVerifyIsSourceAndDestinationPoolTypeOfNfs(storagePoolTypeArray[i], StoragePoolType.NetworkFilesystem, false);
}
}
}

@Test
public void isSourceAndDestinationPoolTypeOfNfsTestAnyAny() {
StoragePoolType[] storagePoolTypeArray = StoragePoolType.values();
for (int i = 0; i < storagePoolTypeArray.length - 1; i++) {
for (int j = 0; j < storagePoolTypeArray.length - 1; j++) {
if (storagePoolTypeArray[i] != StoragePoolType.NetworkFilesystem || storagePoolTypeArray[j] != StoragePoolType.NetworkFilesystem) {
configureAndVerifyIsSourceAndDestinationPoolTypeOfNfs(storagePoolTypeArray[i], storagePoolTypeArray[j], false);
}
}
}
}

private void configureAndVerifyIsSourceAndDestinationPoolTypeOfNfs(StoragePoolType destStoragePoolType, StoragePoolType sourceStoragePoolType, boolean expected) {
VolumeInfo srcVolumeInfo = Mockito.mock(VolumeObject.class);
Mockito.when(srcVolumeInfo.getId()).thenReturn(0l);

DataStore destDataStore = Mockito.mock(PrimaryDataStoreImpl.class);
Mockito.when(destDataStore.getId()).thenReturn(1l);

StoragePoolVO destStoragePool = Mockito.mock(StoragePoolVO.class);
Mockito.when(destStoragePool.getPoolType()).thenReturn(destStoragePoolType);

StoragePoolVO sourceStoragePool = Mockito.mock(StoragePoolVO.class);
Mockito.when(sourceStoragePool.getPoolType()).thenReturn(sourceStoragePoolType);

Map<VolumeInfo, DataStore> volumeDataStoreMap = new HashMap<>();
volumeDataStoreMap.put(srcVolumeInfo, destDataStore);

Mockito.doReturn(sourceStoragePool).when(primaryDataStoreDao).findById(0l);
Mockito.doReturn(destStoragePool).when(primaryDataStoreDao).findById(1l);

boolean result = strategy.isSourceAndDestinationPoolTypeOfNfs(volumeDataStoreMap);
Assert.assertEquals(expected, result);
}

}
12 changes: 6 additions & 6 deletions plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/MigrateKVMAsync.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public class MigrateKVMAsync implements Callable<Domain> {
private String vmName = "";
private String destIp = "";
private boolean migrateStorage;
private boolean migrateStorageManaged;
private boolean migrateNonSharedInc;
private boolean autoConvergence;

// Libvirt Migrate Flags reference:
Expand Down Expand Up @@ -87,14 +87,14 @@ public class MigrateKVMAsync implements Callable<Domain> {
private static final int LIBVIRT_VERSION_SUPPORTS_AUTO_CONVERGE = 1002003;

public MigrateKVMAsync(final LibvirtComputingResource libvirtComputingResource, final Domain dm, final Connect dconn, final String dxml,
final boolean migrateStorage, final boolean migrateStorageManaged, final boolean autoConvergence, final String vmName, final String destIp) {
final boolean migrateStorage, final boolean migrateNonSharedInc, final boolean autoConvergence, final String vmName, final String destIp) {
this.libvirtComputingResource = libvirtComputingResource;

this.dm = dm;
this.dconn = dconn;
this.dxml = dxml;
this.migrateStorage = migrateStorage;
this.migrateStorageManaged = migrateStorageManaged;
this.migrateNonSharedInc = migrateNonSharedInc;
this.autoConvergence = autoConvergence;
this.vmName = vmName;
this.destIp = destIp;
Expand All @@ -109,11 +109,11 @@ public Domain call() throws LibvirtException {
}

if (migrateStorage) {
if (migrateStorageManaged) {
flags |= VIR_MIGRATE_NON_SHARED_DISK;
} else {
if (migrateNonSharedInc) {
flags |= VIR_MIGRATE_PERSIST_DEST;
flags |= VIR_MIGRATE_NON_SHARED_INC;
} else {
flags |= VIR_MIGRATE_NON_SHARED_DISK;
}
}

Expand Down
4 changes: 3 additions & 1 deletion ...src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,10 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
//run migration in thread so we can monitor it
s_logger.info("Live migration of instance " + vmName + " initiated to destination host: " + dconn.getURI());
final ExecutorService executor = Executors.newFixedThreadPool(1);
boolean migrateNonSharedInc = command.isMigrateNonSharedInc() && !migrateStorageManaged;

final Callable<Domain> worker = new MigrateKVMAsync(libvirtComputingResource, dm, dconn, xmlDesc,
migrateStorage, migrateStorageManaged,
migrateStorage, migrateNonSharedInc,
command.isAutoConvergence(), vmName, command.getDestinationIp());
final Future<Domain> migrateThread = executor.submit(worker);
executor.shutdown();
Expand Down