Copilot commented on code in PR #12919:
URL: https://github.com/apache/cloudstack/pull/12919#discussion_r3012350826
##########
server/src/main/java/com/cloud/api/query/QueryManagerImpl.java:
##########
@@ -4486,6 +4487,30 @@ private Pair<List<Long>, Integer>
searchForServiceOfferingIdsAndCount(ListServic
return new Pair<>(offeringIds, count);
}
+ protected void addVmCurrentClusterHostTags(VMInstanceVO vmInstance,
List<String> hostTags) {
+ if (vmInstance == null) {
+ return;
+ }
+ Long hostId = vmInstance.getHostId() == null ?
vmInstance.getLastHostId() : vmInstance.getHostId();
+ if (hostId == null) {
+ return;
+ }
+ HostVO host = hostDao.findById(hostId);
+ if (host == null) {
+ logger.warn("Unable to find host with id " + hostId);
+ return;
+ }
+ List<String> clusterTags =
_hostTagDao.listByClusterId(host.getClusterId());
+ if (CollectionUtils.isEmpty(clusterTags)) {
+ logger.warn("No host tags defined for hosts in the cluster " +
host.getClusterId());
Review 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.
```suggestion
logger.debug("No host tags defined for hosts in the cluster " +
host.getClusterId());
```
##########
server/src/main/java/com/cloud/api/query/QueryManagerImpl.java:
##########
@@ -4486,6 +4487,30 @@ private Pair<List<Long>, Integer>
searchForServiceOfferingIdsAndCount(ListServic
return new Pair<>(offeringIds, count);
}
+ protected void addVmCurrentClusterHostTags(VMInstanceVO vmInstance,
List<String> hostTags) {
+ if (vmInstance == null) {
+ return;
+ }
+ Long hostId = vmInstance.getHostId() == null ?
vmInstance.getLastHostId() : vmInstance.getHostId();
+ if (hostId == null) {
+ return;
+ }
+ HostVO host = hostDao.findById(hostId);
+ if (host == null) {
+ logger.warn("Unable to find host with id " + hostId);
+ return;
+ }
+ List<String> clusterTags =
_hostTagDao.listByClusterId(host.getClusterId());
+ if (CollectionUtils.isEmpty(clusterTags)) {
+ logger.warn("No host tags defined for hosts in the cluster " +
host.getClusterId());
+ return;
+ }
+ Set<String> existingTagsSet = new HashSet<>(hostTags);
+ clusterTags.stream()
+ .filter(tag -> !existingTagsSet.contains(tag))
Review 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.
```suggestion
.filter(tag -> !existingTagsSet.contains(tag))
.peek(existingTagsSet::add)
```
##########
engine/schema/src/main/java/com/cloud/host/dao/HostTagsDaoImpl.java:
##########
@@ -235,4 +244,12 @@ public List<HostTagVO> searchByIds(Long... tagIds) {
return tagList;
}
+
+ @Override
+ public List<String> listByClusterId(Long clusterId) {
+ List<Long> hostIds = hostDao.listIdsByClusterId(clusterId);
+ SearchCriteria<String> sc = tagSearch.create();
+ sc.setParameters("idIN", hostIds.toArray());
Review 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).
```suggestion
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());
```
##########
engine/schema/src/main/java/com/cloud/host/dao/HostTagsDaoImpl.java:
##########
@@ -235,4 +244,12 @@ public List<HostTagVO> searchByIds(Long... tagIds) {
return tagList;
}
+
+ @Override
+ 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);
+ }
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
