Re: [pve-devel] [PATCH storage 3/4] update import/export storage API

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

On June 9, 2021 3:18 pm, Wolfgang Bumiller wrote:
> This bumps APIVER, but also APIAGE, see below.
> 
> The import methods (volume_import, volume_import_formats):
>   This additionally gets the '$snapshot' parameter which is
>   already present on the export side as an informational
>   piece to know which of the snapshots is the *current* one.
>   The current "disk" state will be set to this snapshot.
>   This, too, is required for our btrfs implementation.
>     `volume_import_formats` can obviously not make much
>   *use* of this parameter, but it'll still be useful to know
>   that the information is actually available in the import
>   call, so its presence will be checked in the btrfs
>   implementation.
> 
> Currently this is intended to be used for btrfs send/recv
> support, which in theory could also get additional metadata
> similar to how we do the "tar+size" format, however, we
> currently only really use this within this repository in
> storage_migrate() which has this information readily
> available anyway.
> 
> On the export side (volume_export, volume_export_formats):
>   The `$with_snapshots` option can now also be an ordered
>   array of snapshots to include, as a hint for storages
>   which need this. (As of the next commit this is only
>   btrfs, and only when also specifying a base snapshot,
>   which is a case we can currently not run into except on
>   the command line interface.)
>     The current providers of the `with_snapshot` option will
>   still treat it as a boolean (since eg. for ZFS you cannot
>   really "skip" snapshots AFAIK).
>     This is mainly intended for storages which do not have a
>   strong association between snapshots and the originals, or
>   an ordering (eg. btrfs and lvm-thin allow creating
>   arbitrary snapshot trees, and with btrfs you can even
>   create a "circular" connection between subvolumes, also we
>   could consider reflink based copies snapshots on xfs in
>   the future maybe?)
> 
> While `import_formats` and `export_formats` now take the same
> parameters, I appended the `$snapshot` parameter to the end of
> `import_formats` to the end for compatibility.

since we are currently in a window where we can do breaking changes, we 
could think about breaking for the sake of consistency. do we have any 
other cleanups planned that would cause us to reset APIAGE?

