Copilot commented on code in PR #12014:
URL: https://github.com/apache/cloudstack/pull/12014#discussion_r2502636312
##########
server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java:
##########
@@ -387,18 +861,42 @@ public void testGetBestMigration() throws
ConfigurationException {
vmIdServiceOfferingMap.put(vm.getId(), serviceOffering);
}
- Mockito.when(managementServer.listHostsForMigrationOfVM(vm1, 0L, 500L,
null, vmList)).thenReturn(
- new Ternary<>(new Pair<>(List.of(destHost), 1),
List.of(destHost), Map.of(destHost, false)));
- Mockito.when(managementServer.listHostsForMigrationOfVM(vm2, 0L, 500L,
null, vmList)).thenReturn(
- new Ternary<>(new Pair<>(List.of(destHost), 1),
List.of(destHost), Map.of(destHost, false)));
- Mockito.when(balancedAlgorithm.getMetrics(cluster, vm1,
serviceOffering, destHost, new HashMap<>(),
- new HashMap<>(), false)).thenReturn(new Ternary<>(1.0, 0.5,
1.5));
-
- Mockito.when(balancedAlgorithm.getMetrics(cluster, vm2,
serviceOffering, destHost, new HashMap<>(),
- new HashMap<>(), false)).thenReturn(new Ternary<>(1.0, 2.5,
1.5));
-
- Pair<VirtualMachine, Host> bestMigration =
clusterDrsService.getBestMigration(cluster, balancedAlgorithm,
- vmList, vmIdServiceOfferingMap, new HashMap<>(), new
HashMap<>());
+ // Create caches for the new method signature
+ Map<Long, List<? extends Host>> vmToCompatibleHostsCache = new
HashMap<>();
+ vmToCompatibleHostsCache.put(vm1.getId(), List.of(destHost));
+ vmToCompatibleHostsCache.put(vm2.getId(), List.of(destHost));
+
+ Map<Long, Map<Host, Boolean>> vmToStorageMotionCache = new HashMap<>();
+ vmToStorageMotionCache.put(vm1.getId(), Map.of(destHost, false));
+ vmToStorageMotionCache.put(vm2.getId(), Map.of(destHost, false));
+
+ Map<Long, com.cloud.deploy.DeploymentPlanner.ExcludeList>
vmToExcludesMap = new HashMap<>();
+ vmToExcludesMap.put(vm1.getId(),
Mockito.mock(com.cloud.deploy.DeploymentPlanner.ExcludeList.class));
+ vmToExcludesMap.put(vm2.getId(),
Mockito.mock(com.cloud.deploy.DeploymentPlanner.ExcludeList.class));
+
+ // Create capacity maps with dummy data for getClusterImbalance
(include both source and dest hosts)
+ Map<Long, Ternary<Long, Long, Long>> hostCpuCapacityMap = new
HashMap<>();
+ hostCpuCapacityMap.put(host.getId(), new Ternary<>(2000L, 0L, 3000L));
// Source host
+ hostCpuCapacityMap.put(destHost.getId(), new Ternary<>(1000L, 0L,
2000L)); // Dest host
+ Map<Long, Ternary<Long, Long, Long>> hostMemoryCapacityMap = new
HashMap<>();
+ hostMemoryCapacityMap.put(host.getId(), new Ternary<>(2L * 1024L *
1024L * 1024L, 0L, 3L * 1024L * 1024L * 1024L)); // Source host
+ hostMemoryCapacityMap.put(destHost.getId(), new Ternary<>(1024L *
1024L * 1024L, 0L, 2L * 1024L * 1024L * 1024L)); // Dest host
+
+ // Mock getMetrics for the optimized 10-parameter version used by
getBestMigration
+ // Return better improvement for vm1, worse for vm2
+ Mockito.doReturn(new Ternary<>(1.0, 0.5,
1.5)).when(balancedAlgorithm).getMetrics(
+ Mockito.eq(cluster), Mockito.eq(vm1),
Mockito.any(ServiceOffering.class),
+ Mockito.eq(destHost), Mockito.eq(hostCpuCapacityMap),
Mockito.eq(hostMemoryCapacityMap), Mockito.any(Boolean.class),
+ Mockito.any(Double.class), Mockito.any(double[].class),
Mockito.any(Map.class));
+ Mockito.doReturn(new Ternary<>(0.5, 2.5,
1.5)).when(balancedAlgorithm).getMetrics(
+ Mockito.eq(cluster), Mockito.eq(vm2),
Mockito.any(ServiceOffering.class),
+ Mockito.eq(destHost), Mockito.eq(hostCpuCapacityMap),
Mockito.eq(hostMemoryCapacityMap), Mockito.any(Boolean.class),
+ Mockito.any(Double.class), Mockito.any(double[].class),
Mockito.any(Map.class));
+
+
Review Comment:
Excessive blank line before method call. Line 896 has an extra blank line
that should be removed for code consistency.
```suggestion
```
##########
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(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 ID: {} destHost: {} (optimized)",
cluster, preImbalance, postImbalance, getName(), vm,
vm.getHostId(), destHost);
- // This needs more research to determine the cost and benefit of a
migration
- // TODO: Cost should be a factor of the VM size and the host capacity
- // TODO: Benefit should be a factor of the VM size and the host
capacity and the number of VMs on the host
- final double improvement = postImbalance - preImbalance;
- final double cost = 0;
- final double benefit = 1;
- return new Ternary<>(improvement, cost, benefit);
+ return calculateMetricsFromImbalances(postImbalance, preImbalance);
Review Comment:
Incorrect parameter order in `calculateMetricsFromImbalances`. The method is
called with `(postImbalance, preImbalance)` but based on the interface
definition and the logic in `calculateMetricsFromImbalances` (which calculates
`improvement = preImbalance - postImbalance`), the correct order should be
`(preImbalance, postImbalance)`. This will result in negative improvement
values when they should be positive.
```suggestion
return calculateMetricsFromImbalances(preImbalance, postImbalance);
```
--
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]
