resolve merge problems in the backup framework #10457

DaanHoogland · 2025-02-24T19:53:26Z

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

Screenshots (if appropriate):

How Has This Been Tested?

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

codecov · 2025-02-24T19:59:51Z

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.

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

weizhouapache · 2025-02-25T09:00:49Z

@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 · 2025-02-25T09:31:59Z

@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 · 2025-02-25T11:38:03Z

@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);
-                }
-            }
-        });
-    }
-

…p/VeeamBackupProvider.java

DaanHoogland · 2025-02-25T12:53:10Z

@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 · 2025-02-25T13:05:39Z

@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 · 2025-02-25T14:04:51Z

Quality Gate failed

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

See analysis details on SonarQube Cloud

DaanHoogland · 2025-02-25T14:17:36Z

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 · 2025-02-25T14:53:57Z

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

DaanHoogland · 2025-02-25T15:01:50Z

@blueorangutan test

blueorangutan · 2025-02-25T15:14:03Z

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

abh1sar

code LGTM

abh1sar · 2025-02-25T08:08:29Z

plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java

@@ -414,6 +407,11 @@ public boolean willDeleteBackupsOnOfferingRemoval() {
        return false;
    }

+    @Override
+    public void syncBackups(VirtualMachine vm, Backup.Metric metric) {


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

abh1sar · 2025-02-25T08:10:26Z

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

@@ -344,6 +352,23 @@ public List<Backup.RestorePoint> listRestorePoints(VirtualMachine vm) {
        return getClient(vm.getDataCenterId()).listRestorePoints(backupName, vm.getInstanceName());
    }

+    private Backup checkAndUpdateIfBackupEntryExistsForRestorePoint(List<Backup> backupsInDb, Backup.RestorePoint restorePoint, Backup.Metric metric) {


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

abh1sar · 2025-02-25T09:01:06Z

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

@@ -328,54 +328,12 @@ public Map<VirtualMachine, Backup.Metric> getBackupMetrics(final Long zoneId, fi

    @Override


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

winterhazel · 2025-02-25T18:04:18Z

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 · 2025-02-26T08:55:18Z

[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

resolve merge problems in the backup framework

aeb0710

boring-cyborg bot added component:api component:backup labels Feb 24, 2025

some cleanups while waiting for compile

f50dbd0

DaanHoogland mentioned this pull request Feb 24, 2025

[VMware] Disk controller mappings #10454

Open

9 tasks

DaanHoogland requested review from abh1sar and winterhazel February 24, 2025 20:11

winterhazel requested changes Feb 24, 2025

View reviewed changes

api/src/main/java/org/apache/cloudstack/backup/BackupProvider.java Outdated Show resolved Hide resolved

re-remove sync method (and do some warning cleanups)

daeb86c

DaanHoogland requested a review from winterhazel February 25, 2025 08:45

DaanHoogland and others added 2 commits February 25, 2025 13:30

revert plugins/backup/veeam/src/main/java/org/apache/cloudstack/backu…

2cc0ecd

…p/VeeamBackupProvider.java

Veeam: set backed_volumes for each backup (#9898)

6da5db5

winterhazel approved these changes Feb 25, 2025

View reviewed changes

apache deleted a comment from blueorangutan Feb 25, 2025

weizhouapache approved these changes Feb 25, 2025

View reviewed changes

abh1sar approved these changes Feb 25, 2025

View reviewed changes

DaanHoogland merged commit 48f890a into main Feb 26, 2025
47 of 50 checks passed

DaanHoogland deleted the backup-framework-merge-problems branch February 26, 2025 13:12

DaanHoogland added this to the 4.21.0 milestone Feb 26, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resolve merge problems in the backup framework #10457

resolve merge problems in the backup framework #10457

DaanHoogland commented Feb 24, 2025

codecov bot commented Feb 24, 2025 •

edited

Loading

weizhouapache commented Feb 25, 2025

DaanHoogland commented Feb 25, 2025

weizhouapache commented Feb 25, 2025

DaanHoogland commented Feb 25, 2025

winterhazel commented Feb 25, 2025

winterhazel left a comment

weizhouapache left a comment

sonarqubecloud bot commented Feb 25, 2025

DaanHoogland commented Feb 25, 2025

blueorangutan commented Feb 25, 2025

DaanHoogland commented Feb 25, 2025

blueorangutan commented Feb 25, 2025

abh1sar left a comment

abh1sar Feb 25, 2025

abh1sar Feb 25, 2025

abh1sar Feb 25, 2025

winterhazel commented Feb 25, 2025

blueorangutan commented Feb 26, 2025

		@@ -328,54 +328,12 @@ public Map<VirtualMachine, Backup.Metric> getBackupMetrics(final Long zoneId, fi

		@Override

resolve merge problems in the backup framework #10457

resolve merge problems in the backup framework #10457

Conversation

DaanHoogland commented Feb 24, 2025

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

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

codecov bot commented Feb 24, 2025 • edited Loading

Codecov Report

weizhouapache commented Feb 25, 2025

DaanHoogland commented Feb 25, 2025

weizhouapache commented Feb 25, 2025

DaanHoogland commented Feb 25, 2025

winterhazel commented Feb 25, 2025

winterhazel left a comment

Choose a reason for hiding this comment

weizhouapache left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 25, 2025

Quality Gate failed

DaanHoogland commented Feb 25, 2025

blueorangutan commented Feb 25, 2025

DaanHoogland commented Feb 25, 2025

blueorangutan commented Feb 25, 2025

abh1sar left a comment

Choose a reason for hiding this comment

abh1sar Feb 25, 2025

Choose a reason for hiding this comment

abh1sar Feb 25, 2025

Choose a reason for hiding this comment

abh1sar Feb 25, 2025

Choose a reason for hiding this comment

winterhazel commented Feb 25, 2025

blueorangutan commented Feb 26, 2025

codecov bot commented Feb 24, 2025 •

edited

Loading