From d25521e96fa9ed2f961ae550f925f23ab6aec76a Mon Sep 17 00:00:00 2001 From: Vishesh Date: Mon, 18 Sep 2023 17:41:06 +0530 Subject: [PATCH] Fix issues in VM Scheduler (#7782) --- .../vm/schedule/VMScheduleManagerImpl.java | 2 +- .../cloudstack/vm/schedule/VMScheduler.java | 2 +- .../vm/schedule/VMSchedulerImpl.java | 31 ++++---- .../vm/schedule/VMSchedulerImplTest.java | 73 ++++++++----------- test/integration/smoke/test_vm_schedule.py | 2 +- 5 files changed, 47 insertions(+), 63 deletions(-) diff --git a/server/src/main/java/org/apache/cloudstack/vm/schedule/VMScheduleManagerImpl.java b/server/src/main/java/org/apache/cloudstack/vm/schedule/VMScheduleManagerImpl.java index 4b183be6494..0bb2f94ba7f 100644 --- a/server/src/main/java/org/apache/cloudstack/vm/schedule/VMScheduleManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/vm/schedule/VMScheduleManagerImpl.java @@ -127,7 +127,7 @@ public class VMScheduleManagerImpl extends MutualExclusiveIdsManagerBase impleme return Transaction.execute((TransactionCallback) status -> { VMScheduleVO vmSchedule = vmScheduleDao.persist(new VMScheduleVO(cmd.getVmId(), finalDescription, cronExpression.toString(), timeZoneId, finalAction, finalStartDate, finalEndDate, cmd.getEnabled())); - vmScheduler.scheduleNextJob(vmSchedule); + vmScheduler.scheduleNextJob(vmSchedule, new Date()); CallContext.current().setEventResourceId(vm.getId()); CallContext.current().setEventResourceType(ApiCommandResourceType.VirtualMachine); return createResponse(vmSchedule); diff --git a/server/src/main/java/org/apache/cloudstack/vm/schedule/VMScheduler.java b/server/src/main/java/org/apache/cloudstack/vm/schedule/VMScheduler.java index 94867dd9399..fa96a1c97b9 100644 --- a/server/src/main/java/org/apache/cloudstack/vm/schedule/VMScheduler.java +++ b/server/src/main/java/org/apache/cloudstack/vm/schedule/VMScheduler.java @@ -32,5 +32,5 @@ public interface VMScheduler extends Manager, Scheduler { void updateScheduledJob(VMScheduleVO vmSchedule); - Date scheduleNextJob(VMScheduleVO vmSchedule); + Date scheduleNextJob(VMScheduleVO vmSchedule, Date timestamp); } diff --git a/server/src/main/java/org/apache/cloudstack/vm/schedule/VMSchedulerImpl.java b/server/src/main/java/org/apache/cloudstack/vm/schedule/VMSchedulerImpl.java index aab970e3442..34548fc9258 100644 --- a/server/src/main/java/org/apache/cloudstack/vm/schedule/VMSchedulerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/vm/schedule/VMSchedulerImpl.java @@ -96,20 +96,11 @@ public class VMSchedulerImpl extends ManagerBase implements VMScheduler { @Override public void updateScheduledJob(VMScheduleVO vmSchedule) { removeScheduledJobs(Longs.asList(vmSchedule.getId())); - scheduleNextJob(vmSchedule); - } - - public Date scheduleNextJob(Long vmScheduleId) { - VMScheduleVO vmSchedule = vmScheduleDao.findById(vmScheduleId); - if (vmSchedule != null) { - return scheduleNextJob(vmSchedule); - } - LOGGER.debug(String.format("VM Schedule [id=%s] is removed. Not scheduling next job.", vmScheduleId)); - return null; + scheduleNextJob(vmSchedule, new Date()); } @Override - public Date scheduleNextJob(VMScheduleVO vmSchedule) { + public Date scheduleNextJob(VMScheduleVO vmSchedule, Date timestamp) { if (!vmSchedule.getEnabled()) { LOGGER.debug(String.format("VM Schedule [id=%s] for VM [id=%s] is disabled. Not scheduling next job.", vmSchedule.getUuid(), vmSchedule.getVmId())); return null; @@ -127,7 +118,12 @@ public class VMSchedulerImpl extends ManagerBase implements VMScheduler { return null; } - ZonedDateTime now = ZonedDateTime.now(vmSchedule.getTimeZoneId()); + ZonedDateTime now; + if (timestamp != null) { + now = ZonedDateTime.ofInstant(timestamp.toInstant(), vmSchedule.getTimeZoneId()); + } else { + now = ZonedDateTime.now(vmSchedule.getTimeZoneId()); + } ZonedDateTime zonedStartDate = ZonedDateTime.ofInstant(startDate.toInstant(), vmSchedule.getTimeZoneId()); ZonedDateTime zonedEndDate = null; if (endDate != null) { @@ -178,8 +174,7 @@ public class VMSchedulerImpl extends ManagerBase implements VMScheduler { // Adding 1 minute to currentTimestamp to ensure that // jobs which were to be run at current time, doesn't cause issues currentTimestamp = DateUtils.addMinutes(new Date(), 1); - - scheduleNextJobs(); + scheduleNextJobs(currentTimestamp); final TimerTask schedulerPollTask = new ManagedContextTimerTask() { @Override @@ -193,7 +188,7 @@ public class VMSchedulerImpl extends ManagerBase implements VMScheduler { }; vmSchedulerTimer = new Timer("VMSchedulerPollTask"); - vmSchedulerTimer.schedule(schedulerPollTask, 5000L, 60 * 1000L); + vmSchedulerTimer.scheduleAtFixedRate(schedulerPollTask, 5000L, 60 * 1000L); return true; } @@ -207,7 +202,7 @@ public class VMSchedulerImpl extends ManagerBase implements VMScheduler { try { if (scanLock.lock(30)) { try { - scheduleNextJobs(); + scheduleNextJobs(currentTimestamp); } finally { scanLock.unlock(); } @@ -236,10 +231,10 @@ public class VMSchedulerImpl extends ManagerBase implements VMScheduler { } } - private void scheduleNextJobs() { + private void scheduleNextJobs(Date timestamp) { for (final VMScheduleVO schedule : vmScheduleDao.listAllActiveSchedules()) { try { - scheduleNextJob(schedule); + scheduleNextJob(schedule, timestamp); } catch (Exception e) { LOGGER.warn("Error in scheduling next job for schedule " + schedule.getUuid(), e); } diff --git a/server/src/test/java/org/apache/cloudstack/vm/schedule/VMSchedulerImplTest.java b/server/src/test/java/org/apache/cloudstack/vm/schedule/VMSchedulerImplTest.java index bb622f51cbb..b391472250c 100644 --- a/server/src/test/java/org/apache/cloudstack/vm/schedule/VMSchedulerImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/vm/schedule/VMSchedulerImplTest.java @@ -89,22 +89,22 @@ public class VMSchedulerImplTest { MockitoAnnotations.initMocks(this); PowerMockito.mockStatic(ActionEventUtils.class); Mockito.when(ActionEventUtils.onScheduledActionEvent(Mockito.anyLong(), Mockito.anyLong(), Mockito.anyString(), - Mockito.anyString(), Mockito.anyLong(), Mockito.anyString(), Mockito.anyBoolean(), Mockito.anyLong())).thenReturn(1L); + Mockito.anyString(), Mockito.anyLong(), Mockito.anyString(), Mockito.anyBoolean(), + Mockito.anyLong())) + .thenReturn(1L); Mockito.when(ActionEventUtils.onCompletedActionEvent(Mockito.anyLong(), Mockito.anyLong(), Mockito.any(), Mockito.anyString(), Mockito.anyBoolean(), Mockito.anyString(), Mockito.anyLong(), Mockito.anyString(), Mockito.anyLong())).thenReturn(1L); } - private void prepareMocksForProcessJob(VirtualMachine vm, VMScheduledJob vmScheduledJob, VirtualMachine.State vmState, VMSchedule.Action action, Long executeJobReturnValue) { - Mockito.when(vm.getState()).thenReturn(vmState); - Mockito.when(vmScheduledJob.getAction()).thenReturn(action); - - if (executeJobReturnValue != null) { - Mockito.doReturn(executeJobReturnValue).when(vmScheduler).executeStartVMJob(Mockito.any(VirtualMachine.class), Mockito.anyLong()); - Mockito.doReturn(executeJobReturnValue).when(vmScheduler).executeStopVMJob(Mockito.any(VirtualMachine.class), Mockito.anyBoolean(), Mockito.anyLong()); - Mockito.doReturn(executeJobReturnValue).when(vmScheduler).executeRebootVMJob(Mockito.any(VirtualMachine.class), Mockito.anyBoolean(), Mockito.anyLong()); - } + @Test + public void testProcessJobRunning() { + executeProcessJobWithVMStateAndActionNonSkipped(VirtualMachine.State.Running, VMSchedule.Action.STOP); + executeProcessJobWithVMStateAndActionNonSkipped(VirtualMachine.State.Running, VMSchedule.Action.FORCE_STOP); + executeProcessJobWithVMStateAndActionNonSkipped(VirtualMachine.State.Running, VMSchedule.Action.REBOOT); + executeProcessJobWithVMStateAndActionNonSkipped(VirtualMachine.State.Running, VMSchedule.Action.FORCE_REBOOT); + executeProcessJobWithVMStateAndActionNonSkipped(VirtualMachine.State.Stopped, VMSchedule.Action.START); } private void executeProcessJobWithVMStateAndActionNonSkipped(VirtualMachine.State state, VMSchedule.Action action) { @@ -125,13 +125,20 @@ public class VMSchedulerImplTest { Assert.assertEquals(expectedValue, jobId); } - @Test - public void testProcessJobRunning() { - executeProcessJobWithVMStateAndActionNonSkipped(VirtualMachine.State.Running, VMSchedule.Action.STOP); - executeProcessJobWithVMStateAndActionNonSkipped(VirtualMachine.State.Running, VMSchedule.Action.FORCE_STOP); - executeProcessJobWithVMStateAndActionNonSkipped(VirtualMachine.State.Running, VMSchedule.Action.REBOOT); - executeProcessJobWithVMStateAndActionNonSkipped(VirtualMachine.State.Running, VMSchedule.Action.FORCE_REBOOT); - executeProcessJobWithVMStateAndActionNonSkipped(VirtualMachine.State.Stopped, VMSchedule.Action.START); + private void prepareMocksForProcessJob(VirtualMachine vm, VMScheduledJob vmScheduledJob, + VirtualMachine.State vmState, VMSchedule.Action action, + Long executeJobReturnValue) { + Mockito.when(vm.getState()).thenReturn(vmState); + Mockito.when(vmScheduledJob.getAction()).thenReturn(action); + + if (executeJobReturnValue != null) { + Mockito.doReturn(executeJobReturnValue).when(vmScheduler).executeStartVMJob( + Mockito.any(VirtualMachine.class), Mockito.anyLong()); + Mockito.doReturn(executeJobReturnValue).when(vmScheduler).executeStopVMJob( + Mockito.any(VirtualMachine.class), Mockito.anyBoolean(), Mockito.anyLong()); + Mockito.doReturn(executeJobReturnValue).when(vmScheduler).executeRebootVMJob( + Mockito.any(VirtualMachine.class), Mockito.anyBoolean(), Mockito.anyLong()); + } } @Test @@ -172,7 +179,7 @@ public class VMSchedulerImplTest { Mockito.when(vmSchedule.getStartDate()).thenReturn(startDate); Mockito.when(userVmManager.getUserVm(Mockito.anyLong())).thenReturn(vm); Mockito.when(vmScheduledJobDao.persist(Mockito.any())).thenThrow(EntityExistsException.class); - Date actualScheduledTime = vmScheduler.scheduleNextJob(vmSchedule); + Date actualScheduledTime = vmScheduler.scheduleNextJob(vmSchedule, new Date()); Assert.assertEquals(expectedScheduledTime, actualScheduledTime); } @@ -190,7 +197,7 @@ public class VMSchedulerImplTest { Mockito.when(vmSchedule.getTimeZoneId()).thenReturn(TimeZone.getTimeZone("UTC").toZoneId()); Mockito.when(vmSchedule.getStartDate()).thenReturn(startDate); Mockito.when(userVmManager.getUserVm(Mockito.anyLong())).thenReturn(vm); - Date actualScheduledTime = vmScheduler.scheduleNextJob(vmSchedule); + Date actualScheduledTime = vmScheduler.scheduleNextJob(vmSchedule, new Date()); Assert.assertEquals(expectedScheduledTime, actualScheduledTime); } @@ -226,7 +233,7 @@ public class VMSchedulerImplTest { expectedScheduledTime = DateUtils.addDays(expectedScheduledTime, 1); } - Date actualScheduledTime = vmScheduler.scheduleNextJob(vmSchedule); + Date actualScheduledTime = vmScheduler.scheduleNextJob(vmSchedule, new Date()); Assert.assertEquals(expectedScheduledTime, actualScheduledTime); } @@ -242,7 +249,7 @@ public class VMSchedulerImplTest { Mockito.when(vmSchedule.getTimeZoneId()).thenReturn(TimeZone.getTimeZone("UTC").toZoneId()); Mockito.when(vmSchedule.getStartDate()).thenReturn(DateUtils.addDays(now, -1)); Mockito.when(userVmManager.getUserVm(Mockito.anyLong())).thenReturn(vm); - Date actualScheduledTime = vmScheduler.scheduleNextJob(vmSchedule); + Date actualScheduledTime = vmScheduler.scheduleNextJob(vmSchedule, new Date()); Assert.assertEquals(expectedScheduledTime, actualScheduledTime); } @@ -277,7 +284,7 @@ public class VMSchedulerImplTest { expectedScheduledTime = DateUtils.addDays(expectedScheduledTime, 1); } - Date actualScheduledTime = vmScheduler.scheduleNextJob(vmSchedule); + Date actualScheduledTime = vmScheduler.scheduleNextJob(vmSchedule, new Date()); Assert.assertEquals(expectedScheduledTime, actualScheduledTime); } @@ -286,7 +293,7 @@ public class VMSchedulerImplTest { VMScheduleVO vmSchedule = Mockito.mock(VMScheduleVO.class); Mockito.when(vmSchedule.getEndDate()).thenReturn(DateUtils.addMinutes(new Date(), -5)); Mockito.when(vmSchedule.getEnabled()).thenReturn(true); - Date actualDate = vmScheduler.scheduleNextJob(vmSchedule); + Date actualDate = vmScheduler.scheduleNextJob(vmSchedule, new Date()); Assert.assertNull(actualDate); } @@ -294,28 +301,10 @@ public class VMSchedulerImplTest { public void testScheduleNextJobScheduleDisabled() { VMScheduleVO vmSchedule = Mockito.mock(VMScheduleVO.class); Mockito.when(vmSchedule.getEnabled()).thenReturn(false); - Date actualDate = vmScheduler.scheduleNextJob(vmSchedule); + Date actualDate = vmScheduler.scheduleNextJob(vmSchedule, new Date()); Assert.assertNull(actualDate); } - @Test - public void testScheduleNextJobScheduleIdNotExists() { - long vmScheduleId = 1; - Mockito.when(vmScheduleDao.findById(vmScheduleId)).thenReturn(null); - Date actualDate = vmScheduler.scheduleNextJob(vmScheduleId); - Assert.assertNull(actualDate); - } - - @Test - public void testScheduleNextJobScheduleIdExists() { - long vmScheduleId = 1; - VMScheduleVO vmScheduleVO = Mockito.mock(VMScheduleVO.class); - Date date = Mockito.mock(Date.class); - Mockito.when(vmScheduleDao.findById(vmScheduleId)).thenReturn(vmScheduleVO); - Mockito.doReturn(date).when(vmScheduler).scheduleNextJob(vmScheduleVO); - Date actualDate = vmScheduler.scheduleNextJob(vmScheduleId); - Assert.assertEquals(date, actualDate); - } @Test public void testExecuteJobs() { diff --git a/test/integration/smoke/test_vm_schedule.py b/test/integration/smoke/test_vm_schedule.py index e66470dff73..bd699cf4d2b 100644 --- a/test/integration/smoke/test_vm_schedule.py +++ b/test/integration/smoke/test_vm_schedule.py @@ -571,7 +571,7 @@ class TestVMSchedule(cloudstackTestCase): stop_vmschedule.delete(self.apiclient) # To ensure that all vm schedules have been deleted and all of their jobs have been completed - time.sleep(15) + time.sleep(60) # Verify VM Schedule is deleted self.assertEqual(