Re: [pve-devel] [PATCH v1 pve-storage 3/8] pluginbase: document SectionConfig methods

["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:
> > This commit adds docstrings for the relevant PVE::SectionConfig
> > methods in the context of the storage plugin API.
> > 
> > Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> > ---
> >  src/PVE/Storage/PluginBase.pm | 194 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 192 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/PVE/Storage/PluginBase.pm b/src/PVE/Storage/PluginBase.pm
> > index 16977f3..5f7e6fd 100644
> > --- a/src/PVE/Storage/PluginBase.pm
> > +++ b/src/PVE/Storage/PluginBase.pm
> > @@ -88,7 +88,7 @@ package PVE::Storage::PluginBase;
> >  use strict;
> >  use warnings;
> >  
> > -use Carp qw(croak);
> > +use Carp qw(croak confess);
> >  
> >  use parent qw(PVE::SectionConfig);
> >  
> > @@ -100,27 +100,217 @@ use parent qw(PVE::SectionConfig);
> >  
> >  =cut
> >  
> > +=head3 $plugin->type()
> > +
> > +B<REQUIRED:> Must be implemented in every storage plugin.

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

>
> I am not sure whether we should do
>
> s/implemented in/implemented by
>
> for this whole thing and SectionConfig, but I won't note it for every
> instance ;)
>
> > +
> > +This should return a string with which the plugin can be uniquely identified.
>
> a string which uniquely identifies the plugin.
>
> > +Any string is acceptable, as long as it's descriptive and you're sure it won't
> > +conflict with another plugin. In the cases of built-in plugins, you will often
> > +find the filesystem name or something similar being used.
>
> I'd replace that with something like:
>
> Commonly, the name of the storage technology or filesystem supported by
> the plugin is used. If it is necessary to differentiate multiple
> plugins for the same storage technology/filesystem, add a suffix
> denoting the supported variant or feature set.
>
> > +
> > +See C<L<PVE::SectionConfig/type>> for more information.
> > +
> > +=cut
> > +
> >  sub type {
> >      croak "implement me in sub-class\n";
> >  }
> >  
> > +=head3 $plugin->properties()
> > +
> > +B<OPTIONAL:> May be implemented in a storage plugin.
> > +
> > +This method should be implemented if there are additional properties to be used
> > +by your plugin. Since properties are global and may be reused across plugins,
>
> s/used/registered
>
> IMHO that conveys better what the difference between properties and
> options are..
>
> "global and may be reused across plugins" is kinda superfluous, maybe
>
> "Since properties share a global namespace across all plugins"?
>
> > +the names of properties must not collide with one another.
>
> each property can only be registered once by a single plugin, but used
> as an option by many.
>
> > +
> > +When implementing a third-party plugin, it is recommended to prefix properties
> > +with some kind of identifier, like so:
>
> Third-party plugins should therefore prefix any properties they register
> with their plugin type, for example:
>
> > +
> > +    sub properties {
> > +	return {
> > +	    'example-storage-address' => {
> > +		description => 'Host address of the ExampleStorage to connect to.',
> > +		type => 'string',
> > +	    },
> > +	    'example-storage-pool' => {
> > +		description => 'Name of the ExampleStorage pool to use.',
> > +		type => 'string',
> > +	    },
> > +	    # [...]
> > +	};
> > +    }
> > +
> > +However, it is encouraged to reuse properties of inbuilt plugins whenever
> > +possible. There are a few provided properties that are regarded as I<sensitive>
> > +and will be treated differently in order to not expose them or write them as
> > +plain text into configuration files. One such property fit for external use is
> > +C<password>, which you can use to provide a password, API secret, or similar.
>
> If possible, generic properties registered by built-in plugins should be
> reused.
>
> (inbuilt is a very weird, very British word ;))
>
> the rest of that is kinda misplaced here and should be its own section
> somewhere (it affects options just as much as properties). we should
> probably allow registering sensitive properties for plugins?

Yes, we should! And Fiona's already on it 🎉 [0]

[0]: https://lore.proxmox.com/pve-devel/20250321134852.103871-11-f.ebner@proxmox.com/

>
> > +
> > +See C<L<PVE::SectionConfig/properties>> for more information.
> > +
> > +=cut
> > +
> >  sub properties {
> >      my ($class) = @_;
> >      return $class->SUPER::properties();
> >  }
> >  
> > +=head3 $plugin->options()
> > +
> > +B<REQUIRED:> Must be implemented in every storage plugin.
> > +
> > +This method returns a hash of the properties used by the plugin. Because
> > +properties are shared among plugins, it is recommended to reuse any existing
> > +ones of inbuilt plugins and only define custom properties via
> > +C<L<< properties()|/"$plugin->properties()" >>> if necessary.
>
> Everything but the first sentence can be a reference to the properties
> part above..
>
> > +
> > +The properties a plugin uses are then declared as follows:
>
> or just, "For example:" ? ;)
>
> > +
> > +    sub options {
> > +	return {
> > +	    nodes => { optional => 1 },
> > +	    content => { optional => 1 },
> > +	    disable => { optional => 1 },
> > +	    'example-storage-pool' => { fixed => 1 },
> > +	    'example-storage-address' => { fixed => 1 },
> > +	    password => { optional => 1 },
> > +	};
> > +    }
> > +
> > +C<optional> properties are not required to be set. It is recommended to set
>
> s/set/make or "declare"?
>
> > +most properties optional by default, unless it I<really> is required to always
> > +exist.
>
> s/it/they
> s/exist/be set
>
> > +
> > +C<fixed> properties can only be set when creating a new storage via the plugin
> > +and cannot be changed afterwards.
>
> s/creating a new storage/adding a new storage config entry/ ? a bit more
> precise to differentiate it from actually "creating a storage" as in
> formatting a disk/..
>
> I am not sure what "via the plugin" is supposed to mean there, I think
> it can just be dropped.
>
> > +
> > +See C<L<PVE::SectionConfig/options>> for more information.
> > +
> > +=cut
> > +
> >  sub options {
> >      my ($class) = @_;
> >      return $class->SUPER::options();
> >  }
> >  
> > +=head3 $plugin->plugindata()
> > +
> > +B<REQUIRED:> Must be implemented in every storage plugin.
> > +
> > +This method returns a hash that specifies additional capabilities of the storage
> > +plugin, such as what kinds of data may be stored on it or what VM disk formats
>
> s/data/content or s/data/volumes
>
> > +the storage supports. Additionally, defaults may also be set. This is done
> > +through the C<content> and C<format> keys.
>
> but how about re-organizing it a bit?
>
> This method returns a hash declaring which content types and image
> formats this plugin (or the underlying storage) supports.
>
> and then describe each key?
>
> > +
> > +The C<content> key is used to declare B<supported content types> and their
> > +defaults, while the C<format> key declares B<supported disk formats> and the
> > +default disk format of the storage:
>
> "and their defaults" sounds weird.
>
> The 'content' key stores a list containing two hashes. The first list
> element is a hash declaring which content types are supported by the
> plugin. The second list element is a hash declaring which content types
> are used by default if no explicit 'content' option is set on a plugin
> instance.
>
> and maybe link somewhere else for a description of the content type
> values, so that there is a single place that various parts that need it
> can link to?
>
> and then
>
> The 'format' key stores a list containing two hashes. The first list
> element is a hash declaring which image formats are supported by the
> storage plugin. The second list element is the default image format that
> should be used for images on the storage if non is explicitly provided.
>
> > +
> > +    sub plugindata {
> > +	return {
> > +	    content => [
> > +		# possible content types
> > +		{
> > +		    images => 1,   # disk images
> > +		    rootdir => 1,  # container root directories
> > +		    vztmpl => 1,   # container templates
> > +		    iso => 1,      # iso images
> > +		    backup => 1,   # vzdump backup files
> > +		    snippets => 1, # snippets
> > +		    import => 1,   # imports; see explanation below
> > +		    none => 1,     # no content; see explanation below
> > +		},
> > +		# defaults if 'content' isn't explicitly set
> > +		{
> > +		    images => 1,   # store disk images by default
> > +		    rootdir => 1   # store containers by default
> > +		    # possible to add more or have no defaults
> > +		}
> > +	    ],
> > +	    format => [
> > +		# possible disk formats
> > +		{
> > +		    raw => 1,   # raw disk image
> > +		    qcow2 => 1, # QEMU image format
> > +		    vmdk => 1,  # VMware image format
> > +		    subvol => 1 # subvolumes; see explanation below
> > +		},
> > +		"qcow2" # default if 'format' isn't explicitly set
> > +	    ]
> > +	    # [...]
> > +	};
> > +    }
> > +
> > +While the example above depicts a rather capable storage, the following
> > +shows a simpler storage that can only be used for VM disks:
>
> raw-formatted VM disks
>
> > +
> > +    sub plugindata {
> > +	return {
> > +	    content => [
> > +		{ images => 1 },
> > +	    ],
> > +	    format => [
> > +		{ raw => 1 },
> > +		"raw",
> > +	    ]
> > +	};
> > +    }
> > +
> > +Which content types and formats are supported depends on the underlying storage
> > +implementation.
> > +
> > +B<Regarding C<import>:> The C<import> content type is used internally to
> > +interface with virtual guests of foreign sources or formats. The corresponding
> > +functionality has not yet been published to the public parts of the storage
> > +API. Third-party plugins therefore should not declare this content type.
>
> this should live somewhere where content types are described
>
> > +
> > +B<Regarding C<none>:> The C<none> content type denotes the I<absence> of other
> > +types of content, i.e. this content type may only be set on a storage if no
> > +other content type is set. This is used internally for storages that support
> > +adding another storage "on top" of them; at the moment, this makes it possible
> > +to set up an LVM (thin) pool on top of an iSCSI LUN. The corresponding
> > +functionality has not yet been published to the public parts of the storage
> > +API. Third-party plugins therefore should not declare this content type.
>
> same
>
> > +
> > +B<Regarding C<subvol>:> The C<subvol> format is used internally to allow the
> > +root directories of containers to use ZFS subvolumes (also known as
> > +I<ZFS datasets>, not to be confused with I<ZVOLs>). Third-party plugins should
> > +not declare this format type.
>
> this is incomplete, but same.
>
> > +
> > +There is one more key, C<select_existing>, which is used internally for
> > +ISCSI-related GUI functionality. Third-party plugins should not declare this
> > +key.
>
> this is okay here :)
>
> > +
> > +=cut
> > +
> >  sub plugindata {
> >      my ($class) = @_;
> >      return $class->SUPER::plugindata();
> >  }
> >  
> > +=head3 $plugin->private()
> > +
> > +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.>
> > +
> > +Returns a hash containing the tracked plugin metadata, most notably the
> > +C<propertyList>, which contains all known properties of all plugins.
> > +
> > +C<L<PVE::Storage::Plugin>> uses this to predefine a lot of useful properties
> > +that are relevant for all plugins. Core functionality such as defining
> > +whether a storage is shared, which nodes may use it, whether a storage
> > +is enabled or not, etc. are implemented via these properties.
>
> this is not true and sounds like it is very interesting to look at/mess
> with ;) in reality, it is part of SectionConfig machinery, so just say
> that and that it must not be overridden and be done with it?
>
> if PluginBase becomes the real base plugin then it and some other
> helpers would need to move here anyway..
>
> > +
> > +See C<L<PVE::SectionConfig/private>> for more information.
> > +
> > +=cut
> > +
> >  sub private {
> > -    croak "implement me in sub-class\n";
> > +    confess "private() is provided by PVE::Storage::Plugin and must not be overridden";
> >  }
> >  
> >  =head2 GENERAL
> > -- 
> > 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