From a3b86573b921ee470ac60b3e801a6d18f7939658 Mon Sep 17 00:00:00 2001 From: Alena Prokharchyk Date: Thu, 1 Aug 2013 15:34:55 -0700 Subject: [PATCH] CLOUDSTACK-4020: lock nic entry in releaseNic method. Otherwise multiple threads can try to release the same nic at the same time, and it will lead to NPEs and backend failures Conflicts: server/src/com/cloud/network/NetworkManagerImpl.java server/src/com/cloud/vm/VirtualMachineManagerImpl.java --- .../com/cloud/network/NetworkManagerImpl.java | 13 ++- .../cloud/vm/VirtualMachineManagerImpl.java | 84 ++++++++++++------- 2 files changed, 61 insertions(+), 36 deletions(-) diff --git a/server/src/com/cloud/network/NetworkManagerImpl.java b/server/src/com/cloud/network/NetworkManagerImpl.java index b4890970ec1..ea3d552ca80 100755 --- a/server/src/com/cloud/network/NetworkManagerImpl.java +++ b/server/src/com/cloud/network/NetworkManagerImpl.java @@ -2236,33 +2236,32 @@ public class NetworkManagerImpl extends ManagerBase implements NetworkManager, L ConcurrentOperationException, ResourceUnavailableException { List nics = _nicDao.listByVmId(vmProfile.getId()); for (NicVO nic : nics) { - releaseNic(vmProfile, nic); + releaseNic(vmProfile, nic.getId()); } } - + @Override @DB public void releaseNic(VirtualMachineProfile vmProfile, Nic nic) throws ConcurrentOperationException, ResourceUnavailableException { - NicVO nicVO = _nicDao.findById(nic.getId()); - releaseNic(vmProfile, nicVO); + releaseNic(vmProfile, nic.getId()); } @DB - protected void releaseNic(VirtualMachineProfile vmProfile, NicVO nicVO) + protected void releaseNic(VirtualMachineProfile vmProfile, long nicId) throws ConcurrentOperationException, ResourceUnavailableException { //lock the nic Transaction txn = Transaction.currentTxn(); txn.start(); - NicVO nic = _nicDao.lockRow(nicVO.getId(), true); + NicVO nic = _nicDao.lockRow(nicId, true); if (nic == null) { throw new ConcurrentOperationException("Unable to acquire lock on nic " + nic); } Nic.State originalState = nic.getState(); - NetworkVO network = _networksDao.findById(nicVO.getNetworkId()); + NetworkVO network = _networksDao.findById(nic.getNetworkId()); if (originalState == Nic.State.Reserved || originalState == Nic.State.Reserving) { if (nic.getReservationStrategy() == Nic.ReservationStrategy.Start) { diff --git a/server/src/com/cloud/vm/VirtualMachineManagerImpl.java b/server/src/com/cloud/vm/VirtualMachineManagerImpl.java index 5b7aa5b281d..08bca43e733 100755 --- a/server/src/com/cloud/vm/VirtualMachineManagerImpl.java +++ b/server/src/com/cloud/vm/VirtualMachineManagerImpl.java @@ -2953,6 +2953,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac } @Override + @DB public boolean removeVmFromNetwork(VirtualMachine vm, Network network, URI broadcastUri) throws ConcurrentOperationException, ResourceUnavailableException { VMInstanceVO vmVO = _vmDao.findById(vm.getId()); ReservationContext context = new ReservationContextImpl(null, null, _accountMgr.getActiveUser(User.UID_SYSTEM), @@ -2968,53 +2969,78 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac VirtualMachineTO vmTO = hvGuru.implement(vmProfile); Nic nic = null; - if (broadcastUri != null) { nic = _nicsDao.findByNetworkIdInstanceIdAndBroadcastUri(network.getId(), vm.getId(), broadcastUri.toString()); } else { nic = _networkModel.getNicInNetwork(vm.getId(), network.getId()); } - - if (nic == null) { + + if (nic == null){ s_logger.warn("Could not get a nic with " + network); return false; } - + // don't delete default NIC on a user VM if (nic.isDefaultNic() && vm.getType() == VirtualMachine.Type.User) { s_logger.warn("Failed to remove nic from " + vm + " in " + network + ", nic is default."); throw new CloudRuntimeException("Failed to remove nic from " + vm + " in " + network + ", nic is default."); } - NicProfile nicProfile = new NicProfile(nic, network, nic.getBroadcastUri(), nic.getIsolationUri(), - _networkModel.getNetworkRate(network.getId(), vm.getId()), - _networkModel.isSecurityGroupSupportedInNetwork(network), - _networkModel.getNetworkTag(vmProfile.getVirtualMachine().getHypervisorType(), network)); - - //1) Unplug the nic - if (vm.getState() == State.Running) { - NicTO nicTO = toNicTO(nicProfile, vmProfile.getVirtualMachine().getHypervisorType()); - s_logger.debug("Un-plugging nic for vm " + vm + " from network " + network); - boolean result = unplugNic(network, nicTO, vmTO, context, dest); - if (result) { - s_logger.debug("Nic is unplugged successfully for vm " + vm + " in network " + network); - } else { - s_logger.warn("Failed to unplug nic for the vm " + vm + " from network " + network); - return false; + //Lock on nic is needed here + Nic lock = _nicsDao.acquireInLockTable(nic.getId()); + if (lock == null) { + //check if nic is still there. Return if it was released already + if (_nicsDao.findById(nic.getId()) == null) { + if (s_logger.isDebugEnabled()) { + s_logger.debug("Not need to remove the vm " + vm + " from network " + network + " as the vm doesn't have nic in this network"); + } + return true; } - } else if (vm.getState() != State.Stopped) { - s_logger.warn("Unable to remove vm " + vm + " from network " + network); - throw new ResourceUnavailableException("Unable to remove vm " + vm + " from network, is not in the right state", - DataCenter.class, vm.getDataCenterId()); + throw new ConcurrentOperationException("Unable to lock nic " + nic.getId()); } + + if (s_logger.isDebugEnabled()) { + s_logger.debug("Lock is acquired for nic id " + lock.getId() + " as a part of remove vm " + vm + " from network " + network); + } + + try { + NicProfile nicProfile = new NicProfile(nic, network, nic.getBroadcastUri(), nic.getIsolationUri(), + _networkModel.getNetworkRate(network.getId(), vm.getId()), + _networkModel.isSecurityGroupSupportedInNetwork(network), + _networkModel.getNetworkTag(vmProfile.getVirtualMachine().getHypervisorType(), network)); - //2) Release the nic - _networkMgr.releaseNic(vmProfile, nic); - s_logger.debug("Successfully released nic " + nic + "for vm " + vm); + //1) Unplug the nic + if (vm.getState() == State.Running) { + NicTO nicTO = toNicTO(nicProfile, vmProfile.getVirtualMachine().getHypervisorType()); + s_logger.debug("Un-plugging nic for vm " + vm + " from network " + network); + boolean result = unplugNic(network, nicTO, vmTO, context, dest); + if (result) { + s_logger.debug("Nic is unplugged successfully for vm " + vm + " in network " + network ); + } else { + s_logger.warn("Failed to unplug nic for the vm " + vm + " from network " + network); + return false; + } + } else if (vm.getState() != State.Stopped) { + s_logger.warn("Unable to remove vm " + vm + " from network " + network); + throw new ResourceUnavailableException("Unable to remove vm " + vm + " from network, is not in the right state", + DataCenter.class, vm.getDataCenterId()); + } - //3) Remove the nic - _networkMgr.removeNic(vmProfile, nic); - return true; + //2) Release the nic + _networkMgr.releaseNic(vmProfile, nic); + s_logger.debug("Successfully released nic " + nic + "for vm " + vm); + + //3) Remove the nic + _networkMgr.removeNic(vmProfile, nic); + return true; + } finally { + if (lock != null) { + _nicsDao.releaseFromLockTable(lock.getId()); + if (s_logger.isDebugEnabled()) { + s_logger.debug("Lock is released for nic id " + lock.getId() + " as a part of remove vm " + vm + " from network " + network); + } + } + } } @Override