resolve merge problems in the backup framework

[DaanHoogland]

Description

This PR solves merge problems in the backup framework and providers.

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)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)
• build/CI
• test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 27 lines in your changes missing coverage. Please review.

Project coverage is 16.16%. Comparing base (24b7c66) to head (6da5db5).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...che/cloudstack/backup/NetworkerBackupProvider.java	0.00%	19 Missing ⚠️
...rg/apache/cloudstack/backup/NASBackupProvider.java	0.00%	4 Missing ⚠️
.../apache/cloudstack/backup/DummyBackupProvider.java	0.00%	3 Missing ⚠️
.../apache/cloudstack/backup/VeeamBackupProvider.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##              main   #10457       +/-   ##
============================================
+ Coverage     4.00%   16.16%   +12.15%     
- Complexity       0    13269    +13269     
============================================
  Files          396     5666     +5270     
  Lines        32530   498051   +465521     
  Branches      5766    60259    +54493     
============================================
+ Hits          1302    80489    +79187     
- Misses       31078   408554   +377476     
- Partials       150     9008     +8858     

Flag	Coverage Δ
uitests	4.00% <ø> (ø)
unittests	17.01% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[weizhouapache]

@DaanHoogland
the title says the PR aims to fix the merge problems, but more than 90% of this PR are related to logging

my suggestion is, revert VeeamBackupProvider.java and then cherry-pick the commit #9898

git checkout c80b8860e49c61252588c8cf8f40277dcf2c4ee8 plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/VeeamBackupProvider.java
git cherry-pick 21b5e4dcae5b3cb73637fc82410bd8e4fd55e99c

[DaanHoogland]

@DaanHoogland the title says the PR aims to fix the merge problems, but more than 90% of this PR are related to logging

yes, you are right, but these are rather safe.

my suggestion is, revert VeeamBackupProvider.java and then cherry-pick the commit #9898

git checkout c80b8860e49c61252588c8cf8f40277dcf2c4ee8 plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/VeeamBackupProvider.java
git cherry-pick 21b5e4dcae5b3cb73637fc82410bd8e4fd55e99c

What you suggest will not lead to a compiling version without further fixes either, neither starting on main or starting from this branch.

The question is if https://github.com/apache/cloudstack/pull/10457/files#diff-35507d9957e6b1b12096c22153fc877ea347a7dd2316a0c670a0c80a6c3da3a4R330-R405 does the job.

[weizhouapache]

@DaanHoogland the title says the PR aims to fix the merge problems, but more than 90% of this PR are related to logging

yes, you are right, but these are rather safe.

my suggestion is, revert VeeamBackupProvider.java and then cherry-pick the commit #9898

git checkout c80b8860e49c61252588c8cf8f40277dcf2c4ee8 plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/VeeamBackupProvider.java
git cherry-pick 21b5e4dcae5b3cb73637fc82410bd8e4fd55e99c

What you suggest will not lead to a compiling version without further fixes either, neither starting on main or starting from this branch.

The question is if https://github.com/apache/cloudstack/pull/10457/files#diff-35507d9957e6b1b12096c22153fc877ea347a7dd2316a0c670a0c80a6c3da3a4R330-R405 does the job.

oh, sorry, the commit I pasted was wrong, the correct commit is a8b18a5 which is the last commit before merge forward.

git checkout c80b8860e49c61252588c8cf8f40277dcf2c4ee8 plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/VeeamBackupProvider.java
wget https://github.com/apache/cloudstack/pull/9898.patch
patch -p1 <9898.patch
# fix conflicts

I tried locally, the difference is

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 a7ce0c09cc6..c6b93568828 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
@@ -334,6 +334,7 @@ public class VeeamBackupProvider extends AdapterBase implements BackupProvider,
         backup.setAccountId(vm.getAccountId());
         backup.setDomainId(vm.getDomainId());
         backup.setZoneId(vm.getDataCenterId());
+        backup.setBackedUpVolumes(BackupManagerImpl.createVolumeInfoFromVolumes(volumeDao.findByInstance(vm.getId())));
         backupDao.persist(backup);
         return backup;
     }
@@ -344,59 +345,6 @@ public class VeeamBackupProvider extends AdapterBase implements BackupProvider,
         return getClient(vm.getDataCenterId()).listRestorePoints(backupName, vm.getInstanceName());
     }
 
