Skip to content

Commit 336154a

Browse files
suryag1201Gupta, Surya
andauthored
[CSTACKEX-339] ASUP Upstream review comments (#117)
### Description Upstream PR : apache#14193 1- This allocates an intermediate list and then traverses it three more times. Since this runs once per pool per interval, on very large pools this adds avoidable CPU/GC overhead. Consider computing rootDiskCount, dataDiskCount, and totalLogicalSizeBytes in a single pass over volumes (filtering by CS_VOLUME_STATES inline) to reduce allocations and repeated streaming. 2- This treats a missing/unknown is_all_flash_optimized (null) as false, which classifies the node as fas and can incorrectly set cluster platformType when ONTAP doesn’t return these fields (permissions/older versions/partial responses). Consider returning null (unknown) when the required flags are absent and only adding non-null classifications to the rollup set so ASUP doesn’t misreport hardware type. 3- The DAO method is documented as “non-destroyed” volumes, but the SQL currently includes volumes in transitional/pre-provisioning states (e.g., Allocated) because it only excludes Destroy/Expunged. That can incorrectly flip multiPrimaryStoragePoolVm=true for VMs whose data disk hasn’t actually been created/moved yet. Tighten the state predicates to align with the intended “physically exists” semantics (e.g., include only the same stable states you count elsewhere, or explicitly exclude Allocated/upload-related states). 4- lock.unlock() is executed even when lock.lock(...) returns false (early return), which can attempt to unlock a lock this thread never acquired. Additionally, GlobalLock.getInternLock(...) typically requires releasing the reference (e.g., releaseRef()) to avoid leaking the interned lock object. Track a boolean locked and only unlock() when true, and ensure the lock reference is released in finally (even when not acquired). fixed the above issues <!--- ******************************************************************************* --> <!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. --> <!--- PLEASE PUT AN 'X' in only **ONE** box --> <!--- ******************************************************************************* --> ### Types of changes - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] New feature (non-breaking change which adds functionality) - [x] Bug fix (non-breaking change which fixes an issue) - [ ] Enhancement (improves an existing feature and functionality) - [ ] Cleanup (Code refactoring and cleanup, that may add test cases) - [ ] Build/CI - [ ] Test (unit or integration test code) ### Feature/Enhancement Scale or Bug Severity #### Feature/Enhancement Scale - [x] Major - [ ] Minor #### Bug Severity - [ ] BLOCKER - [ ] Critical - [ ] Major - [ ] Minor - [ ] Trivial ### Screenshots (if appropriate): ### How Has This Been Tested? <!-- Please describe in detail how you tested your changes. --> <!-- Include details of your testing environment, and the tests you ran to --> #### How did you try to break this feature and the system with this change? <!-- see how your change affects other areas of the code, etc. --> <!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document --> --------- Co-authored-by: Gupta, Surya <Surya.Gupta@netapp.com>
1 parent e2a2682 commit 336154a

6 files changed

Lines changed: 71 additions & 31 deletions

File tree

‎engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -183,8 +183,8 @@ public interface VolumeDao extends GenericDao<VolumeVO, Long>, StateDao<Volume.S
183183
boolean existsWithKmsKey(long kmsKeyId);
184184

185185
/**
186-
* Returns true if any VM with a non-destroyed ROOT volume on {@code poolId} also has a
187-
* non-destroyed DATADISK on a different primary storage pool. Existence check only
186+
* Returns true if any VM with a Ready ROOT volume on poolId also has an attached
187+
* Ready DATADISK on a different primary storage pool. Existence check only
188188
* ({@code LIMIT 1}); does not load volumes into memory.
189189
*/
190190
boolean hasMultiPrimaryStoragePoolVm(long poolId);

‎engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -96,14 +96,14 @@ public class VolumeDaoImpl extends GenericDaoBase<VolumeVO, Long> implements Vol
9696
protected static final String SELECT_HYPERTYPE_FROM_CLUSTER_VOLUME = "SELECT c.hypervisor_type from volumes v, storage_pool s, cluster c where v.pool_id = s.id and s.cluster_id = c.id and v.id = ?";
9797
protected static final String SELECT_HYPERTYPE_FROM_ZONE_VOLUME = "SELECT s.hypervisor from volumes v, storage_pool s where v.pool_id = s.id and v.id = ?";
9898
protected static final String SELECT_POOLSCOPE = "SELECT s.scope from storage_pool s, volumes v where s.id = v.pool_id and v.id = ?";
99-
// Looks for a VM whose root disk is on this pool and whose data disk is on another pool.
99+
// Looks for a VM whose Ready root disk is on this pool and whose attached Ready data disk is on another pool.
100100
// LIMIT 1 stops after the first match so we do not load all volumes.
101101
private static final String HAS_MULTI_PRIMARY_STORAGE_POOL_VM =
102102
"SELECT 1 FROM volumes root INNER JOIN volumes data ON data.instance_id = root.instance_id "
103103
+ "WHERE root.pool_id = ? AND root.volume_type = 'ROOT' AND root.instance_id IS NOT NULL "
104-
+ "AND root.removed IS NULL AND root.state NOT IN ('Destroy', 'Expunged') "
104+
+ "AND root.removed IS NULL AND root.state = 'Ready' "
105105
+ "AND data.volume_type = 'DATADISK' AND data.pool_id IS NOT NULL AND data.pool_id <> ? "
106-
+ "AND data.removed IS NULL AND data.state NOT IN ('Destroy', 'Expunged') LIMIT 1";
106+
+ "AND data.removed IS NULL AND data.state = 'Ready' LIMIT 1";
107107

108108
private static final String ORDER_POOLS_NUMBER_OF_VOLUMES_FOR_ACCOUNT_PART1 = "SELECT pool.id, SUM(IF(vol.state='Ready' AND vol.account_id = ?, 1, 0)) FROM `cloud`.`storage_pool` pool LEFT JOIN `cloud`.`volumes` vol ON pool.id = vol.pool_id WHERE pool.data_center_id = ? ";
109109
private static final String ORDER_POOLS_NUMBER_OF_VOLUMES_FOR_ACCOUNT_PART2 = " GROUP BY pool.id ORDER BY 2 ASC ";

‎plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/asup/OntapAsupManager.java‎

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -268,14 +268,18 @@ protected void pushAsupTelemetry() {
268268
logger.debug("ONTAP ASUP: another management server holds the ASUP lock; skipping this cycle.");
269269
return;
270270
}
271-
logger.debug("ONTAP ASUP: pushing telemetry for {} pool(s) [CloudStack version={}]",
272-
pools.size(), getCloudStackVersion());
273-
Map<String, AsupClusterClient> clientsByStorageIp = new HashMap<>();
274-
for (StoragePoolVO pool : pools) {
275-
pushAsupForStoragePool(pool, clientsByStorageIp);
271+
try {
272+
logger.debug("ONTAP ASUP: pushing telemetry for {} pool(s) [CloudStack version={}]",
273+
pools.size(), getCloudStackVersion());
274+
Map<String, AsupClusterClient> clientsByStorageIp = new HashMap<>();
275+
for (StoragePoolVO pool : pools) {
276+
pushAsupForStoragePool(pool, clientsByStorageIp);
277+
}
278+
} finally {
279+
lock.unlock();
276280
}
277281
} finally {
278-
lock.unlock();
282+
lock.releaseRef();
279283
}
280284
}
281285

