Re: [pve-devel] [PATCH v12 qemu-server 15/16] api: support VM disk import

[Fabian Ebner <f.ebner@proxmox.com>]

Am 14.03.22 um 16:54 schrieb Fabian Grünbichler:
> On March 9, 2022 11:09 am, Fabian Ebner wrote:

---snip---

>>  my $check_drive_param = sub {
>>      my ($param, $storecfg, $extra_checks) = @_;
>>  
>>      for my $opt (sort keys $param->%*) {
>>  	next if !PVE::QemuServer::is_valid_drivename($opt);
>>  
>> -	my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
>> +	my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
> 
> technically belongs into the previous patch, our non-alloc schema is 
> just tolerant enough because it doesn't look at the volids too closely 
> and accepts the NEW_DISK_RE syntax as potential existing volid..
> 

Makes sense. I guess I wanted to keep the other patch minimal, but
there's no good reason for that.

>>  	raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive;
>>  
>> +	if ($drive->{'import-from'}) {
>> +	    die "'import-from' requires special syntax - use <storage ID>:0,import-from=<source>\n"
>> +		if $drive->{file} !~ $NEW_DISK_RE || $3 != 0;
> 
> should probably be a param_exc
> 
>> +
>> +	    if ($opt eq 'efidisk0') {
>> +		for my $required (qw(efitype pre-enrolled-keys)) {
>> +		    die "$opt - need to specify '$required' when using 'import-from'\n"
>> +			if !defined($drive->{$required});
> 
> same here
> 
>> +		}
>> +	    } elsif ($opt eq 'tpmstate0') {
>> +		die "$opt - need to specify 'version' when using 'import-from'\n"
>> +		    if !defined($drive->{version});
> 
> and here
> 

Will change it.

>> +	    }
>> +	}
>> +
>>  	PVE::QemuServer::cleanup_drive_path($opt, $storecfg, $drive);
>>  
>>  	$extra_checks->($drive) if $extra_checks;
>>  
>> -	$param->{$opt} = PVE::QemuServer::print_drive($drive);
>> +	$param->{$opt} = PVE::QemuServer::print_drive($drive, 1);
>>      }
>>  };
>>  
>> -my $NEW_DISK_RE = qr!^(([^/:\s]+):)?(\d+(\.\d+)?)$!;
>>  my $check_storage_access = sub {
>>     my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage) = @_;
>>  
>> -   PVE::QemuConfig->foreach_volume($settings, sub {
>> +   $foreach_volume_with_alloc->($settings, sub {
>>  	my ($ds, $drive) = @_;
>>  
>>  	my $isCDROM = PVE::QemuServer::drive_is_cdrom($drive);
>> @@ -106,6 +137,20 @@ my $check_storage_access = sub {
>>  	} else {
>>  	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
>>  	}
>> +
>> +	if (my $src_image = $drive->{'import-from'}) {
>> +	    my $src_vmid;
>> +	    my ($src_storeid) = PVE::Storage::parse_volume_id($src_image, 1);
>> +	    if ($src_storeid) { # PVE-managed volume
> 
> nit, could be
> 
> if (PVE::Storage::parse_volume_id($src_image, 1)) { # PVE-managed
> 
> since we don't actually care about the sid here, and parse_volume_id 
> will return undef when $noerr is set.
> 
>> +		$src_vmid = (PVE::Storage::parse_volname($storecfg, $src_image))[2]
> 
> is there some case where we expect parse_volume_id to work, but the 
> volume to not have an associated guest? because perl doesn't mind us 
> accessing the resulting array at arbitrary indices, so this doesn't fail 
> if $src_vmid is undef..
> 

Yes, when importing from an iscsi storage (not sure if there's other
cases). The check below and $import_from_volid both handle the case
where $src_vmid is undef.

> these should probably also check some more stuff (at least the volume 
> type?) - else we get strange errors when attempting to import 
> non-image-volumes (some of which even have owners, for example backup 
> archives..), and what exactly gets caught where is basically up to the 
> storage plugin via parse_volname and volume_has_feature..

Will add a check for vtype.

> 
>> +	    }
>> +
>> +	    if ($src_vmid) { # might be actively used by VM and will be copied via clone_disk()
>> +		$rpcenv->check($authuser, "/vms/${src_vmid}", ['VM.Clone']);
>> +	    } else {
>> +		PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $src_image);
>> +	    }
>> +	}
>>      });
>>  
>>     $rpcenv->check($authuser, "/storage/$settings->{vmstatestorage}", ['Datastore.AllocateSpace'])
>> @@ -164,6 +209,87 @@ my $check_storage_access_migrate = sub {
>>  	if !$scfg->{content}->{images};
>>  };
>>  
>> +my $import_from_volid = sub {
>> +    my ($storecfg, $src_volid, $dest_info, $vollist) = @_;
>> +
>> +    die "cannot import from cloudinit disk\n"
>> +	if PVE::QemuServer::Drive::drive_is_cloudinit({ file => $src_volid });
>> +
>> +    my ($src_storeid, $src_volname) = PVE::Storage::parse_volume_id($src_volid);
> 
> technically this is already implied by the sub's name, we checked it 
> already outside, but we need the store id for the bwlimit below..
> 

Yes, it's not intended to be a check, although if it does fail something
went terribly wrong and it's good that we abort ;) I'll move it closer
to where it's actually used and I'll drop the unused $src_volname.

---snip---

>> +	    if (my $source = delete $disk->{'import-from'}) {
>> +		my $dst_volid;
>> +		my ($src_storeid) = PVE::Storage::parse_volume_id($source, 1);
>> +
>> +		if ($src_storeid) { # PVE-managed volume
> 
> same as above applies here as well, $src_storeid is not used here, so 
> can be shortened.
> 
>> +		    die "could not get size of $source\n"
>> +			if !PVE::Storage::volume_size_info($storecfg, $source, 10);
> 
> this could move into $import_from_volid?

Will do.

---snip---

>> @@ -242,7 +415,7 @@ my $create_disks = sub {
>>  	}
>>      };
>>  
>> -    eval { PVE::QemuConfig->foreach_volume($settings, $code); };
>> +    eval { $foreach_volume_with_alloc->($settings, $code); };
>>  
>>      # free allocated images on error
>>      if (my $err = $@) {
>> @@ -1285,7 +1458,7 @@ my $update_vm_api  = sub {
>>  
>>  	    my $check_drive_perms = sub {
>>  		my ($opt, $val) = @_;
>> -		my $drive = PVE::QemuServer::parse_drive($opt, $val);
>> +		my $drive = PVE::QemuServer::parse_drive($opt, $val, 1);
> 
> same applies here (move to previous patch?)
> 
>>  		# FIXME: cloudinit: CDROM or Disk?
>>  		if (PVE::QemuServer::drive_is_cdrom($drive)) { # CDROM
>>  		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM']);
>> @@ -1391,7 +1564,7 @@ my $update_vm_api  = sub {
>>  		    # default legacy boot order implies all cdroms anyway
>>  		    if (@bootorder) {
>>  			# append new CD drives to bootorder to mark them bootable
>> -			my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
>> +			my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
> 
> same
> 

---snip---

>> @@ -547,7 +566,7 @@ sub drive_is_read_only {
>>  #        [,iothread=on][,serial=serial][,model=model]
>>  
>>  sub parse_drive {
>> -    my ($key, $data) = @_;
>> +    my ($key, $data, $with_alloc) = @_;
> 
> technically previous patch, same as all the other changes in this file 
> below this change
> 

Ack.