Incremental nas backup fixes

[abh1sar]

Description

This PR contains some fixes that are required on top of #13074

1. Concurrent deletes can modify the same backup chain in parallel. There is no synchronisation mechanism, so in a race condition a backup can be left in the Hidden state without ever being deleted. Using a per-VM global lock to synchronise deletes.

2. If a backup can be deleted only checks if its immediate child is present or not (not hidden and not removed). But it should check all its dependents. Otherwise it will be wrongly deleted.

3. Removed dead code: findChainTail and findChainParent

4. Added null check before accessing answer.getNeedsCleanup()

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?

Reproduced the failing test cases and verified with the fixes.

1. Create 50 backups and delete them in parallel.
2. Create a backup chain b1->b2->b3->b4 - delete b2 and then delete b1 - b1 should remain hidden and not deleted on the backup repository.
3. Tested backups, delete and restore in general

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

[abh1sar]

HI @jmsperu
Can you please review the changes?

[Copilot]

Pull request overview

This PR builds on the incremental NAS backup work by hardening incremental chain deletion: it serializes chain-mutating deletes with a per-VM lock, fixes the “can I delete this backup?” decision to consider all downstream dependents (not just the immediate child), removes dead chain-navigation helpers, and adds a null-check around answer.getNeedsCleanup().

Changes:

• Serialize backup-chain deletions per VM using a GlobalLock and re-read the target backup row under the lock to avoid race-induced “stuck Hidden” backups.
• Update chain delete semantics to detect any live descendants (via CHAIN_POSITION) before physically deleting a backup; enhance sweep behavior and add multiple new unit tests for edge cases.
• Remove unused chain helper methods and add a null-check before calling answer.getNeedsCleanup().

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java	Adds per-VM locking for chain deletes and updates descendant detection + sweeping logic for incremental backup chains.
plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java	Updates and extends unit tests to cover new lock behavior and additional chain-delete edge cases.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[abh1sar]

@blueorangutan package

[blueorangutan]

@abh1sar a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

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

