Re: [pve-devel] [RFC storage 4/6] fix #3307: make it possible to set protection for backups

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

Am 24.09.21 um 10:54 schrieb Dominik Csapak:
> On 9/17/21 15:02, Fabian Ebner wrote:
>> A protected backup is not removed by free_image and ignored when
>> pruning.
>>
>> The protection_file_path function is introduced in Storage.pm, so that
>> it can also be used by vzdump itself and in archive_remove.
>>
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>>
>> Needs an APIAGE+APIVER bump (can be covered by the one for patch #2),
>> because of the new attribute.
>>
>> I decided to not just re-use the "renamed is protected" behavior when
>> pruning, because:
>> 1. it wouldn't protect against removal
>> 2. it would make it necessary to rename notes and log files too
>>
>> Should the protection files rather be hidden files?
> 
> imho no, because if users clean the dir up manually, they're likely
> to miss those
> 
>>
>> Should we also bother to set the immutable flag on filesystems that
>> support it?
> 
> if a user want to remove that file manually, why should we make
> it harder for a user? my guess is that most would then either
> remove the protection via the api, or come to the forum, where
> there will probably be someone that explains how to remove
> the immutable flag
> 
> i think it would just add complexity for little gain
> 
> if we would want it, setting the immutable flag on the
> volume to protect it could work though, but since
> there can be an arbitrary filesystem underneath,
> we would have to test each time
> 
>>
>>   PVE/API2/Storage/Content.pm | 37 ++++++++++++++++++++++++++++---------
>>   PVE/Storage.pm              |  9 +++++++++
>>   PVE/Storage/DirPlugin.pm    | 26 ++++++++++++++++++++++++--
>>   PVE/Storage/Plugin.pm       |  7 +++++++
>>   test/prune_backups_test.pm  | 13 +++++++++++++
>>   5 files changed, 81 insertions(+), 11 deletions(-)
>>
>> diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
>> index b3dc593..45b8de8 100644
>> --- a/PVE/API2/Storage/Content.pm
>> +++ b/PVE/API2/Storage/Content.pm
>> @@ -113,6 +113,11 @@ __PACKAGE__->register_method ({
>>               },
>>               optional => 1,
>>           },
>> +        protected => {
>> +            description => "Protection status. Currently only 
>> supported for backups.",
>> +            type => 'boolean',
>> +            optional => 1,
>> +        },
>>           },
>>       },
>>       links => [ { rel => 'child', href => "{volid}" } ],
>> @@ -295,7 +300,12 @@ __PACKAGE__->register_method ({
>>           description => "Optional notes.",
>>           optional => 1,
>>           type => 'string',
>> -        }
>> +        },
>> +        protected => {
>> +        description => "Protection status. Currently only supported 
>> for backups.",
>> +        type => 'boolean',
>> +        optional => 1,
>> +        },
>>       },
>>       },
>>       code => sub {
>> @@ -321,12 +331,14 @@ __PACKAGE__->register_method ({
>>           format => $format,
>>       };
>> -    # keep going if fetching an optional attribute fails
>> -    eval {
>> -        my $notes = PVE::Storage::get_volume_attribute($cfg, $volid, 
>> 'notes');
>> -        $entry->{notes} = $notes if defined($notes);
>> -    };
>> -    warn $@ if $@;
>> +    for my $attribute (qw(notes protected)) {
>> +        # keep going if fetching an optional attribute fails
>> +        eval {
>> +        my $value = PVE::Storage::get_volume_attribute($cfg, $volid, 
>> $attribute);
>> +        $entry->{$attribute} = $value if defined($value);
>> +        };
>> +        warn $@ if $@;
>> +    }
>>       return $entry;
>>       }});
>> @@ -356,6 +368,11 @@ __PACKAGE__->register_method ({
>>           type => 'string',
>>           optional => 1,
>>           },
>> +        protected => {
>> +        description => "Protection status. Currently only supported 
>> for backups.",
>> +        type => 'boolean',
>> +        optional => 1,
>> +        },
>>       },
>>       },
>>       returns => { type => 'null' },
>> @@ -371,8 +388,10 @@ __PACKAGE__->register_method ({
>>       PVE::Storage::check_volume_access($rpcenv, $authuser, $cfg, 
>> undef, $volid);
>> -    if (exists $param->{notes}) {
>> -        PVE::Storage::update_volume_attribute($cfg, $volid, 'notes', 
>> $param->{notes});
>> +    for my $attr (qw(notes protected)) {
>> +        if (exists $param->{$attr}) {
>> +        PVE::Storage::update_volume_attribute($cfg, $volid, $attr, 
>> $param->{$attr});
>> +        }
>>       }
>>       return undef;
>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>> index e7d8e62..3beb645 100755
>> --- a/PVE/Storage.pm
>> +++ b/PVE/Storage.pm
>> @@ -1463,6 +1463,12 @@ sub decompressor_info {
>>       return $info;
>>   }
>> +sub protection_file_path {
>> +    my ($path) = @_;
>> +
>> +    return "${path}.protected";
>> +}
>> +
>>   sub archive_info {
>>       my ($archive) = shift;
>>       my $info;
>> @@ -1494,6 +1500,9 @@ sub archive_info {
>>   sub archive_remove {
>>       my ($archive_path) = @_;
>> +    die "cannot remove protected archive '$archive_path'\n"
>> +    if -e protection_file_path($archive_path);
>> +
>>       my $dirname = dirname($archive_path);
>>       my $archive_info = eval { archive_info($archive_path) } // {};
>>       my $logfn = $archive_info->{logfilename};
>> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
>> index 6ab9ef2..99a6eaf 100644
>> --- a/PVE/Storage/DirPlugin.pm
>> +++ b/PVE/Storage/DirPlugin.pm
>> @@ -2,8 +2,11 @@ package PVE::Storage::DirPlugin;
>>   use strict;
>>   use warnings;
>> +
>>   use Cwd;
>>   use File::Path;
>> +use IO::File;
>> +
>>   use PVE::Storage::Plugin;
>>   use PVE::JSONSchema qw(get_standard_option);
>> @@ -93,13 +96,16 @@ sub get_volume_attribute {
>>       my ($vtype) = $class->parse_volname($volname);
>>       return if $vtype ne 'backup';
>> +    my $path = $class->filesystem_path($scfg, $volname);
>> +
>>       if ($attribute eq 'notes') {
>> -    my $path = $class->filesystem_path($scfg, $volname);
>>       $path .= $class->SUPER::NOTES_EXT;
>>       return PVE::Tools::file_get_contents($path) if -f $path;
>>       return '';
>> +    } elsif ($attribute eq 'protected') {
>> +    return -e PVE::Storage::protection_file_path($path) ? 1 : 0;
>>       }
>>       return;
>> @@ -111,8 +117,9 @@ sub update_volume_attribute {
>>       my ($vtype) = $class->parse_volname($volname);
>>       die "only backups support attribute '$attribute'\n" if $vtype ne 
>> 'backup';
>> +    my $path = $class->filesystem_path($scfg, $volname);
>> +
>>       if ($attribute eq 'notes') {
>> -    my $path = $class->filesystem_path($scfg, $volname);
>>       $path .= $class->SUPER::NOTES_EXT;
>>       if (defined($value) && $value ne '') {
>> @@ -120,6 +127,21 @@ sub update_volume_attribute {
>>       } elsif (-e $path) {
>>           unlink $path or die "could not delete notes - $!\n";
>>       }
>> +    return;
>> +    } elsif ($attribute eq 'protected') {
>> +    my $protection_path = PVE::Storage::protection_file_path($path);
>> +
>> +    return if !((-e $protection_path) xor $value); # protection 
>> status already correct
> 
> is that not a '(-e $protection_path) == $value' ?
> if that works, i'd prefer it, since its simpler to read

The operands could be undef, which produces a warning for '=='. Since 
xor and ! are for boolean expressions, I used those. But if there is a 
cleaner way to compare to booleans in Perl, I'm all ears ;)

> 
>> +
>> +    if ($value) {
>> +        my $fh = IO::File->new($protection_path, O_CREAT, 0644)
>> +        or die "unable to create protection file '$protection_path' - 
>> $!\n";
>> +        close($fh);
>> +    } else {
>> +        unlink $protection_path
>> +        or die "unable to remove protection file '$protection_path' - 
>> $!\n";
> 
> same thing as in the first patch, only a non ENOENT should probably 
> raise an error
> 
Will change that in v2.

>> +    }
>> +
>>       return;
>>       }
>> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
>> index 1182008..1ebd705 100644
>> --- a/PVE/Storage/Plugin.pm
>> +++ b/PVE/Storage/Plugin.pm
>> @@ -782,6 +782,9 @@ sub alloc_image {
>>   sub free_image {
>>       my ($class, $storeid, $scfg, $volname, $isBase, $format) = @_;
>> +    die "cannot remove protected volume '$volname' on '$storeid'\n"
>> +    if $class->get_volume_attribute($scfg, $storeid, $volname, 
>> 'protected');
>> +
>>       my $path = $class->filesystem_path($scfg, $volname);
>>       if ($isBase) {
>> @@ -871,6 +874,7 @@ sub update_volume_notes {
>>   # Should die if there is an error fetching the attribute.
>>   # Possible attributes:
>>   # notes     - user-provided comments/notes.
>> +# protected - not to be removed by free_image, and for backups, 
>> ignored when pruning.
>>   sub get_volume_attribute {
>>       my ($class, $scfg, $storeid, $volname, $attribute) = @_;
>> @@ -1115,6 +1119,7 @@ my $get_subdir_files = sub {
>>           $info->{notes} = $notes if defined($notes);
>>           }
>> +        $info->{protected} = 1 if -e 
>> PVE::Storage::protection_file_path($original);
>>       } elsif ($tt eq 'snippets') {
>>           $info = {
>> @@ -1320,6 +1325,8 @@ sub prune_backups {
>>           $prune_entry->{mark} = 'protected';
>>       }
>> +    $prune_entry->{mark} = 'protected' if $backup->{protected};
>> +
>>       push @{$prune_list}, $prune_entry;
>>       }
>> diff --git a/test/prune_backups_test.pm b/test/prune_backups_test.pm
>> index a87c4b3..8ad6144 100644
>> --- a/test/prune_backups_test.pm
>> +++ b/test/prune_backups_test.pm
>> @@ -29,6 +29,12 @@ foreach my $vmid (@vmids) {
>>           'ctime' => $basetime - 24*60*60 - 60*60,
>>           'vmid'  => $vmid,
>>       },
>> +    {
>> +        'volid' => 
>> "$storeid:backup/vzdump-qemu-$vmid-2019_12_31-11_18_51.tar.zst",
>> +        'ctime' => $basetime - 24*60*60 - 60*60 + 30,
>> +        'vmid'  => $vmid,
>> +        'protected' => 1,
>> +    },
>>       {
>>           'volid' => 
>> "$storeid:backup/vzdump-qemu-$vmid-2019_12_31-11_19_21.tar.zst",
>>           'ctime' => $basetime - 24*60*60 - 60*60 + 60,
>> @@ -140,6 +146,13 @@ sub generate_expected {
>>           'mark'  => $marks->[1],
>>           'vmid'  => $vmid,
>>           },
>> +        {
>> +        'volid' => 
>> "$storeid:backup/vzdump-qemu-$vmid-2019_12_31-11_18_51.tar.zst",
>> +        'type'  => 'qemu',
>> +        'ctime' => $basetime - 24*60*60 - 60*60 + 30,
>> +        'mark'  => 'protected',
>> +        'vmid'  => $vmid,
>> +        },
>>           {
>>           'volid' => 
>> "$storeid:backup/vzdump-qemu-$vmid-2019_12_31-11_19_21.tar.zst",
>>           'type'  => 'qemu',
>>
> 
>