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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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).

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