[pve-devel] [RFC v10 qemu-server/manager] API for disk import and OVF

[pve-devel] [RFC v10 qemu-server/manager] API for disk import and OVF

[Fabian Ebner]

Extend qm importdisk/importovf functionality to the API.


Used Dominic's latest version[0] as a starting point. GUI part still
needs to be rebased/updated, so it's not included here.


Changes from v9:

* Split patch into smaller parts

* Some minor (style) fixes/improvements (see individual patches)

* Drop $manifest_only parameter for parse_ovf

    Instead, untaint the path when calling file_size_info, which makes
    the call also work via API which uses the -T switch. If we do want
    to keep $manifest_only, I'd argue that it should also skip the
    file existence check and not only the file size check. Opinions?

* Re-use do_import function

    Rather than duplicating most of it. The down side is the need to
    add a new parameter for skipping configuration update. But I
    suppose the plan is to have qm import switch to the new API at
    some point, and then do_import can be simplified.

* Instead of adding an import-sources parameter to the API, use a new
  import-from property for the disk, that's only available with
  import/alloc-enabled API endpoints via its own version of the schema

    Avoids the split across regular drive key parameters and
    'import_soruces', which avoids quite a bit of cross-checking
    between the two and parsing/passing around the latter.

    The big downsides are:
    * Schema handling is a bit messy.
    * Need to special case print_drive, because we do intermediate
      parse/print to cleanup drive paths. At a first glance, this
      seems not too easy to avoid without complicating things elsewehere.
    * Using the import-aware parse_drive in parse_volume, because that
      is used via the foreach_volume iterators handling the parameters
      of the import-enabled endpoints. Could be avoided by using for
      loops with the import-aware parse_drive instead of
      foreach_volume.

    Counter-arguments for using a single schema (citing Fabian G.):
    * docs/schema dump/api docs: shouldn't look like you can put that
      everywhere where we use the config schema
    * shouldn't have nasty side-effects if someone puts it into the
      config


After all, the 'import-from' disk property approach turned out to be
a uglier than I hoped it would.

My problem with the 'import-sources' API parameter approach (see [0]
for details) is that it requires specifying both
    scsi0: <target storage>:-1,<properties>
    import-sources: scsi0=/path/or/volid/for/source
leading to a not ideal user interface and parameter handling needing
cross-checks to verify that the right combination is specified, and
passing both around at the same time.

Another approach would be adding a new special volid syntax using
    my $IMPORT_DISK_RE = qr!^(([^/:\s]+):)import:(.*)$!;
allowing for e.g.
    qm set 126 -scsi1 rbdkvm:import:myzpool:vm-114-disk-0,aio=native
    qm set 126 -scsi2 rbdkvm:import:/dev/zvol/myzpool/vm-114-disk-1,backup=0
Yes, it's a hack, but it would avoid the pain points from both other
approaches and be very simple. See the end of the mail for a POC.


[0]: https://lists.proxmox.com/pipermail/pve-devel/2021-June/048564.html


pve-manager:

Fabian Ebner (1):
  api: nodes: add readovf endpoint

 PVE/API2/Nodes.pm | 7 +++++++
 1 file changed, 7 insertions(+)


qemu-server:

Dominic Jäger (1):
  api: support VM disk import

Fabian Ebner (6):
  schema: add pve-volume-id-or-absolute-path
  parse ovf: untaint path when calling file_size_info
  api: add endpoint for parsing .ovf files
  image convert: allow block device as source
  schema: drive: use separate schema when disk allocation is possible
  api: create disks: factor out common part from if/else

 PVE/API2/Qemu.pm             | 86 +++++++++++++++++++++++----------
 PVE/API2/Qemu/Makefile       |  2 +-
 PVE/API2/Qemu/OVF.pm         | 55 +++++++++++++++++++++
 PVE/QemuConfig.pm            |  2 +-
 PVE/QemuServer.pm            | 57 +++++++++++++++++++---
 PVE/QemuServer/Drive.pm      | 92 +++++++++++++++++++++++++++---------
 PVE/QemuServer/ImportDisk.pm |  2 +-
 PVE/QemuServer/OVF.pm        |  9 ++--
 8 files changed, 242 insertions(+), 63 deletions(-)
 create mode 100644 PVE/API2/Qemu/OVF.pm

-- 
2.30.2

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 6992f6f..6a22899 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -21,8 +21,9 @@ use PVE::ReplicationConfig;
 use PVE::GuestHelpers;
 use PVE::QemuConfig;
 use PVE::QemuServer;
-use PVE::QemuServer::Drive;
 use PVE::QemuServer::CPUConfig;
+use PVE::QemuServer::Drive;
+use PVE::QemuServer::ImportDisk;
 use PVE::QemuServer::Monitor qw(mon_cmd);
 use PVE::QemuServer::Machine;
 use PVE::QemuMigrate;
@@ -64,6 +65,7 @@ my $resolve_cdrom_alias = sub {
 };
 
 my $NEW_DISK_RE = qr!^(([^/:\s]+):)?(\d+(\.\d+)?)$!;
+my $IMPORT_DISK_RE = qr!^(([^/:\s]+):)import:(.*)$!;
 my $check_storage_access = sub {
    my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage) = @_;
 
@@ -86,6 +88,9 @@ my $check_storage_access = sub {
 	    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
 	    raise_param_exc({ storage => "storage '$storeid' does not support vm images"})
 		if !$scfg->{content}->{images};
+	} elsif (!$isCDROM && ($volid =~ $IMPORT_DISK_RE)) {
+	    warn "check access matched: $2 and $3\n";
+	    warn "TODO implement import access check!\n";
 	} else {
 	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
 	}
