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 973D61FF164
for <inbox@lore.proxmox.com>; Fri, 11 Apr 2025 15:49:18 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 6CB7E1B7E6;
Fri, 11 Apr 2025 15:49:11 +0200 (CEST)
Date: Fri, 11 Apr 2025 15:49:06 +0200
From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: Fabian =?utf-8?Q?Gr=C3=BCnbichler?= <f.gruenbichler@proxmox.com>
Message-ID: <wkzj4l7kxucljczf4342mlazkf2wlbzculbs4q6g4mtjsvrs6f@qujn7bv6vp4a>
References: <20250326142059.261938-1-m.carrara@proxmox.com>
<20250326142059.261938-4-m.carrara@proxmox.com>
<1743423551.a0519q1x3m.astroid@yuna.none>
MIME-Version: 1.0
Content-Disposition: inline
In-Reply-To: <1743423551.a0519q1x3m.astroid@yuna.none>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.081 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>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
On Mon, Mar 31, 2025 at 05:13:10PM +0200, Fabian Gr=FCnbichler 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);
> > =
> > =3Dcut
> > =
> > +=3Dhead3 $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 iden=
tified.
> =
> 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 wi=
ll 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.
> > +
> > +=3Dcut
> > +
> > sub type {
> > croak "implement me in sub-class\n";
> > }
> > =
> > +=3Dhead3 $plugin->properties()
> > +
> > +B<OPTIONAL:> May be implemented in a storage plugin.
> > +
> > +This method should be implemented if there are additional properties t=
o be used
> > +by your plugin. Since properties are global and may be reused across p=
lugins,
> =
> 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 pr=
operties
> > +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' =3D> {
> > + description =3D> 'Host address of the ExampleStorage to connect to.',
> > + type =3D> 'string',
> > + },
> > + 'example-storage-pool' =3D> {
> > + description =3D> 'Name of the ExampleStorage pool to use.',
> > + type =3D> 'string',
> > + },
> > + # [...]
> > + };
> > + }
> > +
> > +However, it is encouraged to reuse properties of inbuilt plugins whene=
ver
> > +possible. There are a few provided properties that are regarded as I<s=
ensitive>
> > +and will be treated differently in order to not expose them or write t=
hem as
> > +plain text into configuration files. One such property fit for externa=
l use is
> > +C<password>, which you can use to provide a password, API secret, or s=
imilar.
> =
> 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.
> > +
> > +=3Dcut
> > +
> > sub properties {
> > my ($class) =3D @_;
> > return $class->SUPER::properties();
> > }
> > =
> > +=3Dhead3 $plugin->options()
> > +
> > +B<REQUIRED:> Must be implemented in every storage plugin.
> > +
> > +This method returns a hash of the properties used by the plugin. Becau=
se
> > +properties are shared among plugins, it is recommended to reuse any ex=
isting
> > +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 =3D> { optional =3D> 1 },
> > + content =3D> { optional =3D> 1 },
> > + disable =3D> { optional =3D> 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' =3D> { fixed =3D> 1 },
> > + 'example-storage-address' =3D> { fixed =3D> 1 },
> > + password =3D> { optional =3D> 1 },
> > + };
> > + }
> > +
> > +C<optional> properties are not required to be set. It is recommended t=
o set
> =
> s/set/make or "declare"?
> =
> > +most properties optional by default, unless it I<really> is required t=
o always
> > +exist.
> =
> s/it/they
> s/exist/be set
> =
> > +
> > +C<fixed> properties can only be set when creating a new storage via th=
e 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.
> > +
> > +=3Dcut
> > +
> > sub options {
> > my ($class) =3D @_;
> > return $class->SUPER::options();
> > }
> > =
> > +=3Dhead3 $plugin->plugindata()
> > +
> > +B<REQUIRED:> Must be implemented in every storage plugin.
> > +
> > +This method returns a hash that specifies additional capabilities of t=
he 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 t=
heir
> > +defaults, while the C<format> key declares B<supported disk formats> a=
nd 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 =3D> [
> > + # possible content types
> > + {
> > + images =3D> 1, # disk images
> > + rootdir =3D> 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=3D0)
> > + vztmpl =3D> 1, # container templates
> > + iso =3D> 1, # iso images
> > + backup =3D> 1, # vzdump backup files
> > + snippets =3D> 1, # snippets
> > + import =3D> 1, # imports; see explanation below
> > + none =3D> 1, # no content; see explanation below
> > + },
> > + # defaults if 'content' isn't explicitly set
> > + {
> > + images =3D> 1, # store disk images by default
> > + rootdir =3D> 1 # store containers by default
> > + # possible to add more or have no defaults
> > + }
> > + ],
> > + format =3D> [
> > + # possible disk formats
> > + {
> > + raw =3D> 1, # raw disk image
> > + qcow2 =3D> 1, # QEMU image format
> > + vmdk =3D> 1, # VMware image format
> > + subvol =3D> 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 =3D> [
> > + { images =3D> 1 },
> > + ],
> > + format =3D> [
> > + { raw =3D> 1 },
> > + "raw",
> > + ]
> > + };
> > + }
> > +
> > +Which content types and formats are supported depends on the underlyin=
g 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 corre=
sponding
> > +functionality has not yet been published to the public parts of the st=
orage
> > +API. Third-party plugins therefore should not declare this content typ=
e.
> =
> 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 s=
upport
> > +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 st=
orage
> > +API. Third-party plugins therefore should not declare this content typ=
e.
> =
> same
> =
> > +
> > +B<Regarding C<subvol>:> The C<subvol> format is used internally to all=
ow the
> > +root directories of containers to use ZFS subvolumes (also known as
> > +I<ZFS datasets>, not to be confused with I<ZVOLs>). Third-party plugin=
s should
> > +not declare this format type.
> =
> this is incomplete, but same.
(btrfs subvolumes and size=3D0 dirs)
> =
> > +
> > +There is one more key, C<select_existing>, which is used internally for
> > +ISCSI-related GUI functionality. Third-party plugins should not declar=
e this
> > +key.
> =
> this is okay here :)
> =
> > +
> > +=3Dcut
> > +
> > sub plugindata {
> > my ($class) =3D @_;
> > return $class->SUPER::plugindata();
> > }
> > =
> > +=3Dhead3 $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 prop=
erties
> > +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.
> > +
> > +=3Dcut
> > +
> > sub private {
> > - croak "implement me in sub-class\n";
> > + confess "private() is provided by PVE::Storage::Plugin and must no=
t be overridden";
> > }
> > =
> > =3Dhead2 GENERAL
> > -- =
> > 2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel