From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id DA9E71FF3A0 for ; Thu, 13 Jun 2024 12:29:24 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 76FEC1E759; Thu, 13 Jun 2024 12:30:00 +0200 (CEST) Message-ID: <0b831199-947f-4849-96ed-c0e5f19a51f4@proxmox.com> Date: Thu, 13 Jun 2024 12:29:55 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Proxmox VE development discussion , Max Carrara References: <20240524132209.703402-1-d.csapak@proxmox.com> <20240524132209.703402-16-d.csapak@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: X-SPAM-LEVEL: Spam detection results: 0 AWL 0.021 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH qemu-server v4 3/4] api: create: implement extracting disks when needed for import-from X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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 >> --- >> 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