Repository navigation
Improve performance of listTemplates API #13566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 4.22
Are you sure you want to change the base?
Changes from all commits
3fd789b
cf6beb4
54d4392
222cf25
21955d9
42fe302
79dbb93
0f81e12
37c52a1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -207,6 +207,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; | ||
|
|
@@ -328,6 +329,7 @@ | |
| import com.cloud.storage.dao.VolumeDao; | ||
| import com.cloud.tags.ResourceTagVO; | ||
| import com.cloud.tags.dao.ResourceTagDao; | ||
| import com.cloud.template.VirtualMachineTemplate; | ||
| import com.cloud.template.VirtualMachineTemplate.State; | ||
| import com.cloud.template.VirtualMachineTemplate.TemplateFilter; | ||
| import com.cloud.user.Account; | ||
|
|
@@ -5146,7 +5148,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, templateType, isVnf, forCks)); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -5201,7 +5207,8 @@ protected boolean checkIfDomainSharesTemplates(Long domainId) { | |
| private Pair<List<TemplateJoinVO>, Integer> templateChecks(boolean isIso, List<HypervisorType> hypers, Map<String, String> 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<TemplateJoinVO> sc) { | ||
| Filter searchFilter, SearchCriteria<TemplateJoinVO> sc, | ||
| TemplateListFilter listFilter) { | ||
| if (!isIso) { | ||
| // add hypervisor criteria for template case | ||
| if (hypers != null && !hypers.isEmpty()) { | ||
|
|
@@ -5308,6 +5315,14 @@ private Pair<List<TemplateJoinVO>, Integer> templateChecks(boolean isIso, List<H | |
|
|
||
| // search unique templates and find details by Ids | ||
| Pair<List<TemplateJoinVO>, 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 { | ||
|
|
@@ -5329,6 +5344,122 @@ private Pair<List<TemplateJoinVO>, Integer> templateChecks(boolean isIso, List<H | |
| // about what the code was doing. | ||
| } | ||
|
|
||
| /** | ||
| * Build the immutable filter the bypass-the-view DAO consumes. Mirrors the | ||
| * SearchCriteria construction in {@link #searchForTemplatesInternal}; the | ||
| * SearchBuilder path is unchanged. The "hard" markers (sharedAccountIds, | ||
| * domainIdsForFeaturedCommunity, tags, requiresViewFallback) drive | ||
| * {@link TemplateListFilter#canBypass()} — when any are populated, | ||
| * {@code templateChecks} routes to the legacy view-based path instead. | ||
| */ | ||
| private TemplateListFilter buildTemplateListFilter(Long templateId, List<Long> ids, String name, String keyword, | ||
| TemplateFilter templateFilter, boolean isIso, Boolean bootable, | ||
| Long pageSize, Long startIndex, Long zoneId, HypervisorType hyperType, | ||
| List<HypervisorType> hypers, boolean showDomr, boolean onlyReady, | ||
| List<Account> permittedAccounts, Account caller, | ||
| ListProjectResourcesCriteria listProjectResourcesCriteria, | ||
| Map<String, String> tags, boolean showRemovedTmpl, Long parentTemplateId, | ||
| Boolean showUnique, String templateType, Boolean isVnf, Boolean forCks) { | ||
| TemplateListFilter.Builder b = TemplateListFilter.builder() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should the fast path also handle the arch, os category, extension, storage and image store filters? looks like it drops them and returns every template |
||
| .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) | ||
| .templateType(templateType) | ||
| .isVnf(isVnf) | ||
| .forCks(forCks) | ||
| .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<Long> 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) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what happens if an admin passes domainid or isrecursive=false with templatefilter=self? looks like the fast path ignores both |
||
| 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<List<TemplateJoinVO>, Integer> findTemplatesByIdOrTempZonePair(Pair<List<TemplateJoinVO>, Integer> templateDataPair, | ||
| boolean showRemoved, boolean showUnique, Account caller) { | ||
|
|
@@ -6342,7 +6473,8 @@ public String getConfigComponentName() { | |
| @Override | ||
| public ConfigKey<?>[] getConfigKeys() { | ||
| return new ConfigKey<?>[] {AllowUserViewDestroyedVM, UserVMDeniedDetails, UserVMReadOnlyDetails, SortKeyAscending, | ||
| AllowUserViewAllDomainAccounts, AllowUserViewAllDataCenters, SharePublicTemplatesWithOtherDomains, ReturnVmStatsOnVmList}; | ||
| AllowUserViewAllDomainAccounts, AllowUserViewAllDataCenters, SharePublicTemplatesWithOtherDomains, | ||
| ReturnVmStatsOnVmList, BypassTemplateView }; | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: the concatenated description string has inconsistent leading/trailing space placement across the lines, worth double-checking the rendered text doesn't end up with a missing or doubled space.