From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 3FED86CBAF for ; Fri, 24 Sep 2021 10:55:32 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3305B2FA41 for ; Fri, 24 Sep 2021 10:55:02 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 23D932FA0A for ; Fri, 24 Sep 2021 10:55:01 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id EE79043673; Fri, 24 Sep 2021 10:55:00 +0200 (CEST) Message-ID: <001f7e45-8693-6364-94bc-2261bfb9f3a0@proxmox.com> Date: Fri, 24 Sep 2021 10:54:59 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:93.0) Gecko/20100101 Thunderbird/93.0 Content-Language: en-US To: Proxmox VE development discussion , Fabian Ebner References: <20210917130227.248852-1-f.ebner@proxmox.com> <20210917130227.248852-5-f.ebner@proxmox.com> From: Dominik Csapak In-Reply-To: <20210917130227.248852-5-f.ebner@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.356 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [plugin.pm, dirplugin.pm, content.pm, storage.pm] Subject: Re: [pve-devel] [RFC storage 4/6] fix #3307: make it possible to set protection for backups X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Sep 2021 08:55:32 -0000 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 > --- > > 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 > + > + 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 > + } > + > 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', >