On Mon, Mar 31, 2025 at 05:13:10PM +0200, 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.carr...@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.
>
> 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.
While in perl we seem to parse this via `/\S+/` (greedily), I'd argue we
should request to limit this to `[a-z\-]`...
Technically, `PVE::SectionConfig->parse_section_header()` would parse
this line:
My:Plugin"with-a-BAD:name
as a header of type `My:Plugin"with-a-BAD` and section id `name`...
We may want to fix that...
>
> > +
> > +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?
>
> > +
> > +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 },
^ Currently these 3 need to actually be listed by the plugin, but we
probably want to enforce that they exist?
If the storage doesn't list the `disable` flag you, well, cannot disable
it...
> > + '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
Side note...
`rootdir` is one we may need to rethink in the future.
The actual volume type for a container is still actually an "image".
(pve-container uses `vdisk_alloc` to get a `raw` file and then runs
`mkfs` on it or uses a *subvol* type (zfs, btrfs, or dirplugin with
size=0)
> > + 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.
(btrfs subvolumes and size=0 dirs)
>
> > +
> > +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
