From 46a80a599a9237dfb66738cf7934e068bd12e626 Mon Sep 17 00:00:00 2001 From: alena Date: Tue, 29 Mar 2011 16:42:25 -0700 Subject: [PATCH] bug 9192: multiple improvementes to listVms command. 1) No longer do multiple searches involving "domain" table; only one join with domain is being done. 2) Do join with domain table only when command is executed by domainAdmin 3) Added index for "path" field in "domain" table 4) No longer do joins with account table as account_id is already present in vm_instance table. --- .../com/cloud/api/commands/ListVMsCmd.java | 2 +- server/src/com/cloud/acl/DomainChecker.java | 6 +- server/src/com/cloud/api/ApiDBUtils.java | 2 +- .../src/com/cloud/api/ApiResponseHelper.java | 9 +- .../src/com/cloud/vm/UserVmManagerImpl.java | 184 +++++++----------- setup/db/create-schema.sql | 3 +- setup/db/schema-222to224.sql | 5 +- 7 files changed, 84 insertions(+), 127 deletions(-) diff --git a/api/src/com/cloud/api/commands/ListVMsCmd.java b/api/src/com/cloud/api/commands/ListVMsCmd.java index f6351361b29..ca96b338ae0 100755 --- a/api/src/com/cloud/api/commands/ListVMsCmd.java +++ b/api/src/com/cloud/api/commands/ListVMsCmd.java @@ -47,7 +47,7 @@ public class ListVMsCmd extends BaseListCmd { @Parameter(name=ApiConstants.DOMAIN_ID, type=CommandType.LONG, description="the domain ID. If used with the account parameter, lists virtual machines for the specified account in this domain.") private Long domainId; - @Parameter(name=ApiConstants.IS_RECURSIVE, type=CommandType.BOOLEAN, description="defaults to false, but if true, lists all vms from the parent specified by the domain id till leaves.") + @Parameter(name=ApiConstants.IS_RECURSIVE, type=CommandType.BOOLEAN, description="Must be used with domainId parameter. Defaults to false, but if true, lists all vms from the parent specified by the domain id till leaves.") private Boolean recursive; @Parameter(name=ApiConstants.GROUP_ID, type=CommandType.LONG, description="the group ID") diff --git a/server/src/com/cloud/acl/DomainChecker.java b/server/src/com/cloud/acl/DomainChecker.java index 720e44a226d..ee98e80b3ce 100755 --- a/server/src/com/cloud/acl/DomainChecker.java +++ b/server/src/com/cloud/acl/DomainChecker.java @@ -53,7 +53,11 @@ public class DomainChecker extends AdapterBase implements SecurityChecker { throw new PermissionDeniedException(account + " is disabled."); } - if (!_domainDao.isChildDomain(account.getDomainId(), domain.getId())) { + if (account.getType() == Account.ACCOUNT_TYPE_NORMAL) { + if (account.getDomainId() != domain.getId()) { + throw new PermissionDeniedException(account + " does not have permission to operate within " + domain); + } + } else if (!_domainDao.isChildDomain(account.getDomainId(), domain.getId())) { throw new PermissionDeniedException(account + " does not have permission to operate within " + domain); } diff --git a/server/src/com/cloud/api/ApiDBUtils.java b/server/src/com/cloud/api/ApiDBUtils.java index 26446a73360..571226d8040 100755 --- a/server/src/com/cloud/api/ApiDBUtils.java +++ b/server/src/com/cloud/api/ApiDBUtils.java @@ -299,7 +299,7 @@ public class ApiDBUtils { ///////////////////////////////////////////////////////////// public static Account findAccountById(Long accountId) { - return _accountDao.findById(accountId); + return _accountDao.findByIdIncludingRemoved(accountId); } public static Account findAccountByIdIncludingRemoved(Long accountId) { diff --git a/server/src/com/cloud/api/ApiResponseHelper.java b/server/src/com/cloud/api/ApiResponseHelper.java index 7a0d931b18b..6507d3870b4 100755 --- a/server/src/com/cloud/api/ApiResponseHelper.java +++ b/server/src/com/cloud/api/ApiResponseHelper.java @@ -995,17 +995,10 @@ public class ApiResponseHelper implements ResponseGenerator { public UserVmResponse createUserVmResponse(UserVm userVm) { UserVmResponse userVmResponse = new UserVmResponse(); Account acct = ApiDBUtils.findAccountById(Long.valueOf(userVm.getAccountId())); - // FIXME - this check should be done in searchForUserVm method in - // ManagementServerImpl; - // otherwise the number of vms returned is not going to match pageSize - // request parameter - if ((acct != null) && (acct.getRemoved() == null)) { + if (acct != null) { userVmResponse.setAccountName(acct.getAccountName()); userVmResponse.setDomainId(acct.getDomainId()); userVmResponse.setDomainName(ApiDBUtils.findDomainById(acct.getDomainId()).getName()); - } else { - return null; // the account has been deleted, skip this VM in the - // response } userVmResponse.setId(userVm.getId()); diff --git a/server/src/com/cloud/vm/UserVmManagerImpl.java b/server/src/com/cloud/vm/UserVmManagerImpl.java index 2198d4dad09..266c29cda2f 100755 --- a/server/src/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/com/cloud/vm/UserVmManagerImpl.java @@ -2621,58 +2621,57 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager @Override public List searchForUserVMs(ListVMsCmd cmd) throws InvalidParameterValueException, PermissionDeniedException { - Account account = UserContext.current().getCaller(); + Account caller = UserContext.current().getCaller(); Long domainId = cmd.getDomainId(); String accountName = cmd.getAccountName(); - Long accountId = null; Boolean isRecursive = cmd.isRecursive(); String hypervisor = cmd.getHypervisor(); - List domainsToSearchForVms = new ArrayList(); - boolean isAdmin = false; + Long accountId = null; String path = null; - if ((account == null) || isAdmin(account.getType())) { - isAdmin = true; - if (domainId != null) { - if ((account != null) && !_domainDao.isChildDomain(account.getDomainId(), domainId)) { - throw new PermissionDeniedException("Invalid domain id (" + domainId + ") given, unable to list virtual machines."); - } - - if (accountName != null) { - account = _accountDao.findActiveAccount(accountName, domainId); - if (account == null) { - throw new InvalidParameterValueException("Unable to find account " + accountName + " in domain " + domainId); - } - accountId = account.getId(); - } - } - if (account.getType() == Account.ACCOUNT_TYPE_DOMAIN_ADMIN) { - DomainVO domain = _domainDao.findById(account.getDomainId()); - if (domain != null) { - path = domain.getPath(); - } - } - } else { - accountId = account.getId(); - } - - if(isRecursive == null) { - isRecursive = false; - } - if(isRecursive && domainId != null) { - DomainVO parentDomain = _domainDao.findById(domainId); - if(parentDomain.getName().equals("ROOT")) { - domainsToSearchForVms.addAll(_domainDao.listAll()); - return recursivelySearchForVms(cmd, path, isAdmin, domainsToSearchForVms, accountId); - }else { - domainsToSearchForVms.add(parentDomain); - domainsToSearchForVms.addAll(_domainDao.findAllChildren(parentDomain.getPath(), parentDomain.getId())); - return recursivelySearchForVms(cmd, path, isAdmin, domainsToSearchForVms, accountId); - } - } else if(isRecursive && domainId == null){ + if (isRecursive != null && isRecursive && domainId == null){ throw new InvalidParameterValueException("Please enter a parent domain id for listing vms recursively"); } - + + if (domainId != null) { + //Verify if user is authorized to see instances belonging to the domain + DomainVO domain = _domainDao.findById(domainId); + if (domain == null) { + throw new InvalidParameterValueException("Domain id=" + domainId + " doesn't exist"); + } + _accountMgr.checkAccess(caller, domain); + } + + boolean isAdmin = false; + + if (_accountMgr.isAdmin(caller.getType())) { + isAdmin = true; + if (accountName != null && domainId != null) { + caller = _accountDao.findActiveAccount(accountName, domainId); + if (caller == null) { + throw new InvalidParameterValueException("Unable to find account " + accountName + " in domain " + domainId); + } + accountId = caller.getId(); + } + + if (caller.getType() == Account.ACCOUNT_TYPE_DOMAIN_ADMIN) { + if (isRecursive == null) { + DomainVO domain = _domainDao.findById(caller.getDomainId()); + path = domain.getPath(); + } + } + } else { + accountId = caller.getId(); + } + + if (isRecursive != null && isRecursive && isAdmin) { + if (isRecursive) { + DomainVO domain = _domainDao.findById(domainId); + path = domain.getPath(); + domainId = null; + } + } + Criteria c = new Criteria("id", Boolean.TRUE, cmd.getStartIndex(), cmd.getPageSizeVal()); c.addCriteria(Criteria.KEYWORD, cmd.getKeyword()); c.addCriteria(Criteria.ID, cmd.getId()); @@ -2683,19 +2682,22 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager c.addCriteria(Criteria.FOR_VIRTUAL_NETWORK, cmd.getForVirtualNetwork()); c.addCriteria(Criteria.NETWORKID, cmd.getNetworkId()); + if (domainId != null) { + c.addCriteria(Criteria.DOMAINID, domainId); + } + if (path != null) { c.addCriteria(Criteria.PATH, path); } if (HypervisorType.getType(hypervisor) != HypervisorType.None){ c.addCriteria(Criteria.HYPERVISOR, hypervisor); - }else if (hypervisor != null){ + } else if (hypervisor != null){ throw new InvalidParameterValueException("Invalid HypervisorType " + hypervisor); } // ignore these search requests if it's not an admin - if (isAdmin == true) { - c.addCriteria(Criteria.DOMAINID, domainId); + if (isAdmin) { c.addCriteria(Criteria.PODID, cmd.getPodId()); c.addCriteria(Criteria.HOSTID, cmd.getHostId()); c.addCriteria(Criteria.STORAGE_ID, cmd.getStorageId()); @@ -2709,60 +2711,14 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager return searchForUserVMs(c); } - private List recursivelySearchForVms(ListVMsCmd cmd, String path, boolean isAdmin, List domainToSearchWithin, Long accountId) { - - List result = new ArrayList(); - String hypervisor = cmd.getHypervisor(); - for(DomainVO domain : domainToSearchWithin) { - - Criteria c = new Criteria("id", Boolean.TRUE, cmd.getStartIndex(), cmd.getPageSizeVal()); - c.addCriteria(Criteria.KEYWORD, cmd.getKeyword()); - c.addCriteria(Criteria.ID, cmd.getId()); - c.addCriteria(Criteria.NAME, cmd.getInstanceName()); - c.addCriteria(Criteria.STATE, cmd.getState()); - c.addCriteria(Criteria.DATACENTERID, cmd.getZoneId()); - c.addCriteria(Criteria.GROUPID, cmd.getGroupId()); - c.addCriteria(Criteria.FOR_VIRTUAL_NETWORK, cmd.getForVirtualNetwork()); - c.addCriteria(Criteria.NETWORKID, cmd.getNetworkId()); - - if (path != null) { - c.addCriteria(Criteria.PATH, path); - } - - // ignore these search requests if it's not an admin - if (isAdmin == true) { - c.addCriteria(Criteria.DOMAINID, domain.getId()); - c.addCriteria(Criteria.PODID, cmd.getPodId()); - c.addCriteria(Criteria.HOSTID, cmd.getHostId()); - } - - if (HypervisorType.getType(hypervisor) != HypervisorType.None){ - c.addCriteria(Criteria.HYPERVISOR, hypervisor); - }else if (hypervisor != null){ - throw new InvalidParameterValueException("Invalid HypervisorType " + hypervisor); - } - - if (accountId != null) { - c.addCriteria(Criteria.ACCOUNTID, new Object[] {accountId}); - } - c.addCriteria(Criteria.ISADMIN, isAdmin); - - result.addAll(searchForUserVMs(c)); - } - return result; - } @Override public List searchForUserVMs(Criteria c) { Filter searchFilter = new Filter(UserVmVO.class, c.getOrderBy(), c.getAscending(), c.getOffset(), c.getLimit()); SearchBuilder sb = _vmDao.createSearchBuilder(); - - // some criteria matter for generating the join condition Object[] accountIds = (Object[]) c.getCriteria(Criteria.ACCOUNTID); Object domainId = c.getCriteria(Criteria.DOMAINID); - - // get the rest of the criteria Object id = c.getCriteria(Criteria.ID); Object name = c.getCriteria(Criteria.NAME); Object state = c.getCriteria(Criteria.STATE); @@ -2793,21 +2749,14 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager sb.and("hypervisorType", sb.entity().getHypervisorType(), SearchCriteria.Op.EQ); sb.and("hostIdEQ", sb.entity().getHostId(), SearchCriteria.Op.EQ); sb.and("hostIdIN", sb.entity().getHostId(), SearchCriteria.Op.IN); + sb.and("domainId", sb.entity().getDomainId(), SearchCriteria.Op.EQ); - if (domainId != null || path != null) { - // if accountId isn't specified, we can do a domain match for the admin case + if (path != null) { SearchBuilder domainSearch = _domainDao.createSearchBuilder(); - domainSearch.and("id", domainSearch.entity().getId(), SearchCriteria.Op.EQ); domainSearch.and("path", domainSearch.entity().getPath(), SearchCriteria.Op.LIKE); sb.join("domainSearch", domainSearch, sb.entity().getDomainId(), domainSearch.entity().getId(), JoinBuilder.JoinType.INNER); } - if (storageId != null) { - SearchBuilder volumeSearch = _volsDao.createSearchBuilder(); - volumeSearch.and("poolId", volumeSearch.entity().getPoolId(), SearchCriteria.Op.EQ); - sb.join("volumeSearch", volumeSearch, sb.entity().getId(), volumeSearch.entity().getInstanceId(), JoinBuilder.JoinType.INNER); - } - if (groupId != null && (Long)groupId == -1) { SearchBuilder vmSearch = _groupVMMapDao.createSearchBuilder(); vmSearch.and("instanceId", vmSearch.entity().getInstanceId(), SearchCriteria.Op.EQ); @@ -2829,13 +2778,14 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager sb.join("nicSearch", nicSearch, sb.entity().getId(), nicSearch.entity().getInstanceId(), JoinBuilder.JoinType.INNER); } - SearchBuilder accountRemoved = _accountDao.createSearchBuilder(); - accountRemoved.and("accountremoved", accountRemoved.entity().getRemoved(), SearchCriteria.Op.NULL); - sb.join("accountRemoved", accountRemoved, sb.entity().getAccountId(), accountRemoved.entity().getId(), JoinBuilder.JoinType.INNER); + if (storageId != null) { + SearchBuilder volumeSearch = _volsDao.createSearchBuilder(); + volumeSearch.and("poolId", volumeSearch.entity().getPoolId(), SearchCriteria.Op.EQ); + sb.join("volumeSearch", volumeSearch, sb.entity().getId(), volumeSearch.entity().getInstanceId(), JoinBuilder.JoinType.INNER); + } // populate the search criteria with the values passed in - SearchCriteria sc = sb.create(); - + SearchCriteria sc = sb.create(); if (groupId != null && (Long)groupId == -1){ sc.setJoinParameters("vmSearch", "instanceId", (Object)null); } else if (groupId != null ) { @@ -2855,6 +2805,7 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager if (id != null) { sc.setParameters("id", id); } + if (accountIds != null) { if (accountIds.length == 1) { if (accountIds[0] != null) { @@ -2863,18 +2814,16 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager } else { sc.setParameters("accountIdIN", accountIds); } - } else if (domainId != null) { - sc.setJoinParameters("domainSearch", "id", domainId); + } + + if (domainId != null) { + sc.setParameters("domainId", domainId); } if (path != null) { sc.setJoinParameters("domainSearch", "path", path + "%"); } - if (storageId != null) { - sc.setJoinParameters("volumeSearch", "poolId", storageId); - } - if (networkId != null) { sc.setJoinParameters("nicSearch", "networkId", networkId); } @@ -2882,6 +2831,7 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager if (name != null) { sc.setParameters("name", "%" + name + "%"); } + if (state != null) { if (notState != null && (Boolean) notState == true) { sc.setParameters("stateNEQ", state); @@ -2893,6 +2843,8 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager if (hypervisor != null){ sc.setParameters("hypervisorType", hypervisor); } + + //Don't show Destroyed and Expunging vms to the end user if ((isAdmin != null) && ((Boolean) isAdmin != true)) { sc.setParameters("stateNIN", "Destroyed", "Expunging"); } @@ -2900,7 +2852,7 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager if (zone != null) { sc.setParameters("dataCenterId", zone); - if(state == null) { + if (state == null) { sc.setParameters("stateNEQ", "Destroyed"); } } @@ -2929,6 +2881,10 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager } } } + + if (storageId != null) { + sc.setJoinParameters("volumeSearch", "poolId", storageId); + } return _vmDao.search(sc, searchFilter); } diff --git a/setup/db/create-schema.sql b/setup/db/create-schema.sql index 1b727bb089b..73869d5fba1 100755 --- a/setup/db/create-schema.sql +++ b/setup/db/create-schema.sql @@ -967,7 +967,8 @@ CREATE TABLE `cloud`.`domain` ( `removed` datetime COMMENT 'date removed', `state` char(32) NOT NULL default 'Active' COMMENT 'state of the domain', PRIMARY KEY (`id`), - UNIQUE (parent, name, removed) + UNIQUE (parent, name, removed), + INDEX `i_domain__path`(`path`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8; CREATE TABLE `cloud`.`account` ( diff --git a/setup/db/schema-222to224.sql b/setup/db/schema-222to224.sql index 56be9cd85e1..c5dc4c636b8 100644 --- a/setup/db/schema-222to224.sql +++ b/setup/db/schema-222to224.sql @@ -57,4 +57,7 @@ ALTER TABLE `cloud`.`cluster` ADD INDEX `i_cluster__allocation_state`(`allocatio ALTER TABLE `cloud`.`host_pod_ref` ADD COLUMN `allocation_state` varchar(32) NOT NULL DEFAULT 'Enabled'; ALTER TABLE `cloud`.`host_pod_ref` ADD INDEX `i_host_pod_ref__allocation_state`(`allocation_state`); ALTER TABLE `cloud`.`host` ADD COLUMN `allocation_state` varchar(32) NOT NULL DEFAULT 'Enabled'; -ALTER TABLE `cloud`.`host` ADD INDEX `i_host__allocation_state`(`allocation_state`); \ No newline at end of file +ALTER TABLE `cloud`.`host` ADD INDEX `i_host__allocation_state`(`allocation_state`); + +ALTER TABLE `cloud`.`domain` ADD INDEX `i_domain__path`(`path`); +