Skip to content

Commit a7ba6a1

Browse files
SadiJrSadiJr
SadiJr
and
SadiJr
authored
[Veeam] Improve remove backup process (#6580)
* Allow delete backups but keep backup offering Co-authored-by: SadiJr <sadi@scclouds.com.br>
1 parent 7936eb0 commit a7ba6a1

File tree

8 files changed

+204
-24
lines changed

8 files changed

+204
-24
lines changed

‎api/src/main/java/org/apache/cloudstack/api/command/user/backup/DeleteBackupCmd.java‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import org.apache.cloudstack.api.response.SuccessResponse;
3232
import org.apache.cloudstack.backup.BackupManager;
3333
import org.apache.cloudstack.context.CallContext;
34+
import org.apache.commons.lang3.BooleanUtils;
3435

3536
import com.cloud.event.EventTypes;
3637
import com.cloud.exception.ConcurrentOperationException;
@@ -61,6 +62,13 @@ public class DeleteBackupCmd extends BaseAsyncCmd {
6162
description = "id of the VM backup")
6263
private Long backupId;
6364

65+
@Parameter(name = ApiConstants.FORCED,
66+
type = CommandType.BOOLEAN,
67+
required = false,
68+
description = "force the deletion of backup which removes the entire backup chain but keep VM in Backup Offering",
69+
since = "4.18.0.0")
70+
private Boolean forced;
71+
6472
/////////////////////////////////////////////////////
6573
/////////////////// Accessors ///////////////////////
6674
/////////////////////////////////////////////////////
@@ -69,14 +77,18 @@ public Long getId() {
6977
return backupId;
7078
}
7179

80+
public Boolean isForced() {
81+
return BooleanUtils.isTrue(forced);
82+
}
83+
7284
/////////////////////////////////////////////////////
7385
/////////////// API Implementation///////////////////
7486
/////////////////////////////////////////////////////
7587

7688
@Override
7789
public void execute() throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException, ConcurrentOperationException, ResourceAllocationException, NetworkRuleConflictException {
7890
try {
79-
boolean result = backupManager.deleteBackup(backupId);
91+
boolean result = backupManager.deleteBackup(getId(), isForced());
8092
if (result) {
8193
SuccessResponse response = new SuccessResponse(getCommandName());
8294
response.setResponseName(getCommandName());

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,9 +135,11 @@ public interface BackupManager extends BackupService, Configurable, PluggableSer
135135

136136
/**
137137
* Deletes a backup
138+
* @param backupId The Id of Backup to exclude
139+
* @param forced Indicates if backup will be force removed or not
138140
* @return returns operation success
139141
*/
140-
boolean deleteBackup(final Long backupId);
142+
boolean deleteBackup(final Long backupId, final Boolean forced);
141143

142144
BackupOffering updateBackupOffering(UpdateBackupOfferingCmd updateBackupOfferingCmd);
143145
}

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,10 +79,11 @@ public interface BackupProvider {
7979

8080
/**
8181
* Delete an existing backup
82-
* @param backup
82+
* @param backuo The backup to exclude
83+
* @param forced Indicates if backup will be force removed or not
8384
* @return
8485
*/
85-
boolean deleteBackup(Backup backup);
86+
boolean deleteBackup(Backup backup, boolean forced);
8687

8788
/**
8889
* Restore VM from backup

‎plugins/backup/dummy/src/main/java/org/apache/cloudstack/backup/DummyBackupProvider.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ public boolean takeBackup(VirtualMachine vm) {
129129
}
130130

131131
@Override
132-
public boolean deleteBackup(Backup backup) {
132+
public boolean deleteBackup(Backup backup, boolean forced) {
133133
return true;
134134
}
135135

‎plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/VeeamBackupProvider.java‎

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@
5353
import com.cloud.utils.exception.CloudRuntimeException;
5454
import com.cloud.vm.VMInstanceVO;
5555
import com.cloud.vm.VirtualMachine;
56+
import com.cloud.vm.dao.VMInstanceDao;
5657

5758
public class VeeamBackupProvider extends AdapterBase implements BackupProvider, Configurable {
5859

@@ -86,8 +87,10 @@ public class VeeamBackupProvider extends AdapterBase implements BackupProvider,
8687
private VmwareDatacenterDao vmwareDatacenterDao;
8788
@Inject
8889
private BackupDao backupDao;
90+
@Inject
91+
private VMInstanceDao vmInstanceDao;
8992

90-
private VeeamClient getClient(final Long zoneId) {
93+
protected VeeamClient getClient(final Long zoneId) {
9194
try {
9295
return new VeeamClient(VeeamUrl.valueIn(zoneId), VeeamUsername.valueIn(zoneId), VeeamPassword.valueIn(zoneId),
9396
VeeamValidateSSLSecurity.valueIn(zoneId), VeeamApiRequestTimeout.valueIn(zoneId), VeeamRestoreTimeout.valueIn(zoneId));
@@ -201,9 +204,31 @@ public boolean takeBackup(final VirtualMachine vm) {
201204
}
202205

203206
@Override
204-
public boolean deleteBackup(Backup backup) {
205-
// Veeam does not support removal of a restore point or point-in-time backup
206-
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");
207+
public boolean deleteBackup(Backup backup, boolean forced) {
208+
VMInstanceVO vm = vmInstanceDao.findByIdIncludingRemoved(backup.getVmId());
209+
if (vm == null) {
210+
throw new CloudRuntimeException(String.format("Could not find any VM associated with the Backup [uuid: %s, externalId: %s].", backup.getUuid(), backup.getExternalId()));
211+
}
212+
if (!forced) {
213+
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. "
214+
+ "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",
215+
"https://helpcenter.veeam.com/docs/backup/vsphere/retention_separate_vms.html?ver=110"));
216+
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. "
217+
+ "Use forced:true to skip this verification and remove the complete backup chain.");
218+
}
219+
VeeamClient client = getClient(vm.getDataCenterId());
220+
boolean result = client.deleteBackup(backup.getExternalId());
221+
if (BooleanUtils.isFalse(result)) {
222+
return false;
223+
}
224+
225+
List<Backup> allBackups = backupDao.listByVmId(backup.getZoneId(), backup.getVmId());
226+
for (Backup b : allBackups) {
227+
if (b.getId() != backup.getId()) {
228+
backupDao.remove(b.getId());
229+
}
230+
}
231+
return result;
207232
}
208233

209234
@Override

‎plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/veeam/VeeamClient.java‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@
8383

8484
public class VeeamClient {
8585
private static final Logger LOG = Logger.getLogger(VeeamClient.class);
86+
private static final String FAILED_TO_DELETE = "Failed to delete";
8687

8788
private final URI apiURI;
8889

@@ -586,7 +587,7 @@ public boolean setJobSchedule(final String jobName) {
586587
String.format("$job = Get-VBRJob -Name \"%s\"", jobName),
587588
"if ($job) { Set-VBRJobSchedule -Job $job -Daily -At \"11:00\" -DailyKind Weekdays }"
588589
));
589-
return result.first() && !result.second().isEmpty() && !result.second().contains("Failed to delete");
590+
return result.first() && !result.second().isEmpty() && !result.second().contains(FAILED_TO_DELETE);
590591
}
591592

592593
public boolean deleteJobAndBackup(final String jobName) {
@@ -598,7 +599,22 @@ public boolean deleteJobAndBackup(final String jobName) {
598599
"$repo = Get-VBRBackupRepository",
599600
"Sync-VBRBackupRepository -Repository $repo"
600601
));
601-
return result.first() && !result.second().contains("Failed to delete");
602+
return result.first() && !result.second().contains(FAILED_TO_DELETE);
603+
}
604+
605+
public boolean deleteBackup(final String restorePointId) {
606+
LOG.debug(String.format("Trying to delete restore point [name: %s].", restorePointId));
607+
Pair<Boolean, String> result = executePowerShellCommands(Arrays.asList(
608+
String.format("$restorePoint = Get-VBRRestorePoint ^| Where-Object { $_.Id -eq '%s' }", restorePointId),
609+
"if ($restorePoint) { Remove-VBRRestorePoint -Oib $restorePoint -Confirm:$false",
610+
"$repo = Get-VBRBackupRepository",
611+
"Sync-VBRBackupRepository -Repository $repo",
612+
"} else { ",
613+
" Write-Output \"Failed to delete\"",
614+
" Exit 1",
615+
"}"
616+
));
617+
return result.first() && !result.second().contains(FAILED_TO_DELETE);
602618
}
603619

604620
public Map<String, Backup.Metric> getBackupMetrics() {

‎plugins/backup/veeam/src/test/java/org/apache/cloudstack/backup/VeeamBackupProviderTest.java‎

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
18+
package org.apache.cloudstack.backup;
19+
20+
import static org.junit.Assert.assertEquals;
21+
22+
import java.util.ArrayList;
23+
import java.util.List;
24+
25+
import org.apache.cloudstack.backup.dao.BackupDao;
26+
import org.apache.cloudstack.backup.veeam.VeeamClient;
27+
import org.junit.Test;
28+
import org.junit.runner.RunWith;
29+
import org.mockito.InjectMocks;
30+
import org.mockito.Mock;
31+
import org.mockito.Mockito;
32+
import org.mockito.Spy;
33+
import org.mockito.junit.MockitoJUnitRunner;
34+
35+
import com.cloud.utils.exception.CloudRuntimeException;
36+
import com.cloud.vm.VMInstanceVO;
37+
import com.cloud.vm.dao.VMInstanceDao;
38+
39+
@RunWith(MockitoJUnitRunner.class)
40+
public class VeeamBackupProviderTest {
41+
@Spy
42+
@InjectMocks
43+
VeeamBackupProvider backupProvider = new VeeamBackupProvider();
44+
45+
@Mock
46+
VeeamClient client;
47+
48+
@Mock
49+
VMInstanceDao vmInstanceDao;
50+
51+
@Mock
52+
BackupDao backupDao;
53+
54+
@Test
55+
public void deleteBackupTestExceptionWhenVmIsNull() {
56+
BackupVO backup = new BackupVO();
57+
backup.setVmId(1l);
58+
backup.setExternalId("abc");
59+
Mockito.when(vmInstanceDao.findByIdIncludingRemoved(Mockito.anyLong())).thenReturn(null);
60+
try {
61+
backupProvider.deleteBackup(backup, false);
62+
} catch (Exception e) {
63+
assertEquals(CloudRuntimeException.class, e.getClass());
64+
String expected = String.format("Could not find any VM associated with the Backup [uuid: %s, externalId: %s].", backup.getUuid(), "abc");
65+
assertEquals(expected , e.getMessage());
66+
}
67+
}
68+
69+
@Test
70+
public void deleteBackupTestExceptionWhenForcedIsFalse() {
71+
VMInstanceVO vmInstanceVO = new VMInstanceVO();
72+
BackupVO backup = new BackupVO();
73+
backup.setVmId(1l);
74+
backup.setExternalId("abc");
75+
Mockito.when(vmInstanceDao.findByIdIncludingRemoved(Mockito.anyLong())).thenReturn(vmInstanceVO);
76+
try {
77+
backupProvider.deleteBackup(backup, false);
78+
} catch (Exception e) {
79+
assertEquals(CloudRuntimeException.class, e.getClass());
80+
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. "
81+
+ "Use forced:true to skip this verification and remove the complete backup chain.";
82+
assertEquals(expected , e.getMessage());
83+
}
84+
}
85+
86+
@Test
87+
public void deleteBackupTestSuccessWhenForcedIsTrueAndHasJustOneBackup() {
88+
VMInstanceVO vmInstanceVO = new VMInstanceVO();
89+
vmInstanceVO.setInstanceName("test");
90+
vmInstanceVO.setDataCenterId(2l);
91+
BackupVO backup = new BackupVO();
92+
backup.setVmId(1l);
93+
backup.setExternalId("abc");
94+
backup.setType("Full");
95+
backup.setZoneId(3l);
96+
97+
Mockito.when(vmInstanceDao.findByIdIncludingRemoved(Mockito.anyLong())).thenReturn(vmInstanceVO);
98+
Mockito.doReturn(client).when(backupProvider).getClient(2l);
99+
Mockito.doReturn(true).when(client).deleteBackup("abc");
100+
List<Backup> backups = new ArrayList<>();
101+
backups.add(backup);
102+
Mockito.when(backupDao.listByVmId(3l, 1l)).thenReturn(backups);
103+
Mockito.verify(backupDao, Mockito.never()).remove(Mockito.anyLong());
104+
boolean result = backupProvider.deleteBackup(backup, true);
105+
assertEquals(true, result);
106+
}
107+
108+
@Test
109+
public void deleteBackupTestSuccessWhenForcedIsTrueAndHasMoreThanOneBackup() {
110+
VMInstanceVO vmInstanceVO = new VMInstanceVO();
111+
vmInstanceVO.setInstanceName("test");
112+
vmInstanceVO.setDataCenterId(2l);
113+
BackupVO backup = Mockito.mock(BackupVO.class);
114+
Mockito.when(backup.getId()).thenReturn(1l);
115+
Mockito.when(backup.getVmId()).thenReturn(1l);
116+
Mockito.when(backup.getExternalId()).thenReturn("abc");
117+
Mockito.when(backup.getZoneId()).thenReturn(3l);
118+
119+
BackupVO backup2 = Mockito.mock(BackupVO.class);
120+
Mockito.when(backup2.getId()).thenReturn(2l);
121+
122+
Mockito.when(vmInstanceDao.findByIdIncludingRemoved(Mockito.anyLong())).thenReturn(vmInstanceVO);
123+
Mockito.doReturn(client).when(backupProvider).getClient(2l);
124+
Mockito.doReturn(true).when(client).deleteBackup("abc");
125+
List<Backup> backups = new ArrayList<>();
126+
backups.add(backup);
127+
backups.add(backup2);
128+
Mockito.when(backupDao.listByVmId(3l, 1l)).thenReturn(backups);
129+
boolean result = backupProvider.deleteBackup(backup, true);
130+
Mockito.verify(backupDao, Mockito.times(1)).remove(2l);
131+
assertEquals(true, result);
132+
}
133+
}

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

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
import java.util.TimeZone;
2727
import java.util.Timer;
2828
import java.util.TimerTask;
29-
import java.util.stream.Collectors;
3029

3130
import javax.inject.Inject;
3231
import javax.naming.ConfigurationException;
@@ -65,7 +64,6 @@
6564
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
6665
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
6766
import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils;
68-
import org.apache.commons.collections.CollectionUtils;
6967
import org.apache.commons.lang3.BooleanUtils;
7068
import org.apache.commons.lang3.StringUtils;
7169
import org.apache.log4j.Logger;
@@ -691,7 +689,7 @@ protected Pair<Boolean, String> restoreBackedUpVolume(final String backedUpVolum
691689

692690
@Override
693691
@ActionEvent(eventType = EventTypes.EVENT_VM_BACKUP_DELETE, eventDescription = "deleting VM backup", async = true)
694-
public boolean deleteBackup(final Long backupId) {
692+
public boolean deleteBackup(final Long backupId, final Boolean forced) {
695693
final BackupVO backup = backupDao.findByIdIncludingRemoved(backupId);
696694
if (backup == null) {
697695
throw new CloudRuntimeException("Backup " + backupId + " does not exist");
@@ -703,19 +701,12 @@ public boolean deleteBackup(final Long backupId) {
703701
}
704702
validateForZone(vm.getDataCenterId());
705703
accountManager.checkAccess(CallContext.current().getCallingAccount(), null, true, vm);
706-
final BackupOffering offering = backupOfferingDao.findByIdIncludingRemoved(vm.getBackupOfferingId());
704+
final BackupOffering offering = backupOfferingDao.findByIdIncludingRemoved(backup.getBackupOfferingId());
707705
if (offering == null) {
708-
throw new CloudRuntimeException("VM backup offering ID " + vm.getBackupOfferingId() + " does not exist");
709-
}
710-
List<Backup> backupsForVm = backupDao.listByVmId(vm.getDataCenterId(), vmId);
711-
if (CollectionUtils.isNotEmpty(backupsForVm)) {
712-
backupsForVm = backupsForVm.stream().filter(vmBackup -> vmBackup.getId() != backupId).collect(Collectors.toList());
713-
if (backupsForVm.size() <= 0 && vm.getRemoved() != null) {
714-
removeVMFromBackupOffering(vmId, true);
715-
}
706+
throw new CloudRuntimeException(String.format("Backup offering with ID [%s] does not exist.", backup.getBackupOfferingId()));
716707
}
717708
final BackupProvider backupProvider = getBackupProvider(offering.getProvider());
718-
boolean result = backupProvider.deleteBackup(backup);
709+
boolean result = backupProvider.deleteBackup(backup, forced);
719710
if (result) {
720711
return backupDao.remove(backup.getId());
721712
}

0 commit comments

Comments
 (0)