Copilot commented on code in PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#discussion_r3682943831
##########
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_RECURRING_FAILURE_LIMIT_REACHED, 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:
The scoped fallback logic is likely bypassed by calling `valueInScope(...,
true)` for Account/Domain/Zone. If the boolean enables parent/default fallback,
`value` will rarely (or never) be null, so Domain/Zone/global precedence won’t
work as intended (e.g., a Zone override could be ignored because Account-scope
lookup already returned a default). Use a mode that returns null when not
explicitly set for that scope (or otherwise detect ‘unset’) for
Account/Domain/Zone, and only fall back to `key.value()` at the end.
##########
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 with `EventTypes.EVENT_SNAPSHOT_CREATE`,
which means it becomes part of the same event stream that
`countConsecutiveFailedAttempts(..., EVENT_SNAPSHOT_CREATE, ...)` scans. Since
it’s not LEVEL_ERROR, it will stop the scan and effectively reset the
consecutive-failure count on the next attempt. Log this WARN with a distinct
event type (e.g. `EVENT_SNAPSHOT_RECURRING_FAILURE_LIMIT_REACHED`, consistent
with `handleFailedSnapshotDispatch`) so failure counting remains accurate.
--
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]
