From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <t.lamprecht@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by lists.proxmox.com (Postfix) with ESMTPS id AA6D9616F7
 for <pve-devel@lists.proxmox.com>; Fri, 20 Nov 2020 14:22:12 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id 95C2711CDE
 for <pve-devel@lists.proxmox.com>; Fri, 20 Nov 2020 14:21:42 +0100 (CET)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
 [212.186.127.180])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with ESMTPS id 04ABB11CD2
 for <pve-devel@lists.proxmox.com>; Fri, 20 Nov 2020 14:21:41 +0100 (CET)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
 by proxmox-new.maurer-it.com (Proxmox) with ESMTP id B493443B27
 for <pve-devel@lists.proxmox.com>; Fri, 20 Nov 2020 14:21:40 +0100 (CET)
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
 Dominik Csapak <d.csapak@proxmox.com>
References: <20201120095049.15194-1-d.csapak@proxmox.com>
 <20201120095049.15194-4-d.csapak@proxmox.com>
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
Message-ID: <d96bfca7-b540-bfce-9bf3-fd4a98b95124@proxmox.com>
Date: Fri, 20 Nov 2020 14:21:39 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:83.0) Gecko/20100101
 Thunderbird/83.0
MIME-Version: 1.0
In-Reply-To: <20201120095049.15194-4-d.csapak@proxmox.com>
Content-Type: text/plain; charset=UTF-8
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable
X-SPAM-LEVEL: Spam detection results:  0
 AWL -0.086 Adjusted score from AWL reputation of From: address
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 NICE_REPLY_A           -0.001 Looks like a legit reply (A)
 RCVD_IN_DNSWL_MED        -2.3 Sender listed at https://www.dnswl.org/,
 medium trust
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
 http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
 information. [scan.pm, hardware.pm, cluster.pm, metricserverconfig.pm,
 services.pm, nodeconfig.pm]
Subject: Re: [pve-devel] [PATCH manager 3/4] api2/cluster: add
 'metricserver' api endpoints
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Fri, 20 Nov 2020 13:22:12 -0000

On 20.11.20 10:50, Dominik Csapak wrote:
> modeled after our typical api endpoints for sectionschema configs
>=20
> 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
>=20
> 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;
> =20
>  my $have_sdn;
>  eval {
> @@ -43,6 +44,11 @@ __PACKAGE__->register_method ({
>      path =3D> 'replication',
>  });
> =20
> +__PACKAGE__->register_method ({
> +    subclass =3D> "PVE::API2::MetricServerConfig",
> +    path =3D> 'metricserver',
> +});
> +
>  __PACKAGE__->register_method ({
>      subclass =3D> "PVE::API2::ClusterConfig",
>      path =3D> 'config',
> @@ -132,6 +138,7 @@ __PACKAGE__->register_method ({
>  	    { name =3D> 'config' },
>  	    { name =3D> 'acme' },
>  	    { name =3D> 'ceph' },
> +	    { name =3D> 'metricserver' },

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

>  	    ];
> =20
>  	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 =3D 			\
>  	NodeConfig.pm		\
>  	Scan.pm			\
>  	Hardware.pm		\
> +	MetricServerConfig.pm	\
>  	Services.pm
> =20
>  all:
> diff --git a/PVE/API2/MetricServerConfig.pm b/PVE/API2/MetricServerConf=
ig.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 =3D> 'index',
> +    path =3D> '',
> +    method =3D> 'GET',
> +    description =3D> "List configured metric servers.",
> +    permissions =3D> {
> +	check =3D> ['perm', '/', ['Sys.Audit']],
> +    },
> +    parameters =3D> {
> +	additionalProperties =3D> 0,
> +	properties =3D> {},
> +    },
> +    returns =3D> {
> +	type =3D> 'array',
> +	items =3D> {
> +	    type =3D> "object",
> +	    properties =3D> {
> +		id =3D> {
> +		    description =3D> "The ID of the entry.",
> +		    type =3D> 'string'
> +		},
> +		disable =3D> {
> +		    description =3D> "Flag to disable the plugin.",
> +		    type =3D> 'boolean',
> +		},
> +		type =3D> {
> +		    description =3D> "Plugin type.",
> +		    type =3D> 'string',
> +		},
> +		server =3D> {
> +		    description =3D> "Server dns name or IP address",
> +		    type =3D> 'string',
> +		},
> +		port =3D> {
> +		    description =3D> "Server network port",
> +		    type =3D> 'integer',
> +		},

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

> +	    },
> +	},
> +	links =3D> [ { rel =3D> 'child', href =3D> "{id}" } ],
> +    },
> +    code =3D> sub {
> +	my ($param) =3D @_;
> +
> +	my $res =3D [];
> +	my $status_cfg =3D 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 =3D $status_cfg->{ids}->{$id};
> +	    push @$res, {
> +		id =3D> $id,
> +		disable =3D> $plugin_config->{disable} // 0,
> +		type =3D> $plugin_config->{type},
> +		server =3D> $plugin_config->{server},
> +		port =3D> $plugin_config->{port},
> +	    };
> +	}
> +
> +	return $res;
> +    }});
> +
> +__PACKAGE__->register_method ({
> +    name =3D> 'read',
> +    path =3D> '{id}',
> +    method =3D> 'GET',
> +    description =3D> "Read replication job configuration.",

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

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

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

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

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

> +	my $id =3D $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 =3D> 'create',
> +    path =3D> '',
> +    protected =3D> 1,
> +    method =3D> 'POST',
> +    description =3D> "Create a new external metric server config",
> +    permissions =3D> {
> +	check =3D> ['perm', '/', ['Sys.Modify']],
> +    },
> +    parameters =3D> PVE::Status::Plugin->createSchema(),
> +    returns =3D> { type =3D> 'null' },
> +    code =3D> sub {
> +	my ($param) =3D @_;
> +
> +	my $type =3D extract_param($param, 'type');
> +	my $plugin =3D PVE::Status::Plugin->lookup($type);
> +	my $id =3D extract_param($param, 'id');
> +
> +	my $code =3D sub {
> +	    my $cfg =3D PVE::Cluster::cfs_read_file('status.cfg');
> +
> +	    die "Metric server '$id' already exists\n"
> +		if $cfg->{ids}->{$id};
> +
> +	    my $opts =3D $plugin->check_config($id, $param, 1, 1);
> +	    $cfg->{ids}->{$id} =3D $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 passe=
d 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 nois=
e)

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



> +		foreach my $k (PVE::Tools::split_list($delete)) {
> +		    my $d =3D $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 ex=
tra 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=20

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

above is unused

> +
> +	my $code =3D sub {
> +	    my $cfg =3D PVE::Cluster::cfs_read_file('status.cfg');
> +
> +	    my $id =3D $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;
>=20