Skip to content

Commit c78a777

Browse files
nvazqueznvazquez
authored
Fix: memory leak on volume allocation (#7136)
1 parent f39b02a commit c78a777

File tree

6 files changed

+158
-9
lines changed

6 files changed

+158
-9
lines changed

‎api/src/main/java/com/cloud/deploy/DeployDestination.java‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -147,13 +147,7 @@ public String toString() {
147147
destination.append("Storage(");
148148
if (displayStorage && _storage != null) {
149149
StringBuffer storageBuf = new StringBuffer();
150-
//String storageStr = "";
151150
for (Volume vol : _storage.keySet()) {
152-
if (!storageBuf.toString().equals("")) {
153-
storageBuf.append(storageBuf.toString());
154-
storageBuf.append(", ");
155-
}
156-
storageBuf.append(storageBuf);
157151
storageBuf.append("Volume(");
158152
storageBuf.append(vol.getId());
159153
storageBuf.append("|");
@@ -162,7 +156,7 @@ public String toString() {
162156
storageBuf.append(_storage.get(vol).getId());
163157
storageBuf.append(")");
164158
}
165-
destination.append(storageBuf.toString());
159+
destination.append(storageBuf);
166160
}
167161
return destination.append(")]").toString();
168162
}

‎engine/schema/src/main/java/com/cloud/host/dao/HostDao.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,4 +149,12 @@ public interface HostDao extends GenericDao<HostVO, Long>, StateDao<Status, Stat
149149
* @return the number of hosts/agents this {@see ManagementServer} has responsibility over
150150
*/
151151
int countByMs(long msid);
152+
153+
/**
154+
* Retrieves the hypervisor versions of the hosts in the datacenter which are in Up state in ascending order
155+
* @param datacenterId data center id
156+
* @param hypervisorType hypervisor type of the hosts
157+
* @return ordered list of hypervisor versions
158+
*/
159+
List<String> listOrderedHostsHypervisorVersionsInDatacenter(long datacenterId, HypervisorType hypervisorType);
152160
}

‎engine/schema/src/main/java/com/cloud/host/dao/HostDaoImpl.java‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,10 @@ public class HostDaoImpl extends GenericDaoBase<HostVO, Long> implements HostDao
9090
"from vm_instance vm " +
9191
"join host h on (vm.host_id=h.id) " +
9292
"where vm.service_offering_id= ? and vm.state not in (\"Destroyed\", \"Expunging\", \"Error\") group by h.id";
93-
93+
private static final String GET_ORDERED_HW_VERSIONS_IN_DC = "select hypervisor_version from host " +
94+
"where type = 'Routing' and status = 'Up' and hypervisor_type = ? and data_center_id = ? " +
95+
"group by hypervisor_version " +
96+
"order by hypervisor_version asc";
9497

9598
protected SearchBuilder<HostVO> TypePodDcStatusSearch;
9699

@@ -1299,6 +1302,25 @@ public int countByMs(long msid) {
12991302
return getCount(sc);
13001303
}
13011304

1305+
@Override
1306+
public List<String> listOrderedHostsHypervisorVersionsInDatacenter(long datacenterId, HypervisorType hypervisorType) {
1307+
PreparedStatement pstmt = null;
1308+
List<String> result = new ArrayList<>();
1309+
try {
1310+
TransactionLegacy txn = TransactionLegacy.currentTxn();
1311+
pstmt = txn.prepareAutoCloseStatement(GET_ORDERED_HW_VERSIONS_IN_DC);
1312+
pstmt.setString(1, Objects.toString(hypervisorType));
1313+
pstmt.setLong(2, datacenterId);
1314+
ResultSet resultSet = pstmt.executeQuery();
1315+
while (resultSet.next()) {
1316+
result.add(resultSet.getString(1));
1317+
}
1318+
} catch (SQLException e) {
1319+
s_logger.error("Error trying to obtain hypervisor version on datacenter", e);
1320+
}
1321+
return result;
1322+
}
1323+
13021324
@Override
13031325
public List<HostVO> listAllHostsByType(Host.Type type) {
13041326
SearchCriteria<HostVO> sc = TypeSearch.create();

‎server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4078,7 +4078,8 @@ private int getMaxDataVolumesSupported(UserVmVO vm) {
40784078
} else {
40794079
HypervisorType hypervisorType = vm.getHypervisorType();
40804080
if (hypervisorType != null && CollectionUtils.isNotEmpty(supportingDefaultHV) && supportingDefaultHV.contains(hypervisorType)) {
4081-
maxDataVolumesSupported = _hypervisorCapabilitiesDao.getMaxDataVolumesLimit(hypervisorType, "default");
4081+
String hwVersion = getMinimumHypervisorVersionInDatacenter(vm.getDataCenterId(), hypervisorType);
4082+
maxDataVolumesSupported = _hypervisorCapabilitiesDao.getMaxDataVolumesLimit(hypervisorType, hwVersion);
40824083
}
40834084
}
40844085
if (maxDataVolumesSupported == null || maxDataVolumesSupported.intValue() <= 0) {
@@ -4090,6 +4091,16 @@ private int getMaxDataVolumesSupported(UserVmVO vm) {
40904091
return maxDataVolumesSupported.intValue();
40914092
}
40924093

4094+
protected String getMinimumHypervisorVersionInDatacenter(long datacenterId, HypervisorType hypervisorType) {
4095+
String defaultHypervisorVersion = "default";
4096+
if (hypervisorType == HypervisorType.Simulator) {
4097+
return defaultHypervisorVersion;
4098+
}
4099+
List<String> hwVersions = _hostDao.listOrderedHostsHypervisorVersionsInDatacenter(datacenterId, hypervisorType);
4100+
String minHwVersion = CollectionUtils.isNotEmpty(hwVersions) ? hwVersions.get(0) : defaultHypervisorVersion;
4101+
return StringUtils.isBlank(minHwVersion) ? defaultHypervisorVersion : minHwVersion;
4102+
}
4103+
40934104
private Long getDeviceId(UserVmVO vm, Long deviceId) {
40944105
// allocate deviceId
40954106
int maxDevices = getMaxDataVolumesSupported(vm) + 2; // add 2 to consider devices root volume and cdrom

‎server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1197,4 +1197,36 @@ private void prepareAndRunIsNewDiskOfferingTheSameAndCustomServiceOffering(long
11971197
boolean result = volumeApiServiceImpl.isNewDiskOfferingTheSameAndCustomServiceOffering(existingDiskOffering, newDiskOffering);
11981198
Assert.assertEquals(expectedResult, result);
11991199
}
1200+
1201+
private void testBaseListOrderedHostsHypervisorVersionInDc(List<String> hwVersions, HypervisorType hypervisorType,
1202+
String expected) {
1203+
when(_hostDao.listOrderedHostsHypervisorVersionsInDatacenter(anyLong(), any(HypervisorType.class)))
1204+
.thenReturn(hwVersions);
1205+
String min = volumeApiServiceImpl.getMinimumHypervisorVersionInDatacenter(1L, hypervisorType);
1206+
Assert.assertEquals(expected, min);
1207+
}
1208+
1209+
@Test
1210+
public void testGetMinimumHypervisorVersionInDatacenterSimulator() {
1211+
List<String> hwVersions = List.of("4.17.3.0-SNAPSHOT");
1212+
HypervisorType hypervisorType = HypervisorType.Simulator;
1213+
String expected = "default";
1214+
testBaseListOrderedHostsHypervisorVersionInDc(hwVersions, hypervisorType, expected);
1215+
}
1216+
1217+
@Test
1218+
public void testGetMinimumHypervisorVersionInDatacenterEmptyVersion() {
1219+
List<String> hwVersions = List.of("", "xxxx", "yyyy");
1220+
HypervisorType hypervisorType = HypervisorType.KVM;
1221+
String expected = "default";
1222+
testBaseListOrderedHostsHypervisorVersionInDc(hwVersions, hypervisorType, expected);
1223+
}
1224+
1225+
@Test
1226+
public void testGetMinimumHypervisorVersionInDatacenterVersions() {
1227+
List<String> hwVersions = List.of("6.7", "6.7.1", "6.7.2");
1228+
HypervisorType hypervisorType = HypervisorType.VMware;
1229+
String expected = "6.7";
1230+
testBaseListOrderedHostsHypervisorVersionInDc(hwVersions, hypervisorType, expected);
1231+
}
12001232
}

‎test/integration/smoke/test_vm_life_cycle.py‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -873,6 +873,88 @@ def test_11_destroy_vm_and_volumes(self):
873873

874874
self.assertEqual(Volume.list(self.apiclient, id=vol1.id), None, "List response contains records when it should not")
875875

876+
@attr(tags = ["devcloud", "advanced", "advancedns", "smoke", "basic", "sg"], required_hardware="false")
877+
def test_12_start_vm_multiple_volumes_allocated(self):
878+
"""Test attaching multiple datadisks and start VM
879+
"""
880+
881+
# Validate the following
882+
# 1. Deploys a VM without starting it and attaches multiple datadisks to it
883+
# 2. Start VM successfully
884+
# 3. Destroys the VM with DataDisks option
885+
886+
custom_disk_offering = DiskOffering.list(self.apiclient, name='Custom')[0]
887+
888+
# Create VM without starting it
889+
vm = VirtualMachine.create(
890+
self.apiclient,
891+
self.services["small"],
892+
accountid=self.account.name,
893+
domainid=self.account.domainid,
894+
serviceofferingid=self.small_offering.id,
895+
startvm=False
896+
)
897+
self.cleanup.append(vm)
898+
899+
hosts = Host.list(
900+
self.apiclient,
901+
zoneid=self.zone.id,
902+
type='Routing',
903+
hypervisor=self.hypervisor,
904+
state='Up')
905+
906+
if self.hypervisor.lower() in ["simulator"] or not hosts[0].hypervisorversion:
907+
hypervisor_version = "default"
908+
else:
909+
hypervisor_version = hosts[0].hypervisorversion
910+
911+
res = self.dbclient.execute("select max_data_volumes_limit from hypervisor_capabilities where "
912+
"hypervisor_type='%s' and hypervisor_version='%s';" %
913+
(self.hypervisor.lower(), hypervisor_version))
914+
if isinstance(res, list) and len(res) > 0:
915+
max_volumes = res[0][0]
916+
if max_volumes > 14:
917+
max_volumes = 14
918+
else:
919+
max_volumes = 6
920+
921+
# Create and attach volumes
922+
self.services["custom_volume"]["customdisksize"] = 1
923+
self.services["custom_volume"]["zoneid"] = self.zone.id
924+
for i in range(max_volumes):
925+
volume = Volume.create_custom_disk(
926+
self.apiclient,
927+
self.services["custom_volume"],
928+
account=self.account.name,
929+
domainid=self.account.domainid,
930+
diskofferingid=custom_disk_offering.id
931+
)
932+
self.cleanup.append(volume)
933+
VirtualMachine.attach_volume(vm, self.apiclient, volume)
934+
935+
# Start the VM
936+
self.debug("Starting VM - ID: %s" % vm.id)
937+
vm.start(self.apiclient)
938+
list_vm_response = VirtualMachine.list(
939+
self.apiclient,
940+
id=vm.id
941+
)
942+
self.assertEqual(
943+
isinstance(list_vm_response, list),
944+
True,
945+
"Check list response returns a valid list"
946+
)
947+
self.assertNotEqual(
948+
len(list_vm_response),
949+
0,
950+
"Check VM available in List Virtual Machines"
951+
)
952+
self.assertEqual(
953+
list_vm_response[0].state,
954+
"Running",
955+
"Check virtual machine is in running state"
956+
)
957+
876958

877959
class TestSecuredVmMigration(cloudstackTestCase):
878960

0 commit comments

Comments
 (0)