Skip to content

Commit 13842a6

Browse files
winterhazelDaan Hoogland
winterhazel
authored and
Daan Hoogland
committed
Address reviews
1 parent d6f4fc3 commit 13842a6

7 files changed

Lines changed: 22 additions & 44 deletions

File tree

‎api/src/main/java/org/apache/cloudstack/api/command/user/bucket/DeleteBucketCmd.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
package org.apache.cloudstack.api.command.user.bucket;
1818

1919
import com.cloud.exception.ConcurrentOperationException;
20+
import com.cloud.exception.ResourceAllocationException;
2021
import org.apache.cloudstack.acl.RoleType;
2122
import org.apache.cloudstack.storage.object.Bucket;
2223
import com.cloud.user.Account;
@@ -82,7 +83,7 @@ public ApiCommandResourceType getApiResourceType() {
8283
}
8384

8485
@Override
85-
public void execute() throws ConcurrentOperationException {
86+
public void execute() throws ConcurrentOperationException, ResourceAllocationException {
8687
CallContext.current().setEventDetails("Bucket Id: " + this._uuidMgr.getUuid(Bucket.class, getId()));
8788
boolean result = _bucketService.deleteBucket(id, CallContext.current().getCallingAccount());
8889
SuccessResponse response = new SuccessResponse(getCommandName());

‎api/src/main/java/org/apache/cloudstack/backup/BackupManager.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ public interface BackupManager extends BackupService, Configurable, PluggableSer
224224
* @param forced Indicates if backup will be force removed or not
225225
* @return returns operation success
226226
*/
227-
boolean deleteBackup(final Long backupId, final Boolean forced);
227+
boolean deleteBackup(final Long backupId, final Boolean forced) throws ResourceAllocationException;
228228

229229
void validateBackupForZone(Long zoneId);
230230

‎api/src/main/java/org/apache/cloudstack/storage/object/BucketApiService.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ public interface BucketApiService {
9595
*/
9696
Bucket createBucket(CreateBucketCmd cmd);
9797

98-
boolean deleteBucket(long bucketId, Account caller);
98+
boolean deleteBucket(long bucketId, Account caller) throws ResourceAllocationException;
9999

100100
boolean updateBucket(UpdateBucketCmd cmd, Account caller) throws ResourceAllocationException;
101101

‎server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java‎

Lines changed: 9 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -843,18 +843,12 @@ private void createCheckedBackup(CreateBackupCmd cmd, Account owner, boolean isS
843843
resourceLimitMgr.incrementResourceCount(vm.getAccountId(), Resource.ResourceType.backup);
844844
resourceLimitMgr.incrementResourceCount(vm.getAccountId(), Resource.ResourceType.backup_storage, backup.getSize());
845845
}
846-
} catch (Exception e) {
847-
if (e instanceof ResourceAllocationException) {
848-
ResourceAllocationException rae = (ResourceAllocationException)e;
849-
if (isScheduledBackup && (Resource.ResourceType.backup.equals(rae.getResourceType()) ||
850-
Resource.ResourceType.backup_storage.equals(rae.getResourceType()))) {
851-
sendExceededBackupLimitAlert(owner.getUuid(), rae.getResourceType());
852-
}
853-
throw rae;
854-
} else if (e instanceof CloudRuntimeException) {
855-
throw (CloudRuntimeException)e;
846+
} catch (ResourceAllocationException e) {
847+
if (isScheduledBackup && (Resource.ResourceType.backup.equals(e.getResourceType()) ||
848+
Resource.ResourceType.backup_storage.equals(e.getResourceType()))) {
849+
sendExceededBackupLimitAlert(owner.getUuid(), e.getResourceType());
856850
}
857-
throw new CloudRuntimeException("Failed to create backup for VM with ID: " + vm.getUuid(), e);
851+
throw e;
858852
}
859853
}
860854

@@ -907,7 +901,7 @@ protected Long getBackupScheduleId(Object job) {
907901
* @param vmId The ID of the VM associated with the backups
908902
* @param backupScheduleId Backup schedule ID of the backups
909903
*/
910-
protected void deleteOldestBackupFromScheduleIfRequired(Long vmId, long backupScheduleId) {
904+
protected void deleteOldestBackupFromScheduleIfRequired(Long vmId, long backupScheduleId) throws ResourceAllocationException {
911905
BackupScheduleVO backupScheduleVO = backupScheduleDao.findById(backupScheduleId);
912906
if (backupScheduleVO == null || backupScheduleVO.getMaxBackups() == 0) {
913907
logger.info("The schedule does not have a retention specified and, hence, not deleting any backups from it.", vmId);
@@ -931,7 +925,7 @@ protected void deleteOldestBackupFromScheduleIfRequired(Long vmId, long backupSc
931925
* @param amountOfBackupsToDelete Number of backups to be deleted from the list of backups
932926
* @param backupScheduleId ID of the backup schedule associated with the backups
933927
*/
934-
protected void deleteExcessBackups(List<BackupVO> backups, int amountOfBackupsToDelete, long backupScheduleId) {
928+
protected void deleteExcessBackups(List<BackupVO> backups, int amountOfBackupsToDelete, long backupScheduleId) throws ResourceAllocationException {
935929
logger.debug("Deleting the [{}] oldest backups from the schedule [ID: {}].", amountOfBackupsToDelete, backupScheduleId);
936930

937931
for (int i = 0; i < amountOfBackupsToDelete; i++) {
@@ -1529,7 +1523,7 @@ protected Pair<Boolean, String> restoreBackedUpVolume(final Backup.VolumeInfo ba
15291523

15301524
@Override
15311525
@ActionEvent(eventType = EventTypes.EVENT_VM_BACKUP_DELETE, eventDescription = "deleting VM backup", async = true)
1532-
public boolean deleteBackup(final Long backupId, final Boolean forced) {
1526+
public boolean deleteBackup(final Long backupId, final Boolean forced) throws ResourceAllocationException {
15331527
final BackupVO backup = backupDao.findByIdIncludingRemoved(backupId);
15341528
if (backup == null) {
15351529
throw new CloudRuntimeException("Backup " + backupId + " does not exist");
@@ -1552,7 +1546,7 @@ public boolean deleteBackup(final Long backupId, final Boolean forced) {
15521546
return deleteCheckedBackup(forced, backupProvider, backup, vm);
15531547
}
15541548

1555-
private boolean deleteCheckedBackup(Boolean forced, BackupProvider backupProvider, BackupVO backup, VMInstanceVO vm) {
1549+
private boolean deleteCheckedBackup(Boolean forced, BackupProvider backupProvider, BackupVO backup, VMInstanceVO vm) throws ResourceAllocationException {
15561550
Account owner = accountManager.getAccount(backup.getAccountId());
15571551
long backupSize = backup.getSize() != null ? backup.getSize() : 0L;
15581552
try (CheckedReservation backupReservation = new CheckedReservation(owner, Resource.ResourceType.backup,
@@ -1572,11 +1566,6 @@ private boolean deleteCheckedBackup(Boolean forced, BackupProvider backupProvide
15721566
}
15731567
}
15741568
throw new CloudRuntimeException("Failed to delete the backup");
1575-
} catch (Exception e) {
1576-
if (e instanceof CloudRuntimeException) {
1577-
throw (CloudRuntimeException) e;
1578-
}
1579-
throw new CloudRuntimeException("Failed to delete the backup due to: " + e.getMessage(), e);
15801569
}
15811570
}
15821571

‎server/src/main/java/org/apache/cloudstack/storage/object/BucketApiServiceImpl.java‎

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -163,13 +163,6 @@ private BucketVO createCheckedBucket(CreateBucketCmd cmd, Account owner, long si
163163
(cmd.getQuota() * Resource.ResourceType.bytesToGiB));
164164
}
165165
return bucket;
166-
} catch (Exception e) {
167-
if (e instanceof ResourceAllocationException) {
168-
throw (ResourceAllocationException)e;
169-
} else if (e instanceof CloudRuntimeException) {
170-
throw (CloudRuntimeException)e;
171-
}
172-
throw new CloudRuntimeException(String.format("Failed to create bucket due to: %s", e.getMessage()), e);
173166
}
174167
}
175168

@@ -236,7 +229,7 @@ public Bucket createBucket(CreateBucketCmd cmd) {
236229

237230
@Override
238231
@ActionEvent(eventType = EventTypes.EVENT_BUCKET_DELETE, eventDescription = "deleting bucket")
239-
public boolean deleteBucket(long bucketId, Account caller) {
232+
public boolean deleteBucket(long bucketId, Account caller) throws ResourceAllocationException {
240233
Bucket bucket = _bucketDao.findById(bucketId);
241234
if (bucket == null) {
242235
throw new InvalidParameterValueException("Unable to find bucket with ID: " + bucketId);
@@ -247,7 +240,7 @@ public boolean deleteBucket(long bucketId, Account caller) {
247240
return deleteCheckedBucket(objectStore, bucket, objectStoreVO);
248241
}
249242

250-
private boolean deleteCheckedBucket(ObjectStoreEntity objectStore, Bucket bucket, ObjectStoreVO objectStoreVO) {
243+
private boolean deleteCheckedBucket(ObjectStoreEntity objectStore, Bucket bucket, ObjectStoreVO objectStoreVO) throws ResourceAllocationException {
251244
Account owner = _accountMgr.getAccount(bucket.getAccountId());
252245
try (CheckedReservation bucketReservation = new CheckedReservation(owner, Resource.ResourceType.bucket,
253246
bucket.getId(), null, -1L, reservationDao, resourceLimitManager);
@@ -265,11 +258,6 @@ private boolean deleteCheckedBucket(ObjectStoreEntity objectStore, Bucket bucket
265258
return true;
266259
}
267260
return false;
268-
} catch (Exception e) {
269-
if (e instanceof CloudRuntimeException) {
270-
throw (CloudRuntimeException) e;
271-
}
272-
throw new CloudRuntimeException(String.format("Failed to delete bucket due to: %s", e.getMessage()), e);
273261
}
274262
}
275263

‎server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1526,7 +1526,7 @@ public void testGetIpToNetworkMapFromBackup() {
15261526
}
15271527

15281528
@Test
1529-
public void testDeleteBackupVmNotFound() {
1529+
public void testDeleteBackupVmNotFound() throws ResourceAllocationException {
15301530
Long backupId = 1L;
15311531
Long vmId = 2L;
15321532
Long zoneId = 3L;
@@ -1782,21 +1782,21 @@ public void getBackupScheduleTestReturnNullWhenSpecifiedBackupScheduleIdIsNotALo
17821782
}
17831783

17841784
@Test
1785-
public void deleteOldestBackupFromScheduleIfRequiredTestSkipDeletionWhenBackupScheduleIsNotFound() {
1785+
public void deleteOldestBackupFromScheduleIfRequiredTestSkipDeletionWhenBackupScheduleIsNotFound() throws ResourceAllocationException {
17861786
backupManager.deleteOldestBackupFromScheduleIfRequired(1L, 1L);
17871787
Mockito.verify(backupManager, Mockito.never()).deleteExcessBackups(Mockito.anyList(), Mockito.anyInt(), Mockito.anyLong());
17881788
}
17891789

17901790
@Test
1791-
public void deleteOldestBackupFromScheduleIfRequiredTestSkipDeletionWhenRetentionIsEqualToZero() {
1791+
public void deleteOldestBackupFromScheduleIfRequiredTestSkipDeletionWhenRetentionIsEqualToZero() throws ResourceAllocationException {
17921792
Mockito.when(backupScheduleDao.findById(1L)).thenReturn(backupScheduleVOMock);
17931793
Mockito.when(backupScheduleVOMock.getMaxBackups()).thenReturn(0);
17941794
backupManager.deleteOldestBackupFromScheduleIfRequired(1L, 1L);
17951795
Mockito.verify(backupManager, Mockito.never()).deleteExcessBackups(Mockito.anyList(), Mockito.anyInt(), Mockito.anyLong());
17961796
}
17971797

17981798
@Test
1799-
public void deleteOldestBackupFromScheduleIfRequiredTestSkipDeletionWhenAmountOfBackupsToBeDeletedIsLessThanOne() {
1799+
public void deleteOldestBackupFromScheduleIfRequiredTestSkipDeletionWhenAmountOfBackupsToBeDeletedIsLessThanOne() throws ResourceAllocationException {
18001800
List<BackupVO> backups = List.of(Mockito.mock(BackupVO.class), Mockito.mock(BackupVO.class));
18011801
Mockito.when(backupScheduleDao.findById(1L)).thenReturn(backupScheduleVOMock);
18021802
Mockito.when(backupScheduleVOMock.getMaxBackups()).thenReturn(2);
@@ -1806,7 +1806,7 @@ public void deleteOldestBackupFromScheduleIfRequiredTestSkipDeletionWhenAmountOf
18061806
}
18071807

18081808
@Test
1809-
public void deleteOldestBackupFromScheduleIfRequiredTestDeleteBackupsWhenRequired() {
1809+
public void deleteOldestBackupFromScheduleIfRequiredTestDeleteBackupsWhenRequired() throws ResourceAllocationException {
18101810
List<BackupVO> backups = List.of(Mockito.mock(BackupVO.class), Mockito.mock(BackupVO.class));
18111811
Mockito.when(backupScheduleDao.findById(1L)).thenReturn(backupScheduleVOMock);
18121812
Mockito.when(backupScheduleVOMock.getMaxBackups()).thenReturn(1);
@@ -1817,7 +1817,7 @@ public void deleteOldestBackupFromScheduleIfRequiredTestDeleteBackupsWhenRequire
18171817
}
18181818

18191819
@Test
1820-
public void deleteExcessBackupsTestEnsureBackupsAreDeletedWhenMethodIsCalled() {
1820+
public void deleteExcessBackupsTestEnsureBackupsAreDeletedWhenMethodIsCalled() throws ResourceAllocationException {
18211821
try (MockedStatic<ActionEventUtils> actionEventUtils = Mockito.mockStatic(ActionEventUtils.class)) {
18221822
List<BackupVO> backups = List.of(Mockito.mock(BackupVO.class),
18231823
Mockito.mock(BackupVO.class),

‎server/src/test/java/org/apache/cloudstack/storage/object/BucketApiServiceImplTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,7 @@ public void testCreateBucket() {
199199
}
200200

201201
@Test
202-
public void testDeleteBucket() {
202+
public void testDeleteBucket() throws ResourceAllocationException {
203203
Long bucketId = 1L;
204204
Long objectStoreId = 3L;
205205
String bucketName = "bucket1";

0 commit comments

Comments
 (0)