[pve-devel] [PATCH storage] volume import: assume target API version is at least 9

[Max Carrara]

On Mon Jun 10, 2024 at 11:04 AM CEST, Fiona Ebner wrote:
> The storage API version has been bumped to at least 9 since
> libpve-storage = 7.0-4. If the source node is on Proxmox VE 8, where
> this change will come in, then the target node can be assumed to be
> running either Proxmox VE 8 or, during upgrade, the latest version of
> Proxmox VE 7.4, so it's safe to assume a storage API version of at
> least 9 in all cases.
>
> As reported by Maximiliano, the fact that the 'apiinfo' call was
> guarded with a quiet eval could lead to strange errors for replication
> on a customer system where an SSH connection could not always be
> established, because the target's API version would fall back to 1.
> Because of that, the '-base' argument would be missing for the import
> call on the target which would in turn lead to an error about the
> target ZFS volume already existing (rather than doing an incremental
> sync).
>
> Reported-by: Maximiliano Sandoval <m.sandoval at proxmox.com>
> Signed-off-by: Fiona Ebner <f.ebner at proxmox.com>

Wasn't able to test this at the moment yet (please holler at me if you
need me to), but these changes are pretty straightforward anyway.

I completely agree with all of these changes; LGTM.

Reviewed-by: Max Carrara <m.carrara at proxmox.com>

> ---
>  src/PVE/Storage.pm | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index f19a115..bee2361 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -709,7 +709,7 @@ sub storage_migrate_snapshot {
>  }
>  
>  my $volume_import_prepare = sub {
> -    my ($volid, $format, $path, $apiver, $opts) = @_;
> +    my ($volid, $format, $path, $opts) = @_;
>  
>      my $base_snapshot = $opts->{base_snapshot};
>      my $snapshot = $opts->{snapshot};
> @@ -724,11 +724,11 @@ my $volume_import_prepare = sub {
>      if ($migration_snapshot) {
>  	push @$recv, '-delete-snapshot', $snapshot;
>      }
> -    push @$recv, '-allow-rename', $allow_rename if $apiver >= 5;
> +    push @$recv, '-allow-rename', $allow_rename;
>  
>      if (defined($base_snapshot)) {
>  	# Check if the snapshot exists on the remote side:
> -	push @$recv, '-base', $base_snapshot if $apiver >= 9;
> +	push @$recv, '-base', $base_snapshot;
>      }
>  
>      return $recv;
> @@ -814,12 +814,7 @@ sub storage_migrate {
>  	$import_fn = "tcp://$net";
>      }
>  
> -    my $target_apiver = 1; # if there is no apiinfo call, assume 1
> -    my $get_api_version = [@$ssh, 'pvesm', 'apiinfo'];
> -    my $match_api_version = sub { $target_apiver = $1 if $_[0] =~ m!^APIVER (\d+)$!; };
> -    eval { run_command($get_api_version, logfunc => $match_api_version); };
> -
> -    my $recv = [ @$ssh, '--', $volume_import_prepare->($target_volid, $format, $import_fn, $target_apiver, $opts)->@* ];
> +    my $recv = [ @$ssh, '--', $volume_import_prepare->($target_volid, $format, $import_fn, $opts)->@* ];
>  
>      my $new_volid;
>      my $pattern = volume_imported_message(undef, 1);
> @@ -911,8 +906,7 @@ sub storage_migrate {
>  	    run_command($cmds, logfunc => $match_volid_and_log);
>  	}
>  
> -	die "unable to get ID of the migrated volume\n"
> -	    if !defined($new_volid) && $target_apiver >= 5;
> +	die "unable to get ID of the migrated volume\n" if !defined($new_volid);
>      };
>      my $err = $@;
>      if ($opts->{migration_snapshot}) {
> @@ -1961,7 +1955,7 @@ sub volume_import_start {
>      my $info = IO::File->new();
>  
>      my $unix = $opts->{unix} // "/run/pve/storage-migrate-$vmid.$$.unix";
> -    my $import = $volume_import_prepare->($volid, $format, "unix://$unix", APIVER, $opts);
> +    my $import = $volume_import_prepare->($volid, $format, "unix://$unix", $opts);
>  
>      unlink $unix;
>      my $cpid = open3($input, $info, $info, @$import)