Address reviews
diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/bucket/DeleteBucketCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/bucket/DeleteBucketCmd.java index 8cd2790..e7b940f 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/bucket/DeleteBucketCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/bucket/DeleteBucketCmd.java
@@ -17,6 +17,7 @@ package org.apache.cloudstack.api.command.user.bucket; import com.cloud.exception.ConcurrentOperationException; +import com.cloud.exception.ResourceAllocationException; import org.apache.cloudstack.acl.RoleType; import org.apache.cloudstack.storage.object.Bucket; import com.cloud.user.Account; @@ -82,7 +83,7 @@ } @Override - public void execute() throws ConcurrentOperationException { + public void execute() throws ConcurrentOperationException, ResourceAllocationException { CallContext.current().setEventDetails("Bucket Id: " + this._uuidMgr.getUuid(Bucket.class, getId())); boolean result = _bucketService.deleteBucket(id, CallContext.current().getCallingAccount()); SuccessResponse response = new SuccessResponse(getCommandName());
diff --git a/api/src/main/java/org/apache/cloudstack/backup/BackupManager.java b/api/src/main/java/org/apache/cloudstack/backup/BackupManager.java index db05131..2f9bfdd 100644 --- a/api/src/main/java/org/apache/cloudstack/backup/BackupManager.java +++ b/api/src/main/java/org/apache/cloudstack/backup/BackupManager.java
@@ -224,7 +224,7 @@ * @param forced Indicates if backup will be force removed or not * @return returns operation success */ - boolean deleteBackup(final Long backupId, final Boolean forced); + boolean deleteBackup(final Long backupId, final Boolean forced) throws ResourceAllocationException; void validateBackupForZone(Long zoneId);
diff --git a/api/src/main/java/org/apache/cloudstack/storage/object/BucketApiService.java b/api/src/main/java/org/apache/cloudstack/storage/object/BucketApiService.java index e27ef30..8c16413 100644 --- a/api/src/main/java/org/apache/cloudstack/storage/object/BucketApiService.java +++ b/api/src/main/java/org/apache/cloudstack/storage/object/BucketApiService.java
@@ -95,7 +95,7 @@ */ Bucket createBucket(CreateBucketCmd cmd); - boolean deleteBucket(long bucketId, Account caller); + boolean deleteBucket(long bucketId, Account caller) throws ResourceAllocationException; boolean updateBucket(UpdateBucketCmd cmd, Account caller) throws ResourceAllocationException;
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 088517e..a38d4f6 100644 --- a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java
@@ -843,18 +843,12 @@ resourceLimitMgr.incrementResourceCount(vm.getAccountId(), Resource.ResourceType.backup); resourceLimitMgr.incrementResourceCount(vm.getAccountId(), Resource.ResourceType.backup_storage, backup.getSize()); } - } catch (Exception e) { - if (e instanceof ResourceAllocationException) { - ResourceAllocationException rae = (ResourceAllocationException)e; - if (isScheduledBackup && (Resource.ResourceType.backup.equals(rae.getResourceType()) || - Resource.ResourceType.backup_storage.equals(rae.getResourceType()))) { - sendExceededBackupLimitAlert(owner.getUuid(), rae.getResourceType()); - } - throw rae; - } else if (e instanceof CloudRuntimeException) { - throw (CloudRuntimeException)e; + } catch (ResourceAllocationException e) { + if (isScheduledBackup && (Resource.ResourceType.backup.equals(e.getResourceType()) || + Resource.ResourceType.backup_storage.equals(e.getResourceType()))) { + sendExceededBackupLimitAlert(owner.getUuid(), e.getResourceType()); } - throw new CloudRuntimeException("Failed to create backup for VM with ID: " + vm.getUuid(), e); + throw e; } } @@ -907,7 +901,7 @@ * @param vmId The ID of the VM associated with the backups * @param backupScheduleId Backup schedule ID of the backups */ - protected void deleteOldestBackupFromScheduleIfRequired(Long vmId, long backupScheduleId) { + protected void deleteOldestBackupFromScheduleIfRequired(Long vmId, long backupScheduleId) throws ResourceAllocationException { BackupScheduleVO backupScheduleVO = backupScheduleDao.findById(backupScheduleId); if (backupScheduleVO == null || backupScheduleVO.getMaxBackups() == 0) { logger.info("The schedule does not have a retention specified and, hence, not deleting any backups from it.", vmId); @@ -931,7 +925,7 @@ * @param amountOfBackupsToDelete Number of backups to be deleted from the list of backups * @param backupScheduleId ID of the backup schedule associated with the backups */ - protected void deleteExcessBackups(List<BackupVO> backups, int amountOfBackupsToDelete, long backupScheduleId) { + protected void deleteExcessBackups(List<BackupVO> backups, int amountOfBackupsToDelete, long backupScheduleId) throws ResourceAllocationException { logger.debug("Deleting the [{}] oldest backups from the schedule [ID: {}].", amountOfBackupsToDelete, backupScheduleId); for (int i = 0; i < amountOfBackupsToDelete; i++) { @@ -1529,7 +1523,7 @@ @Override @ActionEvent(eventType = EventTypes.EVENT_VM_BACKUP_DELETE, eventDescription = "deleting VM backup", async = true) - public boolean deleteBackup(final Long backupId, final Boolean forced) { + public boolean deleteBackup(final Long backupId, final Boolean forced) throws ResourceAllocationException { final BackupVO backup = backupDao.findByIdIncludingRemoved(backupId); if (backup == null) { throw new CloudRuntimeException("Backup " + backupId + " does not exist"); @@ -1552,7 +1546,7 @@ return deleteCheckedBackup(forced, backupProvider, backup, vm); } - private boolean deleteCheckedBackup(Boolean forced, BackupProvider backupProvider, BackupVO backup, VMInstanceVO vm) { + private boolean deleteCheckedBackup(Boolean forced, BackupProvider backupProvider, BackupVO backup, VMInstanceVO vm) throws ResourceAllocationException { Account owner = accountManager.getAccount(backup.getAccountId()); long backupSize = backup.getSize() != null ? backup.getSize() : 0L; try (CheckedReservation backupReservation = new CheckedReservation(owner, Resource.ResourceType.backup, @@ -1572,11 +1566,6 @@ } } throw new CloudRuntimeException("Failed to delete the backup"); - } catch (Exception e) { - if (e instanceof CloudRuntimeException) { - throw (CloudRuntimeException) e; - } - throw new CloudRuntimeException("Failed to delete the backup due to: " + e.getMessage(), e); } }
diff --git a/server/src/main/java/org/apache/cloudstack/storage/object/BucketApiServiceImpl.java b/server/src/main/java/org/apache/cloudstack/storage/object/BucketApiServiceImpl.java index 59adb3a..21f0199 100644 --- a/server/src/main/java/org/apache/cloudstack/storage/object/BucketApiServiceImpl.java +++ b/server/src/main/java/org/apache/cloudstack/storage/object/BucketApiServiceImpl.java
@@ -163,13 +163,6 @@ (cmd.getQuota() * Resource.ResourceType.bytesToGiB)); } return bucket; - } catch (Exception e) { - if (e instanceof ResourceAllocationException) { - throw (ResourceAllocationException)e; - } else if (e instanceof CloudRuntimeException) { - throw (CloudRuntimeException)e; - } - throw new CloudRuntimeException(String.format("Failed to create bucket due to: %s", e.getMessage()), e); } } @@ -236,7 +229,7 @@ @Override @ActionEvent(eventType = EventTypes.EVENT_BUCKET_DELETE, eventDescription = "deleting bucket") - public boolean deleteBucket(long bucketId, Account caller) { + public boolean deleteBucket(long bucketId, Account caller) throws ResourceAllocationException { Bucket bucket = _bucketDao.findById(bucketId); if (bucket == null) { throw new InvalidParameterValueException("Unable to find bucket with ID: " + bucketId); @@ -247,7 +240,7 @@ return deleteCheckedBucket(objectStore, bucket, objectStoreVO); } - private boolean deleteCheckedBucket(ObjectStoreEntity objectStore, Bucket bucket, ObjectStoreVO objectStoreVO) { + private boolean deleteCheckedBucket(ObjectStoreEntity objectStore, Bucket bucket, ObjectStoreVO objectStoreVO) throws ResourceAllocationException { Account owner = _accountMgr.getAccount(bucket.getAccountId()); try (CheckedReservation bucketReservation = new CheckedReservation(owner, Resource.ResourceType.bucket, bucket.getId(), null, -1L, reservationDao, resourceLimitManager); @@ -265,11 +258,6 @@ return true; } return false; - } catch (Exception e) { - if (e instanceof CloudRuntimeException) { - throw (CloudRuntimeException) e; - } - throw new CloudRuntimeException(String.format("Failed to delete bucket due to: %s", e.getMessage()), e); } }
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 7395f22..6abacc3 100644 --- a/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java +++ b/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java
@@ -1526,7 +1526,7 @@ } @Test - public void testDeleteBackupVmNotFound() { + public void testDeleteBackupVmNotFound() throws ResourceAllocationException { Long backupId = 1L; Long vmId = 2L; Long zoneId = 3L; @@ -1782,13 +1782,13 @@ } @Test - public void deleteOldestBackupFromScheduleIfRequiredTestSkipDeletionWhenBackupScheduleIsNotFound() { + public void deleteOldestBackupFromScheduleIfRequiredTestSkipDeletionWhenBackupScheduleIsNotFound() throws ResourceAllocationException { backupManager.deleteOldestBackupFromScheduleIfRequired(1L, 1L); Mockito.verify(backupManager, Mockito.never()).deleteExcessBackups(Mockito.anyList(), Mockito.anyInt(), Mockito.anyLong()); } @Test - public void deleteOldestBackupFromScheduleIfRequiredTestSkipDeletionWhenRetentionIsEqualToZero() { + public void deleteOldestBackupFromScheduleIfRequiredTestSkipDeletionWhenRetentionIsEqualToZero() throws ResourceAllocationException { Mockito.when(backupScheduleDao.findById(1L)).thenReturn(backupScheduleVOMock); Mockito.when(backupScheduleVOMock.getMaxBackups()).thenReturn(0); backupManager.deleteOldestBackupFromScheduleIfRequired(1L, 1L); @@ -1796,7 +1796,7 @@ } @Test - public void deleteOldestBackupFromScheduleIfRequiredTestSkipDeletionWhenAmountOfBackupsToBeDeletedIsLessThanOne() { + public void deleteOldestBackupFromScheduleIfRequiredTestSkipDeletionWhenAmountOfBackupsToBeDeletedIsLessThanOne() throws ResourceAllocationException { List<BackupVO> backups = List.of(Mockito.mock(BackupVO.class), Mockito.mock(BackupVO.class)); Mockito.when(backupScheduleDao.findById(1L)).thenReturn(backupScheduleVOMock); Mockito.when(backupScheduleVOMock.getMaxBackups()).thenReturn(2); @@ -1806,7 +1806,7 @@ } @Test - public void deleteOldestBackupFromScheduleIfRequiredTestDeleteBackupsWhenRequired() { + public void deleteOldestBackupFromScheduleIfRequiredTestDeleteBackupsWhenRequired() throws ResourceAllocationException { List<BackupVO> backups = List.of(Mockito.mock(BackupVO.class), Mockito.mock(BackupVO.class)); Mockito.when(backupScheduleDao.findById(1L)).thenReturn(backupScheduleVOMock); Mockito.when(backupScheduleVOMock.getMaxBackups()).thenReturn(1); @@ -1817,7 +1817,7 @@ } @Test - public void deleteExcessBackupsTestEnsureBackupsAreDeletedWhenMethodIsCalled() { + public void deleteExcessBackupsTestEnsureBackupsAreDeletedWhenMethodIsCalled() throws ResourceAllocationException { try (MockedStatic<ActionEventUtils> actionEventUtils = Mockito.mockStatic(ActionEventUtils.class)) { List<BackupVO> backups = List.of(Mockito.mock(BackupVO.class), Mockito.mock(BackupVO.class),
diff --git a/server/src/test/java/org/apache/cloudstack/storage/object/BucketApiServiceImplTest.java b/server/src/test/java/org/apache/cloudstack/storage/object/BucketApiServiceImplTest.java index 6dbdd45..a4429be 100644 --- a/server/src/test/java/org/apache/cloudstack/storage/object/BucketApiServiceImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/storage/object/BucketApiServiceImplTest.java
@@ -199,7 +199,7 @@ } @Test - public void testDeleteBucket() { + public void testDeleteBucket() throws ResourceAllocationException { Long bucketId = 1L; Long objectStoreId = 3L; String bucketName = "bucket1";