From cf1deb8a5d4d71b860a2bf5b0a6f285530e8f4f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bernardo=20De=20Marco=20Gon=C3=A7alves?= Date: Wed, 16 Jul 2025 10:23:37 -0300 Subject: [PATCH 1/2] fix deletion of backup schedules --- .../user/backup/DeleteBackupScheduleCmd.java | 16 +-- .../api/response/BackupScheduleResponse.java | 13 +- .../cloudstack/backup/BackupSchedule.java | 1 + .../cloudstack/backup/BackupScheduleVO.java | 9 ++ .../backup/dao/BackupScheduleDaoImpl.java | 3 +- .../META-INF/db/schema-42010to42100.sql | 3 + .../cloudstack/backup/BackupManagerImpl.java | 47 ++++++-- .../cloudstack/backup/BackupManagerTest.java | 111 +++++++++++++++++- .../views/compute/backup/BackupSchedule.vue | 2 +- 9 files changed, 176 insertions(+), 29 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/backup/DeleteBackupScheduleCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/backup/DeleteBackupScheduleCmd.java index 548f4d67b232..e3e36ace4013 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/backup/DeleteBackupScheduleCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/backup/DeleteBackupScheduleCmd.java @@ -43,7 +43,7 @@ description = "Deletes the backup schedule of a VM", responseObject = SuccessResponse.class, since = "4.14.0", authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}) -public class DeleteBackupScheduleCmd extends BaseCmd { +public class DeleteBackupScheduleCmd extends BaseCmd { @Inject private BackupManager backupManager; @@ -52,17 +52,13 @@ public class DeleteBackupScheduleCmd extends BaseCmd { //////////////// API parameters ///////////////////// ///////////////////////////////////////////////////// - @Parameter(name = ApiConstants.VIRTUAL_MACHINE_ID, - type = CommandType.UUID, - entityType = UserVmResponse.class, - description = "ID of the VM") + @Parameter(name = ApiConstants.VIRTUAL_MACHINE_ID, type = CommandType.UUID, entityType = UserVmResponse.class, + description = "ID of the VM from which all backup schedules will be deleted. It has precedence over the 'id' parameter, " + + "i.e., when the 'virtualmachineid' parameter is specified, the 'id' parameter will be ignored.") private Long vmId; - @Parameter(name = ApiConstants.ID, - type = CommandType.UUID, - entityType = BackupScheduleResponse.class, - description = "ID of the schedule", - since = "4.20.1") + @Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = BackupScheduleResponse.class, + description = "ID of the backup schedule to be deleted.", since = "4.20.1") private Long id; ///////////////////////////////////////////////////// diff --git a/api/src/main/java/org/apache/cloudstack/api/response/BackupScheduleResponse.java b/api/src/main/java/org/apache/cloudstack/api/response/BackupScheduleResponse.java index d7c6f96add5b..92f1bad54470 100644 --- a/api/src/main/java/org/apache/cloudstack/api/response/BackupScheduleResponse.java +++ b/api/src/main/java/org/apache/cloudstack/api/response/BackupScheduleResponse.java @@ -28,6 +28,9 @@ @EntityReference(value = BackupSchedule.class) public class BackupScheduleResponse extends BaseResponse { + @SerializedName(ApiConstants.ID) + @Param(description = "ID of the backup schedule.") + private String id; @SerializedName(ApiConstants.VIRTUAL_MACHINE_NAME) @Param(description = "name of the VM") @@ -51,7 +54,11 @@ public class BackupScheduleResponse extends BaseResponse { @SerializedName(ApiConstants.MAX_BACKUPS) @Param(description = "maximum number of backups retained") - private Integer maxBakups; + private Integer maxBackups; + + public void setId(String id) { + this.id = id; + } public String getVmName() { return vmName; @@ -93,7 +100,7 @@ public void setTimezone(String timezone) { this.timezone = timezone; } - public void setMaxBakups(Integer maxBakups) { - this.maxBakups = maxBakups; + public void setMaxBackups(Integer maxBackups) { + this.maxBackups = maxBackups; } } diff --git a/api/src/main/java/org/apache/cloudstack/backup/BackupSchedule.java b/api/src/main/java/org/apache/cloudstack/backup/BackupSchedule.java index 4ff946be9cd4..f439b3a91394 100644 --- a/api/src/main/java/org/apache/cloudstack/backup/BackupSchedule.java +++ b/api/src/main/java/org/apache/cloudstack/backup/BackupSchedule.java @@ -31,4 +31,5 @@ public interface BackupSchedule extends InternalIdentity { Date getScheduledTimestamp(); Long getAsyncJobId(); Integer getMaxBackups(); + String getUuid(); } diff --git a/engine/schema/src/main/java/org/apache/cloudstack/backup/BackupScheduleVO.java b/engine/schema/src/main/java/org/apache/cloudstack/backup/BackupScheduleVO.java index 0258c42c52ba..75c7a8be55c4 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/backup/BackupScheduleVO.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/backup/BackupScheduleVO.java @@ -18,6 +18,7 @@ package org.apache.cloudstack.backup; import java.util.Date; +import java.util.UUID; import javax.persistence.Column; import javax.persistence.Entity; @@ -39,6 +40,9 @@ public class BackupScheduleVO implements BackupSchedule { @Column(name = "id") private long id; + @Column(name = "uuid", nullable = false) + private String uuid = UUID.randomUUID().toString(); + @Column(name = "vm_id") private Long vmId; @@ -84,6 +88,11 @@ public long getId() { return id; } + @Override + public String getUuid() { + return uuid; + } + public Long getVmId() { return vmId; } diff --git a/engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupScheduleDaoImpl.java b/engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupScheduleDaoImpl.java index aac2e3bf2320..9374798dde32 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupScheduleDaoImpl.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupScheduleDaoImpl.java @@ -92,12 +92,13 @@ public List getSchedulesToExecute(Date currentTimestamp) { public BackupScheduleResponse newBackupScheduleResponse(BackupSchedule schedule) { VMInstanceVO vm = vmInstanceDao.findByIdIncludingRemoved(schedule.getVmId()); BackupScheduleResponse response = new BackupScheduleResponse(); + response.setId(schedule.getUuid()); response.setVmId(vm.getUuid()); response.setVmName(vm.getHostName()); response.setIntervalType(schedule.getScheduleType()); response.setSchedule(schedule.getSchedule()); response.setTimezone(schedule.getTimezone()); - response.setMaxBakups(schedule.getMaxBackups()); + response.setMaxBackups(schedule.getMaxBackups()); response.setObjectName("backupschedule"); return response; } diff --git a/engine/schema/src/main/resources/META-INF/db/schema-42010to42100.sql b/engine/schema/src/main/resources/META-INF/db/schema-42010to42100.sql index 3fd4914b2e27..9688d3c76374 100644 --- a/engine/schema/src/main/resources/META-INF/db/schema-42010to42100.sql +++ b/engine/schema/src/main/resources/META-INF/db/schema-42010to42100.sql @@ -233,3 +233,6 @@ CREATE TABLE IF NOT EXISTS `cloud`.`gui_themes_details` ( PRIMARY KEY (`id`), CONSTRAINT `fk_gui_themes_details__gui_theme_id` FOREIGN KEY (`gui_theme_id`) REFERENCES `gui_themes`(`id`) ); + +CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backup_schedule', 'uuid', 'VARCHAR(40) NOT NULL'); +UPDATE `cloud`.`backup_schedule` SET uuid = UUID(); diff --git a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java index 68b6a2ad0470..cd42ccbadf85 100644 --- a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java @@ -71,6 +71,7 @@ import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils; import org.apache.commons.lang3.BooleanUtils; +import org.apache.commons.lang3.ObjectUtils; import org.apache.commons.lang3.StringUtils; import com.amazonaws.util.CollectionUtils; @@ -515,24 +516,44 @@ public List listBackupSchedule(final Long vmId) { public boolean deleteBackupSchedule(DeleteBackupScheduleCmd cmd) { Long vmId = cmd.getVmId(); Long id = cmd.getId(); - if (Objects.isNull(vmId) && Objects.isNull(id)) { - throw new InvalidParameterValueException("Either instance ID or ID of backup schedule needs to be specified"); + if (ObjectUtils.allNull(vmId, id)) { + throw new InvalidParameterValueException("Either instance ID or ID of backup schedule needs to be specified."); } + if (Objects.nonNull(vmId)) { - final VMInstanceVO vm = findVmById(vmId); - validateForZone(vm.getDataCenterId()); - accountManager.checkAccess(CallContext.current().getCallingAccount(), null, true, vm); - return deleteAllVMBackupSchedules(vm.getId()); - } else { - final BackupSchedule schedule = backupScheduleDao.findById(id); - if (schedule == null) { - throw new CloudRuntimeException("Could not find the requested backup schedule."); - } - return backupScheduleDao.remove(schedule.getId()); + checkCallerAccessToBackupScheduleVm(vmId); + return deleteAllVmBackupSchedules(vmId); + } + + BackupSchedule schedule = backupScheduleDao.findById(id); + if (schedule == null) { + throw new InvalidParameterValueException("Could not find the requested backup schedule."); } + checkCallerAccessToBackupScheduleVm(schedule.getVmId()); + return backupScheduleDao.remove(schedule.getId()); + } + + /** + * Checks if the backup framework is enabled for the zone in which the VM with specified ID is allocated and + * if the caller has access to the VM. + * + * @param vmId The ID of the virtual machine to check access for + * @throws PermissionDeniedException if the caller doesn't have access to the VM + * @throws CloudRuntimeException if the backup framework is disabled + */ + protected void checkCallerAccessToBackupScheduleVm(long vmId) { + VMInstanceVO vm = findVmById(vmId); + validateForZone(vm.getDataCenterId()); + accountManager.checkAccess(CallContext.current().getCallingAccount(), null, true, vm); } - private boolean deleteAllVMBackupSchedules(long vmId) { + /** + * Deletes all backup schedules associated with a specific VM. + * + * @param vmId The ID of the virtual machine whose backup schedules should be deleted + * @return true if all backup schedules were successfully deleted, false if any deletion failed + */ + protected boolean deleteAllVmBackupSchedules(long vmId) { List vmBackupSchedules = backupScheduleDao.listByVM(vmId); boolean success = true; for (BackupScheduleVO vmBackupSchedule : vmBackupSchedules) { diff --git a/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java b/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java index 808511e6fdf5..57ce555b3077 100644 --- a/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java +++ b/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java @@ -47,6 +47,7 @@ import org.apache.cloudstack.api.ServerApiException; import org.apache.cloudstack.api.command.admin.backup.UpdateBackupOfferingCmd; import org.apache.cloudstack.api.command.user.backup.CreateBackupScheduleCmd; +import org.apache.cloudstack.api.command.user.backup.DeleteBackupScheduleCmd; import org.apache.cloudstack.backup.dao.BackupDao; import org.apache.cloudstack.backup.dao.BackupOfferingDao; import org.apache.cloudstack.backup.dao.BackupScheduleDao; @@ -75,6 +76,8 @@ import java.util.TimeZone; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; @@ -127,7 +130,21 @@ public class BackupManagerTest { @Mock AlertManager alertManager; - private AccountVO account; + @Mock + private VMInstanceVO vmInstanceVOMock; + + @Mock + private CallContext callContextMock; + + @Mock + private AccountVO accountVOMock; + + @Mock + private DeleteBackupScheduleCmd deleteBackupScheduleCmdMock; + + @Mock + private BackupScheduleVO backupScheduleVOMock; + private UserVO user; private String[] hostPossibleValues = {"127.0.0.1", "hostname"}; @@ -657,4 +674,96 @@ public void testBackupSyncTask() { } } } + + @Test + public void checkCallerAccessToBackupScheduleVmTestExecuteAccessCheckMethods() { + long vmId = 1L; + long dataCenterId = 2L; + + try (MockedStatic mockedCallContext = Mockito.mockStatic(CallContext.class)) { + Mockito.when(vmInstanceDao.findById(vmId)).thenReturn(vmInstanceVOMock); + Mockito.when(vmInstanceVOMock.getDataCenterId()).thenReturn(dataCenterId); + Mockito.when(backupManager.isDisabled(dataCenterId)).thenReturn(false); + + mockedCallContext.when(CallContext::current).thenReturn(callContextMock); + Mockito.when(callContextMock.getCallingAccount()).thenReturn(accountVOMock); + Mockito.doNothing().when(accountManager).checkAccess(accountVOMock, null, true, vmInstanceVOMock); + backupManager.checkCallerAccessToBackupScheduleVm(vmId); + + verify(accountManager, times(1)).checkAccess(accountVOMock, null, true, vmInstanceVOMock); + } + } + + @Test + public void deleteAllVmBackupSchedulesTestReturnSuccessWhenAllSchedulesAreDeleted() { + long vmId = 1L; + List backupSchedules = List.of(Mockito.mock(BackupScheduleVO.class), Mockito.mock(BackupScheduleVO.class)); + Mockito.when(backupScheduleDao.listByVM(vmId)).thenReturn(backupSchedules); + Mockito.when(backupSchedules.get(0).getId()).thenReturn(2L); + Mockito.when(backupSchedules.get(1).getId()).thenReturn(3L); + Mockito.when(backupScheduleDao.remove(Mockito.anyLong())).thenReturn(true); + + boolean success = backupManager.deleteAllVmBackupSchedules(vmId); + assertTrue(success); + Mockito.verify(backupScheduleDao, times(2)).remove(Mockito.anyLong()); + } + + @Test + public void deleteAllVmBackupSchedulesTestReturnFalseWhenAnyDeletionFails() { + long vmId = 1L; + List backupSchedules = List.of(Mockito.mock(BackupScheduleVO.class), Mockito.mock(BackupScheduleVO.class)); + Mockito.when(backupScheduleDao.listByVM(vmId)).thenReturn(backupSchedules); + Mockito.when(backupSchedules.get(0).getId()).thenReturn(2L); + Mockito.when(backupSchedules.get(1).getId()).thenReturn(3L); + Mockito.when(backupScheduleDao.remove(2L)).thenReturn(true); + Mockito.when(backupScheduleDao.remove(3L)).thenReturn(false); + + boolean success = backupManager.deleteAllVmBackupSchedules(vmId); + assertFalse(success); + Mockito.verify(backupScheduleDao, times(2)).remove(Mockito.anyLong()); + } + + @Test(expected = InvalidParameterValueException.class) + public void deleteBackupScheduleTestThrowExceptionWhenVmIdAndScheduleIdAreNull() { + when(deleteBackupScheduleCmdMock.getVmId()).thenReturn(null); + when(deleteBackupScheduleCmdMock.getId()).thenReturn(null); + + backupManager.deleteBackupSchedule(deleteBackupScheduleCmdMock); + } + + @Test + public void deleteBackupScheduleTestDeleteVmSchedulesWhenVmIdIsSpecified() { + long vmId = 1L; + + when(deleteBackupScheduleCmdMock.getVmId()).thenReturn(vmId); + Mockito.doNothing().when(backupManager).checkCallerAccessToBackupScheduleVm(vmId); + Mockito.doReturn(true).when(backupManager).deleteAllVmBackupSchedules(vmId); + + boolean success = backupManager.deleteBackupSchedule(deleteBackupScheduleCmdMock); + assertTrue(success); + } + + @Test(expected = InvalidParameterValueException.class) + public void deleteBackupScheduleTestThrowExceptionWhenSpecificScheduleIsNotFound() { + long id = 1L; + when(deleteBackupScheduleCmdMock.getId()).thenReturn(id); + when(deleteBackupScheduleCmdMock.getVmId()).thenReturn(null); + backupManager.deleteBackupSchedule(deleteBackupScheduleCmdMock); + } + + @Test + public void deleteBackupScheduleTestDeleteSpecificScheduleWhenItsIdIsSpecified() { + long id = 1L; + long vmId = 2L; + when(deleteBackupScheduleCmdMock.getId()).thenReturn(id); + when(deleteBackupScheduleCmdMock.getVmId()).thenReturn(null); + when(backupScheduleDao.findById(id)).thenReturn(backupScheduleVOMock); + when(backupScheduleVOMock.getVmId()).thenReturn(vmId); + Mockito.doNothing().when(backupManager).checkCallerAccessToBackupScheduleVm(vmId); + when(backupScheduleVOMock.getId()).thenReturn(id); + when(backupScheduleDao.remove(id)).thenReturn(true); + + boolean success = backupManager.deleteBackupSchedule(deleteBackupScheduleCmdMock); + assertTrue(success); + } } diff --git a/ui/src/views/compute/backup/BackupSchedule.vue b/ui/src/views/compute/backup/BackupSchedule.vue index 0e1106e6504b..fd615e8e8d69 100644 --- a/ui/src/views/compute/backup/BackupSchedule.vue +++ b/ui/src/views/compute/backup/BackupSchedule.vue @@ -167,7 +167,7 @@ export default { methods: { handleClickDelete (record) { const params = {} - params.virtualmachineid = record.virtualmachineid + params.id = record.id this.actionLoading = true postAPI('deleteBackupSchedule', params).then(json => { if (json.deletebackupscheduleresponse.success) { From 2dd5fc4f1b543e1eaaffd41c3d2115b8473ab277 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bernardo=20De=20Marco=20Gon=C3=A7alves?= Date: Wed, 16 Jul 2025 12:42:06 -0300 Subject: [PATCH 2/2] apply suggestion --- .../user/backup/DeleteBackupScheduleCmd.java | 6 +++--- .../cloudstack/backup/BackupManagerImpl.java | 18 +++++++++--------- .../cloudstack/backup/BackupManagerTest.java | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/backup/DeleteBackupScheduleCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/backup/DeleteBackupScheduleCmd.java index e3e36ace4013..dbfd21d5a56b 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/backup/DeleteBackupScheduleCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/backup/DeleteBackupScheduleCmd.java @@ -53,12 +53,12 @@ public class DeleteBackupScheduleCmd extends BaseCmd { ///////////////////////////////////////////////////// @Parameter(name = ApiConstants.VIRTUAL_MACHINE_ID, type = CommandType.UUID, entityType = UserVmResponse.class, - description = "ID of the VM from which all backup schedules will be deleted. It has precedence over the 'id' parameter, " + - "i.e., when the 'virtualmachineid' parameter is specified, the 'id' parameter will be ignored.") + description = "ID of the VM from which all backup schedules will be deleted.") private Long vmId; @Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = BackupScheduleResponse.class, - description = "ID of the backup schedule to be deleted.", since = "4.20.1") + since = "4.20.1", description = "ID of the backup schedule to be deleted. It has precedence over the 'virtualmachineid' parameter, " + + "i.e., when the 'id' parameter is specified, the 'virtualmachineid' parameter will be ignored.") private Long id; ///////////////////////////////////////////////////// diff --git a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java index cd42ccbadf85..4a53edd95709 100644 --- a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java @@ -520,17 +520,17 @@ public boolean deleteBackupSchedule(DeleteBackupScheduleCmd cmd) { throw new InvalidParameterValueException("Either instance ID or ID of backup schedule needs to be specified."); } - if (Objects.nonNull(vmId)) { - checkCallerAccessToBackupScheduleVm(vmId); - return deleteAllVmBackupSchedules(vmId); + if (Objects.nonNull(id)) { + BackupSchedule schedule = backupScheduleDao.findById(id); + if (schedule == null) { + throw new InvalidParameterValueException("Could not find the requested backup schedule."); + } + checkCallerAccessToBackupScheduleVm(schedule.getVmId()); + return backupScheduleDao.remove(schedule.getId()); } - BackupSchedule schedule = backupScheduleDao.findById(id); - if (schedule == null) { - throw new InvalidParameterValueException("Could not find the requested backup schedule."); - } - checkCallerAccessToBackupScheduleVm(schedule.getVmId()); - return backupScheduleDao.remove(schedule.getId()); + checkCallerAccessToBackupScheduleVm(vmId); + return deleteAllVmBackupSchedules(vmId); } /** diff --git a/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java b/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java index 57ce555b3077..969a4202c51d 100644 --- a/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java +++ b/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java @@ -735,6 +735,7 @@ public void deleteBackupScheduleTestThrowExceptionWhenVmIdAndScheduleIdAreNull() public void deleteBackupScheduleTestDeleteVmSchedulesWhenVmIdIsSpecified() { long vmId = 1L; + when(deleteBackupScheduleCmdMock.getId()).thenReturn(null); when(deleteBackupScheduleCmdMock.getVmId()).thenReturn(vmId); Mockito.doNothing().when(backupManager).checkCallerAccessToBackupScheduleVm(vmId); Mockito.doReturn(true).when(backupManager).deleteAllVmBackupSchedules(vmId); @@ -747,7 +748,6 @@ public void deleteBackupScheduleTestDeleteVmSchedulesWhenVmIdIsSpecified() { public void deleteBackupScheduleTestThrowExceptionWhenSpecificScheduleIsNotFound() { long id = 1L; when(deleteBackupScheduleCmdMock.getId()).thenReturn(id); - when(deleteBackupScheduleCmdMock.getVmId()).thenReturn(null); backupManager.deleteBackupSchedule(deleteBackupScheduleCmdMock); }