MitchDrage commented on code in PR #14305:
URL: https://github.com/apache/cloudstack/pull/14305#discussion_r4205666843
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -826,18 +826,15 @@ private void createCheckedBackup(CreateBackupCmd cmd,
Account owner, boolean isS
true, 0);
Pair<Boolean, Backup> result = backupProvider.takeBackup(vm,
cmd.getQuiesceVM());
+ Backup backup = result.second();
if (!result.first()) {
+ if (backup != null) {
+ updateBackupFromCmd(backup.getId(), cmd, backupScheduleId);
+ }
Review Comment:
I did state this in the issue I raised, but on reflection I don't think
Error rows should be rotated.
listBySchedule counts only BackedUp backups toward maxbackups, which I think
is right - a backup that failed shouldn't take one of the schedule's retention
slots or push out a completed backup.
Error rows only occur when cleanup failed on the host, which this PR now
raises as an alert naming the host and mount point, so an admin can clear the
mount and delete the backup by hand. Deleting them automatically risks removing
a destination a stuck job is still writing to.
On resource accounting: these rows weren't added to the account's backup
resource count at creation, although the resource count recalculation includes
them and deleting one decrements the count. e8412df59f increments the account
count when a failed backup is kept in Error state, so creation, recalculation
and deletion will all now agree on the numbers.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
