Copilot commented on code in PR #12994:
URL: https://github.com/apache/cloudstack/pull/12994#discussion_r3062659963
##########
server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java:
##########
@@ -134,6 +136,9 @@ public class ClusterDrsServiceImpl extends ManagerBase
implements ClusterDrsServ
@Inject
ServiceOfferingDao serviceOfferingDao;
+ @Inject
+ UserVmDetailsDao userVmdetailsDao;
Review Comment:
Field name `userVmdetailsDao` has inconsistent camel-casing (Details vs
details). Rename to `userVmDetailsDao` to match `UserVmDetailsDao` and typical
project conventions; this also reduces the chance of typos when referenced.
```suggestion
UserVmDetailsDao userVmDetailsDao;
```
##########
server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java:
##########
@@ -633,12 +635,20 @@ Pair<VirtualMachine, Host> getBestMigration(Cluster
cluster, ClusterDrsAlgorithm
return bestMigration;
}
- private boolean skipDrs(VirtualMachine vm, List<? extends Host>
compatibleHosts, ServiceOffering serviceOffering) {
+ private boolean shouldSkipVMForDRS(VirtualMachine vm) {
if (vm.getType().isUsedBySystem() || vm.getState() !=
VirtualMachine.State.Running) {
return true;
}
- if (MapUtils.isNotEmpty(vm.getDetails()) &&
-
"true".equalsIgnoreCase(vm.getDetails().get(VmDetailConstants.SKIP_DRS))) {
+
+ UserVmDetailVO skipDrsDetail = userVmdetailsDao.findDetail(vm.getId(),
VmDetailConstants.SKIP_DRS);
+ if (skipDrsDetail != null &&
"true".equalsIgnoreCase(skipDrsDetail.getValue())) {
+ return true;
+ }
Review Comment:
`findDetail` is executed every time `shouldSkipVMForDRS` is called; note
this method is now used both in the main VM iteration and again in `skipDrs`,
which can cause duplicate DB reads for the same VM within one DRS run. Consider
caching the result per VM ID for the duration of the DRS operation (or
refactoring call sites so the skip decision is computed once and passed along)
to avoid repeated lookups.
##########
server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java:
##########
@@ -477,10 +482,7 @@ private Pair<Map<Long, List<? extends Host>>, Map<Long,
Map<Host, Boolean>>> get
for (VirtualMachine vm : vmList) {
// Skip ineligible VMs
- if (vm.getType().isUsedBySystem() ||
- vm.getState() != VirtualMachine.State.Running ||
- (MapUtils.isNotEmpty(vm.getDetails()) &&
-
"true".equalsIgnoreCase(vm.getDetails().get(VmDetailConstants.SKIP_DRS)))) {
+ if (shouldSkipVMForDRS(vm)) {
continue;
}
Review Comment:
`shouldSkipVMForDRS` performs a DB lookup (`findDetail`) and is called
inside the `vmList` loop, which can introduce an N+1 query pattern for large
clusters. Consider prefetching `SKIP_DRS` details for all VM IDs in a single
query (or building a `Map<vmId, skip>` once per DRS run) and then using that
cache in `shouldSkipVMForDRS`.
--
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]
