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

["Max Carrara" <m.carrara@proxmox.com>]

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",
> > +	},
> > +	# [...]
> > +    ]
> > +
> > +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>).
>
> 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.
>
> or dies if retrieving the snapshot information for the given volume
> failed. ?
>
> > +
> > +=cut
> > +
> >  sub volume_snapshot_info {
> >      my ($class, $scfg, $storeid, $volname) = @_;
> >      croak "implement me in sub-class\n";
> >  }
> >  
> > -# Asserts that a rollback to $snap on $volname is possible.
> > -# If certain snapshots are preventing the rollback and $blockers is an array
> > -# reference, the snapshot names can be pushed onto $blockers prior to dying.
> > +=head3 $plugin->volume_rollback_is_possible(\%scfg, $storeid, $volname, $snap [, \@blockers])
> > +
> > +=head3 $plugin->volume_rollback_is_possible(...)
> > +
> > +B<OPTIONAL:> May be implemented if the underlying storage supports snapshots.
> > +
> > +Asserts whether a rollback to C<$snap> on C<$volname> is possible, C<die>s if
> > +it isn't.
> > +
> > +Optionally, C<\@blockers> may be provided, which may contain the names of
> > +snapshots that are preventing the rollback. Should any such snapshots exist,
> > +they should be pushed to this listref pior to C<die>-ing. The caller may then
> > +use this listref when handling errors.
>
> If the optional paramater C<\@blockers> is provided, the names of any
> snapshots blocking the rollback should be added to this listref prior to
> C<die>-ing.
>
> > +
> > +=cut
> > +
> >  sub volume_rollback_is_possible {
> >      my ($class, $scfg, $storeid, $volname, $snap, $blockers) = @_;
> >      croak "implement me in sub-class\n";
> >  }
> >  
> > +=head3 $plugin->volume_snapshot_rollback(\%scfg, $storeid, $volname, $snap)
> > +
> > +Performs a rollback to the given C<$snap>shot on C<$volname>.
> > +
> > +C<die>s in case of errors.
> > +
> > +=cut
> > +
> >  sub volume_snapshot_rollback {
> >      my ($class, $scfg, $storeid, $volname, $snap) = @_;
> >      croak "implement me in sub-class\n";
> >  }
> >  
> > +=head3 $plugin->volume_snapshot_delete(\%scfg, $storeid, $volname, $snap [, $running])
> > +
> > +Deletes the C<$snap>shot of C<$volname>.
> > +
> > +C<die>s in case of errors.
> > +
> > +Optionally, the guest that owns the given volume may be C<$running> (= C<1>).
>
> Again, this is phrased weird *and* gives no information to a potential
> implementor of this API.. why should a plugin care?
>
> > +
> > +B<Deprecated:> The C<$running> parameter is deprecated and will be removed on the
> > +next C<APIAGE> reset.
>
> and this makes it even more confusing!
>
> > +
> > +=cut
> > +
> >  sub volume_snapshot_delete {
> >      my ($class, $scfg, $storeid, $volname, $snap, $running) = @_;
> >      croak "implement me in sub-class\n";
> >  }
> >  
> > +=head3 $plugin->volume_snapshot_needs_fsfreeze()
> > +
> > +Returns whether filesystems on top of the volume need to flush their journal for
> > +consistency before a snapshot is taken. See L<fsfreeze(8)>.
> > +
> > +This is needed for container mountpoints.
>
> this doesn't really tell me what I should do here either.. (and it's
> also a really ugly interace that we seemingly introduced because RBD
> snapshots cannot be mounted RO otherwise?? does this really need to be
> part of the plugin interface? Oo)
>
> > +
> > +=cut
> > +
> >  sub volume_snapshot_needs_fsfreeze {
> >      croak "implement me in sub-class\n";
> >  }
> >  
> > +=head3 $plugin->storage_can_replicate(\%scfg, $storeid, $format)
> > +
> > +Returns whether volumes in a given C<$format> support replication.
> > +
> > +See C<L<< plugindata()|/"$plugin->plugindata()" >>> for all disk formats.
> > +
>
> should probably note that replication is limited to ZFS at the moment in
> any case?
>
> > +=cut
> > +
> >  sub storage_can_replicate {
> >      my ($class, $scfg, $storeid, $format) = @_;
> >      croak "implement me in sub-class\n";
> >  }
> >  
> > +=head3 $plugin->volume_has_feature(\%scfg, $feature, $storeid, $volname, $snapname [, $running, \%opts])
> > +
> > +=head3 $plugin->volume_has_feature(...)
> > +
> > +B<REQUIRED:> Must be implemented in every storage plugin.
> > +
> > +Checks whether a volume C<$volname> or its snapshot C<$snapname> supports the
> > +given C<$feature>, returning C<1> if it does and C<undef> otherwise. The guest
> > +owning the volume may optionally be C<$running>.
>
> that last sentence again doesn't make any sense the way it is phrased..
>
> > +
> > +C<$feature> may be one of the following:
> > +
> > +    clone      # linked clone is possible
> > +    copy       # full clone is possible
>
> actually, this is "bit-wise copying from" is possible, right?
>
> > +    replicate  # replication is possible
> > +    snapshot   # taking a snapshot is possible
> > +    sparseinit # volume is sparsely initialized
> > +    template   # conversion to base image is possible
> > +    rename     # renaming volumes is possible
> > +
> > +Which features are available under which circumstances depends on multiple
> > +factors, such as the underlying storage implementation, the format used, etc.
>
> *and whether the corresponding guest is currently running*, which is the
> whole point of that parameter ;)
>
> this really needs to explain what the values for those feature keys
> mean..

