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

[Christoph Heiss <c.heiss@proxmox.com>]

On Tue, Jan 10, 2023 at 01:34:14PM +0100, Fiona Ebner wrote:
> 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).
Thanks for the pointers!

>
> 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.
After looking into it, my first though was maybe to add a (boolean)
parameter to the `finish` endpoint.
But creating a separate endpoint and calling that before `finish` sounds
very reasonable as well.
Any thoughts on what would be more idiomatic/reasonable?

>
> > 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.
Thanks for the idea, that may be doable!

>
> 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.
I'd base it off the decision from above - if the `proxmox-backup-client`
version supports setting it directly, use that, otherwise simply fall
back.

>
> > 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.