diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java index 829a4d26fcb..65766464827 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java @@ -1349,30 +1349,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) { diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index df157bf2bb5..86c87ed02b8 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/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); + } }