@@ -212,6 +217,29 @@ my $create_disks = sub {
 	    $disk->{size} = PVE::Tools::convert_size($size, 'kb' => 'b');
 	    delete $disk->{format}; # no longer needed
 	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
+	} elsif ($volid =~ $IMPORT_DISK_RE) {
+	    my ($storeid, $source) = ($2, $3);
+
+	    $source = PVE::Storage::abs_filesystem_path($storecfg, $source, 1);
+	    my $src_size = PVE::Storage::file_size_info($source);
+	    die "Could not get file size of $source" if !defined($src_size);
+
+	    my (undef, $dst_volid) = PVE::QemuServer::ImportDisk::do_import(
+		$source,
+		$vmid,
+		$storeid,
+		{
+		    drive_name => $ds,
+		    format => $disk->{format},
+		    'skip-config-update' => 1,
+		},
+	    );
+
+	    push @$vollist, $dst_volid;
+	    $disk->{file} = $dst_volid;
+	    $disk->{size} = $src_size;
+	    delete $disk->{format}; # no longer needed
+	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
 	} else {
 
 	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
index 51ad52e..7557cac 100755
--- a/PVE/QemuServer/ImportDisk.pm
+++ b/PVE/QemuServer/ImportDisk.pm
@@ -71,7 +71,7 @@ sub do_import {
 	PVE::Storage::activate_volumes($storecfg, [$dst_volid]);
 	PVE::QemuServer::qemu_img_convert($src_path, $dst_volid, $src_size, undef, $zeroinit);
 	PVE::Storage::deactivate_volumes($storecfg, [$dst_volid]);
-	PVE::QemuConfig->lock_config($vmid, $create_drive);
+	PVE::QemuConfig->lock_config($vmid, $create_drive) if !$params->{'skip-config-update'};
     };
     if (my $err = $@) {
 	eval { PVE::Storage::vdisk_free($storecfg, $dst_volid) };
-- 
2.30.2

[pve-devel] [PATCH v10 qemu-server 1/7] schema: add pve-volume-id-or-absolute-path

[Fabian Ebner]

Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
[split into its own patch + style fixes]
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Changes from v9:
    * Style fixes.

 PVE/QemuServer.pm | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 0071a06..819eb5f 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1054,11 +1054,17 @@ PVE::JSONSchema::register_format('pve-volume-id-or-qm-path', \&verify_volume_id_
 sub verify_volume_id_or_qm_path {
     my ($volid, $noerr) = @_;
 
-    if ($volid eq 'none' || $volid eq 'cdrom' || $volid =~ m|^/|) {
-	return $volid;
-    }
+    return $volid if $volid eq 'none' || $volid eq 'cdrom';
+
+    return verify_volume_id_or_absolute_path($volid, $noerr);
+}
+
+PVE::JSONSchema::register_format('pve-volume-id-or-absolute-path', \&verify_volume_id_or_absolute_path);
+sub verify_volume_id_or_absolute_path {
+    my ($volid, $noerr) = @_;
+
+    return $volid if $volid =~ m|^/|;
 
-    # if its neither 'none' nor 'cdrom' nor a path, check if its a volume-id
     $volid = eval { PVE::JSONSchema::check_format('pve-volume-id', $volid, '') };
     if ($@) {
 	return if $noerr;
-- 
2.30.2

[pve-devel] [PATCH v10 qemu-server 2/7] parse ovf: untaint path when calling file_size_info

[Fabian Ebner]

Prepare for calling parse_ovf via API, where the -T switch is used.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v10.

 PVE/QemuServer/OVF.pm | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/PVE/QemuServer/OVF.pm b/PVE/QemuServer/OVF.pm
index 0376cbf..4a0d373 100644
--- a/PVE/QemuServer/OVF.pm
+++ b/PVE/QemuServer/OVF.pm
@@ -221,10 +221,11 @@ ovf:Item[rasd:InstanceID='%s']/rasd:ResourceType", $controller_id);
 	    die "error parsing $filepath, file seems not to exist at $backing_file_path\n";
 	}
 
-	my $virtual_size;
-	if ( !($virtual_size = PVE::Storage::file_size_info($backing_file_path)) ) {
-	    die "error parsing $backing_file_path, size seems to be $virtual_size\n";
-	}
+	my $virtual_size = PVE::Storage::file_size_info(
+	    ($backing_file_path =~ m|^(/.*)|)[0] # untaint
+	);
+	die "error parsing $backing_file_path, cannot determine file size\n"
+	    if !$virtual_size;
 
 	$pve_disk = {
 	    disk_address => $pve_disk_address,
-- 
2.30.2

Re: [pve-devel] [PATCH v10 qemu-server 2/7] parse ovf: untaint path when calling file_size_info

[Fabian Grünbichler]

On January 13, 2022 11:08 am, Fabian Ebner wrote:
> Prepare for calling parse_ovf via API, where the -T switch is used.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> New in v10.
> 
>  PVE/QemuServer/OVF.pm | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/PVE/QemuServer/OVF.pm b/PVE/QemuServer/OVF.pm
> index 0376cbf..4a0d373 100644
> --- a/PVE/QemuServer/OVF.pm
> +++ b/PVE/QemuServer/OVF.pm
> @@ -221,10 +221,11 @@ ovf:Item[rasd:InstanceID='%s']/rasd:ResourceType", $controller_id);
>  	    die "error parsing $filepath, file seems not to exist at $backing_file_path\n";
>  	}
>  
> -	my $virtual_size;
> -	if ( !($virtual_size = PVE::Storage::file_size_info($backing_file_path)) ) {
> -	    die "error parsing $backing_file_path, size seems to be $virtual_size\n";
> -	}
> +	my $virtual_size = PVE::Storage::file_size_info(
> +	    ($backing_file_path =~ m|^(/.*)|)[0] # untaint
> +	);
> +	die "error parsing $backing_file_path, cannot determine file size\n"
> +	    if !$virtual_size;

nit: I think I'd prefer untainting outside of the call to 
file_size_info..

>  
>  	$pve_disk = {
>  	    disk_address => $pve_disk_address,
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

[pve-devel] [PATCH v10 qemu-server 3/7] api: add endpoint for parsing .ovf files

[Fabian Ebner]

Co-developed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
[split into its own patch + minor improvements/style fixes]
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Changes from v9:
    * Include $! in the error for the file check.
    * Have json_ovf_properties return all of them rather than just
      the disk-related ones, and add description for cpu/name/memory.
    * Tiny style fixes foreach -> for, etc.

The file check can also fail because of permission problems, since the
API endpoint is not protected => 1, when used via the web UI and when
the manifest is at a location not accessible to www-data (e.g. in
/root/ on a default installation).

Including $! in the error message helps of course, but I'm sure
there'll be users wondering why they get permission errors while being
logged in as root in the web UI. Not sure what to do about it though.

 PVE/API2/Qemu/Makefile |  2 +-
 PVE/API2/Qemu/OVF.pm   | 55 ++++++++++++++++++++++++++++++++++++++++++
 PVE/QemuServer.pm      | 32 ++++++++++++++++++++++++
 3 files changed, 88 insertions(+), 1 deletion(-)
 create mode 100644 PVE/API2/Qemu/OVF.pm

diff --git a/PVE/API2/Qemu/Makefile b/PVE/API2/Qemu/Makefile
index 5d4abda..bdd4762 100644
--- a/PVE/API2/Qemu/Makefile
+++ b/PVE/API2/Qemu/Makefile
@@ -1,4 +1,4 @@
-SOURCES=Agent.pm CPU.pm Machine.pm
+SOURCES=Agent.pm CPU.pm Machine.pm OVF.pm
 
 .PHONY: install
 install:
diff --git a/PVE/API2/Qemu/OVF.pm b/PVE/API2/Qemu/OVF.pm
new file mode 100644
index 0000000..b1d79d2
--- /dev/null
+++ b/PVE/API2/Qemu/OVF.pm
@@ -0,0 +1,55 @@
+package PVE::API2::Qemu::OVF;
+
+use strict;
+use warnings;
+
+use PVE::JSONSchema qw(get_standard_option);
+use PVE::QemuServer::OVF;
+use PVE::RESTHandler;
+
+use base qw(PVE::RESTHandler);
+
+__PACKAGE__->register_method ({
+    name => 'index',
+    path => '',
+    method => 'GET',
+    proxyto => 'node',
+    description => "Read an .ovf manifest.",
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    node => get_standard_option('pve-node'),
+	    manifest => {
+		description => ".ovf manifest",
+		type => 'string',
+	    },
+	},
+    },
+    returns => {
+	description => "VM config according to .ovf manifest.",
+	type => "object",
+    },
+    returns => {
+	type => 'object',
+	additionalProperties => 1,
+	properties => PVE::QemuServer::json_ovf_properties({}),
+    },
+    code => sub {
+	my ($param) = @_;
+
+	my $manifest = $param->{manifest};
+	die "check for file $manifest failed - $!\n" if !-f $manifest;
+
+	my $parsed = PVE::QemuServer::OVF::parse_ovf($manifest);
+	my $result;
+	$result->{cores} = $parsed->{qm}->{cores};
+	$result->{name} =  $parsed->{qm}->{name};
+	$result->{memory} = $parsed->{qm}->{memory};
+	my $disks = $parsed->{disks};
+	for my $disk (@$disks) {
+	    $result->{$disk->{disk_address}} = $disk->{backing_file};
+	}
+	return $result;
+    }});
+
+1;
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 819eb5f..98eb6b3 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2221,6 +2221,38 @@ sub json_config_properties {
     return $prop;
 }
 
+# Properties that we can read from an OVF file
+sub json_ovf_properties {
+    my $prop = shift;
+
+    for my $device (PVE::QemuServer::Drive::valid_drive_names()) {
+	$prop->{$device} = {
+	    type => 'string',
+	    format => 'pve-volume-id-or-absolute-path',
+	    description => "Disk image that gets imported to $device",
+	    optional => 1,
+	};
+    }
+
+    $prop->{cores} = {
+	type => 'integer',
+	description => "The number of CPU cores.",
+	optional => 1,
+    };
+    $prop->{memory} = {
+	type => 'integer',
+	description => "Amount of RAM for the VM in MB.",
+	optional => 1,
+    };
+    $prop->{name} = {
+	type => 'string',
+	description => "Name of the VM.",
+	optional => 1,
+    };
+
+    return $prop;
+}
+
 # return copy of $confdesc_cloudinit to generate documentation
 sub cloudinit_config_properties {
 
-- 
2.30.2

[pve-devel] [PATCH v10 manager 1/1] api: nodes: add readovf endpoint

[Fabian Ebner]

Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
[split into its own patch + add to index]
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Needs dependency bump for qemu-server.

Changes from v9:
    * Add entry to /node/'s index.

 PVE/API2/Nodes.pm | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index d57a1937..5f6208d5 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -49,6 +49,7 @@ use PVE::API2::LXC;
 use PVE::API2::Network;
 use PVE::API2::NodeConfig;
 use PVE::API2::Qemu::CPU;
+use PVE::API2::Qemu::OVF;
 use PVE::API2::Qemu;
 use PVE::API2::Replication;
 use PVE::API2::Services;
@@ -71,6 +72,11 @@ __PACKAGE__->register_method ({
     path => 'qemu',
 });
 
+__PACKAGE__->register_method ({
+    subclass => "PVE::API2::Qemu::OVF",
+    path => 'readovf',
+});
+
 __PACKAGE__->register_method ({
     subclass => "PVE::API2::LXC",
     path => 'lxc',
@@ -233,6 +239,7 @@ __PACKAGE__->register_method ({
 	    { name => 'network' },
 	    { name => 'qemu' },
 	    { name => 'query-url-metadata' },
+	    { name => 'readovf' },
 	    { name => 'replication' },
 	    { name => 'report' },
 	    { name => 'rrd' }, # fixme: remove?
-- 
2.30.2

Re: [pve-devel] [PATCH v10 manager 1/1] api: nodes: add readovf endpoint

[Fabian Grünbichler]

missing a reason why this is added to the generic node index, and not 
somewhere under qemu?

On January 13, 2022 11:08 am, Fabian Ebner wrote:
> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
> [split into its own patch + add to index]
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Needs dependency bump for qemu-server.
> 
> Changes from v9:
>     * Add entry to /node/'s index.
> 
>  PVE/API2/Nodes.pm | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
> index d57a1937..5f6208d5 100644
> --- a/PVE/API2/Nodes.pm
> +++ b/PVE/API2/Nodes.pm
> @@ -49,6 +49,7 @@ use PVE::API2::LXC;
>  use PVE::API2::Network;
>  use PVE::API2::NodeConfig;
>  use PVE::API2::Qemu::CPU;
> +use PVE::API2::Qemu::OVF;
>  use PVE::API2::Qemu;
>  use PVE::API2::Replication;
>  use PVE::API2::Services;
> @@ -71,6 +72,11 @@ __PACKAGE__->register_method ({
>      path => 'qemu',
>  });
>  
> +__PACKAGE__->register_method ({
> +    subclass => "PVE::API2::Qemu::OVF",
> +    path => 'readovf',
> +});
> +
>  __PACKAGE__->register_method ({
>      subclass => "PVE::API2::LXC",
>      path => 'lxc',
> @@ -233,6 +239,7 @@ __PACKAGE__->register_method ({
>  	    { name => 'network' },
>  	    { name => 'qemu' },
>  	    { name => 'query-url-metadata' },
> +	    { name => 'readovf' },
>  	    { name => 'replication' },
>  	    { name => 'report' },
>  	    { name => 'rrd' }, # fixme: remove?
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 

Re: [pve-devel] [PATCH v10 manager 1/1] api: nodes: add readovf endpoint

[Fabian Ebner]

Am 17.01.22 um 16:38 schrieb Fabian Grünbichler:
> missing a reason why this is added to the generic node index, and not
> somewhere under qemu?
>

I suppose because having it under /nodes/{node}/qemu/ would imply having 
it under /nodes/{node}/qemu/{vmid}/ (as we don't support having both 
path/{regex} and path/static at the same time), but it doesn't depend on 
any vmid?


> On January 13, 2022 11:08 am, Fabian Ebner wrote:
>> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
>> [split into its own patch + add to index]
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>>
>> Needs dependency bump for qemu-server.
>>
>> Changes from v9:
>>      * Add entry to /node/'s index.
>>
>>   PVE/API2/Nodes.pm | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
>> index d57a1937..5f6208d5 100644
>> --- a/PVE/API2/Nodes.pm
>> +++ b/PVE/API2/Nodes.pm
>> @@ -49,6 +49,7 @@ use PVE::API2::LXC;
>>   use PVE::API2::Network;
>>   use PVE::API2::NodeConfig;
>>   use PVE::API2::Qemu::CPU;
>> +use PVE::API2::Qemu::OVF;
>>   use PVE::API2::Qemu;
>>   use PVE::API2::Replication;
>>   use PVE::API2::Services;
>> @@ -71,6 +72,11 @@ __PACKAGE__->register_method ({
>>       path => 'qemu',
>>   });
>>   
>> +__PACKAGE__->register_method ({
>> +    subclass => "PVE::API2::Qemu::OVF",
>> +    path => 'readovf',
>> +});
>> +
>>   __PACKAGE__->register_method ({
>>       subclass => "PVE::API2::LXC",
>>       path => 'lxc',
>> @@ -233,6 +239,7 @@ __PACKAGE__->register_method ({
>>   	    { name => 'network' },
>>   	    { name => 'qemu' },
>>   	    { name => 'query-url-metadata' },
>> +	    { name => 'readovf' },
>>   	    { name => 'replication' },
>>   	    { name => 'report' },
>>   	    { name => 'rrd' }, # fixme: remove?
>> -- 
>> 2.30.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

Re: [pve-devel] [PATCH v10 manager 1/1] api: nodes: add readovf endpoint

[Fabian Grünbichler]

On January 18, 2022 9:35 am, Fabian Ebner wrote:
> Am 17.01.22 um 16:38 schrieb Fabian Grünbichler:
>> missing a reason why this is added to the generic node index, and not
>> somewhere under qemu?
>>
> 
> I suppose because having it under /nodes/{node}/qemu/ would imply having 
> it under /nodes/{node}/qemu/{vmid}/ (as we don't support having both 
> path/{regex} and path/static at the same time), but it doesn't depend on 
> any vmid?

ack, that makes sense, and now that you mention it I remember having 
this exact discussion back when the CPU model stuff was introduced ;)

> 
> 
>> On January 13, 2022 11:08 am, Fabian Ebner wrote:
>>> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
>>> [split into its own patch + add to index]
>>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>>> ---
>>>
>>> Needs dependency bump for qemu-server.
>>>
>>> Changes from v9:
>>>      * Add entry to /node/'s index.
>>>
>>>   PVE/API2/Nodes.pm | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
>>> index d57a1937..5f6208d5 100644
>>> --- a/PVE/API2/Nodes.pm
>>> +++ b/PVE/API2/Nodes.pm
>>> @@ -49,6 +49,7 @@ use PVE::API2::LXC;
>>>   use PVE::API2::Network;
>>>   use PVE::API2::NodeConfig;
>>>   use PVE::API2::Qemu::CPU;
>>> +use PVE::API2::Qemu::OVF;
>>>   use PVE::API2::Qemu;
>>>   use PVE::API2::Replication;
>>>   use PVE::API2::Services;
>>> @@ -71,6 +72,11 @@ __PACKAGE__->register_method ({
>>>       path => 'qemu',
>>>   });
>>>   
>>> +__PACKAGE__->register_method ({
>>> +    subclass => "PVE::API2::Qemu::OVF",
>>> +    path => 'readovf',
>>> +});
>>> +
>>>   __PACKAGE__->register_method ({
>>>       subclass => "PVE::API2::LXC",
>>>       path => 'lxc',
>>> @@ -233,6 +239,7 @@ __PACKAGE__->register_method ({
>>>   	    { name => 'network' },
>>>   	    { name => 'qemu' },
>>>   	    { name => 'query-url-metadata' },
>>> +	    { name => 'readovf' },
>>>   	    { name => 'replication' },
>>>   	    { name => 'report' },
>>>   	    { name => 'rrd' }, # fixme: remove?
>>> -- 
>>> 2.30.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
> 

[pve-devel] [PATCH v10 qemu-server 4/7] image convert: allow block device as source

[Fabian Ebner]

Necessary to import from an existing storage using block-device
volumes like ZFS.

Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
[split into its own patch]
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

No changes from v9 (except splitting the patch).

 PVE/QemuServer.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 98eb6b3..c18e1f6 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -7296,7 +7296,7 @@ sub qemu_img_convert {
 	$src_path = PVE::Storage::path($storecfg, $src_volid, $snapname);
 	$src_is_iscsi = ($src_path =~ m|^iscsi://|);
 	$cachemode = 'none' if $src_scfg->{type} eq 'zfspool';
-    } elsif (-f $src_volid) {
+    } elsif (-f $src_volid || -b $src_volid) {
 	$src_path = $src_volid;
 	if ($src_path =~ m/\.($PVE::QemuServer::Drive::QEMU_FORMAT_RE)$/) {
 	    $src_format = $1;
-- 
2.30.2

[pve-devel] [RFC v10 qemu-server 5/7] schema: drive: use separate schema when disk allocation is possible

[Fabian Ebner]

via the special syntax <storeid>:<size>.

Not worth it by itself, but this is anticipating a new 'import-from'
parameter which is only used upon import/allocation, but shouldn't be
part of the schema for the config or other API enpoints.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v10.

 PVE/API2/Qemu.pm        | 12 ++++++--
 PVE/QemuServer.pm       |  9 ++++--
 PVE/QemuServer/Drive.pm | 62 +++++++++++++++++++++++++++++------------
 3 files changed, 60 insertions(+), 23 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 6992f6f..e6a6cdc 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -570,7 +570,9 @@ __PACKAGE__->register_method({
 		    default => 0,
 		    description => "Start VM after it was created successfully.",
 		},
-	    }),
+	    },
+	    1, # with_disk_alloc
+	),
     },
     returns => {
 	type => 'string',
@@ -1545,7 +1547,9 @@ __PACKAGE__->register_method({
 		    maximum => 30,
 		    optional => 1,
 		},
-	    }),
+	    },
+	    1, # with_disk_alloc
+	),
     },
     returns => {
 	type => 'string',
@@ -1593,7 +1597,9 @@ __PACKAGE__->register_method({
 		    maxLength => 40,
 		    optional => 1,
 		},
-	    }),
+	    },
+	    1, # with_disk_alloc
+	),
     },
     returns => { type => 'null' },
     code => sub {
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index c18e1f6..f880f32 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2202,7 +2202,7 @@ sub verify_usb_device {
 
 # add JSON properties for create and set function
 sub json_config_properties {
-    my $prop = shift;
+    my ($prop, $with_disk_alloc) = @_;
 
     my $skip_json_config_opts = {
 	parent => 1,
@@ -2215,7 +2215,12 @@ sub json_config_properties {
 
     foreach my $opt (keys %$confdesc) {
 	next if $skip_json_config_opts->{$opt};
-	$prop->{$opt} = $confdesc->{$opt};
+
+	if ($with_disk_alloc && is_valid_drivename($opt)) {
+	    $prop->{$opt} = $PVE::QemuServer::Drive::drivedesc_hash_with_alloc->{$opt};
+	} else {
+	    $prop->{$opt} = $confdesc->{$opt};
+	}
     }
 
     return $prop;
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index 7b82fb2..f024269 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -3,6 +3,8 @@ package PVE::QemuServer::Drive;
 use strict;
 use warnings;
 
+use Storable qw(dclone);
+
 use PVE::Storage;
 use PVE::JSONSchema qw(get_standard_option);
 
@@ -33,6 +35,8 @@ our $MAX_SATA_DISKS = 6;
 our $MAX_UNUSED_DISKS = 256;
 
 our $drivedesc_hash;
+# Schema when disk allocation is possible.
+our $drivedesc_hash_with_alloc = {};
 
 my %drivedesc_base = (
     volume => { alias => 'file' },
@@ -262,14 +266,10 @@ my $ide_fmt = {
 };
 PVE::JSONSchema::register_format("pve-qm-ide", $ide_fmt);
 
-my $ALLOCATION_SYNTAX_DESC =
-    "Use the special syntax STORAGE_ID:SIZE_IN_GiB to allocate a new volume.";
-
 my $idedesc = {
     optional => 1,
     type => 'string', format => $ide_fmt,
-    description => "Use volume as IDE hard disk or CD-ROM (n is 0 to " .($MAX_IDE_DISKS -1) . "). " .
-	$ALLOCATION_SYNTAX_DESC,
+    description => "Use volume as IDE hard disk or CD-ROM (n is 0 to " .($MAX_IDE_DISKS - 1) . ").",
 };
 PVE::JSONSchema::register_standard_option("pve-qm-ide", $idedesc);
 
@@ -285,8 +285,7 @@ my $scsi_fmt = {
 my $scsidesc = {
     optional => 1,
     type => 'string', format => $scsi_fmt,
-    description => "Use volume as SCSI hard disk or CD-ROM (n is 0 to " . ($MAX_SCSI_DISKS - 1) . "). " .
-	$ALLOCATION_SYNTAX_DESC,
+    description => "Use volume as SCSI hard disk or CD-ROM (n is 0 to " . ($MAX_SCSI_DISKS - 1) . ").",
 };
 PVE::JSONSchema::register_standard_option("pve-qm-scsi", $scsidesc);
 
@@ -298,8 +297,7 @@ my $sata_fmt = {
 my $satadesc = {
     optional => 1,
     type => 'string', format => $sata_fmt,
-    description => "Use volume as SATA hard disk or CD-ROM (n is 0 to " . ($MAX_SATA_DISKS - 1). "). " .
-	$ALLOCATION_SYNTAX_DESC,
+    description => "Use volume as SATA hard disk or CD-ROM (n is 0 to " . ($MAX_SATA_DISKS - 1). ").",
 };
 PVE::JSONSchema::register_standard_option("pve-qm-sata", $satadesc);
 
@@ -311,8 +309,7 @@ my $virtio_fmt = {
 my $virtiodesc = {
     optional => 1,
     type => 'string', format => $virtio_fmt,
-    description => "Use volume as VIRTIO hard disk (n is 0 to " . ($MAX_VIRTIO_DISKS - 1) . "). " .
-	$ALLOCATION_SYNTAX_DESC,
+    description => "Use volume as VIRTIO hard disk (n is 0 to " . ($MAX_VIRTIO_DISKS - 1) . ").",
 };
 PVE::JSONSchema::register_standard_option("pve-qm-virtio", $virtiodesc);
 
@@ -359,9 +356,7 @@ my $efidisk_fmt = {
 my $efidisk_desc = {
     optional => 1,
     type => 'string', format => $efidisk_fmt,
-    description => "Configure a Disk for storing EFI vars. " .
-	$ALLOCATION_SYNTAX_DESC . " Note that SIZE_IN_GiB is ignored here " .
-	"and that the default EFI vars are copied to the volume instead.",
+    description => "Configure a Disk for storing EFI vars.",
 };
 
 PVE::JSONSchema::register_standard_option("pve-qm-efidisk", $efidisk_desc);
@@ -397,10 +392,7 @@ my $tpmstate_fmt = {
 my $tpmstate_desc = {
     optional => 1,
     type => 'string', format => $tpmstate_fmt,
-    description => "Configure a Disk for storing TPM state. " .
-	$ALLOCATION_SYNTAX_DESC . " Note that SIZE_IN_GiB is ignored here " .
-	"and that the default size of 4 MiB will always be used instead. The " .
-	"format is also fixed to 'raw'.",
+    description => "Configure a Disk for storing TPM state. The format is fixed to 'raw'.",
 };
 use constant TPMSTATE_DISK_SIZE => 4 * 1024 * 1024;
 
@@ -434,27 +426,61 @@ my $unuseddesc = {
     description => "Reference to unused volumes. This is used internally, and should not be modified manually.",
 };
 
+my $with_alloc_desc_cache = {
+    unused => $unuseddesc, # Allocation for unused is not supported currently.
+};
+my $desc_with_alloc = sub {
+    my ($type, $desc) = @_;
+
+    return $with_alloc_desc_cache->{$type} if $with_alloc_desc_cache->{$type};
+
+    my $new_desc = dclone($desc);
+
+    my $extra_note = '';
+    if ($type eq 'efidisk') {
+	$extra_note = " Note that SIZE_IN_GiB is ignored here and that the default EFI vars are ".
+	    "copied to the volume instead.";
+    } elsif ($type eq 'tpmstate') {
+	$extra_note = " Note that SIZE_IN_GiB is ignored here and 4 MiB will be used instead.";
+    }
+
+    $new_desc->{description} .= "Use the special syntax STORAGE_ID:SIZE_IN_GiB to allocate a new ".
+	"volume.${extra_note}";
+
+    $with_alloc_desc_cache->{$type} = $new_desc;
+
+    return $new_desc;
+};
+
 for (my $i = 0; $i < $MAX_IDE_DISKS; $i++)  {
     $drivedesc_hash->{"ide$i"} = $idedesc;
+    $drivedesc_hash_with_alloc->{"ide$i"} = $desc_with_alloc->('ide', $idedesc);
 }
 
 for (my $i = 0; $i < $MAX_SATA_DISKS; $i++)  {
     $drivedesc_hash->{"sata$i"} = $satadesc;
+    $drivedesc_hash_with_alloc->{"sata$i"} = $desc_with_alloc->('sata', $satadesc);
 }
 
 for (my $i = 0; $i < $MAX_SCSI_DISKS; $i++)  {
     $drivedesc_hash->{"scsi$i"} = $scsidesc;
+    $drivedesc_hash_with_alloc->{"scsi$i"} = $desc_with_alloc->('scsi', $scsidesc);
 }
 
 for (my $i = 0; $i < $MAX_VIRTIO_DISKS; $i++)  {
     $drivedesc_hash->{"virtio$i"} = $virtiodesc;
+    $drivedesc_hash_with_alloc->{"virtio$i"} = $desc_with_alloc->('virtio', $virtiodesc);
 }
 
 $drivedesc_hash->{efidisk0} = $efidisk_desc;
+$drivedesc_hash_with_alloc->{efidisk0} = $desc_with_alloc->('efidisk', $efidisk_desc);
+
 $drivedesc_hash->{tpmstate0} = $tpmstate_desc;
+$drivedesc_hash_with_alloc->{tpmstate0} = $desc_with_alloc->('tpmstate', $tpmstate_desc);
 
 for (my $i = 0; $i < $MAX_UNUSED_DISKS; $i++) {
     $drivedesc_hash->{"unused$i"} = $unuseddesc;
+    $drivedesc_hash_with_alloc->{"unused$i"} = $desc_with_alloc->('unused', $unuseddesc);
 }
 
 sub valid_drive_names_for_boot {
-- 
2.30.2

[pve-devel] [RFC v10 qemu-server 6/7] api: support VM disk import

[Fabian Ebner]

From: Dominic Jäger <d.jaeger@proxmox.com>

Extend qm importdisk functionality to the API.

Co-authored-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Co-authored-by: Dominic Jäger <d.jaeger@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Changes from v9:

* Instead of adding an import-sources parameter to the API, use a new
  import-from property for the disk, that's only available with
  import/alloc-enabled API endpoints via its own version of the schema

    Avoids the split across regular drive key parameters and
    'import_soruces', which avoids quite a bit of cross-checking
    between the two and parsing/passing around the latter.

    The big downsides are:
    * Schema handling is a bit messy.
    * Need to special case print_drive, because we do intermediate
      parse/print to cleanup drive paths. Seems not too easy to avoid
      without complicating things elsewehere.
    * Using the import-aware parse_drive in parse_volume, because that
      is used via the foreach_volume iterators handling the parameters
      of the import-enabled endpoints. Could be avoided by using for
      loops instead.

    Counter-arguments for using a single schema (citing Fabian G.):
    * docs/schema dump/api docs: shouldn't look like you can put that
      everywhere where we use the config schema
    * shouldn't have nasty side-effects if someone puts it into the
      config

* Don't iterate over unused disks in create_disks()

    Would need to be its own patch and need to make sure everything
    also works with respect to usual (i.e. non-import) new disk
    creation, etc.

* Re-use do_import function

    Rather than duplicating most of it. The down side is the need to
    add a new parameter for skipping configuration update. But I
    suppose the plan is to have qm import switch to the new API at
    some point, and then do_import can be simplified.

* Drop format supported check

    Instead rely on resolve_dst_disk_format (via do_import) to pick
    the most appropriate format.

 PVE/API2/Qemu.pm             | 86 +++++++++++++++++++++++++-----------
 PVE/QemuConfig.pm            |  2 +-
 PVE/QemuServer/Drive.pm      | 32 +++++++++++---
 PVE/QemuServer/ImportDisk.pm |  2 +-
 4 files changed, 87 insertions(+), 35 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index e6a6cdc..8c74ecc 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -21,8 +21,9 @@ use PVE::ReplicationConfig;
 use PVE::GuestHelpers;
 use PVE::QemuConfig;
 use PVE::QemuServer;
-use PVE::QemuServer::Drive;
 use PVE::QemuServer::CPUConfig;
+use PVE::QemuServer::Drive;
+use PVE::QemuServer::ImportDisk;
 use PVE::QemuServer::Monitor qw(mon_cmd);
 use PVE::QemuServer::Machine;
 use PVE::QemuMigrate;
@@ -89,6 +90,10 @@ my $check_storage_access = sub {
 	} else {
 	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
 	}
+
+	if (my $source_image = $drive->{'import-from'}) {
+	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $source_image);
+	}
     });
 
    $rpcenv->check($authuser, "/storage/$settings->{vmstatestorage}", ['Datastore.AllocateSpace'])
@@ -162,6 +167,9 @@ my $create_disks = sub {
 	my $volid = $disk->{file};
 	my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
 
+	die "'import-from' requires special volume ID - use <storage ID>:0,import-from=<source>\n"
+	    if $disk->{'import-from'} && $volid !~ $NEW_DISK_RE;
+
 	if (!$volid || $volid eq 'none' || $volid eq 'cdrom') {
 	    delete $disk->{size};
 	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
@@ -190,28 +198,52 @@ my $create_disks = sub {
 	} elsif ($volid =~ $NEW_DISK_RE) {
 	    my ($storeid, $size) = ($2 || $default_storage, $3);
 	    die "no storage ID specified (and no default storage)\n" if !$storeid;
-	    my $defformat = PVE::Storage::storage_default_format($storecfg, $storeid);
-	    my $fmt = $disk->{format} || $defformat;
-
-	    $size = PVE::Tools::convert_size($size, 'gb' => 'kb'); # vdisk_alloc uses kb
-
-	    my $volid;
-	    if ($ds eq 'efidisk0') {
-		my $smm = PVE::QemuServer::Machine::machine_type_is_q35($conf);
-		($volid, $size) = PVE::QemuServer::create_efidisk(
-		    $storecfg, $storeid, $vmid, $fmt, $arch, $disk, $smm);
-	    } elsif ($ds eq 'tpmstate0') {
-		# swtpm can only use raw volumes, and uses a fixed size
-		$size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb');
-		$volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, "raw", undef, $size);
+
+	    if (my $source = delete $disk->{'import-from'}) {
+		$source = PVE::Storage::abs_filesystem_path($storecfg, $source, 1);
+		my $src_size = PVE::Storage::file_size_info($source);
+		die "Could not get file size of $source" if !defined($src_size);
+
+		my (undef, $dst_volid) = PVE::QemuServer::ImportDisk::do_import(
+		    $source,
+		    $vmid,
+		    $storeid,
+		    {
+			drive_name => $ds,
+			format => $disk->{format},
+			'skip-config-update' => 1,
+		    },
+		);
+
+		push @$vollist, $dst_volid;
+		$disk->{file} = $dst_volid;
+		$disk->{size} = $src_size;
+		delete $disk->{format}; # no longer needed
+		$res->{$ds} = PVE::QemuServer::print_drive($disk);
 	    } else {
-		$volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size);
+		my $defformat = PVE::Storage::storage_default_format($storecfg, $storeid);
+		my $fmt = $disk->{format} || $defformat;
+
+		$size = PVE::Tools::convert_size($size, 'gb' => 'kb'); # vdisk_alloc uses kb
+
+		my $volid;
+		if ($ds eq 'efidisk0') {
+		    my $smm = PVE::QemuServer::Machine::machine_type_is_q35($conf);
+		    ($volid, $size) = PVE::QemuServer::create_efidisk(
+			$storecfg, $storeid, $vmid, $fmt, $arch, $disk, $smm);
+		} elsif ($ds eq 'tpmstate0') {
+		    # swtpm can only use raw volumes, and uses a fixed size
+		    $size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb');
+		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, "raw", undef, $size);
+		} else {
+		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size);
+		}
+		push @$vollist, $volid;
+		$disk->{file} = $volid;
+		$disk->{size} = PVE::Tools::convert_size($size, 'kb' => 'b');
+		delete $disk->{format}; # no longer needed
+		$res->{$ds} = PVE::QemuServer::print_drive($disk);
 	    }
-	    push @$vollist, $volid;
-	    $disk->{file} = $volid;
-	    $disk->{size} = PVE::Tools::convert_size($size, 'kb' => 'b');
-	    delete $disk->{format}; # no longer needed
-	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
 	} else {
 
 	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
@@ -639,11 +671,11 @@ __PACKAGE__->register_method({
 
 	    foreach my $opt (keys %$param) {
 		if (PVE::QemuServer::is_valid_drivename($opt)) {
-		    my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
+		    my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
 		    raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive;
 
 		    PVE::QemuServer::cleanup_drive_path($opt, $storecfg, $drive);
-		    $param->{$opt} = PVE::QemuServer::print_drive($drive);
+		    $param->{$opt} = PVE::QemuServer::print_drive($drive, 1);
 		}
 	    }
 
@@ -1207,11 +1239,11 @@ my $update_vm_api  = sub {
     foreach my $opt (keys %$param) {
 	if (PVE::QemuServer::is_valid_drivename($opt)) {
 	    # cleanup drive path
-	    my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
+	    my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
 	    raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive;
 	    PVE::QemuServer::cleanup_drive_path($opt, $storecfg, $drive);
 	    $check_replication->($drive);
-	    $param->{$opt} = PVE::QemuServer::print_drive($drive);
+	    $param->{$opt} = PVE::QemuServer::print_drive($drive, 1);
 	} elsif ($opt =~ m/^net(\d+)$/) {
 	    # add macaddr
 	    my $net = PVE::QemuServer::parse_net($param->{$opt});
@@ -1288,7 +1320,7 @@ my $update_vm_api  = sub {
 
 	    my $check_drive_perms = sub {
 		my ($opt, $val) = @_;
-		my $drive = PVE::QemuServer::parse_drive($opt, $val);
+		my $drive = PVE::QemuServer::parse_drive($opt, $val, 1);
 		# FIXME: cloudinit: CDROM or Disk?
 		if (PVE::QemuServer::drive_is_cdrom($drive)) { # CDROM
 		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM']);
@@ -1384,7 +1416,7 @@ my $update_vm_api  = sub {
 		    # default legacy boot order implies all cdroms anyway
 		    if (@bootorder) {
 			# append new CD drives to bootorder to mark them bootable
-			my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
+			my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
 			if (PVE::QemuServer::drive_is_cdrom($drive, 1) && !grep(/^$opt$/, @bootorder)) {
 			    push @bootorder, $opt;
 			    $conf->{pending}->{boot} = PVE::QemuServer::print_bootorder(\@bootorder);
diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index b993378..76b4314 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -101,7 +101,7 @@ sub parse_volume {
 	}
 	$volume = { 'file' => $volume_string };
     } else {
-	$volume = PVE::QemuServer::Drive::parse_drive($key, $volume_string);
+	$volume = PVE::QemuServer::Drive::parse_drive($key, $volume_string, 1);
     }
 
     die "unable to parse volume\n" if !defined($volume) && !$noerr;
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index f024269..828076d 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -409,6 +409,20 @@ my $alldrive_fmt = {
     %efitype_fmt,
 };
 
+my %import_from_fmt = (
+    'import-from' => {
+	type => 'string',
+	format => 'pve-volume-id-or-absolute-path',
+	format_description => 'source volume',
+	description => "When creating a new disk, import from this source.",
+	optional => 1,
+    },
+);
+my $alldrive_fmt_with_alloc = {
+    %$alldrive_fmt,
+    %import_from_fmt,
+};
+
 my $unused_fmt = {
     volume => { alias => 'file' },
     file => {
@@ -436,6 +450,8 @@ my $desc_with_alloc = sub {
 
     my $new_desc = dclone($desc);
 
+    $new_desc->{format}->{'import-from'} = $import_from_fmt{'import-from'};
+
     my $extra_note = '';
     if ($type eq 'efidisk') {
 	$extra_note = " Note that SIZE_IN_GiB is ignored here and that the default EFI vars are ".
@@ -445,7 +461,8 @@ my $desc_with_alloc = sub {
     }
 
     $new_desc->{description} .= "Use the special syntax STORAGE_ID:SIZE_IN_GiB to allocate a new ".
-	"volume.${extra_note}";
+	"volume.${extra_note} Use the 'import-from' parameter to import from an existing volume ".
+	"instead (SIZE_IN_GiB is ignored then).";
 
     $with_alloc_desc_cache->{$type} = $new_desc;
 
@@ -547,7 +564,7 @@ sub drive_is_read_only {
 #        [,iothread=on][,serial=serial][,model=model]
 
 sub parse_drive {
-    my ($key, $data) = @_;
+    my ($key, $data, $with_alloc) = @_;
 
     my ($interface, $index);
 
@@ -558,12 +575,14 @@ sub parse_drive {
 	return;
     }
 
-    if (!defined($drivedesc_hash->{$key})) {
+    my $desc_hash = $with_alloc ? $drivedesc_hash_with_alloc : $drivedesc_hash;
+
+    if (!defined($desc_hash->{$key})) {
 	warn "invalid drive key: $key\n";
 	return;
     }
 
-    my $desc = $drivedesc_hash->{$key}->{format};
+    my $desc = $desc_hash->{$key}->{format};
     my $res = eval { PVE::JSONSchema::parse_property_string($desc, $data) };
     return if !$res;
     $res->{interface} = $interface;
@@ -623,9 +642,10 @@ sub parse_drive {
 }
 
 sub print_drive {
-    my ($drive) = @_;
+    my ($drive, $with_alloc) = @_;
     my $skip = [ 'index', 'interface' ];
-    return PVE::JSONSchema::print_property_string($drive, $alldrive_fmt, $skip);
+    my $fmt = $with_alloc ? $alldrive_fmt_with_alloc : $alldrive_fmt;
+    return PVE::JSONSchema::print_property_string($drive, $fmt, $skip);
 }
 
 sub get_bootdisks {
diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
index 51ad52e..7557cac 100755
--- a/PVE/QemuServer/ImportDisk.pm
+++ b/PVE/QemuServer/ImportDisk.pm
@@ -71,7 +71,7 @@ sub do_import {
 	PVE::Storage::activate_volumes($storecfg, [$dst_volid]);
 	PVE::QemuServer::qemu_img_convert($src_path, $dst_volid, $src_size, undef, $zeroinit);
 	PVE::Storage::deactivate_volumes($storecfg, [$dst_volid]);
-	PVE::QemuConfig->lock_config($vmid, $create_drive);
+	PVE::QemuConfig->lock_config($vmid, $create_drive) if !$params->{'skip-config-update'};
     };
     if (my $err = $@) {
 	eval { PVE::Storage::vdisk_free($storecfg, $dst_volid) };
-- 
2.30.2

Re: [pve-devel] [RFC v10 qemu-server 6/7] api: support VM disk import

[Fabian Grünbichler]

On January 13, 2022 11:08 am, Fabian Ebner wrote:
> From: Dominic Jäger <d.jaeger@proxmox.com>
> 
> Extend qm importdisk functionality to the API.
> 
> Co-authored-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Co-authored-by: Dominic Jäger <d.jaeger@proxmox.com>
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Changes from v9:
> 
> * Instead of adding an import-sources parameter to the API, use a new
>   import-from property for the disk, that's only available with
>   import/alloc-enabled API endpoints via its own version of the schema
> 
>     Avoids the split across regular drive key parameters and
>     'import_soruces', which avoids quite a bit of cross-checking
>     between the two and parsing/passing around the latter.
> 
>     The big downsides are:
>     * Schema handling is a bit messy.
>     * Need to special case print_drive, because we do intermediate
>       parse/print to cleanup drive paths. Seems not too easy to avoid
>       without complicating things elsewehere.
>     * Using the import-aware parse_drive in parse_volume, because that
>       is used via the foreach_volume iterators handling the parameters
>       of the import-enabled endpoints. Could be avoided by using for
>       loops instead.
> 
>     Counter-arguments for using a single schema (citing Fabian G.):
>     * docs/schema dump/api docs: shouldn't look like you can put that
>       everywhere where we use the config schema
>     * shouldn't have nasty side-effects if someone puts it into the
>       config
> 
> * Don't iterate over unused disks in create_disks()
> 
>     Would need to be its own patch and need to make sure everything
>     also works with respect to usual (i.e. non-import) new disk
>     creation, etc.
> 
> * Re-use do_import function
> 
>     Rather than duplicating most of it. The down side is the need to
>     add a new parameter for skipping configuration update. But I
>     suppose the plan is to have qm import switch to the new API at
>     some point, and then do_import can be simplified.
> 
> * Drop format supported check
> 
>     Instead rely on resolve_dst_disk_format (via do_import) to pick
>     the most appropriate format.
> 
>  PVE/API2/Qemu.pm             | 86 +++++++++++++++++++++++++-----------
>  PVE/QemuConfig.pm            |  2 +-
>  PVE/QemuServer/Drive.pm      | 32 +++++++++++---
>  PVE/QemuServer/ImportDisk.pm |  2 +-
>  4 files changed, 87 insertions(+), 35 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index e6a6cdc..8c74ecc 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -21,8 +21,9 @@ use PVE::ReplicationConfig;
>  use PVE::GuestHelpers;
>  use PVE::QemuConfig;
>  use PVE::QemuServer;
> -use PVE::QemuServer::Drive;
>  use PVE::QemuServer::CPUConfig;
> +use PVE::QemuServer::Drive;
> +use PVE::QemuServer::ImportDisk;
>  use PVE::QemuServer::Monitor qw(mon_cmd);
>  use PVE::QemuServer::Machine;
>  use PVE::QemuMigrate;
> @@ -89,6 +90,10 @@ my $check_storage_access = sub {
>  	} else {
>  	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
>  	}
> +
> +	if (my $source_image = $drive->{'import-from'}) {
> +	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $source_image);
> +	}
>      });
>  
>     $rpcenv->check($authuser, "/storage/$settings->{vmstatestorage}", ['Datastore.AllocateSpace'])
> @@ -162,6 +167,9 @@ my $create_disks = sub {
>  	my $volid = $disk->{file};
>  	my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
>  
> +	die "'import-from' requires special volume ID - use <storage ID>:0,import-from=<source>\n"
> +	    if $disk->{'import-from'} && $volid !~ $NEW_DISK_RE;
> +

nit: check and message disagree ($NEW_DISK_RE allows more than just '0')

not sure whether it's worth it to add an $IMPORT_DISK_RE that is limited 
to 0? otherwise users might expect something like

-scsi0 STORAGE:128,import-from:/some/small/image.raw

to import and resize on the fly ;)

>  	if (!$volid || $volid eq 'none' || $volid eq 'cdrom') {
>  	    delete $disk->{size};
>  	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
> @@ -190,28 +198,52 @@ my $create_disks = sub {
>  	} elsif ($volid =~ $NEW_DISK_RE) {
>  	    my ($storeid, $size) = ($2 || $default_storage, $3);
>  	    die "no storage ID specified (and no default storage)\n" if !$storeid;
> -	    my $defformat = PVE::Storage::storage_default_format($storecfg, $storeid);
> -	    my $fmt = $disk->{format} || $defformat;
> -
> -	    $size = PVE::Tools::convert_size($size, 'gb' => 'kb'); # vdisk_alloc uses kb
> -
> -	    my $volid;
> -	    if ($ds eq 'efidisk0') {
> -		my $smm = PVE::QemuServer::Machine::machine_type_is_q35($conf);
> -		($volid, $size) = PVE::QemuServer::create_efidisk(
> -		    $storecfg, $storeid, $vmid, $fmt, $arch, $disk, $smm);
> -	    } elsif ($ds eq 'tpmstate0') {
> -		# swtpm can only use raw volumes, and uses a fixed size
> -		$size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb');
> -		$volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, "raw", undef, $size);
> +
> +	    if (my $source = delete $disk->{'import-from'}) {
> +		$source = PVE::Storage::abs_filesystem_path($storecfg, $source, 1);
> +		my $src_size = PVE::Storage::file_size_info($source);
> +		die "Could not get file size of $source" if !defined($src_size);

nit: missing '\n'?

> +
> +		my (undef, $dst_volid) = PVE::QemuServer::ImportDisk::do_import(
> +		    $source,
> +		    $vmid,
> +		    $storeid,
> +		    {
> +			drive_name => $ds,
> +			format => $disk->{format},
> +			'skip-config-update' => 1,
> +		    },
> +		);
> +
> +		push @$vollist, $dst_volid;
> +		$disk->{file} = $dst_volid;
> +		$disk->{size} = $src_size;
> +		delete $disk->{format}; # no longer needed
> +		$res->{$ds} = PVE::QemuServer::print_drive($disk);
>  	    } else {
> -		$volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size);
> +		my $defformat = PVE::Storage::storage_default_format($storecfg, $storeid);
> +		my $fmt = $disk->{format} || $defformat;
> +
> +		$size = PVE::Tools::convert_size($size, 'gb' => 'kb'); # vdisk_alloc uses kb
> +
> +		my $volid;
> +		if ($ds eq 'efidisk0') {
> +		    my $smm = PVE::QemuServer::Machine::machine_type_is_q35($conf);
> +		    ($volid, $size) = PVE::QemuServer::create_efidisk(
> +			$storecfg, $storeid, $vmid, $fmt, $arch, $disk, $smm);
> +		} elsif ($ds eq 'tpmstate0') {
> +		    # swtpm can only use raw volumes, and uses a fixed size
> +		    $size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb');
> +		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, "raw", undef, $size);
> +		} else {
> +		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size);
> +		}
> +		push @$vollist, $volid;
> +		$disk->{file} = $volid;
> +		$disk->{size} = PVE::Tools::convert_size($size, 'kb' => 'b');
> +		delete $disk->{format}; # no longer needed
> +		$res->{$ds} = PVE::QemuServer::print_drive($disk);
>  	    }
> -	    push @$vollist, $volid;
> -	    $disk->{file} = $volid;
> -	    $disk->{size} = PVE::Tools::convert_size($size, 'kb' => 'b');
> -	    delete $disk->{format}; # no longer needed
> -	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
>  	} else {
>  
>  	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
> @@ -639,11 +671,11 @@ __PACKAGE__->register_method({
>  
>  	    foreach my $opt (keys %$param) {
>  		if (PVE::QemuServer::is_valid_drivename($opt)) {
> -		    my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
> +		    my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
>  		    raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive;
>  
>  		    PVE::QemuServer::cleanup_drive_path($opt, $storecfg, $drive);
> -		    $param->{$opt} = PVE::QemuServer::print_drive($drive);
> +		    $param->{$opt} = PVE::QemuServer::print_drive($drive, 1);
>  		}
>  	    }
>  
> @@ -1207,11 +1239,11 @@ my $update_vm_api  = sub {
>      foreach my $opt (keys %$param) {
>  	if (PVE::QemuServer::is_valid_drivename($opt)) {
>  	    # cleanup drive path
> -	    my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
> +	    my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
>  	    raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive;
>  	    PVE::QemuServer::cleanup_drive_path($opt, $storecfg, $drive);
>  	    $check_replication->($drive);
> -	    $param->{$opt} = PVE::QemuServer::print_drive($drive);
> +	    $param->{$opt} = PVE::QemuServer::print_drive($drive, 1);
>  	} elsif ($opt =~ m/^net(\d+)$/) {
>  	    # add macaddr
>  	    my $net = PVE::QemuServer::parse_net($param->{$opt});
> @@ -1288,7 +1320,7 @@ my $update_vm_api  = sub {
>  
>  	    my $check_drive_perms = sub {
>  		my ($opt, $val) = @_;
> -		my $drive = PVE::QemuServer::parse_drive($opt, $val);
> +		my $drive = PVE::QemuServer::parse_drive($opt, $val, 1);
>  		# FIXME: cloudinit: CDROM or Disk?
>  		if (PVE::QemuServer::drive_is_cdrom($drive)) { # CDROM
>  		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM']);
> @@ -1384,7 +1416,7 @@ my $update_vm_api  = sub {
>  		    # default legacy boot order implies all cdroms anyway
>  		    if (@bootorder) {
>  			# append new CD drives to bootorder to mark them bootable
> -			my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
> +			my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
>  			if (PVE::QemuServer::drive_is_cdrom($drive, 1) && !grep(/^$opt$/, @bootorder)) {
>  			    push @bootorder, $opt;
>  			    $conf->{pending}->{boot} = PVE::QemuServer::print_bootorder(\@bootorder);
> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
> index b993378..76b4314 100644
> --- a/PVE/QemuConfig.pm
> +++ b/PVE/QemuConfig.pm
> @@ -101,7 +101,7 @@ sub parse_volume {
>  	}
>  	$volume = { 'file' => $volume_string };
>      } else {
> -	$volume = PVE::QemuServer::Drive::parse_drive($key, $volume_string);
> +	$volume = PVE::QemuServer::Drive::parse_drive($key, $volume_string, 1);
>      }
>  
>      die "unable to parse volume\n" if !defined($volume) && !$noerr;
> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
> index f024269..828076d 100644
> --- a/PVE/QemuServer/Drive.pm
> +++ b/PVE/QemuServer/Drive.pm
> @@ -409,6 +409,20 @@ my $alldrive_fmt = {
>      %efitype_fmt,
>  };
>  
> +my %import_from_fmt = (
> +    'import-from' => {
> +	type => 'string',
> +	format => 'pve-volume-id-or-absolute-path',
> +	format_description => 'source volume',
> +	description => "When creating a new disk, import from this source.",
> +	optional => 1,
> +    },
> +);
> +my $alldrive_fmt_with_alloc = {
> +    %$alldrive_fmt,
> +    %import_from_fmt,
> +};
> +
>  my $unused_fmt = {
>      volume => { alias => 'file' },
>      file => {
> @@ -436,6 +450,8 @@ my $desc_with_alloc = sub {
>  
>      my $new_desc = dclone($desc);
>  
> +    $new_desc->{format}->{'import-from'} = $import_from_fmt{'import-from'};
> +
>      my $extra_note = '';
>      if ($type eq 'efidisk') {
>  	$extra_note = " Note that SIZE_IN_GiB is ignored here and that the default EFI vars are ".
> @@ -445,7 +461,8 @@ my $desc_with_alloc = sub {
>      }
>  
>      $new_desc->{description} .= "Use the special syntax STORAGE_ID:SIZE_IN_GiB to allocate a new ".
> -	"volume.${extra_note}";
> +	"volume.${extra_note} Use the 'import-from' parameter to import from an existing volume ".
> +	"instead (SIZE_IN_GiB is ignored then).";

or change the check above, and make this

+	"volume.${extra_note} Use STORAGE_ID:0 and the 'import-from' parameter to import from an existing volume.";

>  
>      $with_alloc_desc_cache->{$type} = $new_desc;
>  
> @@ -547,7 +564,7 @@ sub drive_is_read_only {
>  #        [,iothread=on][,serial=serial][,model=model]
>  
>  sub parse_drive {
> -    my ($key, $data) = @_;
> +    my ($key, $data, $with_alloc) = @_;
>  
>      my ($interface, $index);
>  
> @@ -558,12 +575,14 @@ sub parse_drive {
>  	return;
>      }
>  
> -    if (!defined($drivedesc_hash->{$key})) {
> +    my $desc_hash = $with_alloc ? $drivedesc_hash_with_alloc : $drivedesc_hash;
> +
> +    if (!defined($desc_hash->{$key})) {
>  	warn "invalid drive key: $key\n";
>  	return;
>      }
>  
> -    my $desc = $drivedesc_hash->{$key}->{format};
> +    my $desc = $desc_hash->{$key}->{format};
>      my $res = eval { PVE::JSONSchema::parse_property_string($desc, $data) };
>      return if !$res;
>      $res->{interface} = $interface;
> @@ -623,9 +642,10 @@ sub parse_drive {
>  }
>  
>  sub print_drive {
> -    my ($drive) = @_;
> +    my ($drive, $with_alloc) = @_;
>      my $skip = [ 'index', 'interface' ];
> -    return PVE::JSONSchema::print_property_string($drive, $alldrive_fmt, $skip);
> +    my $fmt = $with_alloc ? $alldrive_fmt_with_alloc : $alldrive_fmt;
> +    return PVE::JSONSchema::print_property_string($drive, $fmt, $skip);
>  }
>  
>  sub get_bootdisks {
> diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
> index 51ad52e..7557cac 100755
> --- a/PVE/QemuServer/ImportDisk.pm
> +++ b/PVE/QemuServer/ImportDisk.pm
> @@ -71,7 +71,7 @@ sub do_import {
>  	PVE::Storage::activate_volumes($storecfg, [$dst_volid]);
>  	PVE::QemuServer::qemu_img_convert($src_path, $dst_volid, $src_size, undef, $zeroinit);
>  	PVE::Storage::deactivate_volumes($storecfg, [$dst_volid]);
> -	PVE::QemuConfig->lock_config($vmid, $create_drive);
> +	PVE::QemuConfig->lock_config($vmid, $create_drive) if !$params->{'skip-config-update'};
>      };
>      if (my $err = $@) {
>  	eval { PVE::Storage::vdisk_free($storecfg, $dst_volid) };
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 

Re: [pve-devel] [RFC v10 qemu-server 6/7] api: support VM disk import

[Fabian Ebner]

Am 17.01.22 um 16:39 schrieb Fabian Grünbichler:
> On January 13, 2022 11:08 am, Fabian Ebner wrote:
>> From: Dominic Jäger <d.jaeger@proxmox.com>
>>
>> Extend qm importdisk functionality to the API.
>>
>> Co-authored-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> Co-authored-by: Dominic Jäger <d.jaeger@proxmox.com>
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>>
>> Changes from v9:
>>
>> * Instead of adding an import-sources parameter to the API, use a new
>>    import-from property for the disk, that's only available with
>>    import/alloc-enabled API endpoints via its own version of the schema
>>
>>      Avoids the split across regular drive key parameters and
>>      'import_soruces', which avoids quite a bit of cross-checking
>>      between the two and parsing/passing around the latter.
>>
>>      The big downsides are:
>>      * Schema handling is a bit messy.
>>      * Need to special case print_drive, because we do intermediate
>>        parse/print to cleanup drive paths. Seems not too easy to avoid
>>        without complicating things elsewehere.
>>      * Using the import-aware parse_drive in parse_volume, because that
>>        is used via the foreach_volume iterators handling the parameters
>>        of the import-enabled endpoints. Could be avoided by using for
>>        loops instead.
>>
>>      Counter-arguments for using a single schema (citing Fabian G.):
>>      * docs/schema dump/api docs: shouldn't look like you can put that
>>        everywhere where we use the config schema
>>      * shouldn't have nasty side-effects if someone puts it into the
>>        config
>>
>> * Don't iterate over unused disks in create_disks()
>>
>>      Would need to be its own patch and need to make sure everything
>>      also works with respect to usual (i.e. non-import) new disk
>>      creation, etc.
>>
>> * Re-use do_import function
>>
>>      Rather than duplicating most of it. The down side is the need to
>>      add a new parameter for skipping configuration update. But I
>>      suppose the plan is to have qm import switch to the new API at
>>      some point, and then do_import can be simplified.
>>
>> * Drop format supported check
>>
>>      Instead rely on resolve_dst_disk_format (via do_import) to pick
>>      the most appropriate format.
>>
>>   PVE/API2/Qemu.pm             | 86 +++++++++++++++++++++++++-----------
>>   PVE/QemuConfig.pm            |  2 +-
>>   PVE/QemuServer/Drive.pm      | 32 +++++++++++---
>>   PVE/QemuServer/ImportDisk.pm |  2 +-
>>   4 files changed, 87 insertions(+), 35 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index e6a6cdc..8c74ecc 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -21,8 +21,9 @@ use PVE::ReplicationConfig;
>>   use PVE::GuestHelpers;
>>   use PVE::QemuConfig;
>>   use PVE::QemuServer;
>> -use PVE::QemuServer::Drive;
>>   use PVE::QemuServer::CPUConfig;
>> +use PVE::QemuServer::Drive;
>> +use PVE::QemuServer::ImportDisk;
>>   use PVE::QemuServer::Monitor qw(mon_cmd);
>>   use PVE::QemuServer::Machine;
>>   use PVE::QemuMigrate;
>> @@ -89,6 +90,10 @@ my $check_storage_access = sub {
>>   	} else {
>>   	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
>>   	}
>> +
>> +	if (my $source_image = $drive->{'import-from'}) {
>> +	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $source_image);
>> +	}
>>       });
>>   
>>      $rpcenv->check($authuser, "/storage/$settings->{vmstatestorage}", ['Datastore.AllocateSpace'])
>> @@ -162,6 +167,9 @@ my $create_disks = sub {
>>   	my $volid = $disk->{file};
>>   	my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
>>   
>> +	die "'import-from' requires special volume ID - use <storage ID>:0,import-from=<source>\n"
>> +	    if $disk->{'import-from'} && $volid !~ $NEW_DISK_RE;
>> +
> 
> nit: check and message disagree ($NEW_DISK_RE allows more than just '0')
> 
> not sure whether it's worth it to add an $IMPORT_DISK_RE that is limited
> to 0? otherwise users might expect something like
> 
> -scsi0 STORAGE:128,import-from:/some/small/image.raw
> 
> to import and resize on the fly ;)

The error just gives an example, but I agree that's not clear without 
any "e.g." in there ;) And by allowing (and ignoring) more patterns now, 
adding the "resize on the fly" feature becomes much more difficult in 
the future (should we ever want that). I'll make the check more strict 
(just adding a check for size 0 avoids the need for a second RE) and 
adapt the description below as suggested.

> 
>>   	if (!$volid || $volid eq 'none' || $volid eq 'cdrom') {
>>   	    delete $disk->{size};
>>   	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
>> @@ -190,28 +198,52 @@ my $create_disks = sub {
>>   	} elsif ($volid =~ $NEW_DISK_RE) {
>>   	    my ($storeid, $size) = ($2 || $default_storage, $3);
>>   	    die "no storage ID specified (and no default storage)\n" if !$storeid;
>> -	    my $defformat = PVE::Storage::storage_default_format($storecfg, $storeid);
>> -	    my $fmt = $disk->{format} || $defformat;
>> -
>> -	    $size = PVE::Tools::convert_size($size, 'gb' => 'kb'); # vdisk_alloc uses kb
>> -
>> -	    my $volid;
>> -	    if ($ds eq 'efidisk0') {
>> -		my $smm = PVE::QemuServer::Machine::machine_type_is_q35($conf);
>> -		($volid, $size) = PVE::QemuServer::create_efidisk(
>> -		    $storecfg, $storeid, $vmid, $fmt, $arch, $disk, $smm);
>> -	    } elsif ($ds eq 'tpmstate0') {
>> -		# swtpm can only use raw volumes, and uses a fixed size
>> -		$size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb');
>> -		$volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, "raw", undef, $size);
>> +
>> +	    if (my $source = delete $disk->{'import-from'}) {
>> +		$source = PVE::Storage::abs_filesystem_path($storecfg, $source, 1);
>> +		my $src_size = PVE::Storage::file_size_info($source);
>> +		die "Could not get file size of $source" if !defined($src_size);
> 
> nit: missing '\n'?
> 
>> +
>> +		my (undef, $dst_volid) = PVE::QemuServer::ImportDisk::do_import(
>> +		    $source,
>> +		    $vmid,
>> +		    $storeid,
>> +		    {
>> +			drive_name => $ds,
>> +			format => $disk->{format},
>> +			'skip-config-update' => 1,
>> +		    },
>> +		);
>> +
>> +		push @$vollist, $dst_volid;
>> +		$disk->{file} = $dst_volid;
>> +		$disk->{size} = $src_size;
>> +		delete $disk->{format}; # no longer needed
>> +		$res->{$ds} = PVE::QemuServer::print_drive($disk);
>>   	    } else {
>> -		$volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size);
>> +		my $defformat = PVE::Storage::storage_default_format($storecfg, $storeid);
>> +		my $fmt = $disk->{format} || $defformat;
>> +
>> +		$size = PVE::Tools::convert_size($size, 'gb' => 'kb'); # vdisk_alloc uses kb
>> +
>> +		my $volid;
>> +		if ($ds eq 'efidisk0') {
>> +		    my $smm = PVE::QemuServer::Machine::machine_type_is_q35($conf);
>> +		    ($volid, $size) = PVE::QemuServer::create_efidisk(
>> +			$storecfg, $storeid, $vmid, $fmt, $arch, $disk, $smm);
>> +		} elsif ($ds eq 'tpmstate0') {
>> +		    # swtpm can only use raw volumes, and uses a fixed size
>> +		    $size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb');
>> +		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, "raw", undef, $size);
>> +		} else {
>> +		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size);
>> +		}
>> +		push @$vollist, $volid;
>> +		$disk->{file} = $volid;
>> +		$disk->{size} = PVE::Tools::convert_size($size, 'kb' => 'b');
>> +		delete $disk->{format}; # no longer needed
>> +		$res->{$ds} = PVE::QemuServer::print_drive($disk);
>>   	    }
>> -	    push @$vollist, $volid;
>> -	    $disk->{file} = $volid;
>> -	    $disk->{size} = PVE::Tools::convert_size($size, 'kb' => 'b');
>> -	    delete $disk->{format}; # no longer needed
>> -	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
>>   	} else {
>>   
>>   	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
>> @@ -639,11 +671,11 @@ __PACKAGE__->register_method({
>>   
>>   	    foreach my $opt (keys %$param) {
>>   		if (PVE::QemuServer::is_valid_drivename($opt)) {
>> -		    my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
>> +		    my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
>>   		    raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive;
>>   
>>   		    PVE::QemuServer::cleanup_drive_path($opt, $storecfg, $drive);
>> -		    $param->{$opt} = PVE::QemuServer::print_drive($drive);
>> +		    $param->{$opt} = PVE::QemuServer::print_drive($drive, 1);
>>   		}
>>   	    }
>>   
>> @@ -1207,11 +1239,11 @@ my $update_vm_api  = sub {
>>       foreach my $opt (keys %$param) {
>>   	if (PVE::QemuServer::is_valid_drivename($opt)) {
>>   	    # cleanup drive path
>> -	    my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
>> +	    my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
>>   	    raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive;
>>   	    PVE::QemuServer::cleanup_drive_path($opt, $storecfg, $drive);
>>   	    $check_replication->($drive);
>> -	    $param->{$opt} = PVE::QemuServer::print_drive($drive);
>> +	    $param->{$opt} = PVE::QemuServer::print_drive($drive, 1);
>>   	} elsif ($opt =~ m/^net(\d+)$/) {
>>   	    # add macaddr
>>   	    my $net = PVE::QemuServer::parse_net($param->{$opt});
>> @@ -1288,7 +1320,7 @@ my $update_vm_api  = sub {
>>   
>>   	    my $check_drive_perms = sub {
>>   		my ($opt, $val) = @_;
>> -		my $drive = PVE::QemuServer::parse_drive($opt, $val);
>> +		my $drive = PVE::QemuServer::parse_drive($opt, $val, 1);
>>   		# FIXME: cloudinit: CDROM or Disk?
>>   		if (PVE::QemuServer::drive_is_cdrom($drive)) { # CDROM
>>   		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM']);
>> @@ -1384,7 +1416,7 @@ my $update_vm_api  = sub {
>>   		    # default legacy boot order implies all cdroms anyway
>>   		    if (@bootorder) {
>>   			# append new CD drives to bootorder to mark them bootable
>> -			my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
>> +			my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1);
>>   			if (PVE::QemuServer::drive_is_cdrom($drive, 1) && !grep(/^$opt$/, @bootorder)) {
>>   			    push @bootorder, $opt;
>>   			    $conf->{pending}->{boot} = PVE::QemuServer::print_bootorder(\@bootorder);
>> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
>> index b993378..76b4314 100644
>> --- a/PVE/QemuConfig.pm
>> +++ b/PVE/QemuConfig.pm
>> @@ -101,7 +101,7 @@ sub parse_volume {
>>   	}
>>   	$volume = { 'file' => $volume_string };
>>       } else {
>> -	$volume = PVE::QemuServer::Drive::parse_drive($key, $volume_string);
>> +	$volume = PVE::QemuServer::Drive::parse_drive($key, $volume_string, 1);
>>       }
>>   
>>       die "unable to parse volume\n" if !defined($volume) && !$noerr;
>> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
>> index f024269..828076d 100644
>> --- a/PVE/QemuServer/Drive.pm
>> +++ b/PVE/QemuServer/Drive.pm
>> @@ -409,6 +409,20 @@ my $alldrive_fmt = {
>>       %efitype_fmt,
>>   };
>>   
>> +my %import_from_fmt = (
>> +    'import-from' => {
>> +	type => 'string',
>> +	format => 'pve-volume-id-or-absolute-path',
>> +	format_description => 'source volume',
>> +	description => "When creating a new disk, import from this source.",
>> +	optional => 1,
>> +    },
>> +);
>> +my $alldrive_fmt_with_alloc = {
>> +    %$alldrive_fmt,
>> +    %import_from_fmt,
>> +};
>> +
>>   my $unused_fmt = {
>>       volume => { alias => 'file' },
>>       file => {
>> @@ -436,6 +450,8 @@ my $desc_with_alloc = sub {
>>   
>>       my $new_desc = dclone($desc);
>>   
>> +    $new_desc->{format}->{'import-from'} = $import_from_fmt{'import-from'};
>> +
>>       my $extra_note = '';
>>       if ($type eq 'efidisk') {
>>   	$extra_note = " Note that SIZE_IN_GiB is ignored here and that the default EFI vars are ".
>> @@ -445,7 +461,8 @@ my $desc_with_alloc = sub {
>>       }
>>   
>>       $new_desc->{description} .= "Use the special syntax STORAGE_ID:SIZE_IN_GiB to allocate a new ".
>> -	"volume.${extra_note}";
>> +	"volume.${extra_note} Use the 'import-from' parameter to import from an existing volume ".
>> +	"instead (SIZE_IN_GiB is ignored then).";
> 
> or change the check above, and make this
> 
> +	"volume.${extra_note} Use STORAGE_ID:0 and the 'import-from' parameter to import from an existing volume.";
> 
>>   
>>       $with_alloc_desc_cache->{$type} = $new_desc;
>>   
>> @@ -547,7 +564,7 @@ sub drive_is_read_only {
>>   #        [,iothread=on][,serial=serial][,model=model]
>>   
>>   sub parse_drive {
>> -    my ($key, $data) = @_;
>> +    my ($key, $data, $with_alloc) = @_;
>>   
>>       my ($interface, $index);
>>   
>> @@ -558,12 +575,14 @@ sub parse_drive {
>>   	return;
>>       }
>>   
>> -    if (!defined($drivedesc_hash->{$key})) {
>> +    my $desc_hash = $with_alloc ? $drivedesc_hash_with_alloc : $drivedesc_hash;
>> +
>> +    if (!defined($desc_hash->{$key})) {
>>   	warn "invalid drive key: $key\n";
>>   	return;
>>       }
>>   
>> -    my $desc = $drivedesc_hash->{$key}->{format};
>> +    my $desc = $desc_hash->{$key}->{format};
>>       my $res = eval { PVE::JSONSchema::parse_property_string($desc, $data) };
>>       return if !$res;
>>       $res->{interface} = $interface;
>> @@ -623,9 +642,10 @@ sub parse_drive {
>>   }
>>   
>>   sub print_drive {
>> -    my ($drive) = @_;
>> +    my ($drive, $with_alloc) = @_;
>>       my $skip = [ 'index', 'interface' ];
>> -    return PVE::JSONSchema::print_property_string($drive, $alldrive_fmt, $skip);
>> +    my $fmt = $with_alloc ? $alldrive_fmt_with_alloc : $alldrive_fmt;
>> +    return PVE::JSONSchema::print_property_string($drive, $fmt, $skip);
>>   }
>>   
>>   sub get_bootdisks {
>> diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
>> index 51ad52e..7557cac 100755
>> --- a/PVE/QemuServer/ImportDisk.pm
>> +++ b/PVE/QemuServer/ImportDisk.pm
>> @@ -71,7 +71,7 @@ sub do_import {
>>   	PVE::Storage::activate_volumes($storecfg, [$dst_volid]);
>>   	PVE::QemuServer::qemu_img_convert($src_path, $dst_volid, $src_size, undef, $zeroinit);
>>   	PVE::Storage::deactivate_volumes($storecfg, [$dst_volid]);
>> -	PVE::QemuConfig->lock_config($vmid, $create_drive);
>> +	PVE::QemuConfig->lock_config($vmid, $create_drive) if !$params->{'skip-config-update'};
>>       };
>>       if (my $err = $@) {
>>   	eval { PVE::Storage::vdisk_free($storecfg, $dst_volid) };
>> -- 
>> 2.30.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

Re: [pve-devel] [RFC v10 qemu-server 6/7] api: support VM disk import

[Fabian Ebner]

Am 13.01.22 um 11:08 schrieb Fabian Ebner:
> +
> +	    if (my $source = delete $disk->{'import-from'}) {

I'm adding a comment here in v11, because otherwise it's not clear where 
volume activation happens:
+               # abs_filesystem_path also calls activate_volume when 
$source is a volid

I'm also adding "The source should not be actively used by another 
process!" to the description of the import-from parameter in v11.

> +		$source = PVE::Storage::abs_filesystem_path($storecfg, $source, 1);

But there are a couple of issues here:

1. There's no protection against using a source volume that's actively 
used by a guest/other operation. While it's not possible to detect in 
general, I wonder if we should behave more like a full clone and lock 
the owning VM?

1a. we could check if the volume is referenced in the config/snapshots, 
but migration picks up everything, so it might be preferable not to.

1b. the volume might be configured in a VM that doesn't own it...

2. Related: avoiding concurrent activation of volumes on a shared LVM.

3. Related: cannot deactivate any volumes as the might be used by 
something else.

4. abs_filesystem_path does not work for RBD when krbd=0, because the 
plugin produces an "rbd:XYZ" path and the -f || -b check doesn't like 
that. But full clone does work, passing the volid to qemu_img_convert 
and that's likely what we should do here as well, when we are dealing 
with an existing volid rather than an absolute path.

5. Add your own ;)

TL;DR: I'd like to behave much more like full clone, when we are dealing 
with a volid rather than an absolute path.

> +		my $src_size = PVE::Storage::file_size_info($source);
> +		die "Could not get file size of $source" if !defined($src_size);
> +
> +		my (undef, $dst_volid) = PVE::QemuServer::ImportDisk::do_import(

Re: [pve-devel] [RFC v10 qemu-server 6/7] api: support VM disk import

[Fabian Grünbichler]

On January 26, 2022 12:40 pm, Fabian Ebner wrote:
> Am 13.01.22 um 11:08 schrieb Fabian Ebner:
>> +
>> +	    if (my $source = delete $disk->{'import-from'}) {
> 
> I'm adding a comment here in v11, because otherwise it's not clear where 
> volume activation happens:
> +               # abs_filesystem_path also calls activate_volume when 
> $source is a volid
> 
> I'm also adding "The source should not be actively used by another 
> process!" to the description of the import-from parameter in v11.

sounds good

> 
>> +		$source = PVE::Storage::abs_filesystem_path($storecfg, $source, 1);
> 
> But there are a couple of issues here:
> 
> 1. There's no protection against using a source volume that's actively 
> used by a guest/other operation. While it's not possible to detect in 
> general, I wonder if we should behave more like a full clone and lock 
> the owning VM?
> 
> 1a. we could check if the volume is referenced in the config/snapshots, 
> but migration picks up everything, so it might be preferable not to.
> 
> 1b. the volume might be configured in a VM that doesn't own it...
> 
> 2. Related: avoiding concurrent activation of volumes on a shared LVM.
> 
> 3. Related: cannot deactivate any volumes as the might be used by 
> something else.
> 
> 4. abs_filesystem_path does not work for RBD when krbd=0, because the 
> plugin produces an "rbd:XYZ" path and the -f || -b check doesn't like 
> that. But full clone does work, passing the volid to qemu_img_convert 
> and that's likely what we should do here as well, when we are dealing 
> with an existing volid rather than an absolute path.
> 
> 5. Add your own ;)
> 
> TL;DR: I'd like to behave much more like full clone, when we are dealing 
> with a volid rather than an absolute path.

yeah. it sounds to me like we could do most of that properly by just 
(iff import source is a volume) check whether the owning VM resides on 
the current node, and lock if so (and fail if not?). not sure whether 
we'd want to require it to be stopped as well if the volume is 
referenced in the current config? for full clones we pass the 'running' 
state to the storage layer's volume_has_feature - but that seems to not 
use the information in any way?

that way we can skip deactivation altogether (it's only relevant for 
shared storages that require it for migration, and by locking the owning 
VM and having a requirement for it to be on the same node at import-time, 
no migration can happen in parallel anyway..). or we could deactivate if 
an owning VM exists and is not running, like we do at the end of full 
clones.

1b is 'all bets are off' territory anyway IMHO - there is no sane way to 
handle all the edge cases..

> 
>> +		my $src_size = PVE::Storage::file_size_info($source);
>> +		die "Could not get file size of $source" if !defined($src_size);
>> +
>> +		my (undef, $dst_volid) = PVE::QemuServer::ImportDisk::do_import(
> 

Re: [pve-devel] [RFC v10 qemu-server 6/7] api: support VM disk import

[Fabian Ebner]

Am 26.01.22 um 13:42 schrieb Fabian Grünbichler:
> On January 26, 2022 12:40 pm, Fabian Ebner wrote:
>> Am 13.01.22 um 11:08 schrieb Fabian Ebner:
>>> +
>>> +	    if (my $source = delete $disk->{'import-from'}) {
>>
>> I'm adding a comment here in v11, because otherwise it's not clear where
>> volume activation happens:
>> +               # abs_filesystem_path also calls activate_volume when
>> $source is a volid
>>
>> I'm also adding "The source should not be actively used by another
>> process!" to the description of the import-from parameter in v11.
> 
> sounds good
> 
>>
>>> +		$source = PVE::Storage::abs_filesystem_path($storecfg, $source, 1);
>>
>> But there are a couple of issues here:
>>
>> 1. There's no protection against using a source volume that's actively
>> used by a guest/other operation. While it's not possible to detect in
>> general, I wonder if we should behave more like a full clone and lock
>> the owning VM?
>>
>> 1a. we could check if the volume is referenced in the config/snapshots,
>> but migration picks up everything, so it might be preferable not to.
>>
>> 1b. the volume might be configured in a VM that doesn't own it...
>>
>> 2. Related: avoiding concurrent activation of volumes on a shared LVM.
>>
>> 3. Related: cannot deactivate any volumes as the might be used by
>> something else.
>>
>> 4. abs_filesystem_path does not work for RBD when krbd=0, because the
>> plugin produces an "rbd:XYZ" path and the -f || -b check doesn't like
>> that. But full clone does work, passing the volid to qemu_img_convert
>> and that's likely what we should do here as well, when we are dealing
>> with an existing volid rather than an absolute path.
>>
>> 5. Add your own ;)
>>
>> TL;DR: I'd like to behave much more like full clone, when we are dealing
>> with a volid rather than an absolute path.
> 
> yeah. it sounds to me like we could do most of that properly by just
> (iff import source is a volume) check whether the owning VM resides on
> the current node, and lock if so (and fail if not?). not sure whether
> we'd want to require it to be stopped as well if the volume is
> referenced in the current config?

For full clone, we use qemu_drive_mirror if the VM is running (and not
cloning from a snapshot). I'll think about re-using clone_disk(), then
having a running VM shouldn't be a problem either.

> for full clones we pass the 'running'
> state to the storage layer's volume_has_feature - but that seems to not
> use the information in any way?

Yes, seems like it was a intended for future use, but was never actually
required (well, some external plugin might be using it).

> 
> that way we can skip deactivation altogether (it's only relevant for
> shared storages that require it for migration, and by locking the owning
> VM and having a requirement for it to be on the same node at import-time,
> no migration can happen in parallel anyway..). or we could deactivate if
> an owning VM exists and is not running, like we do at the end of full
> clones.

Sounds good. We actually only deactivate at the end of full clone if the
clone was to a different target node. Since the new config moves to a
different node then, deactivating the cloned volumes is required of
course, but I /think/ deactivating the source volumes is actually
optional (why should it depend on whether there's a target or not?).

> 
> 1b is 'all bets are off' territory anyway IMHO - there is no sane way to
> handle all the edge cases..
> 
>>
>>> +		my $src_size = PVE::Storage::file_size_info($source);
>>> +		die "Could not get file size of $source" if !defined($src_size);
>>> +
>>> +		my (undef, $dst_volid) = PVE::QemuServer::ImportDisk::do_import(
>>

Re: [pve-devel] [RFC v10 qemu-server 6/7] api: support VM disk import

[Fabian Grünbichler]

On January 27, 2022 9:21 am, Fabian Ebner wrote:
> Am 26.01.22 um 13:42 schrieb Fabian Grünbichler:
>> 
>> that way we can skip deactivation altogether (it's only relevant for
>> shared storages that require it for migration, and by locking the owning
>> VM and having a requirement for it to be on the same node at import-time,
>> no migration can happen in parallel anyway..). or we could deactivate if
>> an owning VM exists and is not running, like we do at the end of full
>> clones.
> 
> Sounds good. We actually only deactivate at the end of full clone if the
> clone was to a different target node. Since the new config moves to a
> different node then, deactivating the cloned volumes is required of
> course, but I /think/ deactivating the source volumes is actually
> optional (why should it depend on whether there's a target or not?).

yeah, that is a bit strange. the source can stay activated IMHO.

of course the target for full_clone needs to be de-activated if the 
clone is a cross-node operation, but that is never the case for 
importing (if we require owning VMs of source volumes to be on the 
current node), so we could also leave those activated or just do 
whatever we do in the regular creation path ;)

Re: [pve-devel] [RFC v10 qemu-server 6/7] api: support VM disk import

[Fabian Ebner]

Am 13.01.22 um 11:08 schrieb Fabian Ebner:
> @@ -89,6 +90,10 @@ my $check_storage_access = sub {
>  	} else {
>  	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
>  	}
> +
> +	if (my $source_image = $drive->{'import-from'}) {
> +	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $source_image);
> +	}
>      });
>  

AFAICT, if $vmid doesn't match the one from the volume, the check
requires Datastore.Allocate privileges on the storage, which might be a
bit much for many scenarios. Should the check rather be something like

if ($ownerid) {
    # check VM.Clone for owner VM
    # Note that v11 will use clone_disk() for such disks
} else {
    # PVE::Storage::check_volume_access
}

?

Re: [pve-devel] [RFC v10 qemu-server 6/7] api: support VM disk import

[Fabian Grünbichler]

On February 22, 2022 1:11 pm, Fabian Ebner wrote:
> Am 13.01.22 um 11:08 schrieb Fabian Ebner:
>> @@ -89,6 +90,10 @@ my $check_storage_access = sub {
>>  	} else {
>>  	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
>>  	}
>> +
>> +	if (my $source_image = $drive->{'import-from'}) {
>> +	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $source_image);
>> +	}
>>      });
>>  
> 
> AFAICT, if $vmid doesn't match the one from the volume, the check
> requires Datastore.Allocate privileges on the storage, which might be a
> bit much for many scenarios. Should the check rather be something like
> 
> if ($ownerid) {
>     # check VM.Clone for owner VM
>     # Note that v11 will use clone_disk() for such disks

sounds sensible to me - if it's possible to copy the volume (and other 
stuff) already with that privilege, then re-using that check for the 
similar functionality here is fine.

check_volume_access is like that because it assumes that the caller 
already checked that changes/access to the guest itself is okay - so if 
a 'foreign' volume is queried, it requires more permissions since it 
assumes that assumption doesn't hold.

since most queries here will be for 'foreign' volumes, checking that 
cloning from that owner is okay and then passing in that vmid instead of 
the target one would probably be best (as that still handles the 
question of whether accessing the source storage is okay in general), 
and of course we'd need that fallback anyway in case there is no 
owner/we don't have clone permissions?

> } else {
>     # PVE::Storage::check_volume_access
> }
> 
> ?
> 

[pve-devel] [RFC v10 qemu-server 7/7] api: create disks: factor out common part from if/else

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v10.

 PVE/API2/Qemu.pm | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 8c74ecc..fa6aa9c 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -204,7 +204,7 @@ my $create_disks = sub {
 		my $src_size = PVE::Storage::file_size_info($source);
 		die "Could not get file size of $source" if !defined($src_size);
 
-		my (undef, $dst_volid) = PVE::QemuServer::ImportDisk::do_import(
+		(undef, $volid) = PVE::QemuServer::ImportDisk::do_import(
 		    $source,
 		    $vmid,
 		    $storeid,
@@ -215,18 +215,13 @@ my $create_disks = sub {
 		    },
 		);
 
-		push @$vollist, $dst_volid;
-		$disk->{file} = $dst_volid;
 		$disk->{size} = $src_size;
-		delete $disk->{format}; # no longer needed
-		$res->{$ds} = PVE::QemuServer::print_drive($disk);
 	    } else {
 		my $defformat = PVE::Storage::storage_default_format($storecfg, $storeid);
 		my $fmt = $disk->{format} || $defformat;
 
 		$size = PVE::Tools::convert_size($size, 'gb' => 'kb'); # vdisk_alloc uses kb
 
-		my $volid;
 		if ($ds eq 'efidisk0') {
 		    my $smm = PVE::QemuServer::Machine::machine_type_is_q35($conf);
 		    ($volid, $size) = PVE::QemuServer::create_efidisk(
@@ -238,12 +233,13 @@ my $create_disks = sub {
 		} else {
 		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size);
 		}
-		push @$vollist, $volid;
-		$disk->{file} = $volid;
 		$disk->{size} = PVE::Tools::convert_size($size, 'kb' => 'b');
-		delete $disk->{format}; # no longer needed
-		$res->{$ds} = PVE::QemuServer::print_drive($disk);
 	    }
+
+	    push @$vollist, $volid;
+	    $disk->{file} = $volid;
+	    delete $disk->{format}; # no longer needed
+	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
 	} else {
 
 	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
-- 
2.30.2

Re: [pve-devel] [RFC v10 qemu-server/manager] API for disk import and OVF

[Fabian Grünbichler]

On January 13, 2022 11:08 am, Fabian Ebner wrote:
> Extend qm importdisk/importovf functionality to the API.
> 
> 
> Used Dominic's latest version[0] as a starting point. GUI part still
> needs to be rebased/updated, so it's not included here.
> 
> 
> Changes from v9:
> 
> * Split patch into smaller parts
> 
> * Some minor (style) fixes/improvements (see individual patches)
> 
> * Drop $manifest_only parameter for parse_ovf
> 
>     Instead, untaint the path when calling file_size_info, which makes
>     the call also work via API which uses the -T switch. If we do want
>     to keep $manifest_only, I'd argue that it should also skip the
>     file existence check and not only the file size check. Opinions?
> 
> * Re-use do_import function
> 
>     Rather than duplicating most of it. The down side is the need to
>     add a new parameter for skipping configuration update. But I
>     suppose the plan is to have qm import switch to the new API at
>     some point, and then do_import can be simplified.
> 
> * Instead of adding an import-sources parameter to the API, use a new
>   import-from property for the disk, that's only available with
>   import/alloc-enabled API endpoints via its own version of the schema
> 
>     Avoids the split across regular drive key parameters and
>     'import_soruces', which avoids quite a bit of cross-checking
>     between the two and parsing/passing around the latter.
> 
>     The big downsides are:
>     * Schema handling is a bit messy.
>     * Need to special case print_drive, because we do intermediate
>       parse/print to cleanup drive paths. At a first glance, this
>       seems not too easy to avoid without complicating things elsewehere.
>     * Using the import-aware parse_drive in parse_volume, because that
>       is used via the foreach_volume iterators handling the parameters
>       of the import-enabled endpoints. Could be avoided by using for
>       loops with the import-aware parse_drive instead of
>       foreach_volume.
> 
>     Counter-arguments for using a single schema (citing Fabian G.):
>     * docs/schema dump/api docs: shouldn't look like you can put that
>       everywhere where we use the config schema
>     * shouldn't have nasty side-effects if someone puts it into the
>       config
> 
> 
> After all, the 'import-from' disk property approach turned out to be
> a uglier than I hoped it would.
> 
> My problem with the 'import-sources' API parameter approach (see [0]
> for details) is that it requires specifying both
>     scsi0: <target storage>:-1,<properties>
>     import-sources: scsi0=/path/or/volid/for/source
> leading to a not ideal user interface and parameter handling needing
> cross-checks to verify that the right combination is specified, and
> passing both around at the same time.
> 
> Another approach would be adding a new special volid syntax using
>     my $IMPORT_DISK_RE = qr!^(([^/:\s]+):)import:(.*)$!;
> allowing for e.g.
>     qm set 126 -scsi1 rbdkvm:import:myzpool:vm-114-disk-0,aio=native
>     qm set 126 -scsi2 rbdkvm:import:/dev/zvol/myzpool/vm-114-disk-1,backup=0
> Yes, it's a hack, but it would avoid the pain points from both other
> approaches and be very simple. See the end of the mail for a POC.

the biggest down-side is that this 'invades' the storage plugin-owned 
volume ID namespace (further than the pre-existing, documented new disk 
syntax) - a volume ID is defined as anything starting with a storage 
identifier, followed by ':', followed by an arbitrary string (where the 
semantics of that string are up to the plugin).

while I don't think there is a storage plugin out there that somehow 
starts its volume IDs with 'import:' by default, it still doesn't feel 
very nice.. there might be plugins that are not very strict in their 
parsing that might accept such an 'import volid' as regular volid, and 
parse the latter part (which can be a regular volid after all!) instead 
of the full thing, which would make code paths that should not accept 
import volids accept them and just use the source without importing, 
with potentially disastrous consequences :-/

that being said - other then this bigger conceptual question I only 
found nits/fixup/followup material - so if we want to take the current 
RFC I'd give this a more in-depth test spin in the next few days and 
apply with the small stuff adressed ;)

> 
> 
> [0]: https://lists.proxmox.com/pipermail/pve-devel/2021-June/048564.html
> 
> 
> pve-manager:
> 
> Fabian Ebner (1):
>   api: nodes: add readovf endpoint
> 
>  PVE/API2/Nodes.pm | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> 
> qemu-server:
> 
> Dominic Jäger (1):
>   api: support VM disk import
> 
> Fabian Ebner (6):
>   schema: add pve-volume-id-or-absolute-path
>   parse ovf: untaint path when calling file_size_info
>   api: add endpoint for parsing .ovf files
>   image convert: allow block device as source
>   schema: drive: use separate schema when disk allocation is possible
>   api: create disks: factor out common part from if/else
> 
>  PVE/API2/Qemu.pm             | 86 +++++++++++++++++++++++----------
>  PVE/API2/Qemu/Makefile       |  2 +-
>  PVE/API2/Qemu/OVF.pm         | 55 +++++++++++++++++++++
>  PVE/QemuConfig.pm            |  2 +-
>  PVE/QemuServer.pm            | 57 +++++++++++++++++++---
>  PVE/QemuServer/Drive.pm      | 92 +++++++++++++++++++++++++++---------
>  PVE/QemuServer/ImportDisk.pm |  2 +-
>  PVE/QemuServer/OVF.pm        |  9 ++--
>  8 files changed, 242 insertions(+), 63 deletions(-)
>  create mode 100644 PVE/API2/Qemu/OVF.pm
> 
> -- 
> 2.30.2
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 6992f6f..6a22899 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -21,8 +21,9 @@ use PVE::ReplicationConfig;
>  use PVE::GuestHelpers;
>  use PVE::QemuConfig;
>  use PVE::QemuServer;
> -use PVE::QemuServer::Drive;
>  use PVE::QemuServer::CPUConfig;
> +use PVE::QemuServer::Drive;
> +use PVE::QemuServer::ImportDisk;
>  use PVE::QemuServer::Monitor qw(mon_cmd);
>  use PVE::QemuServer::Machine;
>  use PVE::QemuMigrate;
> @@ -64,6 +65,7 @@ my $resolve_cdrom_alias = sub {
>  };
>  
>  my $NEW_DISK_RE = qr!^(([^/:\s]+):)?(\d+(\.\d+)?)$!;
> +my $IMPORT_DISK_RE = qr!^(([^/:\s]+):)import:(.*)$!;
>  my $check_storage_access = sub {
>     my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage) = @_;
>  
> @@ -86,6 +88,9 @@ my $check_storage_access = sub {
>  	    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
>  	    raise_param_exc({ storage => "storage '$storeid' does not support vm images"})
>  		if !$scfg->{content}->{images};
> +	} elsif (!$isCDROM && ($volid =~ $IMPORT_DISK_RE)) {
> +	    warn "check access matched: $2 and $3\n";
> +	    warn "TODO implement import access check!\n";
>  	} else {
>  	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
>  	}
> @@ -212,6 +217,29 @@ my $create_disks = sub {
>  	    $disk->{size} = PVE::Tools::convert_size($size, 'kb' => 'b');
>  	    delete $disk->{format}; # no longer needed
>  	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
> +	} elsif ($volid =~ $IMPORT_DISK_RE) {
> +	    my ($storeid, $source) = ($2, $3);
> +
> +	    $source = PVE::Storage::abs_filesystem_path($storecfg, $source, 1);
> +	    my $src_size = PVE::Storage::file_size_info($source);
> +	    die "Could not get file size of $source" if !defined($src_size);
> +
> +	    my (undef, $dst_volid) = PVE::QemuServer::ImportDisk::do_import(
> +		$source,
> +		$vmid,
> +		$storeid,
> +		{
> +		    drive_name => $ds,
> +		    format => $disk->{format},
> +		    'skip-config-update' => 1,
> +		},
> +	    );
> +
> +	    push @$vollist, $dst_volid;
> +	    $disk->{file} = $dst_volid;
> +	    $disk->{size} = $src_size;
> +	    delete $disk->{format}; # no longer needed
> +	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
>  	} else {
>  
>  	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
> diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
> index 51ad52e..7557cac 100755
> --- a/PVE/QemuServer/ImportDisk.pm
> +++ b/PVE/QemuServer/ImportDisk.pm
> @@ -71,7 +71,7 @@ sub do_import {
>  	PVE::Storage::activate_volumes($storecfg, [$dst_volid]);
>  	PVE::QemuServer::qemu_img_convert($src_path, $dst_volid, $src_size, undef, $zeroinit);
>  	PVE::Storage::deactivate_volumes($storecfg, [$dst_volid]);
> -	PVE::QemuConfig->lock_config($vmid, $create_drive);
> +	PVE::QemuConfig->lock_config($vmid, $create_drive) if !$params->{'skip-config-update'};
>      };
>      if (my $err = $@) {
>  	eval { PVE::Storage::vdisk_free($storecfg, $dst_volid) };
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 

Re: [pve-devel] [RFC v10 qemu-server/manager] API for disk import and OVF

[Fabian Ebner]

Am 17.01.22 um 16:43 schrieb Fabian Grünbichler:
> On January 13, 2022 11:08 am, Fabian Ebner wrote:
>> Extend qm importdisk/importovf functionality to the API.
>>
>>
>> Used Dominic's latest version[0] as a starting point. GUI part still
>> needs to be rebased/updated, so it's not included here.
>>
>>
>> Changes from v9:
>>
>> * Split patch into smaller parts
>>
>> * Some minor (style) fixes/improvements (see individual patches)
>>
>> * Drop $manifest_only parameter for parse_ovf
>>
>>      Instead, untaint the path when calling file_size_info, which makes
>>      the call also work via API which uses the -T switch. If we do want
>>      to keep $manifest_only, I'd argue that it should also skip the
>>      file existence check and not only the file size check. Opinions?
>>
>> * Re-use do_import function
>>
>>      Rather than duplicating most of it. The down side is the need to
>>      add a new parameter for skipping configuration update. But I
>>      suppose the plan is to have qm import switch to the new API at
>>      some point, and then do_import can be simplified.
>>
>> * Instead of adding an import-sources parameter to the API, use a new
>>    import-from property for the disk, that's only available with
>>    import/alloc-enabled API endpoints via its own version of the schema
>>
>>      Avoids the split across regular drive key parameters and
>>      'import_soruces', which avoids quite a bit of cross-checking
>>      between the two and parsing/passing around the latter.
>>
>>      The big downsides are:
>>      * Schema handling is a bit messy.
>>      * Need to special case print_drive, because we do intermediate
>>        parse/print to cleanup drive paths. At a first glance, this
>>        seems not too easy to avoid without complicating things elsewehere.
>>      * Using the import-aware parse_drive in parse_volume, because that
>>        is used via the foreach_volume iterators handling the parameters
>>        of the import-enabled endpoints. Could be avoided by using for
>>        loops with the import-aware parse_drive instead of
>>        foreach_volume.
>>
>>      Counter-arguments for using a single schema (citing Fabian G.):
>>      * docs/schema dump/api docs: shouldn't look like you can put that
>>        everywhere where we use the config schema
>>      * shouldn't have nasty side-effects if someone puts it into the
>>        config
>>
>>
>> After all, the 'import-from' disk property approach turned out to be
>> a uglier than I hoped it would.
>>
>> My problem with the 'import-sources' API parameter approach (see [0]
>> for details) is that it requires specifying both
>>      scsi0: <target storage>:-1,<properties>
>>      import-sources: scsi0=/path/or/volid/for/source
>> leading to a not ideal user interface and parameter handling needing
>> cross-checks to verify that the right combination is specified, and
>> passing both around at the same time.
>>
>> Another approach would be adding a new special volid syntax using
>>      my $IMPORT_DISK_RE = qr!^(([^/:\s]+):)import:(.*)$!;
>> allowing for e.g.
>>      qm set 126 -scsi1 rbdkvm:import:myzpool:vm-114-disk-0,aio=native
>>      qm set 126 -scsi2 rbdkvm:import:/dev/zvol/myzpool/vm-114-disk-1,backup=0
>> Yes, it's a hack, but it would avoid the pain points from both other
>> approaches and be very simple. See the end of the mail for a POC.
> 
> the biggest down-side is that this 'invades' the storage plugin-owned
> volume ID namespace (further than the pre-existing, documented new disk
> syntax) - a volume ID is defined as anything starting with a storage
> identifier, followed by ':', followed by an arbitrary string (where the
> semantics of that string are up to the plugin).
> 
> while I don't think there is a storage plugin out there that somehow
> starts its volume IDs with 'import:' by default, it still doesn't feel
> very nice.. there might be plugins that are not very strict in their
> parsing that might accept such an 'import volid' as regular volid, and
> parse the latter part (which can be a regular volid after all!) instead
> of the full thing, which would make code paths that should not accept
> import volids accept them and just use the source without importing,
> with potentially disastrous consequences :-/
> 

Ok, good point!

> that being said - other then this bigger conceptual question I only
> found nits/fixup/followup material - so if we want to take the current
> RFC I'd give this a more in-depth test spin in the next few days and
> apply with the small stuff adressed ;)
> 

Fine by me. I could also re-spin with the import-sources API parameter 
if that's preferred. If we go with the import-from approach, besides 
addressing your nits, I'd also not make parse_volume unconditionally 
parse with the extended schema, but replace the foreach_volume iterators 
in the appropriate places.

>>
>>
>> [0]: https://lists.proxmox.com/pipermail/pve-devel/2021-June/048564.html
>>
>>
>> pve-manager:
>>
>> Fabian Ebner (1):
>>    api: nodes: add readovf endpoint
>>
>>   PVE/API2/Nodes.pm | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>>
>> qemu-server:
>>
>> Dominic Jäger (1):
>>    api: support VM disk import
>>
>> Fabian Ebner (6):
>>    schema: add pve-volume-id-or-absolute-path
>>    parse ovf: untaint path when calling file_size_info
>>    api: add endpoint for parsing .ovf files
>>    image convert: allow block device as source
>>    schema: drive: use separate schema when disk allocation is possible
>>    api: create disks: factor out common part from if/else
>>
>>   PVE/API2/Qemu.pm             | 86 +++++++++++++++++++++++----------
>>   PVE/API2/Qemu/Makefile       |  2 +-
>>   PVE/API2/Qemu/OVF.pm         | 55 +++++++++++++++++++++
>>   PVE/QemuConfig.pm            |  2 +-
>>   PVE/QemuServer.pm            | 57 +++++++++++++++++++---
>>   PVE/QemuServer/Drive.pm      | 92 +++++++++++++++++++++++++++---------
>>   PVE/QemuServer/ImportDisk.pm |  2 +-
>>   PVE/QemuServer/OVF.pm        |  9 ++--
>>   8 files changed, 242 insertions(+), 63 deletions(-)
>>   create mode 100644 PVE/API2/Qemu/OVF.pm
>>
>> -- 
>> 2.30.2
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index 6992f6f..6a22899 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -21,8 +21,9 @@ use PVE::ReplicationConfig;
>>   use PVE::GuestHelpers;
>>   use PVE::QemuConfig;
>>   use PVE::QemuServer;
>> -use PVE::QemuServer::Drive;
>>   use PVE::QemuServer::CPUConfig;
>> +use PVE::QemuServer::Drive;
>> +use PVE::QemuServer::ImportDisk;
>>   use PVE::QemuServer::Monitor qw(mon_cmd);
>>   use PVE::QemuServer::Machine;
>>   use PVE::QemuMigrate;
>> @@ -64,6 +65,7 @@ my $resolve_cdrom_alias = sub {
>>   };
>>   
>>   my $NEW_DISK_RE = qr!^(([^/:\s]+):)?(\d+(\.\d+)?)$!;
>> +my $IMPORT_DISK_RE = qr!^(([^/:\s]+):)import:(.*)$!;
>>   my $check_storage_access = sub {
>>      my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage) = @_;
>>   
>> @@ -86,6 +88,9 @@ my $check_storage_access = sub {
>>   	    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
>>   	    raise_param_exc({ storage => "storage '$storeid' does not support vm images"})
>>   		if !$scfg->{content}->{images};
>> +	} elsif (!$isCDROM && ($volid =~ $IMPORT_DISK_RE)) {
>> +	    warn "check access matched: $2 and $3\n";
>> +	    warn "TODO implement import access check!\n";
>>   	} else {
>>   	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
>>   	}
>> @@ -212,6 +217,29 @@ my $create_disks = sub {
>>   	    $disk->{size} = PVE::Tools::convert_size($size, 'kb' => 'b');
>>   	    delete $disk->{format}; # no longer needed
>>   	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
>> +	} elsif ($volid =~ $IMPORT_DISK_RE) {
>> +	    my ($storeid, $source) = ($2, $3);
>> +
>> +	    $source = PVE::Storage::abs_filesystem_path($storecfg, $source, 1);
>> +	    my $src_size = PVE::Storage::file_size_info($source);
>> +	    die "Could not get file size of $source" if !defined($src_size);
>> +
>> +	    my (undef, $dst_volid) = PVE::QemuServer::ImportDisk::do_import(
>> +		$source,
>> +		$vmid,
>> +		$storeid,
>> +		{
>> +		    drive_name => $ds,
>> +		    format => $disk->{format},
>> +		    'skip-config-update' => 1,
>> +		},
>> +	    );
>> +
>> +	    push @$vollist, $dst_volid;
>> +	    $disk->{file} = $dst_volid;
>> +	    $disk->{size} = $src_size;
>> +	    delete $disk->{format}; # no longer needed
>> +	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
>>   	} else {
>>   
>>   	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
>> diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
>> index 51ad52e..7557cac 100755
>> --- a/PVE/QemuServer/ImportDisk.pm
>> +++ b/PVE/QemuServer/ImportDisk.pm
>> @@ -71,7 +71,7 @@ sub do_import {
>>   	PVE::Storage::activate_volumes($storecfg, [$dst_volid]);
>>   	PVE::QemuServer::qemu_img_convert($src_path, $dst_volid, $src_size, undef, $zeroinit);
>>   	PVE::Storage::deactivate_volumes($storecfg, [$dst_volid]);
>> -	PVE::QemuConfig->lock_config($vmid, $create_drive);
>> +	PVE::QemuConfig->lock_config($vmid, $create_drive) if !$params->{'skip-config-update'};
>>       };
>>       if (my $err = $@) {
>>   	eval { PVE::Storage::vdisk_free($storecfg, $dst_volid) };
>> -- 
>> 2.30.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

Re: [pve-devel] [RFC v10 qemu-server/manager] API for disk import and OVF

[Fabian Grünbichler]

On January 18, 2022 10:08 am, Fabian Ebner wrote:
> Am 17.01.22 um 16:43 schrieb Fabian Grünbichler:
>> On January 13, 2022 11:08 am, Fabian Ebner wrote:
>>> Extend qm importdisk/importovf functionality to the API.
>>>
>>>
>>> Used Dominic's latest version[0] as a starting point. GUI part still
>>> needs to be rebased/updated, so it's not included here.
>>>
>>>
>>> Changes from v9:
>>>
>>> * Split patch into smaller parts
>>>
>>> * Some minor (style) fixes/improvements (see individual patches)
>>>
>>> * Drop $manifest_only parameter for parse_ovf
>>>
>>>      Instead, untaint the path when calling file_size_info, which makes
>>>      the call also work via API which uses the -T switch. If we do want
>>>      to keep $manifest_only, I'd argue that it should also skip the
>>>      file existence check and not only the file size check. Opinions?
>>>
>>> * Re-use do_import function
>>>
>>>      Rather than duplicating most of it. The down side is the need to
>>>      add a new parameter for skipping configuration update. But I
>>>      suppose the plan is to have qm import switch to the new API at
>>>      some point, and then do_import can be simplified.
>>>
>>> * Instead of adding an import-sources parameter to the API, use a new
>>>    import-from property for the disk, that's only available with
>>>    import/alloc-enabled API endpoints via its own version of the schema
>>>
>>>      Avoids the split across regular drive key parameters and
>>>      'import_soruces', which avoids quite a bit of cross-checking
>>>      between the two and parsing/passing around the latter.
>>>
>>>      The big downsides are:
>>>      * Schema handling is a bit messy.
>>>      * Need to special case print_drive, because we do intermediate
>>>        parse/print to cleanup drive paths. At a first glance, this
>>>        seems not too easy to avoid without complicating things elsewehere.
>>>      * Using the import-aware parse_drive in parse_volume, because that
>>>        is used via the foreach_volume iterators handling the parameters
>>>        of the import-enabled endpoints. Could be avoided by using for
>>>        loops with the import-aware parse_drive instead of
>>>        foreach_volume.
>>>
>>>      Counter-arguments for using a single schema (citing Fabian G.):
>>>      * docs/schema dump/api docs: shouldn't look like you can put that
>>>        everywhere where we use the config schema
>>>      * shouldn't have nasty side-effects if someone puts it into the
>>>        config
>>>
>>>
>>> After all, the 'import-from' disk property approach turned out to be
>>> a uglier than I hoped it would.
>>>
>>> My problem with the 'import-sources' API parameter approach (see [0]
>>> for details) is that it requires specifying both
>>>      scsi0: <target storage>:-1,<properties>
>>>      import-sources: scsi0=/path/or/volid/for/source
>>> leading to a not ideal user interface and parameter handling needing
>>> cross-checks to verify that the right combination is specified, and
>>> passing both around at the same time.
>>>
>>> Another approach would be adding a new special volid syntax using
>>>      my $IMPORT_DISK_RE = qr!^(([^/:\s]+):)import:(.*)$!;
>>> allowing for e.g.
>>>      qm set 126 -scsi1 rbdkvm:import:myzpool:vm-114-disk-0,aio=native
>>>      qm set 126 -scsi2 rbdkvm:import:/dev/zvol/myzpool/vm-114-disk-1,backup=0
>>> Yes, it's a hack, but it would avoid the pain points from both other
>>> approaches and be very simple. See the end of the mail for a POC.
>> 
>> the biggest down-side is that this 'invades' the storage plugin-owned
>> volume ID namespace (further than the pre-existing, documented new disk
>> syntax) - a volume ID is defined as anything starting with a storage
>> identifier, followed by ':', followed by an arbitrary string (where the
>> semantics of that string are up to the plugin).
>> 
>> while I don't think there is a storage plugin out there that somehow
>> starts its volume IDs with 'import:' by default, it still doesn't feel
>> very nice.. there might be plugins that are not very strict in their
>> parsing that might accept such an 'import volid' as regular volid, and
>> parse the latter part (which can be a regular volid after all!) instead
>> of the full thing, which would make code paths that should not accept
>> import volids accept them and just use the source without importing,
>> with potentially disastrous consequences :-/
>> 
> 
> Ok, good point!
> 
>> that being said - other then this bigger conceptual question I only
>> found nits/fixup/followup material - so if we want to take the current
>> RFC I'd give this a more in-depth test spin in the next few days and
>> apply with the small stuff adressed ;)
>> 
> 
> Fine by me. I could also re-spin with the import-sources API parameter 
> if that's preferred. If we go with the import-from approach, besides 
> addressing your nits, I'd also not make parse_volume unconditionally 
> parse with the extended schema, but replace the foreach_volume iterators 
> in the appropriate places.

I actually prefer the import-from approach to the old one, even with the 
duplicate schema downside - it makes it more explicit where we want to
allow 'magically' allocating new volumes, which was always a bit 
confusing when you didn't have a clear picture of the code in your 
head..

> 
>>>
>>>
>>> [0]: https://lists.proxmox.com/pipermail/pve-devel/2021-June/048564.html
>>>
>>>
>>> pve-manager:
>>>
>>> Fabian Ebner (1):
>>>    api: nodes: add readovf endpoint
>>>
>>>   PVE/API2/Nodes.pm | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>>
>>> qemu-server:
>>>
>>> Dominic Jäger (1):
>>>    api: support VM disk import
>>>
>>> Fabian Ebner (6):
>>>    schema: add pve-volume-id-or-absolute-path
>>>    parse ovf: untaint path when calling file_size_info
>>>    api: add endpoint for parsing .ovf files
>>>    image convert: allow block device as source
>>>    schema: drive: use separate schema when disk allocation is possible
>>>    api: create disks: factor out common part from if/else
>>>
>>>   PVE/API2/Qemu.pm             | 86 +++++++++++++++++++++++----------
>>>   PVE/API2/Qemu/Makefile       |  2 +-
>>>   PVE/API2/Qemu/OVF.pm         | 55 +++++++++++++++++++++
>>>   PVE/QemuConfig.pm            |  2 +-
>>>   PVE/QemuServer.pm            | 57 +++++++++++++++++++---
>>>   PVE/QemuServer/Drive.pm      | 92 +++++++++++++++++++++++++++---------
>>>   PVE/QemuServer/ImportDisk.pm |  2 +-
>>>   PVE/QemuServer/OVF.pm        |  9 ++--
>>>   8 files changed, 242 insertions(+), 63 deletions(-)
>>>   create mode 100644 PVE/API2/Qemu/OVF.pm
>>>
>>> -- 
>>> 2.30.2
>>>
>>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>>> index 6992f6f..6a22899 100644
>>> --- a/PVE/API2/Qemu.pm
>>> +++ b/PVE/API2/Qemu.pm
>>> @@ -21,8 +21,9 @@ use PVE::ReplicationConfig;
>>>   use PVE::GuestHelpers;
>>>   use PVE::QemuConfig;
>>>   use PVE::QemuServer;
>>> -use PVE::QemuServer::Drive;
>>>   use PVE::QemuServer::CPUConfig;
>>> +use PVE::QemuServer::Drive;
>>> +use PVE::QemuServer::ImportDisk;
>>>   use PVE::QemuServer::Monitor qw(mon_cmd);
>>>   use PVE::QemuServer::Machine;
>>>   use PVE::QemuMigrate;
>>> @@ -64,6 +65,7 @@ my $resolve_cdrom_alias = sub {
>>>   };
>>>   
>>>   my $NEW_DISK_RE = qr!^(([^/:\s]+):)?(\d+(\.\d+)?)$!;
>>> +my $IMPORT_DISK_RE = qr!^(([^/:\s]+):)import:(.*)$!;
>>>   my $check_storage_access = sub {
>>>      my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage) = @_;
>>>   
>>> @@ -86,6 +88,9 @@ my $check_storage_access = sub {
>>>   	    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
>>>   	    raise_param_exc({ storage => "storage '$storeid' does not support vm images"})
>>>   		if !$scfg->{content}->{images};
>>> +	} elsif (!$isCDROM && ($volid =~ $IMPORT_DISK_RE)) {
>>> +	    warn "check access matched: $2 and $3\n";
>>> +	    warn "TODO implement import access check!\n";
>>>   	} else {
>>>   	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
>>>   	}
>>> @@ -212,6 +217,29 @@ my $create_disks = sub {
>>>   	    $disk->{size} = PVE::Tools::convert_size($size, 'kb' => 'b');
>>>   	    delete $disk->{format}; # no longer needed
>>>   	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
>>> +	} elsif ($volid =~ $IMPORT_DISK_RE) {
>>> +	    my ($storeid, $source) = ($2, $3);
>>> +
>>> +	    $source = PVE::Storage::abs_filesystem_path($storecfg, $source, 1);
>>> +	    my $src_size = PVE::Storage::file_size_info($source);
>>> +	    die "Could not get file size of $source" if !defined($src_size);
>>> +
>>> +	    my (undef, $dst_volid) = PVE::QemuServer::ImportDisk::do_import(
>>> +		$source,
>>> +		$vmid,
>>> +		$storeid,
>>> +		{
>>> +		    drive_name => $ds,
>>> +		    format => $disk->{format},
>>> +		    'skip-config-update' => 1,
>>> +		},
>>> +	    );
>>> +
>>> +	    push @$vollist, $dst_volid;
>>> +	    $disk->{file} = $dst_volid;
>>> +	    $disk->{size} = $src_size;
>>> +	    delete $disk->{format}; # no longer needed
>>> +	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
>>>   	} else {
>>>   
>>>   	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
>>> diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
>>> index 51ad52e..7557cac 100755
>>> --- a/PVE/QemuServer/ImportDisk.pm
>>> +++ b/PVE/QemuServer/ImportDisk.pm
>>> @@ -71,7 +71,7 @@ sub do_import {
>>>   	PVE::Storage::activate_volumes($storecfg, [$dst_volid]);
>>>   	PVE::QemuServer::qemu_img_convert($src_path, $dst_volid, $src_size, undef, $zeroinit);
>>>   	PVE::Storage::deactivate_volumes($storecfg, [$dst_volid]);
>>> -	PVE::QemuConfig->lock_config($vmid, $create_drive);
>>> +	PVE::QemuConfig->lock_config($vmid, $create_drive) if !$params->{'skip-config-update'};
>>>       };
>>>       if (my $err = $@) {
>>>   	eval { PVE::Storage::vdisk_free($storecfg, $dst_volid) };
>>> -- 
>>> 2.30.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
>