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

Fabian Grünbichler f.gruenbichler at proxmox.com
Fri Jan 20 14:46:49 CET 2017


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