Copilot commented on code in PR #13743:
URL: https://github.com/apache/cloudstack/pull/13743#discussion_r4444853897
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java:
##########
@@ -237,58 +295,21 @@ protected void scheduleSnapshots() {
for (final SnapshotScheduleVO snapshotToBeExecuted :
snapshotsToBeExecuted) {
SnapshotScheduleVO tmpSnapshotScheduleVO = null;
+ Long eventId = null;
final long snapshotScheId = snapshotToBeExecuted.getId();
- final long policyId = snapshotToBeExecuted.getPolicyId();
- final long volumeId = snapshotToBeExecuted.getVolumeId();
final VolumeVO volume =
_volsDao.findByIdIncludingRemoved(snapshotToBeExecuted.getVolumeId());
try {
- if (!canSnapshotBeScheduled(snapshotToBeExecuted, volume)) {
+ if (shouldSkipSchedule(snapshotToBeExecuted, volume)) {
continue;
}
tmpSnapshotScheduleVO =
_snapshotScheduleDao.acquireInLockTable(snapshotScheId);
- final Long eventId =
- ActionEventUtils.onScheduledActionEvent(User.UID_SYSTEM,
volume.getAccountId(), EventTypes.EVENT_SNAPSHOT_CREATE, "creating snapshot for
volume Id:" +
- volume.getUuid(), volumeId,
ApiCommandResourceType.Volume.toString(), true, 0);
-
- 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();
- 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);
+ eventId = dispatchSnapshotCreateJob(snapshotToBeExecuted,
volume, tmpSnapshotScheduleVO);
} catch (final Exception e) {
logger.error("The scheduling of snapshot [{}] for volume [{}]
failed due to [{}].", snapshotToBeExecuted, volume, e.toString(), e);
+ if (tmpSnapshotScheduleVO != null) {
+ handleFailedSnapshotDispatch(snapshotToBeExecuted, volume,
tmpSnapshotScheduleVO, eventId, e);
+ }
} finally {
Review Comment:
If `dispatchSnapshotCreateJob` throws after it has already created the
“scheduled” action event (but before returning), `eventId` will still be `null`
here and `handleFailedSnapshotDispatch` won’t complete the scheduled event,
leaving it dangling. A concrete fix is to ensure the scheduled event id is
assigned in the caller before subsequent dispatch steps can throw (e.g., split
“create scheduled event” from the rest, or have `dispatchSnapshotCreateJob`
catch/rewrap exceptions with the `eventId` so the catch block can always call
`onCompletedActionEvent` when applicable).
##########
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);
Review Comment:
The ordering field `"createDate"` looks inconsistent with the comment
(“created”) and the expected SQL in `EventDaoImplTest` (which asserts `ORDER BY
event.created ...`). If the mapped field is actually `created`, this will
silently sort incorrectly (or not at all) depending on ORM mapping. Prefer
ordering by the same property used elsewhere (likely `"created"`) and keep the
secondary `id` ordering for tie-breaking.
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java:
##########
@@ -297,6 +318,225 @@ 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;
+ }
+
+ /**
+ * Builds and dispatches the CreateSnapshotCmd async job for a scheduled
snapshot, returning the "scheduled"
+ * action event id so the caller can complete it if dispatch subsequently
fails.
+ */
+ protected Long dispatchSnapshotCreateJob(final SnapshotScheduleVO
snapshotToBeExecuted, final VolumeVO volume, final SnapshotScheduleVO
tmpSnapshotScheduleVO) throws Exception {
+ final long snapshotScheId = snapshotToBeExecuted.getId();
+ final long policyId = snapshotToBeExecuted.getPolicyId();
+ final long volumeId = snapshotToBeExecuted.getVolumeId();
+
+ final Long eventId =
+ ActionEventUtils.onScheduledActionEvent(User.UID_SYSTEM,
volume.getAccountId(), EventTypes.EVENT_SNAPSHOT_CREATE, "creating snapshot for
volume Id:" +
+ volume.getUuid(), volumeId,
ApiCommandResourceType.Volume.toString(), true, 0);
+
+ 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);
+
+ return eventId;
+ }
+
+ /**
+ * 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;
+ }
Review Comment:
Using `valueInScope(..., inherit=true)` for Account/Domain can return a
Global value before the Zone scope is checked, which makes Zone overrides
unreachable when Account/Domain aren’t set. To preserve the intended precedence
(Account → Domain → Zone → Global), query each scope with `inherit=false` and
only fall back to `key.value()` at the end (or otherwise ensure Zone is
consulted before any implicit Global fallback is applied).
##########
api/src/main/java/com/cloud/event/EventTypes.java:
##########
@@ -1094,6 +1096,8 @@ public class EventTypes {
// Snapshots
entityEventDetails.put(EVENT_SNAPSHOT_CREATE, Snapshot.class);
+ entityEventDetails.put(EVENT_SNAPSHOT_SKIPPED, Snapshot.class);
+ entityEventDetails.put(EVENT_SNAPSHOT_RECURRING_FAILURE_LIMIT_REACHED,
Snapshot.class);
Review Comment:
The new events (`SNAPSHOT.SKIPPED`,
`SNAPSHOT.RECURRING.FAILURE.LIMIT.REACHED`) are logged against the Volume
resource in `SnapshotSchedulerImpl` (resource id = volume id, resource type =
Volume). Mapping them to `Snapshot.class` risks incorrect entity lookups
(volumeId ≠ snapshotId) in any code that relies on `entityEventDetails`.
Consider mapping these new event types to `Volume.class` (or aligning the
logged resource type/id with Snapshot if that’s required by the event system).
##########
engine/schema/src/test/java/com/cloud/event/dao/EventDaoImplTest.java:
##########
@@ -0,0 +1,85 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package com.cloud.event.dao;
+
+import com.cloud.event.Event;
+import com.cloud.event.EventVO;
+import com.cloud.utils.db.Filter;
+import com.cloud.utils.db.SearchCriteria;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.InjectMocks;
+import org.mockito.Spy;
+import org.mockito.junit.MockitoJUnitRunner;
+import org.springframework.test.util.ReflectionTestUtils;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.verify;
+
+@RunWith(MockitoJUnitRunner.class)
+public class EventDaoImplTest {
+
+ @Spy
+ @InjectMocks
+ EventDaoImpl eventDaoImpl = new EventDaoImpl();
+
+ @Test
+ public void
listLatestOutcomeEventsByResourceQueriesOutcomesByResourceAndTypeExcludingArchived()
{
+ List<EventVO> expected = Collections.singletonList(new EventVO());
+
doReturn(expected).when(eventDaoImpl).listBy(any(SearchCriteria.class),
any(Filter.class));
+
+ List<EventVO> result =
eventDaoImpl.listLatestOutcomeEventsByResource(1L, "Volume", "SNAPSHOT.CREATE",
3);
+
+ Assert.assertSame(expected, result);
+
+ ArgumentCaptor<SearchCriteria> scCaptor =
ArgumentCaptor.forClass(SearchCriteria.class);
+ ArgumentCaptor<Filter> filterCaptor =
ArgumentCaptor.forClass(Filter.class);
+ verify(eventDaoImpl).listBy(scCaptor.capture(),
filterCaptor.capture());
+
+ SearchCriteria<EventVO> sc = scCaptor.getValue();
+ Assert.assertEquals("event.resource_id = ? AND event.resource_type =
? AND event.type = ? AND event.state IN (?,?) AND event.archived = ? ",
+ sc.getWhereClause());
Review Comment:
This assertion is brittle because it depends on the exact formatting/order
of the generated where-clause string (which can change with minor
DAO/SearchBuilder changes). It would be more robust to assert on the captured
parameters (which you already do) and only minimally validate the presence of
key predicates rather than exact full-string equality.
##########
server/src/test/java/com/cloud/storage/snapshot/SnapshotSchedulerImplTest.java:
##########
@@ -202,76 +253,458 @@ public void
canSnapshotBeScheduledTestAccountIsRemovedOrDisabledReturnFalse() {
public void canSnapshotBeScheduledTestSnapshotPolicyIsRemovedCallRemove() {
SnapshotScheduleVO snapshotScheduleVO = new SnapshotScheduleVO();
- Mockito.doReturn(1l).when(volumeVoMock).getPoolId();
-
Mockito.doReturn(false).when(snapshotSchedulerImplSpy).isAccountRemovedOrDisabled(Mockito.any(),
Mockito.any());
-
Mockito.doReturn(null).when(snapshotPolicyDaoMock).findById(Mockito.any());
+ doReturn(1l).when(volumeVoMock).getPoolId();
+
doReturn(false).when(snapshotSchedulerImplSpy).isAccountRemovedOrDisabled(Mockito.any(),
Mockito.any());
+ doReturn(null).when(snapshotPolicyDaoMock).findById(Mockito.any());
boolean result =
snapshotSchedulerImplSpy.canSnapshotBeScheduled(snapshotScheduleVO,
volumeVoMock);
Assert.assertTrue(result);
- Mockito.verify(snapshotScheduleDaoMock).remove(Mockito.anyLong());
+ verify(snapshotScheduleDaoMock).remove(Mockito.anyLong());
}
@Test
public void
canSnapshotBeScheduledTestSnapshotPolicyIsNotRemovedDoNotCallRemove() {
SnapshotScheduleVO snapshotScheduleVO = new SnapshotScheduleVO();
SnapshotPolicyVO snapshotPolicyVO = new SnapshotPolicyVO();
- Mockito.doReturn(1l).when(volumeVoMock).getPoolId();
-
Mockito.doReturn(false).when(snapshotSchedulerImplSpy).isAccountRemovedOrDisabled(Mockito.any(),
Mockito.any());
-
Mockito.doReturn(snapshotPolicyVO).when(snapshotPolicyDaoMock).findById(Mockito.any());
+ doReturn(1l).when(volumeVoMock).getPoolId();
+
doReturn(false).when(snapshotSchedulerImplSpy).isAccountRemovedOrDisabled(Mockito.any(),
Mockito.any());
+
doReturn(snapshotPolicyVO).when(snapshotPolicyDaoMock).findById(Mockito.any());
boolean result =
snapshotSchedulerImplSpy.canSnapshotBeScheduled(snapshotScheduleVO,
volumeVoMock);
Assert.assertTrue(result);
- Mockito.verify(snapshotScheduleDaoMock,
Mockito.never()).remove(Mockito.anyLong());
+ verify(snapshotScheduleDaoMock, never()).remove(Mockito.anyLong());
}
@Test
public void
scheduleNextSnapshotJobIfNecessaryTestAsyncJobIsNullThenScheduleNextSnapshot() {
- Mockito.doReturn(1L).when(snapshotScheduleVoMock).getAsyncJobId();
-
Mockito.doReturn(null).when(asyncJobDaoMock).findByIdIncludingRemoved(Mockito.any());
- Mockito.doReturn(new
Date()).when(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
+ doReturn(1L).when(snapshotScheduleVoMock).getAsyncJobId();
+
doReturn(null).when(asyncJobDaoMock).findByIdIncludingRemoved(Mockito.any());
+ doReturn(new
Date()).when(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
snapshotSchedulerImplSpy.scheduleNextSnapshotJobIfNecessary(snapshotScheduleVoMock);
-
Mockito.verify(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
+
verify(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
}
@Test
public void
scheduleNextSnapshotJobIfNecessaryTestAsyncJobSucceededThenScheduleNextSnapshot()
{
- Mockito.doReturn(1L).when(snapshotScheduleVoMock).getAsyncJobId();
-
Mockito.doReturn(asyncJobVoMock).when(asyncJobDaoMock).findByIdIncludingRemoved(Mockito.any());
-
Mockito.doReturn(JobInfo.Status.SUCCEEDED).when(asyncJobVoMock).getStatus();
- Mockito.doReturn(new
Date()).when(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
+ doReturn(1L).when(snapshotScheduleVoMock).getAsyncJobId();
+
doReturn(asyncJobVoMock).when(asyncJobDaoMock).findByIdIncludingRemoved(Mockito.any());
+ doReturn(JobInfo.Status.SUCCEEDED).when(asyncJobVoMock).getStatus();
+ doReturn(new
Date()).when(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
snapshotSchedulerImplSpy.scheduleNextSnapshotJobIfNecessary(snapshotScheduleVoMock);
-
Mockito.verify(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
+
verify(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
}
@Test
public void
scheduleNextSnapshotJobIfNecessaryTestAsyncJobFailedThenScheduleNextSnapshot() {
- Mockito.doReturn(1L).when(snapshotScheduleVoMock).getAsyncJobId();
-
Mockito.doReturn(asyncJobVoMock).when(asyncJobDaoMock).findByIdIncludingRemoved(Mockito.any());
-
Mockito.doReturn(JobInfo.Status.FAILED).when(asyncJobVoMock).getStatus();
- Mockito.doReturn(new
Date()).when(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
+ doReturn(1L).when(snapshotScheduleVoMock).getAsyncJobId();
+
doReturn(asyncJobVoMock).when(asyncJobDaoMock).findByIdIncludingRemoved(Mockito.any());
+ doReturn(JobInfo.Status.FAILED).when(asyncJobVoMock).getStatus();
+ doReturn(new
Date()).when(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
snapshotSchedulerImplSpy.scheduleNextSnapshotJobIfNecessary(snapshotScheduleVoMock);
-
Mockito.verify(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
+
verify(snapshotSchedulerImplSpy).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
}
@Test
public void
scheduleNextSnapshotJobIfNecessaryTestAsyncJobInProgressThenDoNothing() {
- Mockito.doReturn(1L).when(snapshotScheduleVoMock).getAsyncJobId();
-
Mockito.doReturn(asyncJobVoMock).when(asyncJobDaoMock).findByIdIncludingRemoved(Mockito.any());
-
Mockito.doReturn(JobInfo.Status.IN_PROGRESS).when(asyncJobVoMock).getStatus();
+ doReturn(1L).when(snapshotScheduleVoMock).getAsyncJobId();
+
doReturn(asyncJobVoMock).when(asyncJobDaoMock).findByIdIncludingRemoved(Mockito.any());
+ doReturn(JobInfo.Status.IN_PROGRESS).when(asyncJobVoMock).getStatus();
snapshotSchedulerImplSpy.scheduleNextSnapshotJobIfNecessary(snapshotScheduleVoMock);
- Mockito.verify(snapshotSchedulerImplSpy,
Mockito.never()).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
+ verify(snapshotSchedulerImplSpy,
never()).scheduleNextSnapshotJob(Mockito.any(SnapshotScheduleVO.class));
+ }
+
+ // --- countConsecutiveFailedAttempts (#13454) ---
+
+ @Test
+ public void
countConsecutiveFailedAttemptsTestLimitZeroReturnsZeroWithoutQuerying() {
+ int result =
snapshotSchedulerImplSpy.countConsecutiveFailedAttempts(1L, 0);
+
+ Assert.assertEquals(0, result);
+ verify(eventDaoMock,
never()).listLatestOutcomeEventsByResource(Mockito.anyLong(),
Mockito.anyString(), Mockito.anyString(), Mockito.anyInt());
+ }
+
+ @Test
+ public void countConsecutiveFailedAttemptsTestStopsAtFirstNonErrorEvent() {
+ EventVO error1 = mock(EventVO.class);
+ doReturn(EventVO.LEVEL_ERROR).when(error1).getLevel();
+ EventVO error2 = mock(EventVO.class);
+ doReturn(EventVO.LEVEL_ERROR).when(error2).getLevel();
+ EventVO success = mock(EventVO.class);
+ doReturn(EventVO.LEVEL_INFO).when(success).getLevel();
+
+ doReturn(List.of(error1, error2,
success)).when(eventDaoMock).listLatestOutcomeEventsByResource(Mockito.anyLong(),
Mockito.anyString(), Mockito.anyString(), Mockito.anyInt());
+
+ int result =
snapshotSchedulerImplSpy.countConsecutiveFailedAttempts(1L, 3);
+
+ Assert.assertEquals(2, result);
+ }
+
+ @Test
+ public void countConsecutiveFailedAttemptsTestNoEventsReturnsZero() {
+
doReturn(Collections.emptyList()).when(eventDaoMock).listLatestOutcomeEventsByResource(Mockito.anyLong(),
Mockito.anyString(), Mockito.anyString(), Mockito.anyInt());
+
+ int result =
snapshotSchedulerImplSpy.countConsecutiveFailedAttempts(1L, 3);
+
+ Assert.assertEquals(0, result);
+ }
+
+ // --- getScopedConfigValue (#13454) ---
+
+ @Test
+ public void
getScopedConfigValueTestFallsBackToGlobalDefaultWhenNoScopeOverrideConfigured()
{
+ doReturn(1L).when(volumeVoMock).getAccountId();
+ doReturn(1L).when(volumeVoMock).getDataCenterId();
+ doReturn(1L).when(accountVoMock).getDomainId();
+
+ Integer result =
snapshotSchedulerImplSpy.getScopedConfigValue(SnapshotManager.SnapshotRecurringMaxFailures,
volumeVoMock, accountVoMock);
+
+ Assert.assertEquals(Integer.valueOf(3), result);
+ }
+
Review Comment:
The tests only cover the global fallback case, but the new logic is
specifically about precedence across Account/Domain/Zone/Global. Add unit tests
that set different values per scope (especially Zone-only overrides) and assert
the chosen value matches the documented precedence; this would also catch
regressions like implicit fallback short-circuiting later scopes.
--
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]
