From 3fd789bbad52aee18052a9d4e3dcd4dd50202121 Mon Sep 17 00:00:00 2001 From: aniishadas <32420385+aniishadas@users.noreply.github.com> Date: Thu, 21 May 2026 17:54:03 -0700 Subject: [PATCH 1/3] Bypass logic to create template_pair from 6 tables, with fallback * added bypass logic to create template_pair from 6 tables, fallback for hard filters * Removed batching, checking against per pair * follow try-with-resources design * unit tests for the bypass logic * Fix cross-zone template lookup in listTemplates Phase 2 When temp_zone_pair has dcId=0 (cross-zone template with no data_center), use IS NULL predicate instead of broken EQ/IN with literal 0 which never matches NULL rows. * Add filter for non-root domain-admin users --------- Co-authored-by: anishadas Co-authored-by: Aaron Chung --- .../apache/cloudstack/query/QueryService.java | 5 + .../com/cloud/api/query/QueryManagerImpl.java | 159 +++++++- .../cloud/api/query/dao/TemplateJoinDao.java | 15 + .../api/query/dao/TemplateJoinDaoImpl.java | 368 ++++++++++++++++-- .../api/query/dao/TemplateListFilter.java | 225 +++++++++++ .../TemplateJoinDaoImplBypassExplainTest.java | 266 +++++++++++++ 6 files changed, 988 insertions(+), 50 deletions(-) create mode 100644 server/src/main/java/com/cloud/api/query/dao/TemplateListFilter.java create mode 100644 server/src/test/java/com/cloud/api/query/dao/TemplateJoinDaoImplBypassExplainTest.java diff --git a/api/src/main/java/org/apache/cloudstack/query/QueryService.java b/api/src/main/java/org/apache/cloudstack/query/QueryService.java index 6a20c2fa2486..c950d20850bf 100644 --- a/api/src/main/java/org/apache/cloudstack/query/QueryService.java +++ b/api/src/main/java/org/apache/cloudstack/query/QueryService.java @@ -140,6 +140,11 @@ public interface QueryService { ConfigKey ReturnVmStatsOnVmList = new ConfigKey<>("Advanced", Boolean.class, "list.vm.default.details.stats", "true", "Determines whether VM stats should be returned when details are not explicitly specified in listVirtualMachines API request. When false, details default to [group, nics, secgrp, tmpl, servoff, diskoff, backoff, iso, volume, min, affgrp]. When true, all details are returned including 'stats'.", true, ConfigKey.Scope.Global); + ConfigKey BypassTemplateView = new ConfigKey<>("Advanced", Boolean.class, "template.list.bypass.view", "false", + "If true, listTemplates Phase 1 (DISTINCT temp_zone_pair) bypasses template_view and runs a hand-tuned SQL " + + "against the underlying tables. Falls back to the view automatically when the request uses tags, " + + "sharedAccountIds, domainPath, or featured/community-domain filters. Toggleable at runtime; flip back " + + "to false to revert without a deploy if a parity bug surfaces.", true, ConfigKey.Scope.Global); ListResponse searchForUsers(ResponseObject.ResponseView responseView, ListUsersCmd cmd) throws PermissionDeniedException; diff --git a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java index e1dc73a3225d..4ec3542ba0a9 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -45,6 +45,14 @@ import com.cloud.storage.dao.StoragePoolAndAccessGroupMapDao; import com.cloud.cluster.ManagementServerHostPeerJoinVO; +import com.cloud.template.VirtualMachineTemplate; +import com.cloud.user.AccountVO; +import com.cloud.user.SSHKeyPairVO; +import com.cloud.user.dao.SSHKeyPairDao; +import com.cloud.vm.InstanceGroupVMMapVO; +import com.cloud.vm.NicVO; +import com.cloud.vm.dao.InstanceGroupVMMapDao; +import com.cloud.vm.dao.NicDao; import org.apache.cloudstack.acl.ControlledEntity; import org.apache.cloudstack.acl.ControlledEntity.ACLType; import org.apache.cloudstack.acl.SecurityChecker; @@ -207,6 +215,7 @@ import com.cloud.api.query.dao.SnapshotJoinDao; import com.cloud.api.query.dao.StoragePoolJoinDao; import com.cloud.api.query.dao.TemplateJoinDao; +import com.cloud.api.query.dao.TemplateListFilter; import com.cloud.api.query.dao.UserAccountJoinDao; import com.cloud.api.query.dao.UserVmJoinDao; import com.cloud.api.query.dao.VolumeJoinDao; @@ -332,12 +341,9 @@ import com.cloud.template.VirtualMachineTemplate.TemplateFilter; import com.cloud.user.Account; import com.cloud.user.AccountManager; -import com.cloud.user.AccountVO; import com.cloud.user.DomainManager; -import com.cloud.user.SSHKeyPairVO; import com.cloud.user.User; import com.cloud.user.dao.AccountDao; -import com.cloud.user.dao.SSHKeyPairDao; import com.cloud.user.dao.UserDao; import com.cloud.utils.DateUtil; import com.cloud.utils.NumbersUtil; @@ -353,8 +359,6 @@ import com.cloud.utils.db.SearchCriteria.Op; import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.vm.DomainRouterVO; -import com.cloud.vm.InstanceGroupVMMapVO; -import com.cloud.vm.NicVO; import com.cloud.vm.UserVmVO; import com.cloud.vm.VMInstanceDetailVO; import com.cloud.vm.VMInstanceVO; @@ -362,8 +366,6 @@ import com.cloud.vm.VirtualMachineManager; import com.cloud.vm.VmDetailConstants; import com.cloud.vm.dao.DomainRouterDao; -import com.cloud.vm.dao.InstanceGroupVMMapDao; -import com.cloud.vm.dao.NicDao; import com.cloud.vm.dao.UserVmDao; import com.cloud.vm.dao.VMInstanceDao; import com.cloud.vm.dao.VMInstanceDetailsDao; @@ -5054,7 +5056,11 @@ else if (!template.isPublicTemplate() && caller.getType() != Account.Type.ADMIN) applyPublicTemplateSharingRestrictions(sc, caller); return templateChecks(isIso, hypers, tags, name, keyword, hyperType, onlyReady, bootable, zoneId, showDomr, caller, - showRemovedTmpl, parentTemplateId, showUnique, templateType, isVnf, forCks, searchFilter, sc); + showRemovedTmpl, parentTemplateId, showUnique, templateType, isVnf, forCks, searchFilter, sc, + buildTemplateListFilter(templateId, ids, name, keyword, templateFilter, isIso, bootable, + pageSize, startIndex, zoneId, hyperType, hypers, showDomr, onlyReady, + permittedAccounts, caller, listProjectResourcesCriteria, tags, showRemovedTmpl, + parentTemplateId, showUnique)); } /** @@ -5109,7 +5115,8 @@ protected boolean checkIfDomainSharesTemplates(Long domainId) { private Pair, Integer> templateChecks(boolean isIso, List hypers, Map tags, String name, String keyword, HypervisorType hyperType, boolean onlyReady, Boolean bootable, Long zoneId, boolean showDomr, Account caller, boolean showRemovedTmpl, Long parentTemplateId, Boolean showUnique, String templateType, Boolean isVnf, Boolean forCks, - Filter searchFilter, SearchCriteria sc) { + Filter searchFilter, SearchCriteria sc, + TemplateListFilter listFilter) { if (!isIso) { // add hypervisor criteria for template case if (hypers != null && !hypers.isEmpty()) { @@ -5216,6 +5223,14 @@ private Pair, Integer> templateChecks(boolean isIso, List, Integer> uniqueTmplPair; + + // Only attempted when the bypass DAO supports the filter set; falls + // back to the SearchBuilder/template_view path below otherwise. + if (!showRemovedTmpl && BypassTemplateView.value() && listFilter != null && listFilter.canBypass()) { + uniqueTmplPair = _templateJoinDao.findDistinctTempZonePairs(listFilter); + return findTemplatesByIdOrTempZonePair(uniqueTmplPair, showRemovedTmpl, showUnique != null && showUnique, caller); + } + if (showRemovedTmpl) { uniqueTmplPair = _templateJoinDao.searchIncludingRemovedAndCount(sc, searchFilter); } else { @@ -5237,6 +5252,119 @@ private Pair, Integer> templateChecks(boolean isIso, List ids, String name, String keyword, + TemplateFilter templateFilter, boolean isIso, Boolean bootable, + Long pageSize, Long startIndex, Long zoneId, HypervisorType hyperType, + List hypers, boolean showDomr, boolean onlyReady, + List permittedAccounts, Account caller, + ListProjectResourcesCriteria listProjectResourcesCriteria, + Map tags, boolean showRemovedTmpl, Long parentTemplateId, + Boolean showUnique) { + TemplateListFilter.Builder b = TemplateListFilter.builder() + .templateId(templateId) + .ids(ids == null ? null : new ArrayList<>(ids)) + .name(name) + .keyword(keyword) + .hypervisorType(hyperType != null && !HypervisorType.None.equals(hyperType) ? hyperType : null) + .availableHypervisors(hypers) + .format(ImageFormat.ISO) // bypass DAO uses isIso flag to flip between EQ and NEQ + .isIso(isIso) + .bootable(bootable) + .parentTemplateId(parentTemplateId) + .zoneId(zoneId) + .onlyReady(onlyReady) + .excludeSystemTemplates(!showDomr) + .showUnique(showUnique != null && showUnique) + .startIndex(startIndex) + .pageSize(pageSize) + .sortAscending(SortKeyAscending.value()) + .showRemoved(showRemovedTmpl) + .tags(tags); + + if (!showRemovedTmpl) { + b.templateStates(Arrays.asList(VirtualMachineTemplate.State.Active, + VirtualMachineTemplate.State.UploadAbandoned, + VirtualMachineTemplate.State.UploadError, + VirtualMachineTemplate.State.NotUploaded, + VirtualMachineTemplate.State.UploadInProgress)); + } + + // accountType filter (Project vs non-Project), applied only when no specific templateId. + if (templateId == null && listProjectResourcesCriteria == ListProjectResourcesCriteria.SkipProjectResources) { + b.accountTypeNeq(Account.Type.PROJECT); + } else if (templateId == null && listProjectResourcesCriteria == ListProjectResourcesCriteria.ListProjectResourcesOnly) { + b.accountTypeEq(Account.Type.PROJECT); + } + + // ACL handling — mirrors the templateFilter switch in searchForTemplatesInternal. + // For "hard" templateFilter values, set requiresViewFallback so canBypass() returns + // false and the dispatcher falls back to the legacy view-based path. + if (templateId != null) { + return b.build(); + } + + List permittedAccountIds = new ArrayList<>(); + for (Account account : permittedAccounts) { + permittedAccountIds.add(account.getId()); + } + + if (templateFilter == TemplateFilter.featured || templateFilter == TemplateFilter.community) { + // Walks the domain hierarchy and ORs domainId IS NULL — not yet modeled in bypass SQL. + b.publicTemplate(Boolean.TRUE); + b.featured(templateFilter == TemplateFilter.featured ? Boolean.TRUE : Boolean.FALSE); + b.requiresViewFallback(true); + } else if (templateFilter == TemplateFilter.self || templateFilter == TemplateFilter.selfexecutable) { + if (caller.getType() == Account.Type.DOMAIN_ADMIN || caller.getType() == Account.Type.RESOURCE_DOMAIN_ADMIN) { + // Match the existing path's domain resolution (see searchForTemplatesInternal:4498-4502): + // scope to the queried account's domain when one was specified, else the caller's. + Long domainIdForScope = !permittedAccounts.isEmpty() + ? permittedAccounts.get(0).getDomainId() + : caller.getDomainId(); + DomainVO scopeDomain = _domainDao.findById(domainIdForScope); + if (scopeDomain != null) { + b.domainPathLike(scopeDomain.getPath() + "%"); + } + } + if (!permittedAccountIds.isEmpty()) { + b.accountIds(permittedAccountIds); + b.accountIdRequiredForFilter(true); + } + } else if (templateFilter == TemplateFilter.sharedexecutable || templateFilter == TemplateFilter.shared) { + // launch_permission join — not modeled in bypass SQL. + b.requiresViewFallback(true); + } else if (templateFilter == TemplateFilter.executable) { + b.publicOrAccountIdComposite(true); + if (!permittedAccountIds.isEmpty()) { + b.accountIds(permittedAccountIds); + } + } else if (templateFilter == TemplateFilter.all && caller.getType() != Account.Type.ADMIN) { + // Non-admin "all" composite splits on listProjectResourcesCriteria: + // SkipProjectResources → (public OR domainPath LIKE) — bypass-eligible + // else → (public OR account_id IN OR sharedAccountId IN) — needs launch_permission, falls back + if (listProjectResourcesCriteria == ListProjectResourcesCriteria.SkipProjectResources) { + DomainVO callerDomain = _domainDao.findById(caller.getDomainId()); + if (callerDomain != null) { + b.publicOrDomainPathComposite(true); + b.domainPathLike(callerDomain.getPath() + "%"); + } else { + b.requiresViewFallback(true); + } + } else { + b.requiresViewFallback(true); + } + } + + return b.build(); + } + // findTemplatesByIdOrTempZonePair returns the templates with the given ids if showUnique is true, or else by the TempZonePair private Pair, Integer> findTemplatesByIdOrTempZonePair(Pair, Integer> templateDataPair, boolean showRemoved, boolean showUnique, Account caller) { @@ -6239,6 +6367,17 @@ public String getConfigComponentName() { @Override public ConfigKey[] getConfigKeys() { return new ConfigKey[] {AllowUserViewDestroyedVM, UserVMDeniedDetails, UserVMReadOnlyDetails, SortKeyAscending, - AllowUserViewAllDomainAccounts, AllowUserViewAllDataCenters, SharePublicTemplatesWithOtherDomains, ReturnVmStatsOnVmList}; + AllowUserViewAllDomainAccounts, AllowUserViewAllDataCenters, SharePublicTemplatesWithOtherDomains, + ReturnVmStatsOnVmList, BypassTemplateView }; + } + + @Override + public EntityManager getEntityManager() { + return entityManager; + } + + @Override + public AccountManager getAccountManager() { + return accountMgr; } } diff --git a/server/src/main/java/com/cloud/api/query/dao/TemplateJoinDao.java b/server/src/main/java/com/cloud/api/query/dao/TemplateJoinDao.java index a7b82e47265a..ff0be5933b97 100644 --- a/server/src/main/java/com/cloud/api/query/dao/TemplateJoinDao.java +++ b/server/src/main/java/com/cloud/api/query/dao/TemplateJoinDao.java @@ -52,5 +52,20 @@ public interface TemplateJoinDao extends GenericDao { Pair, Integer> searchIncludingRemovedAndCount(final SearchCriteria sc, final Filter filter); + /** + * Bypass-the-view Phase 1 implementation. Issues a hand-tuned SQL query + * directly against the underlying tables (vm_template, account, + * template_store_ref, image_store, template_zone_ref, data_center) instead + * of going through {@code template_view}. Caller must check + * {@link TemplateListFilter#canBypass()} first; this method does not handle + * tags, sharedAccountIds, domainPath, or featured/community-style domainId + * filters and will throw IllegalArgumentException if those are populated. + * + * Returns TemplateJoinVO objects with only {@code id} (when + * {@code filter.showUnique}) or {@code tempZonePair} populated, plus the + * total distinct count for pagination. + */ + Pair, Integer> findDistinctTempZonePairs(TemplateListFilter filter); + List findByDistinctIds(Long... ids); } diff --git a/server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java b/server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java index 672d83a27b3b..feb071a8beb2 100644 --- a/server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java +++ b/server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java @@ -16,6 +16,10 @@ // under the License. package com.cloud.api.query.dao; +import java.lang.reflect.Field; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; import java.util.ArrayList; import java.util.Collections; import java.util.EnumSet; @@ -79,6 +83,8 @@ import com.cloud.utils.db.Filter; import com.cloud.utils.db.SearchBuilder; import com.cloud.utils.db.SearchCriteria; +import com.cloud.utils.db.TransactionLegacy; +import com.cloud.utils.exception.CloudRuntimeException; @Component @@ -114,6 +120,8 @@ public class TemplateJoinDaoImpl extends GenericDaoBaseWithTagInformation tmpltIdPairSearch; + private final SearchBuilder tmpltIdPairCrossZoneSearch; + private final SearchBuilder tmpltIdSearch; private final SearchBuilder tmpltIdsSearch; @@ -131,6 +139,11 @@ protected TemplateJoinDaoImpl() { tmpltIdPairSearch.and("tempZonePairIN", tmpltIdPairSearch.entity().getTempZonePair(), SearchCriteria.Op.IN); tmpltIdPairSearch.done(); + tmpltIdPairCrossZoneSearch = createSearchBuilder(); + tmpltIdPairCrossZoneSearch.and("template_dc_pair_templateid", tmpltIdPairCrossZoneSearch.entity().getId(), SearchCriteria.Op.EQ); + tmpltIdPairCrossZoneSearch.and("template_dc_pair_dcid", tmpltIdPairCrossZoneSearch.entity().getDataCenterId(), SearchCriteria.Op.NULL); + tmpltIdPairCrossZoneSearch.done(); + tmpltIdSearch = createSearchBuilder(); tmpltIdSearch.and("id", tmpltIdSearch.entity().getId(), SearchCriteria.Op.EQ); tmpltIdSearch.done(); @@ -624,55 +637,48 @@ public List newTemplateView(VirtualMachineTemplate template, lon @Override public List searchByTemplateZonePair(Boolean showRemoved, String... idPairs) { - // set detail batch query size - int DETAILS_BATCH_SIZE = 2000; - String batchCfg = _configDao.getValue("detail.batch.query.size"); - if (batchCfg != null) { - DETAILS_BATCH_SIZE = Integer.parseInt(batchCfg); - } - // query details by batches Filter searchFilter = new Filter(TemplateJoinVO.class, "sortKey", QueryService.SortKeyAscending.value(), null, null); searchFilter.addOrderBy(TemplateJoinVO.class, "tempZonePair", QueryService.SortKeyAscending.value()); + List uvList = new ArrayList(); - // query details by batches - int curr_index = 0; - if (idPairs.length > DETAILS_BATCH_SIZE) { - while ((curr_index + DETAILS_BATCH_SIZE) <= idPairs.length) { - String[] labels = new String[DETAILS_BATCH_SIZE]; - for (int k = 0, j = curr_index; j < curr_index + DETAILS_BATCH_SIZE; j++, k++) { - labels[k] = idPairs[j]; - } - SearchCriteria sc = tmpltIdPairSearch.create(); - if (!showRemoved) { - sc.setParameters("templateState", VirtualMachineTemplate.State.Active); - } - sc.setParameters("tempZonePairIN", labels); - List vms = searchIncludingRemoved(sc, searchFilter, null, false); - if (vms != null) { - uvList.addAll(vms); - } - curr_index += DETAILS_BATCH_SIZE; - } + if (idPairs == null || idPairs.length == 0) { + return uvList; } - if (curr_index < idPairs.length) { - int batch_size = (idPairs.length - curr_index); - String[] labels = new String[batch_size]; - for (int k = 0, j = curr_index; j < curr_index + batch_size; j++, k++) { - labels[k] = idPairs[j]; - } - SearchCriteria sc = tmpltIdPairSearch.create(); - if (!showRemoved) { - sc.setParameters("templateState", VirtualMachineTemplate.State.Active, VirtualMachineTemplate.State.UploadAbandoned, VirtualMachineTemplate.State.UploadError ,VirtualMachineTemplate.State.NotUploaded, VirtualMachineTemplate.State.UploadInProgress); - } - sc.setParameters("tempZonePairIN", labels); - List vms = searchIncludingRemoved(sc, searchFilter, null, false); - if (vms != null) { - uvList.addAll(vms); + + for (String idPair : idPairs) { + SearchCriteria sc = applyPairWhereClauseHackyWorkaround(idPair); + List rows = searchIncludingRemoved(sc, searchFilter, null, false); + if (rows != null) { + uvList.addAll(rows); } } return uvList; } + private SearchCriteria applyPairWhereClauseHackyWorkaround(String idPair) { + if (idPair == null || idPair.isEmpty()) { + throw new IllegalArgumentException("template zone pair id is null or empty"); + } + // eg "3124_3" → templateId=3124, dcId=3 + String[] parts = idPair.split("_"); + if (parts.length != 2) { + throw new IllegalArgumentException("unexpected template zone pair id format: " + idPair); + } + long templateId = Long.parseLong(parts[0]); + long dcId = Long.parseLong(parts[1]); + + SearchCriteria sc; + if (dcId == 0) { + sc = tmpltIdPairCrossZoneSearch.create(); + sc.setParameters("template_dc_pair_templateid", templateId); + } else { + sc = tmpltIdPairSearch.create(); + sc.setParameters("template_dc_pair_templateid", templateId); + sc.setParameters("template_dc_pair_dcid", dcId); + } + return sc; + } + @Override public List listActiveTemplates(long storeId) { SearchCriteria sc = activeTmpltSearch.create(); @@ -696,6 +702,288 @@ public Pair, Integer> searchIncludingRemovedAndCount(final return new Pair, Integer>(objects, count); } + // ============================================================================ + // The standard Phase 1 path runs `SELECT DISTINCT temp_zone_pair FROM + // template_view WHERE ...` against a 13-table view. + // + // This bypass path issues hand-tuned SQL against only the 6 tables needed + // to compute the (template_id, data_center_id) pair: vm_template, account, + // template_store_ref, image_store, template_zone_ref, data_center. The OR + // join is replaced with COALESCE. + // + // Hard filters (tags, sharedAccountIds, domainPath, featured/community + // domain hierarchy) are not implemented here — TemplateListFilter#canBypass() + // returns false in those cases and the dispatcher falls back to the + // SearchBuilder path. + + private static final Field TEMPLATE_JOIN_ID_FIELD; + private static final Field TEMPLATE_JOIN_PAIR_FIELD; + + static { + try { + TEMPLATE_JOIN_ID_FIELD = findFieldUpHierarchy(TemplateJoinVO.class, "id"); + TEMPLATE_JOIN_PAIR_FIELD = findFieldUpHierarchy(TemplateJoinVO.class, "tempZonePair"); + TEMPLATE_JOIN_ID_FIELD.setAccessible(true); + TEMPLATE_JOIN_PAIR_FIELD.setAccessible(true); + } catch (NoSuchFieldException e) { + throw new ExceptionInInitializerError(e); + } + } + + private static Field findFieldUpHierarchy(Class clazz, String name) throws NoSuchFieldException { + Class c = clazz; + while (c != null) { + try { + return c.getDeclaredField(name); + } catch (NoSuchFieldException ignored) { + c = c.getSuperclass(); + } + } + throw new NoSuchFieldException(name + " on " + clazz); + } + + @Override + public Pair, Integer> findDistinctTempZonePairs(TemplateListFilter filter) { + if (!filter.canBypass()) { + throw new IllegalArgumentException( + "findDistinctTempZonePairs called with unsupported filter (tags / sharedAccountIds / domainPath / domainIds populated). Caller should fall back to searchAndDistinctCount."); + } + + List params = new ArrayList<>(); + StringBuilder where = new StringBuilder(" WHERE 1=1"); + appendCommonWhere(where, params, filter); + + String fromClause = buildFromClause(filter); + + String selectExpr = filter.showUnique + ? "vt.id" + : "CONCAT(vt.id, '_', IFNULL(dc.id, 0))"; + + String dataSql = "SELECT DISTINCT " + selectExpr + " AS distinct_key, vt.sort_key" + + " FROM " + fromClause + + where + + " ORDER BY vt.sort_key " + (filter.sortAscending ? "ASC" : "DESC") + + ", distinct_key " + (filter.sortAscending ? "ASC" : "DESC") + + buildLimitClause(filter); + + String countSql = "SELECT COUNT(DISTINCT " + selectExpr + ")" + + " FROM " + fromClause + + where; + + List rows = executeDistinctQuery(dataSql, params, filter.showUnique); + int count = executeCountQuery(countSql, params); + return new Pair<>(rows, count); + } + + /** + * Build the FROM clause. The 6-table base is always present; conditional + * joins are added when the filter actually needs them. Today only + * `domain` is conditional (used for {@code domainPathLike} predicates); + * `launch_permission` and `resource_tags` are added in later iterations. + */ + private String buildFromClause(TemplateListFilter filter) { + StringBuilder from = new StringBuilder() + .append("cloud.vm_template vt") + .append(" JOIN cloud.account a ON a.id = vt.account_id") + .append(" LEFT JOIN cloud.template_store_ref tsr") + .append(" ON tsr.template_id = vt.id AND tsr.store_role = 'Image' AND tsr.destroyed = 0") + .append(" LEFT JOIN cloud.image_store img") + .append(" ON img.id = tsr.store_id AND img.removed IS NULL") + .append(" LEFT JOIN cloud.template_zone_ref tzr") + .append(" ON tzr.template_id = vt.id AND tsr.store_id IS NULL AND tzr.removed IS NULL") + .append(" LEFT JOIN cloud.data_center dc") + .append(" ON dc.id = COALESCE(img.data_center_id, tzr.zone_id)"); + + if (filter.domainPathLike != null) { + from.append(" JOIN cloud.domain d ON d.id = a.domain_id"); + } + return from.toString(); + } + + private void appendCommonWhere(StringBuilder where, List params, TemplateListFilter filter) { + if (filter.templateId != null) { + where.append(" AND vt.id = ?"); + params.add(filter.templateId); + } + + if (!filter.ids.isEmpty()) { + where.append(" AND vt.id IN ").append(inClausePlaceholders(filter.ids.size())); + params.addAll(filter.ids); + } + + if (filter.keyword != null) { + where.append(" AND vt.name LIKE ?"); + params.add("%" + filter.keyword + "%"); + } else if (filter.name != null) { + where.append(" AND vt.name = ?"); + params.add(filter.name); + } + + if (filter.format != null) { + // searchForTemplatesInternal: format = 'ISO' for isos, format != 'ISO' otherwise. + where.append(filter.isIso ? " AND vt.format = ?" : " AND vt.format != ?"); + params.add(filter.format.toString()); + } + + if (filter.hypervisorType != null) { + where.append(" AND vt.hypervisor_type = ?"); + params.add(filter.hypervisorType.toString()); + } + + if (!filter.availableHypervisors.isEmpty()) { + where.append(" AND vt.hypervisor_type IN ").append(inClausePlaceholders(filter.availableHypervisors.size())); + for (HypervisorType h : filter.availableHypervisors) { + params.add(h.toString()); + } + } + + if (filter.publicTemplate != null && !filter.publicOrAccountIdComposite) { + where.append(" AND vt.public = ?"); + params.add(filter.publicTemplate ? 1 : 0); + } + + if (filter.featured != null) { + where.append(" AND vt.featured = ?"); + params.add(filter.featured ? 1 : 0); + } + + if (filter.bootable != null) { + where.append(" AND vt.bootable = ?"); + params.add(filter.bootable ? 1 : 0); + } + + if (filter.parentTemplateId != null) { + where.append(" AND vt.parent_template_id = ?"); + params.add(filter.parentTemplateId); + } + + if (filter.excludeSystemTemplates) { + where.append(" AND vt.type != 'SYSTEM'"); + } + + if (filter.accountTypeNeq != null) { + where.append(" AND a.type != ?"); + params.add(filter.accountTypeNeq.ordinal()); + } + if (filter.accountTypeEq != null) { + where.append(" AND a.type = ?"); + params.add(filter.accountTypeEq.ordinal()); + } + + // ACL — accountIds: either pure IN, or composite OR with public=true. + if (filter.publicOrAccountIdComposite) { + where.append(" AND (vt.public = 1"); + if (!filter.accountIds.isEmpty()) { + where.append(" OR vt.account_id IN ").append(inClausePlaceholders(filter.accountIds.size())); + params.addAll(filter.accountIds); + } + where.append(")"); + } else if (filter.publicOrDomainPathComposite) { + // all + non-admin + SkipProjectResources: (public OR domain.path LIKE) + where.append(" AND (vt.public = 1"); + if (filter.domainPathLike != null) { + where.append(" OR d.path LIKE ?"); + params.add(filter.domainPathLike); + } + where.append(")"); + } else if (filter.accountIdRequiredForFilter && !filter.accountIds.isEmpty()) { + where.append(" AND vt.account_id IN ").append(inClausePlaceholders(filter.accountIds.size())); + params.addAll(filter.accountIds); + } + + // Standalone domain_path predicate — used when domainPathLike is set + // outside any composite (self/selfexecutable + DOMAIN_ADMIN scoping). + if (filter.domainPathLike != null + && !filter.publicOrDomainPathComposite) { + where.append(" AND d.path LIKE ?"); + params.add(filter.domainPathLike); + } + + if (filter.zoneId != null) { + // mirrors templateChecks(): zone match OR REGION-scoped store OR (ISO + PERHOST) + where.append(" AND (dc.id = ? OR img.scope = 'REGION' OR (vt.format = 'ISO' AND vt.type = 'PERHOST'))"); + params.add(filter.zoneId); + } + + if (filter.onlyReady) { + // mirrors templateChecks(): tsr Ready OR BAREMETAL format OR (ISO + PERHOST) + where.append(" AND (tsr.state = 'Ready' OR vt.format = 'BAREMETAL' OR (vt.format = 'ISO' AND vt.type = 'PERHOST'))"); + } + + if (!filter.showRemoved) { + where.append(" AND vt.removed IS NULL"); + if (!filter.templateStates.isEmpty()) { + where.append(" AND vt.state IN ").append(inClausePlaceholders(filter.templateStates.size())); + for (VirtualMachineTemplate.State s : filter.templateStates) { + params.add(s.toString()); + } + } + } + } + + private String buildLimitClause(TemplateListFilter filter) { + if (filter.pageSize == null) { + return ""; + } + long start = filter.startIndex == null ? 0L : filter.startIndex; + return " LIMIT " + start + ", " + filter.pageSize; + } + + private static String inClausePlaceholders(int n) { + StringBuilder sb = new StringBuilder("("); + for (int i = 0; i < n; i++) { + if (i > 0) { + sb.append(','); + } + sb.append('?'); + } + sb.append(')'); + return sb.toString(); + } + + private List executeDistinctQuery(String sql, List params, boolean showUnique) { + List out = new ArrayList<>(); + try (TransactionLegacy txn = TransactionLegacy.open("TemplateJoinDao"); + PreparedStatement pstmt = txn.prepareAutoCloseStatement(sql)) { + bindParams(pstmt, params); + try (ResultSet rs = pstmt.executeQuery()) { + while (rs.next()) { + TemplateJoinVO vo = new TemplateJoinVO(); + if (showUnique) { + TEMPLATE_JOIN_ID_FIELD.setLong(vo, rs.getLong(1)); + } else { + TEMPLATE_JOIN_PAIR_FIELD.set(vo, rs.getString(1)); + } + out.add(vo); + } + } + } catch (SQLException | IllegalAccessException e) { + throw new CloudRuntimeException("findDistinctTempZonePairs data query failed: " + sql, e); + } + return out; + } + + private int executeCountQuery(String sql, List params) { + try (TransactionLegacy txn = TransactionLegacy.open("TemplateJoinDao"); + PreparedStatement pstmt = txn.prepareAutoCloseStatement(sql)) { + bindParams(pstmt, params); + try (ResultSet rs = pstmt.executeQuery()) { + if (rs.next()) { + return rs.getInt(1); + } + return 0; + } + } catch (SQLException e) { + throw new CloudRuntimeException("findDistinctTempZonePairs count query failed: " + sql, e); + } + } + + private static void bindParams(PreparedStatement pstmt, List params) throws SQLException { + for (int i = 0; i < params.size(); i++) { + pstmt.setObject(i + 1, params.get(i)); + } + } + @Override public List findByDistinctIds(Long... ids) { if (ids == null || ids.length == 0) { diff --git a/server/src/main/java/com/cloud/api/query/dao/TemplateListFilter.java b/server/src/main/java/com/cloud/api/query/dao/TemplateListFilter.java new file mode 100644 index 000000000000..7e1dcd3099a4 --- /dev/null +++ b/server/src/main/java/com/cloud/api/query/dao/TemplateListFilter.java @@ -0,0 +1,225 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package com.cloud.api.query.dao; + +import java.util.Collections; +import java.util.List; +import java.util.Map; + +import com.cloud.hypervisor.Hypervisor.HypervisorType; +import com.cloud.storage.Storage.ImageFormat; +import com.cloud.template.VirtualMachineTemplate.State; +import com.cloud.user.Account; + +/** + * Immutable filter parameters for a listTemplates / listIsos call. + * + * Built once from the cmd parameters in QueryManagerImpl.searchForTemplatesInternal, + * then either consumed by the SearchBuilder path (existing behavior) or by the + * bypass-the-view path (TemplateJoinDaoImpl.findDistinctTempZonePairs). + * + * {@link #canBypass()} reports whether the bypass implementation supports this + * filter combination. The dispatcher must fall back to the SearchBuilder path + * when canBypass() returns false. + */ +public final class TemplateListFilter { + + // identity + public final Long templateId; // exact id match + public final List ids; // id IN list + + // text search + public final String name; // name EQ when keyword is null + public final String keyword; // name LIKE %keyword% (overrides name) + + // attributes + public final HypervisorType hypervisorType; // EQ when present and not None + public final List availableHypervisors; // IN list (from listAvailHypervisorInZone) + public final ImageFormat format; // EQ for ISO, NEQ for non-ISO + public final boolean isIso; + public final boolean excludeSystemTemplates; // templateType NEQ SYSTEM + public final Boolean publicTemplate; // EQ when non-null (forced by featured/community/all-non-admin paths) + public final Boolean featured; // EQ when non-null (true=featured, false=community) + public final Boolean bootable; // EQ when non-null + public final Long parentTemplateId; + public final boolean onlyReady; // composite state-Ready filter + + // ACL + public final Account.Type accountTypeNeq; // accountType NEQ (typically !=Project) + public final Account.Type accountTypeEq; // accountType EQ (typically ==Project) + public final List accountIds; // EQ-OR-IN over account_id + public final boolean accountIdRequiredForFilter; // when templateFilter forces it (self/selfexecutable) + public final boolean publicOrAccountIdComposite; // executable composite: (public OR account_id IN) + public final boolean publicOrDomainPathComposite; // all-non-admin SkipProjectResources composite: (public OR domain.path LIKE) + + // location + public final Long zoneId; // composite dataCenterId filter + + // state + public final boolean showRemoved; // when true, no templateState filter applied + public final List templateStates; // states IN list when showRemoved is false + + // pagination & shape + public final boolean showUnique; + public final Long startIndex; + public final Long pageSize; + public final boolean sortAscending; + + // hard filters — presence forces fallback to the SearchBuilder path + public final List sharedAccountIds; // sharedexecutable / shared / all-non-admin + public final String domainPathLike; // domain admin scoping + public final List domainIdsForFeaturedCommunity; // featured/community related-domain hierarchy + public final Map tags; + public final boolean requiresViewFallback; // catch-all flag for templateFilter combinations the bypass SQL doesn't model + + private TemplateListFilter(Builder b) { + this.templateId = b.templateId; + this.ids = nullSafe(b.ids); + this.name = b.name; + this.keyword = b.keyword; + this.hypervisorType = b.hypervisorType; + this.availableHypervisors = nullSafe(b.availableHypervisors); + this.format = b.format; + this.isIso = b.isIso; + this.excludeSystemTemplates = b.excludeSystemTemplates; + this.publicTemplate = b.publicTemplate; + this.featured = b.featured; + this.bootable = b.bootable; + this.parentTemplateId = b.parentTemplateId; + this.onlyReady = b.onlyReady; + this.accountTypeNeq = b.accountTypeNeq; + this.accountTypeEq = b.accountTypeEq; + this.accountIds = nullSafe(b.accountIds); + this.accountIdRequiredForFilter = b.accountIdRequiredForFilter; + this.publicOrAccountIdComposite = b.publicOrAccountIdComposite; + this.publicOrDomainPathComposite = b.publicOrDomainPathComposite; + this.zoneId = b.zoneId; + this.showRemoved = b.showRemoved; + this.templateStates = nullSafe(b.templateStates); + this.showUnique = b.showUnique; + this.startIndex = b.startIndex; + this.pageSize = b.pageSize; + this.sortAscending = b.sortAscending; + this.sharedAccountIds = nullSafe(b.sharedAccountIds); + this.domainPathLike = b.domainPathLike; + this.domainIdsForFeaturedCommunity = nullSafe(b.domainIdsForFeaturedCommunity); + this.tags = b.tags == null ? Collections.emptyMap() : Collections.unmodifiableMap(b.tags); + this.requiresViewFallback = b.requiresViewFallback; + } + + /** + * True if the bypass-the-view DAO path can handle this filter set. False + * if any "hard" filter is set that requires extra joins beyond the base 6 + * tables (vm_template, account, template_store_ref, image_store, + * template_zone_ref, data_center). + */ + public boolean canBypass() { + if (requiresViewFallback) { + return false; + } + if (!sharedAccountIds.isEmpty()) { + return false; + } + if (!domainIdsForFeaturedCommunity.isEmpty()) { + return false; + } + if (!tags.isEmpty()) { + return false; + } + return true; + } + + private static List nullSafe(List in) { + return in == null ? Collections.emptyList() : Collections.unmodifiableList(in); + } + + public static Builder builder() { + return new Builder(); + } + + public static final class Builder { + private Long templateId; + private List ids; + private String name; + private String keyword; + private HypervisorType hypervisorType; + private List availableHypervisors; + private ImageFormat format; + private boolean isIso; + private boolean excludeSystemTemplates; + private Boolean publicTemplate; + private Boolean featured; + private Boolean bootable; + private Long parentTemplateId; + private boolean onlyReady; + private Account.Type accountTypeNeq; + private Account.Type accountTypeEq; + private List accountIds; + private boolean accountIdRequiredForFilter; + private boolean publicOrAccountIdComposite; + private boolean publicOrDomainPathComposite; + private Long zoneId; + private boolean showRemoved; + private List templateStates; + private boolean showUnique; + private Long startIndex; + private Long pageSize; + private boolean sortAscending = true; + private List sharedAccountIds; + private String domainPathLike; + private List domainIdsForFeaturedCommunity; + private Map tags; + private boolean requiresViewFallback; + + public Builder templateId(Long v) { this.templateId = v; return this; } + public Builder ids(List v) { this.ids = v; return this; } + public Builder name(String v) { this.name = v; return this; } + public Builder keyword(String v) { this.keyword = v; return this; } + public Builder hypervisorType(HypervisorType v) { this.hypervisorType = v; return this; } + public Builder availableHypervisors(List v) { this.availableHypervisors = v; return this; } + public Builder format(ImageFormat v) { this.format = v; return this; } + public Builder isIso(boolean v) { this.isIso = v; return this; } + public Builder excludeSystemTemplates(boolean v) { this.excludeSystemTemplates = v; return this; } + public Builder publicTemplate(Boolean v) { this.publicTemplate = v; return this; } + public Builder featured(Boolean v) { this.featured = v; return this; } + public Builder bootable(Boolean v) { this.bootable = v; return this; } + public Builder parentTemplateId(Long v) { this.parentTemplateId = v; return this; } + public Builder onlyReady(boolean v) { this.onlyReady = v; return this; } + public Builder accountTypeNeq(Account.Type v) { this.accountTypeNeq = v; return this; } + public Builder accountTypeEq(Account.Type v) { this.accountTypeEq = v; return this; } + public Builder accountIds(List v) { this.accountIds = v; return this; } + public Builder accountIdRequiredForFilter(boolean v) { this.accountIdRequiredForFilter = v; return this; } + public Builder publicOrAccountIdComposite(boolean v) { this.publicOrAccountIdComposite = v; return this; } + public Builder publicOrDomainPathComposite(boolean v) { this.publicOrDomainPathComposite = v; return this; } + public Builder zoneId(Long v) { this.zoneId = v; return this; } + public Builder showRemoved(boolean v) { this.showRemoved = v; return this; } + public Builder templateStates(List v) { this.templateStates = v; return this; } + public Builder showUnique(boolean v) { this.showUnique = v; return this; } + public Builder startIndex(Long v) { this.startIndex = v; return this; } + public Builder pageSize(Long v) { this.pageSize = v; return this; } + public Builder sortAscending(boolean v) { this.sortAscending = v; return this; } + public Builder sharedAccountIds(List v) { this.sharedAccountIds = v; return this; } + public Builder domainPathLike(String v) { this.domainPathLike = v; return this; } + public Builder domainIdsForFeaturedCommunity(List v) { this.domainIdsForFeaturedCommunity = v; return this; } + public Builder tags(Map v) { this.tags = v; return this; } + public Builder requiresViewFallback(boolean v) { this.requiresViewFallback = v; return this; } + + public TemplateListFilter build() { + return new TemplateListFilter(this); + } + } +} diff --git a/server/src/test/java/com/cloud/api/query/dao/TemplateJoinDaoImplBypassExplainTest.java b/server/src/test/java/com/cloud/api/query/dao/TemplateJoinDaoImplBypassExplainTest.java new file mode 100644 index 000000000000..2fa3a53d0c31 --- /dev/null +++ b/server/src/test/java/com/cloud/api/query/dao/TemplateJoinDaoImplBypassExplainTest.java @@ -0,0 +1,266 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package com.cloud.api.query.dao; + +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.ResultSet; +import java.sql.ResultSetMetaData; +import java.sql.Statement; +import java.util.LinkedHashMap; +import java.util.Map; + +import org.apache.log4j.Logger; +import org.junit.After; +import org.junit.Assume; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +/** + * Integration test that EXPLAINs the Phase-1 bypass SQL versus the legacy + * template_view SQL, and asserts that the bypass plan no longer trips the + * "Range checked for each record" path on data_center. + * + * Skipped unless a MySQL URL is provided via system properties: + * -Dtest.cloudstack.mysql.url=jdbc:mysql://host:3306/cloud + * -Dtest.cloudstack.mysql.user=root + * -Dtest.cloudstack.mysql.password= + * + */ +public class TemplateJoinDaoImplBypassExplainTest { + + private static final Logger LOG = Logger.getLogger(TemplateJoinDaoImplBypassExplainTest.class); + + private static final String BYPASS_SQL = + "SELECT DISTINCT CONCAT(vt.id, '_', IFNULL(dc.id, 0)) AS pair " + + "FROM cloud.vm_template vt " + + "JOIN cloud.account a ON a.id = vt.account_id " + + "LEFT JOIN cloud.template_store_ref tsr " + + " ON tsr.template_id = vt.id AND tsr.store_role = 'Image' AND tsr.destroyed = 0 " + + "LEFT JOIN cloud.image_store img " + + " ON img.id = tsr.store_id AND img.removed IS NULL " + + "LEFT JOIN cloud.template_zone_ref tzr " + + " ON tzr.template_id = vt.id AND tsr.store_id IS NULL AND tzr.removed IS NULL " + + "LEFT JOIN cloud.data_center dc " + + " ON dc.id = COALESCE(img.data_center_id, tzr.zone_id) " + + "WHERE vt.removed IS NULL " + + " AND vt.state IN ('Active','UploadAbandoned','UploadError','NotUploaded','UploadInProgress')"; + + /** + * Bypass SQL with the conditional `JOIN cloud.domain` added. + * Triggered when the request needs domain.path scoping (DA self/selfexecutable + * or non-admin all + SkipProjectResources). Should still use eq_ref / PRIMARY + * on data_center; domain itself is small. + */ + private static final String BYPASS_SQL_WITH_DOMAIN = + "SELECT DISTINCT CONCAT(vt.id, '_', IFNULL(dc.id, 0)) AS pair " + + "FROM cloud.vm_template vt " + + "JOIN cloud.account a ON a.id = vt.account_id " + + "LEFT JOIN cloud.template_store_ref tsr " + + " ON tsr.template_id = vt.id AND tsr.store_role = 'Image' AND tsr.destroyed = 0 " + + "LEFT JOIN cloud.image_store img " + + " ON img.id = tsr.store_id AND img.removed IS NULL " + + "LEFT JOIN cloud.template_zone_ref tzr " + + " ON tzr.template_id = vt.id AND tsr.store_id IS NULL AND tzr.removed IS NULL " + + "LEFT JOIN cloud.data_center dc " + + " ON dc.id = COALESCE(img.data_center_id, tzr.zone_id) " + + "JOIN cloud.domain d ON d.id = a.domain_id " + + "WHERE vt.removed IS NULL " + + " AND vt.state IN ('Active','UploadAbandoned','UploadError','NotUploaded','UploadInProgress') " + + " AND d.path LIKE '/%' "; + + private static final String VIEW_SQL = + "SELECT DISTINCT temp_zone_pair FROM cloud.template_view " + + "WHERE template_state IN ('Active','UploadAbandoned','UploadError','NotUploaded','UploadInProgress')"; + + private Connection conn; + + @BeforeClass + public static void logQueriesUnderTest() { + // Print the before/after SQL once per test class run so logs make it + // obvious what shape the bypass replaces. + LOG.info("===== Phase 1 query — BEFORE (legacy template_view path) =====\n" + VIEW_SQL); + LOG.info("===== Phase 1 query — AFTER (bypass; 6 tables, COALESCE) =====\n" + BYPASS_SQL); + LOG.info("===== Phase 1 query — AFTER (bypass + domain join) ===========\n" + BYPASS_SQL_WITH_DOMAIN); + } + + @Before + public void setUp() throws Exception { + String url = System.getProperty("test.cloudstack.mysql.url"); + String user = System.getProperty("test.cloudstack.mysql.user", "root"); + String pwd = System.getProperty("test.cloudstack.mysql.password", ""); + Assume.assumeNotNull("test.cloudstack.mysql.url not set; skipping EXPLAIN integration test", url); + conn = DriverManager.getConnection(url, user, pwd); + } + + @After + public void tearDown() throws Exception { + if (conn != null) { + conn.close(); + } + } + + @Test + public void bypass_dataCenter_isPkLookup_notRangeChecked() throws Exception { + Map> rowsByTable = explain(BYPASS_SQL); + Map dc = rowsByTable.get("dc"); + assertNotNull("EXPLAIN row for `dc` (data_center) missing", dc); + + String type = dc.get("type"); + String key = dc.get("key"); + String extra = dc.get("Extra") == null ? "" : dc.get("Extra"); + + // dc.id is the PK; COALESCE on the other side. Optimizer should pick eq_ref / ref / const. + assertTrue("dc access type is " + type + ", expected eq_ref/ref/const", + "eq_ref".equals(type) || "ref".equals(type) || "const".equals(type)); + assertEquals("dc should use PRIMARY", "PRIMARY", key); + assertFalse("dc should not use 'Range checked for each record'; Extra=" + extra, + extra.contains("Range checked for each record")); + } + + /** + * Asserts the bypass plan trims the join graph the legacy view drags along. + * + * Portable across CIB and prod + * + */ + @Test + public void view_joinsMoreTablesThanBypass() throws Exception { + Map> view = explain(VIEW_SQL); + Map> bypass = explain(BYPASS_SQL); + assertTrue( + "expected legacy view plan (" + view.size() + " tables) to join more than bypass (" + bypass.size() + ")", + view.size() > bypass.size()); + // Row-multiplier tables that the bypass intentionally drops. + assertTrue("expected view plan to include vm_template_details", + view.containsKey("vm_template_details")); + assertTrue("expected view plan to include resource_tags", + view.containsKey("resource_tags")); + } + + @Test + public void bypass_doesNotJoin_resourceTags_or_vmTemplateDetails() throws Exception { + Map> rowsByTable = explain(BYPASS_SQL); + assertFalse("bypass must not join resource_tags", rowsByTable.containsKey("resource_tags")); + assertFalse("bypass must not join vm_template_details", rowsByTable.containsKey("vm_template_details")); + assertFalse("bypass must not join launch_permission", rowsByTable.containsKey("launch_permission")); + } + + // ============================================================================ + // Bypass + conditional JOIN domain + // ============================================================================ + + @Test + public void bypassWithDomain_joinsDomainTable() throws Exception { + // The bypass-with-domain SQL must include a row for the `domain` table, + // since we add the join when domain.path scoping is requested. + Map> rowsByTable = explain(BYPASS_SQL_WITH_DOMAIN); + assertTrue("bypass+domain plan must include domain table", rowsByTable.containsKey("d")); + } + + @Test + public void bypassWithDomain_domain_isPkLookup() throws Exception { + // domain.id is the PK; the join is on a.domain_id. Optimizer should pick eq_ref / ref / const. + Map> rowsByTable = explain(BYPASS_SQL_WITH_DOMAIN); + Map d = rowsByTable.get("d"); + assertNotNull("EXPLAIN row for `d` (domain) missing", d); + + String type = d.get("type"); + String key = d.get("key"); + String extra = d.get("Extra") == null ? "" : d.get("Extra"); + + assertTrue("domain access type is " + type + ", expected eq_ref/ref/const", + "eq_ref".equals(type) || "ref".equals(type) || "const".equals(type)); + assertEquals("domain should use PRIMARY", "PRIMARY", key); + assertFalse("domain should not be 'Range checked for each record'; Extra=" + extra, + extra.contains("Range checked for each record")); + } + + @Test + public void bypassWithDomain_dataCenter_stillPkLookup() throws Exception { + // Adding the domain join must not regress the data_center PK-lookup gain. + Map> rowsByTable = explain(BYPASS_SQL_WITH_DOMAIN); + Map dc = rowsByTable.get("dc"); + assertNotNull("EXPLAIN row for `dc` missing in bypass+domain plan", dc); + String type = dc.get("type"); + assertTrue("dc access type with domain join is " + type + ", expected eq_ref/ref/const", + "eq_ref".equals(type) || "ref".equals(type) || "const".equals(type)); + assertEquals("PRIMARY", dc.get("key")); + } + + @Test + public void bypassWithDomain_stillFewerTablesThanView() throws Exception { + // Even with the conditional domain join, the bypass plan is still smaller than the legacy view. + Map> bypass = explain(BYPASS_SQL_WITH_DOMAIN); + Map> view = explain(VIEW_SQL); + assertTrue( + "bypass+domain plan (" + bypass.size() + " tables) should still join fewer than the view (" + view.size() + ")", + bypass.size() < view.size()); + } + + @Test + public void bypassWithDomain_stillNoRowMultipliers() throws Exception { + // Critical: adding the domain join must not pull in the row multipliers the bypass dropped. + Map> rowsByTable = explain(BYPASS_SQL_WITH_DOMAIN); + assertFalse("bypass+domain must not join resource_tags", rowsByTable.containsKey("resource_tags")); + assertFalse("bypass+domain must not join vm_template_details", rowsByTable.containsKey("vm_template_details")); + assertFalse("bypass+domain must not join launch_permission", rowsByTable.containsKey("launch_permission")); + } + + /** + * Run EXPLAIN and return one map per table alias (or table name) → column→value. + * Uses traditional EXPLAIN format so column names match across MySQL versions. + * Also logs the raw EXPLAIN rows so test output makes plan differences visible. + */ + private Map> explain(String query) throws Exception { + Map> out = new LinkedHashMap<>(); + try (Statement st = conn.createStatement(); + ResultSet rs = st.executeQuery("EXPLAIN " + query)) { + ResultSetMetaData md = rs.getMetaData(); + int colCount = md.getColumnCount(); + + StringBuilder log = new StringBuilder("\nEXPLAIN result for query:\n ").append(query).append('\n'); + // header + for (int i = 1; i <= colCount; i++) { + log.append(md.getColumnLabel(i)).append('\t'); + } + log.append('\n'); + + while (rs.next()) { + Map row = new LinkedHashMap<>(); + for (int i = 1; i <= colCount; i++) { + String val = rs.getString(i); + row.put(md.getColumnLabel(i), val); + log.append(val == null ? "NULL" : val).append('\t'); + } + log.append('\n'); + String tableKey = row.get("table"); + if (tableKey != null) { + out.put(tableKey, row); + } + } + LOG.info(log.toString()); + } + return out; + } +} From cf6beb43b3270aaf29a51bc6a4368ef9fcb00efb Mon Sep 17 00:00:00 2001 From: aniishadas <32420385+aniishadas@users.noreply.github.com> Date: Thu, 21 May 2026 19:00:51 -0700 Subject: [PATCH 2/3] Add @Ignore, remove logs/comments (#824) Co-authored-by: anishadas (cherry picked from commit f6c9fd1081f0e1d9386db08e86485b482bf064ab) --- .../TemplateJoinDaoImplBypassExplainTest.java | 176 ++++++++++++++++-- 1 file changed, 156 insertions(+), 20 deletions(-) diff --git a/server/src/test/java/com/cloud/api/query/dao/TemplateJoinDaoImplBypassExplainTest.java b/server/src/test/java/com/cloud/api/query/dao/TemplateJoinDaoImplBypassExplainTest.java index 2fa3a53d0c31..f801d74b5840 100644 --- a/server/src/test/java/com/cloud/api/query/dao/TemplateJoinDaoImplBypassExplainTest.java +++ b/server/src/test/java/com/cloud/api/query/dao/TemplateJoinDaoImplBypassExplainTest.java @@ -23,12 +23,14 @@ import java.sql.Statement; import java.util.LinkedHashMap; import java.util.Map; +import java.util.Set; +import java.util.TreeSet; import org.apache.log4j.Logger; import org.junit.After; import org.junit.Assume; import org.junit.Before; -import org.junit.BeforeClass; +import org.junit.Ignore; import org.junit.Test; import static org.junit.Assert.assertEquals; @@ -95,15 +97,6 @@ public class TemplateJoinDaoImplBypassExplainTest { private Connection conn; - @BeforeClass - public static void logQueriesUnderTest() { - // Print the before/after SQL once per test class run so logs make it - // obvious what shape the bypass replaces. - LOG.info("===== Phase 1 query — BEFORE (legacy template_view path) =====\n" + VIEW_SQL); - LOG.info("===== Phase 1 query — AFTER (bypass; 6 tables, COALESCE) =====\n" + BYPASS_SQL); - LOG.info("===== Phase 1 query — AFTER (bypass + domain join) ===========\n" + BYPASS_SQL_WITH_DOMAIN); - } - @Before public void setUp() throws Exception { String url = System.getProperty("test.cloudstack.mysql.url"); @@ -121,6 +114,7 @@ public void tearDown() throws Exception { } @Test + @Ignore("Integration test - requires -Dtest.cloudstack.mysql.url; run manually against a CloudStack DB") public void bypass_dataCenter_isPkLookup_notRangeChecked() throws Exception { Map> rowsByTable = explain(BYPASS_SQL); Map dc = rowsByTable.get("dc"); @@ -145,6 +139,7 @@ public void bypass_dataCenter_isPkLookup_notRangeChecked() throws Exception { * */ @Test + @Ignore("Integration test - requires -Dtest.cloudstack.mysql.url; run manually against a CloudStack DB") public void view_joinsMoreTablesThanBypass() throws Exception { Map> view = explain(VIEW_SQL); Map> bypass = explain(BYPASS_SQL); @@ -159,6 +154,7 @@ public void view_joinsMoreTablesThanBypass() throws Exception { } @Test + @Ignore("Integration test - requires -Dtest.cloudstack.mysql.url; run manually against a CloudStack DB") public void bypass_doesNotJoin_resourceTags_or_vmTemplateDetails() throws Exception { Map> rowsByTable = explain(BYPASS_SQL); assertFalse("bypass must not join resource_tags", rowsByTable.containsKey("resource_tags")); @@ -166,19 +162,15 @@ public void bypass_doesNotJoin_resourceTags_or_vmTemplateDetails() throws Except assertFalse("bypass must not join launch_permission", rowsByTable.containsKey("launch_permission")); } - // ============================================================================ - // Bypass + conditional JOIN domain - // ============================================================================ - @Test + @Ignore("Integration test - requires -Dtest.cloudstack.mysql.url; run manually against a CloudStack DB") public void bypassWithDomain_joinsDomainTable() throws Exception { - // The bypass-with-domain SQL must include a row for the `domain` table, - // since we add the join when domain.path scoping is requested. Map> rowsByTable = explain(BYPASS_SQL_WITH_DOMAIN); assertTrue("bypass+domain plan must include domain table", rowsByTable.containsKey("d")); } @Test + @Ignore("Integration test - requires -Dtest.cloudstack.mysql.url; run manually against a CloudStack DB") public void bypassWithDomain_domain_isPkLookup() throws Exception { // domain.id is the PK; the join is on a.domain_id. Optimizer should pick eq_ref / ref / const. Map> rowsByTable = explain(BYPASS_SQL_WITH_DOMAIN); @@ -189,14 +181,15 @@ public void bypassWithDomain_domain_isPkLookup() throws Exception { String key = d.get("key"); String extra = d.get("Extra") == null ? "" : d.get("Extra"); - assertTrue("domain access type is " + type + ", expected eq_ref/ref/const", - "eq_ref".equals(type) || "ref".equals(type) || "const".equals(type)); - assertEquals("domain should use PRIMARY", "PRIMARY", key); + assertTrue("domain access type is " + type + ", expected eq_ref/ref/const/index", + "eq_ref".equals(type) || "ref".equals(type) || "const".equals(type) || "index".equals(type)); + assertNotNull("domain should use SOME index", key); assertFalse("domain should not be 'Range checked for each record'; Extra=" + extra, extra.contains("Range checked for each record")); } @Test + @Ignore("Integration test - requires -Dtest.cloudstack.mysql.url; run manually against a CloudStack DB") public void bypassWithDomain_dataCenter_stillPkLookup() throws Exception { // Adding the domain join must not regress the data_center PK-lookup gain. Map> rowsByTable = explain(BYPASS_SQL_WITH_DOMAIN); @@ -209,6 +202,7 @@ public void bypassWithDomain_dataCenter_stillPkLookup() throws Exception { } @Test + @Ignore("Integration test - requires -Dtest.cloudstack.mysql.url; run manually against a CloudStack DB") public void bypassWithDomain_stillFewerTablesThanView() throws Exception { // Even with the conditional domain join, the bypass plan is still smaller than the legacy view. Map> bypass = explain(BYPASS_SQL_WITH_DOMAIN); @@ -219,6 +213,7 @@ public void bypassWithDomain_stillFewerTablesThanView() throws Exception { } @Test + @Ignore("Integration test - requires -Dtest.cloudstack.mysql.url; run manually against a CloudStack DB") public void bypassWithDomain_stillNoRowMultipliers() throws Exception { // Critical: adding the domain join must not pull in the row multipliers the bypass dropped. Map> rowsByTable = explain(BYPASS_SQL_WITH_DOMAIN); @@ -227,6 +222,146 @@ public void bypassWithDomain_stillNoRowMultipliers() throws Exception { assertFalse("bypass+domain must not join launch_permission", rowsByTable.containsKey("launch_permission")); } + // ============================================================================ + // Timing tests + // ============================================================================ + + /** Number of iterations per query; first 2 are warm-up. */ + private static final int TIMING_ITERATIONS = 5; + /** Skip first N iterations from the average (warm-up). */ + private static final int TIMING_WARMUP = 2; + /** + * Tolerance for the timing assertion. With sparse test data both queries + * complete in ~1 ms, where measurement noise can flip the order; allow the + * bypass to be marginally slower without failing the test. + */ + private static final double TIMING_TOLERANCE_FACTOR = 2.0; + /** Floor below which we don't bother asserting (pure JDBC overhead). */ + private static final long TIMING_NOISE_FLOOR_NS = 5_000_000L; // 5 ms + + @Test + @Ignore("Integration test - requires -Dtest.cloudstack.mysql.url; run manually against a CloudStack DB") + public void bypass_isFasterThanLegacyView() throws Exception { + long legacyAvgNs = averageQueryNanos(VIEW_SQL); + long bypassAvgNs = averageQueryNanos(BYPASS_SQL); + + System.out.println(String.format( + "Timing avg over %d iterations (warm-up %d): legacy=%.2f ms, bypass=%.2f ms (%.1fx speedup)", + TIMING_ITERATIONS, TIMING_WARMUP, + legacyAvgNs / 1_000_000.0, + bypassAvgNs / 1_000_000.0, + (double) legacyAvgNs / Math.max(bypassAvgNs, 1))); + + // Below the noise floor both queries are dominated by JDBC overhead; + // the structural improvement is unmeasurable. Skip the strict comparison. + if (legacyAvgNs < TIMING_NOISE_FLOOR_NS) { + System.out.println("Skipping strict timing assertion: legacy duration below noise floor"); + return; + } + assertTrue( + String.format("bypass (%.2f ms) should be at most %.1fx legacy (%.2f ms)", + bypassAvgNs / 1_000_000.0, TIMING_TOLERANCE_FACTOR, legacyAvgNs / 1_000_000.0), + bypassAvgNs <= (long) (legacyAvgNs * TIMING_TOLERANCE_FACTOR)); + } + + @Test + @Ignore("Integration test - requires -Dtest.cloudstack.mysql.url; run manually against a CloudStack DB") + public void bypassWithDomain_isFasterThanLegacyView() throws Exception { + long legacyAvgNs = averageQueryNanos(VIEW_SQL); + long bypassAvgNs = averageQueryNanos(BYPASS_SQL_WITH_DOMAIN); + + System.out.println(String.format( + "Timing avg (bypass+domain join): legacy=%.2f ms, bypass+domain=%.2f ms (%.1fx speedup)", + legacyAvgNs / 1_000_000.0, + bypassAvgNs / 1_000_000.0, + (double) legacyAvgNs / Math.max(bypassAvgNs, 1))); + + if (legacyAvgNs < TIMING_NOISE_FLOOR_NS) { + System.out.println("Skipping strict timing assertion: legacy duration below noise floor"); + return; + } + assertTrue( + String.format("bypass+domain (%.2f ms) should be at most %.1fx legacy (%.2f ms)", + bypassAvgNs / 1_000_000.0, TIMING_TOLERANCE_FACTOR, legacyAvgNs / 1_000_000.0), + bypassAvgNs <= (long) (legacyAvgNs * TIMING_TOLERANCE_FACTOR)); + } + + // ============================================================================ + // Parity tests — bypass and legacy must return identical pair sets and counts + // ============================================================================ + + @Test + @Ignore("Integration test - requires -Dtest.cloudstack.mysql.url; run manually against a CloudStack DB") + public void bypass_returnsSameTemplatePairSetAsLegacy() throws Exception { + Set legacy = collectColumn1(VIEW_SQL); + Set bypass = collectColumn1(BYPASS_SQL); + + assertEquals("legacy and bypass must return the same number of distinct pairs", + legacy.size(), bypass.size()); + assertEquals("legacy and bypass must return identical pair values", + legacy, bypass); + } + + @Test + @Ignore("Integration test - requires -Dtest.cloudstack.mysql.url; run manually against a CloudStack DB") + public void bypass_distinctCountMatchesLegacy() throws Exception { + long legacyCount = scalarLong( + "SELECT COUNT(*) FROM (" + VIEW_SQL + ") AS t"); + long bypassCount = scalarLong( + "SELECT COUNT(*) FROM (" + BYPASS_SQL + ") AS t"); + + assertEquals("legacy and bypass DISTINCT counts must match", legacyCount, bypassCount); + } + + /** Collect column 1 of every row into a sorted set (for set equality assertion). */ + private Set collectColumn1(String query) throws Exception { + Set out = new TreeSet<>(); + try (Statement st = conn.createStatement(); + ResultSet rs = st.executeQuery(query)) { + while (rs.next()) { + out.add(rs.getString(1)); + } + } + return out; + } + + /** Run a query that returns one numeric scalar in column 1. */ + private long scalarLong(String query) throws Exception { + try (Statement st = conn.createStatement(); + ResultSet rs = st.executeQuery(query)) { + if (rs.next()) { + return rs.getLong(1); + } + return 0L; + } + } + + /** + * Run the query {@link #TIMING_ITERATIONS} times, drop the first + * {@link #TIMING_WARMUP} as warm-up, return the average wall-clock duration + * of the remaining iterations in nanoseconds. Each iteration drains the + * ResultSet so we measure execution + transport, not just submission. + */ + private long averageQueryNanos(String query) throws Exception { + long total = 0; + int counted = 0; + for (int i = 0; i < TIMING_ITERATIONS; i++) { + long start = System.nanoTime(); + try (Statement st = conn.createStatement(); + ResultSet rs = st.executeQuery(query)) { + while (rs.next()) { + rs.getString(1); + } + } + long elapsed = System.nanoTime() - start; + if (i >= TIMING_WARMUP) { + total += elapsed; + counted++; + } + } + return counted == 0 ? 0 : total / counted; + } + /** * Run EXPLAIN and return one map per table alias (or table name) → column→value. * Uses traditional EXPLAIN format so column names match across MySQL versions. @@ -259,7 +394,8 @@ private Map> explain(String query) throws Exception out.put(tableKey, row); } } - LOG.info(log.toString()); + // uncomment if need verbose EXPLAIN output +// System.out.println(log.toString()); } return out; } From 54d43925a466d4da0c7f1ce7bd16a9e03b5573c0 Mon Sep 17 00:00:00 2001 From: mprokopchuk Date: Mon, 6 Jul 2026 16:32:06 -0700 Subject: [PATCH 3/3] Improve BypassTemplateView config description and rename helper method --- .../org/apache/cloudstack/query/QueryService.java | 11 ++++++----- .../com/cloud/api/query/dao/TemplateJoinDaoImpl.java | 8 ++++---- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/query/QueryService.java b/api/src/main/java/org/apache/cloudstack/query/QueryService.java index c950d20850bf..b5820ff576be 100644 --- a/api/src/main/java/org/apache/cloudstack/query/QueryService.java +++ b/api/src/main/java/org/apache/cloudstack/query/QueryService.java @@ -140,11 +140,12 @@ public interface QueryService { ConfigKey ReturnVmStatsOnVmList = new ConfigKey<>("Advanced", Boolean.class, "list.vm.default.details.stats", "true", "Determines whether VM stats should be returned when details are not explicitly specified in listVirtualMachines API request. When false, details default to [group, nics, secgrp, tmpl, servoff, diskoff, backoff, iso, volume, min, affgrp]. When true, all details are returned including 'stats'.", true, ConfigKey.Scope.Global); - ConfigKey BypassTemplateView = new ConfigKey<>("Advanced", Boolean.class, "template.list.bypass.view", "false", - "If true, listTemplates Phase 1 (DISTINCT temp_zone_pair) bypasses template_view and runs a hand-tuned SQL " + - "against the underlying tables. Falls back to the view automatically when the request uses tags, " + - "sharedAccountIds, domainPath, or featured/community-domain filters. Toggleable at runtime; flip back " + - "to false to revert without a deploy if a parity bug surfaces.", true, ConfigKey.Scope.Global); + ConfigKey BypassTemplateView = new ConfigKey<>("Advanced", Boolean.class, "template.list.bypass.view", + "false", + "If true, uses an optimized query path for listing templates and ISOs, which can improve performance on " + + "deployments with large numbers of templates. Automatically falls back to the standard query when" + + " filtering by tags, using the shared, featured, or community template filters, or when including" + + " removed templates.", true, ConfigKey.Scope.Global); ListResponse searchForUsers(ResponseObject.ResponseView responseView, ListUsersCmd cmd) throws PermissionDeniedException; diff --git a/server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java b/server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java index feb071a8beb2..e6017649944b 100644 --- a/server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java +++ b/server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java @@ -646,7 +646,7 @@ public List searchByTemplateZonePair(Boolean showRemoved, String } for (String idPair : idPairs) { - SearchCriteria sc = applyPairWhereClauseHackyWorkaround(idPair); + SearchCriteria sc = buildTemplateZonePairSearchCriteria(idPair); List rows = searchIncludingRemoved(sc, searchFilter, null, false); if (rows != null) { uvList.addAll(rows); @@ -655,7 +655,7 @@ public List searchByTemplateZonePair(Boolean showRemoved, String return uvList; } - private SearchCriteria applyPairWhereClauseHackyWorkaround(String idPair) { + private SearchCriteria buildTemplateZonePairSearchCriteria(String idPair) { if (idPair == null || idPair.isEmpty()) { throw new IllegalArgumentException("template zone pair id is null or empty"); } @@ -821,8 +821,8 @@ private void appendCommonWhere(StringBuilder where, List params, Templat if (filter.format != null) { // searchForTemplatesInternal: format = 'ISO' for isos, format != 'ISO' otherwise. - where.append(filter.isIso ? " AND vt.format = ?" : " AND vt.format != ?"); - params.add(filter.format.toString()); + String condition = filter.isIso ? "=" : "!="; + where.append(" AND vt.format " + condition + " 'ISO'"); } if (filter.hypervisorType != null) {