[pve-devel] [RFC storage 4/6] fix #3307: make it possible to set protection for backups
Dominik Csapak
d.csapak at proxmox.com
Fri Sep 24 10:54:59 CEST 2021
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
> +
> + 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',
>
More information about the pve-devel
mailing list