From 60ff09d4f7cadfebafd84e3cbc651369cae116d9 Mon Sep 17 00:00:00 2001 From: nvazquez Date: Mon, 6 Feb 2017 12:04:34 -0300 Subject: [PATCH] CLOUDSTACK-9764: Delete domain failure due to Account Cleanup task --- .../src/com/cloud/user/DomainManagerImpl.java | 225 ++++++++++++------ .../com/cloud/user/DomainManagerImplTest.java | 122 ++++++++-- 2 files changed, 256 insertions(+), 91 deletions(-) diff --git a/server/src/com/cloud/user/DomainManagerImpl.java b/server/src/com/cloud/user/DomainManagerImpl.java index ec0136f9372..423fbc7bcd1 100644 --- a/server/src/com/cloud/user/DomainManagerImpl.java +++ b/server/src/com/cloud/user/DomainManagerImpl.java @@ -34,6 +34,8 @@ import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationSe import org.apache.cloudstack.framework.messagebus.MessageBus; import org.apache.cloudstack.framework.messagebus.PublishScope; import org.apache.cloudstack.region.RegionManager; +import org.apache.commons.collections.CollectionUtils; +import org.apache.commons.lang.BooleanUtils; import com.cloud.configuration.Resource.ResourceOwnerType; import com.cloud.configuration.ResourceLimit; @@ -63,6 +65,7 @@ import com.cloud.utils.Pair; import com.cloud.utils.component.ManagerBase; import com.cloud.utils.db.DB; import com.cloud.utils.db.Filter; +import com.cloud.utils.db.GlobalLock; import com.cloud.utils.db.SearchBuilder; import com.cloud.utils.db.SearchCriteria; import com.cloud.utils.db.Transaction; @@ -109,6 +112,14 @@ public class DomainManagerImpl extends ManagerBase implements DomainManager, Dom @Inject MessageBus _messageBus; + protected GlobalLock getGlobalLock(String name) { + return GlobalLock.getInternLock(name); + } + + protected Account getCaller() { + return CallContext.current().getCallingAccount(); + } + @Override public Domain getDomain(long domainId) { return _domainDao.findById(domainId); @@ -151,7 +162,7 @@ public class DomainManagerImpl extends ManagerBase implements DomainManager, Dom @Override @ActionEvent(eventType = EventTypes.EVENT_DOMAIN_CREATE, eventDescription = "creating Domain") public Domain createDomain(String name, Long parentId, String networkDomain, String domainUUID) { - Account caller = CallContext.current().getCallingAccount(); + Account caller = getCaller(); if (parentId == null) { parentId = Long.valueOf(Domain.ROOT_DOMAIN); @@ -256,7 +267,7 @@ public class DomainManagerImpl extends ManagerBase implements DomainManager, Dom @Override @ActionEvent(eventType = EventTypes.EVENT_DOMAIN_DELETE, eventDescription = "deleting Domain", async = true) public boolean deleteDomain(long domainId, Boolean cleanup) { - Account caller = CallContext.current().getCallingAccount(); + Account caller = getCaller(); DomainVO domain = _domainDao.findById(domainId); @@ -273,82 +284,146 @@ public class DomainManagerImpl extends ManagerBase implements DomainManager, Dom @Override public boolean deleteDomain(DomainVO domain, Boolean cleanup) { - // mark domain as inactive - s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it"); - domain.setState(Domain.State.Inactive); - _domainDao.update(domain.getId(), domain); - boolean rollBackState = false; - boolean hasDedicatedResources = false; + GlobalLock lock = getGlobalLock("AccountCleanup"); + if (lock == null) { + s_logger.debug("Couldn't get the global lock"); + return false; + } + + if (!lock.lock(30)) { + s_logger.debug("Couldn't lock the db"); + return false; + } try { - long ownerId = domain.getAccountId(); - if ((cleanup != null) && cleanup.booleanValue()) { - if (!cleanupDomain(domain.getId(), ownerId)) { - rollBackState = true; - CloudRuntimeException e = - new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " + - domain.getId() + ")."); - e.addProxyObject(domain.getUuid(), "domainId"); - throw e; - } - } else { - //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources - List networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId()); - List accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId()); - List dedicatedResources = _dedicatedDao.listByDomainId(domain.getId()); - if (dedicatedResources != null && !dedicatedResources.isEmpty()) { - s_logger.error("There are dedicated resources for the domain " + domain.getId()); - hasDedicatedResources = true; - } - if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) { - _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain); - if (!_domainDao.remove(domain.getId())) { - rollBackState = true; - CloudRuntimeException e = - new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() + - "); Please make sure all users and sub domains have been removed from the domain before deleting"); - e.addProxyObject(domain.getUuid(), "domainId"); - throw e; - } - _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain); + // mark domain as inactive + s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it"); + domain.setState(Domain.State.Inactive); + _domainDao.update(domain.getId(), domain); + + try { + long ownerId = domain.getAccountId(); + if (BooleanUtils.toBoolean(cleanup)) { + tryCleanupDomain(domain, ownerId); } else { - rollBackState = true; - String msg = null; - if (!accountsForCleanup.isEmpty()) { - msg = accountsForCleanup.size() + " accounts to cleanup"; - } else if (!networkIds.isEmpty()) { - msg = networkIds.size() + " non-removed networks"; - } else if (hasDedicatedResources) { - msg = "dedicated resources."; - } - - CloudRuntimeException e = new CloudRuntimeException("Can't delete the domain yet because it has " + msg); - e.addProxyObject(domain.getUuid(), "domainId"); - throw e; + removeDomainWithNoAccountsForCleanupNetworksOrDedicatedResources(domain); } - } - cleanupDomainOfferings(domain.getId()); - CallContext.current().putContextParameter(Domain.class, domain.getUuid()); - return true; - } catch (Exception ex) { - s_logger.error("Exception deleting domain with id " + domain.getId(), ex); - if (ex instanceof CloudRuntimeException) - throw (CloudRuntimeException)ex; - else - return false; - } finally { - //when success is false - if (rollBackState) { - s_logger.debug("Changing domain id=" + domain.getId() + " state back to " + Domain.State.Active + - " because it can't be removed due to resources referencing to it"); - domain.setState(Domain.State.Active); - _domainDao.update(domain.getId(), domain); + cleanupDomainOfferings(domain.getId()); + CallContext.current().putContextParameter(Domain.class, domain.getUuid()); + return true; + } catch (Exception ex) { + s_logger.error("Exception deleting domain with id " + domain.getId(), ex); + if (ex instanceof CloudRuntimeException) { + rollbackDomainState(domain); + throw (CloudRuntimeException)ex; + } + else + return false; } } + finally { + lock.unlock(); + } } - private void cleanupDomainOfferings(Long domainId) { + /** + * Roll back domain state to Active + * @param domain domain + */ + protected void rollbackDomainState(DomainVO domain) { + s_logger.debug("Changing domain id=" + domain.getId() + " state back to " + Domain.State.Active + + " because it can't be removed due to resources referencing to it"); + domain.setState(Domain.State.Active); + _domainDao.update(domain.getId(), domain); + } + + /** + * Try cleaning up domain. If it couldn't throws CloudRuntimeException + * @param domain domain + * @param ownerId owner id + * @throws ConcurrentOperationException + * @throws ResourceUnavailableException + * @throws CloudRuntimeException when cleanupDomain + */ + protected void tryCleanupDomain(DomainVO domain, long ownerId) throws ConcurrentOperationException, ResourceUnavailableException, CloudRuntimeException { + if (!cleanupDomain(domain.getId(), ownerId)) { + CloudRuntimeException e = + new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " + + domain.getId() + ")."); + e.addProxyObject(domain.getUuid(), "domainId"); + throw e; + } + } + + /** + * First check domain resources before removing domain. There are 2 cases: + *
    + *
  1. Domain doesn't have accounts for cleanup, non-removed networks, or dedicated resources
    2. + *
    • Delete domain
    + *
  2. Domain has one of the following: accounts set for cleanup, non-removed networks, dedicated resources
    3. + *
    • Dont' delete domain
    • Fail operation
    + *
+ * @param domain domain to remove + * @throws CloudRuntimeException when case 2 or when domain cannot be deleted on case 1 + */ + protected void removeDomainWithNoAccountsForCleanupNetworksOrDedicatedResources(DomainVO domain) { + boolean hasDedicatedResources = false; + List networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId()); + List accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId()); + List dedicatedResources = _dedicatedDao.listByDomainId(domain.getId()); + if (CollectionUtils.isNotEmpty(dedicatedResources)) { + s_logger.error("There are dedicated resources for the domain " + domain.getId()); + hasDedicatedResources = true; + } + if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) { + publishRemoveEventsAndRemoveDomain(domain); + } else { + failRemoveOperation(domain, accountsForCleanup, networkIds, hasDedicatedResources); + } + } + + /** + * Fail domain remove operation including proper message + * @param domain domain + * @param accountsForCleanup domain accounts for cleanup + * @param networkIds domain network ids + * @param hasDedicatedResources indicates if domain has dedicated resources + * @throws CloudRuntimeException including descriptive message indicating the reason for failure + */ + protected void failRemoveOperation(DomainVO domain, List accountsForCleanup, List networkIds, boolean hasDedicatedResources) { + String msg = null; + if (!accountsForCleanup.isEmpty()) { + msg = accountsForCleanup.size() + " accounts to cleanup"; + } else if (!networkIds.isEmpty()) { + msg = networkIds.size() + " non-removed networks"; + } else if (hasDedicatedResources) { + msg = "dedicated resources."; + } + + CloudRuntimeException e = new CloudRuntimeException("Can't delete the domain yet because it has " + msg); + e.addProxyObject(domain.getUuid(), "domainId"); + throw e; + } + + /** + * Publish pre-remove and remove domain events and remove domain + * @param domain domain to remove + * @throws CloudRuntimeException when domain cannot be removed + */ + protected void publishRemoveEventsAndRemoveDomain(DomainVO domain) { + _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain); + if (!_domainDao.remove(domain.getId())) { + CloudRuntimeException e = + new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() + + "); Please make sure all users and sub domains have been removed from the domain before deleting"); + e.addProxyObject(domain.getUuid(), "domainId"); + throw e; + } + _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain); + } + + protected void cleanupDomainOfferings(Long domainId) { // delete the service and disk offerings associated with this domain List diskOfferingsForThisDomain = _diskOfferingDao.listByDomainId(domainId); for (DiskOfferingVO diskOffering : diskOfferingsForThisDomain) { @@ -361,7 +436,7 @@ public class DomainManagerImpl extends ManagerBase implements DomainManager, Dom } } - private boolean cleanupDomain(Long domainId, Long ownerId) throws ConcurrentOperationException, ResourceUnavailableException { + protected boolean cleanupDomain(Long domainId, Long ownerId) throws ConcurrentOperationException, ResourceUnavailableException { s_logger.debug("Cleaning up domain id=" + domainId); boolean success = true; DomainVO domainHandle = _domainDao.findById(domainId); @@ -399,7 +474,7 @@ public class DomainManagerImpl extends ManagerBase implements DomainManager, Dom for (AccountVO account : accounts) { if (account.getType() != Account.ACCOUNT_TYPE_PROJECT) { s_logger.debug("Deleting account " + account + " as a part of domain id=" + domainId + " cleanup"); - boolean deleteAccount = _accountMgr.deleteAccount(account, CallContext.current().getCallingUserId(), CallContext.current().getCallingAccount()); + boolean deleteAccount = _accountMgr.deleteAccount(account, CallContext.current().getCallingUserId(), getCaller()); if (!deleteAccount) { s_logger.warn("Failed to cleanup account id=" + account.getId() + " as a part of domain cleanup"); } @@ -407,7 +482,7 @@ public class DomainManagerImpl extends ManagerBase implements DomainManager, Dom } else { ProjectVO project = _projectDao.findByProjectAccountId(account.getId()); s_logger.debug("Deleting project " + project + " as a part of domain id=" + domainId + " cleanup"); - boolean deleteProject = _projectMgr.deleteProject(CallContext.current().getCallingAccount(), CallContext.current().getCallingUserId(), project); + boolean deleteProject = _projectMgr.deleteProject(getCaller(), CallContext.current().getCallingUserId(), project); if (!deleteProject) { s_logger.warn("Failed to cleanup project " + project + " as a part of domain cleanup"); } @@ -470,7 +545,7 @@ public class DomainManagerImpl extends ManagerBase implements DomainManager, Dom @Override public Pair, Integer> searchForDomains(ListDomainsCmd cmd) { - Account caller = CallContext.current().getCallingAccount(); + Account caller = getCaller(); Long domainId = cmd.getId(); boolean listAll = cmd.listAll(); boolean isRecursive = false; @@ -542,7 +617,7 @@ public class DomainManagerImpl extends ManagerBase implements DomainManager, Dom boolean listAll = cmd.listAll(); String path = null; - Account caller = CallContext.current().getCallingAccount(); + Account caller = getCaller(); if (domainId != null) { _accountMgr.checkAccess(caller, getDomain(domainId)); } else { @@ -612,7 +687,7 @@ public class DomainManagerImpl extends ManagerBase implements DomainManager, Dom } // check permissions - Account caller = CallContext.current().getCallingAccount(); + Account caller = getCaller(); _accountMgr.checkAccess(caller, domain); // domain name is unique in the cloud diff --git a/server/test/com/cloud/user/DomainManagerImplTest.java b/server/test/com/cloud/user/DomainManagerImplTest.java index 82d54912b5e..4c4fcccc5bb 100644 --- a/server/test/com/cloud/user/DomainManagerImplTest.java +++ b/server/test/com/cloud/user/DomainManagerImplTest.java @@ -19,29 +19,40 @@ package com.cloud.user; import com.cloud.configuration.dao.ResourceCountDao; import com.cloud.configuration.dao.ResourceLimitDao; +import com.cloud.dc.DedicatedResourceVO; import com.cloud.dc.dao.DedicatedResourceDao; +import com.cloud.domain.Domain; import com.cloud.domain.DomainVO; import com.cloud.domain.dao.DomainDao; +import com.cloud.exception.InvalidParameterValueException; +import com.cloud.exception.PermissionDeniedException; import com.cloud.network.dao.NetworkDomainDao; import com.cloud.projects.ProjectManager; import com.cloud.projects.dao.ProjectDao; import com.cloud.service.dao.ServiceOfferingDao; import com.cloud.storage.dao.DiskOfferingDao; import com.cloud.user.dao.AccountDao; +import com.cloud.utils.db.GlobalLock; +import com.cloud.utils.exception.CloudRuntimeException; + +import java.util.ArrayList; +import java.util.List; + import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService; import org.apache.cloudstack.framework.messagebus.MessageBus; +import org.apache.cloudstack.framework.messagebus.PublishScope; import org.apache.cloudstack.region.RegionManager; import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Matchers; import org.mockito.Mock; import org.mockito.Mockito; +import org.mockito.Spy; import org.mockito.runners.MockitoJUnitRunner; -import javax.inject.Inject; -import java.lang.reflect.Field; - @RunWith(MockitoJUnitRunner.class) public class DomainManagerImplTest { @Mock @@ -73,23 +84,42 @@ public class DomainManagerImplTest { @Mock MessageBus _messageBus; - DomainManagerImpl domainManager; + @Spy + @InjectMocks + DomainManagerImpl domainManager = new DomainManagerImpl(); + + @Mock + DomainVO domain; + @Mock + Account adminAccount; + @Mock + GlobalLock lock; + + List domainAccountsForCleanup; + List domainNetworkIds; + List domainDedicatedResources; + + private static final long DOMAIN_ID = 3l; + private static final long ACCOUNT_ID = 1l; + + private static boolean testDomainCleanup = false; @Before public void setup() throws NoSuchFieldException, SecurityException, IllegalArgumentException, IllegalAccessException { - domainManager = new DomainManagerImpl(); - for (Field field : DomainManagerImpl.class.getDeclaredFields()) { - if (field.getAnnotation(Inject.class) != null) { - field.setAccessible(true); - try { - Field mockField = this.getClass().getDeclaredField( - field.getName()); - field.set(domainManager, mockField.get(this)); - } catch (Exception ignored) { - } - } - } + Mockito.doReturn(adminAccount).when(domainManager).getCaller(); + Mockito.doReturn(lock).when(domainManager).getGlobalLock("AccountCleanup"); + Mockito.when(lock.lock(Mockito.anyInt())).thenReturn(true); + Mockito.when(_domainDao.findById(DOMAIN_ID)).thenReturn(domain); + Mockito.when(domain.getAccountId()).thenReturn(ACCOUNT_ID); + Mockito.when(domain.getId()).thenReturn(DOMAIN_ID); + Mockito.when(_domainDao.remove(DOMAIN_ID)).thenReturn(true); + domainAccountsForCleanup = new ArrayList(); + domainNetworkIds = new ArrayList(); + domainDedicatedResources = new ArrayList(); + Mockito.when(_accountDao.findCleanupsForRemovedAccounts(DOMAIN_ID)).thenReturn(domainAccountsForCleanup); + Mockito.when(_networkDomainDao.listNetworkIdsByDomain(DOMAIN_ID)).thenReturn(domainNetworkIds); + Mockito.when(_dedicatedDao.listByDomainId(DOMAIN_ID)).thenReturn(domainDedicatedResources); } @Test @@ -134,4 +164,64 @@ public class DomainManagerImplTest { Assert.assertEquals(domain, domainManager.findDomainByIdOrPath(1L, "/validDomain/")); } + @Test(expected=InvalidParameterValueException.class) + public void testDeleteDomainNullDomain() { + Mockito.when(_domainDao.findById(DOMAIN_ID)).thenReturn(null); + domainManager.deleteDomain(DOMAIN_ID, testDomainCleanup); + } + + @Test(expected=PermissionDeniedException.class) + public void testDeleteDomainRootDomain() { + Mockito.when(_domainDao.findById(Domain.ROOT_DOMAIN)).thenReturn(domain); + domainManager.deleteDomain(Domain.ROOT_DOMAIN, testDomainCleanup); + } + + @Test + public void testDeleteDomainNoCleanup() { + domainManager.deleteDomain(DOMAIN_ID, testDomainCleanup); + Mockito.verify(domainManager).deleteDomain(domain, testDomainCleanup); + Mockito.verify(domainManager).removeDomainWithNoAccountsForCleanupNetworksOrDedicatedResources(domain); + Mockito.verify(domainManager).cleanupDomainOfferings(DOMAIN_ID); + Mockito.verify(lock).unlock(); + } + + @Test + public void testRemoveDomainWithNoAccountsForCleanupNetworksOrDedicatedResourcesRemoveDomain() { + domainManager.removeDomainWithNoAccountsForCleanupNetworksOrDedicatedResources(domain); + Mockito.verify(domainManager).publishRemoveEventsAndRemoveDomain(domain); + } + + @Test(expected=CloudRuntimeException.class) + public void testRemoveDomainWithNoAccountsForCleanupNetworksOrDedicatedResourcesDontRemoveDomain() { + domainNetworkIds.add(2l); + domainManager.removeDomainWithNoAccountsForCleanupNetworksOrDedicatedResources(domain); + Mockito.verify(domainManager).failRemoveOperation(domain, domainAccountsForCleanup, domainNetworkIds, false); + } + + @Test + public void testPublishRemoveEventsAndRemoveDomainSuccessfulDelete() { + domainManager.publishRemoveEventsAndRemoveDomain(domain); + Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_PRE_REMOVE_DOMAIN_EVENT), + Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain)); + Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_REMOVE_DOMAIN_EVENT), + Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain)); + Mockito.verify(_domainDao).remove(DOMAIN_ID); + } + + @Test(expected=CloudRuntimeException.class) + public void testPublishRemoveEventsAndRemoveDomainExceptionDelete() { + Mockito.when(_domainDao.remove(DOMAIN_ID)).thenReturn(false); + domainManager.publishRemoveEventsAndRemoveDomain(domain); + Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_PRE_REMOVE_DOMAIN_EVENT), + Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain)); + Mockito.verify(_messageBus, Mockito.never()).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_REMOVE_DOMAIN_EVENT), + Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain)); + Mockito.verify(_domainDao).remove(DOMAIN_ID); + } + + @Test(expected=CloudRuntimeException.class) + public void testFailRemoveOperation() { + domainManager.failRemoveOperation(domain, domainAccountsForCleanup, domainNetworkIds, true); + } + }