deal with excessive attempts on failing snapshots - #13743
Open
DaanHoogland wants to merge 2 commits into
Open
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13743 +/- ##
============================================
+ Coverage 19.64% 19.65% +0.01%
- Complexity 19796 19803 +7
============================================
Files 6368 6368
Lines 575119 575020 -99
Branches 70382 70371 -11
============================================
+ Hits 112994 113044 +50
+ Misses 449839 449695 -144
+ Partials 12286 12281 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR adds logic to reduce excessive retry attempts for failing recurring snapshots by tracking outcomes via event history, applying configurable retry/backoff behavior for dispatch failures, and optionally skipping snapshots when a VM hasn’t been running since the last snapshot.
Changes:
- Add configurable max-failure and retry-interval handling for recurring snapshot dispatch failures (and log outcomes as events for consecutive-failure counting).
- Introduce optional “skip snapshot if VM not running since last snapshot” behavior plus a new
SNAPSHOT.SKIPPEDevent type. - Add DAO support to query recent events per resource, and add unit tests covering the new behaviors.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java | Adds failure counting, scoped config resolution, dispatch failure handling with backoff/give-up, and unchanged-volume skip logic. |
| server/src/test/java/com/cloud/storage/snapshot/SnapshotSchedulerImplTest.java | Adds unit tests for consecutive failure counting, scoped config fallback, dispatch failure behavior, and skip logic. |
| server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java | Registers new snapshot-related config keys. |
| server/src/main/java/com/cloud/storage/snapshot/SnapshotManager.java | Defines new scoped config keys for recurring snapshot failure handling and skip behavior. |
| engine/schema/src/main/java/com/cloud/event/dao/EventDaoImpl.java | Implements querying latest events per resource for failure counting. |
| engine/schema/src/main/java/com/cloud/event/dao/EventDao.java | Adds DAO API for latest-events-by-resource lookup. |
| api/src/main/java/com/cloud/event/EventTypes.java | Adds EVENT_SNAPSHOT_SKIPPED constant and registers it for entity-event details. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.52% |
| Branch coverage | 18.68% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
DaanHoogland
marked this pull request as ready for review
July 29, 2026 16:05
- Use a distinct event type (SNAPSHOT.RECURRING.FAILURE.LIMIT.REACHED) for the "gave up" WARN notification instead of EVENT_SNAPSHOT_CREATE, since that event became the latest EVENT_SNAPSHOT_CREATE entry and silently reset countConsecutiveFailedAttempts on the next scan. - Filter out archived events in EventDao#listLatestEventsByResource so stale archived events can't be counted as the latest failure history, matching the archived=false filtering already used by sibling searches in EventDaoImpl. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java:253
- The WARN notification emitted when the consecutive-failure threshold is reached uses
EventTypes.EVENT_SNAPSHOT_CREATE. BecausecountConsecutiveFailedAttemptsscans the latestEVENT_SNAPSHOT_CREATEevents and stops at the first non-ERROR level, this WARN event becomes part of the scanned stream and can unintentionally break/reset the consecutive-failure sequence. The PR already introduces a dedicated event type for this purpose; use it here as well.
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());
|
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.52% |
| Branch coverage | 18.68% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java:229
- New behavior is introduced in
recordSnapshotAttemptOutcome(event emission + consecutive-failure logic), but there’s no targeted unit test coverage validating (a) the correct event types/levels emitted for success/failure/limit-reached and (b) that consecutive-failure counting behaves correctly across the WARN notification boundary. Adding focused tests here would help prevent regressions (especially given the event stream is used as the ‘counter’ source of truth).
/**
* 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());
server/src/test/java/com/cloud/storage/snapshot/SnapshotSchedulerImplTest.java:340
- Test name uses ‘Depot’, which doesn’t match the concept under test (scoped config resolution across account/domain/zone/global). Consider renaming to something clearer like ‘WhenNoScopedOverrideConfigured’ or similar to reflect the actual behavior being verified.
public void getScopedConfigValueTestFallsBackToGlobalDefaultWhenNoDepotConfigured() {
Comment on lines
+426
to
+438
| 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; | ||
| } |
Comment on lines
+249
to
+254
| 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()); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR...
Fixes: #13454
Fixes: #6827
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?