Skip to content

Commit b259a21

Browse files
DaanHooglandDaanHoogland
committed
deal with excesive attempts on failing snapshots
1 parent 6462ccf commit b259a21

5 files changed

Lines changed: 26 additions & 18 deletions

File tree

‎engine/schema/src/main/java/com/cloud/event/dao/EventDao.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,9 +49,10 @@ public interface EventDao extends GenericDao<EventVO, Long> {
4949
public void archiveEvents(List<EventVO> events);
5050

5151
/**
52-
* Returns the most recent events of the given type for a resource, newest first.
52+
* Returns the most recent outcome events (state Created or Completed) of the given type for a resource, newest first.
53+
* Scheduled and Started events are left out, as they precede every attempt and say nothing about how it ended.
5354
* Used to derive how many consecutive attempts have failed for that resource without a dedicated counter column.
5455
*/
55-
List<EventVO> listLatestEventsByResource(long resourceId, String resourceType, String type, int limit);
56+
List<EventVO> listLatestOutcomeEventsByResource(long resourceId, String resourceType, String type, int limit);
5657

5758
}

‎engine/schema/src/main/java/com/cloud/event/dao/EventDaoImpl.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ public EventDaoImpl() {
5555
LatestEventsByResourceSearch.and(FIELD_RESOURCE_ID, LatestEventsByResourceSearch.entity().getResourceId(), Op.EQ);
5656
LatestEventsByResourceSearch.and(FIELD_RESOURCE_TYPE, LatestEventsByResourceSearch.entity().getResourceType(), Op.EQ);
5757
LatestEventsByResourceSearch.and("type", LatestEventsByResourceSearch.entity().getType(), Op.EQ);
58+
LatestEventsByResourceSearch.and("state", LatestEventsByResourceSearch.entity().getState(), Op.IN);
5859
LatestEventsByResourceSearch.and("archived", LatestEventsByResourceSearch.entity().getArchived(), Op.EQ);
5960
LatestEventsByResourceSearch.done();
6061

@@ -137,13 +138,16 @@ public List<EventVO> listToArchiveOrDeleteEvents(List<Long> ids, String type, Da
137138
}
138139

139140
@Override
140-
public List<EventVO> listLatestEventsByResource(long resourceId, String resourceType, String type, int limit) {
141+
public List<EventVO> listLatestOutcomeEventsByResource(long resourceId, String resourceType, String type, int limit) {
141142
SearchCriteria<EventVO> sc = LatestEventsByResourceSearch.create();
142143
sc.setParameters(FIELD_RESOURCE_ID, resourceId);
143144
sc.setParameters(FIELD_RESOURCE_TYPE, resourceType);
144145
sc.setParameters("type", type);
146+
sc.setParameters("state", State.Created, State.Completed);
145147
sc.setParameters("archived", false);
146148
Filter filter = new Filter(EventVO.class, "createDate", false, 0L, (long) limit);
149+
// created has one-second granularity; break ties by insertion order
150+
filter.addOrderBy(EventVO.class, "id", false);
147151
return listBy(sc, filter);
148152
}
149153

‎engine/schema/src/test/java/com/cloud/event/dao/EventDaoImplTest.java‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
// under the License.
1717
package com.cloud.event.dao;
1818

19+
import com.cloud.event.Event;
1920
import com.cloud.event.EventVO;
2021
import com.cloud.utils.db.Filter;
2122
import com.cloud.utils.db.SearchCriteria;
@@ -44,11 +45,11 @@ public class EventDaoImplTest {
4445
EventDaoImpl eventDaoImpl = new EventDaoImpl();
4546

4647
@Test
47-
public void listLatestEventsByResourceQueriesByResourceAndTypeExcludingArchived() {
48+
public void listLatestOutcomeEventsByResourceQueriesOutcomesByResourceAndTypeExcludingArchived() {
4849
List<EventVO> expected = Collections.singletonList(new EventVO());
4950
doReturn(expected).when(eventDaoImpl).listBy(any(SearchCriteria.class), any(Filter.class));
5051

51-
List<EventVO> result = eventDaoImpl.listLatestEventsByResource(1L, "Volume", "SNAPSHOT.CREATE", 3);
52+
List<EventVO> result = eventDaoImpl.listLatestOutcomeEventsByResource(1L, "Volume", "SNAPSHOT.CREATE", 3);
5253

5354
Assert.assertSame(expected, result);
5455

@@ -57,26 +58,27 @@ public void listLatestEventsByResourceQueriesByResourceAndTypeExcludingArchived(
5758
verify(eventDaoImpl).listBy(scCaptor.capture(), filterCaptor.capture());
5859

5960
SearchCriteria<EventVO> sc = scCaptor.getValue();
60-
Assert.assertEquals("event.resource_id = ? AND event.resource_type = ? AND event.type = ? AND event.archived = ? ",
61+
Assert.assertEquals("event.resource_id = ? AND event.resource_type = ? AND event.type = ? AND event.state IN (?,?) AND event.archived = ? ",
6162
sc.getWhereClause());
6263
@SuppressWarnings("unchecked")
6364
Map<String, Object[]> params = (Map<String, Object[]>) ReflectionTestUtils.getField(sc, "_params");
6465
Assert.assertArrayEquals(new Object[]{1L}, params.get("resourceId"));
6566
Assert.assertArrayEquals(new Object[]{"Volume"}, params.get("resourceType"));
6667
Assert.assertArrayEquals(new Object[]{"SNAPSHOT.CREATE"}, params.get("type"));
68+
Assert.assertArrayEquals(new Object[]{Event.State.Created, Event.State.Completed}, params.get("state"));
6769
Assert.assertArrayEquals(new Object[]{false}, params.get("archived"));
6870

6971
Filter filter = filterCaptor.getValue();
70-
Assert.assertEquals(" ORDER BY event.created DESC ", filter.getOrderBy());
72+
Assert.assertEquals(" ORDER BY event.created DESC , event.id DESC ", filter.getOrderBy());
7173
Assert.assertEquals(Long.valueOf(0L), filter.getOffset());
7274
Assert.assertEquals(Long.valueOf(3L), filter.getLimit());
7375
}
7476

7577
@Test
76-
public void listLatestEventsByResourceReturnsEmptyListWhenNoneFound() {
78+
public void listLatestOutcomeEventsByResourceReturnsEmptyListWhenNoneFound() {
7779
doReturn(Collections.emptyList()).when(eventDaoImpl).listBy(any(SearchCriteria.class), any(Filter.class));
7880

79-
List<EventVO> result = eventDaoImpl.listLatestEventsByResource(1L, "Volume", "SNAPSHOT.CREATE", 3);
81+
List<EventVO> result = eventDaoImpl.listLatestOutcomeEventsByResource(1L, "Volume", "SNAPSHOT.CREATE", 3);
8082

8183
Assert.assertTrue(result.isEmpty());
8284
}

‎server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -430,16 +430,17 @@ protected void handleFailedSnapshotDispatch(final SnapshotScheduleVO snapshotToB
430430
}
431431

432432
/**
433-
* Counts how many of the most recent {@code EVENT_SNAPSHOT_CREATE} events logged for this volume are
433+
* Counts how many of the most recent {@code EVENT_SNAPSHOT_CREATE} outcome events logged for this volume are
434434
* consecutive failures (level ERROR), starting from the most recent event and stopping at the first
435-
* non-failure (or absent) event. This derives the "number of failed attempts" from event history instead of a
435+
* non-failure (or absent) event. The Scheduled event logged before every dispatch is not an outcome and is
436+
* 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
436437
* dedicated counter column.
437438
*/
438439
protected int countConsecutiveFailedAttempts(final long volumeId, final int limit) {
439440
if (limit <= 0) {
440441
return 0;
441442
}
442-
final List<EventVO> recentEvents = eventDao.listLatestEventsByResource(volumeId, ApiCommandResourceType.Volume.toString(),
443+
final List<EventVO> recentEvents = eventDao.listLatestOutcomeEventsByResource(volumeId, ApiCommandResourceType.Volume.toString(),
443444
EventTypes.EVENT_SNAPSHOT_CREATE, limit);
444445
int count = 0;
445446
for (final EventVO event : recentEvents) {

‎server/src/test/java/com/cloud/storage/snapshot/SnapshotSchedulerImplTest.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,7 @@ public void countConsecutiveFailedAttemptsTestLimitZeroReturnsZeroWithoutQueryin
333333
int result = snapshotSchedulerImplSpy.countConsecutiveFailedAttempts(1L, 0);
334334

335335
Assert.assertEquals(0, result);
336-
verify(eventDaoMock, never()).listLatestEventsByResource(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyInt());
336+
verify(eventDaoMock, never()).listLatestOutcomeEventsByResource(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyInt());
337337
}
338338

339339
@Test
@@ -345,7 +345,7 @@ public void countConsecutiveFailedAttemptsTestStopsAtFirstNonErrorEvent() {
345345
EventVO success = mock(EventVO.class);
346346
doReturn(EventVO.LEVEL_INFO).when(success).getLevel();
347347

348-
doReturn(List.of(error1, error2, success)).when(eventDaoMock).listLatestEventsByResource(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyInt());
348+
doReturn(List.of(error1, error2, success)).when(eventDaoMock).listLatestOutcomeEventsByResource(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyInt());
349349

350350
int result = snapshotSchedulerImplSpy.countConsecutiveFailedAttempts(1L, 3);
351351

@@ -354,7 +354,7 @@ public void countConsecutiveFailedAttemptsTestStopsAtFirstNonErrorEvent() {
354354

355355
@Test
356356
public void countConsecutiveFailedAttemptsTestNoEventsReturnsZero() {
357-
doReturn(Collections.emptyList()).when(eventDaoMock).listLatestEventsByResource(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyInt());
357+
doReturn(Collections.emptyList()).when(eventDaoMock).listLatestOutcomeEventsByResource(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyInt());
358358

359359
int result = snapshotSchedulerImplSpy.countConsecutiveFailedAttempts(1L, 3);
360360

@@ -390,7 +390,7 @@ private void stubConsecutiveFailureEvents(int count) {
390390
doReturn(EventVO.LEVEL_ERROR).when(event).getLevel();
391391
events.add(event);
392392
}
393-
doReturn(events).when(eventDaoMock).listLatestEventsByResource(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyInt());
393+
doReturn(events).when(eventDaoMock).listLatestOutcomeEventsByResource(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyInt());
394394
}
395395

396396
// Must use a distinct event type from EVENT_SNAPSHOT_CREATE, otherwise it becomes the "latest event" scanned
@@ -405,7 +405,7 @@ private void verifyFailureThresholdEventsRaised(MockedStatic<ActionEventUtils> a
405405
@Test
406406
public void handleFailedSnapshotDispatchTestUnderMaxReschedulesWithRetryIntervalOnly() {
407407
stubVolumeAndAccount();
408-
doReturn(Collections.emptyList()).when(eventDaoMock).listLatestEventsByResource(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyInt());
408+
doReturn(Collections.emptyList()).when(eventDaoMock).listLatestOutcomeEventsByResource(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyInt());
409409
ReflectionTestUtils.setField(snapshotSchedulerImplSpy, "_currentTimestamp", new Date());
410410

411411
try (MockedStatic<ActionEventUtils> actionEventUtilsMocked = Mockito.mockStatic(ActionEventUtils.class)) {
@@ -464,7 +464,7 @@ public void recordSnapshotAttemptOutcomeTestSucceededLogsInfoEventAndReturns() {
464464
}
465465

466466
verify(accountDaoMock, never()).findById(Mockito.anyLong());
467-
verify(eventDaoMock, never()).listLatestEventsByResource(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyInt());
467+
verify(eventDaoMock, never()).listLatestOutcomeEventsByResource(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyInt());
468468
}
469469

470470
// --- dispatchSnapshotCreateJob (#13454) ---

0 commit comments

Comments
 (0)