Its correct that there is the possibility that resource of these accounts might not be removed; but if we take the same scenario with deleteAccount API, where we always return true as success response irrespective of the resources (except project) are still attached to this account or not. If the resources are still attached to the account, CS marks the cleanupRequired field in DB as true and cleanup thread takes care of it in the next run.
So if we are allowing the accounts to get deleted then why are we checking for accounts needs cleanup in deleteDomain API (though accounts are set as removed in this case also). --Sanjay From: Alena Prokharchyk Sent: Wednesday, July 24, 2013 11:09 PM To: dev@cloudstack.apache.org; Sanjay Tripathi; Prasanna Santhanam; Devdeep Singh Cc: cloudstack Subject: Re: Review Request 12853: CLOUDSTACK-3688: Test cases test_accounts.TestDomainForceRemove.test_forceDeleteDomain, not found the domain to delete and failed. The resources of those accounts might not be removed yet. Removing the domain while these resources are still present, will introduce tons of NPEs when we try to access the domain info for them. So removing the domain should be done only after all the accounts are removed. -Alena. From: Sanjay Tripathi <sanjay.tripa...@citrix.com<mailto:sanjay.tripa...@citrix.com>> Reply-To: "dev@cloudstack.apache.org<mailto:dev@cloudstack.apache.org>" <dev@cloudstack.apache.org<mailto:dev@cloudstack.apache.org>>, Sanjay Tripathi <sanjay.tripa...@citrix.com<mailto:sanjay.tripa...@citrix.com>> Date: Wednesday, July 24, 2013 10:18 AM To: Prasanna Santhanam <t...@apache.org<mailto:t...@apache.org>>, Devdeep Singh <devdeep.si...@citrix.com<mailto:devdeep.si...@citrix.com>>, Alena Prokharchyk <alena.prokharc...@citrix.com<mailto:alena.prokharc...@citrix.com>> Cc: "cloudstack-...@incubator.apache.org<mailto:cloudstack-...@incubator.apache.org>" <cloudstack-...@incubator.apache.org<mailto:cloudstack-...@incubator.apache.org>>, Sanjay Tripathi <sanjay.tripa...@citrix.com<mailto:sanjay.tripa...@citrix.com>> Subject: Re: Review Request 12853: CLOUDSTACK-3688: Test cases test_accounts.TestDomainForceRemove.test_forceDeleteDomain, not found the domain to delete and failed. On July 24, 2013, 5:02 p.m., Alena Prokharchyk wrote: > There is no bug in current Java code, and the java part of this review ticket > shouldn't be checked in. > > > In the method cleanupDomain(), we try to remove every account before checking > if accounts marked for cleanup, exist. > > // delete users which will also delete accounts and release resources for > those accounts > SearchCriteria<AccountVO> sc = _accountDao.createSearchCriteria(); > sc.addAnd("domainId", SearchCriteria.Op.EQ, domainId); > List<AccountVO> accounts = _accountDao.search(sc, null); > for (AccountVO account : accounts) { > if (account.getType() != Account.ACCOUNT_TYPE_PROJECT) { > s_logger.debug("Deleting account " + account + " as a part of > domain id=" + domainId + " cleanup"); > boolean deleteAccount = _accountMgr.deleteAccount(account, > UserContext.current().getCallerUserId(), UserContext.current().getCaller()); > if (!deleteAccount) { > s_logger.warn("Failed to cleanup account id=" + > account.getId() + " as a part of domain cleanup"); > } > > > Only after that we verify if there are accounts left. And only if the code > above failed, the code below returns non-empty list: > > // don't remove the domain if there are accounts required cleanup > boolean deleteDomainSuccess = true; > List<AccountVO> accountsForCleanup = > _accountDao.findCleanupsForRemovedAccounts(domainId); > > So it works as expected. > But the accounts which are marked as cleanup=true are set as removed and admin can't list them by using API call. So, the only thing admin can see is the domain without any accounts. Please clarify if that makes sense. - Sanjay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12853/#review23771 ----------------------------------------------------------- On July 23, 2013, 12:34 p.m., Sanjay Tripathi wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12853/ ----------------------------------------------------------- (Updated July 23, 2013, 12:34 p.m.) Review request for cloudstack, Alena Prokharchyk, Devdeep Singh, and Prasanna Santhanam. Bugs: CLOUDSTACK-3688 Repository: cloudstack-git Description ------- CLOUDSTACK-3688: Test cases test_accounts.TestDomainForceRemove.test_forceDeleteDomain, not found the domain to delete and failed. This issue is in CS product and not in the test_script. The problem is coming because in case of deleteDomain with cleanup = true, CS is not allowing deletion of domain if there are any account under that domain needs clean up; though these accounts are removed and admin can't see them in the listaccounts. So CS should not restrict the deleteDomain in case of accounts needs cleanup. Diffs ----- server/src/com/cloud/user/DomainManagerImpl.java 1117ff0 test/integration/component/test_accounts.py 3c284bd Diff: https://reviews.apache.org/r/12853/diff/ Testing ------- Verified marvin test on my local cloudstack setup. Thanks, Sanjay Tripathi
