Fix tracking selected full backup state#1246
Conversation
This PR improves snapshot handling for selected full backup chains. Selected full-backup state is now retained only for domains whose full backup reaches at least one durable destination. Successful selected full snapshots are promoted, while snapshots from failed selected full backups are removed so future differentials do not silently depend on an unusable full backup.
|
Jamie, I've fixed the merge conflict. Please take a look. |
|
I believe it needs more fixes... I will update this with progress. |
ⓘ Promote selected full backup snapshot state only for referenced full-backup chains, and remove the stale per-domain promotion path from `feature-dir.pl`.
|
Jamie, this is ready for review now. |
|
Jamie, is this good to merge or not? |
|
So can you explain the failure mode that this PR fixes? Because in a regular backup to a local destination, the incremental file is updated in feature-dir.pl after the archive is created.. |
Yeah, for local backups. Yet this issue only affects a full backup selected as a differential base. If final packaging or upload fails after tar updates its state, that state points to a backup that doesn’t exist. This PR keeps the state if at least one destination succeeds or otherwise, it removes it, with existing incremental behavior is unchanged. |
|
Ok so the potential problem is that if an incremental backup fails to be uploaded to it's destination, we shouldn't update the state file? Or is it that if a full backup fails to upload we shouldn't clear the state file? I'm not quite sure of the full sequence of events this is fixing.. |
ⓘ Stage and lock selected-full snapshot state, publish it only after each domain archive reaches a durable destination, and discard unusable state on failure.
|
The issue is with a full backup that is selected as the base for future differentials, not with a failed differential upload. The failure sequence is:
So yes, the important case is "if a selected full backup fails to upload, we should not keep and publish the new state”. And, I added a few more fixes on the latest commit because the earlier version still could published state too early. The current version of this PR should make selected-full state fail closed. |
| # Serialize access to every snapshot this backup can read or replace. These | ||
| # locks are acquired before domain locks and held until state is finalized, so | ||
| # overlapping full and differential runs cannot publish, restore or remove each | ||
| # other's snapshot generation. |
There was a problem hiding this comment.
Isn't there already protection against multiple backups running at the same time for the same domain though, via locking? That should protect against this kind of failure.
There was a problem hiding this comment.
Not sufficient protection. The existing domain lock is released, while upload and snapshot publication happen much later.
So destination locking only protects jobs using the same destination URL. Two backups for the same domain but different destinations can potentially overlap, like:
- Full backup creates its staged snapshot
- It releases the domain lock and starts uploading
- A differential backup acquires the domain lock and accesses snapshot state
- The full backup later publishes or discards that state
There was a problem hiding this comment.
Yet, these a3a206f locking will be better to avoid lock_file timeouts.
| } | ||
| } | ||
| @incremental_lockfiles = sort @incremental_lockfiles; | ||
| foreach my $lockfile (@incremental_lockfiles) { |
There was a problem hiding this comment.
Doesn't get_incremental_file already create the directory for the file it returns?
There was a problem hiding this comment.
No. It only returns a pathname.
| # Invalidate every selected snapshot before the first domain is archived. If | ||
| # this process is terminated part-way through a multi-domain backup, no domain | ||
| # that was not reached can retain state for an older full backup generation. | ||
| if ($selected_full) { |
There was a problem hiding this comment.
I think you can exclude this, that feels like an over-complication for a very rare situation.
There was a problem hiding this comment.
I would rather simplify this if necessary, but not remove this crash protection completely. That prevents an interrupted run from leaving old state for a domain whose archive may already have been replaced, without invalidating every domain at the start, right?
ⓘ Serialize full and differential snapshot access with blocking `flock` handles held through final publication or discard. This avoids `lock_file` timeouts and automatically releases locks when a backup exits.
ⓘ Remove redundant ownership and mode verification after snapshot creation and rename, while failing publication if setting the required ownership or permissions fails. #1246 (comment)
|
So I had a look again at the current logic that is supposed to handle the case of a failed full backup, and realized that it doesn't treat failed uploads properly in the case of a full backup, as you correctly pointed out. But the fix to me seems as simple as : ef6b148 If this doesn't work, I would be really interested to see a sequence of Virtualmin CLI commands that demonstrate the problem. I was able to trigger it via a SSH full backup for a single to a working destination, then another SSH backup to a different destination with permissions that prevented an upload. |
|
I think you're right. My PR though also handled crashes and partial multi-destination success, but that extra complexity likely isn't justified. I will rather close this for now. And, for your latest change you could use |
Thanks! But does my fix really address all the issues you found originally though? I am surprised that it's so much simpler.
I considered that, but since $ok is used as an indicator that the archiving process failed in which case @errdoms should already be populated, I felt it was more readable to use $remotefailed. |
No, not really. Yet, I thought you wanted to keep it more simple... and this is why I closed it.
Got it, thanks! |
2 participants
Hello Jamie!
This PR improves snapshot handling for selected full backup chains as discussed in #1238 (comment).
Now selected full-backup state is retained only for domains whose full backup reaches at least one durable destination. Successful selected full snapshots are promoted, while snapshots from failed selected full backups are removed so future differentials do not silently depend on an unusable full backup.