[pve-devel] [PATCH 3/3] add qmimport command

Alexandre DERUMIER aderumier at odiso.com
Mon Jan 23 08:44:31 CET 2017


>>I don't think this is needed as standalone binary, instead 

>># qm import_disk <vmid> <image> <storage> [-format] 

ok. (the standalone binary was just in case it couldn't be include upstream and maintainded outside)


>>should be enough.. I think using something like "path" or "filename" 
>>instead of "image" might be clearer? 

yes, maybe filename

>>there is one high-level issue that I am not sure how to address, except 
>>maybe as part of the description / documentation, since there's no sane 
>>way to detect it: 

>>the image that is imported must not be in use by another VM at the time 
>>of import, because then you will most likely get a garbage copy. it 
>>should probably never affect the source image and the running VM, but I 
>>am not 100% sure whether that is true for all possible storage setups. 

yes. I thinked about that. Maybe simply exclude directory defined in proxmox storage ?



>>anyway, here's my counter proposal: 
>>
>>don't 
>>- use PVE/API2/Qemu.pm's move_disk API call as base, but implement from 
>>scratch to drop all the unneeded cruft (see some examples inline) 
>>- patch clone_disk to accept paths as source 
>>- patch qemu_convert_disk to accept paths as source 

>>instead, in your new CLI method: 
>>- get the needed size from your source disk 
>>- allocate an image of the needed size (with PVE::Storage::vdisk_alloc) 
>>- get target image's path and whether it's possible to sparseinit 
>>- activate target 
>>- call qemu-img convert 
>>- deactivate target 
>>- add target image as unused 

>>optionally 
>>- split out the part of qemu_convert_disk which builds the "qemu-img 
>>convert" commandline and parses it's output, after the whole storage 
>>and config parsing is done, and reuse it 

>>I am not sure whether that last part is actually worth it.. 

>>the result should be smaller and cleaner than your proposal, without the 
>>need to change any of the existing methods. I think this makes more 
>>sense since you only wanted to use a small part of their functionality 
>>anyway.. 
>>
>>If I missed something or something is unclear or does not work as 
>>expected, just shout ;)

Ok,I'll send a patch this way. I just wanted to avoid as much as possible duplicate code.


----- Mail original -----
De: "Fabian Grünbichler" <f.gruenbichler at proxmox.com>
À: "pve-devel" <pve-devel at pve.proxmox.com>
Cc: "aderumier" <aderumier at odiso.com>
Envoyé: Vendredi 20 Janvier 2017 14:46:49
Objet: Re: [pve-devel] [PATCH 3/3] add qmimport command

On Fri, Jan 20, 2017 at 01:21:43AM +0100, Alexandre Derumier wrote: 
> This allow to import an external disk in a vm. 
> 
> qmimport <vmid> <filename> <storage> 

I don't think this is needed as standalone binary, instead 

# qm import_disk <vmid> <image> <storage> [-format] 

should be enough.. I think using something like "path" or "filename" 
instead of "image" might be clearer? 

there is one high-level issue that I am not sure how to address, except 
maybe as part of the description / documentation, since there's no sane 
way to detect it: 

the image that is imported must not be in use by another VM at the time 
of import, because then you will most likely get a garbage copy. it 
should probably never affect the source image and the running VM, but I 
am not 100% sure whether that is true for all possible storage setups. 


anyway, here's my counter proposal: 

don't 
- use PVE/API2/Qemu.pm's move_disk API call as base, but implement from 
scratch to drop all the unneeded cruft (see some examples inline) 
- patch clone_disk to accept paths as source 
- patch qemu_convert_disk to accept paths as source 

instead, in your new CLI method: 
- get the needed size from your source disk 
- allocate an image of the needed size (with PVE::Storage::vdisk_alloc) 
- get target image's path and whether it's possible to sparseinit 
- activate target 
- call qemu-img convert 
- deactivate target 
- add target image as unused 

optionally 
- split out the part of qemu_convert_disk which builds the "qemu-img 
convert" commandline and parses it's output, after the whole storage 
and config parsing is done, and reuse it 

I am not sure whether that last part is actually worth it.. 

