Copilot commented on code in PR #12014:
URL: https://github.com/apache/cloudstack/pull/12014#discussion_r2501956196
##########
server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java:
##########
@@ -372,8 +462,6 @@ List<Ternary<VirtualMachine, Host, Host>>
getDrsPlan(Cluster cluster,
ServiceOffering serviceOffering =
vmIdServiceOfferingMap.get(vm.getId());
migrationPlan.add(new Ternary<>(vm, hostMap.get(vm.getHostId()),
hostMap.get(destHost.getId())));
- hostVmMap.get(vm.getHostId()).remove(vm);
- hostVmMap.get(destHost.getId()).add(vm);
hostVmMap.get(vm.getHostId()).remove(vm);
hostVmMap.get(destHost.getId()).add(vm);
Review Comment:
Duplicate line - this line appears twice consecutively (lines 465-466). One
should be removed.
```suggestion
```
##########
server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java:
##########
@@ -438,32 +532,104 @@ Pair<VirtualMachine, Host> getBestMigration(Cluster
cluster, ClusterDrsAlgorithm
List<VirtualMachine> vmList,
Map<Long, ServiceOffering> vmIdServiceOfferingMap,
Map<Long, Ternary<Long, Long, Long>> hostCpuCapacityMap,
- Map<Long, Ternary<Long, Long, Long>> hostMemoryCapacityMap) throws
ConfigurationException {
+ Map<Long, Ternary<Long, Long, Long>> hostMemoryCapacityMap,
+ Map<Long, List<? extends Host>> vmToCompatibleHostsCache,
+ Map<Long, Map<Host, Boolean>> vmToStorageMotionCache,
+ Map<Long, ExcludeList> vmToExcludesMap) throws
ConfigurationException {
+ // Pre-calculate cluster imbalance once per iteration (same for all
VM-host combinations)
+ Double preImbalance = getClusterImbalance(cluster.getId(),
+ new ArrayList<>(hostCpuCapacityMap.values()),
+ new ArrayList<>(hostMemoryCapacityMap.values()),
+ null);
+
+ // Pre-calculate base metrics array once per iteration for optimized
imbalance calculation
+ // This reduces complexity from O(V × H × H) to O(V × H) per iteration
+ String metricType = getClusterDrsMetric(cluster.getId());
+ Map<Long, Ternary<Long, Long, Long>> baseMetricsMap =
"cpu".equals(metricType) ? hostCpuCapacityMap : hostMemoryCapacityMap;
+ double[] baseMetricsArray = new double[baseMetricsMap.size()];
+ Map<Long, Integer> hostIdToIndexMap = new HashMap<>();
+
+ int index = 0;
+ for (Map.Entry<Long, Ternary<Long, Long, Long>> entry :
baseMetricsMap.entrySet()) {
+ Long hostId = entry.getKey();
+ Ternary<Long, Long, Long> metrics = entry.getValue();
+ long used = metrics.first();
+ long actualTotal = metrics.third() - metrics.second();
+ long free = actualTotal - metrics.first();
+ Double metricValue = getMetricValue(cluster.getId(), used, free,
actualTotal, null);
+ if (metricValue != null) {
+ baseMetricsArray[index] = metricValue;
+ hostIdToIndexMap.put(hostId, index);
+ index++;
+ }
+ }
+
+ // Trim array if some values were null
+ if (index < baseMetricsArray.length) {
+ double[] trimmed = new double[index];
+ System.arraycopy(baseMetricsArray, 0, trimmed, 0,
baseMetricsArray.length);
Review Comment:
Array index mismatch in arraycopy operation. The code uses
`baseMetricsArray.length` as the length parameter, but should use `index` since
we're copying to `trimmed` array. This will cause an
ArrayIndexOutOfBoundsException if any values were null.
Should be: `System.arraycopy(baseMetricsArray, 0, trimmed, 0, index);`
```suggestion
System.arraycopy(baseMetricsArray, 0, trimmed, 0, index);
```
##########
plugins/drs/cluster/condensed/src/main/java/org/apache/cloudstack/cluster/Condensed.java:
##########
@@ -75,20 +75,22 @@ public String getName() {
public Ternary<Double, Double, Double> getMetrics(Cluster cluster,
VirtualMachine vm,
ServiceOffering serviceOffering, Host destHost,
Map<Long, Ternary<Long, Long, Long>> hostCpuMap, Map<Long,
Ternary<Long, Long, Long>> hostMemoryMap,
- Boolean requiresStorageMotion) throws ConfigurationException {
- Double preImbalance =
ClusterDrsAlgorithm.getClusterImbalance(cluster.getId(), new
ArrayList<>(hostCpuMap.values()),
- new ArrayList<>(hostMemoryMap.values()), null);
- Double postImbalance = getImbalancePostMigration(serviceOffering, vm,
destHost, hostCpuMap, hostMemoryMap);
+ Boolean requiresStorageMotion, Double preImbalance,
+ double[] baseMetricsArray, Map<Long, Integer> hostIdToIndexMap)
throws ConfigurationException {
+ // Use provided pre-imbalance if available, otherwise calculate it
+ if (preImbalance == null) {
+ preImbalance =
ClusterDrsAlgorithm.getClusterImbalance(cluster.getId(), new
ArrayList<>(hostCpuMap.values()),
+ new ArrayList<>(hostMemoryMap.values()), null);
+ }
+
+ // Use optimized post-imbalance calculation that adjusts only affected
hosts
+ Double postImbalance = getImbalancePostMigration(serviceOffering, vm,
destHost,
+ cluster.getId(),
ClusterDrsAlgorithm.getVmMetric(serviceOffering, cluster.getId()),
+ baseMetricsArray, hostIdToIndexMap, hostCpuMap, hostMemoryMap);
- logger.debug("Cluster {} pre-imbalance: {} post-imbalance: {}
Algorithm: {} VM: {} srcHost: {} destHost: {}",
+ logger.trace("Cluster {} pre-imbalance: {} post-imbalance: {}
Algorithm: {} VM: {} srcHost: {} destHost: {} (optimized)",
cluster, preImbalance, postImbalance, getName(), vm,
vm.getHostId(), destHost);
Review Comment:
Incorrect variable name in log message. The log message says "(optimized)"
but the variable is named `metricValue` and should reference VM or host for
clarity in trace logs. Consider adding VM/host identifiers to the trace message
for better debugging.
```suggestion
logger.trace("Cluster {} pre-imbalance: {} post-imbalance: {}
Algorithm: {} VM ID: {} srcHost ID: {} destHost ID: {} (optimized)",
cluster, preImbalance, postImbalance, getName(), vm.getId(),
vm.getHostId(), destHost.getId());
```
##########
server/src/main/java/com/cloud/server/ManagementServerImpl.java:
##########
@@ -1923,7 +2016,7 @@ private List<StoragePool>
findAllSuitableStoragePoolsForDetachedVolume(Volume vo
return suitablePools;
}
- private Pair<List<HostVO>, Integer> searchForServers(final Long
startIndex, final Long pageSize, final Object name, final Object type,
+ Pair<List<HostVO>, Integer> searchForServers(final Long startIndex, final
Long pageSize, final Object name, final Object type,
Review Comment:
Method visibility changed from `private` to package-private (no modifier)
without clear justification. This expands the API surface. If this is
intentional for testing purposes, consider using `@VisibleForTesting`
annotation to document the intent.
--
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]
