From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Fiona Ebner <f.ebner@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH storage 3/9] plugin: dir: handle ova files for import
Date: Wed, 17 Apr 2024 15:39:10 +0200 [thread overview]
Message-ID: <1713360754.0ry1mvqt1q.astroid@yuna.none> (raw)
In-Reply-To: <3b7027dc-7dd6-4a1f-bfda-7e21d476df0a@proxmox.com>
On April 17, 2024 3:07 pm, Dominik Csapak wrote:
> On 4/17/24 12:52, Fiona Ebner wrote:
>> Am 16.04.24 um 15:18 schrieb Dominik Csapak:
>>> since we want to handle ova files (which are only ovf+vmdks bundled in a
>>> tar file) for import, add code that handles that.
>>>
>>> we introduce a valid volname for files contained in ovas like this:
>>>
>>> storage:import/archive.ova/disk-1.vmdk
>>>
>>> by basically treating the last part of the path as the name for the
>>> contained disk we want.
>>>
>>> we then provide 3 functions to use for that:
>>>
>>> * copy_needs_extraction: determines from the given volid (like above) if
>>> that needs extraction to copy it, currently only 'import' vtype +
>>> defined format returns true here (if we have more options in the
>>> future, we can of course easily extend that)
>>>
>>> * extract_disk_from_import_file: this actually extracts the file from
>>> the archive. Currently only ova is supported, so the extraction with
>>> 'tar' is hardcoded, but again we can easily extend/modify that should
>>> we need to.
>>>
>>> we currently extract into the import storage in a directory named:
>>> `.tmp_<pid>_<targetvmid>` which should not clash with concurrent
>>> operations (though we do extract it multiple times then)
>>>
>>
>> Could we do "extract upon upload", "tar upon download" instead? Sure
>> some people surely want to drop the ova manually, but we could tell them
>> they need to extract it first too. Depending on the amount of headache
>> this would save us, it might be worth it.
>
> we could, but this opens a whole other can of worms, namely
> what to do with conflicting filenames for different ovas?
>
> we'd then either have to magically match the paths from the ovfs
> to some subdir that don't overlap
we could just use the ova name as dir name, and never store the ova
under that name but use some tmp placeholder for that ;)
>
> or we'd have to abort everytime we encounter identical disk names
>
> IMHO this would be less practical than just extract on demand...
>
>>
>>> alternatively we could implement either a 'tmpstorage' parameter,
>>> or use e.g. '/var/tmp/' or similar, but re-using the current storage
>>> seemed ok.
>>>
>>> * cleanup_extracted_image: intended to cleanup the extracted images from
>>> above, including the surrounding temporary directory
>>>
>>> we have to modify the `parse_ovf` a bit to handle the missing disk
>>> images, and we parse the size out of the ovf part (since this is
>>> informal only, it should be no problem if we cannot parse it sometimes)
>>>
>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>> ---
>>> src/PVE/API2/Storage/Status.pm | 1 +
>>> src/PVE/Storage.pm | 59 ++++++++++++++++++++++++++++++++++
>>> src/PVE/Storage/DirPlugin.pm | 13 +++++++-
>>> src/PVE/Storage/OVF.pm | 53 ++++++++++++++++++++++++++----
>>> src/PVE/Storage/Plugin.pm | 5 +++
>>> 5 files changed, 123 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm
>>> index f7e324f..77ed57c 100644
>>> --- a/src/PVE/API2/Storage/Status.pm
>>> +++ b/src/PVE/API2/Storage/Status.pm
>>> @@ -749,6 +749,7 @@ __PACKAGE__->register_method({
>>> 'efi-state-lost',
>>> 'guest-is-running',
>>> 'nvme-unsupported',
>>> + 'ova-needs-extracting',
>>> 'ovmf-with-lsi-unsupported',
>>> 'serial-port-socket-only',
>>> ],
>>> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
>>> index f8ea93d..bc073ef 100755
>>> --- a/src/PVE/Storage.pm
>>> +++ b/src/PVE/Storage.pm
>>> @@ -2189,4 +2189,63 @@ sub get_import_metadata {
>>> return $plugin->get_import_metadata($scfg, $volname, $storeid);
>>> }
>>>
>>
>> Shouldn't the following three functions call into plugin methods
>> instead? That'd seem much more future-proof to me.
>
> could be, i just did not want to extend the plugin api for that
> but as fabian wrote, maybe we should put them in qemu-server
> altogether for now?
>
> (after thinking about it a bit, i'd be in favor of putting it in
> qemu-server, because mainly i don't want to add to the plugin api further)
>
> what do you think @fiona @fabian?
another alternative would be to put them into the non-storage-plugin OVF
helper module?
>>> +sub copy_needs_extraction {
>>> + my ($volid) = @_;
>>> + my ($storeid, $volname) = parse_volume_id($volid);
>>> + my $cfg = config();
>>> + my $scfg = storage_config($cfg, $storeid);
>>> + my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>>> +
>>> + my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $file_format) =
>>> + $plugin->parse_volname($volname);
>>> +
>>> + return $vtype eq 'import' && defined($file_format);
>>
>> E.g this seems rather hacky, and puts a weird coupling on a future
>> import plugin's parse_volname() function (presence of $file_format).
>
> would it be better to check the volid again for '.ova/something$' ?
> or do you have a better idea?
> (especially if we want to have this maybe in qemu-server)
hmm, could parse_volname return 'ova' as format? or 'ova+vmdk'? we don't
actually need the format for extracting, and afterwards we get it from
the extract file name anyway?
>>> +}
>>> +
>>> +sub extract_disk_from_import_file {
>>> + my ($volid, $vmid) = @_;
>>> +
>>> + my ($storeid, $volname) = parse_volume_id($volid);
>>> + my $cfg = config();
>>> + my $scfg = storage_config($cfg, $storeid);
>>> + my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>>> +
>>> + my ($vtype, $name, undef, undef, undef, undef, $file_format) =
>>> + $plugin->parse_volname($volname);
>>> +
>>> + die "only files with content type 'import' can be extracted\n"
>>> + if $vtype ne 'import' || !defined($file_format);
>>> +
>>> + # extract the inner file from the name
>>> + if ($volid =~ m!${name}/([^/]+)$!) {
>>> + $name = $1;
>>> + }
>>> +
>>> + my ($source_file) = $plugin->path($scfg, $volname, $storeid);
>>> +
>>> + my $destdir = $plugin->get_subdir($scfg, 'import');
>>> + my $pid = $$;
>>> + $destdir .= "/.tmp_${pid}_${vmid}";
>>> + mkdir $destdir;
>>> +
>>> + ($source_file) = $source_file =~ m|^(/.*)|; # untaint
>>> +
>>> + run_command(['tar', '-x', '-C', $destdir, '-f', $source_file, $name]);
>>> +
>>> + return "$destdir/$name";
>>> +}
>>> +
>>> +sub cleanup_extracted_image {
>>> + my ($source) = @_;
>>> +
>>> + if ($source =~ m|^(/.+/\.tmp_[0-9]+_[0-9]+)/[^/]+$|) {
>>> + my $tmpdir = $1;
>>> +
>>> + unlink $source or $! == ENOENT or die "removing image $source failed: $!\n";
>>> + rmdir $tmpdir or $! == ENOENT or die "removing tmpdir $tmpdir failed: $!\n";
>>> + } else {
>>> + die "invalid extraced image path '$source'\n";
>>> + }
>>> +}
>>> +
>>> 1;
>>> diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm
>>> index 4dc7708..50ceab7 100644
>>> --- a/src/PVE/Storage/DirPlugin.pm
>>> +++ b/src/PVE/Storage/DirPlugin.pm
>>> @@ -260,14 +260,25 @@ sub get_import_metadata {
>>> # NOTE: all types must be added to the return schema of the import-metadata API endpoint
>>> my $warnings = [];
>>>
>>> + my $isOva = 0;
>>> + if ($path =~ m!\.ova!) {
>>
>> Would be nicer if parse_volname() would return the $file_format and we
>> chould check for that. Also missing the $ in the regex, so you'd
>> mismatch a weird filename like ABC.ovaXYZ.ovf or?
>
> yeah the $ is missing, and yes, we could return ova/ovf as format there
> as we want to change the 'needs extracting' check anyway
>
>
>>
>>> + $isOva = 1;
>>> + push @$warnings, { type => 'ova-needs-extracting' };
>>> + }
>>> my $res = PVE::Storage::OVF::parse_ovf($path, $isOva);
>>> my $disks = {};
>>> for my $disk ($res->{disks}->@*) {
>>> my $id = $disk->{disk_address};
>>> my $size = $disk->{virtual_size};
>>> my $path = $disk->{relative_path};
>>> + my $volid;
>>> + if ($isOva) {
>>> + $volid = "$storeid:$volname/$path";
>>> + } else {
>>> + $volid = "$storeid:import/$path",
>>> + }
>>> $disks->{$id} = {
>>> - volid => "$storeid:import/$path",
>>> + volid => $volid,
>>> defined($size) ? (size => $size) : (),
>>> };
>>> }
>>> diff --git a/src/PVE/Storage/OVF.pm b/src/PVE/Storage/OVF.pm
>>> index 4a322b9..fb850a8 100644
>>> --- a/src/PVE/Storage/OVF.pm
>>> +++ b/src/PVE/Storage/OVF.pm
>>> @@ -85,11 +85,37 @@ sub id_to_pve {
>>> }
>>> }
>>>
>>> +# technically defined in DSP0004 (https://www.dmtf.org/dsp/DSP0004) as an ABNF
>>> +# but realistically this always takes the form of 'bytes * base^exponent'
>>
>> The comment wrongly says 'bytes' instead of 'byte' (your test examples
>> confirm this).
>>
>>> +sub try_parse_capacity_unit {
>>> + my ($unit_text) = @_;
>>> +
>>> + if ($unit_text =~ m/^\s*byte\s*\*\s*([0-9]+)\s*\^\s*([0-9]+)\s*$/) {
>>
>> Fun regex :P
>>
>>> + my $base = $1;
>>> + my $exp = $2;
>>> + return $base ** $exp;
>>> + }
>>> +
>>> + return undef;
>>> +}
>>> +
>>
>> (...)
>>
>>> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
>>> index deaf8b2..ea069ab 100644
>>> --- a/src/PVE/Storage/Plugin.pm
>>> +++ b/src/PVE/Storage/Plugin.pm
>>> @@ -654,6 +654,11 @@ sub parse_volname {
>>> return ('backup', $fn);
>>> } elsif ($volname =~ m!^snippets/([^/]+)$!) {
>>> return ('snippets', $1);
>>> + } elsif ($volname =~ m!^import/([^/]+\.ova)\/([^/]+)$!) {
>>> + my $archive = $1;
>>> + my $file = $2;
>>> + my (undef, $format, undef) = parse_name_dir($file);
>>> + return ('import', $archive, 0, undef, undef, undef, $format);
>>
>> So we return the same $name for different things here? Not super happy
>> with that either. If we were to get creative we could say the archive is
>> the "base" of the image, but surely also comes with caveats.
>
> i'll change this in a v2 should not be necessary
>
>>
>>> } elsif ($volname =~ m!^import/([^/]+$PVE::Storage::IMPORT_EXT_RE_1)$!) {
>>> return ('import', $1);
>>> } elsif ($volname =~ m!^import/([^/]+\.(raw|vmdk|qcow2))$!) {
>
>
>
> _______________________________________________
> 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
next prev parent reply other threads:[~2024-04-17 13:39 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-16 13:18 [pve-devel] [PATCH storage/qemu-server/pve-manager] implement ova/ovf import for directory type storages Dominik Csapak
2024-04-16 13:18 ` [pve-devel] [PATCH storage 1/9] copy OVF.pm from qemu-server Dominik Csapak
2024-04-16 15:02 ` Thomas Lamprecht
2024-04-17 9:19 ` Fiona Ebner
2024-04-17 9:26 ` Thomas Lamprecht
2024-04-16 13:18 ` [pve-devel] [PATCH storage 2/9] plugin: dir: implement import content type Dominik Csapak
2024-04-17 10:07 ` Fiona Ebner
2024-04-17 10:07 ` Fiona Ebner
2024-04-17 13:13 ` Dominik Csapak
2024-04-17 12:46 ` Fabian Grünbichler
2024-04-16 13:18 ` [pve-devel] [PATCH storage 3/9] plugin: dir: handle ova files for import Dominik Csapak
2024-04-17 10:52 ` Fiona Ebner
2024-04-17 13:07 ` Dominik Csapak
2024-04-17 13:39 ` Fabian Grünbichler [this message]
2024-04-18 7:22 ` Fiona Ebner
2024-04-18 7:25 ` Fiona Ebner
2024-04-18 8:55 ` Fabian Grünbichler
2024-04-17 12:45 ` Fabian Grünbichler
2024-04-17 13:10 ` Dominik Csapak
2024-04-17 13:52 ` Fabian Grünbichler
2024-04-17 14:07 ` Dominik Csapak
2024-04-18 6:46 ` Fabian Grünbichler
2024-04-16 13:18 ` [pve-devel] [PATCH storage 4/9] ovf: implement parsing the ostype Dominik Csapak
2024-04-17 11:32 ` Fiona Ebner
2024-04-17 13:14 ` Dominik Csapak
2024-04-18 7:31 ` Fiona Ebner
2024-04-16 13:18 ` [pve-devel] [PATCH storage 5/9] ovf: implement parsing out firmware type Dominik Csapak
2024-04-17 11:43 ` Fiona Ebner
2024-04-16 13:18 ` [pve-devel] [PATCH storage 6/9] ovf: implement rudimentary boot order Dominik Csapak
2024-04-17 11:54 ` Fiona Ebner
2024-04-17 13:15 ` Dominik Csapak
2024-04-16 13:19 ` [pve-devel] [PATCH storage 7/9] ovf: implement parsing nics Dominik Csapak
2024-04-17 12:09 ` Fiona Ebner
2024-04-17 13:16 ` Dominik Csapak
2024-04-18 8:22 ` Fiona Ebner
2024-04-16 13:19 ` [pve-devel] [PATCH storage 8/9] api: allow ova upload/download Dominik Csapak
2024-04-18 8:05 ` Fiona Ebner
2024-04-16 13:19 ` [pve-devel] [PATCH storage 9/9] plugin: enable import for nfs/btfs/cifs/cephfs Dominik Csapak
2024-04-18 8:43 ` Fiona Ebner
2024-04-16 13:19 ` [pve-devel] [PATCH qemu-server 1/3] api: delete unused OVF.pm Dominik Csapak
2024-04-18 8:52 ` Fiona Ebner
2024-04-18 8:57 ` Dominik Csapak
2024-04-18 9:03 ` Fiona Ebner
2024-04-16 13:19 ` [pve-devel] [PATCH qemu-server 2/3] use OVF from Storage Dominik Csapak
2024-04-18 9:07 ` Fiona Ebner
2024-04-16 13:19 ` [pve-devel] [PATCH qemu-server 3/3] api: create: implement extracting disks when needed for import-from Dominik Csapak
2024-04-18 9:41 ` Fiona Ebner
2024-04-18 9:48 ` Dominik Csapak
2024-04-18 9:55 ` Fiona Ebner
2024-04-18 9:58 ` Dominik Csapak
2024-04-18 10:01 ` Fiona Ebner
2024-04-16 13:19 ` [pve-devel] [PATCH manager 1/4] ui: fix special 'import' icon for non-esxi storages Dominik Csapak
2024-04-16 13:19 ` [pve-devel] [PATCH manager 2/4] ui: guest import: add ova-needs-extracting warning text Dominik Csapak
2024-04-16 13:19 ` [pve-devel] [PATCH manager 3/4] ui: enable import content type for relevant storages Dominik Csapak
2024-04-16 13:19 ` [pve-devel] [PATCH manager 4/4] ui: enable upload/download buttons for 'import' type storages Dominik Csapak
2024-04-17 12:37 ` Fabian Grünbichler
2024-04-18 11:20 ` Fiona Ebner
2024-04-18 11:23 ` Dominik Csapak
2024-04-18 11:26 ` Fiona Ebner
2024-04-17 13:11 ` [pve-devel] [PATCH storage/qemu-server/pve-manager] implement ova/ovf import for directory " Fabian Grünbichler
2024-04-17 13:19 ` Dominik Csapak
2024-04-18 6:40 ` Fabian Grünbichler
2024-04-18 9:27 ` Dominik Csapak
2024-04-18 10:35 ` Fiona Ebner
2024-04-18 11:10 ` Dominik Csapak
2024-04-18 11:13 ` Fiona Ebner
2024-04-18 11:17 ` Fabian Grünbichler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1713360754.0ry1mvqt1q.astroid@yuna.none \
--to=f.gruenbichler@proxmox.com \
--cc=f.ebner@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox