You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by da...@apache.org on 2023/01/30 08:45:01 UTC
[cloudstack] branch 4.17 updated: Fix: memory leak on volume allocation (#7136)
This is an automated email from the ASF dual-hosted git repository.
dahn pushed a commit to branch 4.17
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/4.17 by this push:
new c78a777d3a0 Fix: memory leak on volume allocation (#7136)
c78a777d3a0 is described below
commit c78a777d3a01d072dca13fdfccbc0e29354c1e02
Author: Nicolas Vazquez <ni...@gmail.com>
AuthorDate: Mon Jan 30 05:44:50 2023 -0300
Fix: memory leak on volume allocation (#7136)
---
.../java/com/cloud/deploy/DeployDestination.java | 8 +--
.../src/main/java/com/cloud/host/dao/HostDao.java | 8 +++
.../main/java/com/cloud/host/dao/HostDaoImpl.java | 24 ++++++-
.../com/cloud/storage/VolumeApiServiceImpl.java | 13 +++-
.../cloud/storage/VolumeApiServiceImplTest.java | 32 +++++++++
test/integration/smoke/test_vm_life_cycle.py | 82 ++++++++++++++++++++++
6 files changed, 158 insertions(+), 9 deletions(-)
diff --git a/api/src/main/java/com/cloud/deploy/DeployDestination.java b/api/src/main/java/com/cloud/deploy/DeployDestination.java
index 91b2068e94b..a7ceddadba8 100644
--- a/api/src/main/java/com/cloud/deploy/DeployDestination.java
+++ b/api/src/main/java/com/cloud/deploy/DeployDestination.java
@@ -147,13 +147,7 @@ public class DeployDestination implements Serializable {
destination.append("Storage(");
if (displayStorage && _storage != null) {
StringBuffer storageBuf = new StringBuffer();
- //String storageStr = "";
for (Volume vol : _storage.keySet()) {
- if (!storageBuf.toString().equals("")) {
- storageBuf.append(storageBuf.toString());
- storageBuf.append(", ");
- }
- storageBuf.append(storageBuf);
storageBuf.append("Volume(");
storageBuf.append(vol.getId());
storageBuf.append("|");
@@ -162,7 +156,7 @@ public class DeployDestination implements Serializable {
storageBuf.append(_storage.get(vol).getId());
storageBuf.append(")");
}
- destination.append(storageBuf.toString());
+ destination.append(storageBuf);
}
return destination.append(")]").toString();
}
diff --git a/engine/schema/src/main/java/com/cloud/host/dao/HostDao.java b/engine/schema/src/main/java/com/cloud/host/dao/HostDao.java
index 54d4a5e9ca3..e1392bab541 100644
--- a/engine/schema/src/main/java/com/cloud/host/dao/HostDao.java
+++ b/engine/schema/src/main/java/com/cloud/host/dao/HostDao.java
@@ -149,4 +149,12 @@ public interface HostDao extends GenericDao<HostVO, Long>, StateDao<Status, Stat
* @return the number of hosts/agents this {@see ManagementServer} has responsibility over
*/
int countByMs(long msid);
+
+ /**
+ * Retrieves the hypervisor versions of the hosts in the datacenter which are in Up state in ascending order
+ * @param datacenterId data center id
+ * @param hypervisorType hypervisor type of the hosts
+ * @return ordered list of hypervisor versions
+ */
+ List<String> listOrderedHostsHypervisorVersionsInDatacenter(long datacenterId, HypervisorType hypervisorType);
}
diff --git a/engine/schema/src/main/java/com/cloud/host/dao/HostDaoImpl.java b/engine/schema/src/main/java/com/cloud/host/dao/HostDaoImpl.java
index 8e1f8f470dd..31958d418b1 100644
--- a/engine/schema/src/main/java/com/cloud/host/dao/HostDaoImpl.java
+++ b/engine/schema/src/main/java/com/cloud/host/dao/HostDaoImpl.java
@@ -90,7 +90,10 @@ public class HostDaoImpl extends GenericDaoBase<HostVO, Long> implements HostDao
"from vm_instance vm " +
"join host h on (vm.host_id=h.id) " +
"where vm.service_offering_id= ? and vm.state not in (\"Destroyed\", \"Expunging\", \"Error\") group by h.id";
-
+ private static final String GET_ORDERED_HW_VERSIONS_IN_DC = "select hypervisor_version from host " +
+ "where type = 'Routing' and status = 'Up' and hypervisor_type = ? and data_center_id = ? " +
+ "group by hypervisor_version " +
+ "order by hypervisor_version asc";
protected SearchBuilder<HostVO> TypePodDcStatusSearch;
@@ -1299,6 +1302,25 @@ public class HostDaoImpl extends GenericDaoBase<HostVO, Long> implements HostDao
return getCount(sc);
}
+ @Override
+ public List<String> listOrderedHostsHypervisorVersionsInDatacenter(long datacenterId, HypervisorType hypervisorType) {
+ PreparedStatement pstmt = null;
+ List<String> result = new ArrayList<>();
+ try {
+ TransactionLegacy txn = TransactionLegacy.currentTxn();
+ pstmt = txn.prepareAutoCloseStatement(GET_ORDERED_HW_VERSIONS_IN_DC);
+ pstmt.setString(1, Objects.toString(hypervisorType));
+ pstmt.setLong(2, datacenterId);
+ ResultSet resultSet = pstmt.executeQuery();
+ while (resultSet.next()) {
+ result.add(resultSet.getString(1));
+ }
+ } catch (SQLException e) {
+ s_logger.error("Error trying to obtain hypervisor version on datacenter", e);
+ }
+ return result;
+ }
+
@Override
public List<HostVO> listAllHostsByType(Host.Type type) {
SearchCriteria<HostVO> sc = TypeSearch.create();
diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
index 609e4e37206..4c166cfe41f 100644
--- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
+++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
@@ -4078,7 +4078,8 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
} else {
HypervisorType hypervisorType = vm.getHypervisorType();
if (hypervisorType != null && CollectionUtils.isNotEmpty(supportingDefaultHV) && supportingDefaultHV.contains(hypervisorType)) {
- maxDataVolumesSupported = _hypervisorCapabilitiesDao.getMaxDataVolumesLimit(hypervisorType, "default");
+ String hwVersion = getMinimumHypervisorVersionInDatacenter(vm.getDataCenterId(), hypervisorType);
+ maxDataVolumesSupported = _hypervisorCapabilitiesDao.getMaxDataVolumesLimit(hypervisorType, hwVersion);
}
}
if (maxDataVolumesSupported == null || maxDataVolumesSupported.intValue() <= 0) {
@@ -4090,6 +4091,16 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
return maxDataVolumesSupported.intValue();
}
+ protected String getMinimumHypervisorVersionInDatacenter(long datacenterId, HypervisorType hypervisorType) {
+ String defaultHypervisorVersion = "default";
+ if (hypervisorType == HypervisorType.Simulator) {
+ return defaultHypervisorVersion;
+ }
+ List<String> hwVersions = _hostDao.listOrderedHostsHypervisorVersionsInDatacenter(datacenterId, hypervisorType);
+ String minHwVersion = CollectionUtils.isNotEmpty(hwVersions) ? hwVersions.get(0) : defaultHypervisorVersion;
+ return StringUtils.isBlank(minHwVersion) ? defaultHypervisorVersion : minHwVersion;
+ }
+
private Long getDeviceId(UserVmVO vm, Long deviceId) {
// allocate deviceId
int maxDevices = getMaxDataVolumesSupported(vm) + 2; // add 2 to consider devices root volume and cdrom
diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
index 5b9875bc61e..5050a628246 100644
--- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
+++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
@@ -1197,4 +1197,36 @@ public class VolumeApiServiceImplTest {
boolean result = volumeApiServiceImpl.isNewDiskOfferingTheSameAndCustomServiceOffering(existingDiskOffering, newDiskOffering);
Assert.assertEquals(expectedResult, result);
}
+
+ private void testBaseListOrderedHostsHypervisorVersionInDc(List<String> hwVersions, HypervisorType hypervisorType,
+ String expected) {
+ when(_hostDao.listOrderedHostsHypervisorVersionsInDatacenter(anyLong(), any(HypervisorType.class)))
+ .thenReturn(hwVersions);
+ String min = volumeApiServiceImpl.getMinimumHypervisorVersionInDatacenter(1L, hypervisorType);
+ Assert.assertEquals(expected, min);
+ }
+
+ @Test
+ public void testGetMinimumHypervisorVersionInDatacenterSimulator() {
+ List<String> hwVersions = List.of("4.17.3.0-SNAPSHOT");
+ HypervisorType hypervisorType = HypervisorType.Simulator;
+ String expected = "default";
+ testBaseListOrderedHostsHypervisorVersionInDc(hwVersions, hypervisorType, expected);
+ }
+
+ @Test
+ public void testGetMinimumHypervisorVersionInDatacenterEmptyVersion() {
+ List<String> hwVersions = List.of("", "xxxx", "yyyy");
+ HypervisorType hypervisorType = HypervisorType.KVM;
+ String expected = "default";
+ testBaseListOrderedHostsHypervisorVersionInDc(hwVersions, hypervisorType, expected);
+ }
+
+ @Test
+ public void testGetMinimumHypervisorVersionInDatacenterVersions() {
+ List<String> hwVersions = List.of("6.7", "6.7.1", "6.7.2");
+ HypervisorType hypervisorType = HypervisorType.VMware;
+ String expected = "6.7";
+ testBaseListOrderedHostsHypervisorVersionInDc(hwVersions, hypervisorType, expected);
+ }
}
diff --git a/test/integration/smoke/test_vm_life_cycle.py b/test/integration/smoke/test_vm_life_cycle.py
index 0581a8b67c9..fb597903bd9 100644
--- a/test/integration/smoke/test_vm_life_cycle.py
+++ b/test/integration/smoke/test_vm_life_cycle.py
@@ -873,6 +873,88 @@ class TestVMLifeCycle(cloudstackTestCase):
self.assertEqual(Volume.list(self.apiclient, id=vol1.id), None, "List response contains records when it should not")
+ @attr(tags = ["devcloud", "advanced", "advancedns", "smoke", "basic", "sg"], required_hardware="false")
+ def test_12_start_vm_multiple_volumes_allocated(self):
+ """Test attaching multiple datadisks and start VM
+ """
+
+ # Validate the following
+ # 1. Deploys a VM without starting it and attaches multiple datadisks to it
+ # 2. Start VM successfully
+ # 3. Destroys the VM with DataDisks option
+
+ custom_disk_offering = DiskOffering.list(self.apiclient, name='Custom')[0]
+
+ # Create VM without starting it
+ vm = VirtualMachine.create(
+ self.apiclient,
+ self.services["small"],
+ accountid=self.account.name,
+ domainid=self.account.domainid,
+ serviceofferingid=self.small_offering.id,
+ startvm=False
+ )
+ self.cleanup.append(vm)
+
+ hosts = Host.list(
+ self.apiclient,
+ zoneid=self.zone.id,
+ type='Routing',
+ hypervisor=self.hypervisor,
+ state='Up')
+
+ if self.hypervisor.lower() in ["simulator"] or not hosts[0].hypervisorversion:
+ hypervisor_version = "default"
+ else:
+ hypervisor_version = hosts[0].hypervisorversion
+
+ res = self.dbclient.execute("select max_data_volumes_limit from hypervisor_capabilities where "
+ "hypervisor_type='%s' and hypervisor_version='%s';" %
+ (self.hypervisor.lower(), hypervisor_version))
+ if isinstance(res, list) and len(res) > 0:
+ max_volumes = res[0][0]
+ if max_volumes > 14:
+ max_volumes = 14
+ else:
+ max_volumes = 6
+
+ # Create and attach volumes
+ self.services["custom_volume"]["customdisksize"] = 1
+ self.services["custom_volume"]["zoneid"] = self.zone.id
+ for i in range(max_volumes):
+ volume = Volume.create_custom_disk(
+ self.apiclient,
+ self.services["custom_volume"],
+ account=self.account.name,
+ domainid=self.account.domainid,
+ diskofferingid=custom_disk_offering.id
+ )
+ self.cleanup.append(volume)
+ VirtualMachine.attach_volume(vm, self.apiclient, volume)
+
+ # Start the VM
+ self.debug("Starting VM - ID: %s" % vm.id)
+ vm.start(self.apiclient)
+ list_vm_response = VirtualMachine.list(
+ self.apiclient,
+ id=vm.id
+ )
+ self.assertEqual(
+ isinstance(list_vm_response, list),
+ True,
+ "Check list response returns a valid list"
+ )
+ self.assertNotEqual(
+ len(list_vm_response),
+ 0,
+ "Check VM available in List Virtual Machines"
+ )
+ self.assertEqual(
+ list_vm_response[0].state,
+ "Running",
+ "Check virtual machine is in running state"
+ )
+
class TestSecuredVmMigration(cloudstackTestCase):