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

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

On April 29, 2024 1:21 pm, 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          | 44 ++++++++++++++++++++++++++++++---------
>  PVE/QemuServer.pm         |  5 ++++-
>  PVE/QemuServer/Helpers.pm | 10 +++++++++
>  3 files changed, 48 insertions(+), 11 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 2a349c8c..d32967dc 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) = $plugin->parse_volname($volname);

please use PVE::Storage instead!

> +
> +		raise_param_exc({ $ds => "$src_image has wrong type '$vtype' - needs to be 'images' or 'import'" })
> +		    if $vtype ne 'images' && $vtype ne 'import';
> +
> +		if (PVE::GuestImport::copy_needs_extraction($src_image)) {

like noted in the patch introducing that helper, it could just be
inlined here..

> +		    raise_param_exc({ $ds => "$src_image is not on an storage with 'images' content type."})
> +			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()
> @@ -335,6 +345,7 @@ my sub create_disks : prototype($$$$$$$$$$) {
>      my $res = {};
>  
>      my $live_import_mapping = {};
> +    my $delete_sources = [];

we already have a list of created volumes here that are cleaned up on
error ($vollist), so this is just to also clean them up after importing
if that worked? and then, it's basically just for live importing (since
for non-live imports, we can just free the volume right after the import
was successful?)? but live imports already have their own hash anyway
($live_import_mapping), we could just annotate the volume there?

>  
>      my $code = sub {
>  	my ($ds, $disk) = @_;
> @@ -392,6 +403,12 @@ my sub create_disks : prototype($$$$$$$$$$) {
>  		$needs_creation = $live_import;
>  
>  		if (PVE::Storage::parse_volume_id($source, 1)) { # PVE-managed volume
> +		    if (PVE::GuestImport::copy_needs_extraction($source)) { # needs extraction beforehand
> +			print "extracting $source\n";
> +			$source = PVE::GuestImport::extract_disk_from_import_file($source, $vmid);
> +			print "finished extracting to $source\n";

this is a bit hard to follow, it might be more readable to do

my $extracted_volid = ..;
$source = $extracted_volid;

even if the end result is the same, it makes it much more explicit what
is happening here with $source?

> +			push @$delete_sources, $source;

this could just push to $vollist I think..

> +		    }
>  		    if ($live_import && $ds ne 'efidisk0') {
>  			my $path = PVE::Storage::path($storecfg, $source)
>  			    or die "failed to get a path for '$source'\n";
> @@ -514,13 +531,14 @@ my sub create_disks : prototype($$$$$$$$$$) {
>  	    eval { PVE::Storage::vdisk_free($storecfg, $volid); };
>  	    warn $@ if $@;
>  	}
> +	PVE::QemuServer::Helpers::cleanup_extracted_images($delete_sources);

then this would not be needed, since we cleanup all the freshly
allocated volumes above anyway..

>  	die $err;
>      }
>  
>      # don't return empty import mappings
>      $live_import_mapping = undef if !%$live_import_mapping;
>  
> -    return ($vollist, $res, $live_import_mapping);
> +    return ($vollist, $res, $live_import_mapping, $delete_sources);

this can then be dropped as well..

>  };
>  
>  my $check_cpu_model_access = sub {
> @@ -1079,6 +1097,7 @@ __PACKAGE__->register_method({
>  
>  	my $createfn = sub {
>  	    my $live_import_mapping = {};
> +	    my $delete_sources = [];

so can this

>  
>  	    # ensure no old replication state are exists
>  	    PVE::ReplicationState::delete_guest_states($vmid);
> @@ -1096,7 +1115,7 @@ __PACKAGE__->register_method({
>  
>  		my $vollist = [];
>  		eval {
> -		    ($vollist, my $created_opts, $live_import_mapping) = create_disks(
> +		    ($vollist, my $created_opts, $live_import_mapping, $delete_sources) = create_disks(

and this

>  			$rpcenv,
>  			$authuser,
>  			$conf,
> @@ -1149,6 +1168,7 @@ __PACKAGE__->register_method({
>  			eval { PVE::Storage::vdisk_free($storecfg, $volid); };
>  			warn $@ if $@;
>  		    }
> +		    PVE::QemuServer::Helpers::cleanup_extracted_images($delete_sources);

and this :)

>  		    die "$emsg $err";
>  		}
>  
> @@ -1165,7 +1185,7 @@ __PACKAGE__->register_method({
>  		warn $@ if $@;
>  		return;
>  	    } else {
> -		return $live_import_mapping;
> +		return ($live_import_mapping, $delete_sources);

as well as this

>  	    }
>  	};
>  
> @@ -1192,7 +1212,7 @@ __PACKAGE__->register_method({
>  	    $code = sub {
>  		# If a live import was requested the create function returns
>  		# the mapping for the startup.
> -		my $live_import_mapping = eval { $createfn->() };
> +		my ($live_import_mapping, $delete_sources) = eval { $createfn->() };

this

>  		if (my $err = $@) {
>  		    eval {
>  			my $conffile = PVE::QemuConfig->config_file($vmid);
> @@ -1214,7 +1234,10 @@ __PACKAGE__->register_method({
>  			$vmid,
>  			$conf,
>  			$import_options,
> +			$delete_sources,

this

>  		    );
> +		} else {
> +		    PVE::QemuServer::Helpers::cleanup_extracted_images($delete_sources);

and this?

>  		}
>  	    };
>  	}
> @@ -1939,8 +1962,7 @@ 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(
> +		    my (undef, $created_opts, undef, $delete_sources) = create_disks(

not needed either

>  			$rpcenv,
>  			$authuser,
>  			$conf,
> @@ -1954,6 +1976,8 @@ my $update_vm_api  = sub {
>  		    );
>  		    $conf->{pending}->{$_} = $created_opts->{$_} for keys $created_opts->%*;
>  
> +		    PVE::QemuServer::Helpers::cleanup_extracted_images($delete_sources);

same here

> +
>  		    # default legacy boot order implies all cdroms anyway
>  		    if (@bootorder) {
>  			# append new CD drives to bootorder to mark them bootable
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 82e7d6a6..4bd0ae85 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -7303,7 +7303,7 @@ sub pbs_live_restore {
>  # therefore already handled in the `$create_disks()` call happening in the
>  # `create` api call
>  sub live_import_from_files {
> -    my ($mapping, $vmid, $conf, $restore_options) = @_;
> +    my ($mapping, $vmid, $conf, $restore_options, $delete_sources) = @_;

here

>  
>      my $live_restore_backing = {};
>      for my $dev (keys %$mapping) {
> @@ -7364,6 +7364,8 @@ sub live_import_from_files {
>  	    mon_cmd($vmid, 'blockdev-del', 'node-name' => "drive-$ds-restore");
>  	}
>  
> +	PVE::QemuServer::Helpers::cleanup_extracted_images($delete_sources);

and this could then just free based on a flag in the mapping..

> +
>  	close($qmeventd_fd);
>      };
>  
> @@ -7372,6 +7374,7 @@ sub live_import_from_files {
>      if ($err) {
>  	warn "An error occurred during live-restore: $err\n";
>  	_do_vm_stop($storecfg, $vmid, 1, 1, 10, 0, 1);
> +	PVE::QemuServer::Helpers::cleanup_extracted_images($delete_sources);

and here as well..

>  	die "live-restore failed\n";
>      }
>  
> diff --git a/PVE/QemuServer/Helpers.pm b/PVE/QemuServer/Helpers.pm
> index 0afb6317..f6bec1d4 100644
> --- a/PVE/QemuServer/Helpers.pm
> +++ b/PVE/QemuServer/Helpers.pm
> @@ -6,6 +6,7 @@ use warnings;
>  use File::stat;
>  use JSON;
>  
> +use PVE::GuestImport;
>  use PVE::INotify;
>  use PVE::ProcFSTools;
>  
> @@ -225,4 +226,13 @@ sub windows_version {
>      return $winversion;
>  }
>  
> +sub cleanup_extracted_images {
> +    my ($delete_sources) = @_;
> +
> +    for my $source (@$delete_sources) {
> +	eval { PVE::GuestImport::cleanup_extracted_image($source) };
> +	warn $@ if $@;
> +    }
> +}
> +

and this can then be dropped, since it's just a wrapper around a helper
that is itself just a wrapper of vdisk_free..

>  1;
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> 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