Fix deletion of backup schedules (apache#11222)

bernardodemarco · dhslove · commit 5dd844852612 · 2025-07-28T15:27:54.000+09:00
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
@@ -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.")
     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,
+            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/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
@@ -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
@@ -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
@@ -92,12 +92,13 @@ public List<BackupScheduleVO> 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
@@ -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
@@ -81,6 +81,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.cloud.api.ApiDispatcher;
@@ -516,24 +517,44 @@ public List<BackupSchedule> 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 (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 (ObjectUtils.allNull(vmId, id)) {
+            throw new InvalidParameterValueException("Either instance ID or ID of backup schedule needs to be specified.");
+        }
+
+        if (Objects.nonNull(id)) {
+            BackupSchedule schedule = backupScheduleDao.findById(id);
             if (schedule == null) {
-                throw new CloudRuntimeException("Could not find the requested backup schedule.");
+                throw new InvalidParameterValueException("Could not find the requested backup schedule.");
             }
+            checkCallerAccessToBackupScheduleVm(schedule.getVmId());
             return backupScheduleDao.remove(schedule.getId());
         }
+
+        checkCallerAccessToBackupScheduleVm(vmId);
+        return deleteAllVmBackupSchedules(vmId);
+    }
+
+    /**
+     * 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<BackupScheduleVO> 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
@@ -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<CallContext> 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<BackupScheduleVO> 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<BackupScheduleVO> 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.getId()).thenReturn(null);
+        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);
+        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
@@ -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) {

Original file line number	Diff line number	Diff line change
`@@ -31,4 +31,5 @@ public interface BackupSchedule extends InternalIdentity {`
`31`	`31`	`Date getScheduledTimestamp();`
`32`	`32`	`Long getAsyncJobId();`
`33`	`33`	`Integer getMaxBackups();`
	`34`	`+ String getUuid();`
`34`	`35`	`}`