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