From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id A918C1FF16E
for <inbox@lore.proxmox.com>; Mon, 31 Mar 2025 17:13:27 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 214959DEC;
Mon, 31 Mar 2025 17:13:18 +0200 (CEST)
Date: Mon, 31 Mar 2025 17:13:10 +0200
From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= <f.gruenbichler@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
References: <20250326142059.261938-1-m.carrara@proxmox.com>
<20250326142059.261938-4-m.carrara@proxmox.com>
In-Reply-To: <20250326142059.261938-4-m.carrara@proxmox.com>
MIME-Version: 1.0
User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid)
Message-Id: <1743423551.a0519q1x3m.astroid@yuna.none>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.044 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH v1 pve-storage 3/8] pluginbase: document
SectionConfig methods
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
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.
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?
> +
> +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