Copilot commented on code in PR #12792:
URL: https://github.com/apache/cloudstack/pull/12792#discussion_r2922668687
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -1551,6 +1553,17 @@ public boolean deleteBackup(final Long backupId, final
Boolean forced) {
throw new CloudRuntimeException("Failed to delete the backup");
}
+ private void checkForPendingBackupJobs(final BackupVO backup) {
+ String backupUuid = backup.getUuid();
+ long pendingJobs = asyncJobManager.countPendingJobs(backupUuid,
+ CreateVMFromBackupCmd.class.getName(),
+ CreateVMFromBackupCmdByAdmin.class.getName(),
+ RestoreBackupCmd.class.getName());
Review Comment:
`checkForPendingBackupJobs` only checks for `CreateVMFromBackup*` and
`RestoreBackupCmd`. There is also an async restore operation
`RestoreVolumeFromBackupAndAttachToVMCmd` that uses a backup as input; deleting
the backup while that job is in progress can fail the restore similarly.
Consider including that command class in the pending-jobs check (or
centralizing a list of “backup-consuming” job types).
```suggestion
RestoreBackupCmd.class.getName(),
RestoreVolumeFromBackupAndAttachToVMCmd.class.getName());
```
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -1551,6 +1553,17 @@ public boolean deleteBackup(final Long backupId, final
Boolean forced) {
throw new CloudRuntimeException("Failed to delete the backup");
}
+ private void checkForPendingBackupJobs(final BackupVO backup) {
+ String backupUuid = backup.getUuid();
+ long pendingJobs = asyncJobManager.countPendingJobs(backupUuid,
+ CreateVMFromBackupCmd.class.getName(),
+ CreateVMFromBackupCmdByAdmin.class.getName(),
+ RestoreBackupCmd.class.getName());
+ if (pendingJobs > 0) {
+ throw new CloudRuntimeException("Cannot delete backup while a
create instance from backup or restore operation is in progress, please try
again later.");
Review Comment:
The exception message has awkward grammar (“while a create instance…”) and
doesn’t identify the backup being blocked. Consider rephrasing to reference the
actual API operations (createVMFromBackup/restoreBackup) and include the backup
ID/UUID so operators can correlate the failure quickly.
```suggestion
throw new CloudRuntimeException(String.format(
"Cannot delete backup with UUID [%s] while a
createVMFromBackup or restoreBackup operation is in progress (pending jobs:
%d). Please try again later.",
backupUuid, pendingJobs));
```
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -1529,6 +1529,8 @@ public boolean deleteBackup(final Long backupId, final
Boolean forced) {
}
logger.debug("Deleting backup {} belonging to instance {}",
backup.toString(), vmId);
+ checkForPendingBackupJobs(backup);
+
validateBackupForZone(backup.getZoneId());
accountManager.checkAccess(CallContext.current().getCallingAccount(),
null, true, vm == null ? backup : vm);
Review Comment:
`checkForPendingBackupJobs(backup)` is executed before
`accountManager.checkAccess(...)`. This changes the error precedence (a caller
without access could now get the “pending job” error) and it also runs an extra
DB query before authorization. Consider moving the pending-job check to after
the access check (and after `validateBackupForZone(...)` if you want
config/zone validation to take precedence).
```suggestion
validateBackupForZone(backup.getZoneId());
accountManager.checkAccess(CallContext.current().getCallingAccount(), null,
true, vm == null ? backup : vm);
checkForPendingBackupJobs(backup);
```
##########
server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java:
##########
@@ -1523,6 +1528,29 @@ public void testDeleteBackupVmNotFound() {
}
}
+ @Test(expected = CloudRuntimeException.class)
+ public void testDeleteBackupBlockedByPendingJobs() {
+ Long backupId = 1L;
+ Long vmId = 2L;
+
+ BackupVO backup = mock(BackupVO.class);
+ when(backup.getVmId()).thenReturn(vmId);
+ when(backup.getUuid()).thenReturn("backup-uuid");
+ when(backupDao.findByIdIncludingRemoved(backupId)).thenReturn(backup);
+
+ VMInstanceVO vm = mock(VMInstanceVO.class);
+ when(vmInstanceDao.findByIdIncludingRemoved(vmId)).thenReturn(vm);
+
+ overrideBackupFrameworkConfigValue();
+
+ when(asyncJobManager.countPendingJobs("backup-uuid",
+
"org.apache.cloudstack.api.command.user.vm.CreateVMFromBackupCmd",
+
"org.apache.cloudstack.api.command.admin.vm.CreateVMFromBackupCmdByAdmin",
+
"org.apache.cloudstack.api.command.user.backup.RestoreBackupCmd")).thenReturn(1L);
+
+ backupManager.deleteBackup(backupId, false);
+ }
Review Comment:
This test uses `@Test(expected=...)`, which can pass even if `deleteBackup`
throws for a different reason. To make the intent explicit, consider using
`assertThrows` and asserting the exception message and/or verifying that
`backupProvider.deleteBackup(...)` is never invoked when `countPendingJobs(...)
> 0`.
--
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]
