[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6580: [Veeam] Improve remove backup process

[GitBox <gi...@apache.org>]

DaanHoogland commented on code in PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#discussion_r936719762


##########
plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/VeeamBackupProvider.java:
##########
@@ -201,9 +204,29 @@ public boolean takeBackup(final VirtualMachine vm) {
     }
 
     @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 (result) {
+            List<Backup> allBackups = backupDao.listByVmId(backup.getZoneId(), backup.getVmId());
+            for (Backup b : allBackups) {
+                if (b.getId() != backup.getId()) {
+                    backupDao.remove(b.getId());
+                }
+            }
+        }
+        return result;

Review Comment:
   I´d rather not. Rather I'd have the code simplified by extracting the validations at the start of the method. and extracting
   ```
               List<Backup> allBackups = backupDao.listByVmId(backup.getZoneId(), backup.getVmId());
               for (Backup b : allBackups) {
                   if (b.getId() != backup.getId()) {
                       backupDao.remove(b.getId());
                   }
               }
   ```
   and making it
   ```suggestion
           if (result) {
               <extracted method call here>();
           }
           return result;
   ```
   
   early extra return statements are not improving readability of code.



-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org