On Wed, Apr 02, 2025 at 06:31:54PM +0200, Max Carrara wrote:
> 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
^ Remove or document as deprecated for public APIs.
> > > - parse_volname
> > > - get_subdir
^ Remove or document as deprecated for public APIs.
> > > - filesystem_path
^ Remove or document as deprecated for public APIs.
> > > - path
> > > - find_free_diskname
^ Remove or document as deprecated for public APIs.
While this has been mentioned in the replies for `get_subdir` and
`find_free_diskname`, this is also true for `filesystem_path`.
`filesystem_path` is actually a "DirPlugin API".
Remember, not all storages *have* a file system path...
> > >
> > > Signed-off-by: Max Carrara <m.carr...@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.
(I mean, some storages "know" when things are in-use, so we could
probably start using this for storages which opt-in... but meh...
People *are* going to continue complaining about this, though ;-) )
> > > +
> > > +=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.
I think we should make this one optional soon.
Currently we have some places where we blindly call this, but not all
storages have a path.
> > > +
> > > +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.
Particularly with the backup-provider API we may have storages with
neither paths nor URLs for qemu...
An example where we currently needlessly require this method is when
*deleting* a volume where the API endpoint calls `path()` to get the
volume type, which may as well use `parse_volname()` instead, then
deleting works without this method implemented.
> >
> > 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!
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
