[pve-devel] [RFC storage 4/6] fix #3307: make it possible to set protection for backups
Fabian Ebner
f.ebner at proxmox.com
Fri Sep 24 13:17:18 CEST 2021
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 at 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',
>>
>
>
More information about the pve-devel
mailing list