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.csa...@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@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