@@ -430,17 +434,20 @@ private void addStoragePoolUsage(StoragePoolVO pool, Map<String, Object> payload
430434
// Only count volumes that definitely have a physical object on the ONTAP FlexVolume.
431435
// "Allocated" volumes have a pool_id row in the CS DB but ONTAP provisioning has not
432436
// yet been called, so including them would inflate counts and provisioned size.
433-
List<VolumeVO> cstackVolumes = volumes.stream()
434-
.filter(v -> CS_VOLUME_STATES.contains(v.getState()))
435-
.collect(java.util.stream.Collectors.toList());
436-
437-
long rootDiskCount = cstackVolumes.stream()
438-
.filter(v -> Volume.Type.ROOT.equals(v.getVolumeType())).count();
439-
long dataDiskCount = cstackVolumes.stream()
440-
.filter(v -> Volume.Type.DATADISK.equals(v.getVolumeType())).count();
441-
442-
long totalLogicalSizeBytes = cstackVolumes.stream()
443-
.mapToLong(v -> v.getSize() != null ? v.getSize() : 0L).sum();
437+
long rootDiskCount = 0L;
438+
long dataDiskCount = 0L;
439+
long totalLogicalSizeBytes = 0L;
440+
for (VolumeVO volume : volumes) {
441+
if (!CS_VOLUME_STATES.contains(volume.getState())) {
442+
continue;
443+
}
444+
if (Volume.Type.ROOT.equals(volume.getVolumeType())) {
445+
rootDiskCount++;
446+
} else if (Volume.Type.DATADISK.equals(volume.getVolumeType())) {
447+
dataDiskCount++;
448+
}
449+
totalLogicalSizeBytes += volume.getSize() != null ? volume.getSize() : 0L;
450+
}
444451
payload.put(OntapStorageConstants.ASUP_ROOT_DISK_COUNT, rootDiskCount);
445452
payload.put(OntapStorageConstants.ASUP_DATA_DISK_COUNT, dataDiskCount);
446453
payload.put(OntapStorageConstants.ASUP_TOTAL_LOGICAL_SIZE_BYTES, totalLogicalSizeBytes);
@@ -450,9 +457,9 @@ private void addStoragePoolUsage(StoragePoolVO pool, Map<String, Object> payload
450457
}
451458

452459
/**
453-
* Adds {@code hasMultiPrimaryStoragePoolVm}: true when at least one VM with ROOT on this
454-
* pool also has an attached DATADISK on a different primary storage pool. Uses a single
455-
* {@code LIMIT 1} existence query.
460+
* Adds {@code hasMultiPrimaryStoragePoolVm}: true when at least one VM with a Ready ROOT
461+
* on this pool also has an attached Ready DATADISK on a different primary storage pool.
462+
* Uses a single {@code LIMIT 1} existence query.
456463
*/
457464
private void hasMultiPrimaryStoragePoolVm(StoragePoolVO pool, Map<String, Object> payload) {
458465
try {

‎plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/ClusterNode.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ public class ClusterNode {
3636
@JsonProperty("is_all_flash_optimized")
3737
private Boolean allFlashOptimized;
3838

39-
@JsonProperty("is_performance_optimized")
39+
@JsonProperty("is_perf_optimized")
4040
private Boolean performanceOptimized;
4141

4242
@JsonProperty("is_capacity_optimized")

‎plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,8 @@ public Cluster getClusterInfo() {
153153
* Sets {@link Cluster#setModel(String)} and {@link Cluster#setPlatformType(String)} from one
154154
* {@code GET /api/cluster/nodes}. Distinct models are joined with a comma. Platform type is
155155
* {@code performance}, {@code capacity}, {@code fas}, or {@code composite} when personalities mix.
156+
* A node that omits {@code is_all_flash_optimized} is left out of the rollup so a partial
157+
* response is not reported as {@code fas}.
156158
*/
157159
private void populateNodeAsupFields(Cluster cluster, String authHeader) {
158160
try {
@@ -171,7 +173,10 @@ private void populateNodeAsupFields(Cluster cluster, String authHeader) {
171173
if (node.getModel() != null && !node.getModel().isBlank()) {
172174
models.add(node.getModel().trim());
173175
}
174-
platformTypes.add(classifyNodePlatformType(node));
176+
String platformType = classifyNodePlatformType(node);
177+
if (platformType != null) {
178+
platformTypes.add(platformType);
179+
}
175180
}
176181
if (!models.isEmpty()) {
177182
cluster.setModel(String.join(OntapStorageConstants.COMMA, models));
@@ -211,14 +216,17 @@ public String getClusterVersion(Cluster cluster) {
211216
/**
212217
* Classifies one node: not all-flash → {@code fas}; all-flash + capacity → {@code capacity};
213218
* otherwise all-flash (including performance-optimized or classic AFF) → {@code performance}.
219+
* Returns {@code null} when {@code is_all_flash_optimized} is absent, so unknown hardware is
220+
* not reported as {@code fas}.
214221
*/
215222
private static String classifyNodePlatformType(ClusterNode node) {
216-
boolean allFlash = Boolean.TRUE.equals(node.getAllFlashOptimized());
217-
boolean capacity = Boolean.TRUE.equals(node.getCapacityOptimized());
218-
if (!allFlash) {
223+
if (node.getAllFlashOptimized() == null) {
224+
return null;
225+
}
226+
if (!node.getAllFlashOptimized()) {
219227
return OntapStorageConstants.ASUP_PLATFORM_TYPE_FAS;
220228
}
221-
if (capacity) {
229+
if (Boolean.TRUE.equals(node.getCapacityOptimized())) {
222230
return OntapStorageConstants.ASUP_PLATFORM_TYPE_CAPACITY;
223231
}
224232
return OntapStorageConstants.ASUP_PLATFORM_TYPE_PERFORMANCE;

‎plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,31 @@ public void testGetClusterInfo_mixedPersonalitiesIsComposite() {
370370
assertEquals(OntapStorageConstants.ASUP_PLATFORM_TYPE_COMPOSITE, result.getPlatformType());
371371
}
372372

373+
@Test
374+
public void testGetClusterInfo_missingAllFlashFlagLeavesPlatformUnset() {
375+
stubClusterGet();
376+
when(clusterFeignClient.getClusterNodes(anyString(), anyMap()))
377+
.thenReturn(new OntapResponse<>(List.of(
378+
clusterNode("AFF-A400", null, null, null))));
379+
380+
Cluster result = storageStrategy.getClusterInfo();
381+
assertEquals("AFF-A400", result.getModel());
382+
assertNull(result.getPlatformType());
383+
}
384+
385+
@Test
386+
public void testGetClusterInfo_missingFlagDoesNotChangeKnownPlatform() {
387+
stubClusterGet();
388+
when(clusterFeignClient.getClusterNodes(anyString(), anyMap()))
389+
.thenReturn(new OntapResponse<>(List.of(
390+
clusterNode("AFF-A400", true, true, false),
391+
clusterNode("AFF-A400", null, null, null))));
392+
393+
Cluster result = storageStrategy.getClusterInfo();
394+
assertEquals("AFF-A400", result.getModel());
395+
assertEquals(OntapStorageConstants.ASUP_PLATFORM_TYPE_PERFORMANCE, result.getPlatformType());
396+
}
397+
373398
@Test
374399
public void testGetClusterInfo_nodesGetFailureLeavesModelUnset() {
375400
stubClusterGet();

0 commit comments

Comments
 (0)