Re: [pve-devel] [PATCH storage] fix #4289: pbs: wait for backup verification to finish before updating volume attribute

[Fiona Ebner <f.ebner@proxmox.com>]

Am 10.01.23 um 12:11 schrieb Christoph Heiss:
> On Wed, Jan 04, 2023 at 11:50:38AM +0100, Fiona Ebner wrote:
>> It might not seem that big of a deal, because usually only manual
>> backups use 'protected'.  But by doing it in
>> update_volume_attribute(), you also do it for 'notes', where it's not
>> needed and which is relevant to backup jobs where the increased wait
>> might be very noticeable. So at least, it should only be done for
>> 'protected' if doing it in update_volume_attribute().
> That is actually the case now - updating notes takes a different path
> through update_volume_notes().
> 

Sorry, I missed that.

>>
>> It would be better if the protected flag could be specified upon
>> creation already. Would also fix the following race I guess:
> It definitely would be a lot cleaner. I'll see what I can do and rework
> the whole series.
> Probably involves adding a new parameter to the `proxmox-backup-client
> backup` command and API(?) AFAICS. But this would not be all that bad
> of a feature for the backup client in general, I think.

I think you also need to add support in QEMU (new parameter for the
'backup' QMP command) and the proxmox-backup-qemu library (to handle the
parameter).

Regarding the API, maybe it can be its own endpoint in the backup API
(alongside endpoints like 'blob' and 'finish')? As long as we protect
the backup before marking it as finished it should be good. Just an
idea, not sure if it would be better.

> And I guess I need to figure out a way how to detect whether the new
> parameter is supported or not?

If there is no straightforward way to make that information available in
VZDump.pm, we could also just base the decision off of the PBS version.

One way to decide if the current behavior should be used as a fallback
would be to check the protected status after finishing the backup. That
is slightly racy though, because something else could've already changed
the protection between finishing and the check.

> In case this it not supported, just keeping the current behavior (i.e.
> best-effort via the API and maybe failing) is probably the sensible way.

Yes, to not break existing setups. Also note that non-PBS backup
storages need the current behavior too.