• chainPosition returns Integer.MAX_VALUE for missing/invalid CHAIN_POSITION, but getChainOrderedLeafToRoot sorts in descending order. That means missing metadata sorts to the front (treated as the newest/leaf), contradicting the in-code comment and potentially causing incorrect sweep ordering and dependent detection when metadata is missing or malformed.

    private int chainPosition(Backup b) {
        String s = readDetail(b, NASBackupChainKeys.CHAIN_POSITION);
        if (s == null) {
            return Integer.MAX_VALUE; // no metadata => sort to end
        }
        try {
            return Integer.parseInt(s);
        } catch (NumberFormatException e) {
            return Integer.MAX_VALUE;
        }

[codecov]

Codecov Report

❌ Patch coverage is 63.04348% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 19.45%. Comparing base (333973a) to head (a8937e2).

Files with missing lines	Patch %	Lines
...rg/apache/cloudstack/backup/NASBackupProvider.java	63.04%	13 Missing and 4 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##              main   #13571       +/-   ##
============================================
+ Coverage     3.46%   19.45%   +15.99%     
- Complexity       0    19199    +19199     
============================================
  Files          479     6288     +5809     
  Lines        41162   565359   +524197     
  Branches      7793    69006    +61213     
============================================
+ Hits          1426   110017   +108591     
- Misses       39543   443395   +403852     
- Partials       193    11947    +11754     

Flag	Coverage Δ
uitests	3.46% <ø> (ø)
unittests	20.71% <63.04%> (?)

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

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[blueorangutan]

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

[weizhouapache]

code lgtm

[weizhouapache]

@blueorangutan test

[blueorangutan]

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

[abh1sar]

There was integration test failure in test_backup_recovery_nas.py. Fixed and verified by running the test manually multiple times.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

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

• chainPosition returns Integer.MAX_VALUE when CHAIN_POSITION is missing or unparsable, but the comment says this should "sort to end". With the current leaf-first (descending) sort, MAX_VALUE actually sorts first and can also cause hasLiveChildren to miss real descendants when the current backup’s CHAIN_POSITION is absent, leading to unsafe deletions.

    private int chainPosition(Backup b) {
        String s = readDetail(b, NASBackupChainKeys.CHAIN_POSITION);
        if (s == null) {
            return Integer.MAX_VALUE; // no metadata => sort to end
        }

[weizhouapache]

There was integration test failure in test_backup_recovery_nas.py. Fixed and verified by running the test manually multiple times.

great!

[jmsperu]

Thanks @abh1sar, reviewed. The core fixes look correct:

• The null guard (answer != null && answer.getNeedsCleanup()) is right — the log line above already treats answer as nullable, so the previous code could NPE.
• Switching hasLiveChildren to compare CHAIN_POSITION correctly fixes the "only the immediate child was checked" bug — any non-tombstoned member deeper in the chain now correctly blocks deletion.
• The per-VM GlobalLock plus the re-read via findById under the lock (returning early if the row is already gone) is the right way to close the concurrent-delete race.
• Dead-code removal and the added tests for the deeper-orphan and live-descendant cases look good.

One thing I'd like changed before merge:

In deleteLeafBackupAndSweepPendingAncestors, the sweep now ignores the result of deleteBackupFileAndRow:

for (Backup member : chain) {
    if (member.getId() == backup.getId()) continue;
    if (!isDeletePending(member)) break;
    deleteBackupFileAndRow(member, repo, host);   // return value dropped
}

The previous version stopped the sweep on failure. Because the sweep walks leaf→root, if a child's on-NAS delete fails and the loop continues, we can delete that child's parent while the failed child still exists on the repository, which orphans the child (its base data is gone and it becomes unrestorable). Can we restore the stop-on-failure behaviour, i.e. break/return when deleteBackupFileAndRow returns false, so a partial NAS failure leaves a consistent chain for the next sweep to retry?

Two smaller points to confirm:

1. On the forced path, deleteBackup now holds the per-VM lock and then calls cascadeDeleteSubtree. Could you confirm nothing under it re-enters deleteBackup for the same VM in a way that unbalances the lock ref-count? (GlobalLock is reentrant per thread, so it should be fine, just want to be sure since that method isn't in this diff.)
2. hasLiveChildren relies on chainPosition. If CHAIN_POSITION is ever absent, the > comparison could misjudge dependents. Probably a non-issue since position is always written, but a defensive default may be worth it.

[abh1sar]

@blueorangutan package

[blueorangutan]

@abh1sar a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

[abh1sar]

One thing I'd like changed before merge:

In deleteLeafBackupAndSweepPendingAncestors, the sweep now ignores the result of deleteBackupFileAndRow:

for (Backup member : chain) {
    if (member.getId() == backup.getId()) continue;
    if (!isDeletePending(member)) break;
    deleteBackupFileAndRow(member, repo, host);   // return value dropped
}

The previous version stopped the sweep on failure. Because the sweep walks leaf→root, if a child's on-NAS delete fails and the loop continues, we can delete that child's parent while the failed child still exists on the repository, which orphans the child (its base data is gone and it becomes unrestorable). Can we restore the stop-on-failure behaviour, i.e. break/return when deleteBackupFileAndRow returns false, so a partial NAS failure leaves a consistent chain for the next sweep to retry?

We are deleting Hidden backups that don't have any dependents. Hidden backups without any live children are by design not restorable. Let's say we have 4 Hidden backups in a chain and one live leaf backup. In case one of the Hidden backup delete fails, and we stop walking the chain to delete rest of the backups. These Hidden backups will never get deleted.
Removing the condition makes the delete best-effort. We delete whatever Hidden backups we can (we know they don't have any live dependents, so it is safe to delete.

Two smaller points to confirm:

1. On the forced path, deleteBackup now holds the per-VM lock and then calls cascadeDeleteSubtree. Could you confirm nothing under it re-enters deleteBackup for the same VM in a way that unbalances the lock ref-count? (GlobalLock is reentrant per thread, so it should be fine, just want to be sure since that method isn't in this diff.)

That's right. deleteBackup is only called from BackupManagerImpl.

2. hasLiveChildren relies on chainPosition. If CHAIN_POSITION is ever absent, the > comparison could misjudge dependents. Probably a non-issue since position is always written, but a defensive default may be worth it.

Yes, chain position is always written. Not doing anything about it now. Maybe we can add some defensive code later

[jmsperu]

Thanks @abh1sar, that all makes sense.

On the sweep: agreed, you're right and I'll withdraw that. Since every member the sweep touches is already a Hidden tombstone with no live dependents, none of them are restorable regardless of order, so there's no live backup to orphan. Best-effort is the better behaviour here — stopping on the first failure would strand the remaining tombstones permanently (nothing would ever re-trigger their sweep), whereas continuing frees everything it can and leaves only the single failed file to be reclaimed later. Good call.

Confirmed on the other two: deleteBackup only entering from BackupManagerImpl settles the lock ref-count question, and the CHAIN_POSITION default is fine to leave for a later defensive pass since position is always written.

LGTM

[blueorangutan]

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

[abh1sar]

@blueorangutan test

[blueorangutan]

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

[blueorangutan]

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

Test	Result	Time (s)	Test File
ContextSuite context=TestNASBackupAndRecovery>:setup	Error	0.00	test_backup_recovery_nas.py
test_05_list_volumes_isrecursive	Failure	0.04	test_list_volumes.py
test_07_list_volumes_listall	Failure	0.04	test_list_volumes.py
test_01_deployVMInSharedNetwork	Failure	232.89	test_network.py
test_01_vpn_usage	Error	1.12	test_usage.py