Copilot commented on code in PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#discussion_r3673112606
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java:
##########
@@ -297,6 +354,155 @@ protected void scheduleSnapshots() {
}
}
+ /**
+ * Handles a synchronous failure to dispatch the CreateSnapshotCmd (e.g.
an allocation error) for a recurring
+ * snapshot. Without this, the schedule's {@code scheduledTimestamp} is
never advanced, so it gets retried on
+ * every poll (every {@code snapshot.poll.interval} seconds) forever.
Instead: log a failure event keyed by the
+ * volume (so consecutive failures can be counted from event history), and
either back off by the configured
+ * retry interval, or - once the configured maximum consecutive failures
is reached - give up until the next
+ * regularly scheduled run and raise a WARN notification event.
+ */
+ protected void handleFailedSnapshotDispatch(final SnapshotScheduleVO
snapshotToBeExecuted, final VolumeVO volume,
+ final SnapshotScheduleVO lockedSchedule, final Long eventId, final
Exception cause) {
+ final long volumeId = volume.getId();
+ final Account account = _acctDao.findById(volume.getAccountId());
+ final int maxFailures =
getScopedConfigValue(SnapshotManager.SnapshotRecurringMaxFailures, volume,
account);
+ final int retryInterval =
getScopedConfigValue(SnapshotManager.SnapshotRecurringRetryInterval, volume,
account);
+ final int totalFailures = countConsecutiveFailedAttempts(volumeId,
maxFailures) + 1;
+
+ final String failureMessage = String.format("Failed to create
scheduled snapshot for volume [%s]: %s", volume, cause.getMessage());
+ if (eventId != null) {
+ ActionEventUtils.onCompletedActionEvent(User.UID_SYSTEM,
volume.getAccountId(), EventVO.LEVEL_ERROR,
+ EventTypes.EVENT_SNAPSHOT_CREATE, failureMessage,
volumeId, ApiCommandResourceType.Volume.toString(), eventId);
+ } else {
+ ActionEventUtils.onCreatedActionEvent(User.UID_SYSTEM,
volume.getAccountId(), EventVO.LEVEL_ERROR,
+ EventTypes.EVENT_SNAPSHOT_CREATE, true, failureMessage,
volumeId, ApiCommandResourceType.Volume.toString());
+ }
+
+ if (maxFailures > 0 && totalFailures >= maxFailures) {
+ final Date nextRegularRun =
getNextScheduledTime(snapshotToBeExecuted.getPolicyId(), _currentTimestamp);
+ lockedSchedule.setScheduledTimestamp(nextRegularRun);
+ logger.warn("Snapshot schedule [{}] for volume [{}] has failed
[{}] consecutive times; it will not be retried until its next regularly
scheduled run at [{}].",
+ snapshotToBeExecuted, volume, totalFailures,
nextRegularRun);
+ ActionEventUtils.onCreatedActionEvent(User.UID_SYSTEM,
volume.getAccountId(), EventVO.LEVEL_WARN, EventTypes.EVENT_SNAPSHOT_CREATE,
true,
+ String.format("Recurring snapshot for volume [%s] has
failed %d consecutive times and will not be retried until its next regularly
scheduled run.", volume, totalFailures),
+ volumeId, ApiCommandResourceType.Volume.toString());
+ } else {
+ final Date nextRetry = new Date(_currentTimestamp.getTime() +
retryInterval * 1000L);
+ lockedSchedule.setScheduledTimestamp(nextRetry);
+ logger.debug("Snapshot schedule [{}] for volume [{}] failed [{}]
time(s); retrying at [{}].",
+ snapshotToBeExecuted, volume, totalFailures, nextRetry);
+ }
+ _snapshotScheduleDao.update(lockedSchedule.getId(), lockedSchedule);
+ }
+
+ /**
+ * Counts how many of the most recent {@code EVENT_SNAPSHOT_CREATE} events
logged for this volume are
+ * consecutive failures (level ERROR), starting from the most recent event
and stopping at the first
+ * non-failure (or absent) event. This derives the "number of failed
attempts" from event history instead of a
+ * dedicated counter column.
+ */
+ protected int countConsecutiveFailedAttempts(final long volumeId, final
int limit) {
+ if (limit <= 0) {
+ return 0;
+ }
+ final List<EventVO> recentEvents =
_eventDao.listLatestEventsByResource(volumeId,
ApiCommandResourceType.Volume.toString(),
+ EventTypes.EVENT_SNAPSHOT_CREATE, limit);
+ int count = 0;
+ for (final EventVO event : recentEvents) {
+ if (!EventVO.LEVEL_ERROR.equals(event.getLevel())) {
+ break;
+ }
+ count++;
+ }
+ return count;
+ }
+
+ /**
+ * Resolves a config value in order of most to least specific scope:
account, domain, zone, then global. A
+ * {@link ConfigKey} can only walk a single scope-parent chain
automatically (Account->Domain->Global, or
+ * Zone->Global), so the four scopes are resolved manually here.
+ */
+ protected <T> T getScopedConfigValue(final ConfigKey<T> key, final
VolumeVO volume, final Account account) {
+ T value = key.valueInScope(ConfigKey.Scope.Account,
volume.getAccountId(), true);
+ if (value == null && account != null) {
+ value = key.valueInScope(ConfigKey.Scope.Domain,
account.getDomainId(), true);
+ }
+ if (value == null) {
+ value = key.valueInScope(ConfigKey.Scope.Zone,
volume.getDataCenterId(), true);
+ }
+ if (value == null) {
+ value = key.value();
+ }
+ return value;
+ }
Review Comment:
`getScopedConfigValue` is intended to manually resolve Account → Domain →
Zone → Global, but passing `true` to `valueInScope` typically enables fallback
to defaults up the scope chain. That means the first call (Account scope) can
return a Domain/Global value and prevent Zone overrides from ever being
considered. Use non-fallback lookups for Account/Domain/Zone (e.g.,
`useDefault=false`) and only fall back to `key.value()` at the end, so
Zone-scoped config is honored.
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java:
##########
@@ -206,6 +220,40 @@ protected void
scheduleNextSnapshotJobIfNecessary(SnapshotScheduleVO snapshotSch
scheduleNextSnapshotJob(snapshotSchedule);
}
+ /**
+ * Logs an event for the outcome of a recurring snapshot job (keyed by the
volume, since a fresh snapshot entity
+ * ID is minted on every attempt) so that consecutive failures can be
counted from event history, and raises a
+ * WARN notification once {@link
SnapshotManager#SnapshotRecurringMaxFailures} consecutive failures are reached.
+ */
+ protected void recordSnapshotAttemptOutcome(final SnapshotScheduleVO
snapshotSchedule, final boolean succeeded, final String failureResult) {
+ final VolumeVO volume =
_volsDao.findByIdIncludingRemoved(snapshotSchedule.getVolumeId());
+ if (volume == null) {
+ return;
+ }
+
+ if (succeeded) {
+ ActionEventUtils.onCreatedActionEvent(User.UID_SYSTEM,
volume.getAccountId(), EventVO.LEVEL_INFO, EventTypes.EVENT_SNAPSHOT_CREATE,
true,
+ String.format("Scheduled snapshot creation job for volume
[%s] succeeded.", volume),
+ volume.getId(), ApiCommandResourceType.Volume.toString());
+ return;
+ }
+
+ final Account account = _acctDao.findById(volume.getAccountId());
+ final int maxFailures =
getScopedConfigValue(SnapshotManager.SnapshotRecurringMaxFailures, volume,
account);
+ final int totalFailures =
countConsecutiveFailedAttempts(volume.getId(), maxFailures) + 1;
+
+ ActionEventUtils.onCreatedActionEvent(User.UID_SYSTEM,
volume.getAccountId(), EventVO.LEVEL_ERROR, EventTypes.EVENT_SNAPSHOT_CREATE,
true,
+ String.format("Scheduled snapshot creation job for volume [%s]
failed: %s", volume, failureResult),
+ volume.getId(), ApiCommandResourceType.Volume.toString());
+
+ if (maxFailures > 0 && totalFailures >= maxFailures) {
+ logger.warn("Snapshot schedule [{}] for volume [{}] has failed
[{}] consecutive times.", snapshotSchedule, volume, totalFailures);
+ ActionEventUtils.onCreatedActionEvent(User.UID_SYSTEM,
volume.getAccountId(), EventVO.LEVEL_WARN, EventTypes.EVENT_SNAPSHOT_CREATE,
true,
+ String.format("Recurring snapshot for volume [%s] has
failed %d consecutive times.", volume, totalFailures),
+ volume.getId(), ApiCommandResourceType.Volume.toString());
+ }
Review Comment:
The WARN notification is logged using the same
`EventTypes.EVENT_SNAPSHOT_CREATE` type that `countConsecutiveFailedAttempts`
scans, and `countConsecutiveFailedAttempts` stops at the first non-ERROR level.
This means once a WARN is emitted, the latest event is no longer an ERROR and
the failure counter effectively resets, causing threshold behavior to repeat
incorrectly and undermining “consecutive failures” tracking. Consider logging
threshold notifications under a different event type (recommended), or update
the counting logic to skip/ignore WARN/INFO notification events and fetch
enough history to still count the latest N ERROR attempts accurately.
##########
engine/schema/src/main/java/com/cloud/event/dao/EventDaoImpl.java:
##########
@@ -105,6 +112,16 @@ public List<EventVO>
listToArchiveOrDeleteEvents(List<Long> ids, String type, Da
return search(sc, null);
}
+ @Override
+ public List<EventVO> listLatestEventsByResource(long resourceId, String
resourceType, String type, int limit) {
+ SearchCriteria<EventVO> sc = LatestEventsByResourceSearch.create();
+ sc.setParameters("resourceId", resourceId);
+ sc.setParameters("resourceType", resourceType);
+ sc.setParameters("type", type);
+ Filter filter = new Filter(EventVO.class, "createDate", false, 0L,
(long) limit);
+ return listBy(sc, filter);
+ }
Review Comment:
`listLatestEventsByResource` doesn’t constrain results to non-archived
events, even though `EventVO` supports archiving and other searches in this DAO
explicitly filter by `archived`. This can make consecutive-failure counting
incorrect (e.g., counting old archived failures) and may be surprising for
callers expecting “recent” active events. Add an `archived=false` criterion to
`LatestEventsByResourceSearch` (or otherwise ensure only non-archived events
are returned).
--
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]
