Re: [pve-devel] [PATCH v1 pve-storage 8/8] pluginbase: document import and export methods

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

On Tue Apr 1, 2025 at 10:40 AM CEST, Fabian Grünbichler wrote:
> CCing Fiona in case of further input w.r.t. export/import things
>
> On March 26, 2025 3:20 pm, Max Carrara wrote:
> > Adapt the previous description, slightly rewording it and formatting
> > it for POD under the IMPORTS AND EXPORTS section.
> > 
> > Also add docstrings for the following methods:
> > - volume_export
> > - volume_export_formats
> > - volume_import
> > - volume_import_formats
> > 
> > Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> > Co-authored-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> > ---
> >  src/PVE/Storage/PluginBase.pm | 134 ++++++++++++++++++++++++++--------
> >  1 file changed, 105 insertions(+), 29 deletions(-)
> > 
> > diff --git a/src/PVE/Storage/PluginBase.pm b/src/PVE/Storage/PluginBase.pm
> > index a1bfc8d..cfa5087 100644
> > --- a/src/PVE/Storage/PluginBase.pm
> > +++ b/src/PVE/Storage/PluginBase.pm
> > @@ -1345,55 +1345,131 @@ sub prune_backups {
> >  
> >  =head2 IMPORTS AND EXPORTS
> >  
> > +Any path-based storage is assumed to support C<raw> and C<tar> streams, so
> > +the default implementations in C<L<PVE::Storage::Plugin>> will return this if
> > +C<< $scfg->{path} >> is set (thereby mimicking the old C<< PVE::Storage::storage_migrate() >>
> > +function).

Replying up here to address all of the comments below: Yeah, we'll
rework this entire section here, I think, and expand upon everything
more. Seems like we missed a lot of details. :s

>
> meh, I don't think this makes sense if we want to document the
> interface, we should document the interface, and not the implementation
> of our plugin hierarchy..
>
> > +
> > +Plugins may fall back to methods like C<volume_export>, C<volume_import>, etc.
> > +of C<L<PVE::Storage::Plugin>> in case the format doesn't match their
> > +specialized implementations to reuse the C<raw>/C<tar> code.
>
> and these should move to some helper module and be used by
> PVE::Storage::Plugin, if we want to allow external plugins to re-use
> them as basic implementation..
>
> > +
> > +=head3 FORMATS
> > +
> > +The following formats are all prefixed with image information in the form of a
> > +64 bit little endian unsigned integer (C<< pack('Q\<') >>) in order to be able
> > +to preallocate the image on storages which require it.
>
> "image information" should maybe be a bit more precise, it's easy to
> guess from the name that information==size, but why not spell it out?
>
> > +
> > +=over
> > +
> > +=item * C<< raw+size >> (image files only)
> > +
> > +A raw binary data stream as produced via C<< dd if=$IMAGE_FILE >>.
>
> A binary data stream containing a volume's logical raw data, for example
> as produced via .. if the image is already in raw format, *or via
> qemu-img convert* if not.
>
> > +
> > +=item * C<< qcow2+size >>, C<< vmdk >> (image files only)
>
> missing +size for vmdk
>
> > +
> > +A raw C<qcow2>/C<vmdk>/... file as produced via C<< dd if=some_file.qcow2 >>
> > +for files which are already in C<qcow2> format, or via C<qemu-img convert>.
>
> "A raw qcow2/vmdk/.. file" is confusing..
>
> A binary data stream containing the qcow2/vmdk-formatted contents of a
> qcow2/vmdk file as produced via ..
>
> the qemu-img convert part got moved to the wrong format, it's not needed
> to produce a qcow2+size stream for raw files (we don't do that), but to
> produce a raw+size stream from a qcow2 file, see above.
>
> > +
> > +B<NOTE:> These formats are only valid with C<$with_snapshots> being true (C<1>).
>
> that's not strictly speaking true, but an implementation detail of the
> current implementation. what is true is that raw+size can not contain
> snapshots for obvious reasons.
>
> > +
> > +=item * C<< tar+size >> (subvolumes only)
> > +
> > +A GNU C<tar> stream containing just the inner contents of the subvolume. This
> > +does not distinguish between the contents of a privileged or unprivileged
> > +container. In other words, this is from the root user namespace's point of view
> > +with no uid-mapping in effect. As produced via e.g.
> > +C<< tar -C vm-100-disk-1.subvol -cpf output_file.dat . >>
>
> what is "inner"? should/must the content be relative or absolute?
> anchored? ...
>
> > +
> > +=back
> > +
> >  =cut
> >  
> > -# Import/Export interface:
> > -#   Any path based storage is assumed to support 'raw' and 'tar' streams, so
> > -#   the default implementations will return this if $scfg->{path} is set,
> > -#   mimicking the old PVE::Storage::storage_migrate() function.
> > -#
> > -# Plugins may fall back to PVE::Storage::Plugin::volume_{export,import}...
> > -#   functions in case the format doesn't match their specialized
> > -#   implementations to reuse the raw/tar code.
> > -#
> > -# Format specification:
> > -#   The following formats are all prefixed with image information in the form
> > -#   of a 64 bit little endian unsigned integer (pack('Q<')) in order to be able
> > -#   to preallocate the image on storages which require it.
> > -#
> > -#   raw+size: (image files only)
> > -#     A raw binary data stream such as produced via `dd if=TheImageFile`.
> > -#   qcow2+size, vmdk: (image files only)
> > -#     A raw qcow2/vmdk/... file such as produced via `dd if=some.qcow2` for
> > -#     files which are already in qcow2 format, or via `qemu-img convert`.
> > -#     Note that these formats are only valid with $with_snapshots being true.
> > -#   tar+size: (subvolumes only)
> > -#     A GNU tar stream containing just the inner contents of the subvolume.
> > -#     This does not distinguish between the contents of a privileged or
> > -#     unprivileged container. In other words, this is from the root user
> > -#     namespace's point of view with no uid-mapping in effect.
> > -#     As produced via `tar -C vm-100-disk-1.subvol -cpf TheOutputFile.dat .`
> > +=head3 $plugin->volume_export(\%scfg, $storeid, $fh, $volname, $format [, $snapshot, $base_snapshot, $with_snapshots])
> > +
> > +=head3 $plugin->volume_export(...)
> > +
> > +Exports a volume or a volume's C<$snapshot> into a file handle C<$fh> as a
> > +stream with a desired export C<$format>. See L<FORMATS> for all import/export
> > +formats.
> > +
> > +Optionally, C<$snapshot> (if provided) may have a C<$base_snapshot>, and
> > +C<$with_snapshots> states whether the volume has snapshots overall.
>
> this is incomplete/wrong
>
> $with_snapshots means the export should include snapshots, not whether
> the volume has snapshots..
> $snapshot means "this is the snapshot to export" if only exporting the
> snapshot ($with_snapshots == 0), or "this is the *last* snapshot export"
> if exporting a stream of snapshots ($with_snapshots == 1)
> $base_snapshot means "this is the start of the snapshot range to export"
> (i.e., do an incremental export on top of this base)
>
> this is mostly only relevant for zfs at the moment (other storages can
> either export a volume including its snapshots, or just the volume, but
> no complicated incremental streams of snapshots), but will change once
> we implement replication export/import for other storages..
>
> > +
> > +C<die>s in order to abort the export if there are (grave) problems, if the
> > +given C<$format> is not supported, or if exporting volumes is not supported as
> > +a whole.
> > +
> > +=cut
> >  
> > -# Export a volume into a file handle as a stream of desired format.
> >  sub volume_export {
> >      my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot, $base_snapshot, $with_snapshots) = @_;
> >      croak "implement me in sub-class\n";
> >  }
> >  
> > +=head3 $plugin->volume_export_formats(\%scfg, $storeid, $volname [, $snapshot, $base_snapshot, $with_snapshot])
> > +
> > +=head3 $plugin->volume_export_formats(...)
> > +
> > +B<OPTIONAL:> May be implemented in a storage plugin.
> > +
> > +Returns a list of supported export formats for the given volume or snapshot.
> > +
> > +Optionally, C<$snapshot> (if provided) may have a C<$base_snapshot>, and
> > +C<$with_snapshots> states whether the volume has snapshots overall.
>
> see above.. these parameters are used to affect the returned list of
> formats
>
> > +
> > +If the storage does not support exporting volumes at all, and empty list should
> > +be returned.
> > +
> > +=cut
> > +
> >  sub volume_export_formats {
> >      my ($class, $scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots) = @_;
> >      croak "implement me in sub-class\n";
> >  }
> >  
> > -# Import data from a stream, creating a new or replacing or adding to an existing volume.
> > +=head3 $plugin->volume_import(\%scfg, $storeid, $fh, $volname, $format [, $snapshot, $base_snapshot, $with_snapshots, $allow_rename])
> > +
> > +=head3 $plugin->volume_import(...)
> > +
> > +B<OPTIONAL:> May be implemented in a storage plugin.
> > +
> > +Imports data with the given C<$format> from a stream / filehandle C<$fh>,
> > +creating a new volume, or replacing or adding to an existing one. Returns the
> > +volume ID of the imported volume.
>
> I don't think we ever replace an existing volume? what we might do is
> import new snapshots on top of base_snapshot in case of an incremental
> import..
>
> maybe
>
> creating a new volume or importing additional snapshots on top of an
> existing one.
>
> > +
> > +Optionally, C<$snapshot> (if provided) may have a C<$base_snapshot>, and
> > +C<$with_snapshots> states whether the volume has snapshots overall. Renaming an
> > +existing volume may also optionally be allowed via C<$allow_rename>
>
> see above, but here $snapshot is mainly there to have the same
> arguments for volume_import_formats so a plugin can have a single
> implementation, not because it is used anywhere IIRC..
>
> > +
> > +C<die>s if the import fails, if the given C<$format> is not supported, or if
> > +importing volumes is not supported as a whole.
> > +
> > +=cut
> > +
> >  sub volume_import {
> >      my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot, $base_snapshot, $with_snapshots, $allow_rename) = @_;
> >      croak "implement me in sub-class\n";
> >  }
> >  
> > +=head3 $plugin->volume_import_formats(\%scfg, $storeid, $volname [, $snapshot, $base_snapshot, $with_snapshot])
> > +
> > +=head3 $plugin->volume_import_formats(...)
> > +
> > +B<OPTIONAL:> May be implemented in a storage plugin.
> > +
> > +Returns a list of supported import formats for the given volume or snapshot.
> > +
> > +Optionally, C<$snapshot> (if provided) may have a C<$base_snapshot>, and
> > +C<$with_snapshots> states whether the volume has snapshots overall.
>
> see above, these parameters serve the same purpose as for export_formats
> - to affect the return value / inform the plugin about the intended
> export/import
>
> it might make sense to note somewhere that the order of returned formats
> matters for both, since the first element of the intersection of both
> return values will be used to do a storage migration?
>
> > +
> > +If the storage does not support importing volumes at all, and empty list should
> > +be returned.
> > +
> > +=cut
> > +
> >  sub volume_import_formats {
> >      my ($class, $scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots) = @_;
> >      croak "implement me in sub-class\n";
> >  }
> > -
> >  1;
>
>
> _______________________________________________
> 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