subject:"\[GitHub\] cloudstack pull request #1935\: CLOUDSTACK\-9764\: Delete domain failure due to..."

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-04-07 Thread nvazquez

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r110421451
  
--- Diff: server/test/com/cloud/user/DomainManagerImplTest.java ---
@@ -134,4 +164,69 @@ public void testFindDomainByIdOrPathValidId() {
 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
+public void testFailRemoveOperation() {
+try {
+domainManager.failRemoveOperation(domain, 
domainAccountsForCleanup, domainNetworkIds, true);
--- End diff --

Great, thanks!


---
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.
---

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-04-07 Thread nvazquez

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r110421376
  
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,82 +284,145 @@ 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 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);
+
+boolean rollBackState = false;
+
+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.";
-}
+
removeDomainWithNoAccountsForCleanupNetworksOrDedicatedResources(domain);
+}
 
-CloudRuntimeException e = new 
CloudRuntimeException("Can't delete the domain yet because it has " + msg);
-e.addProxyObject(domain.getUuid(), "domainId");
-throw e;
+

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-04-04 Thread rafaelweingartner

Github user rafaelweingartner commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r103529571
  
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,82 +284,145 @@ 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 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);
+
+boolean rollBackState = false;
+
+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.";
-}
+
removeDomainWithNoAccountsForCleanupNetworksOrDedicatedResources(domain);
+}
 
-CloudRuntimeException e = new 
CloudRuntimeException("Can't delete the domain yet because it has " + msg);
-e.addProxyObject(domain.getUuid(), "domainId");
-throw e;
+

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-04-04 Thread rafaelweingartner

Github user rafaelweingartner commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r102538324
  
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -109,6 +112,20 @@
 @Inject
 MessageBus _messageBus;
 
+static boolean rollBackState = false;
--- End diff --

@nvazquez I have been thinking about this variable you introduced here. I 
think it can cause problems (concurrency problems). The `DomainManagerImpl` is 
a singleton. Therefore, it should not have state variables. The `rollBackState 
` is acting as a state variable for requests that use 
`com.cloud.user.DomainManagerImpl.deleteDomain(DomainVO, Boolean)`. The problem 
is that every call should have its own context/state for `rollBackState`. 
However, this will not happen with the current implementation.

I think we should re-work the use of that variable. What do you think?


---
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.
---

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-04-04 Thread rafaelweingartner

Github user rafaelweingartner commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r103529869
  
