Re: [pve-devel] [PATCH v1 pve-storage 4/8] pluginbase: document general plugin methods

[Maximiliano Sandoval <m.sandoval@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