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

["Max Carrara" <m.carrara@proxmox.com>]

On Mon Mar 31, 2025 at 5:12 PM CEST, Fabian Grünbichler wrote:
> 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@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)" >>>.

Upfront note: Unless I otherwise comment something, I agree with you.
Just sparing myself from writing and you from reading too many ACKs :P

>
> 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/

Hmm, I'd say so, yeah. The original message isn't on lore apparently, so
I'll see if I can dig this up in my mailbox later.

>
> ?
>
> > +
> > +    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.

Oh, right. Thanks for pointing this out! I'll see if I can note down
those differences somewhere.

>
> > +
> > +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?

AFAIR yes. But now you made me check again soon. :P

>
> > +
> > +=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..

Yeah, Wolfgang also told me this last week a little after I sent in this
series. Will correct the docs.

>
> > +
> > +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..

... Yeah, this just reaffirms my stance that a proper investigative look
and documentation for all of this *really* necessary. Even after this
digital archaeology tour we've apparently missed a lot of details, or in
the case of the two methods above, included details that aren't actually
part of the plugin API.

Thanks for going through all of this, by the way -- it's really highly
appreciated!

>
> > +
> > +=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@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> > 
> > 
> > 
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel