Fix/13612 template cleanup on domain delete#13614
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
|
There was a problem hiding this comment.
Pull request overview
This PR addresses a cleanup edge case during account/domain teardown where template deletions that return false could be effectively “masked” by later successful deletions, leading to premature domain deletion attempts while template artifacts still remained in secondary storage.
Changes:
- Update template cleanup loop in
AccountManagerImpl.cleanupAccountto treat anyfalsereturn from_tmpltMgr.delete(...)as a persistent failure signal (without being overwritten by later iterations). - Improve logging for the
falsereturn case and ensure exceptions also forceallTemplatesDeleted = false. - Ensure
accountCleanupNeededis set when any template deletion fails, preventing premature teardown.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logger.debug("Successfully deleted snapshots directories for all volumes under account {} across all zones", account); | ||
| } | ||
|
|
||
|
|
| if (template.getRemoved() == null) { | ||
| try { | ||
| allTemplatesDeleted = _tmpltMgr.delete(callerUserId, template.getId(), null); | ||
| // Fix 1: Prevent boolean overwriting by checking failure explicitly |
There was a problem hiding this comment.
| // Fix 1: Prevent boolean overwriting by checking failure explicitly | |
| // Prevent boolean overwriting by checking failure explicitly |
| } | ||
| } catch (Exception e) { | ||
| logger.warn("Failed to delete template {} while removing account {} due to: ", template, account, e); | ||
| logger.warn("Failed to delete template {} while removing account {} due to exception: ", template, account, e); |
There was a problem hiding this comment.
this is a bit unusual. the “due to” is generally thought to already imply an exception.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13614 +/- ##
============================================
- Coverage 19.50% 19.49% -0.01%
+ Complexity 19444 19435 -9
============================================
Files 6303 6303
Lines 569286 569288 +2
Branches 69791 69792 +1
============================================
- Hits 111040 111010 -30
- Misses 446093 446123 +30
- Partials 12153 12155 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Can we close the pr as @Damans227 has also created a pr to resolve the issue |
Description
This PR fixes an issue where templates belonging to child accounts within a domain were not properly tracked during automated account and domain cleanup, causing domain deletion to fail due to orphaned storage resources.
Functional Changes:
AccountManagerImpl.cleanupAccountto explicitly check the boolean return value of_tmpltMgr.delete(...).allTemplatesDeletedtracking flag from being overwritten on subsequent loop iterations if an earlier template deletion fails.accountCleanupNeeded = true, preventing premature domain teardowns when child resources remain active in secondary storage.Types of changes
Checklist: