all lists on lists.proxmox.com
 help / color / mirror / Atom feed
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


  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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal