Re: [pve-devel] [PATCH manager v2 2/8] ceph: add get api call for single pool

[Dominik Csapak <d.csapak@proxmox.com>]

comments inline

On 11/24/20 11:58 AM, Alwin Antreich wrote:
> Information of a single pool can be queried.
> 
> Signed-off-by: Alwin Antreich <a.antreich@proxmox.com>
> ---
>   PVE/API2/Ceph/POOLS.pm | 113 +++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 108 insertions(+), 5 deletions(-)
> 
> diff --git a/PVE/API2/Ceph/POOLS.pm b/PVE/API2/Ceph/POOLS.pm
> index 744f2bce..19fc1b7e 100644
> --- a/PVE/API2/Ceph/POOLS.pm
> +++ b/PVE/API2/Ceph/POOLS.pm
> @@ -18,11 +18,6 @@ use base qw(PVE::RESTHandler);
>   my $ceph_pool_common_options = sub {
>       my ($nodefault) = shift;
>       my $options = {
> -	name => {
> -	    title => 'Name',
> -	    description => "The name of the pool. It must be unique.",
> -	    type => 'string',
> -	},

why do you take that info here out?
on every instance where you use the ceph_pool_common_options you
add it again apart from the one return values
(where you could remove it conditionally, like with 'nodefault')

>   	size => {
>   	    description => 'Number of replicas per object',
>   	    title => 'Size',
> @@ -218,6 +213,11 @@ __PACKAGE__->register_method ({
>   	additionalProperties => 0,
>   	properties => {
>   	    node => get_standard_option('pve-node'),
> +	    name => {
> +		title => 'Name',
> +		description => "The name of the pool. It must be unique.",
> +		type => 'string',
> +	    },
>   	    add_storages => {
>   		description => "Configure VM and CT storage using the new pool.",
>   		type => 'boolean',
> @@ -374,6 +374,11 @@ __PACKAGE__->register_method ({
>   	additionalProperties => 0,
>   	properties => {
>   	    node => get_standard_option('pve-node'),
> +	    name => {
> +		title => 'Name',
> +		description => "The name of the pool. It must be unique.",
> +		type => 'string',
> +	    },
>   	    %{ $ceph_pool_common_options->('nodefault') },
>   	},
>       },
> @@ -401,4 +406,102 @@ __PACKAGE__->register_method ({
>       }});
>   
>   
> +__PACKAGE__->register_method ({
> +    name => 'getpool',
> +    path => '{name}',
> +    method => 'GET',
> +    description => "List pool settings.",
> +    proxyto => 'node',
> +    protected => 1,
> +    permissions => {
> +	check => ['perm', '/', [ 'Sys.Audit', 'Datastore.Audit' ], any => 1],
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    name => {
> +		description => "The name of the pool. It must be unique.",
> +		type => 'string',
> +	    },
> +	    verbose => {
> +		type => 'boolean',
> +		default => 0,
> +		optional => 1,
> +		description => "If enabled, will display additional data".
> +		    "(eg. statistics).",
> +	    },
> +	},
> +    },
> +    returns => {
> +	type => "object",
> +	properties => {
> +	    id                     => { type => 'integer', title => 'ID' },
> +	    pgp_num                => { type => 'integer', title => 'PGP num' },
> +	    noscrub                => { type => 'boolean', title => 'noscrub' },
> +	    'nodeep-scrub'         => { type => 'boolean', title => 'nodeep-scrub' },
> +	    nodelete               => { type => 'boolean', title => 'nodelete' },
> +	    nopgchange             => { type => 'boolean', title => 'nopgchange' },
> +	    nosizechange           => { type => 'boolean', title => 'nosizechange' },
> +	    write_fadvise_dontneed => { type => 'boolean', title => 'write_fadvise_dontneed' },
> +	    hashpspool             => { type => 'boolean', title => 'hashpspool' },
> +	    use_gmt_hitset         => { type => 'boolean', title => 'use_gmt_hitset' },
> +	    fast_read              => { type => 'boolean', title => 'Fast Read' },
> +	    statistics             => { type => 'object', title => 'Statistics', optional => 1 },
> +	    %{ $ceph_pool_common_options->() },
> +	},
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	PVE::Ceph::Tools::check_ceph_inited();
> +
> +	my $verbose = $param->{verbose};
> +	my $pool = $param->{name};
> +
> +	my $rados = PVE::RADOS->new();
> +	my $res = $rados->mon_command({
> +		prefix => 'osd pool get',
> +		pool   => "$pool",
> +		var    => 'all',
> +	    });
> +
> +	my $data = {
> +	    id                     => $res->{pool_id},
> +	    size                   => $res->{size},
> +	    min_size               => $res->{min_size},
> +	    pg_num                 => $res->{pg_num},
> +	    pgp_num                => $res->{pgp_num},
> +	    crush_rule             => $res->{crush_rule},
> +	    pg_autoscale_mode      => $res->{pg_autoscale_mode},
> +	    noscrub                => "$res->{noscrub}",
> +	    'nodeep-scrub'         => "$res->{'nodeep-scrub'}",
> +	    nodelete               => "$res->{nodelete}",
> +	    nopgchange             => "$res->{nopgchange}",
> +	    nosizechange           => "$res->{nosizechange}",
> +	    write_fadvise_dontneed => "$res->{write_fadvise_dontneed}",
> +	    hashpspool             => "$res->{hashpspool}",
> +	    use_gmt_hitset         => "$res->{use_gmt_hitset}",
> +	    fast_read              => "$res->{fast_read}",
> +	};
> +
> +	if ($verbose) {
> +	    my $stats;
> +	    my $res = $rados->mon_command({ prefix => 'df' });
> +
> +	    foreach my $d (@{$res->{pools}}) {
> +		next if !$d->{stats};
> +		next if !defined($d->{name}) && !$d->{name} ne "$pool";
> +		$data->{statistics} = $d->{stats};
> +	    }
> +
> +	    my $apps = $rados->mon_command({ prefix => "osd pool application get", pool => "$pool", });
> +	    my @application = keys %$apps;
> +	    $data->{application} = $application[0];

why only the first application? can there be more than one?

> +	}
> +
> +	return $data;
> +    }});
> +
> +
>   1;
>