From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <f.ebner@proxmox.com> 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 96EC06CCB2 for <pve-devel@lists.proxmox.com>; Fri, 24 Sep 2021 13:17:21 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 94CCFA010 for <pve-devel@lists.proxmox.com>; Fri, 24 Sep 2021 13:17:21 +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 74CDBA005 for <pve-devel@lists.proxmox.com>; Fri, 24 Sep 2021 13:17:20 +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 4ADB444A96 for <pve-devel@lists.proxmox.com>; Fri, 24 Sep 2021 13:17:20 +0200 (CEST) To: Dominik Csapak <d.csapak@proxmox.com>, Proxmox VE development discussion <pve-devel@lists.proxmox.com> References: <20210917130227.248852-1-f.ebner@proxmox.com> <20210917130227.248852-5-f.ebner@proxmox.com> <001f7e45-8693-6364-94bc-2261bfb9f3a0@proxmox.com> From: Fabian Ebner <f.ebner@proxmox.com> Message-ID: <d64d403c-df21-5c49-242d-cbd4cec605ce@proxmox.com> Date: Fri, 24 Sep 2021 13:17:18 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <001f7e45-8693-6364-94bc-2261bfb9f3a0@proxmox.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.318 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. [dirplugin.pm, plugin.pm, storage.pm, content.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 <pve-devel.lists.proxmox.com> List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe> List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/> List-Post: <mailto:pve-devel@lists.proxmox.com> List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help> List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe> X-List-Received-Date: Fri, 24 Sep 2021 11:17:21 -0000 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', >> > >