[pve-devel] [PATCH 01/19] qm create : make vmid optionnal

[Thomas Lamprecht]

Hi,

I'm appreciating your effort on this topic, I try to give the patches an
initial look.

This patch as done is problematic and its title a bit misleading, you do not
make the VMID optional in qm only but in the API, that's a difference, while
it can be OK in qm it isn't in the API.
I know that because I tried to do once the same thing. :)
We try to adhere strictly to the `POST Once Exactly` [1] principle, because
POST request are most times not idempotent.

Quoting an example from [1]

  >
  > However, non-idempotent HTTP methods, namely POST, may have
  > additional side effects when the same request is sent more than once.
  > To give a common example, POST is used by Web shopping baskets; when
  > the user wishes to finalise their purchase, a POST request is sent to
  > the server which processes the credit card transaction and fills the
  > order.
  >
  > If the POST is sent twice, there is the danger of a duplicate order
  > being made.


So we do not want that this is possible:
# POST /nodes/NODE/qemu/create

We always want to POST on a specific Object, and abort if this object does
already exist, i.e.
# POST /nodes/NODE/qemu/create/VMID

But what you could do, as you want the behavior in the CLI tool not in the
API is to write a small wrapper which has an optional (or no) VMID parameter
, does the VMID preparation as in this patch and then calls the API.

Or you tell the source side that it needs to specify a target VMID when
initiating the migration.

cheers,
Thomas

[1] https://tools.ietf.org/html/draft-nottingham-http-poe-00

On 02/22/2017 02:33 PM, Alexandre Derumier wrote:
> if ommit, we generate it with PVE::Cluster::next_vmid
>
> Signed-off-by: Alexandre Derumier <aderumier at odiso.com>
> ---
>   PVE/API2/Qemu.pm | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index a077ed7..76bba70 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -375,7 +375,7 @@ __PACKAGE__->register_method({
>   	properties => PVE::QemuServer::json_config_properties(
>   	    {
>   		node => get_standard_option('pve-node'),
> -		vmid => get_standard_option('pve-vmid', { completion => \&PVE::Cluster::complete_next_vmid }),
> +		vmid => get_standard_option('pve-vmid', { completion => \&PVE::Cluster::complete_next_vmid, optional => 1 }),
>   		archive => {
>   		    description => "The backup file.",
>   		    type => 'string',
> @@ -431,12 +431,18 @@ __PACKAGE__->register_method({
>   
>   	my $pool = extract_param($param, 'pool');
>   
> -	my $filename = PVE::QemuConfig->config_file($vmid);
> -
>   	my $storecfg = PVE::Storage::config();
>   
>   	PVE::Cluster::check_cfs_quorum();
>   
> +	if (!$vmid) {
> +	    my $nextvmid = PVE::Cluster::complete_next_vmid() if !$vmid;
> +	    $vmid = @$nextvmid[0];
> +	    die "Can't generate a new vmid" if !$vmid;
> +	}
> +
> +	my $filename = PVE::QemuConfig->config_file($vmid);
> +
>   	if (defined($pool)) {
>   	    $rpcenv->check_pool_exist($pool);
>   	}
> @@ -571,6 +577,8 @@ __PACKAGE__->register_method({
>   		    die "create failed - $err";
>   		}
>   
> +		print "vm $vmid created\n";
> +
>   		PVE::AccessControl::add_vm_to_pool($vmid, $pool) if $pool;
>   	    };
>