On 16/04/2024 14:20, Fabian Grünbichler wrote:

> allow to set/update limits, and return them when querying individual pools.

> 

> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

> ---

> 

> Notes:

>     requires bumped pve-access-control

>     

>     v2:

>     - unify run vs config limit checks into helper

>     - avoid hard-coding resource kinds

> 

>  PVE/API2/Pool.pm | 50 ++++++++++++++++++++++++++++++++++++++++++++----

>  1 file changed, 46 insertions(+), 4 deletions(-)

> 

> diff --git a/PVE/API2/Pool.pm b/PVE/API2/Pool.pm

> index 54e744558..031f0160f 100644

> --- a/PVE/API2/Pool.pm

> +++ b/PVE/API2/Pool.pm

> @@ -7,6 +7,7 @@ use PVE::AccessControl;

>  use PVE::Cluster qw (cfs_read_file cfs_write_file);

>  use PVE::Exception qw(raise_param_exc);

>  use PVE::INotify;

> +use PVE::JSONSchema qw(get_standard_option parse_property_string print_property_string);

>  use PVE::Storage;

>  

>  use PVE::SafeSyslog;

> @@ -47,10 +48,7 @@ __PACKAGE__->register_method ({

>  	    type => "object",

>  	    properties => {

>  		poolid => { type => 'string' },

> -		comment => {

> -		    type => 'string',

> -		    optional => 1,

> -		},

> +		comment => { type => 'string', optional => 1 },

>  		members => {

>  		    type => 'array',

>  		    optional => 1,

> @@ -79,6 +77,7 @@ __PACKAGE__->register_method ({

>  			},

>  		    },

>  		},

> +		limits => { type => 'string', optional => 1 },

>  	    },

>  	},

>  	links => [ { rel => 'child', href => "{poolid}" } ],

> @@ -136,6 +135,8 @@ __PACKAGE__->register_method ({

>  		members => $members,

>  	    };

>  	    $pool_info->{comment} = $pool_config->{comment} if defined($pool_config->{comment});

> +	    $pool_info->{limits} = print_property_string($pool_config->{limits}, 'pve-pool-limits')

> +		if defined($pool_config->{limits});

>  	    $pool_info->{poolid} = $poolid;

>  

>  	    push @$res, $pool_info;

> @@ -153,6 +154,26 @@ __PACKAGE__->register_method ({

>  	return $res;

>      }});

>  

> +my $check_run_vs_config_limits = sub {

> +    my ($limits) = @_;

> +

> +    my $exception;

> +

> +    for my $resource (keys $limits->%*) {

> +	next if $resource !~ m/-run$/;

> +

> +	my $config = $resource;

> +	$config =~ s/-run$/-config/;

> +

> +	if (defined($limits->{$config}) && $limits->{$config} < $limits->{$resource}) {

> +	    my $msg = "run limit must be below config limit!";

> +	    $exception->{$resource} = $msg;

> +	}

> +    }

> +

> +    raise_param_exc($exception) if $exception;

> +};

> +

>  __PACKAGE__->register_method ({

>      name => 'create_pool',

>      protected => 1,

> @@ -173,6 +194,7 @@ __PACKAGE__->register_method ({

>  		type => 'string',

>  		optional => 1,

>  	    },

> +	    limits => get_standard_option('pve-pool-limits'),

>  	},

>      },

>      returns => { type => 'null' },

> @@ -196,6 +218,10 @@ __PACKAGE__->register_method ({

>  	    };

>  

>  	    $usercfg->{pools}->{$pool}->{comment} = $param->{comment} if $param->{comment};

> +	    if (my $limits = parse_property_string('pve-pool-limits', $param->{limits} // '')) {

> +		$check_run_vs_config_limits->($limits);

> +		$usercfg->{pools}->{$pool}->{limits} = $limits;

> +	    }

>  

>  	    cfs_write_file("user.cfg", $usercfg);

>  	}, "create pool failed");

> @@ -288,6 +314,9 @@ __PACKAGE__->register_method ({

>  		optional => 1,

>  		default => 0,

>  	    },

> +	    limits => get_standard_option("pve-pool-limits", {

> +		description => "Update pool limits. Passing '0' for a specific limit will remove any currently configured value.",

> +	    }),

>  	},

>      },

>      returns => { type => 'null' },

> @@ -346,6 +375,18 @@ __PACKAGE__->register_method ({

>  		}

>  	    }

>  

> +	    if (my $update = parse_property_string('pve-pool-limits', $param->{limits} // '')) {



Bug: Something like a



`$pool_config->{limits} = {} if !defined($pool_config->{limits});`



is missing here. Without this, a pool created without any pool usage

limits set will not be able to add them as the limits added in `$limits`

will never be written to the reference in `$pool_config->{limits}`.



Since we already default everywhere to the values in

`pool_usage_default`, an empty hash would not overwrite any settings

that weren't set yet.



> +		my $limits = $pool_config->{limits};

> +		for my $kind (sort keys $update->%*) {

> +		    if ($update->{$kind} == 0) {

> +			delete $limits->{$kind};

> +		    } else {

> +			$limits->{$kind} = $update->{$kind};

> +		    }

> +		}

> +		$check_run_vs_config_limits->($limits);

> +	    }

> +

>  	    cfs_write_file("user.cfg", $usercfg);

>  	}, "update pools failed");

>  

> @@ -409,6 +450,7 @@ __PACKAGE__->register_method ({

>  		    },

>  		},

>  	    },

> +	    limits => get_standard_option("pve-pool-limits"),

>  	},

>      },

>      code => sub {

> -- 

> 2.39.2