Copilot commented on code in PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#discussion_r4445194368
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotManager.java:
##########
@@ -53,6 +55,18 @@ public interface SnapshotManager extends Configurable {
public static final ConfigKey<Integer> BackupRetryInterval = new
ConfigKey<Integer>(Integer.class, "backup.retry.interval", "Advanced", "300",
"Time in seconds between retries in backing up snapshot to
secondary", false, ConfigKey.Scope.Global, null);
+ ConfigKey<Integer> SnapshotRecurringMaxFailures = new
ConfigKey<>(Integer.class, "snapshot.recurring.max.failures", "Snapshots", "3",
+ "Maximum number of consecutive failed attempts allowed for a
recurring snapshot before it is left until its next regularly scheduled run and
a failure event is logged. Set to 0 to retry indefinitely.",
+ true, List.of(ConfigKey.Scope.Account, ConfigKey.Scope.Domain,
ConfigKey.Scope.Zone, ConfigKey.Scope.Global), null);
+
+ ConfigKey<Integer> SnapshotRecurringRetryInterval = new
ConfigKey<>(Integer.class, "snapshot.recurring.retry.interval", "Snapshots",
"300",
+ "Time in seconds to wait before retrying a failed recurring
snapshot attempt.",
+ true, List.of(ConfigKey.Scope.Account, ConfigKey.Scope.Domain,
ConfigKey.Scope.Zone, ConfigKey.Scope.Global), null);
+
+ ConfigKey<Boolean> SnapshotSkipIfVmNotRunning = new
ConfigKey<>(Boolean.class, "snapshot.skip.if.vm.not.running", "Snapshots",
"false",
+ "For recurring snapshots, skip taking a new snapshot of a volume
(and log a skipped event instead) when the VM it is attached to has not been
running since the last snapshot was taken, since nothing on the volume could
have changed.",
+ true, List.of(ConfigKey.Scope.Account, ConfigKey.Scope.Domain,
ConfigKey.Scope.Zone, ConfigKey.Scope.Global), null);
Review Comment:
Including `Scope.Global` alongside non-global scopes makes
`ConfigDepotImpl.populateConfiguration()` dereference a missing entry in its
scope map, aborting management-server startup. Remove `Scope.Global` from all
three new scope lists (also lines 64 and 68); global values are supported
implicitly.
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java:
##########
@@ -297,6 +320,230 @@ protected void scheduleSnapshots() {
}
}
+ /**
+ * Returns true (after rescheduling as needed) when this iteration's
snapshot should not be dispatched: either
+ * because it can't be scheduled at all, or because it's a redundant
snapshot of an unchanged volume that gets
+ * skipped and rescheduled to its next regular run instead. Kept as a
single decision point so the caller only
+ * needs one {@code continue}.
+ */
+ protected boolean shouldSkipSchedule(final SnapshotScheduleVO
snapshotToBeExecuted, final VolumeVO volume) {
+ if (!canSnapshotBeScheduled(snapshotToBeExecuted, volume)) {
+ return true;
+ }
+ if (shouldSkipUnchangedVolumeSnapshot(volume)) {
+ skipAndRescheduleSnapshot(snapshotToBeExecuted, volume);
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Logs the "scheduled" action event for a recurring snapshot of the
volume and returns its id, which
+ * {@link #dispatchSnapshotCreateJob} passes on to the job and {@link
#handleFailedSnapshotDispatch} completes if
+ * dispatching fails.
+ */
+ protected Long createScheduledSnapshotEvent(final VolumeVO volume) {
+ return ActionEventUtils.onScheduledActionEvent(User.UID_SYSTEM,
volume.getAccountId(), EventTypes.EVENT_SNAPSHOT_CREATE, "creating snapshot for
volume Id:" +
+ volume.getUuid(), volume.getId(),
ApiCommandResourceType.Volume.toString(), true, 0);
+ }
+
+ /**
+ * Builds and dispatches the CreateSnapshotCmd async job for a scheduled
snapshot, started from the given
+ * "scheduled" action event.
+ */
+ protected void dispatchSnapshotCreateJob(final SnapshotScheduleVO
snapshotToBeExecuted, final VolumeVO volume, final SnapshotScheduleVO
tmpSnapshotScheduleVO,
+ final Long eventId) throws Exception {
+ final long snapshotScheId = snapshotToBeExecuted.getId();
+ final long policyId = snapshotToBeExecuted.getPolicyId();
+ final long volumeId = snapshotToBeExecuted.getVolumeId();
+
+ logger.trace("Mapping parameters required to generate a
CreateSnapshotCmd for snapshot [{}].", snapshotToBeExecuted);
+ final Map<String, String> params = new HashMap<String, String>();
+ params.put(ApiConstants.VOLUME_ID, "" + volumeId);
+ params.put(ApiConstants.POLICY_ID, "" + policyId);
+ params.put("ctxUserId", "1");
+ params.put("ctxAccountId", "" + volume.getAccountId());
+ params.put("ctxStartEventId", String.valueOf(eventId));
+ List<? extends ResourceTag> resourceTags =
taggedResourceService.listByResourceTypeAndId(ResourceTag.ResourceObjectType.SnapshotPolicy,
policyId);
+ if (resourceTags != null && !resourceTags.isEmpty()) {
+ int tagNumber = 0;
+ for (ResourceTag resourceTag : resourceTags) {
+ params.put("tags[" + tagNumber + "].key",
resourceTag.getKey());
+ params.put("tags[" + tagNumber + "].value",
resourceTag.getValue());
+ tagNumber++;
+ }
+ }
+
+ logger.trace("Generating a CreateSnapshotCmd for snapshot [{}] with
parameters: [{}].", snapshotToBeExecuted, params.toString());
+ final CreateSnapshotCmd cmd = new CreateSnapshotCmd();
+ ComponentContext.inject(cmd);
+ _dispatcher.dispatchCreateCmd(cmd, params);
+ params.put("id", "" + cmd.getEntityId());
+ params.put("ctxStartEventId", "1");
+
+ final Date scheduledTimestamp =
snapshotToBeExecuted.getScheduledTimestamp();
+ final String displayTime =
DateUtil.displayDateInTimezone(DateUtil.GMT_TIMEZONE, scheduledTimestamp);
+ logger.debug("Scheduling snapshot [{}] for volume [{}] at [{}].",
snapshotToBeExecuted, volume, displayTime);
+ AsyncJobVO job = new AsyncJobVO("", User.UID_SYSTEM,
volume.getAccountId(), CreateSnapshotCmd.class.getName(),
+ ApiGsonHelper.getBuilder().create().toJson(params),
cmd.getEntityId(),
+ cmd.getApiResourceType() != null ?
cmd.getApiResourceType().toString() : null, null);
+ job.setDispatcher(_asyncDispatcher.getName());
+ final long jobId = _asyncMgr.submitAsyncJob(job);
+ logger.debug("Scheduled snapshot [{}] for volume [{}] as job [{}].",
snapshotToBeExecuted, volume, job);
+
+ tmpSnapshotScheduleVO.setAsyncJobId(jobId);
+ _snapshotScheduleDao.update(snapshotScheId, tmpSnapshotScheduleVO);
+ }
+
+ /**
+ * 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);
+ raiseFailureLimitReachedEvent(volume, 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));
+ } 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}
outcome 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. The Scheduled event logged before every
dispatch is not an outcome and is
+ * excluded by the DAO, otherwise it would interrupt every run of
failures. 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.listLatestOutcomeEventsByResource(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;
+ }
+
+ /**
+ * Implements https://github.com/apache/cloudstack/issues/6827: a
recurring snapshot is redundant when nothing
+ * could have changed on the volume since the last one was taken, i.e.
when the attached VM has not been running
+ * at any point since then. Volumes with no snapshot yet, or that are not
attached to a VM, are never skipped.
+ */
+ protected boolean shouldSkipUnchangedVolumeSnapshot(final VolumeVO volume)
{
+ final Account account = _acctDao.findById(volume.getAccountId());
+ if
(!Boolean.TRUE.equals(getScopedConfigValue(SnapshotManager.SnapshotSkipIfVmNotRunning,
volume, account))) {
+ return false;
+ }
+
+ final Long instanceId = volume.getInstanceId();
+ if (instanceId == null) {
+ return false;
+ }
+
+ final SnapshotVO lastSnapshot = findLastSnapshot(volume.getId());
+ if (lastSnapshot == null) {
+ return false;
+ }
+
+ final VMInstanceVO vm = vmInstanceDao.findById(instanceId);
+ if (vm == null || vm.getPowerState() ==
VirtualMachine.PowerState.PowerOn) {
Review Comment:
This also treats `PowerUnknown` as powered off. If that state predates the
last snapshot, the timestamp check below skips future recurring snapshots even
though an unknown power state does not establish that the VM stopped running.
Please require a confirmed powered-off state before using its timestamp to
suppress backups.
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java:
##########
@@ -297,6 +320,230 @@ protected void scheduleSnapshots() {
}
}
+ /**
+ * Returns true (after rescheduling as needed) when this iteration's
snapshot should not be dispatched: either
+ * because it can't be scheduled at all, or because it's a redundant
snapshot of an unchanged volume that gets
+ * skipped and rescheduled to its next regular run instead. Kept as a
single decision point so the caller only
+ * needs one {@code continue}.
+ */
+ protected boolean shouldSkipSchedule(final SnapshotScheduleVO
snapshotToBeExecuted, final VolumeVO volume) {
+ if (!canSnapshotBeScheduled(snapshotToBeExecuted, volume)) {
+ return true;
+ }
+ if (shouldSkipUnchangedVolumeSnapshot(volume)) {
+ skipAndRescheduleSnapshot(snapshotToBeExecuted, volume);
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Logs the "scheduled" action event for a recurring snapshot of the
volume and returns its id, which
+ * {@link #dispatchSnapshotCreateJob} passes on to the job and {@link
#handleFailedSnapshotDispatch} completes if
+ * dispatching fails.
+ */
+ protected Long createScheduledSnapshotEvent(final VolumeVO volume) {
+ return ActionEventUtils.onScheduledActionEvent(User.UID_SYSTEM,
volume.getAccountId(), EventTypes.EVENT_SNAPSHOT_CREATE, "creating snapshot for
volume Id:" +
+ volume.getUuid(), volume.getId(),
ApiCommandResourceType.Volume.toString(), true, 0);
+ }
+
+ /**
+ * Builds and dispatches the CreateSnapshotCmd async job for a scheduled
snapshot, started from the given
+ * "scheduled" action event.
+ */
+ protected void dispatchSnapshotCreateJob(final SnapshotScheduleVO
snapshotToBeExecuted, final VolumeVO volume, final SnapshotScheduleVO
tmpSnapshotScheduleVO,
+ final Long eventId) throws Exception {
+ final long snapshotScheId = snapshotToBeExecuted.getId();
+ final long policyId = snapshotToBeExecuted.getPolicyId();
+ final long volumeId = snapshotToBeExecuted.getVolumeId();
+
+ logger.trace("Mapping parameters required to generate a
CreateSnapshotCmd for snapshot [{}].", snapshotToBeExecuted);
+ final Map<String, String> params = new HashMap<String, String>();
+ params.put(ApiConstants.VOLUME_ID, "" + volumeId);
+ params.put(ApiConstants.POLICY_ID, "" + policyId);
+ params.put("ctxUserId", "1");
+ params.put("ctxAccountId", "" + volume.getAccountId());
+ params.put("ctxStartEventId", String.valueOf(eventId));
+ List<? extends ResourceTag> resourceTags =
taggedResourceService.listByResourceTypeAndId(ResourceTag.ResourceObjectType.SnapshotPolicy,
policyId);
+ if (resourceTags != null && !resourceTags.isEmpty()) {
+ int tagNumber = 0;
+ for (ResourceTag resourceTag : resourceTags) {
+ params.put("tags[" + tagNumber + "].key",
resourceTag.getKey());
+ params.put("tags[" + tagNumber + "].value",
resourceTag.getValue());
+ tagNumber++;
+ }
+ }
+
+ logger.trace("Generating a CreateSnapshotCmd for snapshot [{}] with
parameters: [{}].", snapshotToBeExecuted, params.toString());
+ final CreateSnapshotCmd cmd = new CreateSnapshotCmd();
+ ComponentContext.inject(cmd);
+ _dispatcher.dispatchCreateCmd(cmd, params);
+ params.put("id", "" + cmd.getEntityId());
+ params.put("ctxStartEventId", "1");
+
+ final Date scheduledTimestamp =
snapshotToBeExecuted.getScheduledTimestamp();
+ final String displayTime =
DateUtil.displayDateInTimezone(DateUtil.GMT_TIMEZONE, scheduledTimestamp);
+ logger.debug("Scheduling snapshot [{}] for volume [{}] at [{}].",
snapshotToBeExecuted, volume, displayTime);
+ AsyncJobVO job = new AsyncJobVO("", User.UID_SYSTEM,
volume.getAccountId(), CreateSnapshotCmd.class.getName(),
+ ApiGsonHelper.getBuilder().create().toJson(params),
cmd.getEntityId(),
+ cmd.getApiResourceType() != null ?
cmd.getApiResourceType().toString() : null, null);
+ job.setDispatcher(_asyncDispatcher.getName());
+ final long jobId = _asyncMgr.submitAsyncJob(job);
+ logger.debug("Scheduled snapshot [{}] for volume [{}] as job [{}].",
snapshotToBeExecuted, volume, job);
+
+ tmpSnapshotScheduleVO.setAsyncJobId(jobId);
+ _snapshotScheduleDao.update(snapshotScheId, tmpSnapshotScheduleVO);
+ }
+
+ /**
+ * 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);
+ raiseFailureLimitReachedEvent(volume, 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));
+ } 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}
outcome 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. The Scheduled event logged before every
dispatch is not an outcome and is
+ * excluded by the DAO, otherwise it would interrupt every run of
failures. 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.listLatestOutcomeEventsByResource(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;
+ }
+
+ /**
+ * Implements https://github.com/apache/cloudstack/issues/6827: a
recurring snapshot is redundant when nothing
+ * could have changed on the volume since the last one was taken, i.e.
when the attached VM has not been running
+ * at any point since then. Volumes with no snapshot yet, or that are not
attached to a VM, are never skipped.
+ */
+ protected boolean shouldSkipUnchangedVolumeSnapshot(final VolumeVO volume)
{
+ final Account account = _acctDao.findById(volume.getAccountId());
+ if
(!Boolean.TRUE.equals(getScopedConfigValue(SnapshotManager.SnapshotSkipIfVmNotRunning,
volume, account))) {
+ return false;
+ }
+
+ final Long instanceId = volume.getInstanceId();
+ if (instanceId == null) {
+ return false;
+ }
+
+ final SnapshotVO lastSnapshot = findLastSnapshot(volume.getId());
+ if (lastSnapshot == null) {
+ return false;
+ }
+
+ final VMInstanceVO vm = vmInstanceDao.findById(instanceId);
+ if (vm == null || vm.getPowerState() ==
VirtualMachine.PowerState.PowerOn) {
+ return false;
+ }
+
+ final Date poweredOffSince = vm.getPowerStateUpdateTime();
+ // If the VM's power state changed at or after the last snapshot, it
may have been running (and the volume
+ // may have changed) at some point since; only skip when it has been
off since strictly before that snapshot.
+ return poweredOffSince != null &&
poweredOffSince.before(lastSnapshot.getCreated());
Review Comment:
If a data volume is modified after its snapshot on VM A, then attached to VM
B that has been off since before that snapshot, this check skips backups of
those unprotected writes. Also require the current attachment to predate the
snapshot, and keep taking snapshots when the attachment time is unknown;
`VolumeDaoImpl.attachVolume()` records that time in `attached`.
##########
engine/schema/src/main/java/com/cloud/event/dao/EventDaoImpl.java:
##########
@@ -125,6 +137,20 @@ public List<EventVO>
listToArchiveOrDeleteEvents(List<Long> ids, String type, Da
return search(sc, null);
}
+ @Override
+ public List<EventVO> listLatestOutcomeEventsByResource(long resourceId,
String resourceType, String type, int limit) {
+ SearchCriteria<EventVO> sc = LatestEventsByResourceSearch.create();
+ sc.setParameters(FIELD_RESOURCE_ID, resourceId);
+ sc.setParameters(FIELD_RESOURCE_TYPE, resourceType);
+ sc.setParameters("type", type);
+ sc.setParameters("state", State.Created, State.Completed);
+ sc.setParameters("archived", false);
+ Filter filter = new Filter(EventVO.class, "createDate", false, 0L,
(long) limit);
+ // created has one-second granularity; break ties by insertion order
+ filter.addOrderBy(EventVO.class, "id", false);
+ return listBy(sc, filter);
Review Comment:
The event table has no resource-scoped index, so a failed volume with no
matching outcome history can require scanning retained events despite the
`LIMIT`. These lookups run under the global `snapshot.poll` lock, so many
failing volumes can delay all snapshot scheduling. Add a schema migration with
a composite index supporting the resource/type predicates and newest-first
ordering.
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java:
##########
@@ -297,6 +320,230 @@ protected void scheduleSnapshots() {
}
}
+ /**
+ * Returns true (after rescheduling as needed) when this iteration's
snapshot should not be dispatched: either
+ * because it can't be scheduled at all, or because it's a redundant
snapshot of an unchanged volume that gets
+ * skipped and rescheduled to its next regular run instead. Kept as a
single decision point so the caller only
+ * needs one {@code continue}.
+ */
+ protected boolean shouldSkipSchedule(final SnapshotScheduleVO
snapshotToBeExecuted, final VolumeVO volume) {
+ if (!canSnapshotBeScheduled(snapshotToBeExecuted, volume)) {
+ return true;
+ }
+ if (shouldSkipUnchangedVolumeSnapshot(volume)) {
+ skipAndRescheduleSnapshot(snapshotToBeExecuted, volume);
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Logs the "scheduled" action event for a recurring snapshot of the
volume and returns its id, which
+ * {@link #dispatchSnapshotCreateJob} passes on to the job and {@link
#handleFailedSnapshotDispatch} completes if
+ * dispatching fails.
+ */
+ protected Long createScheduledSnapshotEvent(final VolumeVO volume) {
+ return ActionEventUtils.onScheduledActionEvent(User.UID_SYSTEM,
volume.getAccountId(), EventTypes.EVENT_SNAPSHOT_CREATE, "creating snapshot for
volume Id:" +
+ volume.getUuid(), volume.getId(),
ApiCommandResourceType.Volume.toString(), true, 0);
+ }
+
+ /**
+ * Builds and dispatches the CreateSnapshotCmd async job for a scheduled
snapshot, started from the given
+ * "scheduled" action event.
+ */
+ protected void dispatchSnapshotCreateJob(final SnapshotScheduleVO
snapshotToBeExecuted, final VolumeVO volume, final SnapshotScheduleVO
tmpSnapshotScheduleVO,
+ final Long eventId) throws Exception {
+ final long snapshotScheId = snapshotToBeExecuted.getId();
+ final long policyId = snapshotToBeExecuted.getPolicyId();
+ final long volumeId = snapshotToBeExecuted.getVolumeId();
+
+ logger.trace("Mapping parameters required to generate a
CreateSnapshotCmd for snapshot [{}].", snapshotToBeExecuted);
+ final Map<String, String> params = new HashMap<String, String>();
+ params.put(ApiConstants.VOLUME_ID, "" + volumeId);
+ params.put(ApiConstants.POLICY_ID, "" + policyId);
+ params.put("ctxUserId", "1");
+ params.put("ctxAccountId", "" + volume.getAccountId());
+ params.put("ctxStartEventId", String.valueOf(eventId));
+ List<? extends ResourceTag> resourceTags =
taggedResourceService.listByResourceTypeAndId(ResourceTag.ResourceObjectType.SnapshotPolicy,
policyId);
+ if (resourceTags != null && !resourceTags.isEmpty()) {
+ int tagNumber = 0;
+ for (ResourceTag resourceTag : resourceTags) {
+ params.put("tags[" + tagNumber + "].key",
resourceTag.getKey());
+ params.put("tags[" + tagNumber + "].value",
resourceTag.getValue());
+ tagNumber++;
+ }
+ }
+
+ logger.trace("Generating a CreateSnapshotCmd for snapshot [{}] with
parameters: [{}].", snapshotToBeExecuted, params.toString());
+ final CreateSnapshotCmd cmd = new CreateSnapshotCmd();
+ ComponentContext.inject(cmd);
+ _dispatcher.dispatchCreateCmd(cmd, params);
+ params.put("id", "" + cmd.getEntityId());
+ params.put("ctxStartEventId", "1");
+
+ final Date scheduledTimestamp =
snapshotToBeExecuted.getScheduledTimestamp();
+ final String displayTime =
DateUtil.displayDateInTimezone(DateUtil.GMT_TIMEZONE, scheduledTimestamp);
+ logger.debug("Scheduling snapshot [{}] for volume [{}] at [{}].",
snapshotToBeExecuted, volume, displayTime);
+ AsyncJobVO job = new AsyncJobVO("", User.UID_SYSTEM,
volume.getAccountId(), CreateSnapshotCmd.class.getName(),
+ ApiGsonHelper.getBuilder().create().toJson(params),
cmd.getEntityId(),
+ cmd.getApiResourceType() != null ?
cmd.getApiResourceType().toString() : null, null);
+ job.setDispatcher(_asyncDispatcher.getName());
+ final long jobId = _asyncMgr.submitAsyncJob(job);
+ logger.debug("Scheduled snapshot [{}] for volume [{}] as job [{}].",
snapshotToBeExecuted, volume, job);
+
+ tmpSnapshotScheduleVO.setAsyncJobId(jobId);
+ _snapshotScheduleDao.update(snapshotScheId, tmpSnapshotScheduleVO);
+ }
+
+ /**
+ * 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);
+ raiseFailureLimitReachedEvent(volume, 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));
+ } 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}
outcome 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. The Scheduled event logged before every
dispatch is not an outcome and is
+ * excluded by the DAO, otherwise it would interrupt every run of
failures. 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.listLatestOutcomeEventsByResource(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;
+ }
+
+ /**
+ * Implements https://github.com/apache/cloudstack/issues/6827: a
recurring snapshot is redundant when nothing
+ * could have changed on the volume since the last one was taken, i.e.
when the attached VM has not been running
+ * at any point since then. Volumes with no snapshot yet, or that are not
attached to a VM, are never skipped.
+ */
+ protected boolean shouldSkipUnchangedVolumeSnapshot(final VolumeVO volume)
{
+ final Account account = _acctDao.findById(volume.getAccountId());
+ if
(!Boolean.TRUE.equals(getScopedConfigValue(SnapshotManager.SnapshotSkipIfVmNotRunning,
volume, account))) {
+ return false;
+ }
+
+ final Long instanceId = volume.getInstanceId();
+ if (instanceId == null) {
+ return false;
+ }
+
+ final SnapshotVO lastSnapshot = findLastSnapshot(volume.getId());
+ if (lastSnapshot == null) {
+ return false;
+ }
+
+ final VMInstanceVO vm = vmInstanceDao.findById(instanceId);
+ if (vm == null || vm.getPowerState() ==
VirtualMachine.PowerState.PowerOn) {
+ return false;
+ }
+
+ final Date poweredOffSince = vm.getPowerStateUpdateTime();
+ // If the VM's power state changed at or after the last snapshot, it
may have been running (and the volume
+ // may have changed) at some point since; only skip when it has been
off since strictly before that snapshot.
+ return poweredOffSince != null &&
poweredOffSince.before(lastSnapshot.getCreated());
+ }
+
+ protected SnapshotVO findLastSnapshot(final long volumeId) {
+ final Filter filter = new Filter(SnapshotVO.class, "created", false,
0L, 1L);
+ final List<SnapshotVO> snapshots = _snapshotDao.listByVolumeId(filter,
volumeId);
Review Comment:
`listByVolumeId` does not filter snapshot state, so this can return an
`Error` or merely `Allocated` snapshot left by a failed attempt. With skipping
enabled, a stopped VM's failed first snapshot then becomes the baseline, and
every subsequent recurring run is skipped despite there being no usable
snapshot. Please restrict the query to successfully taken snapshots before
applying the one-row limit.
--
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]
