You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2022/07/28 19:06:48 UTC

[GitHub] [cloudstack] SadiJr opened a new pull request, #6580: [Veeam] Improve remove backup process

SadiJr opened a new pull request, #6580:
URL: https://github.com/apache/cloudstack/pull/6580

   ### Description
   
   Using the VMWare hypervisor, with the Veeam plugin active, it is not possible to remove backups. Instead, an exception is thrown in ACS, and the only procedure that allows the removal of backups is to remove the VM from the Backup Offering, which causes all backups for that VM to be removed.
   
   Through Veeam's documentation and user forum, it was discovered that it does not allow the removal of specific restore points. Instead, the entire backup chain is removed, leaving only the job in Veeam.
   
   To address this limitation, a new parameter has been added to the `deleteBackup` API to allow users to remove the complete Veeam backup chain. So, when removing a backup, users can pass the `forced` parameter as `true`, to remove all the backup chain, but keeping the VM in the Backup Offering. Otherwise, an exception will be thrown notifying the user of this Veeam limitation and the `forced` option.
   
   ### Types of changes
   
   - [ ] Breaking change (fix or feature that would cause existing functionality to change)
   - [ ] New feature (non-breaking change which adds functionality)
   - [ ] Bug fix (non-breaking change which fixes an issue)
   - [x] Enhancement (improves an existing feature and functionality)
   - [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
   
   ### Feature/Enhancement Scale or Bug Severity
   
   #### Feature/Enhancement Scale
   
   - [ ] Major
   - [x] Minor
   
   ### How Has This Been Tested?
   It was tested in a local lab:
   
   1. I created a new VM and attached this VM to a Backup Offering;
   2. I performed two manual backups;
   3. I attempted (using CLI) to delete one of these backups;
   4. Before, an exception was thrown, forcing the user to remove the VM from Backup Offering;
   5. Now, users can use `forced` option to remove the entire backup chain but keep VM in Backup Offering, plus the error message has been improved.
   Also, I added new unit tests.


-- 
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


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

Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on code in PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#discussion_r936618978


##########
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:
   We could invert the `if` to reduce indentation.



-- 
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


[GitHub] [cloudstack] blueorangutan commented on pull request #6580: [Veeam] Improve remove backup process

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#issuecomment-1204039686

   UI build: :heavy_check_mark:
   Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6580 (SL-JID-2072)


-- 
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


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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#issuecomment-1243513126

   @blueorangutan test centos7 vmware-67u3


-- 
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


[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #6580: [Veeam] Improve remove backup process

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#issuecomment-1198594379

   SonarCloud Quality Gate failed.&nbsp; &nbsp; [![Quality Gate failed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/failed-16px.png 'Quality Gate failed')](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6580)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6580&resolved=false&types=SECURITY_HOTSPOT) [![E](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/E-16px.png 'E')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6580&resolved=false&types=SECURITY_HOTSPOT) [1 Security Hotspot](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6580&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=CODE_SMELL) [8 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=CODE_SMELL)
   
   [![51.3%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/50-16px.png '51.3%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6580&metric=new_coverage&view=list) [51.3% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6580&metric=new_coverage&view=list)  
   [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6580&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6580&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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


[GitHub] [cloudstack] blueorangutan commented on pull request #6580: [Veeam] Improve remove backup process

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#issuecomment-1245890021

   <b>Trillian test result (tid-4878)</b>
   Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 43139 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6580-t4878-vmware-67u3.zip
   Smoke tests completed. 101 look OK, 0 have errors
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   


-- 
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


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

Posted by GitBox <gi...@apache.org>.
SadiJr commented on code in PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#discussion_r936741487


##########
api/src/main/java/org/apache/cloudstack/api/command/user/backup/DeleteBackupCmd.java:
##########
@@ -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",

Review Comment:
   Done, thanks.



##########
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:
   Done, thanks.



-- 
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


[GitHub] [cloudstack] GutoVeronezi commented on pull request #6580: [Veeam] Improve remove backup process

Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#issuecomment-1240982155

   @DaanHoogland, can we run the tests for this one?
   cc: @harikrishna-patnala 
   
   
   @blueorangutan package


-- 
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


[GitHub] [cloudstack] blueorangutan commented on pull request #6580: [Veeam] Improve remove backup process

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#issuecomment-1241729696

   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 4150


-- 
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


[GitHub] [cloudstack] github-actions[bot] commented on pull request #6580: [Veeam] Improve remove backup process

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#issuecomment-1202439868

   This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.


-- 
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


[GitHub] [cloudstack] DaanHoogland merged pull request #6580: [Veeam] Improve remove backup process

Posted by GitBox <gi...@apache.org>.
DaanHoogland merged PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580


-- 
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


[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #6580: [Veeam] Improve remove backup process

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#issuecomment-1204118766

   SonarCloud Quality Gate failed.&nbsp; &nbsp; [![Quality Gate failed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/failed-16px.png 'Quality Gate failed')](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6580)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6580&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6580&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6580&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=CODE_SMELL)
   
   [![43.2%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/40-16px.png '43.2%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6580&metric=new_coverage&view=list) [43.2% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6580&metric=new_coverage&view=list)  
   [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6580&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6580&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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


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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#issuecomment-1202077804

   clgtm, but can you look at the code smells reported by sonarcloud please @SadiJr ?
   
   > SonarCloud Quality Gate failed.    [![Quality Gate failed](https://camo.githubusercontent.com/121da9aaaee76c9a59af5d57beb6400e9b2974722e1f71a33c0b4dc445b55041/68747470733a2f2f736f6e6172736f757263652e6769746875622e696f2f736f6e6172636c6f75642d6769746875622d7374617469632d7265736f75726365732f76322f636865636b732f5175616c6974794761746542616467652f6661696c65642e737667)](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6580)
   > 
   > [![Bug](https://camo.githubusercontent.com/575c33053884b5ef49671786eee7be86afa65d53d1154a8109acf87f7970e71d/68747470733a2f2f736f6e6172736f757263652e6769746875622e696f2f736f6e6172636c6f75642d6769746875622d7374617469632d7265736f75726365732f76322f636f6d6d6f6e2f6275672e737667)](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=BUG) [![A](https://camo.githubusercontent.com/36a358eb301bdb2c21a371c14a76fe760cc9fa629c65d6c8f2a5e0a022019286/68747470733a2f2f736f6e6172736f757263652e6769746875622e696f2f736f6e6172636c6f75642d6769746875622d7374617469632d7265736f75726365732f76322f636865636b732f526174696e6742616467652f412e737667)](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=BUG) [![Vulnerability](https://camo.githubusercontent.com/5d2e7c4abe6651f9de1cfa749a41019b00550ca1fe7f339d25706c542d63
 71e3/68747470733a2f2f736f6e6172736f757263652e6769746875622e696f2f736f6e6172636c6f75642d6769746875622d7374617469632d7265736f75726365732f76322f636f6d6d6f6e2f76756c6e65726162696c6974792e737667)](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=VULNERABILITY) [![A](https://camo.githubusercontent.com/36a358eb301bdb2c21a371c14a76fe760cc9fa629c65d6c8f2a5e0a022019286/68747470733a2f2f736f6e6172736f757263652e6769746875622e696f2f736f6e6172636c6f75642d6769746875622d7374617469632d7265736f75726365732f76322f636865636b732f526174696e6742616467652f412e737667)](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=VULNERABILITY) [![Security Hotspot](https://camo.githubusercontent.com/df4c9bd73bfbbf055a8768ddc07836a345c6d52a36594d1a57b67d03b03d2430/68747470733a2f2f736f6e6172736f75726365
 2e6769746875622e696f2f736f6e6172636c6f75642d6769746875622d7374617469632d7265736f75726365732f76322f636f6d6d6f6e2f73656375726974795f686f7473706f742e737667)](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6580&resolved=false&types=SECURITY_HOTSPOT) [![A](https://camo.githubusercontent.com/36a358eb301bdb2c21a371c14a76fe760cc9fa629c65d6c8f2a5e0a022019286/68747470733a2f2f736f6e6172736f757263652e6769746875622e696f2f736f6e6172636c6f75642d6769746875622d7374617469632d7265736f75726365732f76322f636865636b732f526174696e6742616467652f412e737667)](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6580&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6580&resolved=false&types=SECURITY_HOTSPOT) [![Code Smell](https://camo.githubusercontent.com/3aa5d5d2ca02856fc823191235ea45389dd267369193d110fa2dd216ad7dc214/68747470733a2f2f736f6e6172736f7572636
 52e6769746875622e696f2f736f6e6172636c6f75642d6769746875622d7374617469632d7265736f75726365732f76322f636f6d6d6f6e2f636f64655f736d656c6c2e737667)](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=CODE_SMELL) [![A](https://camo.githubusercontent.com/36a358eb301bdb2c21a371c14a76fe760cc9fa629c65d6c8f2a5e0a022019286/68747470733a2f2f736f6e6172736f757263652e6769746875622e696f2f736f6e6172636c6f75642d6769746875622d7374617469632d7265736f75726365732f76322f636865636b732f526174696e6742616467652f412e737667)](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=CODE_SMELL) [8 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6580&resolved=false&types=CODE_SMELL)
   > 
   > [![51.3%](https://camo.githubusercontent.com/3635951d71c91b0574101f5394bc32e21a4cee7e1590cd516dc3164e0dd45731/68747470733a2f2f736f6e6172736f757263652e6769746875622e696f2f736f6e6172636c6f75642d6769746875622d7374617469632d7265736f75726365732f76322f636865636b732f436f76657261676543686172742f35302e737667)](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6580&metric=new_coverage&view=list) [51.3% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6580&metric=new_coverage&view=list) [![0.0%](https://camo.githubusercontent.com/79462a906ea67fccf40f754ae817836e77820693abc6a4576361a3c9f48f3468/68747470733a2f2f736f6e6172736f757263652e6769746875622e696f2f736f6e6172636c6f75642d6769746875622d7374617469632d7265736f75726365732f76322f636865636b732f4475706c69636174696f6e732f332e737667)](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6580&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://so
 narcloud.io/component_measures?id=apache_cloudstack&pullRequest=6580&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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


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

Posted by 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


[GitHub] [cloudstack] blueorangutan commented on pull request #6580: [Veeam] Improve remove backup process

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#issuecomment-1204032361

   @acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.


-- 
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


[GitHub] [cloudstack] blueorangutan commented on pull request #6580: [Veeam] Improve remove backup process

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#issuecomment-1245019634

   @DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests


-- 
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


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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#issuecomment-1246338578

   Did any Veeam user or otherwise user of the backup framework test this (other than the author)?
   Otherwise it is ready for merge.


-- 
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


[GitHub] [cloudstack] blueorangutan commented on pull request #6580: [Veeam] Improve remove backup process

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#issuecomment-1240983140

   @GutoVeronezi a Jenkins job has been kicked to build packages. It will be bundled with  KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.


-- 
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


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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#issuecomment-1245018852

   let's try insanity;
   @blueorangutan test centos7 vmware-67u3


-- 
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


[GitHub] [cloudstack] SadiJr commented on pull request #6580: [Veeam] Improve remove backup process

Posted by GitBox <gi...@apache.org>.
SadiJr commented on PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#issuecomment-1203195118

   @DaanHoogland thanks for your suggestion. I will take a look about this code smells :).


-- 
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


[GitHub] [cloudstack] acs-robot commented on pull request #6580: [Veeam] Improve remove backup process

Posted by GitBox <gi...@apache.org>.
acs-robot commented on PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#issuecomment-1204031652

   Found UI changes, kicking a new UI QA build
   @blueorangutan ui


-- 
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


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

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on code in PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#discussion_r936238728


##########
api/src/main/java/org/apache/cloudstack/api/command/user/backup/DeleteBackupCmd.java:
##########
@@ -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",

Review Comment:
   Can you please change the description to something like "force the deletion of backup which removes the entire backup chain but keep VM in Backup Offering"? Otherwise, the force parameter looks to me like "force delete just this backup in any case".



-- 
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


[GitHub] [cloudstack] blueorangutan commented on pull request #6580: [Veeam] Improve remove backup process

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#issuecomment-1244482901

   <b>Trillian test result (tid-4872)</b>
   Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 38609 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6580-t4872-vmware-67u3.zip
   Smoke tests completed. 98 look OK, 3 have errors
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_DeployVmAntiAffinityGroup | `Error` | 42.66 | test_affinity_groups.py
   test_DeployVmAntiAffinityGroup_in_project | `Error` | 97.21 | test_affinity_groups_projects.py
   test_03_deploy_and_scale_kubernetes_cluster | `Failure` | 35.95 | test_kubernetes_clusters.py
   test_07_deploy_kubernetes_ha_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
   test_08_upgrade_kubernetes_ha_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
   test_09_delete_kubernetes_ha_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
   


-- 
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


[GitHub] [cloudstack] blueorangutan commented on pull request #6580: [Veeam] Improve remove backup process

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#issuecomment-1243469968

   @DaanHoogland unsupported parameters provided. Supported mgmt server os are: `centos7, centos6, suse15, alma8, ubuntu18, ubuntu22, ubuntu20, rocky8`. Supported hypervisors are: `kvm-centos6, kvm-centos7, kvm-rocky8, kvm-alma8, kvm-ubuntu18, kvm-ubuntu20, kvm-ubuntu22, kvm-suse15, vmware-55u3, vmware-60u2, vmware-65u2, vmware-67u3, vmware-70u1, vmware-70u2, vmware-70u3, xenserver-65sp1, xenserver-71, xenserver-74, xcpng74, xcpng76, xcpng80, xcpng81, xcpng82`


-- 
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


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

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#issuecomment-1243468665

   @blueorangutan test centos7 vmware-67


-- 
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


[GitHub] [cloudstack] blueorangutan commented on pull request #6580: [Veeam] Improve remove backup process

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6580:
URL: https://github.com/apache/cloudstack/pull/6580#issuecomment-1243513522

   @DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests


-- 
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