diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/backup/DeleteBackupCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/backup/DeleteBackupCmd.java index 32344d8992b..4098158a833 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/backup/DeleteBackupCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/backup/DeleteBackupCmd.java @@ -31,6 +31,7 @@ import org.apache.cloudstack.api.response.BackupResponse; import org.apache.cloudstack.api.response.SuccessResponse; import org.apache.cloudstack.backup.BackupManager; import org.apache.cloudstack.context.CallContext; +import org.apache.commons.lang3.BooleanUtils; import com.cloud.event.EventTypes; import com.cloud.exception.ConcurrentOperationException; @@ -61,6 +62,13 @@ public class DeleteBackupCmd extends BaseAsyncCmd { description = "id of the VM backup") private Long backupId; + @Parameter(name = ApiConstants.FORCED, + type = CommandType.BOOLEAN, + required = false, + description = "force the deletion of backup which removes the entire backup chain but keep VM in Backup Offering", + since = "4.18.0.0") + private Boolean forced; + ///////////////////////////////////////////////////// /////////////////// Accessors /////////////////////// ///////////////////////////////////////////////////// @@ -69,6 +77,10 @@ public class DeleteBackupCmd extends BaseAsyncCmd { return backupId; } + public Boolean isForced() { + return BooleanUtils.isTrue(forced); + } + ///////////////////////////////////////////////////// /////////////// API Implementation/////////////////// ///////////////////////////////////////////////////// @@ -76,7 +88,7 @@ public class DeleteBackupCmd extends BaseAsyncCmd { @Override public void execute() throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException, ConcurrentOperationException, ResourceAllocationException, NetworkRuleConflictException { try { - boolean result = backupManager.deleteBackup(backupId); + boolean result = backupManager.deleteBackup(getId(), isForced()); if (result) { SuccessResponse response = new SuccessResponse(getCommandName()); response.setResponseName(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 b983aca4ad6..1285be3b7b3 100644 --- a/api/src/main/java/org/apache/cloudstack/backup/BackupManager.java +++ b/api/src/main/java/org/apache/cloudstack/backup/BackupManager.java @@ -135,9 +135,11 @@ public interface BackupManager extends BackupService, Configurable, PluggableSer /** * Deletes a backup + * @param backupId The Id of Backup to exclude + * @param forced Indicates if backup will be force removed or not * @return returns operation success */ - boolean deleteBackup(final Long backupId); + boolean deleteBackup(final Long backupId, final Boolean forced); BackupOffering updateBackupOffering(UpdateBackupOfferingCmd updateBackupOfferingCmd); } diff --git a/api/src/main/java/org/apache/cloudstack/backup/BackupProvider.java b/api/src/main/java/org/apache/cloudstack/backup/BackupProvider.java index ff05a38eeb6..9c1b14ae60f 100644 --- a/api/src/main/java/org/apache/cloudstack/backup/BackupProvider.java +++ b/api/src/main/java/org/apache/cloudstack/backup/BackupProvider.java @@ -79,10 +79,11 @@ public interface BackupProvider { /** * Delete an existing backup - * @param backup + * @param backuo The backup to exclude + * @param forced Indicates if backup will be force removed or not * @return */ - boolean deleteBackup(Backup backup); + boolean deleteBackup(Backup backup, boolean forced); /** * Restore VM from backup diff --git a/plugins/backup/dummy/src/main/java/org/apache/cloudstack/backup/DummyBackupProvider.java b/plugins/backup/dummy/src/main/java/org/apache/cloudstack/backup/DummyBackupProvider.java index 3139da8fcdd..0297ce82f5e 100644 --- a/plugins/backup/dummy/src/main/java/org/apache/cloudstack/backup/DummyBackupProvider.java +++ b/plugins/backup/dummy/src/main/java/org/apache/cloudstack/backup/DummyBackupProvider.java @@ -129,7 +129,7 @@ public class DummyBackupProvider extends AdapterBase implements BackupProvider { } @Override - public boolean deleteBackup(Backup backup) { + public boolean deleteBackup(Backup backup, boolean forced) { return true; } diff --git a/plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/VeeamBackupProvider.java b/plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/VeeamBackupProvider.java index ca611487820..02f08d602bb 100644 --- a/plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/VeeamBackupProvider.java +++ b/plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/VeeamBackupProvider.java @@ -53,6 +53,7 @@ import com.cloud.utils.db.TransactionStatus; import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.vm.VMInstanceVO; import com.cloud.vm.VirtualMachine; +import com.cloud.vm.dao.VMInstanceDao; public class VeeamBackupProvider extends AdapterBase implements BackupProvider, Configurable { @@ -86,8 +87,10 @@ public class VeeamBackupProvider extends AdapterBase implements BackupProvider, private VmwareDatacenterDao vmwareDatacenterDao; @Inject private BackupDao backupDao; + @Inject + private VMInstanceDao vmInstanceDao; - private VeeamClient getClient(final Long zoneId) { + protected VeeamClient getClient(final Long zoneId) { try { return new VeeamClient(VeeamUrl.valueIn(zoneId), VeeamUsername.valueIn(zoneId), VeeamPassword.valueIn(zoneId), VeeamValidateSSLSecurity.valueIn(zoneId), VeeamApiRequestTimeout.valueIn(zoneId), VeeamRestoreTimeout.valueIn(zoneId)); @@ -201,9 +204,31 @@ public class VeeamBackupProvider extends AdapterBase implements BackupProvider, } @Override - public boolean deleteBackup(Backup backup) { - // Veeam does not support removal of a restore point or point-in-time backup - throw new CloudRuntimeException("Veeam B&R plugin does not allow removal of backup restore point, to delete the backup chain remove VM from the backup offering"); + public boolean deleteBackup(Backup backup, boolean forced) { + VMInstanceVO vm = vmInstanceDao.findByIdIncludingRemoved(backup.getVmId()); + if (vm == null) { + throw new CloudRuntimeException(String.format("Could not find any VM associated with the Backup [uuid: %s, externalId: %s].", backup.getUuid(), backup.getExternalId())); + } + if (!forced) { + LOG.debug(String.format("Veeam backup provider does not have a safe way to remove a single restore point, which results in all backup chain being removed. " + + "More information about this limitation can be found in the links: [%s, %s].", "https://forums.veeam.com/powershell-f26/removing-a-single-restorepoint-t21061.html", + "https://helpcenter.veeam.com/docs/backup/vsphere/retention_separate_vms.html?ver=110")); + throw new CloudRuntimeException("Veeam backup provider does not have a safe way to remove a single restore point, which results in all backup chain being removed. " + + "Use forced:true to skip this verification and remove the complete backup chain."); + } + VeeamClient client = getClient(vm.getDataCenterId()); + boolean result = client.deleteBackup(backup.getExternalId()); + if (BooleanUtils.isFalse(result)) { + return false; + } + + List allBackups = backupDao.listByVmId(backup.getZoneId(), backup.getVmId()); + for (Backup b : allBackups) { + if (b.getId() != backup.getId()) { + backupDao.remove(b.getId()); + } + } + return result; } @Override diff --git a/plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/veeam/VeeamClient.java b/plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/veeam/VeeamClient.java index 2b80ca66540..39f3b5e44da 100644 --- a/plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/veeam/VeeamClient.java +++ b/plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/veeam/VeeamClient.java @@ -83,6 +83,7 @@ import org.apache.commons.lang3.StringUtils; public class VeeamClient { private static final Logger LOG = Logger.getLogger(VeeamClient.class); + private static final String FAILED_TO_DELETE = "Failed to delete"; private final URI apiURI; @@ -586,7 +587,7 @@ public class VeeamClient { String.format("$job = Get-VBRJob -Name \"%s\"", jobName), "if ($job) { Set-VBRJobSchedule -Job $job -Daily -At \"11:00\" -DailyKind Weekdays }" )); - return result.first() && !result.second().isEmpty() && !result.second().contains("Failed to delete"); + return result.first() && !result.second().isEmpty() && !result.second().contains(FAILED_TO_DELETE); } public boolean deleteJobAndBackup(final String jobName) { @@ -598,7 +599,22 @@ public class VeeamClient { "$repo = Get-VBRBackupRepository", "Sync-VBRBackupRepository -Repository $repo" )); - return result.first() && !result.second().contains("Failed to delete"); + return result.first() && !result.second().contains(FAILED_TO_DELETE); + } + + public boolean deleteBackup(final String restorePointId) { + LOG.debug(String.format("Trying to delete restore point [name: %s].", restorePointId)); + Pair result = executePowerShellCommands(Arrays.asList( + String.format("$restorePoint = Get-VBRRestorePoint ^| Where-Object { $_.Id -eq '%s' }", restorePointId), + "if ($restorePoint) { Remove-VBRRestorePoint -Oib $restorePoint -Confirm:$false", + "$repo = Get-VBRBackupRepository", + "Sync-VBRBackupRepository -Repository $repo", + "} else { ", + " Write-Output \"Failed to delete\"", + " Exit 1", + "}" + )); + return result.first() && !result.second().contains(FAILED_TO_DELETE); } public Map getBackupMetrics() { diff --git a/plugins/backup/veeam/src/test/java/org/apache/cloudstack/backup/VeeamBackupProviderTest.java b/plugins/backup/veeam/src/test/java/org/apache/cloudstack/backup/VeeamBackupProviderTest.java new file mode 100644 index 00000000000..57fdb2d66f5 --- /dev/null +++ b/plugins/backup/veeam/src/test/java/org/apache/cloudstack/backup/VeeamBackupProviderTest.java @@ -0,0 +1,133 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.cloudstack.backup; + +import static org.junit.Assert.assertEquals; + +import java.util.ArrayList; +import java.util.List; + +import org.apache.cloudstack.backup.dao.BackupDao; +import org.apache.cloudstack.backup.veeam.VeeamClient; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.mockito.junit.MockitoJUnitRunner; + +import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.vm.VMInstanceVO; +import com.cloud.vm.dao.VMInstanceDao; + +@RunWith(MockitoJUnitRunner.class) +public class VeeamBackupProviderTest { + @Spy + @InjectMocks + VeeamBackupProvider backupProvider = new VeeamBackupProvider(); + + @Mock + VeeamClient client; + + @Mock + VMInstanceDao vmInstanceDao; + + @Mock + BackupDao backupDao; + + @Test + public void deleteBackupTestExceptionWhenVmIsNull() { + BackupVO backup = new BackupVO(); + backup.setVmId(1l); + backup.setExternalId("abc"); + Mockito.when(vmInstanceDao.findByIdIncludingRemoved(Mockito.anyLong())).thenReturn(null); + try { + backupProvider.deleteBackup(backup, false); + } catch (Exception e) { + assertEquals(CloudRuntimeException.class, e.getClass()); + String expected = String.format("Could not find any VM associated with the Backup [uuid: %s, externalId: %s].", backup.getUuid(), "abc"); + assertEquals(expected , e.getMessage()); + } + } + + @Test + public void deleteBackupTestExceptionWhenForcedIsFalse() { + VMInstanceVO vmInstanceVO = new VMInstanceVO(); + BackupVO backup = new BackupVO(); + backup.setVmId(1l); + backup.setExternalId("abc"); + Mockito.when(vmInstanceDao.findByIdIncludingRemoved(Mockito.anyLong())).thenReturn(vmInstanceVO); + try { + backupProvider.deleteBackup(backup, false); + } catch (Exception e) { + assertEquals(CloudRuntimeException.class, e.getClass()); + String expected = "Veeam backup provider does not have a safe way to remove a single restore point, which results in all backup chain being removed. " + + "Use forced:true to skip this verification and remove the complete backup chain."; + assertEquals(expected , e.getMessage()); + } + } + + @Test + public void deleteBackupTestSuccessWhenForcedIsTrueAndHasJustOneBackup() { + VMInstanceVO vmInstanceVO = new VMInstanceVO(); + vmInstanceVO.setInstanceName("test"); + vmInstanceVO.setDataCenterId(2l); + BackupVO backup = new BackupVO(); + backup.setVmId(1l); + backup.setExternalId("abc"); + backup.setType("Full"); + backup.setZoneId(3l); + + Mockito.when(vmInstanceDao.findByIdIncludingRemoved(Mockito.anyLong())).thenReturn(vmInstanceVO); + Mockito.doReturn(client).when(backupProvider).getClient(2l); + Mockito.doReturn(true).when(client).deleteBackup("abc"); + List backups = new ArrayList<>(); + backups.add(backup); + Mockito.when(backupDao.listByVmId(3l, 1l)).thenReturn(backups); + Mockito.verify(backupDao, Mockito.never()).remove(Mockito.anyLong()); + boolean result = backupProvider.deleteBackup(backup, true); + assertEquals(true, result); + } + + @Test + public void deleteBackupTestSuccessWhenForcedIsTrueAndHasMoreThanOneBackup() { + VMInstanceVO vmInstanceVO = new VMInstanceVO(); + vmInstanceVO.setInstanceName("test"); + vmInstanceVO.setDataCenterId(2l); + BackupVO backup = Mockito.mock(BackupVO.class); + Mockito.when(backup.getId()).thenReturn(1l); + Mockito.when(backup.getVmId()).thenReturn(1l); + Mockito.when(backup.getExternalId()).thenReturn("abc"); + Mockito.when(backup.getZoneId()).thenReturn(3l); + + BackupVO backup2 = Mockito.mock(BackupVO.class); + Mockito.when(backup2.getId()).thenReturn(2l); + + Mockito.when(vmInstanceDao.findByIdIncludingRemoved(Mockito.anyLong())).thenReturn(vmInstanceVO); + Mockito.doReturn(client).when(backupProvider).getClient(2l); + Mockito.doReturn(true).when(client).deleteBackup("abc"); + List backups = new ArrayList<>(); + backups.add(backup); + backups.add(backup2); + Mockito.when(backupDao.listByVmId(3l, 1l)).thenReturn(backups); + boolean result = backupProvider.deleteBackup(backup, true); + Mockito.verify(backupDao, Mockito.times(1)).remove(2l); + assertEquals(true, result); + } +} \ No newline at end of file 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 3956fe40fb1..1978cca6870 100644 --- a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java @@ -26,7 +26,6 @@ import java.util.Map; import java.util.TimeZone; import java.util.Timer; import java.util.TimerTask; -import java.util.stream.Collectors; import javax.inject.Inject; import javax.naming.ConfigurationException; @@ -65,7 +64,6 @@ import org.apache.cloudstack.poll.BackgroundPollTask; import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils; -import org.apache.commons.collections.CollectionUtils; import org.apache.commons.lang3.BooleanUtils; import org.apache.commons.lang3.StringUtils; import org.apache.log4j.Logger; @@ -691,7 +689,7 @@ public class BackupManagerImpl extends ManagerBase implements BackupManager { @Override @ActionEvent(eventType = EventTypes.EVENT_VM_BACKUP_DELETE, eventDescription = "deleting VM backup", async = true) - public boolean deleteBackup(final Long backupId) { + public boolean deleteBackup(final Long backupId, final Boolean forced) { final BackupVO backup = backupDao.findByIdIncludingRemoved(backupId); if (backup == null) { throw new CloudRuntimeException("Backup " + backupId + " does not exist"); @@ -703,19 +701,12 @@ public class BackupManagerImpl extends ManagerBase implements BackupManager { } validateForZone(vm.getDataCenterId()); accountManager.checkAccess(CallContext.current().getCallingAccount(), null, true, vm); - final BackupOffering offering = backupOfferingDao.findByIdIncludingRemoved(vm.getBackupOfferingId()); + final BackupOffering offering = backupOfferingDao.findByIdIncludingRemoved(backup.getBackupOfferingId()); if (offering == null) { - throw new CloudRuntimeException("VM backup offering ID " + vm.getBackupOfferingId() + " does not exist"); - } - List backupsForVm = backupDao.listByVmId(vm.getDataCenterId(), vmId); - if (CollectionUtils.isNotEmpty(backupsForVm)) { - backupsForVm = backupsForVm.stream().filter(vmBackup -> vmBackup.getId() != backupId).collect(Collectors.toList()); - if (backupsForVm.size() <= 0 && vm.getRemoved() != null) { - removeVMFromBackupOffering(vmId, true); - } + throw new CloudRuntimeException(String.format("Backup offering with ID [%s] does not exist.", backup.getBackupOfferingId())); } final BackupProvider backupProvider = getBackupProvider(offering.getProvider()); - boolean result = backupProvider.deleteBackup(backup); + boolean result = backupProvider.deleteBackup(backup, forced); if (result) { return backupDao.remove(backup.getId()); }