From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: Max Carrara <m.carrara@proxmox.com>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH v1 pve-storage 7/8] pluginbase: document volume operations
Date: Mon, 14 Apr 2025 10:24:25 +0200 [thread overview]
Message-ID: <vihhk46qty57wbzhzytqyncbwtvabjdhg23h6yxtgvohuxh47o@bzs7sx6aplad> (raw)
In-Reply-To: <D8WAPCS94G4A.1WS3C5TSFLL8R@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
next prev parent reply other threads:[~2025-04-14 8:24 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-03-26 14:20 [pve-devel] [PATCH v1 pve-storage 0/8] Base Module + Documentation for PVE::Storage::Plugin API Max Carrara
2025-03-26 14:20 ` [pve-devel] [PATCH v1 pve-storage 1/8] pluginbase: introduce PVE::Storage::PluginBase with doc scaffold Max Carrara
2025-03-31 15:13 ` Fabian Grünbichler
2025-04-02 16:31 ` Max Carrara
2025-04-03 7:12 ` Fabian Grünbichler
2025-04-03 14:05 ` Max Carrara
2025-03-26 14:20 ` [pve-devel] [PATCH v1 pve-storage 2/8] pluginbase: add high-level plugin API description Max Carrara
2025-03-31 15:13 ` Fabian Grünbichler
2025-04-02 16:31 ` Max Carrara
2025-04-03 7:12 ` Fabian Grünbichler
2025-04-03 14:05 ` Max Carrara
2025-03-26 14:20 ` [pve-devel] [PATCH v1 pve-storage 3/8] pluginbase: document SectionConfig methods Max Carrara
2025-03-31 15:13 ` Fabian Grünbichler
2025-04-02 16:31 ` Max Carrara
2025-04-11 13:49 ` Wolfgang Bumiller
2025-03-26 14:20 ` [pve-devel] [PATCH v1 pve-storage 4/8] pluginbase: document general plugin methods Max Carrara
2025-03-28 12:50 ` Maximiliano Sandoval
2025-03-31 15:12 ` Fabian Grünbichler
2025-04-02 16:31 ` Max Carrara
2025-04-11 14:04 ` Wolfgang Bumiller
2025-03-26 14:20 ` [pve-devel] [PATCH v1 pve-storage 5/8] pluginbase: document hooks Max Carrara
2025-03-28 13:07 ` Maximiliano Sandoval
2025-03-31 15:12 ` Fabian Grünbichler
2025-03-26 14:20 ` [pve-devel] [PATCH v1 pve-storage 6/8] pluginbase: document image operation methods Max Carrara
2025-03-31 15:12 ` Fabian Grünbichler
2025-04-02 16:32 ` Max Carrara
2025-04-03 7:23 ` Fabian Grünbichler
2025-04-03 14:05 ` Max Carrara
2025-04-11 14:08 ` Wolfgang Bumiller
2025-03-26 14:20 ` [pve-devel] [PATCH v1 pve-storage 7/8] pluginbase: document volume operations Max Carrara
2025-03-31 15:12 ` Fabian Grünbichler
2025-04-02 16:32 ` Max Carrara
2025-04-14 8:24 ` Wolfgang Bumiller [this message]
2025-03-26 14:20 ` [pve-devel] [PATCH v1 pve-storage 8/8] pluginbase: document import and export methods Max Carrara
2025-04-01 8:40 ` Fabian Grünbichler
2025-04-01 9:40 ` Fiona Ebner
2025-04-14 14:35 ` Wolfgang Bumiller
2025-04-02 16:32 ` Max Carrara
2025-04-11 13:27 ` [pve-devel] [PATCH v1 pve-storage 0/8] Base Module + Documentation for PVE::Storage::Plugin API Wolfgang Bumiller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=vihhk46qty57wbzhzytqyncbwtvabjdhg23h6yxtgvohuxh47o@bzs7sx6aplad \
--to=w.bumiller@proxmox.com \
--cc=m.carrara@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal