Re: [pve-devel] [PATCH v1 pve-storage 1/8] pluginbase: introduce PVE::Storage::PluginBase with doc scaffold

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

On Mon Mar 31, 2025 at 5:13 PM CEST, Fabian Grünbichler wrote:
> On March 26, 2025 3:20 pm, Max Carrara wrote:
> > Add PVE::Storage::PluginBase, which defines stubs for all methods that
> > storage plugins should implement in order to conform to our plugin
> > API. This makes it much easier for (third-party) developers to see
> > which methods should be implemented.
> > 
> > PluginBase is inserted into the inheritance chain between
> > PVE::Storage::Plugin and PVE::SectionConfig instead of letting the
> > Plugin module inherit from SectionConfig directly. This keeps the
> > inheritance chain linear, avoiding multiple inheritance.
> > 
> > Also provide a scaffold for documentation. Preserve pre-existing
> > comments for context's sake.
> > 
> > Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> > ---
> >  src/PVE/Storage/Makefile      |   1 +
> >  src/PVE/Storage/Plugin.pm     |   2 +-
> >  src/PVE/Storage/PluginBase.pm | 328 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 330 insertions(+), 1 deletion(-)
> >  create mode 100644 src/PVE/Storage/PluginBase.pm
> > 
> > diff --git a/src/PVE/Storage/Makefile b/src/PVE/Storage/Makefile
> > index ce3fd68..f2cdb66 100644
> > --- a/src/PVE/Storage/Makefile
> > +++ b/src/PVE/Storage/Makefile
> > @@ -1,6 +1,7 @@
> >  SOURCES= \
> >  	Common.pm \
> >  	Plugin.pm \
> > +	PluginBase.pm \
> >  	DirPlugin.pm \
> >  	LVMPlugin.pm \
> >  	NFSPlugin.pm \
> > diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> > index 65cf43f..df6882a 100644
> > --- a/src/PVE/Storage/Plugin.pm
> > +++ b/src/PVE/Storage/Plugin.pm
> > @@ -19,7 +19,7 @@ use PVE::Storage::Common;
> >  
> >  use JSON;
> >  
> > -use base qw(PVE::SectionConfig);
> > +use parent qw(PVE::Storage::PluginBase);
> >  
> >  use constant KNOWN_COMPRESSION_FORMATS =>  ('gz', 'lzo', 'zst', 'bz2');
> >  use constant COMPRESSOR_RE => join('|', KNOWN_COMPRESSION_FORMATS);
> > diff --git a/src/PVE/Storage/PluginBase.pm b/src/PVE/Storage/PluginBase.pm
> > new file mode 100644
> > index 0000000..e56aa72
> > --- /dev/null
> > +++ b/src/PVE/Storage/PluginBase.pm
> > @@ -0,0 +1,328 @@
> > +=head1 NAME
> > +
> > +C<PVE::Storage::PluginBase> - Storage Plugin API Interface
> > +
> > +=head1 DESCRIPTION
> > +
> > +=cut
> > +
> > +package PVE::Storage::PluginBase;
> > +
> > +use strict;
> > +use warnings;
> > +
> > +use Carp qw(croak);
> > +
> > +use parent qw(PVE::SectionConfig);
> > +
> > +=head1 PLUGIN INTERFACE METHODS
> > +
> > +=cut
> > +
> > +=head2 PLUGIN DEFINITION
> > +
> > +=cut
> > +
> > +sub type {
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub properties {
> > +    my ($class) = @_;
> > +    return $class->SUPER::properties();
> > +}
> > +
> > +sub options {
> > +    my ($class) = @_;
> > +    return $class->SUPER::options();
> > +}
> > +
> > +sub plugindata {
> > +    my ($class) = @_;
> > +    return $class->SUPER::plugindata();
> > +}
> > +
> > +sub private {
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +=head2 GENERAL
> > +
> > +=cut
> > +
> > +sub check_connection {
> > +    my ($class, $storeid, $scfg) = @_;
> > +
> > +    return 1;
> > +}
> > +
> > +sub activate_storage {
> > +    my ($class, $storeid, $scfg, $cache) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub deactivate_storage {
> > +    my ($class, $storeid, $scfg, $cache) = @_;
> > +
> > +    return;
> > +}
> > +
> > +sub status {
> > +    my ($class, $storeid, $scfg, $cache) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub cluster_lock_storage {
> > +    my ($class, $storeid, $shared, $timeout, $func, @param) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub parse_volname {
> > +    my ($class, $volname) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub get_subdir {
> > +    my ($class, $scfg, $vtype) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub filesystem_path {
> > +    my ($class, $scfg, $volname, $snapname) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub path {
> > +    my ($class, $scfg, $volname, $storeid, $snapname) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub find_free_diskname {
> > +    my ($class, $storeid, $scfg, $vmid, $fmt, $add_fmt_suffix) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +=head2 HOOKS
> > +
> > +=cut
> > +
> > +# called during addition of storage (before the new storage config got written)
>
> called when adding a storage config entry, before the new config gets written
>
> > +# die to abort addition if there are (grave) problems
> > +# NOTE: runs in a storage config *locked* context
> > +sub on_add_hook {
> > +    my ($class, $storeid, $scfg, %param) = @_;
> > +    return undef;
> > +}
> > +
> > +# called during storage configuration update (before the updated storage config got written)
>
> called when updating a storage config entry, before the updated config
> gets written
>
> > +# die to abort the update if there are (grave) problems
> > +# NOTE: runs in a storage config *locked* context
> > +sub on_update_hook {
> > +    my ($class, $storeid, $scfg, %param) = @_;
> > +    return undef;
> > +}
> > +
> > +# called during deletion of storage (before the new storage config got written)
> > +# and if the activate check on addition fails, to cleanup all storage traces
> > +# which on_add_hook may have created.
>
> called when deleting a storage config entry, before the new storage
> config gets written.
>
> also called as part of error handling when undoing the addition of a new
> storage config entry.

Regarding your three responses above: The comments here were preserved
from `::Plugin` for context's sake. But tbh, on second thought, they can
probably just be removed, as they'll be replaced by POD anyways.

>
> > +# die to abort deletion if there are (very grave) problems
> > +# NOTE: runs in a storage config *locked* context
> > +sub on_delete_hook {
> > +    my ($class, $storeid, $scfg) = @_;
> > +    return undef;
> > +}
> > +
> > +=head2 IMAGE OPERATIONS
> > +
>
> should this describe what IMAGES are in the context of PVE? else as a
> newcomer the difference between IMAGE here and VOLUME below might not
> be clear..

Yeah, that's a good idea. Maximiliano and I were also thinking about
maybe adding a GLOSSARY section at the bottom of the file where certain
terms could be explained / defined in more detail in general.
What do you think?

Alternatively, we could also have the top-level description define the
most basic of terms, but I don't want to load the docs here with too
much information up front.

>
> > +=cut
> > +
> > +sub list_images {
> > +    my ($class, $storeid, $scfg, $vmid, $vollist, $cache) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub create_base {
> > +    my ($class, $storeid, $scfg, $volname) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub clone_image {
> > +    my ($class, $scfg, $storeid, $volname, $vmid, $snap) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub alloc_image {
> > +    my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub free_image {
> > +    my ($class, $storeid, $scfg, $volname, $isBase, $format) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +=head2 VOLUME OPERATIONS
>
> see above
>
> > +
> > +=cut
> > +
> > +sub list_volumes {
> > +    my ($class, $storeid, $scfg, $vmid, $content_types) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +# 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.
> > +# protected - not to be removed by free_image, and for backups, ignored when pruning.
> > +sub get_volume_attribute {
> > +    my ($class, $scfg, $storeid, $volname, $attribute) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +# Dies if the attribute is not supported for the volume.
> > +sub update_volume_attribute {
> > +    my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub volume_size_info {
> > +    my ($class, $scfg, $storeid, $volname, $timeout) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub volume_resize {
> > +    my ($class, $scfg, $storeid, $volname, $size, $running) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub volume_snapshot {
> > +    my ($class, $scfg, $storeid, $volname, $snap) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +# Returns a hash with the snapshot names as keys and the following data:
> > +# id        - Unique id to distinguish different snapshots even if the have the same name.
> > +# timestamp - Creation time of the snapshot (seconds since epoch).
> > +# Returns an empty hash if the volume does not exist.
> > +sub volume_snapshot_info {
> > +    my ($class, $scfg, $storeid, $volname) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +# Asserts that a rollback to $snap on $volname is possible.
> > +# If certain snapshots are preventing the rollback and $blockers is an array
> > +# reference, the snapshot names can be pushed onto $blockers prior to dying.
> > +sub volume_rollback_is_possible {
> > +    my ($class, $scfg, $storeid, $volname, $snap, $blockers) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub volume_snapshot_rollback {
> > +    my ($class, $scfg, $storeid, $volname, $snap) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub volume_snapshot_delete {
> > +    my ($class, $scfg, $storeid, $volname, $snap, $running) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub volume_snapshot_needs_fsfreeze {
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub storage_can_replicate {
> > +    my ($class, $scfg, $storeid, $format) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub volume_has_feature {
> > +    my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running, $opts) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub map_volume {
> > +    my ($class, $storeid, $scfg, $volname, $snapname) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub unmap_volume {
> > +    my ($class, $storeid, $scfg, $volname, $snapname) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub activate_volume {
> > +    my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub deactivate_volume {
> > +    my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub rename_volume {
> > +    my ($class, $scfg, $storeid, $source_volname, $target_vmid, $target_volname) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub prune_backups {
> > +    my ($class, $scfg, $storeid, $keep, $vmid, $type, $dryrun, $logfunc) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +=head2 IMPORTS AND EXPORTS
> > +
> > +=cut
> > +
> > +# Import/Export interface:
> > +#   Any path based storage is assumed to support 'raw' and 'tar' streams, so
> > +#   the default implementations will return this if $scfg->{path} is set,
> > +#   mimicking the old PVE::Storage::storage_migrate() function.
> > +#
> > +# Plugins may fall back to PVE::Storage::Plugin::volume_{export,import}...
> > +#   functions in case the format doesn't match their specialized
> > +#   implementations to reuse the raw/tar code.
> > +#
> > +# Format specification:
> > +#   The following formats are all prefixed with image information in the form
> > +#   of a 64 bit little endian unsigned integer (pack('Q<')) in order to be able
> > +#   to preallocate the image on storages which require it.
> > +#
> > +#   raw+size: (image files only)
> > +#     A raw binary data stream such as produced via `dd if=TheImageFile`.
> > +#   qcow2+size, vmdk: (image files only)
> > +#     A raw qcow2/vmdk/... file such as produced via `dd if=some.qcow2` for
> > +#     files which are already in qcow2 format, or via `qemu-img convert`.
> > +#     Note that these formats are only valid with $with_snapshots being true.
> > +#   tar+size: (subvolumes only)
> > +#     A GNU tar stream containing just the inner contents of the subvolume.
> > +#     This does not distinguish between the contents of a privileged or
> > +#     unprivileged container. In other words, this is from the root user
> > +#     namespace's point of view with no uid-mapping in effect.
> > +#     As produced via `tar -C vm-100-disk-1.subvol -cpf TheOutputFile.dat .`
> > +
> > +# Export a volume into a file handle as a stream of desired format.
> > +sub volume_export {
> > +    my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot, $base_snapshot, $with_snapshots) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub volume_export_formats {
> > +    my ($class, $scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +# Import data from a stream, creating a new or replacing or adding to an existing volume.
> > +sub volume_import {
> > +    my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot, $base_snapshot, $with_snapshots, $allow_rename) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +sub volume_import_formats {
> > +    my ($class, $scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots) = @_;
> > +    croak "implement me in sub-class\n";
> > +}
> > +
> > +1;
> > -- 
> > 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