From 21afc889d03b3902496ba992fe157966230b7144 Mon Sep 17 00:00:00 2001 From: alena Date: Mon, 11 Jul 2011 17:45:50 -0700 Subject: [PATCH] bug 10438: always return success on disableAccount when it got disabled successfully in the DB. If his vms failed to stop on the backend, mark account for cleanup and let background thread to do the cleanup job status 10438: resolved fixed --- .../network/element/VirtualRouterElement.java | 1 - .../cloud/server/ManagementServerImpl.java | 44 ------------------- .../com/cloud/user/AccountManagerImpl.java | 36 ++++++++++++--- server/src/com/cloud/user/dao/AccountDao.java | 5 ++- .../com/cloud/user/dao/AccountDaoImpl.java | 34 +++++++++----- 5 files changed, 57 insertions(+), 63 deletions(-) diff --git a/server/src/com/cloud/network/element/VirtualRouterElement.java b/server/src/com/cloud/network/element/VirtualRouterElement.java index 3fe46b969f2..dad075ece1c 100644 --- a/server/src/com/cloud/network/element/VirtualRouterElement.java +++ b/server/src/com/cloud/network/element/VirtualRouterElement.java @@ -55,7 +55,6 @@ import com.cloud.vm.NicProfile; import com.cloud.vm.ReservationContext; import com.cloud.vm.UserVmManager; import com.cloud.vm.VirtualMachine; -import com.cloud.vm.VirtualMachine.State; import com.cloud.vm.VirtualMachineProfile; import com.cloud.vm.dao.DomainRouterDao; import com.cloud.vm.dao.UserVmDao; diff --git a/server/src/com/cloud/server/ManagementServerImpl.java b/server/src/com/cloud/server/ManagementServerImpl.java index c8af79ff956..cf681c02881 100755 --- a/server/src/com/cloud/server/ManagementServerImpl.java +++ b/server/src/com/cloud/server/ManagementServerImpl.java @@ -3640,50 +3640,6 @@ public class ManagementServerImpl implements ManagementServer { return new String[] { "commands.properties" }; } - protected class AccountCleanupTask implements Runnable { - @Override - public void run() { - try { - GlobalLock lock = GlobalLock.getInternLock("AccountCleanup"); - if (lock == null) { - s_logger.debug("Couldn't get the global lock"); - return; - } - - if (!lock.lock(30)) { - s_logger.debug("Couldn't lock the db"); - return; - } - - Transaction txn = null; - try { - txn = Transaction.open(Transaction.CLOUD_DB); - - List accounts = _accountDao.findCleanups(); - s_logger.info("Found " + accounts.size() + " accounts to cleanup"); - for (AccountVO account : accounts) { - s_logger.debug("Cleaning up " + account.getId()); - try { - _accountMgr.cleanupAccount(account, _accountMgr.getSystemUser().getId(), _accountMgr.getSystemAccount()); - } catch (Exception e) { - s_logger.error("Skipping due to error on account " + account.getId(), e); - } - } - } catch (Exception e) { - s_logger.error("Exception ", e); - } finally { - if (txn != null) { - txn.close(); - } - - lock.unlock(); - } - } catch (Exception e) { - s_logger.error("Exception ", e); - } - } - } - protected class EventPurgeTask implements Runnable { @Override public void run() { diff --git a/server/src/com/cloud/user/AccountManagerImpl.java b/server/src/com/cloud/user/AccountManagerImpl.java index 8975b6eff73..7dc33f047af 100755 --- a/server/src/com/cloud/user/AccountManagerImpl.java +++ b/server/src/com/cloud/user/AccountManagerImpl.java @@ -976,6 +976,7 @@ public class AccountManagerImpl implements AccountManager, AccountService, Manag boolean success = false; AccountVO acctForUpdate = _accountDao.createForUpdate(); acctForUpdate.setState(State.enabled); + acctForUpdate.setNeedsCleanup(false); success = _accountDao.update(Long.valueOf(accountId), acctForUpdate); return success; } @@ -1148,14 +1149,19 @@ public class AccountManagerImpl implements AccountManager, AccountService, Manag } AccountVO account = _accountDao.findById(accountId); - if ((account == null) || account.getState().equals(State.disabled)) { + if ((account == null) || (account.getState().equals(State.disabled) && !account.getNeedsCleanup())) { success = true; } else { AccountVO acctForUpdate = _accountDao.createForUpdate(); acctForUpdate.setState(State.disabled); success = _accountDao.update(Long.valueOf(accountId), acctForUpdate); - - success = (success && doDisableAccount(accountId)); + + if (success) { + if (!doDisableAccount(accountId)) { + s_logger.warn("Failed to disable account " + account + " resources as a part of disableAccount call, marking the account for cleanup"); + _accountDao.markForCleanup(accountId); + } + } } return success; } @@ -1776,9 +1782,10 @@ public class AccountManagerImpl implements AccountManager, AccountService, Manag try { txn = Transaction.open(Transaction.CLOUD_DB); - List accounts = _accountDao.findCleanups(); - s_logger.info("Found " + accounts.size() + " accounts to cleanup"); - for (AccountVO account : accounts) { + //Cleanup removed accounts + List removedAccounts = _accountDao.findCleanupsForRemovedAccounts(); + s_logger.info("Found " + removedAccounts.size() + " removed accounts to cleanup"); + for (AccountVO account : removedAccounts) { s_logger.debug("Cleaning up " + account.getId()); try { cleanupAccount(account, getSystemUser().getId(), getSystemAccount()); @@ -1786,6 +1793,23 @@ public class AccountManagerImpl implements AccountManager, AccountService, Manag s_logger.error("Skipping due to error on account " + account.getId(), e); } } + + //cleanup disabled accounts + List disabledAccounts = _accountDao.findCleanupsForDisabledAccounts(); + s_logger.info("Found " + disabledAccounts.size() + " disabled accounts to cleanup"); + for (AccountVO account : disabledAccounts) { + s_logger.debug("Cleaning up " + account.getId()); + try { + if (disableAccount(account.getId())) { + account.setNeedsCleanup(false); + _accountDao.update(account.getId(), account); + } + } catch (Exception e) { + s_logger.error("Skipping due to error on account " + account.getId(), e); + } + } + + } catch (Exception e) { s_logger.error("Exception ", e); } finally { diff --git a/server/src/com/cloud/user/dao/AccountDao.java b/server/src/com/cloud/user/dao/AccountDao.java index 9969bba19c0..2ff13f983ff 100644 --- a/server/src/com/cloud/user/dao/AccountDao.java +++ b/server/src/com/cloud/user/dao/AccountDao.java @@ -37,9 +37,10 @@ public interface AccountDao extends GenericDao { List findActiveAccounts(Long maxAccountId, Filter filter); List findRecentlyDeletedAccounts(Long maxAccountId, Date earliestRemovedDate, Filter filter); List findNewAccounts(Long minAccountId, Filter filter); - List findCleanups(); + List findCleanupsForRemovedAccounts(); List findAdminAccountsForDomain(Long domainId); List findActiveAccountsForDomain(Long domain); void markForCleanup(long accountId); - List listAccounts(String accountName, Long domainId, Filter filter); + List listAccounts(String accountName, Long domainId, Filter filter); + List findCleanupsForDisabledAccounts(); } diff --git a/server/src/com/cloud/user/dao/AccountDaoImpl.java b/server/src/com/cloud/user/dao/AccountDaoImpl.java index ad34a188dd0..1d000b9b92a 100755 --- a/server/src/com/cloud/user/dao/AccountDaoImpl.java +++ b/server/src/com/cloud/user/dao/AccountDaoImpl.java @@ -50,9 +50,9 @@ public class AccountDaoImpl extends GenericDaoBase implements A protected final SearchBuilder AccountNameSearch; protected final SearchBuilder AccountTypeSearch; - protected final SearchBuilder DomainAccountsSearch; - - protected final SearchBuilder CleanupSearch; + protected final SearchBuilder DomainAccountsSearch; + protected final SearchBuilder CleanupForRemovedAccountsSearch; + protected final SearchBuilder CleanupForDisabledAccountsSearch; protected AccountDaoImpl() { AccountNameSearch = createSearchBuilder(); @@ -69,19 +69,33 @@ public class AccountDaoImpl extends GenericDaoBase implements A DomainAccountsSearch.and("removed", DomainAccountsSearch.entity().getRemoved(), SearchCriteria.Op.NULL); DomainAccountsSearch.done(); - CleanupSearch = createSearchBuilder(); - CleanupSearch.and("cleanup", CleanupSearch.entity().getNeedsCleanup(), SearchCriteria.Op.EQ); - CleanupSearch.and("removed", CleanupSearch.entity().getRemoved(), SearchCriteria.Op.NNULL); - CleanupSearch.done(); - + CleanupForRemovedAccountsSearch = createSearchBuilder(); + CleanupForRemovedAccountsSearch.and("cleanup", CleanupForRemovedAccountsSearch.entity().getNeedsCleanup(), SearchCriteria.Op.EQ); + CleanupForRemovedAccountsSearch.and("removed", CleanupForRemovedAccountsSearch.entity().getRemoved(), SearchCriteria.Op.NNULL); + CleanupForRemovedAccountsSearch.done(); + + CleanupForDisabledAccountsSearch = createSearchBuilder(); + CleanupForDisabledAccountsSearch.and("cleanup", CleanupForDisabledAccountsSearch.entity().getNeedsCleanup(), SearchCriteria.Op.EQ); + CleanupForDisabledAccountsSearch.and("removed", CleanupForDisabledAccountsSearch.entity().getRemoved(), SearchCriteria.Op.NULL); + CleanupForDisabledAccountsSearch.and("state", CleanupForDisabledAccountsSearch.entity().getState(), SearchCriteria.Op.EQ); + CleanupForDisabledAccountsSearch.done(); } @Override - public List findCleanups() { - SearchCriteria sc = CleanupSearch.create(); + public List findCleanupsForRemovedAccounts() { + SearchCriteria sc = CleanupForRemovedAccountsSearch.create(); sc.setParameters("cleanup", true); return searchIncludingRemoved(sc, null, null, false); + } + + @Override + public List findCleanupsForDisabledAccounts() { + SearchCriteria sc = CleanupForDisabledAccountsSearch.create(); + sc.setParameters("cleanup", true); + sc.setParameters("state", State.disabled); + + return listBy(sc); } @Override