Fix listing service offerings with different host tags#12919
Fix listing service offerings with different host tags#12919nvazquez wants to merge 1 commit intoapache:4.22from
Conversation
|
@blueorangutan package |
|
@nvazquez a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.22 #12919 +/- ##
=========================================
Coverage 17.60% 17.60%
- Complexity 15676 15678 +2
=========================================
Files 5918 5918
Lines 531667 531697 +30
Branches 65001 65005 +4
=========================================
+ Hits 93617 93631 +14
- Misses 427491 427502 +11
- Partials 10559 10564 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17294 |
There was a problem hiding this comment.
Pull request overview
This PR updates service offering listing logic (used for VM scaling) so that offerings with different host tags are also shown, by expanding the tag filter to include tags present on hosts in the VM’s current cluster and by loosening host-tag matching to OR semantics.
Changes:
- Extend service offering filtering to include host tags from the VM’s current cluster when listing scale-eligible offerings.
- Change host-tag matching from requiring all tags (AND) to allowing any matching tag (OR), while still including untagged offerings.
- Add unit tests for collecting cluster host tags into the VM’s host tag filter.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
server/src/main/java/com/cloud/api/query/QueryManagerImpl.java |
Expands host-tag filtering for service offering listing and adds helper to include cluster tags. |
engine/schema/src/main/java/com/cloud/host/dao/HostTagsDao.java |
Adds DAO API to list host tags by cluster ID. |
engine/schema/src/main/java/com/cloud/host/dao/HostTagsDaoImpl.java |
Implements listing tags by cluster via host IDs + custom search. |
server/src/test/java/com/cloud/api/query/QueryManagerImplTest.java |
Adds tests for the new helper that augments host tags with current cluster tags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SearchCriteria<String> sc = tagSearch.create(); | ||
| sc.setParameters("idIN", hostIds.toArray()); |
There was a problem hiding this comment.
listByClusterId is filtering HostTagVO by the tag record primary key (getId()) instead of the host foreign key (getHostId()). Since you pass host IDs from hostDao.listIdsByClusterId, this will return incorrect results (often empty, or wrong tags if IDs happen to overlap). Update the search builder/criteria to use host_id IN (...) (and ideally select DISTINCT tags).
| SearchCriteria<String> sc = tagSearch.create(); | |
| sc.setParameters("idIN", hostIds.toArray()); | |
| if (hostIds == null || hostIds.isEmpty()) { | |
| return new ArrayList<>(); | |
| } | |
| GenericSearchBuilder<HostTagVO, String> sb = createSearchBuilder(String.class); | |
| sb.select(null, Func.DISTINCT, sb.entity().getTag()); | |
| sb.and("hostIdIN", sb.entity().getHostId(), SearchCriteria.Op.IN); | |
| sb.done(); | |
| SearchCriteria<String> sc = sb.create(); | |
| sc.setParameters("hostIdIN", hostIds.toArray()); |
| public List<String> listByClusterId(Long clusterId) { | ||
| List<Long> hostIds = hostDao.listIdsByClusterId(clusterId); | ||
| SearchCriteria<String> sc = tagSearch.create(); | ||
| sc.setParameters("idIN", hostIds.toArray()); | ||
| return customSearch(sc, null); | ||
| } |
There was a problem hiding this comment.
listByClusterId should defensively handle an empty host list from hostDao.listIdsByClusterId(clusterId). Passing an empty array into an IN criterion can produce invalid SQL (or at best an unnecessary query). Consider returning an empty list early when hostIds is null/empty, and ensure the query returns distinct tags to avoid duplicates from multiple hosts.
| } | ||
| Set<String> existingTagsSet = new HashSet<>(hostTags); | ||
| clusterTags.stream() | ||
| .filter(tag -> !existingTagsSet.contains(tag)) |
There was a problem hiding this comment.
addVmCurrentClusterHostTags can add duplicate tags if clusterTags contains repeats (e.g., multiple hosts in the cluster share the same tag). This can cause redundant/duplicated hostTag... criteria names later when building the search (and at minimum makes the query larger than needed). Consider de-duplicating clusterTags (e.g., distinct() / Set) and/or updating existingTagsSet as tags are added.
| .filter(tag -> !existingTagsSet.contains(tag)) | |
| .filter(tag -> !existingTagsSet.contains(tag)) | |
| .peek(existingTagsSet::add) |
| } | ||
| List<String> clusterTags = _hostTagDao.listByClusterId(host.getClusterId()); | ||
| if (CollectionUtils.isEmpty(clusterTags)) { | ||
| logger.warn("No host tags defined for hosts in the cluster " + host.getClusterId()); |
There was a problem hiding this comment.
Logging "No host tags defined for hosts in the cluster ..." at WARN is likely to be noisy in normal environments (many clusters/hosts may be intentionally untagged, and this method can be invoked frequently when listing offerings). Consider lowering this to DEBUG/INFO, or logging only when host tags are actually required for the request.
| logger.warn("No host tags defined for hosts in the cluster " + host.getClusterId()); | |
| logger.debug("No host tags defined for hosts in the cluster " + host.getClusterId()); |
Description
This PR fixes the listing of service offerings for VMs, including offerings that contain different host tags. Previously only offerings without any host tag or offerings matching exactly the same VM offering host tags were displayed
Fixes: #11407
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?