[pve-devel] [PATCH container 3/3] bwlimit: add parameter to API2 calls

[Thomas Lamprecht]

On 3/29/19 8:28 AM, Stoiko Ivanov wrote:
> for migrate_vm, clone_vm and move_volume
> 

some nits, and as for a change an actual real issue at the bottom :-)

> Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
> ---
>  src/PVE/API2/LXC.pm | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 6de121f..35e6851 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -987,6 +987,12 @@ __PACKAGE__->register_method({
>  		    " mounts. NOTE: deprecated, use 'shared' property of mount point instead.",
>  		optional => 1,
>  	    },
> +	    bwlimit => {
> +		description => "Override i/o bandwidth limit (in KiB/s).",

I/O

> +		optional => 1,
> +		type => 'number',
> +		minimum => '0',
> +	    },
>  	},
>      },
>      returns => {
> @@ -1250,6 +1256,12 @@ __PACKAGE__->register_method({
>  		description => "Target node. Only allowed if the original VM is on shared storage.",
>  		optional => 1,
>  	    }),
> +	    bwlimit => {
> +		description => "Override i/o bandwidth limit (in KiB/s).",

I/O

also, we always speak about "override a limit", but nowhere we talk about
what we override. Maybe this wording could be improved, semi-related to this
I'd add a "default" key here, something like:

default => 'cluster wide clone limit',

or respectively:
default => 'cluster wide migrate limit',


..., this could already help with making it clear what this overrides

> +		optional => 1,
> +		type => 'number',
> +		minimum => '0',
> +	    },
>          },
>      },
>      returns => {
> @@ -1434,6 +1446,7 @@ __PACKAGE__->register_method({
>  		    local $SIG{HUP} = sub { die "interrupted by signal\n"; };
>  
>  		PVE::Storage::activate_volumes($storecfg, $vollist, $snapname);
> +		my $bwlimit = extract_param($param, 'bwlimit');
>  
>  		foreach my $opt (keys %$mountpoints) {
>  		    my $mp = $mountpoints->{$opt};
> @@ -1442,8 +1455,10 @@ __PACKAGE__->register_method({
>  		    my $newvolid;
>  		    if ($fullclone->{$opt}) {
>  			print "create full clone of mountpoint $opt ($volid)\n";
> -			my $target_storage = $storage // PVE::Storage::parse_volume_id($volid);
> -			$newvolid = PVE::LXC::copy_volume($mp, $newid, $target_storage, $storecfg, $newconf, $snapname);
> +			my $source_storage = PVE::Storage::parse_volume_id($volid);
> +			my $target_storage = $storage // $source_storage;
> +			my $clonelimit = PVE::Storage::get_bandwidth_limit('clone', [$source_storage, $target_storage], $bwlimit);
> +			$newvolid = PVE::LXC::copy_volume($mp, $newid, $target_storage, $storecfg, $newconf, $snapname, $clonelimit);
>  		    } else {
>  			print "create linked clone of mount point $opt ($volid)\n";
>  			$newvolid = PVE::Storage::vdisk_clone($storecfg, $volid, $newid, $snapname);
> @@ -1690,7 +1705,13 @@ __PACKAGE__->register_method({
>  		description => 'Prevent changes if current configuration file has different SHA1 digest. This can be used to prevent concurrent modifications.',
>  		maxLength => 40,
>  		optional => 1,
> -	    }
> +	    },
> +	    bwlimit => {
> +		description => "Override i/o bandwidth limit (in KiB/s).",
> +		optional => 1,
> +		type => 'number',
> +		minimum => '0',
> +	    },
>  	},
>      },
>      returns => {
> @@ -1747,6 +1768,9 @@ __PACKAGE__->register_method({
>  
>  		eval {
>  		    PVE::Storage::activate_volumes($storage_cfg, [ $old_volid ]);
> +		    my $bwlimit = extract_param($param, 'bwlimit');
> +		    my $source_storage = PVE::Storage::parse_volume_id($old_volid);
> +		    my $movelimit = PVE::Storage::get_bandwidth_limit('move', [$source_storage, $storage], $bwlimit);

maybe also pass it to copy_volume so that it actually does something ;-)

>  		    $new_volid = PVE::LXC::copy_volume($mpdata, $vmid, $storage, $storage_cfg, $conf);
>  		    $mpdata->{volume} = $new_volid;
>  
>