Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r102530895
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,82 +289,133 @@ public boolean deleteDomain(long domainId, Boolean
cleanup) {
@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<Long> networkIds =
_networkDomainDao.listNetworkIdsByDomain(domain.getId());
- List<AccountVO> accountsForCleanup =
_accountDao.findCleanupsForRemovedAccounts(domain.getId());
- List<DedicatedResourceVO> 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())) {
+ // 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)) {
+ if (!cleanupDomain(domain.getId(), ownerId)) {
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");
+ 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;
}
- _messageBus.publish(_name,
MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
} 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.";
- }
+
checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+ }
- CloudRuntimeException e = new
CloudRuntimeException("Can't delete the domain yet because it has " + msg);
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
+ 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);
}
}
+ }
+ finally {
+ lock.unlock();
+ rollBackState = false;
+ }
+ }
- 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);
- }
+ /**
+ * Check domain resources before removing domain. There are 2 cases:
+ * <ol>
+ * <li>Domain doesn't have accounts for cleanup, non-removed networks,
or dedicated resources</li>
+ * <ul><li>Delete domain</li></ul>
+ * <li>Domain has one of the following: accounts set for cleanup,
non-removed networks, dedicated resources</li>
+ * <ul><li>Dont' delete domain</li><li>Fail operation</li></ul>
+ * </ol>
+ * @param domain domain to remove
+ * @throws CloudRuntimeException when case 2 or when domain cannot be
deleted on case 1
+ */
+ protected void
checkDomainAccountsNetworksAndResourcesBeforeRemoving(DomainVO domain) {
--- End diff --
I agree with you. I changed it to
`removeDomainWithNoAccountsForCleanupNetworksOrDedicatedResources`. I think is
quite more descriptive but still long
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
