[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