Re: [pve-devel] [PATCH manager 3/4] api2/cluster: add 'metricserver' api endpoints

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 20.11.20 10:50, Dominik Csapak wrote:
> modeled after our typical api endpoints for sectionschema configs
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  PVE/API2/Cluster.pm            |   7 +
>  PVE/API2/Makefile              |   1 +
>  PVE/API2/MetricServerConfig.pm | 238 +++++++++++++++++++++++++++++++++
>  3 files changed, 246 insertions(+)
>  create mode 100644 PVE/API2/MetricServerConfig.pm
> 
> diff --git a/PVE/API2/Cluster.pm b/PVE/API2/Cluster.pm
> index e768cbc6..4eb387b1 100644
> --- a/PVE/API2/Cluster.pm
> +++ b/PVE/API2/Cluster.pm
> @@ -29,6 +29,7 @@ use PVE::API2::ClusterConfig;
>  use PVE::API2::Firewall::Cluster;
>  use PVE::API2::HAConfig;
>  use PVE::API2::ReplicationConfig;
> +use PVE::API2::MetricServerConfig;
>  
>  my $have_sdn;
>  eval {
> @@ -43,6 +44,11 @@ __PACKAGE__->register_method ({
>      path => 'replication',
>  });
>  
> +__PACKAGE__->register_method ({
> +    subclass => "PVE::API2::MetricServerConfig",
> +    path => 'metricserver',
> +});
> +
>  __PACKAGE__->register_method ({
>      subclass => "PVE::API2::ClusterConfig",
>      path => 'config',
> @@ -132,6 +138,7 @@ __PACKAGE__->register_method ({
>  	    { name => 'config' },
>  	    { name => 'acme' },
>  	    { name => 'ceph' },
> +	    { name => 'metricserver' },

I'd like to use a folder here,
'metric/server'

>  	    ];
>  
>  	if ($have_sdn) {
> diff --git a/PVE/API2/Makefile b/PVE/API2/Makefile
> index bc5ccc36..fdadbc40 100644
> --- a/PVE/API2/Makefile
> +++ b/PVE/API2/Makefile
> @@ -24,6 +24,7 @@ PERLSOURCE = 			\
>  	NodeConfig.pm		\
>  	Scan.pm			\
>  	Hardware.pm		\
> +	MetricServerConfig.pm	\
>  	Services.pm
>  
>  all:
> diff --git a/PVE/API2/MetricServerConfig.pm b/PVE/API2/MetricServerConfig.pm

please move this inside the Cluster folder, no need to further accumulate most
stuff at the top API2 directory.

> new file mode 100644
> index 00000000..6d4df628
> --- /dev/null
> +++ b/PVE/API2/MetricServerConfig.pm
> @@ -0,0 +1,238 @@
> +package PVE::API2::MetricServerConfig;
> +
> +use warnings;
> +use strict;
> +
> +use PVE::Tools qw(extract_param);
> +use PVE::Exception qw(raise_perm_exc raise_param_exc);
> +use PVE::JSONSchema qw(get_standard_option);
> +use PVE::RPCEnvironment;
> +use PVE::ExtMetric;
> +
> +use PVE::RESTHandler;
> +
> +use base qw(PVE::RESTHandler);
> +
> +__PACKAGE__->register_method ({
> +    name => 'index',
> +    path => '',
> +    method => 'GET',
> +    description => "List configured metric servers.",
> +    permissions => {
> +	check => ['perm', '/', ['Sys.Audit']],
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {},
> +    },
> +    returns => {
> +	type => 'array',
> +	items => {
> +	    type => "object",
> +	    properties => {
> +		id => {
> +		    description => "The ID of the entry.",
> +		    type => 'string'
> +		},
> +		disable => {
> +		    description => "Flag to disable the plugin.",
> +		    type => 'boolean',
> +		},
> +		type => {
> +		    description => "Plugin type.",
> +		    type => 'string',
> +		},
> +		server => {
> +		    description => "Server dns name or IP address",
> +		    type => 'string',
> +		},
> +		port => {
> +		    description => "Server network port",
> +		    type => 'integer',
> +		},

nit (can be done as follow up) could we reuse the createSchema here?
A returnSchema for SectionConfig could make sense in general.

> +	    },
> +	},
> +	links => [ { rel => 'child', href => "{id}" } ],
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $res = [];
> +	my $status_cfg = PVE::Cluster::cfs_read_file('status.cfg');
> +
> +	for my $id (keys %{$status_cfg->{ids}}) {

please sort the keys above, so the returned array stays stable.

> +	    my $plugin_config = $status_cfg->{ids}->{$id};
> +	    push @$res, {
> +		id => $id,
> +		disable => $plugin_config->{disable} // 0,
> +		type => $plugin_config->{type},
> +		server => $plugin_config->{server},
> +		port => $plugin_config->{port},
> +	    };
> +	}
> +
> +	return $res;
> +    }});
> +
> +__PACKAGE__->register_method ({
> +    name => 'read',
> +    path => '{id}',
> +    method => 'GET',
> +    description => "Read replication job configuration.",

But this ain't no "replication" job? Please re-check for copy-is-my-hobby leftovers ;-)

> +    permissions => {
> +	check => ['perm', '/', ['Sys.Audit']],
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    id => {
> +		type => 'string',
> +		format => 'pve-configid',
> +	    },
> +	},
> +    },
> +    returns => { type => 'object' },

this could possibly use the createSchema - with most set as optional? (or a future return schema)

> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $status_cfg = PVE::Cluster::cfs_read_file('status.cfg');

I do not remember anymore, is this OK without locking?

> +	my $id = $param->{id};
> +
> +	if (!defined($status_cfg->{ids}->{$id})) {
> +	    die "status server entry '$id' does not exist\n";
> +	}
> +
> +	return $status_cfg->{ids}->{$id};
> +    }});
> +
> +__PACKAGE__->register_method ({
> +    name => 'create',
> +    path => '',
> +    protected => 1,
> +    method => 'POST',
> +    description => "Create a new external metric server config",
> +    permissions => {
> +	check => ['perm', '/', ['Sys.Modify']],
> +    },
> +    parameters => PVE::Status::Plugin->createSchema(),
> +    returns => { type => 'null' },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $type = extract_param($param, 'type');
> +	my $plugin = PVE::Status::Plugin->lookup($type);
> +	my $id = extract_param($param, 'id');
> +
> +	my $code = sub {
> +	    my $cfg = PVE::Cluster::cfs_read_file('status.cfg');
> +
> +	    die "Metric server '$id' already exists\n"
> +		if $cfg->{ids}->{$id};
> +
> +	    my $opts = $plugin->check_config($id, $param, 1, 1);
> +	    $cfg->{ids}->{$id} = $opts;
> +
> +	    PVE::Cluster::cfs_write_file('status.cfg', $cfg);
> +	};
> +
> +	PVE::Cluster::cfs_lock_file('status.cfg', undef, $code);

I'd rather like if that over general $code variable was dropped and passed directly:

PVE::Cluster::cfs_lock_file('status.cfg', undef, sub {
    ...
});

> +	die $@ if $@;
> +
> +	return undef;

nit, undef is not needed, `return;` is enough (and a little bit less noise)

> +    }});
> +
> +
> +__PACKAGE__->register_method ({
> +    name => 'update',
> +    protected => 1,
> +    path => '{id}',
> +    method => 'PUT',
> +    description => "Update metric server configuration.",
> +    permissions => {
> +	check => ['perm', '/', ['Sys.Modify']],
> +    },
> +    parameters => PVE::Status::Plugin->updateSchema(),
> +    returns => { type => 'null' },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $id = extract_param($param, 'id');
> +	my $digest = extract_param($param, 'digest');
> +	my $delete = extract_param($param, 'delete');
> +
> +	my $code = sub {
> +	    my $cfg = PVE::Cluster::cfs_read_file('status.cfg');
> +
> +	    PVE::SectionConfig::assert_if_modified($cfg, $digest);
> +
> +	    my $data = $cfg->{ids}->{$id};
> +	    die "no such server '$id'\n" if !$data;
> +
> +	    my $plugin = PVE::Status::Plugin->lookup($data->{type});
> +	    my $opts = $plugin->check_config($id, $param, 0, 1);
> +
> +	    foreach my $k (%$opts) {

this is wrong, you need to use explicitly (keys %$opts) or you loop over both,
keys and values!

nit: would be good to keep it consistent when adding a brand new module,
for vs. foreach here.

> +		$data->{$k} = $opts->{$k};
> +	    }
> +
> +	    if ($delete) {
> +		my $options = $plugin->private()->{options}->{$data->{type}};



> +		foreach my $k (PVE::Tools::split_list($delete)) {
> +		    my $d = $options->{$k} ||
> +			die "no such option '$k'\n";
> +		    die "unable to delete required option '$k'\n"
> +			if !$d->{optional};
> +		    die "unable to delete fixed option '$k'\n"
> +			if $d->{fixed};

those seem all short enough, so we could avoid the newlines but add an extra one
before below delete operation - for readability.

> +		    delete $data->{$k};
> +		}
> +	    }
> +
> +	    PVE::Cluster::cfs_write_file('status.cfg', $cfg);
> +	};
> +
> +	PVE::Cluster::cfs_lock_file('status.cfg', undef, $code);

same as above regarding $code 

> +	die $@ if $@;
> +
> +	return undef;
> +    }});
> +
> +__PACKAGE__->register_method ({
> +    name => 'delete',
> +    protected => 1,
> +    path => '{id}',
> +    method => 'DELETE',
> +    description => "Remove Metric server.",
> +    permissions => {
> +	check => ['perm', '/', ['Sys.Modify']],
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    id => {
> +		type => 'string',
> +		format => 'pve-configid',
> +	    },
> +	}
> +    },
> +    returns => { type => 'null' },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $rpcenv = PVE::RPCEnvironment::get();

above is unused

> +
> +	my $code = sub {
> +	    my $cfg = PVE::Cluster::cfs_read_file('status.cfg');
> +
> +	    my $id = $param->{id};
> +	    delete $cfg->{ids}->{$id};
> +	    PVE::Cluster::cfs_write_file('status.cfg', $cfg);
> +	};
> +
> +	PVE::Cluster::cfs_lock_file('status.cfg', undef, $code);

same as above

> +	die $@ if $@;
> +
> +	return undef;
> +    }});
> +
> +1;
>