Re: [pbs-devel] [RFC PATCH proxmox-backup 0/5] add 'protected' setting for snapshots

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 01.09.21 10:25, Dominik Csapak wrote:
> add the means to 'protect' a snapshot against pruning by
> adding a file '.protected' in the snapshot folder
> 
> sending as RFC because i am not sure about a couple things, and there
> is no gui (yet):
> 
> * does it make sense to protect a snapshot this way? or would it be
>   better to have a 'central' protected list somewhere?
>   (sounds not right though..)

I find the flag file quite ok, other approach could be to either make the index
immutable, which is probably not a good idea as the trouble-maker FS like NFS surely
make this a PITA. We could also safe it in the unprotected part of the manifest,
similar to how we do with comments, would reduce the file count and often we have
that info already anyway, but if not it may be a bit more expensive to check
than a stat on a separate file.


> * would we want to protect also against manual removal ? or
>  'remove-vanished' on sync?

Would make sense as long as we allow to "unprotect" it, that's how we do it for guests
in PVE too. IMO it's weird/unexpected to mark it protected and allow some API mechanisms
to still remove it.

> * how should the ui look? do we want *another* button in the content
>   view?

Why not? We explicitly choose the actions columns for that reason, and the row for
a specific backup has only verify + delete anyway.

Alternatives could be:
- add protection column and add an edit button there, similar to how I did the
  edit pencil for the comments
- make a more general edit button for the snapshots, for now this could open a
  dialogue for owner, protection and comment.

Just throwing out things here.

> * would it make sense to specify this flag on backup creation too?

IMO that could make sense, depends on how we see the actual use case, i.e., reasons
for setting it.
If I want to make an safety backup before starting a dist upgrade or the like in my
guest in PVE, I'd probably like to specify this flag immediately and not wait until
the backup is done then login into PBS and set it there.

> 
> the finished series would fix #3602

please add that to the commit subject, at least of the GUI and API patches, i.e., those
that actually enable this feature for (non-local-root) users.

> 
> Dominik Csapak (5):
>   pbs-datastore: add protection info to BackupInfo
>   pbs-datastore: skip protected backups in pruning
>   add protected info of snapshots to api and task logs
>   api2/admin/datastore: add get/set_protection
>   ui: PruneInputPanel: add keepReason 'protected' for protected backups
> 
>  pbs-api-types/src/lib.rs         |   2 +
>  pbs-datastore/src/backup_info.rs |  20 +++++-
>  pbs-datastore/src/prune.rs       |  21 +++---
>  src/api2/admin/datastore.rs      | 112 ++++++++++++++++++++++++++++++-
>  src/server/prune_job.rs          |   4 +-
>  www/datastore/Prune.js           |   4 ++
>  6 files changed, 148 insertions(+), 15 deletions(-)
>