> Therefore we also bump APIAGE, as apart from the btrfs
> storage, none of our other storages need the new parameter.
> 
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---
>  PVE/CLI/pvesm.pm | 23 +++++++++++++++++++++--
>  PVE/Storage.pm   | 48 ++++++++++++++++++++++++++++++++----------------
>  2 files changed, 53 insertions(+), 18 deletions(-)
> 
> diff --git a/PVE/CLI/pvesm.pm b/PVE/CLI/pvesm.pm
> index d28f1ba..b22f759 100755
> --- a/PVE/CLI/pvesm.pm
> +++ b/PVE/CLI/pvesm.pm
> @@ -276,12 +276,23 @@ __PACKAGE__->register_method ({
>  		optional => 1,
>  		default => 0,
>  	    },
> +	    'snapshot-list' => {
> +		description => "Ordered list of snapshots to transfer",
> +		type => 'string',
> +		format => 'string-list',
> +		optional => 1,
> +	    },
>  	},
>      },
>      returns => { type => 'null' },
>      code => sub {
>  	my ($param) = @_;
>  
> +	my $with_snapshots = $param->{'with-snapshots'};
> +	if (defined(my $list = $param->{'snapshot-list'})) {
> +	    $with_snapshots = PVE::Tools::split_list($list);
> +	}
> +
>  	my $filename = $param->{filename};
>  
>  	my $outfh;
> @@ -295,7 +306,7 @@ __PACKAGE__->register_method ({
>  	eval {
>  	    my $cfg = PVE::Storage::config();
>  	    PVE::Storage::volume_export($cfg, $outfh, $param->{volume}, $param->{format},
> -		$param->{snapshot}, $param->{base}, $param->{'with-snapshots'});
> +		$param->{snapshot}, $param->{base}, $with_snapshots);
>  	};
>  	my $err = $@;
>  	if ($filename ne '-') {
> @@ -361,6 +372,13 @@ __PACKAGE__->register_method ({
>  		optional => 1,
>  		default => 0,
>  	    },
> +	    snapshot => {
> +		description => "The current-state snapshot if the stream contains snapshots",
> +		type => 'string',
> +		pattern => qr/[a-z0-9_\-]{1,40}/i,
> +		maxLength => 40,
> +		optional => 1,
> +	    },
>  	},
>      },
>      returns => { type => 'string' },
> @@ -436,7 +454,8 @@ __PACKAGE__->register_method ({
>  	my $volume = $param->{volume};
>  	my $delete = $param->{'delete-snapshot'};
>  	my $imported_volid = PVE::Storage::volume_import($cfg, $infh, $volume, $param->{format},
> -	    $param->{base}, $param->{'with-snapshots'}, $param->{'allow-rename'});
> +	    $param->{base}, $param->{'with-snapshots'}, $param->{'allow-rename'},
> +	    $param->{snapshot});
>  	PVE::Storage::volume_snapshot_delete($cfg, $imported_volid, $delete)
>  	    if defined($delete);
>  	return $imported_volid;
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index f9f8d16..15fcedf 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -41,11 +41,11 @@ use PVE::Storage::PBSPlugin;
>  use PVE::Storage::BTRFSPlugin;
>  
>  # Storage API version. Increment it on changes in storage API interface.
> -use constant APIVER => 8;
> +use constant APIVER => 9;
>  # Age is the number of versions we're backward compatible with.
>  # This is like having 'current=APIVER' and age='APIAGE' in libtool,
>  # see https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
> -use constant APIAGE => 7;
> +use constant APIAGE => 8;
>  
>  # load standard plugins
>  PVE::Storage::DirPlugin->register();
> @@ -716,7 +716,8 @@ sub storage_migrate {
>      my $send = ['pvesm', 'export', $volid, $format, '-', '-with-snapshots', $with_snapshots];
>      my $recv = [@$ssh, '--', 'pvesm', 'import', $target_volid, $format, $import_fn, '-with-snapshots', $with_snapshots];
>      if (defined($snapshot)) {
> -	push @$send, '-snapshot', $snapshot
> +	push @$send, '-snapshot', $snapshot;
> +	push @$recv, '-snapshot', $snapshot;

this one breaks migration using storage_migrate and replication from 
updated to non-updated nodes.. we already introduced a mechanism to 
handle this (`pvesm apiinfo`) the last time we introduced a new 
parameter/breaking change on the receiving side (`--allow-rename`), 
which should likely be used here to stay compatible in both directions.

>      }
>      if ($migration_snapshot) {
>  	push @$recv, '-delete-snapshot', $snapshot;
> @@ -1716,7 +1717,7 @@ sub prune_mark_backup_group {
>      }
>  }
>  
> -sub volume_export {
> +sub volume_export : prototype($$$$$$$) {
>      my ($cfg, $fh, $volid, $format, $snapshot, $base_snapshot, $with_snapshots) = @_;
>  
>      my ($storeid, $volname) = parse_volume_id($volid, 1);
> @@ -1727,18 +1728,27 @@ sub volume_export {
>                                    $snapshot, $base_snapshot, $with_snapshots);
>  }
>  
> -sub volume_import {
> -    my ($cfg, $fh, $volid, $format, $base_snapshot, $with_snapshots, $allow_rename) = @_;
> +sub volume_import : prototype($$$$$$$$) {
> +    my ($cfg, $fh, $volid, $format, $base_snapshot, $with_snapshots, $allow_rename, $snapshot) = @_;
>  
>      my ($storeid, $volname) = parse_volume_id($volid, 1);
>      die "cannot import into volume '$volid'\n" if !$storeid;
>      my $scfg = storage_config($cfg, $storeid);
>      my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> -    return $plugin->volume_import($scfg, $storeid, $fh, $volname, $format,
> -                                  $base_snapshot, $with_snapshots, $allow_rename) // $volid;
> -}
> -
> -sub volume_export_formats {
> +    return $plugin->volume_import(
> +	$scfg,
> +	$storeid,
> +	$fh,
> +	$volname,
> +	$format,
> +	$base_snapshot,
> +	$with_snapshots,
> +	$allow_rename,
> +	$snapshot,
> +    ) // $volid;
> +}
> +
> +sub volume_export_formats : prototype($$$$$) {
>      my ($cfg, $volid, $snapshot, $base_snapshot, $with_snapshots) = @_;
>  
>      my ($storeid, $volname) = parse_volume_id($volid, 1);
> @@ -1750,21 +1760,27 @@ sub volume_export_formats {
>                                            $with_snapshots);
>  }
>  
> -sub volume_import_formats {
> -    my ($cfg, $volid, $base_snapshot, $with_snapshots) = @_;
> +sub volume_import_formats : prototype($$$$$) {
> +    my ($cfg, $volid, $base_snapshot, $with_snapshots, $snapshot) = @_;
>  
>      my ($storeid, $volname) = parse_volume_id($volid, 1);
>      return if !$storeid;
>      my $scfg = storage_config($cfg, $storeid);
>      my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> -    return $plugin->volume_import_formats($scfg, $storeid, $volname,
> -                                          $base_snapshot, $with_snapshots);
> +    return $plugin->volume_import_formats(
> +	$scfg,
> +	$storeid,
> +	$volname,
> +	$base_snapshot,
> +	$with_snapshots,
> +	$snapshot,
> +    );
>  }
>  
>  sub volume_transfer_formats {
>      my ($cfg, $src_volid, $dst_volid, $snapshot, $base_snapshot, $with_snapshots) = @_;
>      my @export_formats = volume_export_formats($cfg, $src_volid, $snapshot, $base_snapshot, $with_snapshots);
> -    my @import_formats = volume_import_formats($cfg, $dst_volid, $base_snapshot, $with_snapshots);
> +    my @import_formats = volume_import_formats($cfg, $dst_volid, $base_snapshot, $with_snapshots, $snapshot);
>      my %import_hash = map { $_ => 1 } @import_formats;
>      my @common = grep { $import_hash{$_} } @export_formats;
>      return @common;
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>