--- Diff: server/test/com/cloud/user/DomainManagerImplTest.java ---
@@ -134,4 +164,69 @@ public void testFindDomainByIdOrPathValidId() {
 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
+public void testFailRemoveOperation() {
+try {
+domainManager.failRemoveOperation(domain, 
domainAccountsForCleanup, domainNetworkIds, true);
--- End diff --

Now that you removed the use of `rollBackState`, made the method last 
problematic to test. Therefore, you can use the `@Test(expected=...)`, instead 
of this very unusual construction here.


---
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.
---

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-22 Thread nvazquez

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r102556528
  
--- Diff: server/test/com/cloud/user/DomainManagerImplTest.java ---
@@ -134,4 +164,67 @@ public void testFindDomainByIdOrPathValidId() {
 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);
+}
+
+//TODO testDeleteDomainCleanup
+
+@Test
+public void testDeleteDomainNoCleanup() {
+domainManager.deleteDomain(DOMAIN_ID, testDomainCleanup);
+Mockito.verify(domainManager).deleteDomain(domain, 
testDomainCleanup);
+
Mockito.verify(domainManager).checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+Mockito.verify(domainManager).cleanupDomainOfferings(DOMAIN_ID);
+Mockito.verify(lock).unlock();
+Assert.assertEquals(false, domainManager.getRollBackState());
+}
+
+@Test
+public void 
testCheckDomainAccountsNetworksAndResourcesBeforeRemovingRemoveDomain() {
+
domainManager.checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+
Mockito.verify(domainManager).publishRemoveEventsAndRemoveDomain(domain);
+}
+
+@Test(expected=CloudRuntimeException.class)
+public void 
testCheckDomainAccountsNetworksAndResourcesBeforeRemovingDontRemoveDomain() {
+domainNetworkIds.add(2l);
+
domainManager.checkDomainAccountsNetworksAndResourcesBeforeRemoving(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() {
--- End diff --

Thanks! I removed `expected` from `@Test` annotation and added a catch 
block to assert exception class. It is a nasty method to test indeed :)


---
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.
---

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-22 Thread rafaelweingartner

Github user rafaelweingartner commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r102536773
  
--- 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 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())) {
+// 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

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-22 Thread rafaelweingartner

Github user rafaelweingartner commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r102535697
  
--- 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 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())) {
+// 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)) {
--- End diff --

Ah, the naming of methods, I also find very hard sometimes; I think most 
programmers have this problem. I think the name you proposed is ok. I am not 
creative today.

I also liked the way the code became cleaner and easier to read. It also 
facilitates future changes.



---
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.
---

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-22 Thread rafaelweingartner

Github user rafaelweingartner commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r102534985
  
--- Diff: server/test/com/cloud/user/DomainManagerImplTest.java ---
@@ -134,4 +164,67 @@ public void testFindDomainByIdOrPathValidId() {
 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);
+}
+
+//TODO testDeleteDomainCleanup
+
+@Test
+public void testDeleteDomainNoCleanup() {
+domainManager.deleteDomain(DOMAIN_ID, testDomainCleanup);
+Mockito.verify(domainManager).deleteDomain(domain, 
testDomainCleanup);
+
Mockito.verify(domainManager).checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+Mockito.verify(domainManager).cleanupDomainOfferings(DOMAIN_ID);
+Mockito.verify(lock).unlock();
+Assert.assertEquals(false, domainManager.getRollBackState());
+}
+
+@Test
+public void 
testCheckDomainAccountsNetworksAndResourcesBeforeRemovingRemoveDomain() {
+
domainManager.checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+
Mockito.verify(domainManager).publishRemoveEventsAndRemoveDomain(domain);
+}
+
+@Test(expected=CloudRuntimeException.class)
+public void 
testCheckDomainAccountsNetworksAndResourcesBeforeRemovingDontRemoveDomain() {
+domainNetworkIds.add(2l);
+
domainManager.checkDomainAccountsNetworksAndResourcesBeforeRemoving(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() {
--- End diff --

Here we still have a problem.
the exception is thrown at line 227 and the code at line 228 is never 
executed. 
That is why I called nasty to write a test here. It is very tricky


---
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.
---

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-22 Thread nvazquez

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r102531132
  
--- 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 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())) {
+// 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

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-22 Thread nvazquez

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r102529801
  
--- 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 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())) {
+// 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)) {
--- End diff --

Done, I liked it how it simplified the code inside those blocks. However, I 
still find it difficult to name methods, do you agree with name 
`tryCleanupDomain`?


---
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.
---

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-22 Thread nvazquez

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r102531047
  
--- Diff: server/test/com/cloud/user/DomainManagerImplTest.java ---
@@ -134,4 +164,67 @@ public void testFindDomainByIdOrPathValidId() {
 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);
+}
+
+//TODO testDeleteDomainCleanup
+
+@Test
+public void testDeleteDomainNoCleanup() {
+domainManager.deleteDomain(DOMAIN_ID, testDomainCleanup);
+Mockito.verify(domainManager).deleteDomain(domain, 
testDomainCleanup);
+
Mockito.verify(domainManager).checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+Mockito.verify(domainManager).cleanupDomainOfferings(DOMAIN_ID);
+Mockito.verify(lock).unlock();
+Assert.assertEquals(false, domainManager.getRollBackState());
+}
+
+@Test
+public void 
testCheckDomainAccountsNetworksAndResourcesBeforeRemovingRemoveDomain() {
+
domainManager.checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+
Mockito.verify(domainManager).publishRemoveEventsAndRemoveDomain(domain);
+}
+
+@Test(expected=CloudRuntimeException.class)
+public void 
testCheckDomainAccountsNetworksAndResourcesBeforeRemovingDontRemoveDomain() {
+domainNetworkIds.add(2l);
+
domainManager.checkDomainAccountsNetworksAndResourcesBeforeRemoving(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() {
--- End diff --

Great, I've added verification, I have missed that out, thanks!


---
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.
---

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-22 Thread nvazquez

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 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())) {
+// 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

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-22 Thread rafaelweingartner

Github user rafaelweingartner commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r102323551
  
--- 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 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())) {
+// 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

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-22 Thread rafaelweingartner

Github user rafaelweingartner commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r102487341
  
--- 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 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())) {
+// 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)) {
--- End diff --

@nvazquez you have already improve greatly this method.
  
How do you feel about another code extraction? lines 312-319 can be easily 
extracted to a method, enabling the reduction of the lines here in these 
try/try/finally/finally blocks. 


---
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.
---

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-22 Thread rafaelweingartner

Github user rafaelweingartner commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r102322603
  
--- 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 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())) {
+// 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

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-22 Thread rafaelweingartner

Github user rafaelweingartner commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r102486318
  
--- Diff: server/test/com/cloud/user/DomainManagerImplTest.java ---
@@ -134,4 +164,67 @@ public void testFindDomainByIdOrPathValidId() {
 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);
+}
+
+//TODO testDeleteDomainCleanup
+
+@Test
+public void testDeleteDomainNoCleanup() {
+domainManager.deleteDomain(DOMAIN_ID, testDomainCleanup);
+Mockito.verify(domainManager).deleteDomain(domain, 
testDomainCleanup);
+
Mockito.verify(domainManager).checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+Mockito.verify(domainManager).cleanupDomainOfferings(DOMAIN_ID);
+Mockito.verify(lock).unlock();
+Assert.assertEquals(false, domainManager.getRollBackState());
+}
+
+@Test
+public void 
testCheckDomainAccountsNetworksAndResourcesBeforeRemovingRemoveDomain() {
+
domainManager.checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+
Mockito.verify(domainManager).publishRemoveEventsAndRemoveDomain(domain);
+}
+
+@Test(expected=CloudRuntimeException.class)
+public void 
testCheckDomainAccountsNetworksAndResourcesBeforeRemovingDontRemoveDomain() {
+domainNetworkIds.add(2l);
+
domainManager.checkDomainAccountsNetworksAndResourcesBeforeRemoving(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() {
--- End diff --

Here is a nasty method to test.
when an exception happens, the variable `rollBackState ` is changed to 
`true`. should not we verify that? 
What do you think about this?


---
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.
---

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-21 Thread nvazquez

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r102264841
  
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,79 +274,97 @@ 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 = GlobalLock.getInternLock("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())) {
+// 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;
+
+try {
+long ownerId = domain.getAccountId();
+if ((cleanup != null) && cleanup.booleanValue()) {
+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.";
+//don't delete the domain if there are accounts set 
for cleanup, or non-removed networks

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-21 Thread nvazquez

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r102265306
  
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,79 +274,97 @@ 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 = GlobalLock.getInternLock("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())) {
+// 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;
+
+try {
+long ownerId = domain.getAccountId();
+if ((cleanup != null) && cleanup.booleanValue()) {
--- End diff --

Done, thanks @rafaelweingartner


---
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.
---

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-21 Thread nvazquez

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r102265150
  
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,79 +274,97 @@ 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 = GlobalLock.getInternLock("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())) {
+// 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;
+
+try {
+long ownerId = domain.getAccountId();
+if ((cleanup != null) && cleanup.booleanValue()) {
+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.";
+//don't delete the domain if there are accounts set 
for cleanup, or non-removed networks

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-10 Thread rafaelweingartner

Github user rafaelweingartner commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r100575787
  
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,79 +274,97 @@ 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 = GlobalLock.getInternLock("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())) {
+// 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;
+
+try {
+long ownerId = domain.getAccountId();
+if ((cleanup != null) && cleanup.booleanValue()) {
+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.";
+//don't delete the domain if there are accounts set 
for cleanup, or non-removed

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-10 Thread rafaelweingartner

Github user rafaelweingartner commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r100574038
  
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,79 +274,97 @@ 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 = GlobalLock.getInternLock("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())) {
+// 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;
+
+try {
+long ownerId = domain.getAccountId();
+if ((cleanup != null) && cleanup.booleanValue()) {
--- End diff --

What about using "org.apache.commons.lang.BooleanUtils.toBoolean(Boolean)" 
here?


---
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.
---

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-10 Thread rafaelweingartner

Github user rafaelweingartner commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r100575967
  
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,79 +274,97 @@ 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 = GlobalLock.getInternLock("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())) {
+// 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;
+
+try {
+long ownerId = domain.getAccountId();
+if ((cleanup != null) && cleanup.booleanValue()) {
+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.";
+//don't delete the domain if there are accounts set 
for cleanup, or non-removed

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-09 Thread koushik-das

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r100478202
  
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,79 +274,97 @@ 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 = GlobalLock.getInternLock("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())) {
+// 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;
+
+try {
+long ownerId = domain.getAccountId();
+if ((cleanup != null) && cleanup.booleanValue()) {
+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.";
+//don't delete the domain if there are accounts set 
for cleanup, or non-removed

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-09 Thread koushik-das

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r100477949
  
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,79 +274,97 @@ 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 = GlobalLock.getInternLock("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())) {
+// 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;
+
+try {
+long ownerId = domain.getAccountId();
+if ((cleanup != null) && cleanup.booleanValue()) {
+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.";
+//don't delete the domain if there are accounts set 
for cleanup, or non-removed

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

2017-02-06 Thread nvazquez

GitHub user nvazquez opened a pull request:

https://github.com/apache/cloudstack/pull/1935

CLOUDSTACK-9764: Delete domain failure due to Account Cleanup task

It was noticed in production environments that `deleteDomain` task failed 
for domains with multiple accounts and resources. Examining logs it was found 
out that if Account Cleanup Task got executed after domain (and all of its 
subchilds) got marked as Inactive; and before delete domain task finishes, it 
produces a failure.

`AccountCleanupTask` gets executed every `account.cleanup.interval` seconds 
looking for:
* Removed accounts
* Disabled accounts
* Inactive domains

As `deleteDomain` marks domain to delete (and its subchilds) as Inactive 
before deleting them, when `AccountCleanupTask` is executed, it removes marked 
domains. When there are resources to cleanup on domain accounts, domain is not 
found throwing exception: `com.cloud.exception.InvalidParameterValueException: 
Please specify a valid domain ID`

### Example
`account.cleanup.interval` = 100


2017-01-26 06:07:03,621 DEBUG [cloud.api.ApiServlet] 
(catalina-exec-8:ctx-50cfa3b6 ctx-92ad5b38) ===END===  10.39.251.17 -- GET  
command=deleteDomain=1910a3dc-6fa6-457b-ab3a-602b0cfb6686=true=json&_=1485439623475

...

// Domain and its subchilds marked as Inactive
2017-01-26 06:07:03,640 DEBUG [cloud.user.DomainManagerImpl] 
(API-Job-Executor-29:ctx-23415942 job-7165 ctx-fe3d13d6) Marking domain id=27 
as Inactive before actually deleting it
2017-01-26 06:07:03,646 DEBUG [cloud.user.DomainManagerImpl] 
(API-Job-Executor-29:ctx-23415942 job-7165 ctx-fe3d13d6) Cleaning up domain 
id=27
2017-01-26 06:07:03,670 DEBUG [cloud.user.DomainManagerImpl] 
(API-Job-Executor-29:ctx-23415942 job-7165 ctx-fe3d13d6) Cleaning up domain 
id=28
2017-01-26 06:07:03,685 DEBUG [cloud.user.DomainManagerImpl] 
(API-Job-Executor-29:ctx-23415942 job-7165 ctx-fe3d13d6) Cleaning up domain 
id=29

...

// AccountCleanupTask removes Inactive domain id=29, no rollback for it
2017-01-26 06:07:44,285 INFO  [cloud.user.AccountManagerImpl] 
(AccountChecker-1:ctx-b8a01824) Found 0 removed accounts to cleanup
2017-01-26 06:07:44,287 INFO  [cloud.user.AccountManagerImpl] 
(AccountChecker-1:ctx-b8a01824) Found 0 disabled accounts to cleanup
2017-01-26 06:07:44,289 INFO  [cloud.user.AccountManagerImpl] 
(AccountChecker-1:ctx-b8a01824) Found 3 inactive domains to cleanup
2017-01-26 06:07:44,292 DEBUG [cloud.user.AccountManagerImpl] 
(AccountChecker-1:ctx-b8a01824) Removing inactive domain id=27
2017-01-26 06:07:44,297 DEBUG [db.Transaction.Transaction] 
(AccountChecker-1:ctx-b8a01824) Rolling back the transaction: Time = 2 Name =  
AccountChecker-1; called by 
-TransactionLegacy.rollback:889-TransactionLegacy.removeUpTo:832-TransactionLegacy.close:656-TransactionContextInterceptor.invoke:36-ReflectiveMethodInvocation.proceed:161-ExposeInvocationInterceptor.invoke:91-ReflectiveMethodInvocation.proceed:172-JdkDynamicAopProxy.invoke:204-$Proxy63.remove:-1-DomainManagerImpl.removeDomain:248-NativeMethodAccessorImpl.invoke0:-2-NativeMethodAccessorImpl.invoke:62
2017-01-26 06:07:44,301 DEBUG [cloud.user.AccountManagerImpl] 
(AccountChecker-1:ctx-b8a01824) Removing inactive domain id=28
2017-01-26 06:07:44,304 DEBUG [db.Transaction.Transaction] 
(AccountChecker-1:ctx-b8a01824) Rolling back the transaction: Time = 2 Name =  
AccountChecker-1; called by 
-TransactionLegacy.rollback:889-TransactionLegacy.removeUpTo:832-TransactionLegacy.close:656-TransactionContextInterceptor.invoke:36-ReflectiveMethodInvocation.proceed:161-ExposeInvocationInterceptor.invoke:91-ReflectiveMethodInvocation.proceed:172-JdkDynamicAopProxy.invoke:204-$Proxy63.remove:-1-DomainManagerImpl.removeDomain:248-NativeMethodAccessorImpl.invoke0:-2-NativeMethodAccessorImpl.invoke:62
2017-01-26 06:07:44,307 DEBUG [cloud.user.AccountManagerImpl] 
(AccountChecker-1:ctx-b8a01824) Removing inactive domain id=29
2017-01-26 06:07:44,319 INFO  [cloud.user.AccountManagerImpl] 
(AccountChecker-1:ctx-b8a01824) Found 0 disabled projects to cleanup

...

// Failure due to domain is already removed
2017-01-26 06:07:46,369 WARN  [cloud.user.AccountManagerImpl] 
(API-Job-Executor-29:ctx-23415942 job-7165 ctx-fe3d13d6) Failed to cleanup 
account Acct[6a6e63ad-d89b-4a53-b3ae-1c06ea3d1899-ac2] due to 
com.cloud.exception.InvalidParameterValueException: Please specify a valid 
domain ID.
at 
com.cloud.resourcelimit.ResourceLimitManagerImpl.recalculateResourceCount(ResourceLimitManagerImpl.java:752)
at com.cloud.vm.UserVmManagerImpl.expunge(UserVmManagerImpl.java:2053)
...
2017-01-26 06:07:46,381 INFO  [cloud.user.AccountManagerImpl] 
(API-Job-Executor-29:ctx-23415942 job-7165 ctx-fe3d13d6) Cleanup for account 
2580 is needed.
2017-01-26 06:07:46,390 DEBUG

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

26 matches

Site Navigation

Mail list logo

Footer information