Re: [pve-devel] [RFC storage 2/6] add generalized functions to manage volume attributes

[Dominik Csapak <d.csapak@proxmox.com>]

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) = @_;
>   
>