Copilot commented on code in PR #12014:
URL: https://github.com/apache/cloudstack/pull/12014#discussion_r2503510633
##########
plugins/drs/cluster/balanced/src/main/java/org/apache/cloudstack/cluster/Balanced.java:
##########
@@ -68,23 +68,26 @@ public String getName() {
return "balanced";
}
+
@Override
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)",
Review Comment:
The log level changed from `debug` to `trace` for the imbalance calculation
messages. While this reduces log verbosity, it makes debugging DRS issues more
difficult since trace logs are typically not enabled in production. Consider
keeping these as debug level or making the log level configurable.
```suggestion
logger.debug("Cluster {} pre-imbalance: {} post-imbalance: {}
Algorithm: {} VM: {} srcHost ID: {} destHost: {} (optimized)",
```
##########
server/src/main/java/com/cloud/server/ManagementServerImpl.java:
##########
@@ -1571,22 +1585,40 @@ public Ternary<Pair<List<? extends Host>, Integer>,
List<? extends Host>, Map<Ho
allHostsPair = searchForServers(startIndex, pageSize, null,
hostType, null, null, null, cluster, null, keyword, null, null, null,
null, srcHost.getId());
allHosts = allHostsPair.first();
- plan = new DataCenterDeployment(srcHost.getDataCenterId(),
srcHost.getPodId(), srcHost.getClusterId(), null, null, null);
+ filteredHosts = allHosts;
}
- final Pair<List<? extends Host>, Integer> otherHosts = new
Pair<>(allHosts, allHostsPair.second());
+ final Pair<List<? extends Host>, Integer> allHostsPairResult = new
Pair<>(allHosts, allHostsPair.second());
Pair<Boolean, List<HostVO>> uefiFilteredResult =
filterUefiHostsForMigration(allHosts, filteredHosts, vm);
if (!uefiFilteredResult.first()) {
- return new Ternary<>(otherHosts, new ArrayList<>(), new
HashMap<>());
+ return new Ternary<Pair<List<? extends Host>, Integer>, List<?
extends Host>, Map<Host, Boolean>>(
+ allHostsPairResult, new ArrayList<>(), new HashMap<>());
}
filteredHosts = uefiFilteredResult.second();
- List<Host> suitableHosts = new ArrayList<>();
+ return new Ternary<Pair<List<? extends Host>, Integer>, List<? extends
Host>, Map<Host, Boolean>>(
+ allHostsPairResult, filteredHosts, requiresStorageMotion);
+ }
+
+ /**
+ * Apply affinity group constraints and other exclusion rules for VM
migration.
+ * This builds an ExcludeList based on affinity groups, DPDK requirements,
and dedicated resources.
+ *
+ * @param vm The virtual machine to migrate
+ * @param vmProfile The VM profile
+ * @param plan The deployment plan
+ * @param vmList List of VMs with current/simulated placements for
affinity processing
+ * @return ExcludeList containing hosts to avoid
+ */
+ @Override
+ public ExcludeList applyAffinityConstraints(VirtualMachine vm,
VirtualMachineProfile vmProfile, DeploymentPlan plan, List<VirtualMachine>
vmList) {
final ExcludeList excludes = new ExcludeList();
- excludes.addHost(srcHostId);
+ excludes.addHost(vm.getHostId());
if (dpdkHelper.isVMDpdkEnabled(vm.getId())) {
- excludeNonDPDKEnabledHosts(plan, excludes);
+ if (plan instanceof DataCenterDeployment) {
+ excludeNonDPDKEnabledHosts((DataCenterDeployment) plan,
excludes);
+ }
}
Review Comment:
The method `applyAffinityConstraints` performs a type check and cast for
`DeploymentPlan` to `DataCenterDeployment`. If the plan is not an instance of
`DataCenterDeployment`, the DPDK exclusion is silently skipped. This could lead
to incorrect host selection for DPDK-enabled VMs. Consider either requiring
`DataCenterDeployment` as the parameter type, or throwing an exception/logging
a warning when the cast fails for DPDK VMs.
##########
server/src/test/java/com/cloud/server/ManagementServerImplTest.java:
##########
@@ -123,15 +153,60 @@ public class ManagementServerImplTest {
@Mock
UserDataManager userDataManager;
- @Spy
- ManagementServerImpl spy = new ManagementServerImpl();
-
@Mock
UserVmDetailsDao userVmDetailsDao;
@Mock
HostDetailsDao hostDetailsDao;
+
+ @Mock
+ VMInstanceDao vmInstanceDao;
+
+ @Mock
+ HostDao hostDao;
+
+ @Mock
+ ServiceOfferingDetailsDao serviceOfferingDetailsDao;
+
+ @Mock
+ VolumeDao volumeDao;
+
+ @Mock
+ ServiceOfferingDao offeringDao;
+
+ @Mock
+ DiskOfferingDao diskOfferingDao;
+
+ @Mock
+ HypervisorCapabilitiesDao hypervisorCapabilitiesDao;
+
+ @Mock
+ DataStoreManager dataStoreManager;
+
+ @Mock
+ PrimaryDataStoreDao primaryDataStoreDao;
+
+ @Mock
+ DpdkHelper dpdkHelper;
+
+ @Mock
+ AffinityGroupVMMapDao affinityGroupVMMapDao;
+
+ @Mock
+ DeploymentPlanningManager dpMgr;
+
+ @Mock
+ DataCenterDao dcDao;
+
+ @Mock
+ HostAllocator hostAllocator;
+
+ @InjectMocks
+ @Spy
+ ManagementServerImpl spy;
Review Comment:
In the test setup, the `ManagementServerImpl spy` field is moved from a
simple `@Spy` annotation to using `@InjectMocks` together with `@Spy`. This
change means Mockito will attempt to inject all `@Mock` fields into the spy.
However, the manual field assignments in the `setup()` method (lines 215-224)
may conflict with `@InjectMocks` auto-injection, potentially causing test
instability or unexpected behavior. Consider removing the manual assignments if
`@InjectMocks` handles them, or document why both are needed.
##########
api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java:
##########
@@ -39,6 +38,10 @@
import static
org.apache.cloudstack.cluster.ClusterDrsService.ClusterDrsMetricUseRatio;
public interface ClusterDrsAlgorithm extends Adapter {
+ // Reusable stateless calculator objects (thread-safe) - created once,
reused for all calculations
+ // Apache Commons Math Mean and StandardDeviation are stateless and
thread-safe
+ Mean MEAN_CALCULATOR = new Mean();
+ StandardDeviation STDDEV_CALCULATOR = new StandardDeviation(false);
Review Comment:
The `MEAN_CALCULATOR` and `STDDEV_CALCULATOR` are declared as public
interface constants. While they're documented as thread-safe and stateless,
sharing mutable calculator objects across all implementations could be a
concurrency risk if the Apache Commons Math library implementation changes in
the future. Consider making these private to each implementation class or
documenting the specific version dependency on Apache Commons Math that
guarantees thread-safety.
```suggestion
// Calculator objects should be instantiated privately in each
implementation class to avoid shared mutable state.
```
##########
api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java:
##########
@@ -59,79 +62,118 @@ public interface ClusterDrsAlgorithm extends Adapter {
boolean needsDrs(Cluster cluster, List<Ternary<Long, Long, Long>> cpuList,
List<Ternary<Long, Long, Long>> memoryList) throws
ConfigurationException;
-
/**
- * Determines the metrics for a given virtual machine and destination host
in a DRS cluster.
- *
- * @param clusterId
- * the ID of the cluster to check
- * @param vm
- * the virtual machine to check
- * @param serviceOffering
- * the service offering for the virtual machine
- * @param destHost
- * the destination host for the virtual machine
- * @param hostCpuMap
- * a map of host IDs to the Ternary of used, reserved and total
CPU on each host
- * @param hostMemoryMap
- * a map of host IDs to the Ternary of used, reserved and total
memory on each host
- * @param requiresStorageMotion
- * whether storage motion is required for the virtual machine
+ * Calculates the metrics (improvement, cost, benefit) for migrating a VM
to a destination host. Improvement is
+ * calculated based on the change in cluster imbalance before and after
the migration.
*
+ * @param cluster the cluster to check
+ * @param vm the virtual machine to check
+ * @param serviceOffering the service offering for the virtual machine
+ * @param destHost the destination host for the virtual machine
+ * @param hostCpuMap a map of host IDs to the Ternary of used, reserved
and total CPU on each host
+ * @param hostMemoryMap a map of host IDs to the Ternary of used, reserved
and total memory on each host
+ * @param requiresStorageMotion whether storage motion is required for the
virtual machine
+ * @param preImbalance the pre-calculated cluster imbalance before
migration (null to calculate it)
+ * @param baseMetricsArray pre-calculated array of all host metrics before
migration
+ * @param hostIdToIndexMap mapping from host ID to index in the metrics
array
* @return a ternary containing improvement, cost, benefit
*/
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;
+ Boolean requiresStorageMotion, Double preImbalance,
+ double[] baseMetricsArray, Map<Long, Integer> hostIdToIndexMap)
throws ConfigurationException;
/**
- * Calculates the imbalance of the cluster after a virtual machine
migration.
- *
- * @param serviceOffering
- * the service offering for the virtual machine
- * @param vm
- * the virtual machine being migrated
- * @param destHost
- * the destination host for the virtual machine
- * @param hostCpuMap
- * a map of host IDs to the Ternary of used, reserved and total
CPU on each host
- * @param hostMemoryMap
- * a map of host IDs to the Ternary of used, reserved and total
memory on each host
+ * Calculates the cluster imbalance after migrating a VM to a destination
host.
*
- * @return a pair containing the CPU and memory imbalance of the cluster
after the migration
+ * @param vm the virtual machine being migrated
+ * @param destHost the destination host for the virtual machine
+ * @param clusterId the cluster ID
+ * @param vmMetric the VM's resource consumption metric
+ * @param baseMetricsArray pre-calculated array of all host metrics before
migration
+ * @param hostIdToIndexMap mapping from host ID to index in the metrics
array
+ * @return the cluster imbalance after migration
*/
- default Double getImbalancePostMigration(ServiceOffering serviceOffering,
VirtualMachine vm,
- Host destHost, Map<Long, Ternary<Long, Long, Long>> hostCpuMap,
- Map<Long, Ternary<Long, Long, Long>> hostMemoryMap) throws
ConfigurationException {
- Pair<Long, Map<Long, Ternary<Long, Long, Long>>> pair =
getHostMetricsMapAndType(destHost.getClusterId(), serviceOffering, hostCpuMap,
hostMemoryMap);
- long vmMetric = pair.first();
- Map<Long, Ternary<Long, Long, Long>> hostMetricsMap = pair.second();
+ default Double getImbalancePostMigration(VirtualMachine vm,
+ Host destHost, Long clusterId, long vmMetric, double[]
baseMetricsArray,
+ Map<Long, Integer> hostIdToIndexMap, Map<Long, Ternary<Long, Long,
Long>> hostCpuMap,
+ Map<Long, Ternary<Long, Long, Long>> hostMemoryMap) {
+ // Create a copy of the base array and adjust only the two affected
hosts
+ double[] adjustedMetrics = new double[baseMetricsArray.length];
+ System.arraycopy(baseMetricsArray, 0, adjustedMetrics, 0,
baseMetricsArray.length);
- List<Double> list = new ArrayList<>();
- for (Long hostId : hostMetricsMap.keySet()) {
- list.add(getMetricValuePostMigration(destHost.getClusterId(),
hostMetricsMap.get(hostId), vmMetric, hostId, destHost.getId(),
vm.getHostId()));
+ long destHostId = destHost.getId();
+ long vmHostId = vm.getHostId();
+
+ // Adjust source host (remove VM resources)
+ Integer sourceIndex = hostIdToIndexMap.get(vmHostId);
+ if (sourceIndex != null && sourceIndex < adjustedMetrics.length) {
+ Map<Long, Ternary<Long, Long, Long>> sourceMetricsMap =
getClusterDrsMetric(clusterId).equals("cpu") ? hostCpuMap : hostMemoryMap;
+ Ternary<Long, Long, Long> sourceMetrics =
sourceMetricsMap.get(vmHostId);
+ if (sourceMetrics != null) {
+ adjustedMetrics[sourceIndex] =
getMetricValuePostMigration(clusterId, sourceMetrics, vmMetric, vmHostId,
destHostId, vmHostId);
+ }
}
- return getImbalance(list);
+
+ // Adjust destination host (add VM resources)
+ Integer destIndex = hostIdToIndexMap.get(destHostId);
+ if (destIndex != null && destIndex < adjustedMetrics.length) {
+ Map<Long, Ternary<Long, Long, Long>> destMetricsMap =
getClusterDrsMetric(clusterId).equals("cpu") ? hostCpuMap : hostMemoryMap;
+ Ternary<Long, Long, Long> destMetrics =
destMetricsMap.get(destHostId);
+ if (destMetrics != null) {
+ adjustedMetrics[destIndex] =
getMetricValuePostMigration(clusterId, destMetrics, vmMetric, destHostId,
destHostId, vmHostId);
+ }
+ }
Review Comment:
In the optimized imbalance calculation, if `sourceIndex` or `destIndex` are
equal to `adjustedMetrics.length` (not just greater), the bounds check `<
adjustedMetrics.length` will pass, but this would be accessing the last valid
index when the index equals the length. However, array indices are 0-based, so
an array of length N has valid indices 0 to N-1. The condition is correct as
written, but the comment or code clarity could be improved. This is actually
correct - no bug here, just noting for clarity.
##########
api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java:
##########
@@ -59,79 +62,118 @@ public interface ClusterDrsAlgorithm extends Adapter {
boolean needsDrs(Cluster cluster, List<Ternary<Long, Long, Long>> cpuList,
List<Ternary<Long, Long, Long>> memoryList) throws
ConfigurationException;
-
/**
- * Determines the metrics for a given virtual machine and destination host
in a DRS cluster.
- *
- * @param clusterId
- * the ID of the cluster to check
- * @param vm
- * the virtual machine to check
- * @param serviceOffering
- * the service offering for the virtual machine
- * @param destHost
- * the destination host for the virtual machine
- * @param hostCpuMap
- * a map of host IDs to the Ternary of used, reserved and total
CPU on each host
- * @param hostMemoryMap
- * a map of host IDs to the Ternary of used, reserved and total
memory on each host
- * @param requiresStorageMotion
- * whether storage motion is required for the virtual machine
+ * Calculates the metrics (improvement, cost, benefit) for migrating a VM
to a destination host. Improvement is
+ * calculated based on the change in cluster imbalance before and after
the migration.
*
+ * @param cluster the cluster to check
+ * @param vm the virtual machine to check
+ * @param serviceOffering the service offering for the virtual machine
+ * @param destHost the destination host for the virtual machine
+ * @param hostCpuMap a map of host IDs to the Ternary of used, reserved
and total CPU on each host
+ * @param hostMemoryMap a map of host IDs to the Ternary of used, reserved
and total memory on each host
+ * @param requiresStorageMotion whether storage motion is required for the
virtual machine
+ * @param preImbalance the pre-calculated cluster imbalance before
migration (null to calculate it)
+ * @param baseMetricsArray pre-calculated array of all host metrics before
migration
+ * @param hostIdToIndexMap mapping from host ID to index in the metrics
array
* @return a ternary containing improvement, cost, benefit
*/
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;
+ Boolean requiresStorageMotion, Double preImbalance,
+ double[] baseMetricsArray, Map<Long, Integer> hostIdToIndexMap)
throws ConfigurationException;
Review Comment:
The new signature for `getMetrics` adds three parameters (`preImbalance`,
`baseMetricsArray`, `hostIdToIndexMap`) for performance optimization. However,
the interface change is a breaking change for any existing implementations of
`ClusterDrsAlgorithm`. Consider whether this should be added as a new
overloaded method with a default implementation that calls the optimized
version, to maintain backward compatibility.
##########
server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java:
##########
@@ -372,8 +466,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:
There's a duplicate line that removes and adds VMs to the host map. Line 469
and 470 are identical - one should be removed. This appears to be a copy-paste
error.
```suggestion
```
--
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]
