Fix selected differential backup chain state handling#1238
Conversation
ⓘ Only copy a per-backup incremental snapshot to the default snapshot after the full backup archive has been created successfully. This prevents failed chained full backups from advancing the baseline used by later non-chained differential backups.
ⓘ Move chained full-backup snapshot promotion out of the per-domain archive step and into the final backup result path, so full backup state is advanced only after all destinations succeed. Clear new full-backup state on failure, and keep destination locks held until state has been updated.
ⓘ Hide and reject legacy scheduled backup ID 1 as a selected full-backup baseline, since increment value 1 already represents legacy differential mode and cannot safely encode a schedule reference.
ⓘ Always keep per-schedule snapshot state for scheduled full backups so selected differential chains can be created later. Fail selected differentials clearly when the chosen full backup state is missing, avoiding misleading backups based on the wrong baseline.
| # snapshot files, so failed full backups must not become a future baseline. | ||
| if ($increment == 0 && &has_incremental_tar()) { | ||
| foreach my $d (@errdoms) { | ||
| my %errids = map { $_->{'id'} ? ($_->{'id'}, 1) : () } @errdoms; |
There was a problem hiding this comment.
This is pretty hard to read - best to use grep and map chained
|
Didn't the old incremental backups before I added this feature already have the behavior that the incremental state could be advanced even before all destinations were complete? |
Yeah, |
|
That's kind of a judgement call , because maybe the admin does prefer the current behavior if they are fine with at least one destination being OK. So maybe limit this PR to bugs in the new feature? |
Ah, I see what you mean now! See this patch d88185d. It will keep selected full-backup snapshot state only after the backup reaches destination, while preserving old behavior. |
|
Hang on, this is getting even more complex and we're fixing a whole bunch of separate issues in this PR that makes it hard to understand! Can we limit this to just bug fixes, and not change the existing behavior of when the incremental state is advanced? That may be something we want, but it should be in a separate PR. |
Okay. I've split it into its own #1246 PR that needs to go in once this one lands. |
| elsif ($mode == 0 && $id =~ /^\d+$/ && $id >= 3) { | ||
| # This is a scheduled full backup. Keep its own snapshot even before | ||
| # any differential schedule references it, so future chains can use it. | ||
| return "$incremental_backups_dir/$id/$d->{'id'}"; |
There was a problem hiding this comment.
This can't be done safely for existing backups, as it would cause the incremental state file to change on Virtualmin upgrade. That's why I did it the way I did..
There was a problem hiding this comment.
Can you elaborate and explain exactly why "this can't be done safely for existing backups" so I can get a better picture of your view of the problem?
There was a problem hiding this comment.
What I mean is, with this change the incremental file for a full backup would change EVEN if there is no differential backup specifically referencing it, which we don't want.
Also, what's with this check for $id =~ /^\d+$/ && $id >= 3 ? The scheduled backup ID doesn't mean anything, except for legacy ID 1. There would never ben an ID 0.
There was a problem hiding this comment.
Understood now! Thanks for pointing this out! I have fixed it in this 5635bcb change. Please check.
| foreach my $lockfile (@lockfiles) { | ||
| &unlock_file($lockfile); | ||
| } | ||
| # Release lock on dest file |
There was a problem hiding this comment.
Why this indentation change?
2 participants
Hello Jamie!
This fixes several edge cases in selected full-backup differential chains considering your latest changes to master!
Previously, full backup snapshot state could be advanced before the full backup was actually usable, or selected differential schedules could be configured against baselines that could not be represented or did not exist.
What changed (you can see more details in each linked commit):
1as a selected full-backup baseline, becauseincrement == 1already represents legacy differential mode.