On Wed, Apr 02, 2025 at 06:32:14PM +0200, Max Carrara wrote:
> On Mon Mar 31, 2025 at 5:12 PM CEST, Fabian Grünbichler wrote:
> > On March 26, 2025 3:20 pm, Max Carrara wrote:
> > > Add docstrings for the following methods:
> > > - list_volumes
> > > - get_volume_attribute
> > > - update_volume_attribute
> > > - volume_size_info
> > > - volume_resize
> > > - volume_snapshot
> > > - volume_snapshot_info
> > > - volume_rollback_is_possible
> > > - volume_snapshot_rollback
> > > - volume_snapshot_delete
> > > - volume_snapshot_needs_fsfreeze
> > > - storage_can_replicate
> > > - volume_has_feature
> > > - map_volume
> > > - unmap_volume
> > > - activate_volume
> > > - deactivate_volume
> > > - rename_volume
> > > - prune_backups
> > >
> > > Signed-off-by: Max Carrara <m.carr...@proxmox.com>
> > > Co-authored-by: Maximiliano Sandoval <m.sando...@proxmox.com>
> > > ---
> > > src/PVE/Storage/PluginBase.pm | 401 ++++++++++++++++++++++++++++++++--
> > > 1 file changed, 388 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/src/PVE/Storage/PluginBase.pm b/src/PVE/Storage/PluginBase.pm
> > > index 37b1471..a1bfc8d 100644
> > > --- a/src/PVE/Storage/PluginBase.pm
> > > +++ b/src/PVE/Storage/PluginBase.pm
> > > @@ -861,108 +861,483 @@ sub free_image {
> > >
> > > =cut
> > >
> > > +=head3 $plugin->list_volumes($storeid, \%scfg, $vmid, \@content_types)
> > > +
> > > +B<REQUIRED:> Must be implemented in every storage plugin.
> > > +
> > > +Returns a list of volumes for the given guest whose content type is
> > > within the
>
> Upfront note: Unless I otherwise comment something, I agree with you.
> Just sparing myself from writing and you from reading too many ACKs :P
>
> >
> > this is wrong - what if $vmid is not set? or if content type is snippets
> > or one of other ones that don't have an associated guest/owner? or what
> > if there is no $vmid given, as in the example below?
>
> Right, thanks for spotting this too.
>
> >
> > > +given C<\@content_types>. If C<\@content_types> is empty, no volumes
> > > will be
> > > +returned. See C<L<< plugindata()|/"$plugin->plugindata()" >>> for all
> > > content types.
> >
> > here we are actually talking about content types for once (and this also
> > translates from content type "images" and "rootdir" to volume type
> > "images"! in PVE::Plugin!).
> >
> > > +
> > > + # List all backups for a guest
> > > + my $backups = $plugin->list_volumes($storeid, $scfg, $vmid,
> > > ['backup']);
> > > +
> > > + # List all containers and virtual machines on the storage
> > > + my $guests = $plugin->list_volumes($storeid, $scfg, undef,
> > > ['rootdir', 'images'])
> >
> > eh, this is a bad example? it doesn't list the guests, it lists their
> > volumes..
> >
> > > +
> > > +The returned listref of hashrefs has the following structure:
> > > +
> > > + [
> > > + {
> > > + content => "images",
> > > + ctime => "1702298038", # creation time as unix timestamp
> > > + format => "raw",
> > > + size => 9663676416, # in bytes!
> > > + vmid => 102,
> > > + volid => "local-lvm:vm-102-disk-0",
^ Just to note this: This is actually the *full id*.
We may want to at some point have an opt-in to *not* have to include the
store id here and have `PVE/Storage.pm` fix this up? It is a very weird
API this way.
> > > + },
> > > + # [...]
> > > + ]
> > > +
> > > +Backups will also contain additional keys:
> > > +
> > > + [
> > > + {
> > > + content => "backup",
> > > + ctime => 1742405070, # creation time as unix timestamp
> > > + format => "tar.zst",
> > > + notes => "...", # comment that was entered when backup was created
> > > + size => 328906840, # in bytes!
> > > + subtype => "lxc", # "lxc" for containers, "qemu" for VMs
> > > + vmid => 106,
> > > + volid => "local:backup/vzdump-lxc-106-2025_03_19-18_24_30.tar.zst",
> > > + },
> > > + # [...]
> > > + ]
> >
> > soooo.. what is the interface here again? -> needs a (complete) schema
> > for the returned type, else how am I supposed to implement this?
>
> I mean, we could define one. I didn't want to clobber the docstring with
> too many examples / too much text here, but I agree.
>
> >
> > > +
> > > +=cut
> > > +
> > > sub list_volumes {
> > > my ($class, $storeid, $scfg, $vmid, $content_types) = @_;
> > > croak "implement me in sub-class\n";
> > > }
> > >
> > > -# Returns undef if the attribute is not supported for the volume.
> > > -# Should die if there is an error fetching the attribute.
> > > -# Possible attributes:
> > > -# notes - user-provided comments/notes.
> > > -# protected - not to be removed by free_image, and for backups, ignored
> > > when pruning.
> > > +=head3 $plugin->get_volume_attribute(\%scfg, $storeid, $volname,
> > > $attribute)
> > > +
> > > +=head3 $plugin->get_volume_attribute(...)
> > > +
> > > +B<REQUIRED:> Must be implemented in every storage plugin.
> > > +
> > > +Returns the value of the given C<$attribute> for a volume. If the
> > > attribute isn't
> > > +supported for the volume, returns C<undef>.
> > > +
> > > +C<die>s if there is an error fetching the attribute.
> > > +
> > > +B<Possible attributes:>
> > > +
> > > +=over
> > > +
> > > +=item * C<notes> (returns scalar)
> >
> > scalar *string* ?
> > > +
> > > +User-provided comments / notes.
> > > +
> > > +=item * C<protected> (returns scalar)
> >
> > scalar *boolean* ?
> >
> > > +
> > > +When set to C<1>, the volume must not be removed by C<L<<
> > > free_image()|/"$plugin->free_image(...)" >>>.
> > > +Backups with C<protected> set to C<1> are ignored when pruning.
> >
> > Backup volumes .. when calculating which backup volumes to prune.
> >
> > > +
> > > +=back
> > > +
> > > +=cut
> > > +
> > > sub get_volume_attribute {
> > > my ($class, $scfg, $storeid, $volname, $attribute) = @_;
> > > croak "implement me in sub-class\n";
> > > }
> > >
> > > -# Dies if the attribute is not supported for the volume.
> > > +=head3 $plugin->update_volume_attribute(\%scfg, $storeid, $volname,
> > > $attribute, $value)
> > > +
> > > +=head3 $plugin->update_volume_attribute(...)
> > > +
> > > +B<REQUIRED:> Must be implemented in every storage plugin.
> > > +
> > > +Sets the value of the given C<$attribute> for a volume to C<$value>.
> > > +
> > > +C<die>s if the attribute isn't supported for the volume (or storage).
> > > +
> > > +For a list of supported attributes, see C<L<<
> > > get_volume_attribute()|/"$plugin->get_volume_attribute(...)" >>>.
> > > +
> > > +=cut
> > > +
> > > sub update_volume_attribute {
> > > my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
> > > croak "implement me in sub-class\n";
> > > }
> > >
> > > +=head3 $plugin->volume_size_info(\%scfg, $storeid, $volname [, $timeout])
> > > +
> > > +B<REQUIRED:> Must be implemented in every storage plugin.
> > > +
> > > +Returns information about the given volume's size. In scalar context,
> > > this returns
> > > +just the volume's size in bytes:
> > > +
> > > + my $size = $plugin->volume_size_info($scfg, $storeid, $volname,
> > > $timeout)
> > > +
> > > +In list context, returns an array with the following structure:
> > > +
> > > + my ($size, $format, $used, $parent, $ctime) =
> > > $plugin->volume_size_info(
> > > + $scfg, $storeid, $volname, $timeout
> > > + )
> > > +
> > > +where C<$size> is the size of the volume in bytes, C<$format> one of the
> > > possible
> > > +L<< formats listed in C<plugindata()>|/"$plugin->plugindata()" >>,
> > > C<$used> the
> > > +amount of space used in bytes, C<$parent> the (optional) name of the
> > > base volume
> > > +and C<$ctime> the creation time as unix timestamp.
> > > +
> > > +Optionally, a C<$timeout> may be provided after which the method should
> > > C<die>.
> > > +This timeout is best passed to other helpers which already support
> > > timeouts,
> > > +such as C<L<< PVE::Tools::run_command|PVE::Tools/run_command >>>.
> > > +
> > > +See also the C<L<<
> > > PVE::Storage::Plugin::file_size_info|PVE::Storage::Plugin/file_size_info
> > > >>> helper.
> >
> > PVE::Storage::file_size_info (without Plugin::)!
> >
> > > +
> > > +=cut
> > > +
> > > sub volume_size_info {
> > > my ($class, $scfg, $storeid, $volname, $timeout) = @_;
> > > croak "implement me in sub-class\n";
> > > }
> > >
> > > +=head3 $plugin->volume_resize(\%scfg, $storeid, $volname, $size [,
> > > $running])
> > > +
> > > +B<REQUIRED:> Must be implemented in every storage plugin.
> > > +
> > > +Resizes a volume to the new C<$size> in bytes. Optionally, the guest
> > > that owns
> > > +the volume may be C<$running> (= C<1>).
^ We should probably emphasize that contrary to `vdisk_alloc`, this is
in fact in *bytes* this time around.
> >
> > the second sentence doesn't make any sense.
> >
> > C<$running> indicates the guest is currently running.
> >
> > but what does that mean/why should a plugin care?
> >
> > > +
> > > +C<die>s in case of errors, or if the underlying storage implementation
> > > or the
> > > +volume's format doesn't support resizing.
> > > +
> > > +This function should not return any value. In previous versions the
> > > returned
> > > +value would be used to determine the new size of the volume or whether
> > > the
> > > +operation succeeded.
> >
> > so since this documents the new version.. shouldn't we just drop the
> > last part?
> >
> > > +
> > > +=cut
> > > +
> > > sub volume_resize {
> > > my ($class, $scfg, $storeid, $volname, $size, $running) = @_;
> > > croak "implement me in sub-class\n";
> > > }
> > >
> > > +=head3 $plugin->volume_snapshot(\%scfg, $storeid, $volname, $snap)
> > > +
> > > +B<OPTIONAL:> May be implemented if the underlying storage supports
> > > snapshots.
> > > +
> > > +Takes a snapshot of a volume and gives it the name provided by C<$snap>.
> >
> > this sounds like this is a two-step process..
> >
> > Takes a snapshot called C<$snap> of the volume C<$volname>.
> >
> > > +
> > > +C<die>s if the underlying storrage doesn't support snapshots or an error
> > > +occurs while taking a snapshot.
> >
> > s/a/the
> > s/storrage/storage
> >
> > > +
> > > +=cut
> > > +
> > > sub volume_snapshot {
> > > my ($class, $scfg, $storeid, $volname, $snap) = @_;
> > > croak "implement me in sub-class\n";
> > > }
> > >
> > > -# Returns a hash with the snapshot names as keys and the following data:
> > > -# id - Unique id to distinguish different snapshots even if the
> > > have the same name.
> > > -# timestamp - Creation time of the snapshot (seconds since epoch).
> > > -# Returns an empty hash if the volume does not exist.
> > > +=head3 $plugin->volume_snapshot_info(\%scfg, $storeid, $volname)
> > > +
> > > +Returns a hashref with the snapshot names as keys and the following
> > > structure:
> > > +
> > > + {
> > > + my_snapshot => {
> > > + id => "01b7e668-58b4-6f42-9a5e-1944c2855c84", # Unique id
> > > to distinguish different snapshots with the same name.
> > > + timestamp => "1729841807", # Creation time of the snapshot
> > > (seconds since epoch).
> > > + },
> > > + # [...]
> > > + }
> > > +
> > > +Returns an empty hashref if the volume does not exist.
This *would* be a weird API because currently this is only implemented
for ZFS and dies everywhere else.
> >
> > or dies if retrieving the snapshot information for the given volume
> > failed. ?
(We could however say that storages should return `undef` for no-error
and also not implemented?)
> >
> > > +
> > > +=cut
> > > +
> > > sub volume_snapshot_info {
> > > my ($class, $scfg, $storeid, $volname) = @_;
> > > 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
