Skip to content

Commit 3e71a43

Browse files
Gupta, SuryaGupta, Surya
authored andcommitted
[CSTACKEX-204] Fix issue for asup push delay
1 parent 11eefe9 commit 3e71a43

4 files changed

Lines changed: 186 additions & 159 deletions

File tree

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

Lines changed: 87 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@
5757
import java.time.Duration;
5858
import java.time.Instant;
5959
import java.util.EnumSet;
60-
import java.util.HashSet;
60+
import java.util.HashMap;
6161
import java.util.LinkedHashMap;
6262
import java.util.List;
6363
import java.util.Map;
@@ -81,11 +81,11 @@
8181
* snapshot telemetry (counts by state, total provisioned size).</li>
8282
* </ul>
8383
*
84-
* <p>Runs are booked one at a time: after each push the next run is scheduled for exactly
85-
* {@link OntapConfigurationManager#AsupIntervalSeconds} later, so the configured interval is
86-
* the real spacing rather than being rounded up to some fixed polling cadence. Editing either
87-
* ONTAP ASUP setting re-books the pending run immediately, with no management-server
88-
* restart.</p>
84+
* <p>Runs are booked one at a time: after each cycle starts, the next run is scheduled for
85+
* {@link OntapConfigurationManager#AsupIntervalSeconds} after that start, so production
86+
* intervals (hours) are start-to-start. REST work sits inside the interval; it is not added
87+
* after it. Editing either ONTAP ASUP setting re-books the pending run immediately, with no
88+
* management-server restart.</p>
8989
*/
9090
public class OntapAsupManager extends ManagerBase {
9191
private static final int ASUP_LOCK_TIMEOUT_SECONDS = 5;
@@ -111,9 +111,9 @@ public class OntapAsupManager extends ManagerBase {
111111
private final ObjectMapper objectMapper = new ObjectMapper();
112112

113113
/**
114-
* Timestamp of the last successful ASUP push. Starts at {@link Instant#EPOCH} so the
115-
* very first run always fires immediately. {@code volatile} ensures the scheduler
116-
* thread's write is visible without synchronization overhead.
114+
* Timestamp of the last ASUP cycle start (or {@link Instant#EPOCH} if none has run).
115+
* The next run is due {@link OntapConfigurationManager#AsupIntervalSeconds} after this
116+
* instant. {@code volatile} so the scheduler thread's write is visible without extra locking.
117117
*/
118118
volatile Instant lastPushTime = Instant.EPOCH;
119119

@@ -195,8 +195,9 @@ private synchronized void scheduleNextRun() {
195195

196196
/**
197197
* Milliseconds remaining until {@link #lastPushTime} plus the live configured interval.
198-
* Zero when a push is already overdue — for example after the interval is shortened, or on
199-
* the very first run when {@link #lastPushTime} is still {@link Instant#EPOCH}.
198+
* Zero when a push is already overdue — for example after the interval is shortened, the
199+
* previous cycle ran longer than the interval, or on the very first run when
200+
* {@link #lastPushTime} is still {@link Instant#EPOCH}.
200201
*/
201202
long millisUntilNextPush() {
202203
Duration configuredInterval = Duration.ofSeconds(
@@ -229,26 +230,15 @@ private void onAsupConfigEdited(String senderAddress, String subject, Object arg
229230
* One ASUP run inside a managed CloudStack context, which the DAO calls made during the push
230231
* require.
231232
*
232-
* <p>The interval is re-checked here as well as when booking, because a run can be booked
233-
* early (an edit that is not yet due). Whatever happens, the next run is booked again in the
234-
* {@code finally} block so the schedule never stalls.</p>
233+
* <p>{@link #lastPushTime} is stamped at the start of the cycle so the next wait is the
234+
* remainder of the interval (work does not get added on top). The next run is booked again
235+
* in {@code finally} so the schedule never stalls.</p>
235236
*/
236237
protected class OntapAsupTask extends ManagedContextRunnable {
237238
@Override
238239
protected void runInContext() {
239240
try {
240-
if (Boolean.FALSE.equals(OntapConfigurationManager.AsupEnabled.value())) {
241-
logger.debug("ONTAP ASUP: telemetry is disabled ({}=false); skipping this cycle.",
242-
OntapConfigurationManager.AsupEnabled.key());
243-
return;
244-
}
245-
Duration configuredInterval = Duration.ofSeconds(
246-
getAsupIntervalSeconds(OntapConfigurationManager.AsupIntervalSeconds.value()));
247-
Instant now = Instant.now();
248-
if (Duration.between(lastPushTime, now).compareTo(configuredInterval) < 0) {
249-
return; // configured interval has not elapsed yet
250-
}
251-
lastPushTime = now;
241+
lastPushTime = Instant.now();
252242
pushAsupTelemetry();
253243
} catch (Exception e) {
254244
logger.warn("ONTAP ASUP: unexpected error during periodic push: {}", e.getMessage());
@@ -265,11 +255,6 @@ protected void runInContext() {
265255
* only one node emits per cycle.</p>
266256
*/
267257
protected void pushAsupTelemetry() {
268-
if (Boolean.FALSE.equals(OntapConfigurationManager.AsupEnabled.value())) {
269-
logger.debug("ONTAP ASUP: telemetry is disabled ({}=false); skipping this cycle.",
270-
OntapConfigurationManager.AsupEnabled.key());
271-
return;
272-
}
273258
List<StoragePoolVO> pools = storagePoolDao.findPoolsByProvider(OntapStorageConstants.ONTAP_PLUGIN_NAME);
274259
if (CollectionUtils.isEmpty(pools)) {
275260
logger.debug("ONTAP ASUP: no ONTAP-backed storage pools found; nothing to push.");
@@ -284,12 +269,9 @@ protected void pushAsupTelemetry() {
284269
}
285270
logger.debug("ONTAP ASUP: pushing telemetry for {} pool(s) [CloudStack version={}]",
286271
pools.size(), getCloudStackVersion());
287-
// Tracks clusters that have already received a heartbeat this cycle, so that multiple
288-
// pools backed by the same ONTAP cluster emit only a single heartbeat (event-id 0),
289-
// while each distinct cluster still gets its own heartbeat per cycle.
290-
Set<String> clustersHeartBeated = new HashSet<>();
272+
Map<String, AsupClusterClient> clientsByStorageIp = new HashMap<>();
291273
for (StoragePoolVO pool : pools) {
292-
pushAsupForStoragePool(pool, clustersHeartBeated);
274+
pushAsupForStoragePool(pool, clientsByStorageIp);
293275
}
294276
} finally {
295277
lock.unlock();
@@ -299,43 +281,51 @@ protected void pushAsupTelemetry() {
299281
/**
300282
* Pushes the heartbeat (event-id 0) and pool (event-id 1) ASUP messages for a single pool.
301283
*
302-
* <p>The heartbeat is emitted at most once per distinct ONTAP cluster per cycle: the cluster's
303-
* UUID (or its storage IP when the UUID is unavailable) is recorded in {@code clustersHeartbeated},
304-
* and subsequent pools backed by the same cluster skip the heartbeat. The pool mapping message
305-
* is always emitted, once per pool.</p>
284+
* <p>{@code clientsByStorageIp} is the per-cycle cache keyed by management {@code storageIP}:
285+
* a cache miss builds the strategy, GETs cluster info, and sends event-0; a hit reuses that
286+
* client and skips event-0. Event-1 is always sent. SVM name in the payload still comes from
287+
* this pool's details.</p>
306288
*
307289
* <p>Best-effort: any failure is logged and swallowed.</p>
308290
*/
309-
protected void pushAsupForStoragePool(StoragePoolVO pool, Set<String> clustersHeartbeated) {
291+
protected void pushAsupForStoragePool(StoragePoolVO pool, Map<String, AsupClusterClient> clientsByStorageIp) {
310292
try {
293+
StorageStrategy strategy;
294+
Cluster cluster;
295+
boolean sendHeartbeat;
311296
Map<String, String> details = storagePoolDetailsDao.listDetailsKeyPairs(pool.getId());
312297
if (details == null || details.isEmpty()) {
313298
logger.warn("ONTAP ASUP: storage pool [{}] has no details; skipping.", pool.getId());
314299
return;
315300
}
316301

317-
StorageStrategy strategy = OntapStorageUtils.getStrategyByStoragePoolDetails(details);
318-
// Fetch the ONTAP cluster once and reuse its identity (uuid, name) and version
319-
// for both messages, avoiding extra REST round-trips.
320-
Cluster cluster = strategy.getClusterInfo();
302+
String storageIp = details.get(OntapStorageConstants.STORAGE_IP);
303+
AsupClusterClient asupClusterClient = StringUtils.isNotBlank(storageIp) ? clientsByStorageIp.get(storageIp) : null;
304+
if (asupClusterClient != null) {
305+
strategy = asupClusterClient.strategy;
306+
cluster = asupClusterClient.cluster;
307+
sendHeartbeat = false;
308+
} else {
309+
strategy = OntapStorageUtils.resolveStrategyFromPoolDetails(details);
310+
cluster = strategy.getClusterInfo();
311+
if (StringUtils.isNotBlank(storageIp)) {
312+
clientsByStorageIp.put(storageIp, new AsupClusterClient(strategy, cluster));
313+
}
314+
sendHeartbeat = true;
315+
}
321316
String ontapVersion = strategy.getClusterVersion(cluster);
322317
String clusterUuid = cluster != null ? cluster.getUuid() : null;
323-
String clusterName = cluster != null ? cluster.getName() : null;
324318
String cloudStackVersion = getCloudStackVersion();
325319
String computerName = getComputerName();
326320

327-
// event-id 0: CloudStack -> ONTAP cluster heartbeat (versions), emitted once per ontap cluster.
328-
// Key on the cluster UUID; fall back to the storage IP if the UUID is unavailable.
329-
String clusterKey = StringUtils.isNotBlank(clusterUuid) ? clusterUuid
330-
: details.get(OntapStorageConstants.STORAGE_IP);
331-
if (clusterKey == null || clustersHeartbeated.add(clusterKey)) {
321+
if (sendHeartbeat) {
332322
EmsApplicationLog heartbeat = buildBaseMessage(computerName, cloudStackVersion);
333323
heartbeat.setEventId(OntapStorageConstants.ASUP_EVENT_ID_HEARTBEAT);
334324
heartbeat.setEventDescription(buildHeartbeatDescription(cloudStackVersion, ontapVersion, clusterUuid));
335325
strategy.sendAsupMessage(heartbeat);
336326
} else {
337-
logger.debug("ONTAP ASUP: heartbeat already sent this cycle for cluster [{}]; skipping for pool [{}]",
338-
defaultUnknown(clusterName), pool.getId());
327+
logger.debug("ONTAP ASUP: heartbeat already sent this cycle for storage IP [{}]; skipping for pool [{}]",
328+
storageIp, pool.getId());
339329
}
340330

341331
// event-id 1: CloudStack storage pool -> backing ONTAP volume mapping, once per pool.
@@ -393,12 +383,29 @@ private String buildPoolDescription(StoragePoolVO pool, Map<String, String> deta
393383
payload.put(OntapStorageConstants.ASUP_CLUSTER_UUID, defaultUnknown(clusterUuid));
394384
payload.put(OntapStorageConstants.ASUP_SVM, defaultUnknown(details.get(OntapStorageConstants.SVM_NAME)));
395385
payload.put(OntapStorageConstants.ASUP_ONTAP_VOLUME_UUID, defaultUnknown(details.get(OntapStorageConstants.VOLUME_UUID)));
396-
addStoragePoolUsage(pool, payload);
386+
List<VolumeVO> volumes = null;
387+
try {
388+
volumes = loadNonDestroyedVolumes(pool);
389+
} catch (Exception e) {
390+
logger.error("ONTAP ASUP: failed to load volumes for pool [{}]: {}", pool.getId(), e.getMessage());
391+
}
392+
if (volumes != null) {
393+
addStoragePoolUsage(pool, payload, volumes);
394+
addSnapshotMetrics(pool, payload, volumes);
395+
}
397396
hasMultiPrimaryStoragePoolVm(pool, payload);
398-
addSnapshotMetrics(pool, payload);
399397
return toJson(payload);
400398
}
401399

400+
/**
401+
* Single CloudStack DB read of non-destroyed volumes on this pool. Shared by usage and
402+
* snapshot counts so the list is not queried three times. Never returns null.
403+
*/
404+
private List<VolumeVO> loadNonDestroyedVolumes(StoragePoolVO pool) {
405+
List<VolumeVO> volumes = volumeDao.findNonDestroyedVolumesByPoolId(pool.getId(), null);
406+
return volumes == null ? java.util.Collections.emptyList() : volumes;
407+
}
408+
402409
/**
403410
* Computes pool usage from CloudStack's volume records and adds it to the payload:
404411
* <ul>
@@ -408,16 +415,12 @@ private String buildPoolDescription(StoragePoolVO pool, Map<String, String> deta
408415
* in bytes; for thin-provisioned volumes this is the logical size requested at
409416
* creation time, not the physical space consumed on ONTAP</li>
410417
* </ul>
411-
* All volume types are counted (both ROOT and DATADISK); the {@code null} type argument
412-
* disables the type filter in the DAO. All derived values are computed in-memory from the
413-
* same single query (no extra round-trips). Best-effort: any failure leaves the usage
418+
* {@code volumes} is the non-destroyed list for this pool (already loaded). Only
419+
* {@link #CS_VOLUME_STATES} are counted. Best-effort: any failure leaves the usage
414420
* fields out and never breaks telemetry.
415421
*/
416-
private void addStoragePoolUsage(StoragePoolVO pool, Map<String, Object> payload) {
422+
private void addStoragePoolUsage(StoragePoolVO pool, Map<String, Object> payload, List<VolumeVO> volumes) {
417423
try {
418-
// Pass null volume-type to include ALL volumes (ROOT + DATADISK).
419-
List<VolumeVO> volumes = volumeDao.findNonDestroyedVolumesByPoolId(pool.getId(), null);
420-
421424
// Only count volumes that definitely have a physical object on the ONTAP FlexVolume.
422425
// "Allocated" volumes have a pool_id row in the CS DB but ONTAP provisioning has not
423426
// yet been called, so including them would inflate counts and provisioned size.
@@ -467,19 +470,18 @@ private void hasMultiPrimaryStoragePoolVm(StoragePoolVO pool, Map<String, Object
467470
*
468471
* <p>Best-effort: any failure leaves the fields out without breaking telemetry.</p>
469472
*/
470-
private void addSnapshotMetrics(StoragePoolVO pool, Map<String, Object> payload) {
471-
addVmSnapshotMetrics(pool, payload);
472-
addVolumeSnapshotMetrics(pool, payload);
473+
private void addSnapshotMetrics(StoragePoolVO pool, Map<String, Object> payload, List<VolumeVO> volumes) {
474+
addVmSnapshotMetrics(pool, payload, volumes);
475+
addVolumeSnapshotMetrics(pool, payload, volumes);
473476
}
474477

475478
/**
476479
* Adds {@code volumeSnapshotCount} to the payload.
477480
* Counts all non-destroyed CloudStack volume-level snapshots for volumes on this pool.
478481
*/
479-
private void addVolumeSnapshotMetrics(StoragePoolVO pool, Map<String, Object> payload) {
482+
private void addVolumeSnapshotMetrics(StoragePoolVO pool, Map<String, Object> payload, List<VolumeVO> volumes) {
480483
try {
481-
List<VolumeVO> volumes = volumeDao.findNonDestroyedVolumesByPoolId(pool.getId(), null);
482-
if (volumes == null || volumes.isEmpty()) {
484+
if (volumes.isEmpty()) {
483485
payload.put(OntapStorageConstants.ASUP_VOLUME_SNAPSHOT_COUNT, 0);
484486
return;
485487
}
@@ -505,10 +507,9 @@ private void addVolumeSnapshotMetrics(StoragePoolVO pool, Map<String, Object> pa
505507
* Counts all active (non-expunging, non-removed) VM snapshots for VMs that have at
506508
* least one volume on this pool.
507509
*/
508-
private void addVmSnapshotMetrics(StoragePoolVO pool, Map<String, Object> payload) {
510+
private void addVmSnapshotMetrics(StoragePoolVO pool, Map<String, Object> payload, List<VolumeVO> volumes) {
509511
try {
510-
List<VolumeVO> volumes = volumeDao.findNonDestroyedVolumesByPoolId(pool.getId(), null);
511-
if (volumes == null || volumes.isEmpty()) {
512+
if (volumes.isEmpty()) {
512513
payload.put(OntapStorageConstants.ASUP_VM_SNAPSHOT_COUNT, 0);
513514
return;
514515
}
@@ -636,4 +637,19 @@ int getAsupIntervalSeconds(Integer configured) {
636637
}
637638
return configured;
638639
}
640+
641+
/**
642+
* One ONTAP cluster HTTP client plus the cluster GET for this ASUP cycle, keyed by
643+
* {@code storageIP}. Discarded when the cycle ends.
644+
*/
645+
static final class AsupClusterClient {
646+
final StorageStrategy strategy;
647+
final Cluster cluster;
648+
649+
AsupClusterClient(StorageStrategy strategy, Cluster cluster) {
650+
this.strategy = strategy;
651+
this.cluster = cluster;
652+
}
653+
}
654+
639655
}

‎plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ public class OntapStorageConstants {
171171
public static final String ASUP_ENABLED_CONFIG_KEY = "ontap.asup.enabled";
172172
public static final String ASUP_ENABLED_DEFAULT = "true";
173173
public static final String ASUP_INTERVAL_CONFIG_KEY = "ontap.asup.interval";
174-
public static final int ASUP_MIN_INTERVAL_SECONDS = 60; // 1 minute (testing; production is 10800)
174+
public static final int ASUP_MIN_INTERVAL_SECONDS = 360; // 6 minutes (demo; production is 10800)
175175
public static final int ASUP_MAX_INTERVAL_SECONDS = 86400; // 24 hours
176176
public static final int ASUP_DEFAULT_INTERVAL_SECONDS = 43200; // 12 hours (twice a day)
177177

‎plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -136,17 +136,25 @@ public static StorageStrategy getStrategyByStoragePoolDetails(Map<String, String
136136
return getStrategyByStoragePoolDetails(details, false);
137137
}
138138

139-
public static StorageStrategy getStrategyByStoragePoolDetails(Map<String, String> details,
140-
boolean validateAggregatesForVolumeCreation) {
139+
/**
140+
* Same as {@link #getStrategyByStoragePoolDetails(Map)} but does not call
141+
* {@link StorageStrategy#connect}. ASUP uses this for cluster GET and EMS POST only.
142+
*/
143+
public static StorageStrategy resolveStrategyFromPoolDetails(Map<String, String> details) {
141144
if (details == null || details.isEmpty()) {
142-
logger.error("getStrategyByStoragePoolDetails: Storage pool details are null or empty");
145+
logger.error("resolveStrategyFromPoolDetails: Storage pool details are null or empty");
143146
throw new CloudRuntimeException("Storage pool details are null or empty");
144147
}
145148
String protocol = details.get(OntapStorageConstants.PROTOCOL);
146149
OntapStorage ontapStorage = new OntapStorage(details.get(OntapStorageConstants.USERNAME), details.get(OntapStorageConstants.PASSWORD),
147150
details.get(OntapStorageConstants.STORAGE_IP), details.get(OntapStorageConstants.SVM_NAME), Long.parseLong(details.get(OntapStorageConstants.SIZE)),
148151
ProtocolType.valueOf(protocol));
149-
StorageStrategy storageStrategy = StorageProviderFactory.getStrategy(ontapStorage);
152+
return StorageProviderFactory.getStrategy(ontapStorage);
153+
}
154+
155+
public static StorageStrategy getStrategyByStoragePoolDetails(Map<String, String> details,
156+
boolean validateAggregatesForVolumeCreation) {
157+
StorageStrategy storageStrategy = resolveStrategyFromPoolDetails(details);
150158
boolean isValid = storageStrategy.connect(validateAggregatesForVolumeCreation);
151159
if (isValid) {
152160
logger.info("Connection to Ontap SVM [{}] successful", details.get(OntapStorageConstants.SVM_NAME));

0 commit comments

Comments
 (0)