the result should be smaller and cleaner than your proposal, without the 
need to change any of the existing methods. I think this makes more 
sense since you only wanted to use a small part of their functionality 
anyway.. 

If I missed something or something is unclear or does not work as 
expected, just shout ;) 

> 
> Signed-off-by: Alexandre Derumier <aderumier at odiso.com> 
> --- 
> Makefile | 1 + 
> PVE/CLI/Makefile | 2 +- 
> PVE/CLI/qmimport.pm | 140 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 
> qmimport | 8 +++ 
> 4 files changed, 150 insertions(+), 1 deletion(-) 
> create mode 100755 PVE/CLI/qmimport.pm 
> create mode 100755 qmimport 
> 
> diff --git a/Makefile b/Makefile 
> index 6a53361..f9a4232 100644 
> --- a/Makefile 
> +++ b/Makefile 
> @@ -73,6 +73,7 @@ install: ${PKGSOURCES} 
> make -C PVE install 
> install -m 0755 qm ${DESTDIR}${SBINDIR} 
> install -m 0755 qmrestore ${DESTDIR}${SBINDIR} 
> + install -m 0755 qmimport ${DESTDIR}${SBINDIR} 
> install -m 0755 pve-bridge ${DESTDIR}${VARLIBDIR}/pve-bridge 
> install -m 0755 pve-bridge-hotplug ${DESTDIR}${VARLIBDIR}/pve-bridge-hotplug 
> install -m 0755 pve-bridgedown ${DESTDIR}${VARLIBDIR}/pve-bridgedown 
> diff --git a/PVE/CLI/Makefile b/PVE/CLI/Makefile 
> index 2fec8e5..55880fa 100644 
> --- a/PVE/CLI/Makefile 
> +++ b/PVE/CLI/Makefile 
> @@ -1,4 +1,4 @@ 
> -SOURCES=qm.pm qmrestore.pm 
> +SOURCES=qm.pm qmrestore.pm qmimport.pm 
> 
> .PHONY: install 
> install: ${SOURCES} 
> diff --git a/PVE/CLI/qmimport.pm b/PVE/CLI/qmimport.pm 
> new file mode 100755 
> index 0000000..92dc314 
> --- /dev/null 
> +++ b/PVE/CLI/qmimport.pm 
> @@ -0,0 +1,140 @@ 
> +package PVE::CLI::qmimport; 
> + 
> +use strict; 
> +use warnings; 
> +use PVE::SafeSyslog; 
> +use PVE::Tools qw(extract_param); 
> +use PVE::INotify; 
> +use PVE::RPCEnvironment; 
> +use PVE::CLIHandler; 
> +use PVE::JSONSchema qw(get_standard_option); 
> +use PVE::Cluster; 
> +use PVE::QemuServer; 
> +use PVE::API2::Qemu; 
> + 
> +use base qw(PVE::CLIHandler); 
> + 
> +sub setup_environment { 
> + PVE::RPCEnvironment->setup_default_cli_env(); 
> +} 
> + 
> +__PACKAGE__->register_method({ 
> + name => 'qmimport', 
> + path => 'qmimport', 

trailing white space 

> + method => 'POST', 
> + description => "Import external volume.", 
> + parameters => { 
> + additionalProperties => 0, 
> + properties => { 
> + vmid => get_standard_option('pve-vmid', { completion => \&PVE::QemuServer::complete_vmid }), 
> + image => { 
> + type => 'string', 
> + description => "The disk image you want to import.", 
> + }, 
> + storage => get_standard_option('pve-storage-id', { 
> + description => "Target storage.", 
> + completion => \&PVE::QemuServer::complete_storage, 
> + }), 
> + 'format' => { 
> + type => 'string', 
> + description => "Target Format.", 
> + enum => [ 'raw', 'qcow2', 'vmdk' ], 
> + optional => 1, 
> + }, 
> + digest => { 
> + type => 'string', 
> + description => 'Prevent changes if current configuration file has different SHA1 digest. This can be used to prevent concurrent !modifications.', 
> + maxLength => 40, 
> + optional => 1, 
> + }, 

we never use digest for CLI methods. 

> + }, 
> + }, 
> + returns => { 
> + type => 'string', 
> + description => "the task ID.", 
> + }, 

also not needed for CLI methods? 

> + code => sub { 
> + my ($param) = @_; 
> + 
> + my $rpcenv = PVE::RPCEnvironment::get(); 
> + 
> + my $authuser = $rpcenv->get_user(); 
> + 
> + raise_param_exc({ importdisk => "Only root may use this option." }) 
> + if $authuser ne 'root at pam'; 
> + 

not needed for CLI methods with the default RPCEnvironment set up. 

> + 
> + my $vmid = extract_param($param, 'vmid'); 
> + 
> + my $digest = extract_param($param, 'digest'); 

see above 

> + 
> + my $image = extract_param($param, 'image'); 
> + 
> + my $storeid = extract_param($param, 'storage'); 
> + 
> + my $format = extract_param($param, 'format'); 
> + 
> + my $storecfg = PVE::Storage::config(); 
> + 
> + my $drive = {}; 
> + $drive->{file} = $image; 
> + 
> + my $updatefn = sub { 
> + 
> + my $conf = PVE::QemuConfig->load_config($vmid); 
> + 
> + PVE::QemuConfig->check_lock($conf); 
> + 
> + die "checksum missmatch (file change by other user?)\n" 
> + if $digest && $digest ne $conf->{digest}; 
> + 
> + die "disk image '$image' does not exist\n" if !-e $image; 
> + 
> + PVE::Cluster::log_msg('info', $authuser, "import disk image VM $vmid: import --image $image --storage $storeid"); 
> + 
> + my $realcmd = sub { 
> + 
> + my $newvollist = []; 
> + 
> + eval { 
> + local $SIG{INT} = $SIG{TERM} = $SIG{QUIT} = $SIG{HUP} = sub { die "interrupted by signal\n"; }; 
> + 
> + 
> + my $newdrive = PVE::QemuServer::clone_disk($storecfg, $vmid, undef, "", $drive, undef, 
> + $vmid, $storeid, $format, 1, $newvollist); 
> + 
> + PVE::QemuConfig->add_unused_volume($conf, $newdrive->{file}); 
> + 
> + PVE::QemuConfig->write_config($vmid, $conf); 
> + 
> + eval { 
> + # try to deactivate volumes - avoid lvm LVs to be active on several nodes 
> + PVE::Storage::deactivate_volumes($storecfg, [ $newdrive->{file} ]); 
> + }; 
> + warn $@ if $@; 
> + }; 
> + if (my $err = $@) { 
> + 
> + foreach my $volid (@$newvollist) { 
> + eval { PVE::Storage::vdisk_free($storecfg, $volid); }; 
> + warn $@ if $@; 
> + } 
> + die "import disk image failed: $err"; 
> + } 
> + 
> + }; 
> + 
> + return $rpcenv->fork_worker('qmimportdisk', $vmid, $authuser, $realcmd); 
> + }; 
> + 
> + return PVE::QemuConfig->lock_config($vmid, $updatefn); 
> + }}); 
> + 
> +our $cmddef = [ __PACKAGE__, 'qmimport', ['vmid', 'image', 'storage'], undef, 
> + sub { 
> + my $upid = shift; 
> + my $status = PVE::Tools::upid_read_status($upid); 
> + exit($status eq 'OK' ? 0 : -1); 
> + }]; 

this does not make sense at all - you are wrapping your output although 
there is nothing else using it? I think this was copied from qmrestore, 
where it does actually make a little sense ;) 

> + 
> +1; 
> diff --git a/qmimport b/qmimport 
> new file mode 100755 
> index 0000000..114364d 
> --- /dev/null 
> +++ b/qmimport 
> @@ -0,0 +1,8 @@ 
> +#!/usr/bin/perl 
> + 
> +use strict; 
> +use warnings; 
> + 
> +use PVE::CLI::qmimport; 
> + 
> +PVE::CLI::qmimport->run_cli_handler(); 
> -- 
> 2.1.4 
> 
> _______________________________________________ 
> pve-devel mailing list 
> pve-devel at pve.proxmox.com 
> http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel 




More information about the pve-devel mailing list