Could add a separate section for those keys as well, as with content
types. Otherwise, this docstring would probably blow up.

>
> > +It's best to check out C<L<PVE::Storage::Plugin>> or C<L<PVE::Storage::ZFSPoolPlugin>>
> > +for examples on how to handle features.
> > +
> > +Additional keys are given in C<\%opts>:
> > +
> > +=over
> > +
> > +=item * C<< valid_target_formats => [...] >>
> > +
> > +Listref of formats for the target of a copy/clone operation that the caller
> > +could work with. The format of the given volume is always considered valid and
> > +if no list is specified, all formats are considered valid.
> > +
> > +=back
> > +
> > +=cut
> > +
> >  sub volume_has_feature {
> >      my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running, $opts) = @_;
> >      croak "implement me in sub-class\n";
> >  }
> >  
> > +=head3 $plugin->map_volume($storeid, \%scfg, $volname [, $snapname])
> > +
> > +B<OPTIONAL:> May be implemented in a storage plugin.
> > +
> > +Maps the device asscoiated with a volume or a volume's snapshot to a filesystem
>
> associated
>
> > +path, returning the path on completion. This method is used by containers.
>
> returning the path. (obviously on completion, how else?)
>
> it's not only used by containers, so maybe drop that part and instead
> note that it only needs to be implemented if `path` doesn't (always)
> return such a path anyway.
>
> > +
> > +C<die>s in case of errors.
> > +
> > +C<L<< unmap_volume()|/"$plugin->unmap_volume(...)" >>> can be used to declare
> > +how the device should be unmapped.
>
> maybe just say that if you implement map, you should also implement
> unmap?
>
> > +
> > +=cut
> > +
> >  sub map_volume {
> >      my ($class, $storeid, $scfg, $volname, $snapname) = @_;
> >      croak "implement me in sub-class\n";
> >  }
> >  
> > +=head3 $plugin->unmap_volume($storeid, \%scfg, $volname [, $snapname])
> > +
> > +B<OPTIONAL:> May be implemented in a storage plugin.
> > +
> > +Unmaps the device associated to a volume or a volume's snapshot.
> > +
> > +C<die>s in case of errors.
> > +
> > +=cut
> > +
> >  sub unmap_volume {
> >      my ($class, $storeid, $scfg, $volname, $snapname) = @_;
> >      croak "implement me in sub-class\n";
> >  }
> >  
> > +=head3 $plugin->activate_volume($storeid, \%scfg, $volname, $snapname [, \%cache])
> > +
> > +=head3 $plugin->activate_volume(...)
> > +
> > +B<REQUIRED:> Must be implemented in every storage plugin.
> > +
> > +Activates a volume or its associated snapshot, making it available to the
> > +system for further use. For example, this could mean activating an LVM volume,
> > +mounting a ZFS dataset, checking whether the volume's file path exists, etc.
> > +
> > +C<die>s in case of errors or if an operation is not supported.
> > +
> > +If this isn't needed, the method should simply be a no-op.
>
> If no activation is needed, 
>
> > +
> > +This method may reuse L<< cached information via C<\%cache>|/"CACHING EXPENSIVE OPERATIONS" >>.
> > +
> > +=cut
> > +
> >  sub activate_volume {
> >      my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
> >      croak "implement me in sub-class\n";
> >  }
> >  
> > +=head3 $plugin->deactivate_volume($storeid, \%scfg, $volname, $snapname [, \%cache])
> > +
> > +=head3 deactivate_volume(...)
> > +
> > +B<REQUIRED:> Must be implemented in every storage plugin.
> > +
> > +Deactivates a volume or its associated snapshot, making it unavailable to
> > +the system. For example, this could mean deactivating an LVM volume,
> > +unmapping a Ceph/RBD device, etc.
> > +
> > +If this isn't needed, the method should simply be a no-op.
>
> If deactivation is not needed/not possible,
>
> should we note here that deactivation will not happen for every
> activation, but only when we know for sure that the volume is no longer
> needed, such as before it is removed or when its owner is migrated to
> another node?

