Re: [pve-devel] [PATCH qemu-server v4 3/4] api: create: implement extracting disks when needed for import-from

[Dominik Csapak <d.csapak@proxmox.com>]

On 6/12/24 18:01, Max Carrara wrote:
> On Fri May 24, 2024 at 3:21 PM CEST, Dominik Csapak wrote:
>> when 'import-from' contains a disk image that needs extraction
>> (currently only from an 'ova' archive), do that in 'create_disks'
>> and overwrite the '$source' volid.
>>
>> Collect the names into a 'delete_sources' list, that we use later
>> to clean it up again (either when we're finished with importing or in an
>> error case).
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   PVE/API2/Qemu.pm          | 55 +++++++++++++++++++++++++++++++--------
>>   PVE/QemuServer.pm         | 12 +++++++++
>>   PVE/QemuServer/Helpers.pm |  5 ++++
>>   3 files changed, 61 insertions(+), 11 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index 2a1d4d79..8c335ac4 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -24,6 +24,7 @@ use PVE::JSONSchema qw(get_standard_option);
>>   use PVE::RESTHandler;
>>   use PVE::ReplicationConfig;
>>   use PVE::GuestHelpers qw(assert_tag_permissions);
>> +use PVE::GuestImport;
>>   use PVE::QemuConfig;
>>   use PVE::QemuServer;
>>   use PVE::QemuServer::Cloudinit;
>> @@ -159,10 +160,19 @@ my $check_storage_access = sub {
>>   
>>   	if (my $src_image = $drive->{'import-from'}) {
>>   	    my $src_vmid;
>> -	    if (PVE::Storage::parse_volume_id($src_image, 1)) { # PVE-managed volume
>> -		(my $vtype, undef, $src_vmid) = PVE::Storage::parse_volname($storecfg, $src_image);
>> -		raise_param_exc({ $ds => "$src_image has wrong type '$vtype' - not an image" })
>> -		    if $vtype ne 'images';
>> +	    if (my ($storeid, $volname) = PVE::Storage::parse_volume_id($src_image, 1)) { # PVE-managed volume
>> +		my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
>> +		my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>> +		(my $vtype, undef, $src_vmid, undef, undef, undef, my $fmt) = $plugin->parse_volname($volname);
>> +
>> +		raise_param_exc({ $ds => "$src_image has wrong type '$vtype' - needs to be 'images' or 'import'" })
>> +		    if $vtype ne 'images' && $vtype ne 'import';
> 
> ... Shouldn't this here be `||` instead of `&&`? According to the error
> message at least.

no?

we raise the  error if it is not 'images' and not 'import', so it's neither
making it || would always fail then because images != import and vice versa ;)

(note the operator is 'ne' not 'eq' )

we could make it more like the error message but then it would have to be:

if !($vtype eq 'images' || $vtype eq 'import')

which should be the same what i posted just with more syntax ;)

> 
>> +
>> +		if (PVE::QemuServer::Helpers::needs_extraction($vtype, $fmt)) {
>> +		    raise_param_exc({ $ds => "$src_image is not on an storage with 'images' content type."})
> 
> s/an storage/a storage
> 
> Also, as I mentioned in another comment, is it maybe possible to display
> the same name that's used in the UI instead of 'images' or 'import'?

we generally stick to the backend terminology in error messages from the backend AFAIK
but i'd be willing to change that if we have precendence somewhere for that

these error message also are for the cli though, and there we use also the backend terminology
for the configs

also we generally try to catch such cases beforehand in the ui (if possible) so the
user does not see the backend error message at all most of the time

> 
>> +			if !$scfg->{content}->{images};
>> +		    $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
>> +		}
>>   	    }
>>   
>>   	    if ($src_vmid) { # might be actively used by VM and will be copied via clone_disk()
>> @@ -392,16 +402,34 @@ my sub create_disks : prototype($$$$$$$$$$) {
>>   		$needs_creation = $live_import;
>>   
>>   		if (PVE::Storage::parse_volume_id($source, 1)) { # PVE-managed volume
>> +		    my ($vtype, undef, undef, undef, undef, undef, $fmt)
>> +			= PVE::Storage::parse_volname($storecfg, $source);
>> +		    my $needs_extraction = PVE::QemuServer::Helpers::needs_extraction($vtype, $fmt);
>> +		    if ($needs_extraction) {
>> +			print "extracting $source\n";
>> +			my $extracted_volid
>> +			     = PVE::GuestImport::extract_disk_from_import_file($source, $vmid);
>> +			print "finished extracting to $extracted_volid\n";
>> +			push @$vollist, $extracted_volid;
>> +			$source = $extracted_volid;
>> +
>> +			my (undef, undef, undef, $parent)
>> +			    = PVE::Storage::volume_size_info($storecfg, $source);
>> +			die "importing from extracted images with backing file ($parent) not allowed\n"
>> +			    if $parent;
>> +		    }
>> +
>>   		    if ($live_import && $ds ne 'efidisk0') {
>>   			my $path = PVE::Storage::path($storecfg, $source)
>>   			    or die "failed to get a path for '$source'\n";
>> -			$source = $path;
>> -			($size, my $source_format) = PVE::Storage::file_size_info($source);
>> -			die "could not get file size of $source\n" if !$size;
>> +			($size, my $source_format) = PVE::Storage::file_size_info($path);
>> +			die "could not get file size of $path\n" if !$size;
>>   			$live_import_mapping->{$ds} = {
>> -			    path => $source,
>> +			    path => $path,
>>   			    format => $source_format,
>>   			};
>> +			$live_import_mapping->{$ds}->{'delete-after-finish'} = $source
>> +			    if $needs_extraction;
>>   		    } else {
>>   			my $dest_info = {
>>   			    vmid => $vmid,
>> @@ -413,8 +441,14 @@ my sub create_disks : prototype($$$$$$$$$$) {
>>   			$dest_info->{efisize} = PVE::QemuServer::get_efivars_size($conf, $disk)
>>   			    if $ds eq 'efidisk0';
>>   
>> -			($dst_volid, $size) = eval {
>> -			    $import_from_volid->($storecfg, $source, $dest_info, $vollist);
>> +			eval {
>> +			    ($dst_volid, $size)
>> +				= $import_from_volid->($storecfg, $source, $dest_info, $vollist);
>> +
>> +			    # remove extracted volumes after importing
>> +			    PVE::Storage::vdisk_free($storecfg, $source) if $needs_extraction;
>> +			    print "cleaned up extracted image $source\n";
>> +			    @$vollist = grep { $_ ne $source } @$vollist;
>>   			};
>>   			die "cannot import from '$source' - $@" if $@;
>>   		    }
>> @@ -1939,7 +1973,6 @@ my $update_vm_api  = sub {
>>   
>>   		    assert_scsi_feature_compatibility($opt, $conf, $storecfg, $param->{$opt})
>>   			if $opt =~ m/^scsi\d+$/;
>> -
>>   		    my (undef, $created_opts) = create_disks(
>>   			$rpcenv,
>>   			$authuser,
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 5df0c96d..76136570 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -7310,6 +7310,7 @@ sub live_import_from_files {
>>       my ($mapping, $vmid, $conf, $restore_options) = @_;
>>   
>>       my $live_restore_backing = {};
>> +    my $sources_to_remove = [];
>>       for my $dev (keys %$mapping) {
>>   	die "disk not support for live-restoring: '$dev'\n"
>>   	    if !is_valid_drivename($dev) || $dev =~ /^(?:efidisk|tpmstate)/;
>> @@ -7330,6 +7331,9 @@ sub live_import_from_files {
>>   	    . ",read-only=on"
>>   	    . ",file.driver=file,file.filename=$path"
>>   	};
>> +
>> +	my $source_volid = $info->{'delete-after-finish'};
>> +	push $sources_to_remove->@*, $source_volid if defined($source_volid);
>>       };
>>   
>>       my $storecfg = PVE::Storage::config();
>> @@ -7373,6 +7377,14 @@ sub live_import_from_files {
>>   
>>       my $err = $@;
>>   
>> +    for my $volid ($sources_to_remove->@*) {
>> +	eval {
>> +	    PVE::Storage::vdisk_free($storecfg, $volid);
>> +	    print "cleaned up extracted image $volid\n";
>> +	};
>> +	warn "An error occurred while cleaning up source images: $@\n" if $@;
>> +    }
>> +
>>       if ($err) {
>>   	warn "An error occurred during live-restore: $err\n";
>>   	_do_vm_stop($storecfg, $vmid, 1, 1, 10, 0, 1);
>> diff --git a/PVE/QemuServer/Helpers.pm b/PVE/QemuServer/Helpers.pm
>> index 0afb6317..15e2496c 100644
>> --- a/PVE/QemuServer/Helpers.pm
>> +++ b/PVE/QemuServer/Helpers.pm
>> @@ -225,4 +225,9 @@ sub windows_version {
>>       return $winversion;
>>   }
>>   
>> +sub needs_extraction {
>> +    my ($vtype, $fmt) = @_;
>> +    return $vtype eq 'import' && $fmt =~ m/^ova\+(.*)$/;
>> +}
>> +
>>   1;
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel