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

Max Carrara m.carrara at proxmox.com
Fri Jun 14 10:36:54 CEST 2024


On Thu Jun 13, 2024 at 12:29 PM CEST, Dominik Csapak wrote:
> 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 at 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 ;)

Yeah, my bad; sterzy had pointed that out off-list, so that was
brainfart on my side. Disregard my comment :D

>
> (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

Alright, that's fair! Thanks for elaborating!

>
> > 
> >> +			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 at lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> > 
> > 





More information about the pve-devel mailing list