Oh, yes. Very good point!

>
> > +
> > +This method may reuse L<< cached information via C<\%cache>|/"CACHING EXPENSIVE OPERATIONS" >>.
> > +
> > +=cut
> > +
> >  sub deactivate_volume {
> >      my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
> >      croak "implement me in sub-class\n";
> >  }
> >  
> > +=head3 $plugin->rename_volume(...)
> > +
> > +=head3 $plugin->rename_volume(\%scfg, $storeid, $source_volname, $target_vmid, $target_volname)
> > +
> > +B<OPTIONAL:> May be implemented in a storage plugin.
> > +
> > +Renames the volume given by C<$source_volname> to C<$target_volname> and assigns
> > +it to the guest C<$target_vmid>. Returns the volume ID of the renamed volume.
> > +
> > +This method is needed for the I<Change Owner> feature.
> > +
> > +C<die>s if the rename failed.
> > +
> > +=cut
> > +
> >  sub rename_volume {
> >      my ($class, $scfg, $storeid, $source_volname, $target_vmid, $target_volname) = @_;
> >      croak "implement me in sub-class\n";
> >  }
> >  
> > +=head3 $plugin->prune_backups(\%scfg, $storeid [, \%keep, $vmid, $type, $dryrun, \&logfunc])
> > +
> > +=head3 $plugin->prune_backups(...)
> > +
> > +Export a volume into a file handle as a stream with a desired format.
>
> copy-paste mistake? ;)
>
> > +
> > +C<die>s if there are (grave) problems while pruning.
> > +
> > +This method may take several optional parameters:
> > +
> > +=over
> > +
> > +=item * C<< \%keep >>
> > +
> > +A hashref containing backup retention policies. It has the following structure:
> > +
> > +    {
> > +	'keep-all'     => 1 # (optional) Whether to keep all backups.
> > +	                    # Conflicts with the other options when true.
> > +	'keep-last'    => N # (optional) Keep the last N backups.
> > +	'keep-hourly'  => N # (optional) Keep backups for the last N different hours.
> > +			    # If there is more than one backup for a single
> > +			    # hour, only the latest one is kept.
> > +	'keep-daily'   => N # (optional) Keep backups for the last N different days.
> > +			    # If there is more than one backup for a single
> > +			    # day, only the latest one is kept.
> > +	'keep-weekly'  => N # (optional) Keep backups for the last N different weeks.
> > +			    # If there is more than one backup for a single
> > +			    # week, only the latest one is kept.
> > +	'keep-monthly' => N # (optional) Keep backups for the last N different months.
> > +			    # If there is more than one backup for a single
> > +			    # month, only the latest one is kept.
> > +	'keep-yearly'  => N # (optional) Keep backups for the last N different years.
> > +			    # If there is more than one backup for a single
> > +			    # year, only the latest one is kept.
> > +    }
>
> should we add a pointer to docs here? or to
> PVE::Storage::prune_mark_backup_group? and should that or its body be
> moved somewhere else?

Yeah, I'll see what I can do here!

>
> > +
> > +=item * C<< $vmid >>
> > +
> > +The guest's ID.
> > +
> > +=item * C<< $type >>
> > +
> > +The type of guest. When C<defined>, it can be one of C<"qemu"> or C<"lxc">.
> > +If C<undefined>, both types of backups will be pruned.
> > +
> > +=item * C<< $dry_run >>
> > +
> > +Whether this is a dry run. If set to C<1> there won't be any change on the
> > +underlying storage.
> > +
> > +=item * C<< \&logfunc >>
> > +
> > +A subroutine ref that can be used to log messages with the following signature:
> > +
> > +    $logfunc->($severity, $message)
> > +
> > +where $severity can be one of C<"info">, C<"err">, or C<"warn">.
> > +
> > +=back
> > +
> > +=cut
> > +
> >  sub prune_backups {
> >      my ($class, $scfg, $storeid, $keep, $vmid, $type, $dryrun, $logfunc) = @_;
> >      croak "implement me in sub-class\n";
> > -- 
> > 2.39.5
> > 
> > 
> > 
> > _______________________________________________
> > pve-devel mailing list
> > pve-devel@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> > 
> > 
> > 
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel