[pve-devel] [PATCH qemu-server v6 5/5] Add a new command line option 'importovf', to create VMs from an OVF manifest

[Fabian Grünbichler]

comments below

On Tue, May 23, 2017 at 04:26:21PM +0200, Emmanuel Kasper wrote:
> Currently the following extracted parameters are used to create a VM:
> * VM name
> * Memory
> * Number of cores
> * Disks
> ---
>  PVE/CLI/qm.pm | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
> index 7c5b2c4..dde75a7 100755
> --- a/PVE/CLI/qm.pm
> +++ b/PVE/CLI/qm.pm
> @@ -18,10 +18,12 @@ use PVE::INotify;
>  use PVE::RPCEnvironment;
>  use PVE::QemuServer;
>  use PVE::QemuServer::ImportDisk;
> +use PVE::QemuServer::OVF;
>  use PVE::API2::Qemu;
>  use JSON;
>  use PVE::JSONSchema qw(get_standard_option);
>  use Term::ReadLine;
> +use Data::Dumper;
>  
>  use PVE::CLIHandler;
>  
> @@ -481,6 +483,70 @@ __PACKAGE__->register_method ({
>  	return undef;
>      }});
>  
> +__PACKAGE__->register_method ({
> +    name => 'importovf',
> +    path => 'importovf',
> +    description => "Create a new VM using parameters read from an OVF manifest",
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::Cluster::complete_next_vmid }),
> +	    manifest => {
> +		type => 'string',
> +		description => 'path to the ovf file',
> +		},
> +	    storage => get_standard_option('pve-storage-id', {
> +		description => 'Target storage ID',
> +		completion => \&PVE::QemuServer::complete_storage,
> +		optional => 0,
> +	    }),
> +	    format => {
> +		type => 'string',
> +		description => 'Target format',
> +		enum => [ 'raw', 'qcow2', 'vmdk' ],
> +		optional => 1,
> +	    },
> +	    dryrun => {
> +		type => 'boolean',
> +		description => 'Print a parsed representation of the extracted OVF parameters, but do not create a VM',
> +		optional => 1,
> +		}
> +	},
> +    },
> +    returns => { type => 'string'},
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $vmid = $param->{vmid};
> +	my $ovf_file = PVE::Tools::extract_param($param, 'manifest');
> +	my $storeid = PVE::Tools::extract_param($param, 'storage');
> +	my $format = PVE::Tools::extract_param($param, 'format');
> +	my $dryrun = PVE::Tools::extract_param($param, 'dryrun');
> +
> +	-f $ovf_file or die "$ovf_file: non-existent or non-regular file\n";

see my comment in the ImportDisk series

> +	my $storecfg = PVE::Storage::config();
> +	PVE::Storage::storage_check_enabled($storecfg, $storeid);
> +
> +	my $parsed = PVE::QemuServer::OVF::parse_ovf($ovf_file);
> +
> +	if ($dryrun) {
> +	    print Dumper($parsed);
> +	    exit(0);
> +	}
> +
> +	$param->{name} = $parsed->{qm}->{name} if defined($parsed->{qm}->{name});
> +	$param->{memory} = $parsed->{qm}->{memory} if defined($parsed->{qm}->{memory});
> +	$param->{cores} = $parsed->{qm}->{cores} if defined($parsed->{qm}->{cores});
> +	$param->{node} = $nodename;
> +
> +	PVE::API2::Qemu->create_vm($param);
> +
> +	foreach my $disk (@{ $parsed->{disks} }) {
> +	    my ($file, $drive) = ($disk->{backing_file}, $disk->{disk_address});
> +	    PVE::QemuServer::ImportDisk::do_import($file, $vmid, $storeid, { drive_name => $drive, format => $format });
> +	}
> +    }
> +});

wouldn't it make more sense to skip the regular create_vm API call?

you basically only use the $createfn part of it, which in this case
boils down to

lock config
check ID is unused
generate smbios UUID
write config

by skipping the create_vm API, you could also keep holding the flock
while importing disks (which could take a while, is neither guarded by a
config level lock nor by an flock, and is completely invisble on the
GUI...). alternatively, we could downgrade the flock to a config level
lock before starting the import - but I am not sure whether any of the
existing lock values are a good fit, and introducing a new one just for
this seems wrong. maybe it would make sense to switch over all of the
create/restore/import API paths to use a new "create" lock instead of
holding the flock for the whole duration?

last but not least, this is missing error handling - at least for the
last part of importing disks? not sure how we want to handle if one of X
disks fails to import? rolling back the whole import is probably the
cleanest..

>  
>  my $print_agent_result = sub {
>      my ($data) = @_;
> @@ -638,6 +704,9 @@ our $cmddef = {
>      terminal => [ __PACKAGE__, 'terminal', ['vmid']],
>  
>      importdisk => [ __PACKAGE__, 'importdisk', ['vmid', 'source', 'storage']],
> +
> +    importovf => [ __PACKAGE__, 'importovf', ['vmid', 'manifest', 'storage']],
> +
>  };
>  
>  1;
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel