[pve-devel] [PATCH v1 pve-storage 4/8] pluginbase: document general plugin methods

[Fabian Grünbichler]

On March 26, 2025 3:20 pm, Max Carrara wrote:
> Add docstrings for the following methods:
> - check_connection
> - activate_storage
> - deactivate_storage
> - status
> - cluster_lock_storage
> - parse_volname
> - get_subdir
> - filesystem_path
> - path
> - find_free_diskname
> 
> Signed-off-by: Max Carrara <m.carrara at proxmox.com>
> ---
>  src/PVE/Storage/PluginBase.pm | 255 ++++++++++++++++++++++++++++++++++
>  1 file changed, 255 insertions(+)
> 
> diff --git a/src/PVE/Storage/PluginBase.pm b/src/PVE/Storage/PluginBase.pm
> index 5f7e6fd..8a61dc3 100644
> --- a/src/PVE/Storage/PluginBase.pm
> +++ b/src/PVE/Storage/PluginBase.pm
> @@ -317,53 +317,308 @@ sub private {
>  
>  =cut
>  
> +=head3 $plugin->check_connection($storeid, \%scfg)
> +
> +B<OPTIONAL:> May be implemented in a storage plugin.
> +
> +Performs a connection check.
> +
> +This method is useful for plugins that require some kind of network connection
> +or similar and is called before C<L<< activate_storage()|/"$plugin->activate_storage($storeid, \%scfg, \%cache)" >>>.

This method can be implemented by network-based storages. It will be
called before storage activation attempts. Non-network storages should
not implement it.

> +
> +Returns C<1> by default and C<0> if the storage isn't online / reachable.

by default doesn't make any sense, even though I know what you mean.

> +
> +C<die>s if an error occurs while performing the connection check.
> +
> +Note that this method's purpose is to simply verify that the storage is
> +I<reachable>, and not necessarily that authentication etc. succeeds.
> +
> +For example: If the storage is mainly accessed via TCP, you can try to simply
> +open a TCP connection to see if it's online:
> +
> +    # In custom module:
> +
> +    use PVE::Network;
> +    use parent qw(PVE::Storage::Plugin)
> +
> +    # [...]
> +
> +    sub check_connection {
> +	my ($class, $storeid, $scfg) = @_;
> +
> +	my $port = $scfg->{port} || 5432;
> +	return PVE::Network::tcp_ping($scfg->{server}, $port, 5);
> +    }
> +
> +=cut
> +
>  sub check_connection {
>      my ($class, $storeid, $scfg) = @_;
>  
>      return 1;
>  }
>  
> +=head3 $plugin->activate_storage($storeid, \%scfg, \%cache)
> +
> +=head3 $plugin->activate_storage(...)
> +
> +B<REQUIRED:> Must be implemented in every storage plugin.
> +
> +Activates the storage, making it ready for further use.
> +
> +In essence, this method performs the steps necessary so that the storage can be
> +used by remaining parts of the system.

this is superfluous (all of that information is already contained in the
previous sentence).

> +
> +In the case of file-based storages, this usually entails creating the directory
> +of the mountpoint, mounting the storage and then creating the directories for
> +the different content types that the storage has enabled. See
> +C<L<PVE::Storage::NFSPlugin>> and C<L<PVE::Storage::CIFSPlugin>> for examples
> +in that regard.
> +
> +Other types of storages would use this method for establishing a connection to
> +the storage and authenticating with it or similar. See C<L<PVE::Storage::ISCSIPlugin>>
> +for an example.
> +

I am not sure this examples provide much benefit in this verbosity..
maybe just a single sentence like

Common examples of activation might include mounting filesystems,
preparing directory trees or establishing sessions with network
storages.

> +If the storage doesn't need to be activated in some way, this method can be a
> +no-op.
> +
> +C<die>s in case of errors.

*Should* die?

> +
> +This method may reuse L<< cached information via C<\%cache>|/"CACHING EXPENSIVE OPERATIONS" >>.
> +
> +=cut
> +
>  sub activate_storage {
>      my ($class, $storeid, $scfg, $cache) = @_;
>      croak "implement me in sub-class\n";
>  }
>  
> +=head3 $plugin->deactivate_storage($storeid, \%scfg, \%cache)
> +
> +=head3 $plugin->deactivate_storage(...)
> +
> +B<OPTIONAL:> May be implemented in a storage plugin.
> +
> +Deactivates the storage. Counterpart to C<L<< activate_storage()|/"$plugin->activate_storage(...)" >>>.
> +
> +This method is used to make the storage unavailable to the rest of the system,
> +which usually entails unmounting the storage, closing an established
> +connection, or similar.
> +
> +Does nothing by default.
> +
> +C<die>s in case of errors.

*Should*

> +
> +This method may reuse L<< cached information via C<\%cache>|/"CACHING EXPENSIVE OPERATIONS" >>.
> +
> +B<NOTE:> This method is currently not used by our API due to historical
> +reasons.
> +
> +=cut
> +
>  sub deactivate_storage {
>      my ($class, $storeid, $scfg, $cache) = @_;
>  
>      return;
>  }
>  
> +=head3 $plugin->status($storeid, \%scfg, \%cache)
> +
> +=head3 $plugin->status(...)
> +
> +B<REQUIRED:> Must be implemented in every storage plugin.
> +
> +Returns a list of scalars in the following form:
> +
> +    my ($total, $available, $used, $active) = $plugin->status($storeid, $scfg, $cache)
> +
> +This list contains the C<$total>, C<$available> and C<$used> storage capacity,
> +respectively, as well as whether the storage is C<$active> or not.

might make sense to include the information which unit the returned
numbers should be in?

> +
> +This method may reuse L<< cached information via C<\%cache>|/"CACHING EXPENSIVE OPERATIONS" >>.
> +
> +=cut
> +
>  sub status {
>      my ($class, $storeid, $scfg, $cache) = @_;
>      croak "implement me in sub-class\n";
>  }
>  
> +=head3 $plugin->cluster_lock_storage($storeid, $shared, $timeout, \&func, @param)
> +
> +=head3 $plugin->cluster_lock_storage(...)
> +
> +B<WARNING:> This method is provided by C<L<PVE::Storage::Plugin>> and
> +must be used as-is. It is merely documented here for informal purposes
> +and B<must not be overridden.>

see my comment on the first patch.. this is not actually part of the
plugin interface, it is a helper that is implemented by
PVE::Storage::Plugin for historical reasons and should be moved
somewhere else and then removed from PVE::Storage::Plugin when we do the
next break of that inheritance hierarchy..

> +
> +Locks the storage with the given C<$storeid> for the entire host and runs the
> +given C<\&func> with the given C<@param>s, unlocking the storage once finished.
> +If the storage is C<$shared>, it is instead locked on the entire cluster. If
> +the storage is already locked, wait for at most the number of seconds given by
> +C<$timeout>.

Attempts to acquire a lock on C<$storeid>, waiting at most C<$timeout>
seconds before giving up. If successful, runs C<\&func> with the given
C<@param>s while holding the lock. If C<$shared> is set, the lock
operation will be cluster-wide (with a timeout for the execution of
C<\&func> of 60 seconds).

This method must be used to guard against concurrent modifications of
the storage by multiple tasks running at the same time, in particular
for actions such as:
- allocating new volumes (including clones) or snapshots
- deleting existing volumes or snapshots
- renaming existing volumes or snapshots

> +
> +This method is used to synchronize access to the given storage, preventing
> +simultaneous modification from multiple workers. This is necessary for
> +operations that modify data on the storage directly, like cloning and
> +allocating images.


> +
> +=cut
> +
>  sub cluster_lock_storage {
>      my ($class, $storeid, $shared, $timeout, $func, @param) = @_;
>      croak "implement me in sub-class\n";
>  }
>  
> +=head3 $plugin->parse_volname($volname)
> +
> +B<REQUIRED:> Must be implemented in every storage plugin.
> +
> +Parses C<$volname>, returning a list representing the parts encoded in
> +the volume name:

s/parts/volume properties/ ?

should we incorporate the outcome of this discussion:

https://lore.proxmox.com/pve-devel/qdnb3vypbucbcf7ch2hsjbeo3hqb5bh4whoinl5xglggpt7b7t@igryfncdupdp/

?

> +
> +    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format)
> +	= $plugin->parse_volname($volname);
> +
> +Not all parts need to be included in the list. Those marked as I<optional>
> +in the list below may be set to C<undef> if not applicable.
> +
> +This method may C<die> in case of errors.

s/may/should/ ?

> +
> +=over
> +
> +=item * C<< $vtype >>
> +
> +The content type ("volume type") of the volume, e.g. C<"images">, C<"iso">,
> +etc.

content type != volume type! the two are related obviously, but the
values are subtly different for images for historic reasons.

> +
> +See C<L<< plugindata()|/"$plugin->plugindata()" >>> for all content types.
> +
> +=item * C<< $name >>
> +
> +The display name of the volume. This is usually what the underlying storage
> +itself uses to address the volume.

Ideally, this maps directly to some entity on the underlying storage.

> +
> +For example, disks for virtual machines that are stored on LVM thin pools are
> +named C<vm-100-disk-0>, C<vm-1337-disk-1>, etc. That would be the C<$name> in
> +this case.

This is rather imprecise, maybe just say

For example, a guest volume named C<vm-100-disk-0> stored on an LVM thin
pool will refer to a thin LV of the same name.

> +
> +=item * C<< $vmid >> (optional)
> +
> +The ID of the guest that owns the volume.

should maybe note that this must be set for `images` or `backup`, and
must not be set for the other vtypes?

> +
> +=item * C<< $basename >> (optional)
> +
> +The C<$name> of the volume this volume is based on. Whether this part
> +is returned or not depends on the plugin and underlying storage.

The C<$name> of this volume's base volume, if it is a linked clone.

?

> +Only applies to disk images.
> +
> +For example, on ZFS, if the VM is a linked clone, C<$basename> refers
> +to the C<$name> of the original disk volume that the parsed disk volume
> +corresponds to.

it is only used for that, and has this semantic across all storages,
right?

> +
> +=item * C<< $basevmid >> (optional)
> +
> +Equivalent to C<$basename>, except that C<$basevmid> refers to the
> +C<$vmid> of the original disk volume instead.

s/original/base

> +
> +=item * C<< $isBase >> (optional)
> +
> +Whether the volume is a base disk image.
> +
> +=item * C<< $format >>
> +
> +The format of the volume. If the volume is a VM disk (C<$vtype> is
> +C<"images">), this should be whatever format the disk is in. For most
> +other content types C<"raw"> should be used.

"images" is currently also for container volumes.. import volumes also
have a format..

> +
> +See C<L<< plugindata()|/"$plugin->plugindata()" >>> for all formats.
> +
> +=back
> +
> +=cut
> +
>  sub parse_volname {
>      my ($class, $volname) = @_;
>      croak "implement me in sub-class\n";
>  }
>  
> +=head3 $plugin->get_subdir(\%scfg, $vtype)

I am not sure about this one long-term.. while the interface is generic
enough, it is only used for the helpers in PVE::Storage which are in
turn used for
- uploading/download things (templates, iso files, ..)
- creating backup archives (or rather, getting the path to the dump dir
  to create them)

> +
> +B<SITUATIONAL:> This method must be implemented for file-based storages.

s/file-based/dir-based

?

> +
> +Returns the path to the sub-directory associated with the given content type
> +(C<$vtype>). See C<L<< plugindata()|/"$plugin->plugindata()" >>> for all
> +content types.

again, this is vtype not content type..

> +
> +The default directory layout is predefined and must not be altered:
> +
> +    my $vtype_subdirs = {
> +	images   => 'images',
> +	rootdir  => 'private',
> +	iso      => 'template/iso',
> +	vztmpl   => 'template/cache',
> +	backup   => 'dump',
> +	snippets => 'snippets',
> +	import   => 'import',
> +    };
> +
> +See also: L<Storage: Directory|"https://pve.proxmox.com/wiki/Storage:_Directory">
> +
> +=cut
> +
>  sub get_subdir {
>      my ($class, $scfg, $vtype) = @_;
>      croak "implement me in sub-class\n";
>  }
>  
> +=head3 $plugin->filesystem_path(\%scfg, $volname [, $snapname])
> +
> +=head3 $plugin->filesystem_path(...)
> +
> +B<SITUATIONAL:> This method must be implemented for file-based storages.

file/dir

> +
> +Returns the absolute path on the filesystem for the given volume or snapshot.
> +In list context, returns path, guest ID and content type:

content/volume

but - this is not actually part of the plugin API. it just looks like it
because Plugin's implementation of path (which is the actual API) calls
it, and thus plugins derived from that base implement or call it as
well.

the public interface if you want a file or blockdev path for a given
volid is PVE::Storage::abs_filesystem_path, which internally also calls
the plugin's `path` method, not the non-public `filesystem_path`.

> +
> +    my $path = $plugin->filesystem_path($scfg, $volname, $snapname)
> +    my ($path, $vmid, $vtype) = $plugin->filesystem_path($scfg, $volname, $snapname)
> +
> +=cut
> +
>  sub filesystem_path {
>      my ($class, $scfg, $volname, $snapname) = @_;
>      croak "implement me in sub-class\n";
>  }
>  
> +=head3 $plugin->path(\%scfg, $volname, $storeid [, $snapname])
> +
> +B<REQUIRED:> Must be implemented in every storage plugin.
> +
> +Returns a unique path that points to the given volume or snapshot depending
> +on the underlying storage implementation. For file-based storages, this
> +method should return the same as C<L<< filesystem_path()|/"$plugin->filesystem_path(...)" >>>.

this is inverted (see above). it should also note that if there is an
actual path corresponding to a volume, it should return that, but that
it can also return something else that Qemu understands in lieu of a
real path.

if multiple paths exist, it should return the most specific/unique one
to avoid collisions between multiple plugin instances of the same
storage type.

> +
> +=cut
> +
>  sub path {
>      my ($class, $scfg, $volname, $storeid, $snapname) = @_;
>      croak "implement me in sub-class\n";
>  }
>  
> +=head3 $plugin->find_free_diskname($storeid, \%scfg, $vmid [, $fmt, $add_fmt_suffix])
> +
> +=head3 $plugin->find_free_diskname(...)
> +
> +B<REQUIRED:> Must be implemented in every storage plugin.
> +
> +Finds and returns the next available disk name, that is, the volume name
> +(C<$volname>) for a new disk image. Optionally, C<$fmt> specifies the
> +format of the disk image and C<$add_fmt_suffix> denotes whether C<$fmt>
> +should be added as suffix to the resulting name.

similarly, this is not actually part of the plugin API, it is an
implementation detail of the existing plugin hierarchy. it is currently
called by Plugin's clone, allocate and rename functionality, so if you
don't implement/override it you need to also override those (which you
likely need to do in any case..).

these two are actually a good example of what I meant with the messiness
of Plugin.pm leaking if we continue deriving *all* plugins from it,
including new external ones..

> +
> +=cut
> +
>  sub find_free_diskname {
>      my ($class, $storeid, $scfg, $vmid, $fmt, $add_fmt_suffix) = @_;
>      croak "implement me in sub-class\n";
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>