-    @Override
-    public void syncBackups(VirtualMachine vm, Backup.Metric metric) {
-        List<Backup.RestorePoint> restorePoints = listRestorePoints(vm);
-        if (CollectionUtils.isEmpty(restorePoints)) {
-            logger.debug("Can't find any restore point to VM: {}", vm);
-            return;
-        }
-        Transaction.execute(new TransactionCallbackNoReturn() {
-            @Override
-            public void doInTransactionWithoutResult(TransactionStatus status) {
-                final List<Backup> backupsInDb = backupDao.listByVmId(null, vm.getId());
-                final List<Long> removeList = backupsInDb.stream().map(InternalIdentity::getId).collect(Collectors.toList());
-                for (final Backup.RestorePoint restorePoint : restorePoints) {
-                    if (!(restorePoint.getId() == null || restorePoint.getType() == null || restorePoint.getCreated() == null)) {
-                        Backup existingBackupEntry = checkAndUpdateIfBackupEntryExistsForRestorePoint(backupsInDb, restorePoint, metric);
-                        if (existingBackupEntry != null) {
-                            removeList.remove(existingBackupEntry.getId());
-                            continue;
-                        }
-
-                        BackupVO backup = new BackupVO();
-                        backup.setVmId(vm.getId());
-                        backup.setExternalId(restorePoint.getId());
-                        backup.setType(restorePoint.getType());
-                        backup.setDate(restorePoint.getCreated());
-                        backup.setStatus(Backup.Status.BackedUp);
-                        if (metric != null) {
-                            backup.setSize(metric.getBackupSize());
-                            backup.setProtectedSize(metric.getDataSize());
-                        }
-                        backup.setBackupOfferingId(vm.getBackupOfferingId());
-                        backup.setAccountId(vm.getAccountId());
-                        backup.setDomainId(vm.getDomainId());
-                        backup.setZoneId(vm.getDataCenterId());
-                        backup.setBackedUpVolumes(BackupManagerImpl.createVolumeInfoFromVolumes(volumeDao.findByInstance(vm.getId())));
-
-                        logger.debug("Creating a new entry in backups: [id: {}, uuid: {}, name: {}, vm_id: {}, external_id: {}, type: {}, date: {}, backup_offering_id: {}, account_id: {}, "
-                                + "domain_id: {}, zone_id: {}].", backup.getId(), backup.getUuid(), backup.getName(), backup.getVmId(), backup.getExternalId(), backup.getType(), backup.getDate(), backup.getBackupOfferingId(), backup.getAccountId(), backup.getDomainId(), backup.getZoneId());
-                        backupDao.persist(backup);
-
-                        ActionEventUtils.onCompletedActionEvent(User.UID_SYSTEM, vm.getAccountId(), EventVO.LEVEL_INFO, EventTypes.EVENT_VM_BACKUP_CREATE,
-                                String.format("Created backup %s for VM ID: %s", backup.getUuid(), vm.getUuid()),
-                                vm.getId(), ApiCommandResourceType.VirtualMachine.toString(),0);
-                    }
-                }
-                for (final Long backupIdToRemove : removeList) {
-                    logger.warn(String.format("Removing backup with ID: [%s].", backupIdToRemove));
-                    backupDao.remove(backupIdToRemove);
-                }
-            }
-        });
-    }
-

[DaanHoogland]

@winterhazel @weizhouapache , is it correct that we can drop the whole transactional bit in as in org.apache.cloudstack.backup.VeeamBackupProvider#syncBackups?

pushed changes as @weizhouapache suggested.

[winterhazel]

@winterhazel @weizhouapache , is it correct that we can drop the whole transactional bit in as in org.apache.cloudstack.backup.VeeamBackupProvider#syncBackups?

Correct, the transaction was moved to org.apache.cloudstack.backup.BackupManagerImpl.BackupSyncTask#syncBackups

[winterhazel]

Code looks ok

[weizhouapache]

code lgtm

[sonarqubecloud]

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 40%)

See analysis details on SonarQube Cloud

[DaanHoogland]

thanks guys, As this is a merge forward, I am not sure if we should do a full regression suite on this, especially since the major changes require 3rd party software. What do you think? (packaging is underway)

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12561

[DaanHoogland]

@blueorangutan test

[blueorangutan]

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[abh1sar]

code LGTM

[abh1sar]

syncBackups has been moved to BackupManagerImpl. Need to remove syncBackups from all providers.

[abh1sar]

this method has also been moved along with syncBackups to BackupManagerImpl. can be removed from here

[abh1sar]

Please restore the code in createNewBackupEntryForRestorePoint and listRestorePoints, and delete code from syncBackups

[winterhazel]

thanks guys, As this is a merge forward, I am not sure if we should do a full regression suite on this, especially since the major changes require 3rd party software. What do you think? (packaging is underway)

Up to you guys, but I think we should we should be good already, as the tests that involve the only change here that could affect something already passed on #10017.

[blueorangutan]

[SF] Trillian test result (tid-12491)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 61098 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10457-t12491-kvm-ol8.zip
Smoke tests completed. 140 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_06_purge_expunged_vm_background_task	Failure	407.26	test_purge_expunged_vms.py