From 50ca91a83e58da98054d64b66afab259f6c13485 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 21 Jun 2022 16:16:17 +0530 Subject: [PATCH 1/6] Fix VMware VM migration with volume in case of local storage --- .../com/cloud/hypervisor/guru/VMwareGuru.java | 30 ++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java index a5e4140357ba..548734a9924c 100644 --- a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java +++ b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java @@ -27,6 +27,8 @@ import javax.inject.Inject; +import com.cloud.storage.StoragePoolHostVO; +import com.cloud.storage.dao.StoragePoolHostDao; import org.apache.cloudstack.acl.ControlledEntity; import org.apache.cloudstack.backup.Backup; import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine; @@ -184,6 +186,7 @@ public class VMwareGuru extends HypervisorGuruBase implements HypervisorGuru, Co @Inject UserVmDao userVmDao; @Inject DiskOfferingDao diskOfferingDao; @Inject PhysicalNetworkDao physicalNetworkDao; + @Inject StoragePoolHostDao storagePoolHostDao; protected VMwareGuru() { super(); @@ -1070,6 +1073,21 @@ private boolean isInterClusterMigration(Long srcClusterId, Long destClusterId) { return srcClusterId != null && destClusterId != null && ! srcClusterId.equals(destClusterId); } + private String getHostGuidForLocalStorage(Map volumeToPool) { + String hostGuidInTargetCluster = null; + for (Map.Entry entry : volumeToPool.entrySet()) { + Volume volume = entry.getKey(); + StoragePool pool = entry.getValue(); + if (volume.getVolumeType().equals(Volume.Type.ROOT) && pool.isLocal()) { + List storagePoolHostVOs = storagePoolHostDao.listByPoolId(pool.getId()); + StoragePoolHostVO storagePoolHostVO = storagePoolHostVOs.get(0); + HostVO hostVO = _hostDao.findById(storagePoolHostVO.getHostId()); + hostGuidInTargetCluster = hostVO.getGuid(); + } + } + return hostGuidInTargetCluster; + } + private String getHostGuidInTargetCluster(boolean isInterClusterMigration, Long destClusterId) { String hostGuidInTargetCluster = null; if (isInterClusterMigration) { @@ -1096,6 +1114,7 @@ public List finalizeMigrate(VirtualMachine vm, Map // OfflineVmwareMigration: specialised migration command List> volumeToFilerTo = new ArrayList>(); Long poolClusterId = null; + boolean isLocalStorageMigration = false; for (Map.Entry entry : volumeToPool.entrySet()) { Volume volume = entry.getKey(); StoragePool pool = entry.getValue(); @@ -1104,13 +1123,22 @@ public List finalizeMigrate(VirtualMachine vm, Map if (pool.getClusterId() != null) { poolClusterId = pool.getClusterId(); } + if (volume.getVolumeType().equals(Volume.Type.ROOT) && pool.isLocal()) { + isLocalStorageMigration = true; + } volumeToFilerTo.add(new Pair(volumeTo, filerTo)); } final Long destClusterId = poolClusterId; final Long srcClusterId = vmManager.findClusterAndHostIdForVm(vm.getId()).first(); final boolean isInterClusterMigration = isInterClusterMigration(destClusterId, srcClusterId); + String targetHostGuid = null; + if (isLocalStorageMigration) { + targetHostGuid = getHostGuidForLocalStorage(volumeToPool); + } else { + targetHostGuid = getHostGuidInTargetCluster(isInterClusterMigration, destClusterId); + } MigrateVmToPoolCommand migrateVmToPoolCommand = new MigrateVmToPoolCommand(vm.getInstanceName(), - volumeToFilerTo, getHostGuidInTargetCluster(isInterClusterMigration, destClusterId), true); + volumeToFilerTo, targetHostGuid, true); commands.add(migrateVmToPoolCommand); // OfflineVmwareMigration: cleanup if needed From 8c53536369d431c6b7da927c2e8b49d6ec446ced Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Wed, 22 Jun 2022 14:42:33 +0530 Subject: [PATCH 2/6] Break the loop once target host is found --- .../src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java index 548734a9924c..b5134a848377 100644 --- a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java +++ b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java @@ -1083,6 +1083,7 @@ private String getHostGuidForLocalStorage(Map volumeToPool) StoragePoolHostVO storagePoolHostVO = storagePoolHostVOs.get(0); HostVO hostVO = _hostDao.findById(storagePoolHostVO.getHostId()); hostGuidInTargetCluster = hostVO.getGuid(); + break; } } return hostGuidInTargetCluster; From 9daca66a898c1b5233615213ab449371643562e4 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Mon, 27 Jun 2022 15:35:15 +0530 Subject: [PATCH 3/6] Code optimisations in getting the target host guid for local storage --- .../com/cloud/hypervisor/guru/VMwareGuru.java | 29 +++++++------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java index b5134a848377..4a22d2fbd18c 100644 --- a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java +++ b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java @@ -1073,20 +1073,11 @@ private boolean isInterClusterMigration(Long srcClusterId, Long destClusterId) { return srcClusterId != null && destClusterId != null && ! srcClusterId.equals(destClusterId); } - private String getHostGuidForLocalStorage(Map volumeToPool) { - String hostGuidInTargetCluster = null; - for (Map.Entry entry : volumeToPool.entrySet()) { - Volume volume = entry.getKey(); - StoragePool pool = entry.getValue(); - if (volume.getVolumeType().equals(Volume.Type.ROOT) && pool.isLocal()) { - List storagePoolHostVOs = storagePoolHostDao.listByPoolId(pool.getId()); - StoragePoolHostVO storagePoolHostVO = storagePoolHostVOs.get(0); - HostVO hostVO = _hostDao.findById(storagePoolHostVO.getHostId()); - hostGuidInTargetCluster = hostVO.getGuid(); - break; - } - } - return hostGuidInTargetCluster; + private String getHostGuidForLocalStorage(StoragePool pool) { + List storagePoolHostVOs = storagePoolHostDao.listByPoolId(pool.getId()); + StoragePoolHostVO storagePoolHostVO = storagePoolHostVOs.get(0); + HostVO hostVO = _hostDao.findById(storagePoolHostVO.getHostId()); + return hostVO.getGuid(); } private String getHostGuidInTargetCluster(boolean isInterClusterMigration, Long destClusterId) { @@ -1115,7 +1106,7 @@ public List finalizeMigrate(VirtualMachine vm, Map // OfflineVmwareMigration: specialised migration command List> volumeToFilerTo = new ArrayList>(); Long poolClusterId = null; - boolean isLocalStorageMigration = false; + StoragePool targetVmPool = null; for (Map.Entry entry : volumeToPool.entrySet()) { Volume volume = entry.getKey(); StoragePool pool = entry.getValue(); @@ -1125,7 +1116,9 @@ public List finalizeMigrate(VirtualMachine vm, Map poolClusterId = pool.getClusterId(); } if (volume.getVolumeType().equals(Volume.Type.ROOT) && pool.isLocal()) { - isLocalStorageMigration = true; + if (volume.getVolumeType().equals(Volume.Type.ROOT)) { + targetVmPool = pool; + } } volumeToFilerTo.add(new Pair(volumeTo, filerTo)); } @@ -1133,8 +1126,8 @@ public List finalizeMigrate(VirtualMachine vm, Map final Long srcClusterId = vmManager.findClusterAndHostIdForVm(vm.getId()).first(); final boolean isInterClusterMigration = isInterClusterMigration(destClusterId, srcClusterId); String targetHostGuid = null; - if (isLocalStorageMigration) { - targetHostGuid = getHostGuidForLocalStorage(volumeToPool); + if (targetVmPool != null && targetVmPool.isLocal()) { + targetHostGuid = getHostGuidForLocalStorage(targetVmPool); } else { targetHostGuid = getHostGuidInTargetCluster(isInterClusterMigration, destClusterId); } From e87fd13326c7c9e831d4f106c46f6bec2b2bc892 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 28 Jun 2022 12:36:35 +0530 Subject: [PATCH 4/6] Fixed code smells and added unit test --- .../com/cloud/hypervisor/guru/VMwareGuru.java | 22 ++-- .../cloud/hypervisor/guru/VMwareGuruTest.java | 119 ++++++++++++++++++ 2 files changed, 132 insertions(+), 9 deletions(-) create mode 100644 plugins/hypervisors/vmware/src/test/java/com/cloud/hypervisor/guru/VMwareGuruTest.java diff --git a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java index 4a22d2fbd18c..851abdc3a91b 100644 --- a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java +++ b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java @@ -1116,21 +1116,15 @@ public List finalizeMigrate(VirtualMachine vm, Map poolClusterId = pool.getClusterId(); } if (volume.getVolumeType().equals(Volume.Type.ROOT) && pool.isLocal()) { - if (volume.getVolumeType().equals(Volume.Type.ROOT)) { - targetVmPool = pool; - } + targetVmPool = pool; } volumeToFilerTo.add(new Pair(volumeTo, filerTo)); } final Long destClusterId = poolClusterId; final Long srcClusterId = vmManager.findClusterAndHostIdForVm(vm.getId()).first(); final boolean isInterClusterMigration = isInterClusterMigration(destClusterId, srcClusterId); - String targetHostGuid = null; - if (targetVmPool != null && targetVmPool.isLocal()) { - targetHostGuid = getHostGuidForLocalStorage(targetVmPool); - } else { - targetHostGuid = getHostGuidInTargetCluster(isInterClusterMigration, destClusterId); - } + String targetHostGuid = getTargetHostGuid(targetVmPool, destClusterId, isInterClusterMigration); + MigrateVmToPoolCommand migrateVmToPoolCommand = new MigrateVmToPoolCommand(vm.getInstanceName(), volumeToFilerTo, targetHostGuid, true); commands.add(migrateVmToPoolCommand); @@ -1149,6 +1143,16 @@ public List finalizeMigrate(VirtualMachine vm, Map return commands; } + private String getTargetHostGuid(StoragePool targetVmPool, Long destClusterId, boolean isInterClusterMigration) { + String targetHostGuid = null; + if (targetVmPool != null && targetVmPool.isLocal()) { + targetHostGuid = getHostGuidForLocalStorage(targetVmPool); + } else { + targetHostGuid = getHostGuidInTargetCluster(isInterClusterMigration, destClusterId); + } + return targetHostGuid; + } + @Override protected VirtualMachineTO toVirtualMachineTO(VirtualMachineProfile vmProfile) { return super.toVirtualMachineTO(vmProfile); diff --git a/plugins/hypervisors/vmware/src/test/java/com/cloud/hypervisor/guru/VMwareGuruTest.java b/plugins/hypervisors/vmware/src/test/java/com/cloud/hypervisor/guru/VMwareGuruTest.java new file mode 100644 index 000000000000..ca618ac1b0f9 --- /dev/null +++ b/plugins/hypervisors/vmware/src/test/java/com/cloud/hypervisor/guru/VMwareGuruTest.java @@ -0,0 +1,119 @@ +// 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.guru; + +import com.cloud.agent.api.Command; +import com.cloud.agent.api.MigrateVmToPoolCommand; +import com.cloud.dc.ClusterDetailsDao; +import com.cloud.host.HostVO; +import com.cloud.host.dao.HostDao; +import com.cloud.storage.StoragePool; +import com.cloud.storage.StoragePoolHostVO; +import com.cloud.storage.Volume; +import com.cloud.storage.dao.StoragePoolHostDao; +import com.cloud.utils.Pair; +import com.cloud.vm.VirtualMachine; +import com.cloud.vm.VirtualMachineManager; +import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; +import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.MockitoAnnotations; +import org.mockito.Spy; +import org.powermock.core.classloader.annotations.PrepareForTest; +import org.powermock.modules.junit4.PowerMockRunner; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.test.context.support.AnnotationConfigContextLoader; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +@RunWith(PowerMockRunner.class) +@PrepareForTest({VMwareGuru.class}) +@ContextConfiguration(loader = AnnotationConfigContextLoader.class) +public class VMwareGuruTest { + + @Spy + @InjectMocks + private VMwareGuru vMwareGuru = new VMwareGuru(); + + @Mock + PrimaryDataStoreDao _storagePoolDao; + + @Mock + StoragePoolHostDao storagePoolHostDao; + + @Mock + HostDao _hostDao; + + @Mock + VirtualMachineManager vmManager; + + @Mock + ClusterDetailsDao _clusterDetailsDao; + + @Before + public void testSetUp() throws Exception { + MockitoAnnotations.initMocks(this); + } + + @Test + public void finalizeMigrateForLocalStorageToHaveTargetHostGuid(){ + VirtualMachine vm = Mockito.mock(VirtualMachine.class); + Map volumeToPool = new HashMap<>(); + Volume rootVolume = Mockito.mock(Volume.class); + Volume dataVolume = Mockito.mock(Volume.class); + StoragePool localStorage = Mockito.mock(StoragePool.class); + volumeToPool.put(rootVolume, localStorage); + volumeToPool.put(dataVolume, localStorage); + + // prepare localstorage host guid + StoragePoolVO storagePoolVO = Mockito.mock(StoragePoolVO.class); + StoragePoolHostVO storagePoolHostVO = Mockito.mock(StoragePoolHostVO.class); + HostVO hostVO = Mockito.mock(HostVO.class); + + Mockito.when(localStorage.getId()).thenReturn(1L); + Mockito.when(vm.getId()).thenReturn(1L); + Mockito.when(_storagePoolDao.findById(1L)).thenReturn(storagePoolVO); + Mockito.when(rootVolume.getVolumeType()).thenReturn(Volume.Type.ROOT); + Mockito.when(dataVolume.getVolumeType()).thenReturn(Volume.Type.DATADISK); + Mockito.when(localStorage.isLocal()).thenReturn(true); + Pair clusterAndHost = new Pair<>(1L, 1L); + + Mockito.when(vmManager.findClusterAndHostIdForVm(1L)).thenReturn(clusterAndHost); + + List storagePoolHostVOS = new ArrayList<>(); + storagePoolHostVOS.add(storagePoolHostVO); + Mockito.when(storagePoolHostDao.listByPoolId(1L)).thenReturn(storagePoolHostVOS); + Mockito.when(storagePoolHostVO.getHostId()).thenReturn(2L); + Mockito.when(_hostDao.findById(2L)).thenReturn(hostVO); + Mockito.when(hostVO.getGuid()).thenReturn("HostSystem:host-a@x.x.x.x"); + + List commandsList = vMwareGuru.finalizeMigrate(vm, volumeToPool); + + MigrateVmToPoolCommand migrateVmToPoolCommand = (MigrateVmToPoolCommand) commandsList.get(0); + Assert.assertEquals("HostSystem:host-a@x.x.x.x", migrateVmToPoolCommand.getHostGuidInTargetCluster()); + } + +} From 2f1702fa5ad553ddc214235904b694daa2c18a64 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 28 Jun 2022 13:18:06 +0530 Subject: [PATCH 5/6] few variable changes --- .../java/com/cloud/hypervisor/guru/VMwareGuru.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java index 851abdc3a91b..f156a3420d81 100644 --- a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java +++ b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java @@ -1106,7 +1106,7 @@ public List finalizeMigrate(VirtualMachine vm, Map // OfflineVmwareMigration: specialised migration command List> volumeToFilerTo = new ArrayList>(); Long poolClusterId = null; - StoragePool targetVmPool = null; + StoragePool targetLocalPoolForVM = null; for (Map.Entry entry : volumeToPool.entrySet()) { Volume volume = entry.getKey(); StoragePool pool = entry.getValue(); @@ -1116,14 +1116,14 @@ public List finalizeMigrate(VirtualMachine vm, Map poolClusterId = pool.getClusterId(); } if (volume.getVolumeType().equals(Volume.Type.ROOT) && pool.isLocal()) { - targetVmPool = pool; + targetLocalPoolForVM = pool; } volumeToFilerTo.add(new Pair(volumeTo, filerTo)); } final Long destClusterId = poolClusterId; final Long srcClusterId = vmManager.findClusterAndHostIdForVm(vm.getId()).first(); final boolean isInterClusterMigration = isInterClusterMigration(destClusterId, srcClusterId); - String targetHostGuid = getTargetHostGuid(targetVmPool, destClusterId, isInterClusterMigration); + String targetHostGuid = getTargetHostGuid(targetLocalPoolForVM, destClusterId, isInterClusterMigration); MigrateVmToPoolCommand migrateVmToPoolCommand = new MigrateVmToPoolCommand(vm.getInstanceName(), volumeToFilerTo, targetHostGuid, true); @@ -1143,10 +1143,10 @@ public List finalizeMigrate(VirtualMachine vm, Map return commands; } - private String getTargetHostGuid(StoragePool targetVmPool, Long destClusterId, boolean isInterClusterMigration) { + private String getTargetHostGuid(StoragePool targetLocalPoolForVM, Long destClusterId, boolean isInterClusterMigration) { String targetHostGuid = null; - if (targetVmPool != null && targetVmPool.isLocal()) { - targetHostGuid = getHostGuidForLocalStorage(targetVmPool); + if (targetLocalPoolForVM != null) { + targetHostGuid = getHostGuidForLocalStorage(targetLocalPoolForVM); } else { targetHostGuid = getHostGuidInTargetCluster(isInterClusterMigration, destClusterId); } From 5b7b05e812d57561fb3331014f3e591b21c84e1c Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 28 Jun 2022 13:29:36 +0530 Subject: [PATCH 6/6] Added a comment --- .../src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java index f156a3420d81..5d29be9ebc94 100644 --- a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java +++ b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java @@ -1146,6 +1146,7 @@ public List finalizeMigrate(VirtualMachine vm, Map private String getTargetHostGuid(StoragePool targetLocalPoolForVM, Long destClusterId, boolean isInterClusterMigration) { String targetHostGuid = null; if (targetLocalPoolForVM != null) { + // Get the target host for local storage migration targetHostGuid = getHostGuidForLocalStorage(targetLocalPoolForVM); } else { targetHostGuid = getHostGuidInTargetCluster(isInterClusterMigration, destClusterId);