* [pve-devel] [PATCH storage 1/6] dir plugin: update notes: don't attempt to remove non-existent notes
2021-09-17 13:02 [pve-devel] [RFC storage/manager] fix #3307: allow backups to be marked as protected Fabian Ebner
@ 2021-09-17 13:02 ` Fabian Ebner
2021-09-24 8:54 ` Dominik Csapak
2021-09-17 13:02 ` [pve-devel] [RFC storage 2/6] add generalized functions to manage volume attributes Fabian Ebner
` (5 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Fabian Ebner @ 2021-09-17 13:02 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
PVE/Storage/DirPlugin.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
index 2267f11..0423e5f 100644
--- a/PVE/Storage/DirPlugin.pm
+++ b/PVE/Storage/DirPlugin.pm
@@ -109,7 +109,7 @@ sub update_volume_notes {
if (defined($notes) && $notes ne '') {
PVE::Tools::file_set_contents($path, $notes);
- } else {
+ } elsif (-e $path) {
unlink $path or die "could not delete notes - $!\n";
}
return;
--
2.30.2
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pve-devel] [PATCH storage 1/6] dir plugin: update notes: don't attempt to remove non-existent notes
2021-09-17 13:02 ` [pve-devel] [PATCH storage 1/6] dir plugin: update notes: don't attempt to remove non-existent notes Fabian Ebner
@ 2021-09-24 8:54 ` Dominik Csapak
2021-09-24 9:03 ` Thomas Lamprecht
0 siblings, 1 reply; 20+ messages in thread
From: Dominik Csapak @ 2021-09-24 8:54 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Ebner
On 9/17/21 15:02, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> PVE/Storage/DirPlugin.pm | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
> index 2267f11..0423e5f 100644
> --- a/PVE/Storage/DirPlugin.pm
> +++ b/PVE/Storage/DirPlugin.pm
> @@ -109,7 +109,7 @@ sub update_volume_notes {
>
> if (defined($notes) && $notes ne '') {
> PVE::Tools::file_set_contents($path, $notes);
> - } else {
> + } elsif (-e $path) {
> unlink $path or die "could not delete notes - $!\n";
> }
> return;
>
nit: it is still racy, and imho the correct solution would be to
ignore the error but only if the file did not exists
iow $! is ENOENT
for most cases it's enough though and i am not sure if the
added complexity is worth it...
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pve-devel] [PATCH storage 1/6] dir plugin: update notes: don't attempt to remove non-existent notes
2021-09-24 8:54 ` Dominik Csapak
@ 2021-09-24 9:03 ` Thomas Lamprecht
2021-09-24 10:40 ` Fabian Ebner
0 siblings, 1 reply; 20+ messages in thread
From: Thomas Lamprecht @ 2021-09-24 9:03 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak, Fabian Ebner
On 24.09.21 10:54, Dominik Csapak wrote:
> On 9/17/21 15:02, Fabian Ebner wrote:
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>> PVE/Storage/DirPlugin.pm | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
>> index 2267f11..0423e5f 100644
>> --- a/PVE/Storage/DirPlugin.pm
>> +++ b/PVE/Storage/DirPlugin.pm
>> @@ -109,7 +109,7 @@ sub update_volume_notes {
>> if (defined($notes) && $notes ne '') {
>> PVE::Tools::file_set_contents($path, $notes);
>> - } else {
>> + } elsif (-e $path) {
>> unlink $path or die "could not delete notes - $!\n";
>> }
>> return;
>>
>
> nit: it is still racy, and imho the correct solution would be to
> ignore the error but only if the file did not exists
> iow $! is ENOENT
>
> for most cases it's enough though and i am not sure if the
> added complexity is worth it...
IMO it's worth it and it's really not much complexity..
use POSIX; # or `use POSIX qw(ENOENT);` if we filter default-exports from the POSIX module already
unlink $path;
die "could not delete notes - $!\n" if $! && $! != ENOENT;
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pve-devel] [PATCH storage 1/6] dir plugin: update notes: don't attempt to remove non-existent notes
2021-09-24 9:03 ` Thomas Lamprecht
@ 2021-09-24 10:40 ` Fabian Ebner
0 siblings, 0 replies; 20+ messages in thread
From: Fabian Ebner @ 2021-09-24 10:40 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion, Dominik Csapak
Am 24.09.21 um 11:03 schrieb Thomas Lamprecht:
> On 24.09.21 10:54, Dominik Csapak wrote:
>> On 9/17/21 15:02, Fabian Ebner wrote:
>>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>>> ---
>>> PVE/Storage/DirPlugin.pm | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
>>> index 2267f11..0423e5f 100644
>>> --- a/PVE/Storage/DirPlugin.pm
>>> +++ b/PVE/Storage/DirPlugin.pm
>>> @@ -109,7 +109,7 @@ sub update_volume_notes {
>>> if (defined($notes) && $notes ne '') {
>>> PVE::Tools::file_set_contents($path, $notes);
>>> - } else {
>>> + } elsif (-e $path) {
>>> unlink $path or die "could not delete notes - $!\n";
>>> }
>>> return;
>>>
>>
>> nit: it is still racy, and imho the correct solution would be to
>> ignore the error but only if the file did not exists
>> iow $! is ENOENT
>>
Good point.
>> for most cases it's enough though and i am not sure if the
>> added complexity is worth it...
>
> IMO it's worth it and it's really not much complexity..
>
> use POSIX; # or `use POSIX qw(ENOENT);` if we filter default-exports from the POSIX module already
>
> unlink $path;
> die "could not delete notes - $!\n" if $! && $! != ENOENT;
>
Ok, I'll do that in v2.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [pve-devel] [RFC storage 2/6] add generalized functions to manage volume attributes
2021-09-17 13:02 [pve-devel] [RFC storage/manager] fix #3307: allow backups to be marked as protected Fabian Ebner
2021-09-17 13:02 ` [pve-devel] [PATCH storage 1/6] dir plugin: update notes: don't attempt to remove non-existent notes Fabian Ebner
@ 2021-09-17 13:02 ` Fabian Ebner
2021-09-24 8:54 ` Dominik Csapak
2021-09-17 13:02 ` [pve-devel] [PATCH storage 3/6] prune mark: preserve additional information for the keep-all case Fabian Ebner
` (4 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Fabian Ebner @ 2021-09-17 13:02 UTC (permalink / raw)
To: pve-devel
replacing the ones for handling notes. The generic implementation in
Plugin.pm will fall back to the methods for notes to ensure backwards
compatibility with external plugins.
This is mainly done to avoid the need to add new methods every time a
new attribute is added.
Not adding a timeout parameter like the notes functions have, because
it was not used and can still be added if it ever is needed in the
future.
For get_volume_attribute, undef will indicate that the attribute is
not supported. This makes it possible to distinguish "not supported"
from "error getting the attribute", which is useful when the attribute
is important for an operation. For example, free_image checking for
protection (introduced in a later patch) can abort if getting the
'protected' attribute fails.
Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
Hope I didn't miss a way this would break external plugins.
Requires an APIAGE+APIVER bump.
The alternative to undef indicating "not supported" would be to add a
separate method like supports_volume_attribute. Or adapt
volume_has_feature, but it doesn't really feel like the right fit as
it's currently intended for features for guest images only.
PVE/API2/Storage/Content.pm | 7 +++---
PVE/Storage.pm | 12 +++++-----
PVE/Storage/BTRFSPlugin.pm | 4 ++--
PVE/Storage/CIFSPlugin.pm | 10 ++++----
PVE/Storage/CephFSPlugin.pm | 10 ++++----
PVE/Storage/DirPlugin.pm | 47 +++++++++++++++++++++++--------------
PVE/Storage/NFSPlugin.pm | 10 ++++----
PVE/Storage/PBSPlugin.pm | 28 ++++++++++++++--------
PVE/Storage/Plugin.pm | 36 ++++++++++++++++++++++++++++
9 files changed, 113 insertions(+), 51 deletions(-)
diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
index 4d0ceb6..b3dc593 100644
--- a/PVE/API2/Storage/Content.pm
+++ b/PVE/API2/Storage/Content.pm
@@ -321,11 +321,12 @@ __PACKAGE__->register_method ({
format => $format,
};
- # not all storages/types support notes, so ignore errors here
+ # keep going if fetching an optional attribute fails
eval {
- my $notes = PVE::Storage::get_volume_notes($cfg, $volid);
+ my $notes = PVE::Storage::get_volume_attribute($cfg, $volid, 'notes');
$entry->{notes} = $notes if defined($notes);
};
+ warn $@ if $@;
return $entry;
}});
@@ -371,7 +372,7 @@ __PACKAGE__->register_method ({
PVE::Storage::check_volume_access($rpcenv, $authuser, $cfg, undef, $volid);
if (exists $param->{notes}) {
- PVE::Storage::update_volume_notes($cfg, $volid, $param->{notes});
+ PVE::Storage::update_volume_attribute($cfg, $volid, 'notes', $param->{notes});
}
return undef;
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 7b319fe..729c90e 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -215,24 +215,24 @@ sub file_size_info {
return PVE::Storage::Plugin::file_size_info($filename, $timeout);
}
-sub get_volume_notes {
- my ($cfg, $volid, $timeout) = @_;
+sub get_volume_attribute {
+ my ($cfg, $volid, $attribute) = @_;
my ($storeid, $volname) = parse_volume_id($volid);
my $scfg = storage_config($cfg, $storeid);
my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
- return $plugin->get_volume_notes($scfg, $storeid, $volname, $timeout);
+ return $plugin->get_volume_attribute($scfg, $storeid, $volname, $attribute);
}
-sub update_volume_notes {
- my ($cfg, $volid, $notes, $timeout) = @_;
+sub update_volume_attribute {
+ my ($cfg, $volid, $attribute, $value) = @_;
my ($storeid, $volname) = parse_volume_id($volid);
my $scfg = storage_config($cfg, $storeid);
my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
- $plugin->update_volume_notes($scfg, $storeid, $volname, $notes, $timeout);
+ return $plugin->update_volume_attribute($scfg, $storeid, $volname, $attribute, $value);
}
sub volume_size_info {
diff --git a/PVE/Storage/BTRFSPlugin.pm b/PVE/Storage/BTRFSPlugin.pm
index dbc1244..61bede2 100644
--- a/PVE/Storage/BTRFSPlugin.pm
+++ b/PVE/Storage/BTRFSPlugin.pm
@@ -136,9 +136,9 @@ sub status {
return PVE::Storage::DirPlugin::status($class, $storeid, $scfg, $cache);
}
-# TODO: sub get_volume_notes {}
+# TODO: sub get_volume_attribute {}
-# TODO: sub update_volume_notes {}
+# TODO: sub update_volume_attribute {}
# croak would not include the caller from within this module
sub __error {
diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
index c5f3894..bee6885 100644
--- a/PVE/Storage/CIFSPlugin.pm
+++ b/PVE/Storage/CIFSPlugin.pm
@@ -286,13 +286,15 @@ sub check_connection {
return 1;
}
-sub get_volume_notes {
+sub get_volume_attribute {
my $class = shift;
- PVE::Storage::DirPlugin::get_volume_notes($class, @_);
+
+ return PVE::Storage::DirPlugin::get_volume_attribute($class, @_);
}
-sub update_volume_notes {
+sub update_volume_attribute {
my $class = shift;
- PVE::Storage::DirPlugin::update_volume_notes($class, @_);
+
+ return PVE::Storage::DirPlugin::update_volume_attribute($class, @_);
}
1;
diff --git a/PVE/Storage/CephFSPlugin.pm b/PVE/Storage/CephFSPlugin.pm
index 3b9a791..2c06804 100644
--- a/PVE/Storage/CephFSPlugin.pm
+++ b/PVE/Storage/CephFSPlugin.pm
@@ -232,14 +232,16 @@ sub deactivate_storage {
}
}
-sub get_volume_notes {
+sub get_volume_attribute {
my $class = shift;
- PVE::Storage::DirPlugin::get_volume_notes($class, @_);
+
+ return PVE::Storage::DirPlugin::get_volume_attribute($class, @_);
}
-sub update_volume_notes {
+sub update_volume_attribute {
my $class = shift;
- PVE::Storage::DirPlugin::update_volume_notes($class, @_);
+
+ return PVE::Storage::DirPlugin::update_volume_attribute($class, @_);
}
1;
diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
index 0423e5f..6ab9ef2 100644
--- a/PVE/Storage/DirPlugin.pm
+++ b/PVE/Storage/DirPlugin.pm
@@ -87,32 +87,43 @@ sub parse_is_mountpoint {
return $is_mp; # contains a path
}
-sub get_volume_notes {
- my ($class, $scfg, $storeid, $volname, $timeout) = @_;
+sub get_volume_attribute {
+ my ($class, $scfg, $storeid, $volname, $attribute) = @_;
- my $path = $class->filesystem_path($scfg, $volname);
- $path .= $class->SUPER::NOTES_EXT;
+ my ($vtype) = $class->parse_volname($volname);
+ return if $vtype ne 'backup';
- return PVE::Tools::file_get_contents($path) if -f $path;
+ if ($attribute eq 'notes') {
+ my $path = $class->filesystem_path($scfg, $volname);
+ $path .= $class->SUPER::NOTES_EXT;
- return '';
-}
+ return PVE::Tools::file_get_contents($path) if -f $path;
-sub update_volume_notes {
- my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
+ return '';
+ }
- my ($vtype) = $class->parse_volname($volname);
- die "only backups can have notes\n" if $vtype ne 'backup';
+ return;
+}
- my $path = $class->filesystem_path($scfg, $volname);
- $path .= $class->SUPER::NOTES_EXT;
+sub update_volume_attribute {
+ my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
- if (defined($notes) && $notes ne '') {
- PVE::Tools::file_set_contents($path, $notes);
- } elsif (-e $path) {
- unlink $path or die "could not delete notes - $!\n";
+ my ($vtype) = $class->parse_volname($volname);
+ die "only backups support attribute '$attribute'\n" if $vtype ne 'backup';
+
+ if ($attribute eq 'notes') {
+ my $path = $class->filesystem_path($scfg, $volname);
+ $path .= $class->SUPER::NOTES_EXT;
+
+ if (defined($value) && $value ne '') {
+ PVE::Tools::file_set_contents($path, $value);
+ } elsif (-e $path) {
+ unlink $path or die "could not delete notes - $!\n";
+ }
+ return;
}
- return;
+
+ die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n";
}
sub status {
diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
index 39bf15a..3a6efe5 100644
--- a/PVE/Storage/NFSPlugin.pm
+++ b/PVE/Storage/NFSPlugin.pm
@@ -188,13 +188,15 @@ sub check_connection {
return 1;
}
-sub get_volume_notes {
+sub get_volume_attribute {
my $class = shift;
- PVE::Storage::DirPlugin::get_volume_notes($class, @_);
+
+ return PVE::Storage::DirPlugin::get_volume_attribute($class, @_);
}
-sub update_volume_notes {
+sub update_volume_attribute {
my $class = shift;
- PVE::Storage::DirPlugin::update_volume_notes($class, @_);
+
+ return PVE::Storage::DirPlugin::update_volume_attribute($class, @_);
}
1;
diff --git a/PVE/Storage/PBSPlugin.pm b/PVE/Storage/PBSPlugin.pm
index bb1c382..d8e1ac8 100644
--- a/PVE/Storage/PBSPlugin.pm
+++ b/PVE/Storage/PBSPlugin.pm
@@ -782,24 +782,32 @@ sub deactivate_volume {
return 1;
}
-sub get_volume_notes {
- my ($class, $scfg, $storeid, $volname, $timeout) = @_;
+sub get_volume_attribute {
+ my ($class, $scfg, $storeid, $volname, $attribute) = @_;
+
+ if ($attribute eq 'notes') {
+ my (undef, $name, undef, undef, undef, undef, $format) = $class->parse_volname($volname);
- my (undef, $name, undef, undef, undef, undef, $format) = $class->parse_volname($volname);
+ my $data = run_client_cmd($scfg, $storeid, "snapshot", [ "notes", "show", $name ]);
- my $data = run_client_cmd($scfg, $storeid, "snapshot", [ "notes", "show", $name ]);
+ return $data->{notes} // '';
+ }
- return $data->{notes};
+ return;
}
-sub update_volume_notes {
- my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
+sub update_volume_attribute {
+ my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
- my (undef, $name, undef, undef, undef, undef, $format) = $class->parse_volname($volname);
+ if ($attribute eq 'notes') {
+ my (undef, $name, undef, undef, undef, undef, $format) = $class->parse_volname($volname);
- run_client_cmd($scfg, $storeid, "snapshot", [ "notes", "update", $name, $notes ], 1);
+ run_client_cmd($scfg, $storeid, "snapshot", [ "notes", "update", $name, $value ], 1);
- return undef;
+ return;
+ }
+
+ die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n";
}
sub volume_size_info {
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index 417d1fd..1182008 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -851,18 +851,52 @@ sub file_size_info {
return wantarray ? ($size, $format, $used, $parent, $st->ctime) : $size;
}
+# FIXME remove on the next APIAGE reset.
+# Deprecated, use get_volume_attribute instead.
sub get_volume_notes {
my ($class, $scfg, $storeid, $volname, $timeout) = @_;
die "volume notes are not supported for $class";
}
+# FIXME remove on the next APIAGE reset.
+# Deprecated, use update_volume_attribute instead.
sub update_volume_notes {
my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
die "volume notes are not supported for $class";
}
+# Returns undef if the attribute is not supported for the volume.
+# Should die if there is an error fetching the attribute.
+# Possible attributes:
+# notes - user-provided comments/notes.
+sub get_volume_attribute {
+ my ($class, $scfg, $storeid, $volname, $attribute) = @_;
+
+ if ($attribute eq 'notes') {
+ my $notes = eval { $class->get_volume_notes($scfg, $storeid, $volname); };
+ if (my $err = $@) {
+ return if $err =~ m/^volume notes are not supported/;
+ die $err;
+ }
+ return $notes;
+ }
+
+ return;
+}
+
+# Dies if the attribute is not supported for the volume.
+sub update_volume_attribute {
+ my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
+
+ if ($attribute eq 'notes') {
+ $class->update_volume_notes($scfg, $storeid, $volname, $value);
+ }
+
+ die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n";
+}
+
sub volume_size_info {
my ($class, $scfg, $storeid, $volname, $timeout) = @_;
my $path = $class->filesystem_path($scfg, $volname);
@@ -1098,6 +1132,8 @@ my $get_subdir_files = sub {
return $res;
};
+# If attributes are set on a volume, they should be included in the result.
+# See get_volume_attribute for a list of possible attributes.
sub list_volumes {
my ($class, $storeid, $scfg, $vmid, $content_types) = @_;
--
2.30.2
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pve-devel] [RFC storage 2/6] add generalized functions to manage volume attributes
2021-09-17 13:02 ` [pve-devel] [RFC storage 2/6] add generalized functions to manage volume attributes Fabian Ebner
@ 2021-09-24 8:54 ` Dominik Csapak
2021-09-24 11:05 ` Fabian Ebner
0 siblings, 1 reply; 20+ messages in thread
From: Dominik Csapak @ 2021-09-24 8:54 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Ebner
On 9/17/21 15:02, Fabian Ebner wrote:
> replacing the ones for handling notes. The generic implementation in
> Plugin.pm will fall back to the methods for notes to ensure backwards
> compatibility with external plugins.
>
> This is mainly done to avoid the need to add new methods every time a
> new attribute is added.
>
> Not adding a timeout parameter like the notes functions have, because
> it was not used and can still be added if it ever is needed in the
> future.
>
> For get_volume_attribute, undef will indicate that the attribute is
> not supported. This makes it possible to distinguish "not supported"
> from "error getting the attribute", which is useful when the attribute
> is important for an operation. For example, free_image checking for
> protection (introduced in a later patch) can abort if getting the
> 'protected' attribute fails.
>
> Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>
> Hope I didn't miss a way this would break external plugins.
i think it would break a plugin that is not derived from
the bas Plugin.pm no?
an example:
i have a CustomCIFSPlugin that uses CIFSPlugin as base
i have implemented a custom 'get_volume_notes' function
after this patch, 'get_volume_attribute' will be called, which
falls back to the CIFSPlugin version which calls the DirPlugin
'get_volume_attribute' instead of my custom 'get_volume_notes'
since we did not document the custom plugins very well, i am
not sure if that's even a supported scenario, or if a custom
plugin always have to use the 'Plugin' as a base
if it is supported, we could only solve it by either make
it a breaking api change, or by keeping the '*_volume_notes'
around and fall back to *_volume_attribute implmentation
or did i make a mistake anywhere in my assumptions?
>
> Requires an APIAGE+APIVER bump.
>
> The alternative to undef indicating "not supported" would be to add a
> separate method like supports_volume_attribute. Or adapt
> volume_has_feature, but it doesn't really feel like the right fit as
> it's currently intended for features for guest images only.
>
> PVE/API2/Storage/Content.pm | 7 +++---
> PVE/Storage.pm | 12 +++++-----
> PVE/Storage/BTRFSPlugin.pm | 4 ++--
> PVE/Storage/CIFSPlugin.pm | 10 ++++----
> PVE/Storage/CephFSPlugin.pm | 10 ++++----
> PVE/Storage/DirPlugin.pm | 47 +++++++++++++++++++++++--------------
> PVE/Storage/NFSPlugin.pm | 10 ++++----
> PVE/Storage/PBSPlugin.pm | 28 ++++++++++++++--------
> PVE/Storage/Plugin.pm | 36 ++++++++++++++++++++++++++++
> 9 files changed, 113 insertions(+), 51 deletions(-)
>
> diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
> index 4d0ceb6..b3dc593 100644
> --- a/PVE/API2/Storage/Content.pm
> +++ b/PVE/API2/Storage/Content.pm
> @@ -321,11 +321,12 @@ __PACKAGE__->register_method ({
> format => $format,
> };
>
> - # not all storages/types support notes, so ignore errors here
> + # keep going if fetching an optional attribute fails
> eval {
> - my $notes = PVE::Storage::get_volume_notes($cfg, $volid);
> + my $notes = PVE::Storage::get_volume_attribute($cfg, $volid, 'notes');
> $entry->{notes} = $notes if defined($notes);
> };
> + warn $@ if $@;
>
> return $entry;
> }});
> @@ -371,7 +372,7 @@ __PACKAGE__->register_method ({
> PVE::Storage::check_volume_access($rpcenv, $authuser, $cfg, undef, $volid);
>
> if (exists $param->{notes}) {
> - PVE::Storage::update_volume_notes($cfg, $volid, $param->{notes});
> + PVE::Storage::update_volume_attribute($cfg, $volid, 'notes', $param->{notes});
> }
>
> return undef;
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 7b319fe..729c90e 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -215,24 +215,24 @@ sub file_size_info {
> return PVE::Storage::Plugin::file_size_info($filename, $timeout);
> }
>
> -sub get_volume_notes {
> - my ($cfg, $volid, $timeout) = @_;
> +sub get_volume_attribute {
> + my ($cfg, $volid, $attribute) = @_;
>
> my ($storeid, $volname) = parse_volume_id($volid);
> my $scfg = storage_config($cfg, $storeid);
> my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>
> - return $plugin->get_volume_notes($scfg, $storeid, $volname, $timeout);
> + return $plugin->get_volume_attribute($scfg, $storeid, $volname, $attribute);
> }
>
> -sub update_volume_notes {
> - my ($cfg, $volid, $notes, $timeout) = @_;
> +sub update_volume_attribute {
> + my ($cfg, $volid, $attribute, $value) = @_;
>
> my ($storeid, $volname) = parse_volume_id($volid);
> my $scfg = storage_config($cfg, $storeid);
> my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>
> - $plugin->update_volume_notes($scfg, $storeid, $volname, $notes, $timeout);
> + return $plugin->update_volume_attribute($scfg, $storeid, $volname, $attribute, $value);
> }
>
> sub volume_size_info {
> diff --git a/PVE/Storage/BTRFSPlugin.pm b/PVE/Storage/BTRFSPlugin.pm
> index dbc1244..61bede2 100644
> --- a/PVE/Storage/BTRFSPlugin.pm
> +++ b/PVE/Storage/BTRFSPlugin.pm
> @@ -136,9 +136,9 @@ sub status {
> return PVE::Storage::DirPlugin::status($class, $storeid, $scfg, $cache);
> }
>
> -# TODO: sub get_volume_notes {}
> +# TODO: sub get_volume_attribute {}
>
> -# TODO: sub update_volume_notes {}
> +# TODO: sub update_volume_attribute {}
>
> # croak would not include the caller from within this module
> sub __error {
> diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
> index c5f3894..bee6885 100644
> --- a/PVE/Storage/CIFSPlugin.pm
> +++ b/PVE/Storage/CIFSPlugin.pm
> @@ -286,13 +286,15 @@ sub check_connection {
> return 1;
> }
>
> -sub get_volume_notes {
> +sub get_volume_attribute {
> my $class = shift;
> - PVE::Storage::DirPlugin::get_volume_notes($class, @_);
> +
> + return PVE::Storage::DirPlugin::get_volume_attribute($class, @_);
> }
> -sub update_volume_notes {
> +sub update_volume_attribute {
> my $class = shift;
> - PVE::Storage::DirPlugin::update_volume_notes($class, @_);
> +
> + return PVE::Storage::DirPlugin::update_volume_attribute($class, @_);
> }
>
> 1;
> diff --git a/PVE/Storage/CephFSPlugin.pm b/PVE/Storage/CephFSPlugin.pm
> index 3b9a791..2c06804 100644
> --- a/PVE/Storage/CephFSPlugin.pm
> +++ b/PVE/Storage/CephFSPlugin.pm
> @@ -232,14 +232,16 @@ sub deactivate_storage {
> }
> }
>
> -sub get_volume_notes {
> +sub get_volume_attribute {
> my $class = shift;
> - PVE::Storage::DirPlugin::get_volume_notes($class, @_);
> +
> + return PVE::Storage::DirPlugin::get_volume_attribute($class, @_);
> }
>
> -sub update_volume_notes {
> +sub update_volume_attribute {
> my $class = shift;
> - PVE::Storage::DirPlugin::update_volume_notes($class, @_);
> +
> + return PVE::Storage::DirPlugin::update_volume_attribute($class, @_);
> }
>
> 1;
> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
> index 0423e5f..6ab9ef2 100644
> --- a/PVE/Storage/DirPlugin.pm
> +++ b/PVE/Storage/DirPlugin.pm
> @@ -87,32 +87,43 @@ sub parse_is_mountpoint {
> return $is_mp; # contains a path
> }
>
> -sub get_volume_notes {
> - my ($class, $scfg, $storeid, $volname, $timeout) = @_;
> +sub get_volume_attribute {
> + my ($class, $scfg, $storeid, $volname, $attribute) = @_;
>
> - my $path = $class->filesystem_path($scfg, $volname);
> - $path .= $class->SUPER::NOTES_EXT;
> + my ($vtype) = $class->parse_volname($volname);
> + return if $vtype ne 'backup';
>
> - return PVE::Tools::file_get_contents($path) if -f $path;
> + if ($attribute eq 'notes') {
> + my $path = $class->filesystem_path($scfg, $volname);
> + $path .= $class->SUPER::NOTES_EXT;
>
> - return '';
> -}
> + return PVE::Tools::file_get_contents($path) if -f $path;
>
> -sub update_volume_notes {
> - my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
> + return '';
> + }
>
> - my ($vtype) = $class->parse_volname($volname);
> - die "only backups can have notes\n" if $vtype ne 'backup';
> + return;
> +}
>
> - my $path = $class->filesystem_path($scfg, $volname);
> - $path .= $class->SUPER::NOTES_EXT;
> +sub update_volume_attribute {
> + my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
>
> - if (defined($notes) && $notes ne '') {
> - PVE::Tools::file_set_contents($path, $notes);
> - } elsif (-e $path) {
> - unlink $path or die "could not delete notes - $!\n";
> + my ($vtype) = $class->parse_volname($volname);
> + die "only backups support attribute '$attribute'\n" if $vtype ne 'backup';
> +
> + if ($attribute eq 'notes') {
> + my $path = $class->filesystem_path($scfg, $volname);
> + $path .= $class->SUPER::NOTES_EXT;
> +
> + if (defined($value) && $value ne '') {
> + PVE::Tools::file_set_contents($path, $value);
> + } elsif (-e $path) {
> + unlink $path or die "could not delete notes - $!\n";
> + }
> + return;
> }
> - return;
> +
> + die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n";
> }
>
> sub status {
> diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
> index 39bf15a..3a6efe5 100644
> --- a/PVE/Storage/NFSPlugin.pm
> +++ b/PVE/Storage/NFSPlugin.pm
> @@ -188,13 +188,15 @@ sub check_connection {
> return 1;
> }
>
> -sub get_volume_notes {
> +sub get_volume_attribute {
> my $class = shift;
> - PVE::Storage::DirPlugin::get_volume_notes($class, @_);
> +
> + return PVE::Storage::DirPlugin::get_volume_attribute($class, @_);
> }
> -sub update_volume_notes {
> +sub update_volume_attribute {
> my $class = shift;
> - PVE::Storage::DirPlugin::update_volume_notes($class, @_);
> +
> + return PVE::Storage::DirPlugin::update_volume_attribute($class, @_);
> }
>
> 1;
> diff --git a/PVE/Storage/PBSPlugin.pm b/PVE/Storage/PBSPlugin.pm
> index bb1c382..d8e1ac8 100644
> --- a/PVE/Storage/PBSPlugin.pm
> +++ b/PVE/Storage/PBSPlugin.pm
> @@ -782,24 +782,32 @@ sub deactivate_volume {
> return 1;
> }
>
> -sub get_volume_notes {
> - my ($class, $scfg, $storeid, $volname, $timeout) = @_;
> +sub get_volume_attribute {
> + my ($class, $scfg, $storeid, $volname, $attribute) = @_;
> +
> + if ($attribute eq 'notes') {
> + my (undef, $name, undef, undef, undef, undef, $format) = $class->parse_volname($volname);
>
> - my (undef, $name, undef, undef, undef, undef, $format) = $class->parse_volname($volname);
> + my $data = run_client_cmd($scfg, $storeid, "snapshot", [ "notes", "show", $name ]);
>
> - my $data = run_client_cmd($scfg, $storeid, "snapshot", [ "notes", "show", $name ]);
> + return $data->{notes} // '';
> + }
>
> - return $data->{notes};
> + return;
> }
>
> -sub update_volume_notes {
> - my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
> +sub update_volume_attribute {
> + my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
>
> - my (undef, $name, undef, undef, undef, undef, $format) = $class->parse_volname($volname);
> + if ($attribute eq 'notes') {
> + my (undef, $name, undef, undef, undef, undef, $format) = $class->parse_volname($volname);
>
> - run_client_cmd($scfg, $storeid, "snapshot", [ "notes", "update", $name, $notes ], 1);
> + run_client_cmd($scfg, $storeid, "snapshot", [ "notes", "update", $name, $value ], 1);
>
> - return undef;
> + return;
> + }
> +
> + die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n";
> }
>
> sub volume_size_info {
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 417d1fd..1182008 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -851,18 +851,52 @@ sub file_size_info {
> return wantarray ? ($size, $format, $used, $parent, $st->ctime) : $size;
> }
>
> +# FIXME remove on the next APIAGE reset.
> +# Deprecated, use get_volume_attribute instead.
> sub get_volume_notes {
> my ($class, $scfg, $storeid, $volname, $timeout) = @_;
>
> die "volume notes are not supported for $class";
> }
>
> +# FIXME remove on the next APIAGE reset.
> +# Deprecated, use update_volume_attribute instead.
> sub update_volume_notes {
> my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
>
> die "volume notes are not supported for $class";
> }
>
> +# Returns undef if the attribute is not supported for the volume.
> +# Should die if there is an error fetching the attribute.
> +# Possible attributes:
> +# notes - user-provided comments/notes.
> +sub get_volume_attribute {
> + my ($class, $scfg, $storeid, $volname, $attribute) = @_;
> +
> + if ($attribute eq 'notes') {
> + my $notes = eval { $class->get_volume_notes($scfg, $storeid, $volname); };
> + if (my $err = $@) {
> + return if $err =~ m/^volume notes are not supported/;
> + die $err;
> + }
> + return $notes;
> + }
> +
> + return;
> +}
> +
> +# Dies if the attribute is not supported for the volume.
> +sub update_volume_attribute {
> + my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
> +
> + if ($attribute eq 'notes') {
> + $class->update_volume_notes($scfg, $storeid, $volname, $value);
> + }
> +
> + die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n";
> +}
> +
> sub volume_size_info {
> my ($class, $scfg, $storeid, $volname, $timeout) = @_;
> my $path = $class->filesystem_path($scfg, $volname);
> @@ -1098,6 +1132,8 @@ my $get_subdir_files = sub {
> return $res;
> };
>
> +# If attributes are set on a volume, they should be included in the result.
> +# See get_volume_attribute for a list of possible attributes.
> sub list_volumes {
> my ($class, $storeid, $scfg, $vmid, $content_types) = @_;
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pve-devel] [RFC storage 2/6] add generalized functions to manage volume attributes
2021-09-24 8:54 ` Dominik Csapak
@ 2021-09-24 11:05 ` Fabian Ebner
2021-09-24 11:16 ` Dominik Csapak
0 siblings, 1 reply; 20+ messages in thread
From: Fabian Ebner @ 2021-09-24 11:05 UTC (permalink / raw)
To: Dominik Csapak, Proxmox VE development discussion
Am 24.09.21 um 10:54 schrieb Dominik Csapak:
> On 9/17/21 15:02, Fabian Ebner wrote:
>> replacing the ones for handling notes. The generic implementation in
>> Plugin.pm will fall back to the methods for notes to ensure backwards
>> compatibility with external plugins.
>>
>> This is mainly done to avoid the need to add new methods every time a
>> new attribute is added.
>>
>> Not adding a timeout parameter like the notes functions have, because
>> it was not used and can still be added if it ever is needed in the
>> future.
>>
>> For get_volume_attribute, undef will indicate that the attribute is
>> not supported. This makes it possible to distinguish "not supported"
>> from "error getting the attribute", which is useful when the attribute
>> is important for an operation. For example, free_image checking for
>> protection (introduced in a later patch) can abort if getting the
>> 'protected' attribute fails.
>>
>> Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>>
>> Hope I didn't miss a way this would break external plugins.
>
> i think it would break a plugin that is not derived from
> the bas Plugin.pm no?
>
> an example:
>
> i have a CustomCIFSPlugin that uses CIFSPlugin as base
>
> i have implemented a custom 'get_volume_notes' function
>
> after this patch, 'get_volume_attribute' will be called, which
> falls back to the CIFSPlugin versionwhich calls the DirPlugin
> 'get_volume_attribute' instead of my custom 'get_volume_notes'
>
Right. Having the fall-back only in Plugin.pm would not be enough then.
> since we did not document the custom plugins very well, i am
> not sure if that's even a supported scenario, or if a custom
> plugin always have to use the 'Plugin' as a base
>
> if it is supported, we could only solve it by either make
> it a breaking api change, or by keeping the '*_volume_notes'
> around and fall back to *_volume_attribute implmentation
>
That wouldn't solve the problem described above, would it? You still
follow the same call-path and land in DirPlugin's get_volume_attribute
in the end.
Yes, it would be necessary to keep the *_volume_notes functions around,
but I think you need to have the opposite fall-back, i.e.
*_volume_attribute has to call *_volume_notes to catch derived
implementations like Plugin.pm.
> or did i make a mistake anywhere in my assumptions?
>
>>
>> Requires an APIAGE+APIVER bump.
>>
>> The alternative to undef indicating "not supported" would be to add a
>> separate method like supports_volume_attribute. Or adapt
>> volume_has_feature, but it doesn't really feel like the right fit as
>> it's currently intended for features for guest images only.
>>
>> PVE/API2/Storage/Content.pm | 7 +++---
>> PVE/Storage.pm | 12 +++++-----
>> PVE/Storage/BTRFSPlugin.pm | 4 ++--
>> PVE/Storage/CIFSPlugin.pm | 10 ++++----
>> PVE/Storage/CephFSPlugin.pm | 10 ++++----
>> PVE/Storage/DirPlugin.pm | 47 +++++++++++++++++++++++--------------
>> PVE/Storage/NFSPlugin.pm | 10 ++++----
>> PVE/Storage/PBSPlugin.pm | 28 ++++++++++++++--------
>> PVE/Storage/Plugin.pm | 36 ++++++++++++++++++++++++++++
>> 9 files changed, 113 insertions(+), 51 deletions(-)
>>
>> diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
>> index 4d0ceb6..b3dc593 100644
>> --- a/PVE/API2/Storage/Content.pm
>> +++ b/PVE/API2/Storage/Content.pm
>> @@ -321,11 +321,12 @@ __PACKAGE__->register_method ({
>> format => $format,
>> };
>> - # not all storages/types support notes, so ignore errors here
>> + # keep going if fetching an optional attribute fails
>> eval {
>> - my $notes = PVE::Storage::get_volume_notes($cfg, $volid);
>> + my $notes = PVE::Storage::get_volume_attribute($cfg, $volid,
>> 'notes');
>> $entry->{notes} = $notes if defined($notes);
>> };
>> + warn $@ if $@;
>> return $entry;
>> }});
>> @@ -371,7 +372,7 @@ __PACKAGE__->register_method ({
>> PVE::Storage::check_volume_access($rpcenv, $authuser, $cfg,
>> undef, $volid);
>> if (exists $param->{notes}) {
>> - PVE::Storage::update_volume_notes($cfg, $volid,
>> $param->{notes});
>> + PVE::Storage::update_volume_attribute($cfg, $volid, 'notes',
>> $param->{notes});
>> }
>> return undef;
>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>> index 7b319fe..729c90e 100755
>> --- a/PVE/Storage.pm
>> +++ b/PVE/Storage.pm
>> @@ -215,24 +215,24 @@ sub file_size_info {
>> return PVE::Storage::Plugin::file_size_info($filename, $timeout);
>> }
>> -sub get_volume_notes {
>> - my ($cfg, $volid, $timeout) = @_;
>> +sub get_volume_attribute {
>> + my ($cfg, $volid, $attribute) = @_;
>> my ($storeid, $volname) = parse_volume_id($volid);
>> my $scfg = storage_config($cfg, $storeid);
>> my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>> - return $plugin->get_volume_notes($scfg, $storeid, $volname,
>> $timeout);
>> + return $plugin->get_volume_attribute($scfg, $storeid, $volname,
>> $attribute);
>> }
>> -sub update_volume_notes {
>> - my ($cfg, $volid, $notes, $timeout) = @_;
>> +sub update_volume_attribute {
>> + my ($cfg, $volid, $attribute, $value) = @_;
>> my ($storeid, $volname) = parse_volume_id($volid);
>> my $scfg = storage_config($cfg, $storeid);
>> my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>> - $plugin->update_volume_notes($scfg, $storeid, $volname, $notes,
>> $timeout);
>> + return $plugin->update_volume_attribute($scfg, $storeid,
>> $volname, $attribute, $value);
>> }
>> sub volume_size_info {
>> diff --git a/PVE/Storage/BTRFSPlugin.pm b/PVE/Storage/BTRFSPlugin.pm
>> index dbc1244..61bede2 100644
>> --- a/PVE/Storage/BTRFSPlugin.pm
>> +++ b/PVE/Storage/BTRFSPlugin.pm
>> @@ -136,9 +136,9 @@ sub status {
>> return PVE::Storage::DirPlugin::status($class, $storeid, $scfg,
>> $cache);
>> }
>> -# TODO: sub get_volume_notes {}
>> +# TODO: sub get_volume_attribute {}
>> -# TODO: sub update_volume_notes {}
>> +# TODO: sub update_volume_attribute {}
>> # croak would not include the caller from within this module
>> sub __error {
>> diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
>> index c5f3894..bee6885 100644
>> --- a/PVE/Storage/CIFSPlugin.pm
>> +++ b/PVE/Storage/CIFSPlugin.pm
>> @@ -286,13 +286,15 @@ sub check_connection {
>> return 1;
>> }
>> -sub get_volume_notes {
>> +sub get_volume_attribute {
>> my $class = shift;
>> - PVE::Storage::DirPlugin::get_volume_notes($class, @_);
>> +
>> + return PVE::Storage::DirPlugin::get_volume_attribute($class, @_);
>> }
>> -sub update_volume_notes {
>> +sub update_volume_attribute {
>> my $class = shift;
>> - PVE::Storage::DirPlugin::update_volume_notes($class, @_);
>> +
>> + return PVE::Storage::DirPlugin::update_volume_attribute($class, @_);
>> }
>> 1;
>> diff --git a/PVE/Storage/CephFSPlugin.pm b/PVE/Storage/CephFSPlugin.pm
>> index 3b9a791..2c06804 100644
>> --- a/PVE/Storage/CephFSPlugin.pm
>> +++ b/PVE/Storage/CephFSPlugin.pm
>> @@ -232,14 +232,16 @@ sub deactivate_storage {
>> }
>> }
>> -sub get_volume_notes {
>> +sub get_volume_attribute {
>> my $class = shift;
>> - PVE::Storage::DirPlugin::get_volume_notes($class, @_);
>> +
>> + return PVE::Storage::DirPlugin::get_volume_attribute($class, @_);
>> }
>> -sub update_volume_notes {
>> +sub update_volume_attribute {
>> my $class = shift;
>> - PVE::Storage::DirPlugin::update_volume_notes($class, @_);
>> +
>> + return PVE::Storage::DirPlugin::update_volume_attribute($class, @_);
>> }
>> 1;
>> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
>> index 0423e5f..6ab9ef2 100644
>> --- a/PVE/Storage/DirPlugin.pm
>> +++ b/PVE/Storage/DirPlugin.pm
>> @@ -87,32 +87,43 @@ sub parse_is_mountpoint {
>> return $is_mp; # contains a path
>> }
>> -sub get_volume_notes {
>> - my ($class, $scfg, $storeid, $volname, $timeout) = @_;
>> +sub get_volume_attribute {
>> + my ($class, $scfg, $storeid, $volname, $attribute) = @_;
>> - my $path = $class->filesystem_path($scfg, $volname);
>> - $path .= $class->SUPER::NOTES_EXT;
>> + my ($vtype) = $class->parse_volname($volname);
>> + return if $vtype ne 'backup';
>> - return PVE::Tools::file_get_contents($path) if -f $path;
>> + if ($attribute eq 'notes') {
>> + my $path = $class->filesystem_path($scfg, $volname);
>> + $path .= $class->SUPER::NOTES_EXT;
>> - return '';
>> -}
>> + return PVE::Tools::file_get_contents($path) if -f $path;
>> -sub update_volume_notes {
>> - my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
>> + return '';
>> + }
>> - my ($vtype) = $class->parse_volname($volname);
>> - die "only backups can have notes\n" if $vtype ne 'backup';
>> + return;
>> +}
>> - my $path = $class->filesystem_path($scfg, $volname);
>> - $path .= $class->SUPER::NOTES_EXT;
>> +sub update_volume_attribute {
>> + my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
>> - if (defined($notes) && $notes ne '') {
>> - PVE::Tools::file_set_contents($path, $notes);
>> - } elsif (-e $path) {
>> - unlink $path or die "could not delete notes - $!\n";
>> + my ($vtype) = $class->parse_volname($volname);
>> + die "only backups support attribute '$attribute'\n" if $vtype ne
>> 'backup';
>> +
>> + if ($attribute eq 'notes') {
>> + my $path = $class->filesystem_path($scfg, $volname);
>> + $path .= $class->SUPER::NOTES_EXT;
>> +
>> + if (defined($value) && $value ne '') {
>> + PVE::Tools::file_set_contents($path, $value);
>> + } elsif (-e $path) {
>> + unlink $path or die "could not delete notes - $!\n";
>> + }
>> + return;
>> }
>> - return;
>> +
>> + die "attribute '$attribute' is not supported for storage type
>> '$scfg->{type}'\n";
>> }
>> sub status {
>> diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
>> index 39bf15a..3a6efe5 100644
>> --- a/PVE/Storage/NFSPlugin.pm
>> +++ b/PVE/Storage/NFSPlugin.pm
>> @@ -188,13 +188,15 @@ sub check_connection {
>> return 1;
>> }
>> -sub get_volume_notes {
>> +sub get_volume_attribute {
>> my $class = shift;
>> - PVE::Storage::DirPlugin::get_volume_notes($class, @_);
>> +
>> + return PVE::Storage::DirPlugin::get_volume_attribute($class, @_);
>> }
>> -sub update_volume_notes {
>> +sub update_volume_attribute {
>> my $class = shift;
>> - PVE::Storage::DirPlugin::update_volume_notes($class, @_);
>> +
>> + return PVE::Storage::DirPlugin::update_volume_attribute($class, @_);
>> }
>> 1;
>> diff --git a/PVE/Storage/PBSPlugin.pm b/PVE/Storage/PBSPlugin.pm
>> index bb1c382..d8e1ac8 100644
>> --- a/PVE/Storage/PBSPlugin.pm
>> +++ b/PVE/Storage/PBSPlugin.pm
>> @@ -782,24 +782,32 @@ sub deactivate_volume {
>> return 1;
>> }
>> -sub get_volume_notes {
>> - my ($class, $scfg, $storeid, $volname, $timeout) = @_;
>> +sub get_volume_attribute {
>> + my ($class, $scfg, $storeid, $volname, $attribute) = @_;
>> +
>> + if ($attribute eq 'notes') {
>> + my (undef, $name, undef, undef, undef, undef, $format) =
>> $class->parse_volname($volname);
>> - my (undef, $name, undef, undef, undef, undef, $format) =
>> $class->parse_volname($volname);
>> + my $data = run_client_cmd($scfg, $storeid, "snapshot", [ "notes",
>> "show", $name ]);
>> - my $data = run_client_cmd($scfg, $storeid, "snapshot", [ "notes",
>> "show", $name ]);
>> + return $data->{notes} // '';
>> + }
>> - return $data->{notes};
>> + return;
>> }
>> -sub update_volume_notes {
>> - my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
>> +sub update_volume_attribute {
>> + my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
>> - my (undef, $name, undef, undef, undef, undef, $format) =
>> $class->parse_volname($volname);
>> + if ($attribute eq 'notes') {
>> + my (undef, $name, undef, undef, undef, undef, $format) =
>> $class->parse_volname($volname);
>> - run_client_cmd($scfg, $storeid, "snapshot", [ "notes", "update",
>> $name, $notes ], 1);
>> + run_client_cmd($scfg, $storeid, "snapshot", [ "notes", "update",
>> $name, $value ], 1);
>> - return undef;
>> + return;
>> + }
>> +
>> + die "attribute '$attribute' is not supported for storage type
>> '$scfg->{type}'\n";
>> }
>> sub volume_size_info {
>> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
>> index 417d1fd..1182008 100644
>> --- a/PVE/Storage/Plugin.pm
>> +++ b/PVE/Storage/Plugin.pm
>> @@ -851,18 +851,52 @@ sub file_size_info {
>> return wantarray ? ($size, $format, $used, $parent, $st->ctime)
>> : $size;
>> }
>> +# FIXME remove on the next APIAGE reset.
>> +# Deprecated, use get_volume_attribute instead.
>> sub get_volume_notes {
>> my ($class, $scfg, $storeid, $volname, $timeout) = @_;
>> die "volume notes are not supported for $class";
>> }
>> +# FIXME remove on the next APIAGE reset.
>> +# Deprecated, use update_volume_attribute instead.
>> sub update_volume_notes {
>> my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
>> die "volume notes are not supported for $class";
>> }
>> +# Returns undef if the attribute is not supported for the volume.
>> +# Should die if there is an error fetching the attribute.
>> +# Possible attributes:
>> +# notes - user-provided comments/notes.
>> +sub get_volume_attribute {
>> + my ($class, $scfg, $storeid, $volname, $attribute) = @_;
>> +
>> + if ($attribute eq 'notes') {
>> + my $notes = eval { $class->get_volume_notes($scfg, $storeid,
>> $volname); };
>> + if (my $err = $@) {
>> + return if $err =~ m/^volume notes are not supported/;
>> + die $err;
>> + }
>> + return $notes;
>> + }
>> +
>> + return;
>> +}
>> +
>> +# Dies if the attribute is not supported for the volume.
>> +sub update_volume_attribute {
>> + my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
>> +
>> + if ($attribute eq 'notes') {
>> + $class->update_volume_notes($scfg, $storeid, $volname, $value);
>> + }
>> +
>> + die "attribute '$attribute' is not supported for storage type
>> '$scfg->{type}'\n";
>> +}
>> +
>> sub volume_size_info {
>> my ($class, $scfg, $storeid, $volname, $timeout) = @_;
>> my $path = $class->filesystem_path($scfg, $volname);
>> @@ -1098,6 +1132,8 @@ my $get_subdir_files = sub {
>> return $res;
>> };
>> +# If attributes are set on a volume, they should be included in the
>> result.
>> +# See get_volume_attribute for a list of possible attributes.
>> sub list_volumes {
>> my ($class, $storeid, $scfg, $vmid, $content_types) = @_;
>>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pve-devel] [RFC storage 2/6] add generalized functions to manage volume attributes
2021-09-24 11:05 ` Fabian Ebner
@ 2021-09-24 11:16 ` Dominik Csapak
2021-09-24 11:31 ` Fabian Ebner
0 siblings, 1 reply; 20+ messages in thread
From: Dominik Csapak @ 2021-09-24 11:16 UTC (permalink / raw)
To: Fabian Ebner, Proxmox VE development discussion
On 9/24/21 13:05, Fabian Ebner wrote:
> Am 24.09.21 um 10:54 schrieb Dominik Csapak:
>> On 9/17/21 15:02, Fabian Ebner wrote:
>>> replacing the ones for handling notes. The generic implementation in
>>> Plugin.pm will fall back to the methods for notes to ensure backwards
>>> compatibility with external plugins.
>>>
>>> This is mainly done to avoid the need to add new methods every time a
>>> new attribute is added.
>>>
>>> Not adding a timeout parameter like the notes functions have, because
>>> it was not used and can still be added if it ever is needed in the
>>> future.
>>>
>>> For get_volume_attribute, undef will indicate that the attribute is
>>> not supported. This makes it possible to distinguish "not supported"
>>> from "error getting the attribute", which is useful when the attribute
>>> is important for an operation. For example, free_image checking for
>>> protection (introduced in a later patch) can abort if getting the
>>> 'protected' attribute fails.
>>>
>>> Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
>>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>>> ---
>>>
>>> Hope I didn't miss a way this would break external plugins.
>>
>> i think it would break a plugin that is not derived from
>> the bas Plugin.pm no?
>>
>> an example:
>>
>> i have a CustomCIFSPlugin that uses CIFSPlugin as base
>>
>> i have implemented a custom 'get_volume_notes' function
>>
>> after this patch, 'get_volume_attribute' will be called, which
>> falls back to the CIFSPlugin versionwhich calls the DirPlugin
>> 'get_volume_attribute' instead of my custom 'get_volume_notes'
>>
>
> Right. Having the fall-back only in Plugin.pm would not be enough then.
>
>> since we did not document the custom plugins very well, i am
>> not sure if that's even a supported scenario, or if a custom
>> plugin always have to use the 'Plugin' as a base
>>
>> if it is supported, we could only solve it by either make
>> it a breaking api change, or by keeping the '*_volume_notes'
>> around and fall back to *_volume_attribute implmentation
>>
>
> That wouldn't solve the problem described above, would it? You still
> follow the same call-path and land in DirPlugin's get_volume_attribute
> in the end.
>
> Yes, it would be necessary to keep the *_volume_notes functions around,
> but I think you need to have the opposite fall-back, i.e.
> *_volume_attribute has to call *_volume_notes to catch derived
> implementations like Plugin.pm.
i meant in e.g. Content.pm that would call 'get_volume_notes'.
this would call the correct one (if it exists)
and a default which calls 'get_volume_attribute'
would catch all plugins that have no 'get_volume_notes' ?
but i think it could work both ways, though i like your suggested
approach more since we use the new sub everywhere outside the plugins
>
>> or did i make a mistake anywhere in my assumptions?
>>
>>>
>>> Requires an APIAGE+APIVER bump.
>>>
>>> The alternative to undef indicating "not supported" would be to add a
>>> separate method like supports_volume_attribute. Or adapt
>>> volume_has_feature, but it doesn't really feel like the right fit as
>>> it's currently intended for features for guest images only.
>>>
>>> PVE/API2/Storage/Content.pm | 7 +++---
>>> PVE/Storage.pm | 12 +++++-----
>>> PVE/Storage/BTRFSPlugin.pm | 4 ++--
>>> PVE/Storage/CIFSPlugin.pm | 10 ++++----
>>> PVE/Storage/CephFSPlugin.pm | 10 ++++----
>>> PVE/Storage/DirPlugin.pm | 47 +++++++++++++++++++++++--------------
>>> PVE/Storage/NFSPlugin.pm | 10 ++++----
>>> PVE/Storage/PBSPlugin.pm | 28 ++++++++++++++--------
>>> PVE/Storage/Plugin.pm | 36 ++++++++++++++++++++++++++++
>>> 9 files changed, 113 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
>>> index 4d0ceb6..b3dc593 100644
>>> --- a/PVE/API2/Storage/Content.pm
>>> +++ b/PVE/API2/Storage/Content.pm
>>> @@ -321,11 +321,12 @@ __PACKAGE__->register_method ({
>>> format => $format,
>>> };
>>> - # not all storages/types support notes, so ignore errors here
>>> + # keep going if fetching an optional attribute fails
>>> eval {
>>> - my $notes = PVE::Storage::get_volume_notes($cfg, $volid);
>>> + my $notes = PVE::Storage::get_volume_attribute($cfg, $volid,
>>> 'notes');
>>> $entry->{notes} = $notes if defined($notes);
>>> };
>>> + warn $@ if $@;
>>> return $entry;
>>> }});
>>> @@ -371,7 +372,7 @@ __PACKAGE__->register_method ({
>>> PVE::Storage::check_volume_access($rpcenv, $authuser, $cfg,
>>> undef, $volid);
>>> if (exists $param->{notes}) {
>>> - PVE::Storage::update_volume_notes($cfg, $volid,
>>> $param->{notes});
>>> + PVE::Storage::update_volume_attribute($cfg, $volid, 'notes',
>>> $param->{notes});
>>> }
>>> return undef;
>>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>>> index 7b319fe..729c90e 100755
>>> --- a/PVE/Storage.pm
>>> +++ b/PVE/Storage.pm
>>> @@ -215,24 +215,24 @@ sub file_size_info {
>>> return PVE::Storage::Plugin::file_size_info($filename, $timeout);
>>> }
>>> -sub get_volume_notes {
>>> - my ($cfg, $volid, $timeout) = @_;
>>> +sub get_volume_attribute {
>>> + my ($cfg, $volid, $attribute) = @_;
>>> my ($storeid, $volname) = parse_volume_id($volid);
>>> my $scfg = storage_config($cfg, $storeid);
>>> my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>>> - return $plugin->get_volume_notes($scfg, $storeid, $volname,
>>> $timeout);
>>> + return $plugin->get_volume_attribute($scfg, $storeid, $volname,
>>> $attribute);
>>> }
>>> -sub update_volume_notes {
>>> - my ($cfg, $volid, $notes, $timeout) = @_;
>>> +sub update_volume_attribute {
>>> + my ($cfg, $volid, $attribute, $value) = @_;
>>> my ($storeid, $volname) = parse_volume_id($volid);
>>> my $scfg = storage_config($cfg, $storeid);
>>> my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>>> - $plugin->update_volume_notes($scfg, $storeid, $volname, $notes,
>>> $timeout);
>>> + return $plugin->update_volume_attribute($scfg, $storeid,
>>> $volname, $attribute, $value);
>>> }
>>> sub volume_size_info {
>>> diff --git a/PVE/Storage/BTRFSPlugin.pm b/PVE/Storage/BTRFSPlugin.pm
>>> index dbc1244..61bede2 100644
>>> --- a/PVE/Storage/BTRFSPlugin.pm
>>> +++ b/PVE/Storage/BTRFSPlugin.pm
>>> @@ -136,9 +136,9 @@ sub status {
>>> return PVE::Storage::DirPlugin::status($class, $storeid, $scfg,
>>> $cache);
>>> }
>>> -# TODO: sub get_volume_notes {}
>>> +# TODO: sub get_volume_attribute {}
>>> -# TODO: sub update_volume_notes {}
>>> +# TODO: sub update_volume_attribute {}
>>> # croak would not include the caller from within this module
>>> sub __error {
>>> diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
>>> index c5f3894..bee6885 100644
>>> --- a/PVE/Storage/CIFSPlugin.pm
>>> +++ b/PVE/Storage/CIFSPlugin.pm
>>> @@ -286,13 +286,15 @@ sub check_connection {
>>> return 1;
>>> }
>>> -sub get_volume_notes {
>>> +sub get_volume_attribute {
>>> my $class = shift;
>>> - PVE::Storage::DirPlugin::get_volume_notes($class, @_);
>>> +
>>> + return PVE::Storage::DirPlugin::get_volume_attribute($class, @_);
>>> }
>>> -sub update_volume_notes {
>>> +sub update_volume_attribute {
>>> my $class = shift;
>>> - PVE::Storage::DirPlugin::update_volume_notes($class, @_);
>>> +
>>> + return PVE::Storage::DirPlugin::update_volume_attribute($class,
>>> @_);
>>> }
>>> 1;
>>> diff --git a/PVE/Storage/CephFSPlugin.pm b/PVE/Storage/CephFSPlugin.pm
>>> index 3b9a791..2c06804 100644
>>> --- a/PVE/Storage/CephFSPlugin.pm
>>> +++ b/PVE/Storage/CephFSPlugin.pm
>>> @@ -232,14 +232,16 @@ sub deactivate_storage {
>>> }
>>> }
>>> -sub get_volume_notes {
>>> +sub get_volume_attribute {
>>> my $class = shift;
>>> - PVE::Storage::DirPlugin::get_volume_notes($class, @_);
>>> +
>>> + return PVE::Storage::DirPlugin::get_volume_attribute($class, @_);
>>> }
>>> -sub update_volume_notes {
>>> +sub update_volume_attribute {
>>> my $class = shift;
>>> - PVE::Storage::DirPlugin::update_volume_notes($class, @_);
>>> +
>>> + return PVE::Storage::DirPlugin::update_volume_attribute($class,
>>> @_);
>>> }
>>> 1;
>>> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
>>> index 0423e5f..6ab9ef2 100644
>>> --- a/PVE/Storage/DirPlugin.pm
>>> +++ b/PVE/Storage/DirPlugin.pm
>>> @@ -87,32 +87,43 @@ sub parse_is_mountpoint {
>>> return $is_mp; # contains a path
>>> }
>>> -sub get_volume_notes {
>>> - my ($class, $scfg, $storeid, $volname, $timeout) = @_;
>>> +sub get_volume_attribute {
>>> + my ($class, $scfg, $storeid, $volname, $attribute) = @_;
>>> - my $path = $class->filesystem_path($scfg, $volname);
>>> - $path .= $class->SUPER::NOTES_EXT;
>>> + my ($vtype) = $class->parse_volname($volname);
>>> + return if $vtype ne 'backup';
>>> - return PVE::Tools::file_get_contents($path) if -f $path;
>>> + if ($attribute eq 'notes') {
>>> + my $path = $class->filesystem_path($scfg, $volname);
>>> + $path .= $class->SUPER::NOTES_EXT;
>>> - return '';
>>> -}
>>> + return PVE::Tools::file_get_contents($path) if -f $path;
>>> -sub update_volume_notes {
>>> - my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
>>> + return '';
>>> + }
>>> - my ($vtype) = $class->parse_volname($volname);
>>> - die "only backups can have notes\n" if $vtype ne 'backup';
>>> + return;
>>> +}
>>> - my $path = $class->filesystem_path($scfg, $volname);
>>> - $path .= $class->SUPER::NOTES_EXT;
>>> +sub update_volume_attribute {
>>> + my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
>>> - if (defined($notes) && $notes ne '') {
>>> - PVE::Tools::file_set_contents($path, $notes);
>>> - } elsif (-e $path) {
>>> - unlink $path or die "could not delete notes - $!\n";
>>> + my ($vtype) = $class->parse_volname($volname);
>>> + die "only backups support attribute '$attribute'\n" if $vtype ne
>>> 'backup';
>>> +
>>> + if ($attribute eq 'notes') {
>>> + my $path = $class->filesystem_path($scfg, $volname);
>>> + $path .= $class->SUPER::NOTES_EXT;
>>> +
>>> + if (defined($value) && $value ne '') {
>>> + PVE::Tools::file_set_contents($path, $value);
>>> + } elsif (-e $path) {
>>> + unlink $path or die "could not delete notes - $!\n";
>>> + }
>>> + return;
>>> }
>>> - return;
>>> +
>>> + die "attribute '$attribute' is not supported for storage type
>>> '$scfg->{type}'\n";
>>> }
>>> sub status {
>>> diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
>>> index 39bf15a..3a6efe5 100644
>>> --- a/PVE/Storage/NFSPlugin.pm
>>> +++ b/PVE/Storage/NFSPlugin.pm
>>> @@ -188,13 +188,15 @@ sub check_connection {
>>> return 1;
>>> }
>>> -sub get_volume_notes {
>>> +sub get_volume_attribute {
>>> my $class = shift;
>>> - PVE::Storage::DirPlugin::get_volume_notes($class, @_);
>>> +
>>> + return PVE::Storage::DirPlugin::get_volume_attribute($class, @_);
>>> }
>>> -sub update_volume_notes {
>>> +sub update_volume_attribute {
>>> my $class = shift;
>>> - PVE::Storage::DirPlugin::update_volume_notes($class, @_);
>>> +
>>> + return PVE::Storage::DirPlugin::update_volume_attribute($class,
>>> @_);
>>> }
>>> 1;
>>> diff --git a/PVE/Storage/PBSPlugin.pm b/PVE/Storage/PBSPlugin.pm
>>> index bb1c382..d8e1ac8 100644
>>> --- a/PVE/Storage/PBSPlugin.pm
>>> +++ b/PVE/Storage/PBSPlugin.pm
>>> @@ -782,24 +782,32 @@ sub deactivate_volume {
>>> return 1;
>>> }
>>> -sub get_volume_notes {
>>> - my ($class, $scfg, $storeid, $volname, $timeout) = @_;
>>> +sub get_volume_attribute {
>>> + my ($class, $scfg, $storeid, $volname, $attribute) = @_;
>>> +
>>> + if ($attribute eq 'notes') {
>>> + my (undef, $name, undef, undef, undef, undef, $format) =
>>> $class->parse_volname($volname);
>>> - my (undef, $name, undef, undef, undef, undef, $format) =
>>> $class->parse_volname($volname);
>>> + my $data = run_client_cmd($scfg, $storeid, "snapshot", [
>>> "notes", "show", $name ]);
>>> - my $data = run_client_cmd($scfg, $storeid, "snapshot", [
>>> "notes", "show", $name ]);
>>> + return $data->{notes} // '';
>>> + }
>>> - return $data->{notes};
>>> + return;
>>> }
>>> -sub update_volume_notes {
>>> - my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
>>> +sub update_volume_attribute {
>>> + my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
>>> - my (undef, $name, undef, undef, undef, undef, $format) =
>>> $class->parse_volname($volname);
>>> + if ($attribute eq 'notes') {
>>> + my (undef, $name, undef, undef, undef, undef, $format) =
>>> $class->parse_volname($volname);
>>> - run_client_cmd($scfg, $storeid, "snapshot", [ "notes", "update",
>>> $name, $notes ], 1);
>>> + run_client_cmd($scfg, $storeid, "snapshot", [ "notes", "update",
>>> $name, $value ], 1);
>>> - return undef;
>>> + return;
>>> + }
>>> +
>>> + die "attribute '$attribute' is not supported for storage type
>>> '$scfg->{type}'\n";
>>> }
>>> sub volume_size_info {
>>> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
>>> index 417d1fd..1182008 100644
>>> --- a/PVE/Storage/Plugin.pm
>>> +++ b/PVE/Storage/Plugin.pm
>>> @@ -851,18 +851,52 @@ sub file_size_info {
>>> return wantarray ? ($size, $format, $used, $parent, $st->ctime)
>>> : $size;
>>> }
>>> +# FIXME remove on the next APIAGE reset.
>>> +# Deprecated, use get_volume_attribute instead.
>>> sub get_volume_notes {
>>> my ($class, $scfg, $storeid, $volname, $timeout) = @_;
>>> die "volume notes are not supported for $class";
>>> }
>>> +# FIXME remove on the next APIAGE reset.
>>> +# Deprecated, use update_volume_attribute instead.
>>> sub update_volume_notes {
>>> my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
>>> die "volume notes are not supported for $class";
>>> }
>>> +# Returns undef if the attribute is not supported for the volume.
>>> +# Should die if there is an error fetching the attribute.
>>> +# Possible attributes:
>>> +# notes - user-provided comments/notes.
>>> +sub get_volume_attribute {
>>> + my ($class, $scfg, $storeid, $volname, $attribute) = @_;
>>> +
>>> + if ($attribute eq 'notes') {
>>> + my $notes = eval { $class->get_volume_notes($scfg, $storeid,
>>> $volname); };
>>> + if (my $err = $@) {
>>> + return if $err =~ m/^volume notes are not supported/;
>>> + die $err;
>>> + }
>>> + return $notes;
>>> + }
>>> +
>>> + return;
>>> +}
>>> +
>>> +# Dies if the attribute is not supported for the volume.
>>> +sub update_volume_attribute {
>>> + my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
>>> +
>>> + if ($attribute eq 'notes') {
>>> + $class->update_volume_notes($scfg, $storeid, $volname, $value);
>>> + }
>>> +
>>> + die "attribute '$attribute' is not supported for storage type
>>> '$scfg->{type}'\n";
>>> +}
>>> +
>>> sub volume_size_info {
>>> my ($class, $scfg, $storeid, $volname, $timeout) = @_;
>>> my $path = $class->filesystem_path($scfg, $volname);
>>> @@ -1098,6 +1132,8 @@ my $get_subdir_files = sub {
>>> return $res;
>>> };
>>> +# If attributes are set on a volume, they should be included in the
>>> result.
>>> +# See get_volume_attribute for a list of possible attributes.
>>> sub list_volumes {
>>> my ($class, $storeid, $scfg, $vmid, $content_types) = @_;
>>>
>>
>>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pve-devel] [RFC storage 2/6] add generalized functions to manage volume attributes
2021-09-24 11:16 ` Dominik Csapak
@ 2021-09-24 11:31 ` Fabian Ebner
0 siblings, 0 replies; 20+ messages in thread
From: Fabian Ebner @ 2021-09-24 11:31 UTC (permalink / raw)
To: Dominik Csapak, Proxmox VE development discussion
Am 24.09.21 um 13:16 schrieb Dominik Csapak:
> On 9/24/21 13:05, Fabian Ebner wrote:
>> Am 24.09.21 um 10:54 schrieb Dominik Csapak:
>>> On 9/17/21 15:02, Fabian Ebner wrote:
>>>> replacing the ones for handling notes. The generic implementation in
>>>> Plugin.pm will fall back to the methods for notes to ensure backwards
>>>> compatibility with external plugins.
>>>>
>>>> This is mainly done to avoid the need to add new methods every time a
>>>> new attribute is added.
>>>>
>>>> Not adding a timeout parameter like the notes functions have, because
>>>> it was not used and can still be added if it ever is needed in the
>>>> future.
>>>>
>>>> For get_volume_attribute, undef will indicate that the attribute is
>>>> not supported. This makes it possible to distinguish "not supported"
>>>> from "error getting the attribute", which is useful when the attribute
>>>> is important for an operation. For example, free_image checking for
>>>> protection (introduced in a later patch) can abort if getting the
>>>> 'protected' attribute fails.
>>>>
>>>> Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
>>>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>>>> ---
>>>>
>>>> Hope I didn't miss a way this would break external plugins.
>>>
>>> i think it would break a plugin that is not derived from
>>> the bas Plugin.pm no?
>>>
>>> an example:
>>>
>>> i have a CustomCIFSPlugin that uses CIFSPlugin as base
>>>
>>> i have implemented a custom 'get_volume_notes' function
>>>
>>> after this patch, 'get_volume_attribute' will be called, which
>>> falls back to the CIFSPlugin versionwhich calls the DirPlugin
>>> 'get_volume_attribute' instead of my custom 'get_volume_notes'
>>>
>>
>> Right. Having the fall-back only in Plugin.pm would not be enough then.
>>
>>> since we did not document the custom plugins very well, i am
>>> not sure if that's even a supported scenario, or if a custom
>>> plugin always have to use the 'Plugin' as a base
>>>
>>> if it is supported, we could only solve it by either make
>>> it a breaking api change, or by keeping the '*_volume_notes'
>>> around and fall back to *_volume_attribute implmentation
>>>
>>
>> That wouldn't solve the problem described above, would it? You still
>> follow the same call-path and land in DirPlugin's get_volume_attribute
>> in the end.
>>
>> Yes, it would be necessary to keep the *_volume_notes functions
>> around, but I think you need to have the opposite fall-back, i.e.
>> *_volume_attribute has to call *_volume_notes to catch derived
>> implementations like Plugin.pm.
>
>
> i meant in e.g. Content.pm that would call 'get_volume_notes'.
> this would call the correct one (if it exists)
> and a default which calls 'get_volume_attribute'
> would catch all plugins that have no 'get_volume_notes' ?
Yes, it works if the outside still calls the old ones, i.e. having a
fallback in Storage.pm's wrapper would make the above work. Then it'd be
Storage.pm get_volume_attribute calls $plugin->get_volume_notes calls
$plugin->get_volume_attribute, with the last call happening if the
Plugin is recent enough to have implemented the fallback.
>
> but i think it could work both ways, though i like your suggested
> approach more since we use the new sub everywhere outside the plugins
>
>>
>>> or did i make a mistake anywhere in my assumptions?
>>>
>>>>
>>>> Requires an APIAGE+APIVER bump.
>>>>
>>>> The alternative to undef indicating "not supported" would be to add a
>>>> separate method like supports_volume_attribute. Or adapt
>>>> volume_has_feature, but it doesn't really feel like the right fit as
>>>> it's currently intended for features for guest images only.
>>>>
>>>> PVE/API2/Storage/Content.pm | 7 +++---
>>>> PVE/Storage.pm | 12 +++++-----
>>>> PVE/Storage/BTRFSPlugin.pm | 4 ++--
>>>> PVE/Storage/CIFSPlugin.pm | 10 ++++----
>>>> PVE/Storage/CephFSPlugin.pm | 10 ++++----
>>>> PVE/Storage/DirPlugin.pm | 47
>>>> +++++++++++++++++++++++--------------
>>>> PVE/Storage/NFSPlugin.pm | 10 ++++----
>>>> PVE/Storage/PBSPlugin.pm | 28 ++++++++++++++--------
>>>> PVE/Storage/Plugin.pm | 36 ++++++++++++++++++++++++++++
>>>> 9 files changed, 113 insertions(+), 51 deletions(-)
>>>>
>>>> diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
>>>> index 4d0ceb6..b3dc593 100644
>>>> --- a/PVE/API2/Storage/Content.pm
>>>> +++ b/PVE/API2/Storage/Content.pm
>>>> @@ -321,11 +321,12 @@ __PACKAGE__->register_method ({
>>>> format => $format,
>>>> };
>>>> - # not all storages/types support notes, so ignore errors here
>>>> + # keep going if fetching an optional attribute fails
>>>> eval {
>>>> - my $notes = PVE::Storage::get_volume_notes($cfg, $volid);
>>>> + my $notes = PVE::Storage::get_volume_attribute($cfg,
>>>> $volid, 'notes');
>>>> $entry->{notes} = $notes if defined($notes);
>>>> };
>>>> + warn $@ if $@;
>>>> return $entry;
>>>> }});
>>>> @@ -371,7 +372,7 @@ __PACKAGE__->register_method ({
>>>> PVE::Storage::check_volume_access($rpcenv, $authuser, $cfg,
>>>> undef, $volid);
>>>> if (exists $param->{notes}) {
>>>> - PVE::Storage::update_volume_notes($cfg, $volid,
>>>> $param->{notes});
>>>> + PVE::Storage::update_volume_attribute($cfg, $volid,
>>>> 'notes', $param->{notes});
>>>> }
>>>> return undef;
>>>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>>>> index 7b319fe..729c90e 100755
>>>> --- a/PVE/Storage.pm
>>>> +++ b/PVE/Storage.pm
>>>> @@ -215,24 +215,24 @@ sub file_size_info {
>>>> return PVE::Storage::Plugin::file_size_info($filename, $timeout);
>>>> }
>>>> -sub get_volume_notes {
>>>> - my ($cfg, $volid, $timeout) = @_;
>>>> +sub get_volume_attribute {
>>>> + my ($cfg, $volid, $attribute) = @_;
>>>> my ($storeid, $volname) = parse_volume_id($volid);
>>>> my $scfg = storage_config($cfg, $storeid);
>>>> my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>>>> - return $plugin->get_volume_notes($scfg, $storeid, $volname,
>>>> $timeout);
>>>> + return $plugin->get_volume_attribute($scfg, $storeid, $volname,
>>>> $attribute);
>>>> }
>>>> -sub update_volume_notes {
>>>> - my ($cfg, $volid, $notes, $timeout) = @_;
>>>> +sub update_volume_attribute {
>>>> + my ($cfg, $volid, $attribute, $value) = @_;
>>>> my ($storeid, $volname) = parse_volume_id($volid);
>>>> my $scfg = storage_config($cfg, $storeid);
>>>> my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>>>> - $plugin->update_volume_notes($scfg, $storeid, $volname, $notes,
>>>> $timeout);
>>>> + return $plugin->update_volume_attribute($scfg, $storeid,
>>>> $volname, $attribute, $value);
>>>> }
>>>> sub volume_size_info {
>>>> diff --git a/PVE/Storage/BTRFSPlugin.pm b/PVE/Storage/BTRFSPlugin.pm
>>>> index dbc1244..61bede2 100644
>>>> --- a/PVE/Storage/BTRFSPlugin.pm
>>>> +++ b/PVE/Storage/BTRFSPlugin.pm
>>>> @@ -136,9 +136,9 @@ sub status {
>>>> return PVE::Storage::DirPlugin::status($class, $storeid,
>>>> $scfg, $cache);
>>>> }
>>>> -# TODO: sub get_volume_notes {}
>>>> +# TODO: sub get_volume_attribute {}
>>>> -# TODO: sub update_volume_notes {}
>>>> +# TODO: sub update_volume_attribute {}
>>>> # croak would not include the caller from within this module
>>>> sub __error {
>>>> diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
>>>> index c5f3894..bee6885 100644
>>>> --- a/PVE/Storage/CIFSPlugin.pm
>>>> +++ b/PVE/Storage/CIFSPlugin.pm
>>>> @@ -286,13 +286,15 @@ sub check_connection {
>>>> return 1;
>>>> }
>>>> -sub get_volume_notes {
>>>> +sub get_volume_attribute {
>>>> my $class = shift;
>>>> - PVE::Storage::DirPlugin::get_volume_notes($class, @_);
>>>> +
>>>> + return PVE::Storage::DirPlugin::get_volume_attribute($class, @_);
>>>> }
>>>> -sub update_volume_notes {
>>>> +sub update_volume_attribute {
>>>> my $class = shift;
>>>> - PVE::Storage::DirPlugin::update_volume_notes($class, @_);
>>>> +
>>>> + return PVE::Storage::DirPlugin::update_volume_attribute($class,
>>>> @_);
>>>> }
>>>> 1;
>>>> diff --git a/PVE/Storage/CephFSPlugin.pm b/PVE/Storage/CephFSPlugin.pm
>>>> index 3b9a791..2c06804 100644
>>>> --- a/PVE/Storage/CephFSPlugin.pm
>>>> +++ b/PVE/Storage/CephFSPlugin.pm
>>>> @@ -232,14 +232,16 @@ sub deactivate_storage {
>>>> }
>>>> }
>>>> -sub get_volume_notes {
>>>> +sub get_volume_attribute {
>>>> my $class = shift;
>>>> - PVE::Storage::DirPlugin::get_volume_notes($class, @_);
>>>> +
>>>> + return PVE::Storage::DirPlugin::get_volume_attribute($class, @_);
>>>> }
>>>> -sub update_volume_notes {
>>>> +sub update_volume_attribute {
>>>> my $class = shift;
>>>> - PVE::Storage::DirPlugin::update_volume_notes($class, @_);
>>>> +
>>>> + return PVE::Storage::DirPlugin::update_volume_attribute($class,
>>>> @_);
>>>> }
>>>> 1;
>>>> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
>>>> index 0423e5f..6ab9ef2 100644
>>>> --- a/PVE/Storage/DirPlugin.pm
>>>> +++ b/PVE/Storage/DirPlugin.pm
>>>> @@ -87,32 +87,43 @@ sub parse_is_mountpoint {
>>>> return $is_mp; # contains a path
>>>> }
>>>> -sub get_volume_notes {
>>>> - my ($class, $scfg, $storeid, $volname, $timeout) = @_;
>>>> +sub get_volume_attribute {
>>>> + my ($class, $scfg, $storeid, $volname, $attribute) = @_;
>>>> - my $path = $class->filesystem_path($scfg, $volname);
>>>> - $path .= $class->SUPER::NOTES_EXT;
>>>> + my ($vtype) = $class->parse_volname($volname);
>>>> + return if $vtype ne 'backup';
>>>> - return PVE::Tools::file_get_contents($path) if -f $path;
>>>> + if ($attribute eq 'notes') {
>>>> + my $path = $class->filesystem_path($scfg, $volname);
>>>> + $path .= $class->SUPER::NOTES_EXT;
>>>> - return '';
>>>> -}
>>>> + return PVE::Tools::file_get_contents($path) if -f $path;
>>>> -sub update_volume_notes {
>>>> - my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
>>>> + return '';
>>>> + }
>>>> - my ($vtype) = $class->parse_volname($volname);
>>>> - die "only backups can have notes\n" if $vtype ne 'backup';
>>>> + return;
>>>> +}
>>>> - my $path = $class->filesystem_path($scfg, $volname);
>>>> - $path .= $class->SUPER::NOTES_EXT;
>>>> +sub update_volume_attribute {
>>>> + my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
>>>> - if (defined($notes) && $notes ne '') {
>>>> - PVE::Tools::file_set_contents($path, $notes);
>>>> - } elsif (-e $path) {
>>>> - unlink $path or die "could not delete notes - $!\n";
>>>> + my ($vtype) = $class->parse_volname($volname);
>>>> + die "only backups support attribute '$attribute'\n" if $vtype
>>>> ne 'backup';
>>>> +
>>>> + if ($attribute eq 'notes') {
>>>> + my $path = $class->filesystem_path($scfg, $volname);
>>>> + $path .= $class->SUPER::NOTES_EXT;
>>>> +
>>>> + if (defined($value) && $value ne '') {
>>>> + PVE::Tools::file_set_contents($path, $value);
>>>> + } elsif (-e $path) {
>>>> + unlink $path or die "could not delete notes - $!\n";
>>>> + }
>>>> + return;
>>>> }
>>>> - return;
>>>> +
>>>> + die "attribute '$attribute' is not supported for storage type
>>>> '$scfg->{type}'\n";
>>>> }
>>>> sub status {
>>>> diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
>>>> index 39bf15a..3a6efe5 100644
>>>> --- a/PVE/Storage/NFSPlugin.pm
>>>> +++ b/PVE/Storage/NFSPlugin.pm
>>>> @@ -188,13 +188,15 @@ sub check_connection {
>>>> return 1;
>>>> }
>>>> -sub get_volume_notes {
>>>> +sub get_volume_attribute {
>>>> my $class = shift;
>>>> - PVE::Storage::DirPlugin::get_volume_notes($class, @_);
>>>> +
>>>> + return PVE::Storage::DirPlugin::get_volume_attribute($class, @_);
>>>> }
>>>> -sub update_volume_notes {
>>>> +sub update_volume_attribute {
>>>> my $class = shift;
>>>> - PVE::Storage::DirPlugin::update_volume_notes($class, @_);
>>>> +
>>>> + return PVE::Storage::DirPlugin::update_volume_attribute($class,
>>>> @_);
>>>> }
>>>> 1;
>>>> diff --git a/PVE/Storage/PBSPlugin.pm b/PVE/Storage/PBSPlugin.pm
>>>> index bb1c382..d8e1ac8 100644
>>>> --- a/PVE/Storage/PBSPlugin.pm
>>>> +++ b/PVE/Storage/PBSPlugin.pm
>>>> @@ -782,24 +782,32 @@ sub deactivate_volume {
>>>> return 1;
>>>> }
>>>> -sub get_volume_notes {
>>>> - my ($class, $scfg, $storeid, $volname, $timeout) = @_;
>>>> +sub get_volume_attribute {
>>>> + my ($class, $scfg, $storeid, $volname, $attribute) = @_;
>>>> +
>>>> + if ($attribute eq 'notes') {
>>>> + my (undef, $name, undef, undef, undef, undef, $format) =
>>>> $class->parse_volname($volname);
>>>> - my (undef, $name, undef, undef, undef, undef, $format) =
>>>> $class->parse_volname($volname);
>>>> + my $data = run_client_cmd($scfg, $storeid, "snapshot", [
>>>> "notes", "show", $name ]);
>>>> - my $data = run_client_cmd($scfg, $storeid, "snapshot", [
>>>> "notes", "show", $name ]);
>>>> + return $data->{notes} // '';
>>>> + }
>>>> - return $data->{notes};
>>>> + return;
>>>> }
>>>> -sub update_volume_notes {
>>>> - my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
>>>> +sub update_volume_attribute {
>>>> + my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
>>>> - my (undef, $name, undef, undef, undef, undef, $format) =
>>>> $class->parse_volname($volname);
>>>> + if ($attribute eq 'notes') {
>>>> + my (undef, $name, undef, undef, undef, undef, $format) =
>>>> $class->parse_volname($volname);
>>>> - run_client_cmd($scfg, $storeid, "snapshot", [ "notes",
>>>> "update", $name, $notes ], 1);
>>>> + run_client_cmd($scfg, $storeid, "snapshot", [ "notes",
>>>> "update", $name, $value ], 1);
>>>> - return undef;
>>>> + return;
>>>> + }
>>>> +
>>>> + die "attribute '$attribute' is not supported for storage type
>>>> '$scfg->{type}'\n";
>>>> }
>>>> sub volume_size_info {
>>>> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
>>>> index 417d1fd..1182008 100644
>>>> --- a/PVE/Storage/Plugin.pm
>>>> +++ b/PVE/Storage/Plugin.pm
>>>> @@ -851,18 +851,52 @@ sub file_size_info {
>>>> return wantarray ? ($size, $format, $used, $parent,
>>>> $st->ctime) : $size;
>>>> }
>>>> +# FIXME remove on the next APIAGE reset.
>>>> +# Deprecated, use get_volume_attribute instead.
>>>> sub get_volume_notes {
>>>> my ($class, $scfg, $storeid, $volname, $timeout) = @_;
>>>> die "volume notes are not supported for $class";
>>>> }
>>>> +# FIXME remove on the next APIAGE reset.
>>>> +# Deprecated, use update_volume_attribute instead.
>>>> sub update_volume_notes {
>>>> my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
>>>> die "volume notes are not supported for $class";
>>>> }
>>>> +# Returns undef if the attribute is not supported for the volume.
>>>> +# Should die if there is an error fetching the attribute.
>>>> +# Possible attributes:
>>>> +# notes - user-provided comments/notes.
>>>> +sub get_volume_attribute {
>>>> + my ($class, $scfg, $storeid, $volname, $attribute) = @_;
>>>> +
>>>> + if ($attribute eq 'notes') {
>>>> + my $notes = eval { $class->get_volume_notes($scfg, $storeid,
>>>> $volname); };
>>>> + if (my $err = $@) {
>>>> + return if $err =~ m/^volume notes are not supported/;
>>>> + die $err;
>>>> + }
>>>> + return $notes;
>>>> + }
>>>> +
>>>> + return;
>>>> +}
>>>> +
>>>> +# Dies if the attribute is not supported for the volume.
>>>> +sub update_volume_attribute {
>>>> + my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
>>>> +
>>>> + if ($attribute eq 'notes') {
>>>> + $class->update_volume_notes($scfg, $storeid, $volname, $value);
>>>> + }
>>>> +
>>>> + die "attribute '$attribute' is not supported for storage type
>>>> '$scfg->{type}'\n";
>>>> +}
>>>> +
>>>> sub volume_size_info {
>>>> my ($class, $scfg, $storeid, $volname, $timeout) = @_;
>>>> my $path = $class->filesystem_path($scfg, $volname);
>>>> @@ -1098,6 +1132,8 @@ my $get_subdir_files = sub {
>>>> return $res;
>>>> };
>>>> +# If attributes are set on a volume, they should be included in the
>>>> result.
>>>> +# See get_volume_attribute for a list of possible attributes.
>>>> sub list_volumes {
>>>> my ($class, $storeid, $scfg, $vmid, $content_types) = @_;
>>>>
>>>
>>>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [pve-devel] [PATCH storage 3/6] prune mark: preserve additional information for the keep-all case
2021-09-17 13:02 [pve-devel] [RFC storage/manager] fix #3307: allow backups to be marked as protected Fabian Ebner
2021-09-17 13:02 ` [pve-devel] [PATCH storage 1/6] dir plugin: update notes: don't attempt to remove non-existent notes Fabian Ebner
2021-09-17 13:02 ` [pve-devel] [RFC storage 2/6] add generalized functions to manage volume attributes Fabian Ebner
@ 2021-09-17 13:02 ` Fabian Ebner
2021-09-17 13:02 ` [pve-devel] [RFC storage 4/6] fix #3307: make it possible to set protection for backups Fabian Ebner
` (3 subsequent siblings)
6 siblings, 0 replies; 20+ messages in thread
From: Fabian Ebner @ 2021-09-17 13:02 UTC (permalink / raw)
To: pve-devel
Currently, if an entry is already marked as 'protected'.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
This makes adding the test in the next patch much simpler, because
the expected mark of a protected backup can be hardcoded as
'protected'. But even without that, it makes sense to preserve the
more specific mark.
PVE/Storage.pm | 2 ++
1 file changed, 2 insertions(+)
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 729c90e..e7d8e62 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -1670,6 +1670,8 @@ sub prune_mark_backup_group {
if ($keep->{'keep-all'} || scalar(@positive_opts) == 0) {
foreach my $prune_entry (@{$backup_group}) {
+ # preserve additional information like 'protected'
+ next if $prune_entry->{mark} && $prune_entry->{mark} ne 'remove';
$prune_entry->{mark} = 'keep';
}
return;
--
2.30.2
^ permalink raw reply [flat|nested] 20+ messages in thread* [pve-devel] [RFC storage 4/6] fix #3307: make it possible to set protection for backups
2021-09-17 13:02 [pve-devel] [RFC storage/manager] fix #3307: allow backups to be marked as protected Fabian Ebner
` (2 preceding siblings ...)
2021-09-17 13:02 ` [pve-devel] [PATCH storage 3/6] prune mark: preserve additional information for the keep-all case Fabian Ebner
@ 2021-09-17 13:02 ` Fabian Ebner
2021-09-24 8:54 ` Dominik Csapak
2021-09-17 13:02 ` [pve-devel] [RFC storage 5/6] prune: mark renamed and protected backups differently Fabian Ebner
` (2 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Fabian Ebner @ 2021-09-17 13:02 UTC (permalink / raw)
To: pve-devel
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?
Should we also bother to set the immutable flag on filesystems that
support it?
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
+
+ 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";
+ }
+
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',
--
2.30.2
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pve-devel] [RFC storage 4/6] fix #3307: make it possible to set protection for backups
2021-09-17 13:02 ` [pve-devel] [RFC storage 4/6] fix #3307: make it possible to set protection for backups Fabian Ebner
@ 2021-09-24 8:54 ` Dominik Csapak
2021-09-24 11:17 ` Fabian Ebner
0 siblings, 1 reply; 20+ messages in thread
From: Dominik Csapak @ 2021-09-24 8:54 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Ebner
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
> +
> + 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',
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pve-devel] [RFC storage 4/6] fix #3307: make it possible to set protection for backups
2021-09-24 8:54 ` Dominik Csapak
@ 2021-09-24 11:17 ` Fabian Ebner
0 siblings, 0 replies; 20+ messages in thread
From: Fabian Ebner @ 2021-09-24 11:17 UTC (permalink / raw)
To: Dominik Csapak, Proxmox VE development discussion
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',
>>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [pve-devel] [RFC storage 5/6] prune: mark renamed and protected backups differently
2021-09-17 13:02 [pve-devel] [RFC storage/manager] fix #3307: allow backups to be marked as protected Fabian Ebner
` (3 preceding siblings ...)
2021-09-17 13:02 ` [pve-devel] [RFC storage 4/6] fix #3307: make it possible to set protection for backups Fabian Ebner
@ 2021-09-17 13:02 ` Fabian Ebner
2021-09-17 13:02 ` [pve-devel] [RFC storage 6/6] pbs: integrate support for protected Fabian Ebner
2021-09-17 13:02 ` [pve-devel] [RFC manager 1/1] vzdump: skip protected backups for dumpdir pruning Fabian Ebner
6 siblings, 0 replies; 20+ messages in thread
From: Fabian Ebner @ 2021-09-17 13:02 UTC (permalink / raw)
To: pve-devel
While it makes no difference for pruning itself, protected backups are
additionally protected against removal. Avoid the potential to confuse
the two. Also update the description for the API return value and add
an enum constraint.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
PVE/API2/Storage/PruneBackups.pm | 5 +++--
PVE/Storage/Plugin.pm | 2 +-
test/prune_backups_test.pm | 4 ++--
3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/PVE/API2/Storage/PruneBackups.pm b/PVE/API2/Storage/PruneBackups.pm
index 2509a46..e6ab276 100644
--- a/PVE/API2/Storage/PruneBackups.pm
+++ b/PVE/API2/Storage/PruneBackups.pm
@@ -61,9 +61,10 @@ __PACKAGE__->register_method ({
type => 'integer',
},
'mark' => {
- description => "Whether the backup would be kept or removed. For backups that don't " .
- "use the standard naming scheme, it's 'protected'.",
+ description => "Whether the backup would be kept or removed. Backups that are" .
+ " protected or don't use the standard naming scheme are not removed.",
type => 'string',
+ enum => ['keep', 'remove', 'protected', 'renamed'],
},
type => {
description => "One of 'qemu', 'lxc', 'openvz' or 'unknown'.",
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index 1ebd705..a2145fa 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -1322,7 +1322,7 @@ sub prune_backups {
push @{$backup_groups->{$group}}, $prune_entry;
} else {
# ignore backups that don't use the standard naming scheme
- $prune_entry->{mark} = 'protected';
+ $prune_entry->{mark} = 'renamed';
}
$prune_entry->{mark} = 'protected' if $backup->{protected};
diff --git a/test/prune_backups_test.pm b/test/prune_backups_test.pm
index 8ad6144..b57d280 100644
--- a/test/prune_backups_test.pm
+++ b/test/prune_backups_test.pm
@@ -189,7 +189,7 @@ sub generate_expected {
'volid' => "$storeid:backup/vzdump-$vmid-renamed.tar.zst",
'type' => 'unknown',
'ctime' => 1234,
- 'mark' => 'protected',
+ 'mark' => 'renamed',
'vmid' => $vmid,
},
) if !defined($type);
@@ -375,7 +375,7 @@ my $tests = [
{
'volid' => "$storeid:backup/vzdump-lxc-novmid.tar.gz",
'ctime' => 1234,
- 'mark' => 'protected',
+ 'mark' => 'renamed',
'type' => 'lxc',
},
],
--
2.30.2
^ permalink raw reply [flat|nested] 20+ messages in thread* [pve-devel] [RFC storage 6/6] pbs: integrate support for protected
2021-09-17 13:02 [pve-devel] [RFC storage/manager] fix #3307: allow backups to be marked as protected Fabian Ebner
` (4 preceding siblings ...)
2021-09-17 13:02 ` [pve-devel] [RFC storage 5/6] prune: mark renamed and protected backups differently Fabian Ebner
@ 2021-09-17 13:02 ` Fabian Ebner
2021-09-24 8:55 ` Dominik Csapak
2021-09-17 13:02 ` [pve-devel] [RFC manager 1/1] vzdump: skip protected backups for dumpdir pruning Fabian Ebner
6 siblings, 1 reply; 20+ messages in thread
From: Fabian Ebner @ 2021-09-17 13:02 UTC (permalink / raw)
To: pve-devel
free_image doesn't need to check for protection, because that will
happen on the server.
Getting/updating notes has also been refactored to re-use the code
for the PBS api calls.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
Needs new external dependency for strptime (libposix-strptime-perl),
because it's not in perl's POSIX module.
An alternative would be to use perlmod and export the proxmox crate's
function for parsing the timestring.
It depends on Dominik's patches for PBS to work:
https://lists.proxmox.com/pipermail/pbs-devel/2021-September/003926.html
PVE/Storage/PBSPlugin.pm | 59 ++++++++++++++++++++++++++++++++++------
1 file changed, 51 insertions(+), 8 deletions(-)
diff --git a/PVE/Storage/PBSPlugin.pm b/PVE/Storage/PBSPlugin.pm
index d8e1ac8..082d138 100644
--- a/PVE/Storage/PBSPlugin.pm
+++ b/PVE/Storage/PBSPlugin.pm
@@ -9,7 +9,8 @@ use Fcntl qw(F_GETFD F_SETFD FD_CLOEXEC);
use IO::File;
use JSON;
use MIME::Base64 qw(decode_base64);
-use POSIX qw(strftime ENOENT);
+use POSIX qw(mktime strftime ENOENT);
+use POSIX::strptime;
use PVE::APIClient::LWP;
use PVE::JSONSchema qw(get_standard_option);
@@ -218,6 +219,36 @@ sub print_volid {
return "${storeid}:${volname}";
}
+# essentially the inverse of print_volid
+sub api_param_from_volname {
+ my ($class, $volname) = @_;
+
+ my $name = ($class->parse_volname($volname))[1];
+
+ my ($btype, $bid, $timestr) = split('/', $name);
+
+ my @tm = (POSIX::strptime($timestr, "%FT%TZ"));
+ # expect sec, min, hour, mday, mon, year
+ die "error parsing time from '$volname'" if grep { !defined($_) } @tm[0..5];
+
+ my $btime;
+ {
+ local $ENV{TZ} = 'UTC'; # $timestr is UTC
+
+ # Fill in isdst to avoid undef warning. No daylight saving time for UTC.
+ $tm[8] //= 0;
+
+ my $since_epoch = mktime(@tm) or die "error converting time from '$volname'\n";
+ $btime = int($since_epoch);
+ }
+
+ return {
+ 'backup-type' => $btype,
+ 'backup-id' => $bid,
+ 'backup-time' => $btime,
+ };
+}
+
my $USE_CRYPT_PARAMS = {
backup => 1,
restore => 1,
@@ -658,6 +689,7 @@ sub list_volumes {
$info->{verification} = $item->{verification} if defined($item->{verification});
$info->{notes} = $item->{comment} if defined($item->{comment});
+ $info->{protected} = 1 if $item->{protected};
if (defined($item->{fingerprint})) {
$info->{encrypted} = $item->{fingerprint};
} elsif (snapshot_files_encrypted($item->{files})) {
@@ -785,12 +817,19 @@ sub deactivate_volume {
sub get_volume_attribute {
my ($class, $scfg, $storeid, $volname, $attribute) = @_;
- if ($attribute eq 'notes') {
- my (undef, $name, undef, undef, undef, undef, $format) = $class->parse_volname($volname);
+ if ($attribute eq 'notes' || $attribute eq 'protected') {
+ my $param = $class->api_param_from_volname($volname);
- my $data = run_client_cmd($scfg, $storeid, "snapshot", [ "notes", "show", $name ]);
+ my $password = pbs_get_password($scfg, $storeid);
+ my $conn = pbs_api_connect($scfg, $password);
+ my $datastore = $scfg->{datastore};
- return $data->{notes} // '';
+ my $res = eval { $conn->get("/api2/json/admin/datastore/$datastore/$attribute", $param); };
+ if (my $err = $@) {
+ return if $err->{code} == 404; # not supported
+ die $err;
+ }
+ return $res;
}
return;
@@ -799,11 +838,15 @@ sub get_volume_attribute {
sub update_volume_attribute {
my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
- if ($attribute eq 'notes') {
- my (undef, $name, undef, undef, undef, undef, $format) = $class->parse_volname($volname);
+ if ($attribute eq 'notes' || $attribute eq 'protected') {
+ my $param = $class->api_param_from_volname($volname);
+ $param->{$attribute} = $value;
- run_client_cmd($scfg, $storeid, "snapshot", [ "notes", "update", $name, $value ], 1);
+ my $password = pbs_get_password($scfg, $storeid);
+ my $conn = pbs_api_connect($scfg, $password);
+ my $datastore = $scfg->{datastore};
+ $conn->put("/api2/json/admin/datastore/$datastore/$attribute", $param);
return;
}
--
2.30.2
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pve-devel] [RFC storage 6/6] pbs: integrate support for protected
2021-09-17 13:02 ` [pve-devel] [RFC storage 6/6] pbs: integrate support for protected Fabian Ebner
@ 2021-09-24 8:55 ` Dominik Csapak
2021-09-24 11:32 ` Fabian Ebner
0 siblings, 1 reply; 20+ messages in thread
From: Dominik Csapak @ 2021-09-24 8:55 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Ebner
On 9/17/21 15:02, Fabian Ebner wrote:
> free_image doesn't need to check for protection, because that will
> happen on the server.
>
> Getting/updating notes has also been refactored to re-use the code
> for the PBS api calls.
>
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>
> Needs new external dependency for strptime (libposix-strptime-perl),
> because it's not in perl's POSIX module.
>
> An alternative would be to use perlmod and export the proxmox crate's
> function for parsing the timestring.
>
> It depends on Dominik's patches for PBS to work:
> https://lists.proxmox.com/pipermail/pbs-devel/2021-September/003926.html
>
> PVE/Storage/PBSPlugin.pm | 59 ++++++++++++++++++++++++++++++++++------
> 1 file changed, 51 insertions(+), 8 deletions(-)
>
> diff --git a/PVE/Storage/PBSPlugin.pm b/PVE/Storage/PBSPlugin.pm
> index d8e1ac8..082d138 100644
> --- a/PVE/Storage/PBSPlugin.pm
> +++ b/PVE/Storage/PBSPlugin.pm
> @@ -9,7 +9,8 @@ use Fcntl qw(F_GETFD F_SETFD FD_CLOEXEC);
> use IO::File;
> use JSON;
> use MIME::Base64 qw(decode_base64);
> -use POSIX qw(strftime ENOENT);
> +use POSIX qw(mktime strftime ENOENT);
> +use POSIX::strptime;
nit: couldn't we combine those two lines?
>
> use PVE::APIClient::LWP;
> use PVE::JSONSchema qw(get_standard_option);
> @@ -218,6 +219,36 @@ sub print_volid {
> return "${storeid}:${volname}";
> }
>
> +# essentially the inverse of print_volid
> +sub api_param_from_volname {
> + my ($class, $volname) = @_;
> +
> + my $name = ($class->parse_volname($volname))[1];
> +
> + my ($btype, $bid, $timestr) = split('/', $name);
> +
> + my @tm = (POSIX::strptime($timestr, "%FT%TZ"));
> + # expect sec, min, hour, mday, mon, year
> + die "error parsing time from '$volname'" if grep { !defined($_) } @tm[0..5];
> +
> + my $btime;
> + {
> + local $ENV{TZ} = 'UTC'; # $timestr is UTC
> +
> + # Fill in isdst to avoid undef warning. No daylight saving time for UTC.
> + $tm[8] //= 0;
> +
> + my $since_epoch = mktime(@tm) or die "error converting time from '$volname'\n";
> + $btime = int($since_epoch);
> + }
> +
> + return {
> + 'backup-type' => $btype,
> + 'backup-id' => $bid,
> + 'backup-time' => $btime,
> + };
> +}
> +
> my $USE_CRYPT_PARAMS = {
> backup => 1,
> restore => 1,
> @@ -658,6 +689,7 @@ sub list_volumes {
>
> $info->{verification} = $item->{verification} if defined($item->{verification});
> $info->{notes} = $item->{comment} if defined($item->{comment});
> + $info->{protected} = 1 if $item->{protected};
> if (defined($item->{fingerprint})) {
> $info->{encrypted} = $item->{fingerprint};
> } elsif (snapshot_files_encrypted($item->{files})) {
> @@ -785,12 +817,19 @@ sub deactivate_volume {
> sub get_volume_attribute {
> my ($class, $scfg, $storeid, $volname, $attribute) = @_;
>
> - if ($attribute eq 'notes') {
> - my (undef, $name, undef, undef, undef, undef, $format) = $class->parse_volname($volname);
> + if ($attribute eq 'notes' || $attribute eq 'protected') {
> + my $param = $class->api_param_from_volname($volname);
>
> - my $data = run_client_cmd($scfg, $storeid, "snapshot", [ "notes", "show", $name ]);
> + my $password = pbs_get_password($scfg, $storeid);
> + my $conn = pbs_api_connect($scfg, $password);
> + my $datastore = $scfg->{datastore};
>
> - return $data->{notes} // '';
> + my $res = eval { $conn->get("/api2/json/admin/datastore/$datastore/$attribute", $param); };
> + if (my $err = $@) {
> + return if $err->{code} == 404; # not supported
> + die $err;
> + }
> + return $res;
> }
>
> return;
> @@ -799,11 +838,15 @@ sub get_volume_attribute {
> sub update_volume_attribute {
> my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
>
> - if ($attribute eq 'notes') {
> - my (undef, $name, undef, undef, undef, undef, $format) = $class->parse_volname($volname);
> + if ($attribute eq 'notes' || $attribute eq 'protected') {
> + my $param = $class->api_param_from_volname($volname);
> + $param->{$attribute} = $value;
>
> - run_client_cmd($scfg, $storeid, "snapshot", [ "notes", "update", $name, $value ], 1);
> + my $password = pbs_get_password($scfg, $storeid);
> + my $conn = pbs_api_connect($scfg, $password);
> + my $datastore = $scfg->{datastore};
>
> + $conn->put("/api2/json/admin/datastore/$datastore/$attribute", $param);
> return;
> }
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pve-devel] [RFC storage 6/6] pbs: integrate support for protected
2021-09-24 8:55 ` Dominik Csapak
@ 2021-09-24 11:32 ` Fabian Ebner
2021-09-24 11:48 ` Dominik Csapak
0 siblings, 1 reply; 20+ messages in thread
From: Fabian Ebner @ 2021-09-24 11:32 UTC (permalink / raw)
To: Dominik Csapak, Proxmox VE development discussion
Am 24.09.21 um 10:55 schrieb Dominik Csapak:
> On 9/17/21 15:02, Fabian Ebner wrote:
>> free_image doesn't need to check for protection, because that will
>> happen on the server.
>>
>> Getting/updating notes has also been refactored to re-use the code
>> for the PBS api calls.
>>
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>>
>> Needs new external dependency for strptime (libposix-strptime-perl),
>> because it's not in perl's POSIX module.
>>
>> An alternative would be to use perlmod and export the proxmox crate's
>> function for parsing the timestring.
>>
>> It depends on Dominik's patches for PBS to work:
>> https://lists.proxmox.com/pipermail/pbs-devel/2021-September/003926.html
>>
>> PVE/Storage/PBSPlugin.pm | 59 ++++++++++++++++++++++++++++++++++------
>> 1 file changed, 51 insertions(+), 8 deletions(-)
>>
>> diff --git a/PVE/Storage/PBSPlugin.pm b/PVE/Storage/PBSPlugin.pm
>> index d8e1ac8..082d138 100644
>> --- a/PVE/Storage/PBSPlugin.pm
>> +++ b/PVE/Storage/PBSPlugin.pm
>> @@ -9,7 +9,8 @@ use Fcntl qw(F_GETFD F_SETFD FD_CLOEXEC);
>> use IO::File;
>> use JSON;
>> use MIME::Base64 qw(decode_base64);
>> -use POSIX qw(strftime ENOENT);
>> +use POSIX qw(mktime strftime ENOENT);
>> +use POSIX::strptime;
>
> nit: couldn't we combine those two lines?
>
As noted above, this is a different dependency/package. It's not part of
the usual POSIX package.
>> use PVE::APIClient::LWP;
>> use PVE::JSONSchema qw(get_standard_option);
>> @@ -218,6 +219,36 @@ sub print_volid {
>> return "${storeid}:${volname}";
>> }
>> +# essentially the inverse of print_volid
>> +sub api_param_from_volname {
>> + my ($class, $volname) = @_;
>> +
>> + my $name = ($class->parse_volname($volname))[1];
>> +
>> + my ($btype, $bid, $timestr) = split('/', $name);
>> +
>> + my @tm = (POSIX::strptime($timestr, "%FT%TZ"));
>> + # expect sec, min, hour, mday, mon, year
>> + die "error parsing time from '$volname'" if grep { !defined($_) }
>> @tm[0..5];
>> +
>> + my $btime;
>> + {
>> + local $ENV{TZ} = 'UTC'; # $timestr is UTC
>> +
>> + # Fill in isdst to avoid undef warning. No daylight saving time
>> for UTC.
>> + $tm[8] //= 0;
>> +
>> + my $since_epoch = mktime(@tm) or die "error converting time from
>> '$volname'\n";
>> + $btime = int($since_epoch);
>> + }
>> +
>> + return {
>> + 'backup-type' => $btype,
>> + 'backup-id' => $bid,
>> + 'backup-time' => $btime,
>> + };
>> +}
>> +
>> my $USE_CRYPT_PARAMS = {
>> backup => 1,
>> restore => 1,
>> @@ -658,6 +689,7 @@ sub list_volumes {
>> $info->{verification} = $item->{verification} if
>> defined($item->{verification});
>> $info->{notes} = $item->{comment} if defined($item->{comment});
>> + $info->{protected} = 1 if $item->{protected};
>> if (defined($item->{fingerprint})) {
>> $info->{encrypted} = $item->{fingerprint};
>> } elsif (snapshot_files_encrypted($item->{files})) {
>> @@ -785,12 +817,19 @@ sub deactivate_volume {
>> sub get_volume_attribute {
>> my ($class, $scfg, $storeid, $volname, $attribute) = @_;
>> - if ($attribute eq 'notes') {
>> - my (undef, $name, undef, undef, undef, undef, $format) =
>> $class->parse_volname($volname);
>> + if ($attribute eq 'notes' || $attribute eq 'protected') {
>> + my $param = $class->api_param_from_volname($volname);
>> - my $data = run_client_cmd($scfg, $storeid, "snapshot", [ "notes",
>> "show", $name ]);
>> + my $password = pbs_get_password($scfg, $storeid);
>> + my $conn = pbs_api_connect($scfg, $password);
>> + my $datastore = $scfg->{datastore};
>> - return $data->{notes} // '';
>> + my $res = eval {
>> $conn->get("/api2/json/admin/datastore/$datastore/$attribute",
>> $param); };
>> + if (my $err = $@) {
>> + return if $err->{code} == 404; # not supported
>> + die $err;
>> + }
>> + return $res;
>> }
>> return;
>> @@ -799,11 +838,15 @@ sub get_volume_attribute {
>> sub update_volume_attribute {
>> my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
>> - if ($attribute eq 'notes') {
>> - my (undef, $name, undef, undef, undef, undef, $format) =
>> $class->parse_volname($volname);
>> + if ($attribute eq 'notes' || $attribute eq 'protected') {
>> + my $param = $class->api_param_from_volname($volname);
>> + $param->{$attribute} = $value;
>> - run_client_cmd($scfg, $storeid, "snapshot", [ "notes", "update",
>> $name, $value ], 1);
>> + my $password = pbs_get_password($scfg, $storeid);
>> + my $conn = pbs_api_connect($scfg, $password);
>> + my $datastore = $scfg->{datastore};
>> + $conn->put("/api2/json/admin/datastore/$datastore/$attribute",
>> $param);
>> return;
>> }
>>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pve-devel] [RFC storage 6/6] pbs: integrate support for protected
2021-09-24 11:32 ` Fabian Ebner
@ 2021-09-24 11:48 ` Dominik Csapak
0 siblings, 0 replies; 20+ messages in thread
From: Dominik Csapak @ 2021-09-24 11:48 UTC (permalink / raw)
To: Fabian Ebner, Proxmox VE development discussion
On 9/24/21 13:32, Fabian Ebner wrote:
> Am 24.09.21 um 10:55 schrieb Dominik Csapak:
>> On 9/17/21 15:02, Fabian Ebner wrote:
>>> free_image doesn't need to check for protection, because that will
>>> happen on the server.
>>>
>>> Getting/updating notes has also been refactored to re-use the code
>>> for the PBS api calls.
>>>
>>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>>> ---
>>>
>>> Needs new external dependency for strptime (libposix-strptime-perl),
>>> because it's not in perl's POSIX module.
>>>
>>> An alternative would be to use perlmod and export the proxmox crate's
>>> function for parsing the timestring.
>>>
>>> It depends on Dominik's patches for PBS to work:
>>> https://lists.proxmox.com/pipermail/pbs-devel/2021-September/003926.html
>>>
>>> PVE/Storage/PBSPlugin.pm | 59 ++++++++++++++++++++++++++++++++++------
>>> 1 file changed, 51 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/PVE/Storage/PBSPlugin.pm b/PVE/Storage/PBSPlugin.pm
>>> index d8e1ac8..082d138 100644
>>> --- a/PVE/Storage/PBSPlugin.pm
>>> +++ b/PVE/Storage/PBSPlugin.pm
>>> @@ -9,7 +9,8 @@ use Fcntl qw(F_GETFD F_SETFD FD_CLOEXEC);
>>> use IO::File;
>>> use JSON;
>>> use MIME::Base64 qw(decode_base64);
>>> -use POSIX qw(strftime ENOENT);
>>> +use POSIX qw(mktime strftime ENOENT);
>>> +use POSIX::strptime;
>>
>> nit: couldn't we combine those two lines?
>>
>
> As noted above, this is a different dependency/package. It's not part of
> the usual POSIX package.
yeah i read the debian package part, i did not notice that
POSIX::strptime is a seperate perl package as well and thought
it would be automatically exposed in the POSIX module
>
>>> use PVE::APIClient::LWP;
>>> use PVE::JSONSchema qw(get_standard_option);
>>> @@ -218,6 +219,36 @@ sub print_volid {
>>> return "${storeid}:${volname}";
>>> }
>>> +# essentially the inverse of print_volid
>>> +sub api_param_from_volname {
>>> + my ($class, $volname) = @_;
>>> +
>>> + my $name = ($class->parse_volname($volname))[1];
>>> +
>>> + my ($btype, $bid, $timestr) = split('/', $name);
>>> +
>>> + my @tm = (POSIX::strptime($timestr, "%FT%TZ"));
>>> + # expect sec, min, hour, mday, mon, year
>>> + die "error parsing time from '$volname'" if grep { !defined($_)
>>> } @tm[0..5];
>>> +
>>> + my $btime;
>>> + {
>>> + local $ENV{TZ} = 'UTC'; # $timestr is UTC
>>> +
>>> + # Fill in isdst to avoid undef warning. No daylight saving time
>>> for UTC.
>>> + $tm[8] //= 0;
>>> +
>>> + my $since_epoch = mktime(@tm) or die "error converting time from
>>> '$volname'\n";
>>> + $btime = int($since_epoch);
>>> + }
>>> +
>>> + return {
>>> + 'backup-type' => $btype,
>>> + 'backup-id' => $bid,
>>> + 'backup-time' => $btime,
>>> + };
>>> +}
>>> +
>>> my $USE_CRYPT_PARAMS = {
>>> backup => 1,
>>> restore => 1,
>>> @@ -658,6 +689,7 @@ sub list_volumes {
>>> $info->{verification} = $item->{verification} if
>>> defined($item->{verification});
>>> $info->{notes} = $item->{comment} if defined($item->{comment});
>>> + $info->{protected} = 1 if $item->{protected};
>>> if (defined($item->{fingerprint})) {
>>> $info->{encrypted} = $item->{fingerprint};
>>> } elsif (snapshot_files_encrypted($item->{files})) {
>>> @@ -785,12 +817,19 @@ sub deactivate_volume {
>>> sub get_volume_attribute {
>>> my ($class, $scfg, $storeid, $volname, $attribute) = @_;
>>> - if ($attribute eq 'notes') {
>>> - my (undef, $name, undef, undef, undef, undef, $format) =
>>> $class->parse_volname($volname);
>>> + if ($attribute eq 'notes' || $attribute eq 'protected') {
>>> + my $param = $class->api_param_from_volname($volname);
>>> - my $data = run_client_cmd($scfg, $storeid, "snapshot", [
>>> "notes", "show", $name ]);
>>> + my $password = pbs_get_password($scfg, $storeid);
>>> + my $conn = pbs_api_connect($scfg, $password);
>>> + my $datastore = $scfg->{datastore};
>>> - return $data->{notes} // '';
>>> + my $res = eval {
>>> $conn->get("/api2/json/admin/datastore/$datastore/$attribute",
>>> $param); };
>>> + if (my $err = $@) {
>>> + return if $err->{code} == 404; # not supported
>>> + die $err;
>>> + }
>>> + return $res;
>>> }
>>> return;
>>> @@ -799,11 +838,15 @@ sub get_volume_attribute {
>>> sub update_volume_attribute {
>>> my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
>>> - if ($attribute eq 'notes') {
>>> - my (undef, $name, undef, undef, undef, undef, $format) =
>>> $class->parse_volname($volname);
>>> + if ($attribute eq 'notes' || $attribute eq 'protected') {
>>> + my $param = $class->api_param_from_volname($volname);
>>> + $param->{$attribute} = $value;
>>> - run_client_cmd($scfg, $storeid, "snapshot", [ "notes", "update",
>>> $name, $value ], 1);
>>> + my $password = pbs_get_password($scfg, $storeid);
>>> + my $conn = pbs_api_connect($scfg, $password);
>>> + my $datastore = $scfg->{datastore};
>>> + $conn->put("/api2/json/admin/datastore/$datastore/$attribute",
>>> $param);
>>> return;
>>> }
>>>
>>
>>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [pve-devel] [RFC manager 1/1] vzdump: skip protected backups for dumpdir pruning
2021-09-17 13:02 [pve-devel] [RFC storage/manager] fix #3307: allow backups to be marked as protected Fabian Ebner
` (5 preceding siblings ...)
2021-09-17 13:02 ` [pve-devel] [RFC storage 6/6] pbs: integrate support for protected Fabian Ebner
@ 2021-09-17 13:02 ` Fabian Ebner
6 siblings, 0 replies; 20+ messages in thread
From: Fabian Ebner @ 2021-09-17 13:02 UTC (permalink / raw)
To: pve-devel
Keeps the behavior consistent with what happens for storages. It also
is required to not get into conflict with the check in archive_remove,
i.e. pruning here marks a backup as 'remove' and then archive_remove
complains that it's protected.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
I'm noticing now that old manager + new storage will still have the
problem with the added protection check in archive_remove. Is that
considered to be breaking? Can only be triggered by making use of
the new feature (or if .protected files were already present...),
but it would potentially affect scenarios where
vzdump --storage name
vzdump --dumpdir /path/for/name/dump
are used in parallel.
Dependency bump for pve-storage needed.
PVE/VZDump.pm | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index d00be8b2..a5a956c8 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -992,6 +992,13 @@ sub exec_backup_task {
my $pruned = 0;
if (!defined($opts->{storage})) {
my $bklist = get_backup_file_list($opts->{dumpdir}, $bkname);
+
+ for my $prune_entry ($bklist->@*) {
+ if (-e PVE::Storage::protection_file_path($prune_entry->{path})) {
+ $prune_entry->{mark} = 'protected';
+ }
+ }
+
PVE::Storage::prune_mark_backup_group($bklist, $prune_options);
foreach my $prune_entry (@{$bklist}) {
--
2.30.2
^ permalink raw reply [flat|nested] 20+ messages in thread