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

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

[Dietmar Maurer]

> On 09/02/2021 8:48 AM Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:
> 
>  
> On 02.09.21 08:45, Dietmar Maurer wrote:
> >>> * 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.
> > 
> > I would not consider the protected flag for syncs, i.e:
> > - do not sync the protected flag itself
> 
> That I agree (and I did not meant to suggest otherwise).
> 
> > - remove vanished backups even with protected flag set (to be in sync with source)
> 
> That I do not agree, if I marked a snapshot explicitly protected, which
> is a must for the situation to happen with the first point above in mind
> (no syncing of the protection flag itself) then I'm pretty sure that I want
> to keep that snapshot no matter what.

Also OK for me.

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

[Thomas Lamprecht]

On 02.09.21 08:45, Dietmar Maurer wrote:
>>> * 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.
> 
> I would not consider the protected flag for syncs, i.e:
> - do not sync the protected flag itself

That I agree (and I did not meant to suggest otherwise).

> - remove vanished backups even with protected flag set (to be in sync with source)

That I do not agree, if I marked a snapshot explicitly protected, which
is a must for the situation to happen with the first point above in mind
(no syncing of the protection flag itself) then I'm pretty sure that I want
to keep that snapshot no matter what.

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

[Dietmar Maurer]

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

I would not consider the protected flag for syncs, i.e:
- do not sync the protected flag itself
- remove vanished backups even with protected flag set (to be in sync with source)

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

[Thomas Lamprecht]

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(-)
> 

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

[Dominik Csapak]

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..)
* would we want to protect also against manual removal ? or
 'remove-vanished' on sync?
* how should the ui look? do we want *another* button in the content
  view?
* would it make sense to specify this flag on backup creation too?

the finished series would fix #3602

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(-)

-- 
2.30.2