Skip to content

Commit 8b8ea82

Browse files
DaanHooglandDaanHoogland
committed
tests and review addressed
1 parent 6fa5160 commit 8b8ea82

4 files changed

Lines changed: 80 additions & 29 deletions

File tree

‎api/src/main/java/com/cloud/event/EventTypes.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1096,8 +1096,8 @@ public class EventTypes {
10961096

10971097
// Snapshots
10981098
entityEventDetails.put(EVENT_SNAPSHOT_CREATE, Snapshot.class);
1099-
entityEventDetails.put(EVENT_SNAPSHOT_SKIPPED, Snapshot.class);
1100-
entityEventDetails.put(EVENT_SNAPSHOT_RECURRING_FAILURE_LIMIT_REACHED, Snapshot.class);
1099+
entityEventDetails.put(EVENT_SNAPSHOT_SKIPPED, Volume.class);
1100+
entityEventDetails.put(EVENT_SNAPSHOT_RECURRING_FAILURE_LIMIT_REACHED, Volume.class);
11011101
entityEventDetails.put(EVENT_SNAPSHOT_DELETE, Snapshot.class);
11021102
entityEventDetails.put(EVENT_SNAPSHOT_EXTRACT, Snapshot.class);
11031103
entityEventDetails.put(EVENT_SNAPSHOT_ON_PRIMARY, Snapshot.class);

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,10 @@ public void listLatestOutcomeEventsByResourceQueriesOutcomesByResourceAndTypeExc
5858
verify(eventDaoImpl).listBy(scCaptor.capture(), filterCaptor.capture());
5959

6060
SearchCriteria<EventVO> sc = scCaptor.getValue();
61-
Assert.assertEquals("event.resource_id = ? AND event.resource_type = ? AND event.type = ? AND event.state IN (?,?) AND event.archived = ? ",
62-
sc.getWhereClause());
61+
String whereClause = sc.getWhereClause();
62+
for (String predicate : List.of("event.resource_id = ?", "event.resource_type = ?", "event.type = ?", "event.state IN (?,?)", "event.archived = ?")) {
63+
Assert.assertTrue("missing predicate: " + predicate, whereClause.contains(predicate));
64+
}
6365
@SuppressWarnings("unchecked")
6466
Map<String, Object[]> params = (Map<String, Object[]>) ReflectionTestUtils.getField(sc, "_params");
6567
Assert.assertArrayEquals(new Object[]{1L}, params.get("resourceId"));

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

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,9 @@ protected void scheduleSnapshots() {
304304
}
305305

306306
tmpSnapshotScheduleVO = _snapshotScheduleDao.acquireInLockTable(snapshotScheId);
307-
eventId = dispatchSnapshotCreateJob(snapshotToBeExecuted, volume, tmpSnapshotScheduleVO);
307+
// assigned before dispatching, so a failed dispatch can complete this event instead of leaving it dangling
308+
eventId = createScheduledSnapshotEvent(volume);
309+
dispatchSnapshotCreateJob(snapshotToBeExecuted, volume, tmpSnapshotScheduleVO, eventId);
308310
} catch (final Exception e) {
309311
logger.error("The scheduling of snapshot [{}] for volume [{}] failed due to [{}].", snapshotToBeExecuted, volume, e.toString(), e);
310312
if (tmpSnapshotScheduleVO != null) {
@@ -336,18 +338,25 @@ protected boolean shouldSkipSchedule(final SnapshotScheduleVO snapshotToBeExecut
336338
}
337339

338340
/**
339-
* Builds and dispatches the CreateSnapshotCmd async job for a scheduled snapshot, returning the "scheduled"
340-
* action event id so the caller can complete it if dispatch subsequently fails.
341+
* Logs the "scheduled" action event for a recurring snapshot of the volume and returns its id, which
342+
* {@link #dispatchSnapshotCreateJob} passes on to the job and {@link #handleFailedSnapshotDispatch} completes if
343+
* dispatching fails.
341344
*/
342-
protected Long dispatchSnapshotCreateJob(final SnapshotScheduleVO snapshotToBeExecuted, final VolumeVO volume, final SnapshotScheduleVO tmpSnapshotScheduleVO) throws Exception {
345+
protected Long createScheduledSnapshotEvent(final VolumeVO volume) {
346+
return ActionEventUtils.onScheduledActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventTypes.EVENT_SNAPSHOT_CREATE, "creating snapshot for volume Id:" +
347+
volume.getUuid(), volume.getId(), ApiCommandResourceType.Volume.toString(), true, 0);
348+
}
349+
350+
/**
351+
* Builds and dispatches the CreateSnapshotCmd async job for a scheduled snapshot, started from the given
352+
* "scheduled" action event.
353+
*/
354+
protected void dispatchSnapshotCreateJob(final SnapshotScheduleVO snapshotToBeExecuted, final VolumeVO volume, final SnapshotScheduleVO tmpSnapshotScheduleVO,
355+
final Long eventId) throws Exception {
343356
final long snapshotScheId = snapshotToBeExecuted.getId();
344357
final long policyId = snapshotToBeExecuted.getPolicyId();
345358
final long volumeId = snapshotToBeExecuted.getVolumeId();
346359

347-
final Long eventId =
348-
ActionEventUtils.onScheduledActionEvent(User.UID_SYSTEM, volume.getAccountId(), EventTypes.EVENT_SNAPSHOT_CREATE, "creating snapshot for volume Id:" +
349-
volume.getUuid(), volumeId, ApiCommandResourceType.Volume.toString(), true, 0);
350-
351360
logger.trace("Mapping parameters required to generate a CreateSnapshotCmd for snapshot [{}].", snapshotToBeExecuted);
352361
final Map<String, String> params = new HashMap<String, String>();
353362
params.put(ApiConstants.VOLUME_ID, "" + volumeId);
@@ -384,8 +393,6 @@ protected Long dispatchSnapshotCreateJob(final SnapshotScheduleVO snapshotToBeEx
384393

385394
tmpSnapshotScheduleVO.setAsyncJobId(jobId);
386395
_snapshotScheduleDao.update(snapshotScheId, tmpSnapshotScheduleVO);
387-
388-
return eventId;
389396
}
390397

391398
/**

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

Lines changed: 57 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@
3737
import com.cloud.vm.VMInstanceVO;
3838
import com.cloud.vm.VirtualMachine;
3939
import com.cloud.vm.dao.VMInstanceDao;
40+
import org.apache.cloudstack.framework.config.ConfigKey;
41+
import org.apache.cloudstack.framework.config.impl.ConfigDepotImpl;
4042
import org.apache.cloudstack.framework.jobs.AsyncJobDispatcher;
4143
import org.apache.cloudstack.framework.jobs.AsyncJobManager;
4244
import org.apache.cloudstack.framework.jobs.dao.AsyncJobDao;
@@ -374,6 +376,38 @@ public void getScopedConfigValueTestFallsBackToGlobalDefaultWhenNoScopeOverrideC
374376
Assert.assertEquals(Integer.valueOf(3), result);
375377
}
376378

379+
private Integer getScopedMaxFailuresWith(String accountValue, String domainValue, String zoneValue) {
380+
doReturn(1L).when(volumeVoMock).getAccountId();
381+
doReturn(3L).when(volumeVoMock).getDataCenterId();
382+
doReturn(2L).when(accountVoMock).getDomainId();
383+
String name = SnapshotManager.SnapshotRecurringMaxFailures.key();
384+
ConfigDepotImpl configDepotMock = mock(ConfigDepotImpl.class);
385+
doReturn(accountValue).when(configDepotMock).getConfigStringValue(name, ConfigKey.Scope.Account, 1L);
386+
doReturn(domainValue).when(configDepotMock).getConfigStringValue(name, ConfigKey.Scope.Domain, 2L);
387+
doReturn(zoneValue).when(configDepotMock).getConfigStringValue(name, ConfigKey.Scope.Zone, 3L);
388+
ConfigKey.init(configDepotMock);
389+
try {
390+
return snapshotSchedulerImplSpy.getScopedConfigValue(SnapshotManager.SnapshotRecurringMaxFailures, volumeVoMock, accountVoMock);
391+
} finally {
392+
ConfigKey.init(null);
393+
}
394+
}
395+
396+
@Test
397+
public void getScopedConfigValueTestZoneOverrideIsUsedWhenAccountAndDomainAreNotSet() {
398+
Assert.assertEquals(Integer.valueOf(7), getScopedMaxFailuresWith(null, null, "7"));
399+
}
400+
401+
@Test
402+
public void getScopedConfigValueTestDomainOverrideWinsOverZone() {
403+
Assert.assertEquals(Integer.valueOf(5), getScopedMaxFailuresWith(null, "5", "7"));
404+
}
405+
406+
@Test
407+
public void getScopedConfigValueTestAccountOverrideWinsOverDomainAndZone() {
408+
Assert.assertEquals(Integer.valueOf(1), getScopedMaxFailuresWith("1", "5", "7"));
409+
}
410+
377411
// --- handleFailedSnapshotDispatch (#13454) ---
378412

379413
private void stubVolumeAndAccount() {
@@ -470,29 +504,36 @@ public void recordSnapshotAttemptOutcomeTestSucceededLogsInfoEventAndReturns() {
470504
// --- dispatchSnapshotCreateJob (#13454) ---
471505

472506
@Test
473-
public void dispatchSnapshotCreateJobTestBuildsAsyncJobAndReturnsScheduledEventId() throws Exception {
507+
public void createScheduledSnapshotEventTestReturnsScheduledEventIdForVolume() {
508+
doReturn(2L).when(volumeVoMock).getId();
509+
doReturn(1L).when(volumeVoMock).getAccountId();
510+
doReturn("volume-uuid").when(volumeVoMock).getUuid();
511+
512+
try (MockedStatic<ActionEventUtils> actionEventUtilsMocked = Mockito.mockStatic(ActionEventUtils.class)) {
513+
actionEventUtilsMocked.when(() -> ActionEventUtils.onScheduledActionEvent(
514+
Mockito.anyLong(), Mockito.anyLong(), Mockito.eq(EventTypes.EVENT_SNAPSHOT_CREATE), Mockito.anyString(), Mockito.eq(2L), Mockito.eq("Volume"), Mockito.anyBoolean(), Mockito.anyLong())
515+
).thenReturn(42L);
516+
517+
Assert.assertEquals(Long.valueOf(42L), snapshotSchedulerImplSpy.createScheduledSnapshotEvent(volumeVoMock));
518+
}
519+
}
520+
521+
@Test
522+
public void dispatchSnapshotCreateJobTestBuildsAsyncJob() throws Exception {
474523
doReturn(1L).when(snapshotScheduleVoMock).getId();
475524
doReturn(1L).when(snapshotScheduleVoMock).getPolicyId();
476525
doReturn(2L).when(snapshotScheduleVoMock).getVolumeId();
477526
doReturn(new Date()).when(snapshotScheduleVoMock).getScheduledTimestamp();
478527

479528
doReturn(1L).when(volumeVoMock).getAccountId();
480-
doReturn("volume-uuid").when(volumeVoMock).getUuid();
481529

482530
doReturn(Collections.emptyList()).when(taggedResourceServiceMock).listByResourceTypeAndId(Mockito.any(), Mockito.anyLong());
483531
doReturn(7L).when(asyncJobManagerMock).submitAsyncJob(Mockito.any(AsyncJobVO.class));
484532
doReturn("SnapshotDispatcher").when(asyncJobDispatcherMock).getName();
485533
snapshotSchedulerImplSpy.setAsyncJobDispatcher(asyncJobDispatcherMock);
486534

487-
try (MockedStatic<ActionEventUtils> actionEventUtilsMocked = Mockito.mockStatic(ActionEventUtils.class);
488-
MockedStatic<com.cloud.utils.component.ComponentContext> componentContextMocked = Mockito.mockStatic(com.cloud.utils.component.ComponentContext.class)) {
489-
actionEventUtilsMocked.when(() -> ActionEventUtils.onScheduledActionEvent(
490-
Mockito.anyLong(), Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyLong(), Mockito.anyString(), Mockito.anyBoolean(), Mockito.anyLong())
491-
).thenReturn(42L);
492-
493-
Long eventId = snapshotSchedulerImplSpy.dispatchSnapshotCreateJob(snapshotScheduleVoMock, volumeVoMock, snapshotScheduleVoMock);
494-
495-
Assert.assertEquals(Long.valueOf(42L), eventId);
535+
try (MockedStatic<com.cloud.utils.component.ComponentContext> componentContextMocked = Mockito.mockStatic(com.cloud.utils.component.ComponentContext.class)) {
536+
snapshotSchedulerImplSpy.dispatchSnapshotCreateJob(snapshotScheduleVoMock, volumeVoMock, snapshotScheduleVoMock, 42L);
496537
}
497538

498539
verify(snapshotScheduleVoMock).setAsyncJobId(7L);
@@ -544,23 +585,24 @@ public void scheduleSnapshotsTestSkippedScheduleIsNeverLockedOrDispatched() thro
544585
snapshotSchedulerImplSpy.scheduleSnapshots();
545586

546587
verify(snapshotScheduleDaoMock, never()).acquireInLockTable(Mockito.anyLong());
547-
verify(snapshotSchedulerImplSpy, never()).dispatchSnapshotCreateJob(Mockito.any(), Mockito.any(), Mockito.any());
588+
verify(snapshotSchedulerImplSpy, never()).dispatchSnapshotCreateJob(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any());
548589
}
549590

550591
@Test
551-
public void scheduleSnapshotsTestDispatchFailureIsHandledAndLockIsReleased() throws Exception {
592+
public void scheduleSnapshotsTestDispatchFailureCompletesScheduledEventAndLockIsReleased() throws Exception {
552593
ReflectionTestUtils.setField(snapshotSchedulerImplSpy, "_currentTimestamp", new Date());
553594
doReturn(1L).when(snapshotScheduleVoMock).getId();
554595
doReturn(List.of(snapshotScheduleVoMock)).when(snapshotScheduleDaoMock).getSchedulesToExecute(Mockito.any(Date.class));
555596
doReturn(volumeVoMock).when(volumeDaoMock).findByIdIncludingRemoved(Mockito.anyLong());
556597
doReturn(false).when(snapshotSchedulerImplSpy).shouldSkipSchedule(Mockito.any(), Mockito.any());
557598
doReturn(snapshotScheduleVoMock).when(snapshotScheduleDaoMock).acquireInLockTable(1L);
558-
doThrow(new RuntimeException("dispatch boom")).when(snapshotSchedulerImplSpy).dispatchSnapshotCreateJob(Mockito.any(), Mockito.any(), Mockito.any());
599+
doReturn(42L).when(snapshotSchedulerImplSpy).createScheduledSnapshotEvent(volumeVoMock);
600+
doThrow(new RuntimeException("dispatch boom")).when(snapshotSchedulerImplSpy).dispatchSnapshotCreateJob(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any());
559601
doNothing().when(snapshotSchedulerImplSpy).handleFailedSnapshotDispatch(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any());
560602

561603
snapshotSchedulerImplSpy.scheduleSnapshots();
562604

563-
verify(snapshotSchedulerImplSpy).handleFailedSnapshotDispatch(Mockito.eq(snapshotScheduleVoMock), Mockito.eq(volumeVoMock), Mockito.eq(snapshotScheduleVoMock), Mockito.isNull(), Mockito.any(Exception.class));
605+
verify(snapshotSchedulerImplSpy).handleFailedSnapshotDispatch(Mockito.eq(snapshotScheduleVoMock), Mockito.eq(volumeVoMock), Mockito.eq(snapshotScheduleVoMock), Mockito.eq(42L), Mockito.any(Exception.class));
564606
verify(snapshotScheduleDaoMock).releaseFromLockTable(1L);
565607
}
566608

0 commit comments

Comments
 (0)