Re: [pve-devel] [PATCH qemu-server 07/10] mtunnel: add API endpoints

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

On November 10, 2021 8:40 am, Fabian Ebner wrote:
> Am 09.11.21 um 13:46 schrieb Fabian Ebner:
>> Am 05.11.21 um 14:03 schrieb Fabian Grünbichler:
> 
> ---snip---
> 
>>>   use IO::Socket::IP;
>>> +use IO::Socket::UNIX;
>>> +use IPC::Open3;
>>> +use JSON;
>>> +use MIME::Base64;
> 
> Forgot to ask: is this import needed or a left-over from development?

yes

> 
> ---snip---
> 
>> 
>>> +
>>> +            my $migration_snapshot;
>>> +            if ($scfg->{type} eq 'zfspool' || $scfg->{type} eq 
>>> 'btrfs') {
>>> +            $migration_snapshot = '__migration__';
>>> +            }
>>> +
>>> +            my $volid = "$storeid:$volname";
>>> +
>>> +            # find common import/export format, taken from PVE::Storage
>>> +            my @import_formats = 
>>> PVE::Storage::volume_import_formats($state->{storecfg}, $volid, 
>>> $migration_snapshot, undef, $with_snapshots);
>>> +            my @export_formats = 
>>> PVE::Tools::split_list($params->{'export-formats'});
>>> +            my %import_hash = map { $_ => 1 } @import_formats;
>>> +            my @common = grep { $import_hash{$_} } @export_formats;
>>> +            die "no matching import/export format found for storage 
>>> '$storeid'\n"
>>> +            if !@common;
>>> +            $format = $common[0];
>>> +
>>> +            my $input = IO::File->new();
>>> +            my $info = IO::File->new();
>>> +            my $unix = "/run/qemu-server/$vmid.storage";
>>> +
>>> +            my $import_cmd = ['pvesm', 'import', $volid, $format, 
>>> "unix://$unix", '-with-snapshots', $with_snapshots];
>>> +            if ($params->{'allow-rename'}) {
>>> +            push @$import_cmd, '-allow-rename', 
>>> $params->{'allow-rename'};
>>> +            }
>>> +            if ($migration_snapshot) {
>>> +            push @$import_cmd, '-delete-snapshot', $migration_snapshot;
>> 
>> Missing '-snapshot $migration_snapshot'? While the parameter is ignored 
>> by our ZFSPoolPlugin, the BTRFSPlugin aborts if it's not specified 
>> AFAICS. And external plugins might require it too.
> 
> That is, for the 'btrfs' format. In the patch with the export command, a 
> snapshot is only used for ZFS, so it would already fail on export for 
> BTRFS with 'btrfs' format. For external plugins we also don't use a 
> migration snapshot in storage_migrate(), so please disregard that part.

done

> 
>> 
>> In general, we'll need to be careful not to introduce mismatches between 
>> the import and the export parameters. Might it be better if the client 
>> would pass along (most of) the parameters for the import command (which 
>> basically is how it's done for the existing storage_migrate)?
>> 
> 
> On the other hand, that would require being very careful with input 
> validation.

yeah, and since we are crossing a trust boundary here (between two 
clusters) we have to be careful. if we change the export/import code, we 
can always also bump the tunnel API if needed (either to selectively use 
new features only if supported, or to error out early if there was a 
breaking change). just passing in "pass this to `pvesm import`" is 
potentially dangerous if we don't carefully validate the 'this', and 
that is easier if it's structured data ;) so I'd rather do this explicit 
even if it means extending two places when we change the interface.

> 
> ---snip---
>