Re: [pve-devel] [PATCH v1 pve-storage 7/8] pluginbase: document volume operations

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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.carrara@proxmox.com>
> > > Co-authored-by: Maximiliano Sandoval <m.sandoval@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