From e471a46a05b623ffc5cc5f950238b3525bc1c2e9 Mon Sep 17 00:00:00 2001 From: Mike Tutkowski Date: Mon, 21 May 2018 04:52:33 -0600 Subject: [PATCH 1/2] managed-storage: handle VM start in a new cluster on different host (#2656) Example: A VM that uses managed storage is stopped. The VM is then started on a different host in the same cluster. The Start operation fails. To get around this issue, you must either start the VM up on the same host or on a host in a different cluster. The reason is due to a slightly erroneous check in VolumeOrchestrator.prepare. To solve this issue, we should be checking if the cluster ID changes, not if the host ID changes. --- .../orchestration/VolumeOrchestrator.java | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java b/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java index a9035d4100f..e5e0bbfed59 100644 --- a/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java +++ b/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java @@ -1344,30 +1344,29 @@ public class VolumeOrchestrator extends ManagerBase implements VolumeOrchestrati StoragePool pool; for (VolumeTask task : tasks) { if (task.type == VolumeTaskType.NOP) { - pool = (StoragePool)dataStoreMgr.getDataStore(task.pool.getId(), DataStoreRole.Primary); - - if (task.pool != null && task.pool.isManaged()) { - long hostId = vm.getVirtualMachine().getHostId(); - Host host = _hostDao.findById(hostId); - - volService.grantAccess(volFactory.getVolume(task.volume.getId()), host, (DataStore)pool); - } - vol = task.volume; - // For a zone-wide managed storage, it is possible that the VM can be started in another + pool = (StoragePool)dataStoreMgr.getDataStore(task.pool.getId(), DataStoreRole.Primary); + + // For zone-wide managed storage, it is possible that the VM can be started in another // cluster. In that case, make sure that the volume is in the right access group. if (pool.isManaged()) { - long oldHostId = vm.getVirtualMachine().getLastHostId(); - long hostId = vm.getVirtualMachine().getHostId(); + Host lastHost = _hostDao.findById(vm.getVirtualMachine().getLastHostId()); + Host host = _hostDao.findById(vm.getVirtualMachine().getHostId()); - if (oldHostId != hostId) { - Host oldHost = _hostDao.findById(oldHostId); - DataStore storagePool = dataStoreMgr.getDataStore(pool.getId(), DataStoreRole.Primary); + long lastClusterId = lastHost == null || lastHost.getClusterId() == null ? -1 : lastHost.getClusterId(); + long clusterId = host == null || host.getClusterId() == null ? -1 : host.getClusterId(); - storageMgr.removeStoragePoolFromCluster(oldHostId, vol.get_iScsiName(), pool); + if (lastClusterId != clusterId) { + if (lastHost != null) { + storageMgr.removeStoragePoolFromCluster(lastHost.getId(), vol.get_iScsiName(), pool); - volService.revokeAccess(volFactory.getVolume(vol.getId()), oldHost, storagePool); + DataStore storagePool = dataStoreMgr.getDataStore(pool.getId(), DataStoreRole.Primary); + + volService.revokeAccess(volFactory.getVolume(vol.getId()), lastHost, storagePool); + } + + volService.grantAccess(volFactory.getVolume(vol.getId()), host, (DataStore)pool); } } } else if (task.type == VolumeTaskType.MIGRATE) { From 02ece533755282e7d8f4610157b5d9febd199784 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20Beims=20Br=C3=A4scher?= Date: Mon, 21 May 2018 07:54:21 -0300 Subject: [PATCH 2/2] addNicToVirtualMachine: Fixes #2540 handle invalid MAC address arg (#2653) Look for the next available MAC address if the given MAC address in command addNicToVirtualMachine is invalid (null, empty, blank). Fixes #2540 --- .../src/com/cloud/vm/UserVmManagerImpl.java | 15 +++++ .../com/cloud/vm/UserVmManagerImplTest.java | 55 +++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/server/src/com/cloud/vm/UserVmManagerImpl.java b/server/src/com/cloud/vm/UserVmManagerImpl.java index 4ac48c9cd31..d3eed38cc0e 100644 --- a/server/src/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/com/cloud/vm/UserVmManagerImpl.java @@ -1169,6 +1169,8 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir } } + macAddress = validateOrReplaceMacAddress(macAddress, network.getId()); + if(_nicDao.findByNetworkIdAndMacAddress(networkId, macAddress) != null) { throw new CloudRuntimeException("A NIC with this MAC address exists for network: " + network.getUuid()); } @@ -1239,6 +1241,19 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir return _vmDao.findById(vmInstance.getId()); } + /** + * If the given MAC address is invalid it replaces the given MAC with the next available MAC address + */ + protected String validateOrReplaceMacAddress(String macAddress, long networkId) { + if (!NetUtils.isValidMac(macAddress)) { + try { + macAddress = _networkModel.getNextAvailableMacAddressInNetwork(networkId); + } catch (InsufficientAddressCapacityException e) { + throw new CloudRuntimeException(String.format("A MAC address cannot be generated for this NIC in the network [id=%s] ", networkId)); + } + } + return macAddress; + } private void saveExtraDhcpOptions(long nicId, Map dhcpOptions) { List nicExtraDhcpOptionVOList = dhcpOptions diff --git a/server/test/com/cloud/vm/UserVmManagerImplTest.java b/server/test/com/cloud/vm/UserVmManagerImplTest.java index bec9d7dee6a..2744714c67b 100644 --- a/server/test/com/cloud/vm/UserVmManagerImplTest.java +++ b/server/test/com/cloud/vm/UserVmManagerImplTest.java @@ -22,6 +22,7 @@ import java.util.HashMap; import org.apache.cloudstack.api.BaseCmd.HTTPMethod; import org.apache.cloudstack.api.command.user.vm.UpdateVMCmd; import org.apache.cloudstack.context.CallContext; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -34,9 +35,11 @@ import org.powermock.api.mockito.PowerMockito; import org.powermock.core.classloader.annotations.PrepareForTest; import org.powermock.modules.junit4.PowerMockRunner; +import com.cloud.exception.InsufficientAddressCapacityException; import com.cloud.exception.InsufficientCapacityException; import com.cloud.exception.InvalidParameterValueException; import com.cloud.exception.ResourceUnavailableException; +import com.cloud.network.NetworkModel; import com.cloud.storage.GuestOSVO; import com.cloud.storage.dao.GuestOSDao; import com.cloud.user.Account; @@ -70,6 +73,9 @@ public class UserVmManagerImplTest { @Mock private UserVmVO userVmVoMock; + @Mock + private NetworkModel networkModel; + private long vmId = 1l; @Before @@ -221,4 +227,53 @@ public class UserVmManagerImplTest { Mockito.anyString(), Mockito.anyBoolean(), Mockito.any(HTTPMethod.class), Mockito.anyString(), Mockito.anyString(), Mockito.anyString(), Mockito.anyListOf(Long.class), Mockito.anyMap()); } + + @Test + public void validateOrReplaceMacAddressTestMacAddressValid() throws InsufficientAddressCapacityException { + configureValidateOrReplaceMacAddressTest(0, "01:23:45:67:89:ab", "01:23:45:67:89:ab"); + } + + @Test + public void validateOrReplaceMacAddressTestMacAddressNull() throws InsufficientAddressCapacityException { + configureValidateOrReplaceMacAddressTest(1, null, "01:23:45:67:89:ab"); + } + + @Test + public void validateOrReplaceMacAddressTestMacAddressBlank() throws InsufficientAddressCapacityException { + configureValidateOrReplaceMacAddressTest(1, " ", "01:23:45:67:89:ab"); + } + + @Test + public void validateOrReplaceMacAddressTestMacAddressEmpty() throws InsufficientAddressCapacityException { + configureValidateOrReplaceMacAddressTest(1, "", "01:23:45:67:89:ab"); + } + + @Test + public void validateOrReplaceMacAddressTestMacAddressNotValidOption1() throws InsufficientAddressCapacityException { + configureValidateOrReplaceMacAddressTest(1, "abcdef:gh:ij:kl", "01:23:45:67:89:ab"); + } + + @Test + public void validateOrReplaceMacAddressTestMacAddressNotValidOption2() throws InsufficientAddressCapacityException { + configureValidateOrReplaceMacAddressTest(1, "01:23:45:67:89:", "01:23:45:67:89:ab"); + } + + @Test + public void validateOrReplaceMacAddressTestMacAddressNotValidOption3() throws InsufficientAddressCapacityException { + configureValidateOrReplaceMacAddressTest(1, "01:23:45:67:89:az", "01:23:45:67:89:ab"); + } + + @Test + public void validateOrReplaceMacAddressTestMacAddressNotValidOption4() throws InsufficientAddressCapacityException { + configureValidateOrReplaceMacAddressTest(1, "@1:23:45:67:89:ab", "01:23:45:67:89:ab"); + } + + private void configureValidateOrReplaceMacAddressTest(int times, String macAddress, String expectedMacAddress) throws InsufficientAddressCapacityException { + Mockito.when(networkModel.getNextAvailableMacAddressInNetwork(Mockito.anyLong())).thenReturn(expectedMacAddress); + + String returnedMacAddress = userVmManagerImpl.validateOrReplaceMacAddress(macAddress, 1l); + + Mockito.verify(networkModel, Mockito.times(times)).getNextAvailableMacAddressInNetwork(Mockito.anyLong()); + Assert.assertEquals(expectedMacAddress, returnedMacAddress); + } }