From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9])
by lore.proxmox.com (Postfix) with ESMTPS id E0D471FF16E
for <inbox@lore.proxmox.com>; Mon, 14 Apr 2025 10:24:33 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 61E331D91E;
Mon, 14 Apr 2025 10:24:31 +0200 (CEST)
Date: Mon, 14 Apr 2025 10:24:25 +0200
From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: Max Carrara <m.carrara@proxmox.com>
Message-ID: <vihhk46qty57wbzhzytqyncbwtvabjdhg23h6yxtgvohuxh47o@bzs7sx6aplad>
References: <20250326142059.261938-1-m.carrara@proxmox.com>
<20250326142059.261938-8-m.carrara@proxmox.com>
<1743431702.wltqcu20kr.astroid@yuna.none>
<D8WAPCS94G4A.1WS3C5TSFLL8R@proxmox.com>
MIME-Version: 1.0
Content-Disposition: inline
In-Reply-To: <D8WAPCS94G4A.1WS3C5TSFLL8R@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.079 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH v1 pve-storage 7/8] pluginbase: document
volume operations
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.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=FCnbichler 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/PluginBa=
se.pm
> > > index 37b1471..a1bfc8d 100644
> > > --- a/src/PVE/Storage/PluginBase.pm
> > > +++ b/src/PVE/Storage/PluginBase.pm
> > > @@ -861,108 +861,483 @@ sub free_image {
> > > =
> > > =3Dcut
> > > =
> > > +=3Dhead3 $plugin->list_volumes($storeid, \%scfg, $vmid, \@content_ty=
pes)
> > > +
> > > +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 al=
l 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 =3D $plugin->list_volumes($storeid, $scfg, $vmid, ['=
backup']);
> > > +
> > > + # List all containers and virtual machines on the storage
> > > + my $guests =3D $plugin->list_volumes($storeid, $scfg, undef, ['r=
ootdir', '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 =3D> "images",
> > > + ctime =3D> "1702298038", # creation time as unix timestamp
> > > + format =3D> "raw",
> > > + size =3D> 9663676416, # in bytes!
> > > + vmid =3D> 102,
> > > + volid =3D> "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 =3D> "backup",
> > > + ctime =3D> 1742405070, # creation time as unix timestamp
> > > + format =3D> "tar.zst",
> > > + notes =3D> "...", # comment that was entered when backup was cr=
eated
> > > + size =3D> 328906840, # in bytes!
> > > + subtype =3D> "lxc", # "lxc" for containers, "qemu" for VMs
> > > + vmid =3D> 106,
> > > + volid =3D> "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.
> =
> >
> > > +
> > > +=3Dcut
> > > +
> > > sub list_volumes {
> > > my ($class, $storeid, $scfg, $vmid, $content_types) =3D @_;
> > > 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, igno=
red when pruning.
> > > +=3Dhead3 $plugin->get_volume_attribute(\%scfg, $storeid, $volname, $=
attribute)
> > > +
> > > +=3Dhead3 $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 at=
tribute isn't
> > > +supported for the volume, returns C<undef>.
> > > +
> > > +C<die>s if there is an error fetching the attribute.
> > > +
> > > +B<Possible attributes:>
> > > +
> > > +=3Dover
> > > +
> > > +=3Ditem * C<notes> (returns scalar)
> >
> > scalar *string* ?
> > > +
> > > +User-provided comments / notes.
> > > +
> > > +=3Ditem * 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.
> >
> > > +
> > > +=3Dback
> > > +
> > > +=3Dcut
> > > +
> > > sub get_volume_attribute {
> > > my ($class, $scfg, $storeid, $volname, $attribute) =3D @_;
> > > croak "implement me in sub-class\n";
> > > }
> > > =
> > > -# Dies if the attribute is not supported for the volume.
> > > +=3Dhead3 $plugin->update_volume_attribute(\%scfg, $storeid, $volname=
, $attribute, $value)
> > > +
> > > +=3Dhead3 $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(...)" >>>.
> > > +
> > > +=3Dcut
> > > +
> > > sub update_volume_attribute {
> > > my ($class, $scfg, $storeid, $volname, $attribute, $value) =3D @=
_;
> > > croak "implement me in sub-class\n";
> > > }
> > > =
> > > +=3Dhead3 $plugin->volume_size_info(\%scfg, $storeid, $volname [, $ti=
meout])
> > > +
> > > +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 =3D $plugin->volume_size_info($scfg, $storeid, $volname=
, $timeout)
> > > +
> > > +In list context, returns an array with the following structure:
> > > +
> > > + my ($size, $format, $used, $parent, $ctime) =3D $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 sho=
uld C<die>.
> > > +This timeout is best passed to other helpers which already support t=
imeouts,
> > > +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::)!
> >
> > > +
> > > +=3Dcut
> > > +
> > > sub volume_size_info {
> > > my ($class, $scfg, $storeid, $volname, $timeout) =3D @_;
> > > croak "implement me in sub-class\n";
> > > }
> > > =
> > > +=3Dhead3 $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> (=3D 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 implementati=
on 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 wheth=
er the
> > > +operation succeeded.
> >
> > so since this documents the new version.. shouldn't we just drop the
> > last part?
> >
> > > +
> > > +=3Dcut
> > > +
> > > sub volume_resize {
> > > my ($class, $scfg, $storeid, $volname, $size, $running) =3D @_;
> > > croak "implement me in sub-class\n";
> > > }
> > > =
> > > +=3Dhead3 $plugin->volume_snapshot(\%scfg, $storeid, $volname, $snap)
> > > +
> > > +B<OPTIONAL:> May be implemented if the underlying storage supports s=
napshots.
> > > +
> > > +Takes a snapshot of a volume and gives it the name provided by C<$sn=
ap>.
> >
> > 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 e=
rror
> > > +occurs while taking a snapshot.
> >
> > s/a/the
> > s/storrage/storage
> >
> > > +
> > > +=3Dcut
> > > +
> > > sub volume_snapshot {
> > > my ($class, $scfg, $storeid, $volname, $snap) =3D @_;
> > > croak "implement me in sub-class\n";
> > > }
> > > =
> > > -# Returns a hash with the snapshot names as keys and the following d=
ata:
> > > -# id - Unique id to distinguish different snapshots even if t=
he have the same name.
> > > -# timestamp - Creation time of the snapshot (seconds since epoch).
> > > -# Returns an empty hash if the volume does not exist.
> > > +=3Dhead3 $plugin->volume_snapshot_info(\%scfg, $storeid, $volname)
> > > +
> > > +Returns a hashref with the snapshot names as keys and the following =
structure:
> > > +
> > > + {
> > > + my_snapshot =3D> {
> > > + id =3D> "01b7e668-58b4-6f42-9a5e-1944c2855c84", # Uniqu=
e id to distinguish different snapshots with the same name.
> > > + timestamp =3D> "1729841807", # Creation time of the sna=
pshot (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?)
> >
> > > +
> > > +=3Dcut
> > > +
> > > sub volume_snapshot_info {
> > > my ($class, $scfg, $storeid, $volname) =3D @_;
> > > 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