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 102331FF164
for <inbox@lore.proxmox.com>; Fri, 28 Mar 2025 14:08:12 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 944702E7F;
Fri, 28 Mar 2025 14:08:03 +0100 (CET)
References: <20250326142059.261938-1-m.carrara@proxmox.com>
<20250326142059.261938-5-m.carrara@proxmox.com>
User-agent: mu4e 1.10.8; emacs 30.1
From: Maximiliano Sandoval <m.sandoval@proxmox.com>
To: Max Carrara <m.carrara@proxmox.com>
Date: Fri, 28 Mar 2025 13:50:56 +0100
In-reply-to: <20250326142059.261938-5-m.carrara@proxmox.com>
Message-ID: <s8ozfh5wbfy.fsf@proxmox.com>
MIME-Version: 1.0
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.096 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
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
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 4/8] pluginbase: document
general plugin 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: 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>
Max Carrara <m.carrara@proxmox.com> writes:
> Add docstrings for the following methods:
> - check_connection
> - activate_storage
> - deactivate_storage
> - status
> - cluster_lock_storage
> - parse_volname
> - get_subdir
> - filesystem_path
> - path
> - find_free_diskname
>
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
> src/PVE/Storage/PluginBase.pm | 255 ++++++++++++++++++++++++++++++++++
> 1 file changed, 255 insertions(+)
>
> diff --git a/src/PVE/Storage/PluginBase.pm b/src/PVE/Storage/PluginBase.pm
> index 5f7e6fd..8a61dc3 100644
> --- a/src/PVE/Storage/PluginBase.pm
> +++ b/src/PVE/Storage/PluginBase.pm
> @@ -317,53 +317,308 @@ sub private {
>
> =cut
>
> +=head3 $plugin->check_connection($storeid, \%scfg)
> +
> +B<OPTIONAL:> May be implemented in a storage plugin.
> +
> +Performs a connection check.
> +
> +This method is useful for plugins that require some kind of network connection
> +or similar and is called before C<L<< activate_storage()|/"$plugin->activate_storage($storeid, \%scfg, \%cache)" >>>.
> +
> +Returns C<1> by default and C<0> if the storage isn't online / reachable.
small nit: I think this might read a bit better if it was e.g.
```
Returns whether the storage is online and/or reachable.
```
or
```
Returns C<1> when the storage is online and/or reachable, and C<0> otherwise.
```
> +Returns C<1> by default and C<0> if the storage isn't online / reachable.
> +
> +C<die>s if an error occurs while performing the connection check.
> +
> +Note that this method's purpose is to simply verify that the storage is
> +I<reachable>, and not necessarily that authentication etc. succeeds.
> +
> +For example: If the storage is mainly accessed via TCP, you can try to simply
> +open a TCP connection to see if it's online:
> +
> + # In custom module:
> +
> + use PVE::Network;
> + use parent qw(PVE::Storage::Plugin)
> +
> + # [...]
> +
> + sub check_connection {
> + my ($class, $storeid, $scfg) = @_;
> +
> + my $port = $scfg->{port} || 5432;
> + return PVE::Network::tcp_ping($scfg->{server}, $port, 5);
> + }
> +
> +=cut
> +
> sub check_connection {
> my ($class, $storeid, $scfg) = @_;
>
> return 1;
> }
>
> +=head3 $plugin->activate_storage($storeid, \%scfg, \%cache)
> +
> +=head3 $plugin->activate_storage(...)
> +
> +B<REQUIRED:> Must be implemented in every storage plugin.
> +
> +Activates the storage, making it ready for further use.
> +
> +In essence, this method performs the steps necessary so that the storage can be
> +used by remaining parts of the system.
> +
> +In the case of file-based storages, this usually entails creating the directory
> +of the mountpoint, mounting the storage and then creating the directories for
> +the different content types that the storage has enabled. See
> +C<L<PVE::Storage::NFSPlugin>> and C<L<PVE::Storage::CIFSPlugin>> for examples
> +in that regard.
> +
> +Other types of storages would use this method for establishing a connection to
> +the storage and authenticating with it or similar. See C<L<PVE::Storage::ISCSIPlugin>>
> +for an example.
> +
> +If the storage doesn't need to be activated in some way, this method can be a
> +no-op.
> +
> +C<die>s in case of errors.
> +
> +This method may reuse L<< cached information via C<\%cache>|/"CACHING EXPENSIVE OPERATIONS" >>.
> +
> +=cut
> +
> sub activate_storage {
> my ($class, $storeid, $scfg, $cache) = @_;
> croak "implement me in sub-class\n";
> }
>
> +=head3 $plugin->deactivate_storage($storeid, \%scfg, \%cache)
> +
> +=head3 $plugin->deactivate_storage(...)
> +
> +B<OPTIONAL:> May be implemented in a storage plugin.
> +
> +Deactivates the storage. Counterpart to C<L<< activate_storage()|/"$plugin->activate_storage(...)" >>>.
> +
> +This method is used to make the storage unavailable to the rest of the system,
> +which usually entails unmounting the storage, closing an established
> +connection, or similar.
> +
> +Does nothing by default.
> +
> +C<die>s in case of errors.
> +
> +This method may reuse L<< cached information via C<\%cache>|/"CACHING EXPENSIVE OPERATIONS" >>.
> +
> +B<NOTE:> This method is currently not used by our API due to historical
> +reasons.
> +
> +=cut
> +
> sub deactivate_storage {
> my ($class, $storeid, $scfg, $cache) = @_;
>
> return;
> }
>
> +=head3 $plugin->status($storeid, \%scfg, \%cache)
> +
> +=head3 $plugin->status(...)
> +
> +B<REQUIRED:> Must be implemented in every storage plugin.
> +
> +Returns a list of scalars in the following form:
> +
> + my ($total, $available, $used, $active) = $plugin->status($storeid, $scfg, $cache)
> +
> +This list contains the C<$total>, C<$available> and C<$used> storage capacity,
> +respectively, as well as whether the storage is C<$active> or not.
> +
> +This method may reuse L<< cached information via C<\%cache>|/"CACHING EXPENSIVE OPERATIONS" >>.
> +
> +=cut
> +
> sub status {
> my ($class, $storeid, $scfg, $cache) = @_;
> croak "implement me in sub-class\n";
> }
>
> +=head3 $plugin->cluster_lock_storage($storeid, $shared, $timeout, \&func, @param)
> +
> +=head3 $plugin->cluster_lock_storage(...)
> +
> +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.>
> +
> +Locks the storage with the given C<$storeid> for the entire host and runs the
> +given C<\&func> with the given C<@param>s, unlocking the storage once finished.
> +If the storage is C<$shared>, it is instead locked on the entire cluster. If
> +the storage is already locked, wait for at most the number of seconds given by
> +C<$timeout>.
> +
> +This method is used to synchronize access to the given storage, preventing
> +simultaneous modification from multiple workers. This is necessary for
> +operations that modify data on the storage directly, like cloning and
> +allocating images.
> +
> +=cut
> +
> sub cluster_lock_storage {
> my ($class, $storeid, $shared, $timeout, $func, @param) = @_;
> croak "implement me in sub-class\n";
> }
>
> +=head3 $plugin->parse_volname($volname)
> +
> +B<REQUIRED:> Must be implemented in every storage plugin.
> +
> +Parses C<$volname>, returning a list representing the parts encoded in
> +the volume name:
> +
> + my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format)
> + = $plugin->parse_volname($volname);
> +
> +Not all parts need to be included in the list. Those marked as I<optional>
> +in the list below may be set to C<undef> if not applicable.
> +
> +This method may C<die> in case of errors.
> +
> +=over
> +
> +=item * C<< $vtype >>
> +
> +The content type ("volume type") of the volume, e.g. C<"images">, C<"iso">,
> +etc.
> +
> +See C<L<< plugindata()|/"$plugin->plugindata()" >>> for all content types.
> +
> +=item * C<< $name >>
> +
> +The display name of the volume. This is usually what the underlying storage
> +itself uses to address the volume.
> +
> +For example, disks for virtual machines that are stored on LVM thin pools are
> +named C<vm-100-disk-0>, C<vm-1337-disk-1>, etc. That would be the C<$name> in
> +this case.
> +
> +=item * C<< $vmid >> (optional)
> +
> +The ID of the guest that owns the volume.
> +
> +=item * C<< $basename >> (optional)
> +
> +The C<$name> of the volume this volume is based on. Whether this part
> +is returned or not depends on the plugin and underlying storage.
>
I would personally use something along the lines of:
```
Whether the parameter is defined or not depends...
```
> +Only applies to disk images.
> +
> +For example, on ZFS, if the VM is a linked clone, C<$basename> refers
> +to the C<$name> of the original disk volume that the parsed disk volume
> +corresponds to.
> +
> +=item * C<< $basevmid >> (optional)
> +
> +Equivalent to C<$basename>, except that C<$basevmid> refers to the
> +C<$vmid> of the original disk volume instead.
> +
> +=item * C<< $isBase >> (optional)
> +
> +Whether the volume is a base disk image.
> +
> +=item * C<< $format >>
> +
> +The format of the volume. If the volume is a VM disk (C<$vtype> is
> +C<"images">), this should be whatever format the disk is in. For most
> +other content types C<"raw"> should be used.
> +
> +See C<L<< plugindata()|/"$plugin->plugindata()" >>> for all formats.
> +
> +=back
> +
> +=cut
> +
> sub parse_volname {
> my ($class, $volname) = @_;
> croak "implement me in sub-class\n";
> }
>
> +=head3 $plugin->get_subdir(\%scfg, $vtype)
> +
> +B<SITUATIONAL:> This method must be implemented for file-based storages.
> +
> +Returns the path to the sub-directory associated with the given content type
> +(C<$vtype>). See C<L<< plugindata()|/"$plugin->plugindata()" >>> for all
> +content types.
> +
> +The default directory layout is predefined and must not be altered:
> +
> + my $vtype_subdirs = {
> + images => 'images',
> + rootdir => 'private',
> + iso => 'template/iso',
> + vztmpl => 'template/cache',
> + backup => 'dump',
> + snippets => 'snippets',
> + import => 'import',
> + };
> +
> +See also: L<Storage: Directory|"https://pve.proxmox.com/wiki/Storage:_Directory">
> +
> +=cut
> +
> sub get_subdir {
> my ($class, $scfg, $vtype) = @_;
> croak "implement me in sub-class\n";
> }
>
> +=head3 $plugin->filesystem_path(\%scfg, $volname [, $snapname])
> +
> +=head3 $plugin->filesystem_path(...)
> +
> +B<SITUATIONAL:> This method must be implemented for file-based storages.
> +
> +Returns the absolute path on the filesystem for the given volume or snapshot.
> +In list context, returns path, guest ID and content type:
> +
> + my $path = $plugin->filesystem_path($scfg, $volname, $snapname)
> + my ($path, $vmid, $vtype) = $plugin->filesystem_path($scfg, $volname, $snapname)
> +
> +=cut
> +
> sub filesystem_path {
> my ($class, $scfg, $volname, $snapname) = @_;
> croak "implement me in sub-class\n";
> }
>
> +=head3 $plugin->path(\%scfg, $volname, $storeid [, $snapname])
> +
> +B<REQUIRED:> Must be implemented in every storage plugin.
> +
> +Returns a unique path that points to the given volume or snapshot depending
> +on the underlying storage implementation. For file-based storages, this
> +method should return the same as C<L<< filesystem_path()|/"$plugin->filesystem_path(...)" >>>.
> +
> +=cut
> +
> sub path {
> my ($class, $scfg, $volname, $storeid, $snapname) = @_;
> croak "implement me in sub-class\n";
> }
>
> +=head3 $plugin->find_free_diskname($storeid, \%scfg, $vmid [, $fmt, $add_fmt_suffix])
> +
> +=head3 $plugin->find_free_diskname(...)
> +
> +B<REQUIRED:> Must be implemented in every storage plugin.
> +
> +Finds and returns the next available disk name, that is, the volume name
> +(C<$volname>) for a new disk image. Optionally, C<$fmt> specifies the
> +format of the disk image and C<$add_fmt_suffix> denotes whether C<$fmt>
> +should be added as suffix to the resulting name.
> +
> +=cut
> +
> sub find_free_diskname {
> my ($class, $storeid, $scfg, $vmid, $fmt, $add_fmt_suffix) = @_;
> croak "implement me in sub-class\n";
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel