public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin
@ 2020-12-02  9:21 Dominik Csapak
  2020-12-02  9:21 ` [pve-devel] [PATCH common 1/1] tools: add extract_sensitive_params Dominik Csapak
                   ` (10 more replies)
  0 siblings, 11 replies; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02  9:21 UTC (permalink / raw)
  To: pve-devel

this series adds the possibility to write influxdb data via their
v2 api. For influxdb 1.x there is a forwards compatible api since 1.8.0

i am not so sure about how i integrated the http api part into the
existing influxdb plugin, another alternative would be to have
a new InfluxDB2 Plugin that does only http(s).
this way i could omit the permanent 'cfg->proto' check, but i am not
sure if we want to have a new plugin for this..

manager/storage patches depend on common, but the storage patch is not needed for
it to work

pve-common:

Dominik Csapak (1):
  tools: add extract_sensitive_params

 src/PVE/Tools.pm | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

pve-storage:

Dominik Csapak (1):
  api: storage/config: use extract_sensitive_params from tools

 PVE/API2/Storage/Config.pm | 29 ++++-------------------------
 1 file changed, 4 insertions(+), 25 deletions(-)

pve-manager:

Dominik Csapak (7):
  api: cluster/metricserver: prevent simultaneosly setting and deleting
    of property
  status/plugin: extend send/_connect/_disconnect/test_connection
  status/plugin: extend with add/update/delete hooks
  status/influxdb: implement influxdb 2.x http api
  status/influxdb: remove unnecessary comment
  ui: add necessary fields for influxdb http api
  ui: dc/MetricServerView: add onlineHelp to edit windows

 PVE/API2/Cluster/MetricServer.pm    |  41 ++++-
 PVE/ExtMetric.pm                    |   4 +-
 PVE/Status/InfluxDB.pm              | 222 ++++++++++++++++++++++++++--
 PVE/Status/Plugin.pm                |  38 ++++-
 www/manager6/dc/MetricServerView.js | 105 ++++++++++++-
 5 files changed, 375 insertions(+), 35 deletions(-)

pve-docs:

Dominik Csapak (1):
  external metrics server: extend docs to explain http api

 pve-external-metric-server.adoc | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

-- 
2.20.1





^ permalink raw reply	[flat|nested] 21+ messages in thread

* [pve-devel] [PATCH common 1/1] tools: add extract_sensitive_params
  2020-12-02  9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
@ 2020-12-02  9:21 ` Dominik Csapak
  2020-12-03  8:47   ` Thomas Lamprecht
  2020-12-03 15:52   ` [pve-devel] applied: " Thomas Lamprecht
  2020-12-02  9:21 ` [pve-devel] [PATCH storage 1/1] api: storage/config: use extract_sensitive_params from tools Dominik Csapak
                   ` (9 subsequent siblings)
  10 siblings, 2 replies; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02  9:21 UTC (permalink / raw)
  To: pve-devel

moved and generalized from pve-storage, since we'll need it
in more places

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/Tools.pm | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 4b445ea..bda236a 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -48,6 +48,7 @@ template_replace
 safe_print
 trim
 extract_param
+extract_sensitive_params
 file_copy
 get_host_arch
 O_PATH
@@ -807,6 +808,29 @@ sub extract_param {
     return $res;
 }
 
+sub extract_sensitive_params :prototype($$$) {
+    my ($param, $sensitive_list, $delete_list) = @_;
+
+    my $sensitive;
+
+    my %delete = map { $_ => 1 } ($delete_list || [])->@*;
+
+    # always extract sensitive keys, so they don't get written to the www-data readable scfg
+    for my $opt (@$sensitive_list) {
+	# First handle deletions as explicitly setting `undef`, afterwards new values may override
+	# it.
+	if (exists($delete{$opt})) {
+	    $sensitive->{$opt} = undef;
+	}
+
+	if (defined(my $value = extract_param($param, $opt))) {
+	    $sensitive->{$opt} = $value;
+	}
+    }
+
+    return $sensitive;
+}
+
 # Note: we use this to wait until vncterm/spiceterm is ready
 sub wait_for_vnc_port {
     my ($port, $family, $timeout) = @_;
-- 
2.20.1





^ permalink raw reply	[flat|nested] 21+ messages in thread

* [pve-devel] [PATCH storage 1/1] api: storage/config: use extract_sensitive_params from tools
  2020-12-02  9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
  2020-12-02  9:21 ` [pve-devel] [PATCH common 1/1] tools: add extract_sensitive_params Dominik Csapak
@ 2020-12-02  9:21 ` Dominik Csapak
  2021-01-28 16:31   ` [pve-devel] applied: " Thomas Lamprecht
  2020-12-02  9:21 ` [pve-devel] [PATCH manager 1/7] api: cluster/metricserver: prevent simultaneosly setting and deleting of property Dominik Csapak
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02  9:21 UTC (permalink / raw)
  To: pve-devel

we have a more general version there

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/API2/Storage/Config.pm | 29 ++++-------------------------
 1 file changed, 4 insertions(+), 25 deletions(-)

diff --git a/PVE/API2/Storage/Config.pm b/PVE/API2/Storage/Config.pm
index 6e23427..00abd13 100755
--- a/PVE/API2/Storage/Config.pm
+++ b/PVE/API2/Storage/Config.pm
@@ -4,7 +4,7 @@ use strict;
 use warnings;
 
 use PVE::SafeSyslog;
-use PVE::Tools qw(extract_param);
+use PVE::Tools qw(extract_param extract_sensitive_params);
 use PVE::Cluster qw(cfs_read_file cfs_write_file);
 use PVE::Storage;
 use PVE::Storage::Plugin;
@@ -112,28 +112,7 @@ __PACKAGE__->register_method ({
 	return &$api_storage_config($cfg, $param->{storage});
     }});
 
-my sub extract_sensitive_params :prototype($$) {
-    my ($param, $delete_list) = @_;
-
-    my $sensitive;
-
-    my %delete = map { $_ => 1 } ($delete_list || [])->@*;
-
-    # always extract pw and keys, so they don't get written to the www-data readable scfg
-    for my $opt (qw(password encryption-key)) {
-	# First handle deletions as explicitly setting `undef`, afterwards new values may override
-	# it.
-	if (exists($delete{$opt})) {
-	    $sensitive->{$opt} = undef;
-	}
-
-	if (defined(my $value = extract_param($param, $opt))) {
-	    $sensitive->{$opt} = $value;
-	}
-    }
-
-    return $sensitive;
-}
+my $sensitive_params = [qw(password encryption-key)];
 
 __PACKAGE__->register_method ({
     name => 'create',
@@ -182,7 +161,7 @@ __PACKAGE__->register_method ({
 	# fix me in section config create never need an empty entity.
 	delete $param->{nodes} if !$param->{nodes};
 
-	my $sensitive = extract_sensitive_params($param, []);
+	my $sensitive = extract_sensitive_params($param, $sensitive_params, []);
 
 	my $plugin = PVE::Storage::Plugin->lookup($type);
 	my $opts = $plugin->check_config($storeid, $param, 1, 1);
@@ -282,7 +261,7 @@ __PACKAGE__->register_method ({
 	    my $scfg = PVE::Storage::storage_config($cfg, $storeid);
 	    $type = $scfg->{type};
 
-	    my $sensitive = extract_sensitive_params($param, $delete);
+	    my $sensitive = extract_sensitive_params($param, $sensitive_params, $delete);
 
 	    my $plugin = PVE::Storage::Plugin->lookup($type);
 	    my $opts = $plugin->check_config($storeid, $param, 0, 1);
-- 
2.20.1





^ permalink raw reply	[flat|nested] 21+ messages in thread

* [pve-devel] [PATCH manager 1/7] api: cluster/metricserver: prevent simultaneosly setting and deleting of property
  2020-12-02  9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
  2020-12-02  9:21 ` [pve-devel] [PATCH common 1/1] tools: add extract_sensitive_params Dominik Csapak
  2020-12-02  9:21 ` [pve-devel] [PATCH storage 1/1] api: storage/config: use extract_sensitive_params from tools Dominik Csapak
@ 2020-12-02  9:21 ` Dominik Csapak
  2020-12-03  9:05   ` Thomas Lamprecht
  2020-12-02  9:21 ` [pve-devel] [PATCH manager 2/7] status/plugin: extend send/_connect/_disconnect/test_connection Dominik Csapak
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02  9:21 UTC (permalink / raw)
  To: pve-devel

like we do in other apis of section configs (e.g. storage)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/API2/Cluster/MetricServer.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/PVE/API2/Cluster/MetricServer.pm b/PVE/API2/Cluster/MetricServer.pm
index 9a14985e..ec3c7b75 100644
--- a/PVE/API2/Cluster/MetricServer.pm
+++ b/PVE/API2/Cluster/MetricServer.pm
@@ -213,6 +213,8 @@ __PACKAGE__->register_method ({
 		    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};
+		    die "cannot set and delete property '$k' at the same time!\n"
+			if defined($opts->{$k});
 
 		    delete $data->{$k};
 		}
-- 
2.20.1





^ permalink raw reply	[flat|nested] 21+ messages in thread

* [pve-devel] [PATCH manager 2/7] status/plugin: extend send/_connect/_disconnect/test_connection
  2020-12-02  9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
                   ` (2 preceding siblings ...)
  2020-12-02  9:21 ` [pve-devel] [PATCH manager 1/7] api: cluster/metricserver: prevent simultaneosly setting and deleting of property Dominik Csapak
@ 2020-12-02  9:21 ` Dominik Csapak
  2020-12-02  9:21 ` [pve-devel] [PATCH manager 3/7] status/plugin: extend with add/update/delete hooks Dominik Csapak
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02  9:21 UTC (permalink / raw)
  To: pve-devel

by providing the id or cfg to have better context in those methods
we will need that for influxdb http api

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/ExtMetric.pm     |  4 ++--
 PVE/Status/Plugin.pm | 14 +++++++-------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/PVE/ExtMetric.pm b/PVE/ExtMetric.pm
index 448d3925..3358c359 100644
--- a/PVE/ExtMetric.pm
+++ b/PVE/ExtMetric.pm
@@ -49,7 +49,7 @@ sub transactions_start {
     foreach_plug($cfg, sub {
 	my ($plugin, $id, $plugin_config) = @_;
 
-	my $connection = $plugin->_connect($plugin_config);
+	my $connection = $plugin->_connect($plugin_config, $id);
 
 	push @$transactions, {
 	    connection => $connection,
@@ -72,7 +72,7 @@ sub transactions_finish {
 	my $flush_err = $@;
 	warn "$flush_err" if $flush_err;
 
-	$plugin->_disconnect($txn->{connection});
+	$plugin->_disconnect($txn->{connection}, $txn->{cfg});
 	$txn->{connection} = undef;
 	# avoid log spam, already got a send error; disconnect would fail too
 	warn "disconnect failed: $@" if $@ && !$flush_err;
diff --git a/PVE/Status/Plugin.pm b/PVE/Status/Plugin.pm
index c3e0bec1..2fa223ca 100644
--- a/PVE/Status/Plugin.pm
+++ b/PVE/Status/Plugin.pm
@@ -70,12 +70,12 @@ sub parse_section_header {
 }
 
 sub _connect {
-    my ($class, $cfg) = @_;
+    my ($class, $cfg, $id) = @_;
     die "please implement inside plugin";
 }
 
 sub _disconnect {
-    my ($class, $connection) = @_;
+    my ($class, $connection, $cfg) = @_;
 
     $connection->close(); # overwrite if not a simple socket
 }
@@ -115,22 +115,22 @@ sub flush_data {
     return if !defined($txn->{data}) || $txn->{data} eq '';
 
     my $data = delete $txn->{data};
-    eval { $class->send($txn->{connection}, $data) };
+    eval { $class->send($txn->{connection}, $data, $txn->{cfg}) };
     die "metrics send error '$txn->{id}': $@" if $@;
 }
 
 sub send {
-    my ($class, $connection, $data) = @_;
+    my ($class, $connection, $data, $cfg) = @_;
 
     defined($connection->send($data))
 	or die "failed to send metrics: $!\n";
 }
 
 sub test_connection {
-    my ($class, $cfg) = @_;
+    my ($class, $cfg, $id) = @_;
 
-    my $conn = $class->_connect($cfg);
-    $class->_disconnect($conn);
+    my $conn = $class->_connect($cfg, $id);
+    $class->_disconnect($conn, $cfg);
 }
 
 sub update_node_status {
-- 
2.20.1





^ permalink raw reply	[flat|nested] 21+ messages in thread

* [pve-devel] [PATCH manager 3/7] status/plugin: extend with add/update/delete hooks
  2020-12-02  9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
                   ` (3 preceding siblings ...)
  2020-12-02  9:21 ` [pve-devel] [PATCH manager 2/7] status/plugin: extend send/_connect/_disconnect/test_connection Dominik Csapak
@ 2020-12-02  9:21 ` Dominik Csapak
  2020-12-02  9:21 ` [pve-devel] [PATCH manager 4/7] status/influxdb: implement influxdb 2.x http api Dominik Csapak
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02  9:21 UTC (permalink / raw)
  To: pve-devel

like we do in it for the storage section configs

we will need this to store the token for influxdbs http api

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/API2/Cluster/MetricServer.pm | 39 +++++++++++++++++++++++++++-----
 PVE/Status/Plugin.pm             | 24 ++++++++++++++++++++
 2 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/PVE/API2/Cluster/MetricServer.pm b/PVE/API2/Cluster/MetricServer.pm
index ec3c7b75..a6b99d4f 100644
--- a/PVE/API2/Cluster/MetricServer.pm
+++ b/PVE/API2/Cluster/MetricServer.pm
@@ -3,7 +3,7 @@ package PVE::API2::Cluster::MetricServer;
 use warnings;
 use strict;
 
-use PVE::Tools qw(extract_param);
+use PVE::Tools qw(extract_param extract_sensitive_params);
 use PVE::Exception qw(raise_perm_exc raise_param_exc);
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::RPCEnvironment;
@@ -152,6 +152,8 @@ __PACKAGE__->register_method ({
 	my $plugin = PVE::Status::Plugin->lookup($type);
 	my $id = extract_param($param, 'id');
 
+	my $sensitive_params = extract_sensitive_params($param, ['token'], []);
+
 	PVE::Cluster::cfs_lock_file('status.cfg', undef, sub {
 	    my $cfg = PVE::Cluster::cfs_read_file('status.cfg');
 
@@ -160,10 +162,20 @@ __PACKAGE__->register_method ({
 
 	    my $opts = $plugin->check_config($id, $param, 1, 1);
 
-	    $plugin->test_connection($opts);
-
 	    $cfg->{ids}->{$id} = $opts;
 
+	    $plugin->on_add_hook($id, $opts, $sensitive_params);
+
+	    eval {
+		$plugin->test_connection($opts, $id);
+	    };
+
+	    if (my $err = $@) {
+		eval { $plugin->on_delete_hook($id, $opts) };
+		warn "$@\n" if $@;
+		die $err;
+	    }
+
 	    PVE::Cluster::cfs_write_file('status.cfg', $cfg);
 	});
 	die $@ if $@;
@@ -190,6 +202,12 @@ __PACKAGE__->register_method ({
 	my $digest = extract_param($param, 'digest');
 	my $delete = extract_param($param, 'delete');
 
+	if ($delete) {
+	    $delete = [PVE::Tools::split_list($delete)];
+	}
+
+	my $sensitive_params = extract_sensitive_params($param, ['token'], $delete);
+
 	PVE::Cluster::cfs_lock_file('status.cfg', undef, sub {
 	    my $cfg = PVE::Cluster::cfs_read_file('status.cfg');
 
@@ -201,15 +219,13 @@ __PACKAGE__->register_method ({
 	    my $plugin = PVE::Status::Plugin->lookup($data->{type});
 	    my $opts = $plugin->check_config($id, $param, 0, 1);
 
-	    $plugin->test_connection($opts);
-
 	    for my $k (keys %$opts) {
 		$data->{$k} = $opts->{$k};
 	    }
 
 	    if ($delete) {
 		my $options = $plugin->private()->{options}->{$data->{type}};
-		for my $k (PVE::Tools::split_list($delete)) {
+		for my $k (@$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};
@@ -220,6 +236,10 @@ __PACKAGE__->register_method ({
 		}
 	    }
 
+	    $plugin->on_update_hook($id, $data, $sensitive_params);
+
+	    $plugin->test_connection($opts, $id);
+
 	    PVE::Cluster::cfs_write_file('status.cfg', $cfg);
 	});
 	die $@ if $@;
@@ -253,6 +273,13 @@ __PACKAGE__->register_method ({
 	    my $cfg = PVE::Cluster::cfs_read_file('status.cfg');
 
 	    my $id = $param->{id};
+
+	    my $plugin_cfg = $cfg->{ids}->{$id};
+
+	    my $plugin = PVE::Status::Plugin->lookup($plugin_cfg->{type});
+
+	    $plugin->on_delete_hook($id, $plugin_cfg);
+
 	    delete $cfg->{ids}->{$id};
 	    PVE::Cluster::cfs_write_file('status.cfg', $cfg);
 	});
diff --git a/PVE/Status/Plugin.pm b/PVE/Status/Plugin.pm
index 2fa223ca..fae7a0f0 100644
--- a/PVE/Status/Plugin.pm
+++ b/PVE/Status/Plugin.pm
@@ -153,4 +153,28 @@ sub update_storage_status {
     die "please implement inside plugin";
 }
 
+sub on_add_hook {
+    my ($class, $id, $opts, $sensitive_opts) = @_;
+
+    # implement in subclass
+
+    return undef;
+}
+
+sub on_update_hook {
+    my ($class, $id, $opts, $sensitive_opts) = @_;
+
+    # implement in subclass
+
+    return undef;
+}
+
+sub on_delete_hook {
+    my ($class, $id, $opts) = @_;
+
+    # implement in subclass
+
+    return undef;
+}
+
 1;
-- 
2.20.1





^ permalink raw reply	[flat|nested] 21+ messages in thread

* [pve-devel] [PATCH manager 4/7] status/influxdb: implement influxdb 2.x http api
  2020-12-02  9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
                   ` (4 preceding siblings ...)
  2020-12-02  9:21 ` [pve-devel] [PATCH manager 3/7] status/plugin: extend with add/update/delete hooks Dominik Csapak
@ 2020-12-02  9:21 ` Dominik Csapak
  2020-12-02  9:21 ` [pve-devel] [PATCH manager 5/7] status/influxdb: remove unnecessary comment Dominik Csapak
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02  9:21 UTC (permalink / raw)
  To: pve-devel

needs an organization/bucket (previously db) and an optional token
the http client does not fit exactly in the connect/send/disconnect
scheme, so it simply creates a request in 'connect',
does the actual http connection in 'send' and nothing in 'disconnect'

max-body-size is set to 25.000.000 bytes by default (the influxdb default)
and the timeout to 1 second (same as default graphite tcp timeout)

the token (if given) gets saved in /etc/pve/priv/metricserver/$ID.pw
it is optional, because the 1.8.x compatibility api does not need
authentication (in contrast to influxdb 2.x)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/Status/InfluxDB.pm | 215 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 207 insertions(+), 8 deletions(-)

diff --git a/PVE/Status/InfluxDB.pm b/PVE/Status/InfluxDB.pm
index 6663579f..f16af486 100644
--- a/PVE/Status/InfluxDB.pm
+++ b/PVE/Status/InfluxDB.pm
@@ -6,6 +6,8 @@ use warnings;
 use POSIX qw(isnan isinf);
 use Scalar::Util 'looks_like_number';
 use IO::Socket::IP;
+use LWP::UserAgent;
+use HTTP::Request;
 
 use PVE::SafeSyslog;
 
@@ -24,12 +26,51 @@ sub type {
     return 'influxdb';
 }
 
+sub properties {
+    return {
+	organization => {
+	    description => "The influxdb organization. Only necessary when using the http v2 api. ".
+			   "Has no meaning when using v2 compatibility api.",
+	    type => 'string',
+	    optional => 1,
+	},
+	bucket => {
+	    description => "The influxdb bucket/db. Only necessary when using the http v2 api.",
+	    type => 'string',
+	    optional => 1,
+	},
+	token => {
+	    description => "The influxdb access token. Only necessary when using the http v2 api. ".
+			   "If the v2 compatibility api is used, use 'user:password' instead.",
+	    type => 'string',
+	    optional => 1,
+	},
+	influxdbproto => {
+	    type => 'string',
+	    enum => ['udp', 'http', 'https'],
+	    default => 'udp',
+	    optional => 1,
+	},
+	'max-body-size' => {
+	    description => "Influxdb max-body-size. Requests are batched up to this size.",
+	    type => 'integer',
+	    minimum => 1,
+	    default => 25_000_000,
+	}
+    };
+}
 sub options {
     return {
 	server => {},
 	port => {},
 	mtu => { optional => 1 },
 	disable => { optional => 1 },
+	organization => { optional => 1},
+	bucket => { optional => 1},
+	token => { optional => 1},
+	influxdbproto => { optional => 1},
+	timeout => { optional => 1},
+	'max-body-size' => { optional => 1 },
    };
 }
 
@@ -84,21 +125,106 @@ sub update_storage_status {
     build_influxdb_payload($class, $txn, $data, $ctime, $object);
 }
 
-sub _connect {
+sub _send_batch_size {
     my ($class, $cfg) = @_;
+    my $proto = $cfg->{influxdbproto} // 'udp';
+    if ($proto ne 'udp') {
+	return $cfg->{'max-body-size'} // 25_000_000;
+    }
+
+    return $class->SUPER::_send_batch_size($cfg);
+}
+
+sub send {
+    my ($class, $connection, $data, $cfg) = @_;
+
+    my $proto = $cfg->{influxdbproto} // 'udp';
+    if ($proto eq 'udp') {
+	return $class->SUPER::send($connection, $data, $cfg);
+    } elsif ($proto =~ m/^https?$/) {
+	my $ua = LWP::UserAgent->new();
+	$ua->timeout($cfg->{timeout} // 1);
+	$connection->content($data);
+	my $response = $ua->request($connection);
+
+	if (!$response->is_success) {
+	    my $err = $response->status_line;
+	    die "$err\n";
+	}
+    } else {
+	die "invalid protocol\n";
+    }
+
+    return;
+}
+
+sub _disconnect {
+    my ($class, $connection, $cfg) = @_;
+    my $proto = $cfg->{influxdbproto} // 'udp';
+    if ($proto eq 'udp') {
+	return $class->SUPER::_disconnect($connection, $cfg);
+    }
+
+    return;
+}
+
+sub _connect {
+    my ($class, $cfg, $id) = @_;
 
     my $host = $cfg->{server};
     my $port = $cfg->{port};
+    my $proto = $cfg->{influxdbproto} // 'udp';
+
+    if ($proto eq 'udp') {
+	my $socket = IO::Socket::IP->new(
+	    PeerAddr    => $host,
+	    PeerPort    => $port,
+	    Proto       => 'udp',
+	) || die "couldn't create influxdb socket [$host]:$port - $@\n";
+
+	$socket->blocking(0);
+
+	return $socket;
+    } elsif ($proto =~ m/^https?$/) {
+	my $token = get_credentials($id);
+	my $org = $cfg->{organization} // 'proxmox';
+	my $bucket = $cfg->{bucket} // 'proxmox';
+	my $url = "${proto}://${host}:${port}/api/v2/write?org=${org}&bucket=${bucket}";
+
+	my $req = HTTP::Request->new(POST => $url);
+	if (defined($token)) {
+	    $req->header( "Authorization", "Token $token");
+	}
 
-    my $socket = IO::Socket::IP->new(
-        PeerAddr    => $host,
-        PeerPort    => $port,
-        Proto       => 'udp',
-    ) || die "couldn't create influxdb socket [$host]:$port - $@\n";
+	return $req;
+    }
+
+    die "cannot connect to influxdb: invalid protocol\n";
+}
 
-    $socket->blocking(0);
+sub test_connection {
+    my ($class, $cfg, $id) = @_;
+
+    my $proto = $cfg->{influxdbproto} // 'udp';
+    if ($proto eq 'udp') {
+	return $class->SUPER::test_connection($cfg, $id);
+    } elsif ($proto =~ m/^https?$/) {
+	my $host = $cfg->{server};
+	my $port = $cfg->{port};
+	my $url = "${proto}://${host}:${port}/health";
+	my $ua = LWP::UserAgent->new();
+	$ua->timeout($cfg->{timeout} // 1);
+	my $response = $ua->get($url);
+
+	if (!$response->is_success) {
+	    my $err = $response->status_line;
+	    die "$err\n";
+	}
+    } else {
+	die "invalid protocol\n";
+    }
 
-    return $socket;
+    return;
 }
 
 sub build_influxdb_payload {
@@ -177,4 +303,77 @@ sub prepare_value {
     return $value;
 }
 
+my $priv_dir = "/etc/pve/priv/metricserver";
+
+sub cred_file_name {
+    my ($id) = @_;
+    return "${priv_dir}/${id}.pw";
+}
+
+sub delete_credentials {
+    my ($id) = @_;
+
+    if (my $cred_file = cred_file_name($id)) {
+	unlink($cred_file)
+	    or warn "removing influxdb credentials file '$cred_file' failed: $!\n";
+    }
+
+    return;
+}
+
+sub set_credentials {
+    my ($id, $token) = @_;
+
+    my $cred_file = cred_file_name($id);
+
+    mkdir $priv_dir;
+
+    PVE::Tools::file_set_contents($cred_file, "$token");
+}
+
+sub get_credentials {
+    my ($id) = @_;
+
+    my $cred_file = cred_file_name($id);
+
+    return PVE::Tools::file_get_contents($cred_file);
+}
+
+sub on_add_hook {
+    my ($class, $id, $opts, $sensitive_opts) = @_;
+
+    my $token = $sensitive_opts->{token};
+
+    if (defined($token)) {
+	set_credentials($id, $token);
+    } else {
+	delete_credenetials($id);
+    }
+
+    return undef;
+}
+
+sub on_update_hook {
+    my ($class, $id, $opts, $sensitive_opts) = @_;
+    return if !exists($sensitive_opts->{token});
+
+    my $token = $sensitive_opts->{token};
+    if (defined($token)) {
+	set_credentials($id, $token);
+    } else {
+	delete_credenetials($id);
+    }
+
+    return undef;
+}
+
+sub on_delete_hook {
+    my ($class, $id, $opts) = @_;
+
+    delete_credentials($id);
+
+    return undef;
+}
+
+
 1;
-- 
2.20.1





^ permalink raw reply	[flat|nested] 21+ messages in thread

* [pve-devel] [PATCH manager 5/7] status/influxdb: remove unnecessary comment
  2020-12-02  9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
                   ` (5 preceding siblings ...)
  2020-12-02  9:21 ` [pve-devel] [PATCH manager 4/7] status/influxdb: implement influxdb 2.x http api Dominik Csapak
@ 2020-12-02  9:21 ` Dominik Csapak
  2020-12-02  9:21 ` [pve-devel] [PATCH manager 6/7] ui: add necessary fields for influxdb http api Dominik Csapak
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02  9:21 UTC (permalink / raw)
  To: pve-devel

we already have that information in the reference docs, no need to
have it here as well

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/Status/InfluxDB.pm | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/PVE/Status/InfluxDB.pm b/PVE/Status/InfluxDB.pm
index f16af486..9ecce235 100644
--- a/PVE/Status/InfluxDB.pm
+++ b/PVE/Status/InfluxDB.pm
@@ -13,13 +13,6 @@ use PVE::SafeSyslog;
 
 use PVE::Status::Plugin;
 
-# example config (/etc/pve/status.cfg)
-#influxdb:
-#	server test
-#	port 8089
-#	disable 0
-#
-
 use base('PVE::Status::Plugin');
 
 sub type {
-- 
2.20.1





^ permalink raw reply	[flat|nested] 21+ messages in thread

* [pve-devel] [PATCH manager 6/7] ui: add necessary fields for influxdb http api
  2020-12-02  9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
                   ` (6 preceding siblings ...)
  2020-12-02  9:21 ` [pve-devel] [PATCH manager 5/7] status/influxdb: remove unnecessary comment Dominik Csapak
@ 2020-12-02  9:21 ` Dominik Csapak
  2020-12-02  9:21 ` [pve-devel] [PATCH manager 7/7] ui: dc/MetricServerView: add onlineHelp to edit windows Dominik Csapak
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02  9:21 UTC (permalink / raw)
  To: pve-devel

and en/disable them accordingly to the selected mode

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/dc/MetricServerView.js | 101 ++++++++++++++++++++++++++--
 1 file changed, 96 insertions(+), 5 deletions(-)

diff --git a/www/manager6/dc/MetricServerView.js b/www/manager6/dc/MetricServerView.js
index e30ea14e..dfec8c03 100644
--- a/www/manager6/dc/MetricServerView.js
+++ b/www/manager6/dc/MetricServerView.js
@@ -174,6 +174,13 @@ Ext.define('PVE.dc.InfluxDBEdit', {
 
     subject: 'InfluxDB',
 
+    cbindData: function() {
+	let me = this;
+	me.callParent();
+	me.tokenEmptyText = me.isCreate ? '' : gettext('unchanged');
+	return {};
+    },
+
     items: [
 	{
 	    xtype: 'inputpanel',
@@ -209,6 +216,41 @@ Ext.define('PVE.dc.InfluxDBEdit', {
 		    fieldLabel: gettext('Server'),
 		    allowBlank: false,
 		},
+		{
+		    xtype: 'proxmoxintegerfield',
+		    name: 'port',
+		    fieldLabel: gettext('Port'),
+		    value: 8089,
+		    minValue: 1,
+		    maximum: 65536,
+		    allowBlank: false,
+		},
+		{
+		    xtype: 'proxmoxKVComboBox',
+		    name: 'influxdbproto',
+		    fieldLabel: gettext('Protocol'),
+		    value: '__default__',
+		    cbind: {
+			deleteEmpty: '{!isCreate}',
+		    },
+		    comboItems: [
+			['__default__', 'UDP'],
+			['http', 'HTTP'],
+			['https', 'HTTPS'],
+		    ],
+		    listeners: {
+			change: function(field, value) {
+			    let me = this;
+			    let isUdp = value !== 'http' && value !== 'https';
+			    me.up('inputpanel').down('field[name=organization]').setDisabled(isUdp);
+			    me.up('inputpanel').down('field[name=bucket]').setDisabled(isUdp);
+			    me.up('inputpanel').down('field[name=token]').setDisabled(isUdp);
+			    me.up('inputpanel').down('field[name=mtu]').setDisabled(!isUdp);
+			    me.up('inputpanel').down('field[name=timeout]').setDisabled(isUdp);
+			    me.up('inputpanel').down('field[name=max-body-size]').setDisabled(isUdp);
+			},
+		    },
+		},
 	    ],
 
 	    column2: [
@@ -220,18 +262,67 @@ Ext.define('PVE.dc.InfluxDBEdit', {
 		    uncheckedValue: 0,
 		    checked: true,
 		},
+		{
+		    xtype: 'proxmoxtextfield',
+		    name: 'organization',
+		    fieldLabel: gettext('Organization'),
+		    emptyText: 'proxmox',
+		    disabled: true,
+		    cbind: {
+			deleteEmpty: '{!isCreate}',
+		    },
+		},
+		{
+		    xtype: 'proxmoxtextfield',
+		    name: 'bucket',
+		    fieldLabel: gettext('Bucket'),
+		    emptyText: 'proxmox',
+		    disabled: true,
+		    cbind: {
+			deleteEmpty: '{!isCreate}',
+		    },
+		},
+		{
+		    xtype: 'proxmoxtextfield',
+		    name: 'token',
+		    fieldLabel: gettext('Token'),
+		    disabled: true,
+		    allowBlank: true,
+		    deleteEmpty: false,
+		    submitEmpty: false,
+		    cbind: {
+			disabled: '{!isCreate}',
+			emptyText: '{tokenEmptyText}',
+		    },
+		},
+	    ],
+
+	    advancedColumn1: [
 		{
 		    xtype: 'proxmoxintegerfield',
-		    name: 'port',
-		    fieldLabel: gettext('Port'),
-		    value: 8089,
+		    name: 'timeout',
+		    fieldLabel: gettext('Timeout (s)'),
+		    disabled: true,
+		    cbind: {
+			deleteEmpty: '{!isCreate}',
+		    },
 		    minValue: 1,
-		    maximum: 65536,
-		    allowBlank: false,
+		    emptyText: 1,
 		},
 	    ],
 
 	    advancedColumn2: [
+		{
+		    xtype: 'proxmoxintegerfield',
+		    name: 'max-body-size',
+		    fieldLabel: gettext('Batch Size (b)'),
+		    minValue: 1,
+		    emptyText: '25000000',
+		    submitEmpty: false,
+		    cbind: {
+			deleteEmpty: '{!isCreate}',
+		    },
+		},
 		{
 		    xtype: 'proxmoxintegerfield',
 		    name: 'mtu',
-- 
2.20.1





^ permalink raw reply	[flat|nested] 21+ messages in thread

* [pve-devel] [PATCH manager 7/7] ui: dc/MetricServerView: add onlineHelp to edit windows
  2020-12-02  9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
                   ` (7 preceding siblings ...)
  2020-12-02  9:21 ` [pve-devel] [PATCH manager 6/7] ui: add necessary fields for influxdb http api Dominik Csapak
@ 2020-12-02  9:21 ` Dominik Csapak
  2020-12-02  9:21 ` [pve-devel] [PATCH docs 1/1] external metrics server: extend docs to explain http api Dominik Csapak
  2021-01-28 16:36 ` [pve-devel] applied-series: [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Thomas Lamprecht
  10 siblings, 0 replies; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02  9:21 UTC (permalink / raw)
  To: pve-devel

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/dc/MetricServerView.js | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/www/manager6/dc/MetricServerView.js b/www/manager6/dc/MetricServerView.js
index dfec8c03..edb40cc5 100644
--- a/www/manager6/dc/MetricServerView.js
+++ b/www/manager6/dc/MetricServerView.js
@@ -172,6 +172,8 @@ Ext.define('PVE.dc.InfluxDBEdit', {
     extend: 'PVE.dc.MetricServerBaseEdit',
     mixins: ['Proxmox.Mixin.CBind'],
 
+    onlineHelp: 'metric_server_influxdb',
+
     subject: 'InfluxDB',
 
     cbindData: function() {
@@ -343,6 +345,8 @@ Ext.define('PVE.dc.GraphiteEdit', {
     extend: 'PVE.dc.MetricServerBaseEdit',
     mixins: ['Proxmox.Mixin.CBind'],
 
+    onlineHelp: 'metric_server_graphite',
+
     subject: 'Graphite',
 
     items: [
-- 
2.20.1





^ permalink raw reply	[flat|nested] 21+ messages in thread

* [pve-devel] [PATCH docs 1/1] external metrics server: extend docs to explain http api
  2020-12-02  9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
                   ` (8 preceding siblings ...)
  2020-12-02  9:21 ` [pve-devel] [PATCH manager 7/7] ui: dc/MetricServerView: add onlineHelp to edit windows Dominik Csapak
@ 2020-12-02  9:21 ` Dominik Csapak
  2021-01-28 16:36 ` [pve-devel] applied-series: [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Thomas Lamprecht
  10 siblings, 0 replies; 21+ messages in thread
From: Dominik Csapak @ 2020-12-02  9:21 UTC (permalink / raw)
  To: pve-devel

and mention that 1.8.x has a compatible api (and their differences)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pve-external-metric-server.adoc | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/pve-external-metric-server.adoc b/pve-external-metric-server.adoc
index 783e72c..eace999 100644
--- a/pve-external-metric-server.adoc
+++ b/pve-external-metric-server.adoc
@@ -56,3 +56,21 @@ Here is an example configuration for influxdb (on your influxdb server):
 
 With this configuration, your server listens on all IP addresses on port 8089,
 and writes the data in the *proxmox* database
+
+Alternatively, the plugin can be configured to use the http(s) API of InfluxDB 2.x.
+InfluxDB 1.8.x does contain a forwards compatible API endpoint for this v2 API.
+
+To use it, set 'influxdbproto' to 'http' or 'https' (depending on your configuration).
+By default, {pve} uses the organization 'proxmox' and the bucket/db 'proxmox'
+(They can be set with the configuration 'organization' and 'bucket' respectively).
+
+Since InfluxDB's v2 API is only available with authentication, you have
+to generate a token that can write into the correct bucket and set it.
+
+In the v2 compatible API of 1.8.x, you can use 'user:password' as token
+(if required), and can omit the 'organization' since that has no meaning in InfluxDB 1.x.
+
+You can also set the HTTP Timeout (default is 1s) with the 'timeout' setting,
+as well as the maximum batch size (default 25000000 bytes) with the
+'max-body-size' setting (this corresponds to the InfluxDB setting with the
+same name).
-- 
2.20.1





^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [pve-devel] [PATCH common 1/1] tools: add extract_sensitive_params
  2020-12-02  9:21 ` [pve-devel] [PATCH common 1/1] tools: add extract_sensitive_params Dominik Csapak
@ 2020-12-03  8:47   ` Thomas Lamprecht
  2020-12-03  9:16     ` Wolfgang Bumiller
  2020-12-03 15:52   ` [pve-devel] applied: " Thomas Lamprecht
  1 sibling, 1 reply; 21+ messages in thread
From: Thomas Lamprecht @ 2020-12-03  8:47 UTC (permalink / raw)
  To: Proxmox VE development discussion, Dominik Csapak; +Cc: Wolfgang Bumiller

On 02.12.20 10:21, Dominik Csapak wrote:
> moved and generalized from pve-storage, since we'll need it
> in more places
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/Tools.pm | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> index 4b445ea..bda236a 100644
> --- a/src/PVE/Tools.pm
> +++ b/src/PVE/Tools.pm
> @@ -48,6 +48,7 @@ template_replace
>  safe_print
>  trim
>  extract_param
> +extract_sensitive_params
>  file_copy
>  get_host_arch
>  O_PATH
> @@ -807,6 +808,29 @@ sub extract_param {
>      return $res;
>  }
>  

can we have some short comment about what this does and when/why it can be useful here

> +sub extract_sensitive_params :prototype($$$) {
> +    my ($param, $sensitive_list, $delete_list) = @_;
> +
> +    my $sensitive;

I know auto vivification and such things exist, but I'd feel more comfortable
to set above explicitly to and empty hash {} .

> +
> +    my %delete = map { $_ => 1 } ($delete_list || [])->@*;
> +
> +    # always extract sensitive keys, so they don't get written to the www-data readable scfg

not only for scfg anymore, would drop that comment actually completely, that's rather
something for a method comment (see above)

> +    for my $opt (@$sensitive_list) {
> +	# First handle deletions as explicitly setting `undef`, afterwards new values may override
> +	# it.

I know this is just copied, but there's no actual reason for setting to undef vs.
using delete encoded in that comment, it's just merely describing what one sees
when reading the code anyhow..

@Wolfgang, you as original author (pve-storage commit 72385de9e23df) why did you
used undef vs. delete?

> +	if (exists($delete{$opt})) {
> +	    $sensitive->{$opt} = undef;
> +	}
> +
> +	if (defined(my $value = extract_param($param, $opt))) {
> +	    $sensitive->{$opt} = $value;
> +	}
> +    }
> +
> +    return $sensitive;
> +}
> +
>  # Note: we use this to wait until vncterm/spiceterm is ready
>  sub wait_for_vnc_port {
>      my ($port, $family, $timeout) = @_;
> 






^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [pve-devel] [PATCH manager 1/7] api: cluster/metricserver: prevent simultaneosly setting and deleting of property
  2020-12-02  9:21 ` [pve-devel] [PATCH manager 1/7] api: cluster/metricserver: prevent simultaneosly setting and deleting of property Dominik Csapak
@ 2020-12-03  9:05   ` Thomas Lamprecht
  2020-12-04 11:30     ` Dominik Csapak
  0 siblings, 1 reply; 21+ messages in thread
From: Thomas Lamprecht @ 2020-12-03  9:05 UTC (permalink / raw)
  To: Proxmox VE development discussion, Dominik Csapak

On 02.12.20 10:21, Dominik Csapak wrote:
> like we do in other apis of section configs (e.g. storage)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  PVE/API2/Cluster/MetricServer.pm | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/PVE/API2/Cluster/MetricServer.pm b/PVE/API2/Cluster/MetricServer.pm
> index 9a14985e..ec3c7b75 100644
> --- a/PVE/API2/Cluster/MetricServer.pm
> +++ b/PVE/API2/Cluster/MetricServer.pm
> @@ -213,6 +213,8 @@ __PACKAGE__->register_method ({
>  		    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};
> +		    die "cannot set and delete property '$k' at the same time!\n"
> +			if defined($opts->{$k});
>  
>  		    delete $data->{$k};
>  		}
> 

That counts as API change, strictly speaking.. For container and VMs we order
deletions before setting the value, and the one from container is the last
one which got some actual thoughts and discussion going on, IIRC, albeit not
to sure if about that exact behavior (as it was probably pre-existing).

It'd be good to at least decide for one behavior and try making that universal,
as else this is confusing..





^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [pve-devel] [PATCH common 1/1] tools: add extract_sensitive_params
  2020-12-03  8:47   ` Thomas Lamprecht
@ 2020-12-03  9:16     ` Wolfgang Bumiller
  2020-12-03  9:35       ` Thomas Lamprecht
  0 siblings, 1 reply; 21+ messages in thread
From: Wolfgang Bumiller @ 2020-12-03  9:16 UTC (permalink / raw)
  To: Thomas Lamprecht, Proxmox VE development discussion, Dominik Csapak


> On 12/03/2020 9:47 AM Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:
> 
>  
> On 02.12.20 10:21, Dominik Csapak wrote:
> > moved and generalized from pve-storage, since we'll need it
> > in more places
> > 
> > Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> > ---
> >  src/PVE/Tools.pm | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> > index 4b445ea..bda236a 100644
> > --- a/src/PVE/Tools.pm
> > +++ b/src/PVE/Tools.pm
> > @@ -48,6 +48,7 @@ template_replace
> >  safe_print
> >  trim
> >  extract_param
> > +extract_sensitive_params
> >  file_copy
> >  get_host_arch
> >  O_PATH
> > @@ -807,6 +808,29 @@ sub extract_param {
> >      return $res;
> >  }
> >  
> 
> can we have some short comment about what this does and when/why it can be useful here
> 
> > +sub extract_sensitive_params :prototype($$$) {
> > +    my ($param, $sensitive_list, $delete_list) = @_;
> > +
> > +    my $sensitive;
> 
> I know auto vivification and such things exist, but I'd feel more comfortable
> to set above explicitly to and empty hash {} .
> 
> > +
> > +    my %delete = map { $_ => 1 } ($delete_list || [])->@*;
> > +
> > +    # always extract sensitive keys, so they don't get written to the www-data readable scfg
> 
> not only for scfg anymore, would drop that comment actually completely, that's rather
> something for a method comment (see above)
> 
> > +    for my $opt (@$sensitive_list) {
> > +	# First handle deletions as explicitly setting `undef`, afterwards new values may override
> > +	# it.
> 
> I know this is just copied, but there's no actual reason for setting to undef vs.
> using delete encoded in that comment, it's just merely describing what one sees
> when reading the code anyhow..
> 
> @Wolfgang, you as original author (pve-storage commit 72385de9e23df) why did you
> used undef vs. delete?

The update hooks in pve-storage don't get the deletion-list passed on as parameter,
so I translated into putting `undef` into the parameter list.




^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [pve-devel] [PATCH common 1/1] tools: add extract_sensitive_params
  2020-12-03  9:16     ` Wolfgang Bumiller
@ 2020-12-03  9:35       ` Thomas Lamprecht
  0 siblings, 0 replies; 21+ messages in thread
From: Thomas Lamprecht @ 2020-12-03  9:35 UTC (permalink / raw)
  To: Wolfgang Bumiller, Proxmox VE development discussion, Dominik Csapak

On 03.12.20 10:16, Wolfgang Bumiller wrote:
>> On 12/03/2020 9:47 AM Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:
>> On 02.12.20 10:21, Dominik Csapak wrote:
>>> +    for my $opt (@$sensitive_list) {
>>> +	# First handle deletions as explicitly setting `undef`, afterwards new values may override
>>> +	# it.
>>
>> I know this is just copied, but there's no actual reason for setting to undef vs.
>> using delete encoded in that comment, it's just merely describing what one sees
>> when reading the code anyhow..
>>
>> @Wolfgang, you as original author (pve-storage commit 72385de9e23df) why did you
>> used undef vs. delete?
> 
> The update hooks in pve-storage don't get the deletion-list passed on as parameter,
> so I translated into putting `undef` into the parameter list.
> 

OK, then  that would be a much better comment here as it gives an
actual reason, something like

# delete by setting to undef so that add/update hooks can know about it





^ permalink raw reply	[flat|nested] 21+ messages in thread

* [pve-devel] applied: [PATCH common 1/1] tools: add extract_sensitive_params
  2020-12-02  9:21 ` [pve-devel] [PATCH common 1/1] tools: add extract_sensitive_params Dominik Csapak
  2020-12-03  8:47   ` Thomas Lamprecht
@ 2020-12-03 15:52   ` Thomas Lamprecht
  1 sibling, 0 replies; 21+ messages in thread
From: Thomas Lamprecht @ 2020-12-03 15:52 UTC (permalink / raw)
  To: Proxmox VE development discussion, Dominik Csapak

On 02.12.20 10:21, Dominik Csapak wrote:
> moved and generalized from pve-storage, since we'll need it
> in more places
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/Tools.pm | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
>

applied, with some followups for the stuff commented, thanks!




^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [pve-devel] [PATCH manager 1/7] api: cluster/metricserver: prevent simultaneosly setting and deleting of property
  2020-12-03  9:05   ` Thomas Lamprecht
@ 2020-12-04 11:30     ` Dominik Csapak
  2020-12-04 11:57       ` Thomas Lamprecht
  0 siblings, 1 reply; 21+ messages in thread
From: Dominik Csapak @ 2020-12-04 11:30 UTC (permalink / raw)
  To: Thomas Lamprecht, Proxmox VE development discussion

On 12/3/20 10:05 AM, Thomas Lamprecht wrote:
> On 02.12.20 10:21, Dominik Csapak wrote:
>> like we do in other apis of section configs (e.g. storage)
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   PVE/API2/Cluster/MetricServer.pm | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/PVE/API2/Cluster/MetricServer.pm b/PVE/API2/Cluster/MetricServer.pm
>> index 9a14985e..ec3c7b75 100644
>> --- a/PVE/API2/Cluster/MetricServer.pm
>> +++ b/PVE/API2/Cluster/MetricServer.pm
>> @@ -213,6 +213,8 @@ __PACKAGE__->register_method ({
>>   		    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};
>> +		    die "cannot set and delete property '$k' at the same time!\n"
>> +			if defined($opts->{$k});
>>   
>>   		    delete $data->{$k};
>>   		}
>>
> 
> That counts as API change, strictly speaking..

ok, so should we leave it as is for now?

> For container and VMs we order
> deletions before setting the value, and the one from container is the last
> one which got some actual thoughts and discussion going on, IIRC, albeit not
> to sure if about that exact behavior (as it was probably pre-existing).
> 
> It'd be good to at least decide for one behavior and try making that universal,
> as else this is confusing..
> 

yeah that makes sense (though i think the ordering is irrelevant, since
even in container you cannot set and delete at the same time)




^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [pve-devel] [PATCH manager 1/7] api: cluster/metricserver: prevent simultaneosly setting and deleting of property
  2020-12-04 11:30     ` Dominik Csapak
@ 2020-12-04 11:57       ` Thomas Lamprecht
  2020-12-04 12:45         ` Thomas Lamprecht
  0 siblings, 1 reply; 21+ messages in thread
From: Thomas Lamprecht @ 2020-12-04 11:57 UTC (permalink / raw)
  To: Dominik Csapak, Proxmox VE development discussion

On 04.12.20 12:30, Dominik Csapak wrote:
> On 12/3/20 10:05 AM, Thomas Lamprecht wrote:
>> On 02.12.20 10:21, Dominik Csapak wrote:
>>> like we do in other apis of section configs (e.g. storage)
>>>
>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>> ---
>>>   PVE/API2/Cluster/MetricServer.pm | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/PVE/API2/Cluster/MetricServer.pm b/PVE/API2/Cluster/MetricServer.pm
>>> index 9a14985e..ec3c7b75 100644
>>> --- a/PVE/API2/Cluster/MetricServer.pm
>>> +++ b/PVE/API2/Cluster/MetricServer.pm
>>> @@ -213,6 +213,8 @@ __PACKAGE__->register_method ({
>>>               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};
>>> +            die "cannot set and delete property '$k' at the same time!\n"
>>> +            if defined($opts->{$k});
>>>                 delete $data->{$k};
>>>           }
>>>
>>
>> That counts as API change, strictly speaking..
> 
> ok, so should we leave it as is for now?
> 
>> For container and VMs we order
>> deletions before setting the value, and the one from container is the last
>> one which got some actual thoughts and discussion going on, IIRC, albeit not
>> to sure if about that exact behavior (as it was probably pre-existing).
>>
>> It'd be good to at least decide for one behavior and try making that universal,
>> as else this is confusing..
>>
> 
> yeah that makes sense (though i think the ordering is irrelevant, since
> even in container you cannot set and delete at the same time)
> 

I mean yes but no. It's ordered delete first, so it actually just behaves like
a set (update).





^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [pve-devel] [PATCH manager 1/7] api: cluster/metricserver: prevent simultaneosly setting and deleting of property
  2020-12-04 11:57       ` Thomas Lamprecht
@ 2020-12-04 12:45         ` Thomas Lamprecht
  0 siblings, 0 replies; 21+ messages in thread
From: Thomas Lamprecht @ 2020-12-04 12:45 UTC (permalink / raw)
  To: Dominik Csapak, Proxmox VE development discussion

On 04.12.20 12:57, Thomas Lamprecht wrote:
> On 04.12.20 12:30, Dominik Csapak wrote:
>> On 12/3/20 10:05 AM, Thomas Lamprecht wrote:
>>> On 02.12.20 10:21, Dominik Csapak wrote:
>>>> like we do in other apis of section configs (e.g. storage)
>>>>
>>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>>> ---
>>>>   PVE/API2/Cluster/MetricServer.pm | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/PVE/API2/Cluster/MetricServer.pm b/PVE/API2/Cluster/MetricServer.pm
>>>> index 9a14985e..ec3c7b75 100644
>>>> --- a/PVE/API2/Cluster/MetricServer.pm
>>>> +++ b/PVE/API2/Cluster/MetricServer.pm
>>>> @@ -213,6 +213,8 @@ __PACKAGE__->register_method ({
>>>>               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};
>>>> +            die "cannot set and delete property '$k' at the same time!\n"
>>>> +            if defined($opts->{$k});
>>>>                 delete $data->{$k};
>>>>           }
>>>>
>>>
>>> That counts as API change, strictly speaking..
>>
>> ok, so should we leave it as is for now?
>>
>>> For container and VMs we order
>>> deletions before setting the value, and the one from container is the last
>>> one which got some actual thoughts and discussion going on, IIRC, albeit not
>>> to sure if about that exact behavior (as it was probably pre-existing).
>>>
>>> It'd be good to at least decide for one behavior and try making that universal,
>>> as else this is confusing..
>>>
>>
>> yeah that makes sense (though i think the ordering is irrelevant, since
>> even in container you cannot set and delete at the same time)
>>
> 
> I mean yes but no. It's ordered delete first, so it actually just behaves like
> a set (update).
> 

I only looked at the internal update method and missed the check which errors 
also for CTs in the API, so ignore this.





^ permalink raw reply	[flat|nested] 21+ messages in thread

* [pve-devel] applied: [PATCH storage 1/1] api: storage/config: use extract_sensitive_params from tools
  2020-12-02  9:21 ` [pve-devel] [PATCH storage 1/1] api: storage/config: use extract_sensitive_params from tools Dominik Csapak
@ 2021-01-28 16:31   ` Thomas Lamprecht
  0 siblings, 0 replies; 21+ messages in thread
From: Thomas Lamprecht @ 2021-01-28 16:31 UTC (permalink / raw)
  To: Proxmox VE development discussion, Dominik Csapak

On 02.12.20 10:21, Dominik Csapak wrote:
> we have a more general version there
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  PVE/API2/Storage/Config.pm | 29 ++++-------------------------
>  1 file changed, 4 insertions(+), 25 deletions(-)
> 
>

applied, thanks!




^ permalink raw reply	[flat|nested] 21+ messages in thread

* [pve-devel] applied-series: [PATCH common/storage/manager/docs] implement http api for influxdb status plugin
  2020-12-02  9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
                   ` (9 preceding siblings ...)
  2020-12-02  9:21 ` [pve-devel] [PATCH docs 1/1] external metrics server: extend docs to explain http api Dominik Csapak
@ 2021-01-28 16:36 ` Thomas Lamprecht
  10 siblings, 0 replies; 21+ messages in thread
From: Thomas Lamprecht @ 2021-01-28 16:36 UTC (permalink / raw)
  To: Proxmox VE development discussion, Dominik Csapak

On 02.12.20 10:21, Dominik Csapak wrote:
> this series adds the possibility to write influxdb data via their
> v2 api. For influxdb 1.x there is a forwards compatible api since 1.8.0
> 
> i am not so sure about how i integrated the http api part into the
> existing influxdb plugin, another alternative would be to have
> a new InfluxDB2 Plugin that does only http(s).
> this way i could omit the permanent 'cfg->proto' check, but i am not
> sure if we want to have a new plugin for this..
> 
> manager/storage patches depend on common, but the storage patch is not needed for
> it to work
> 
> pve-common:
> 
> Dominik Csapak (1):
>   tools: add extract_sensitive_params
> 
>  src/PVE/Tools.pm | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> pve-storage:
> 
> Dominik Csapak (1):
>   api: storage/config: use extract_sensitive_params from tools
> 
>  PVE/API2/Storage/Config.pm | 29 ++++-------------------------
>  1 file changed, 4 insertions(+), 25 deletions(-)
> 
> pve-manager:
> 
> Dominik Csapak (7):
>   api: cluster/metricserver: prevent simultaneosly setting and deleting
>     of property
>   status/plugin: extend send/_connect/_disconnect/test_connection
>   status/plugin: extend with add/update/delete hooks
>   status/influxdb: implement influxdb 2.x http api
>   status/influxdb: remove unnecessary comment
>   ui: add necessary fields for influxdb http api
>   ui: dc/MetricServerView: add onlineHelp to edit windows
> 
>  PVE/API2/Cluster/MetricServer.pm    |  41 ++++-
>  PVE/ExtMetric.pm                    |   4 +-
>  PVE/Status/InfluxDB.pm              | 222 ++++++++++++++++++++++++++--
>  PVE/Status/Plugin.pm                |  38 ++++-
>  www/manager6/dc/MetricServerView.js | 105 ++++++++++++-
>  5 files changed, 375 insertions(+), 35 deletions(-)
> 
> pve-docs:
> 
> Dominik Csapak (1):
>   external metrics server: extend docs to explain http api
> 
>  pve-external-metric-server.adoc | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 



applied remaining parts of the series, thanks!

FYI: Had to resolve a small merge conflict due to context change in the
"status/plugin: extend send/_connect/_disconnect/test_connection"
commit




^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2021-01-28 16:36 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-02  9:21 [pve-devel] [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Dominik Csapak
2020-12-02  9:21 ` [pve-devel] [PATCH common 1/1] tools: add extract_sensitive_params Dominik Csapak
2020-12-03  8:47   ` Thomas Lamprecht
2020-12-03  9:16     ` Wolfgang Bumiller
2020-12-03  9:35       ` Thomas Lamprecht
2020-12-03 15:52   ` [pve-devel] applied: " Thomas Lamprecht
2020-12-02  9:21 ` [pve-devel] [PATCH storage 1/1] api: storage/config: use extract_sensitive_params from tools Dominik Csapak
2021-01-28 16:31   ` [pve-devel] applied: " Thomas Lamprecht
2020-12-02  9:21 ` [pve-devel] [PATCH manager 1/7] api: cluster/metricserver: prevent simultaneosly setting and deleting of property Dominik Csapak
2020-12-03  9:05   ` Thomas Lamprecht
2020-12-04 11:30     ` Dominik Csapak
2020-12-04 11:57       ` Thomas Lamprecht
2020-12-04 12:45         ` Thomas Lamprecht
2020-12-02  9:21 ` [pve-devel] [PATCH manager 2/7] status/plugin: extend send/_connect/_disconnect/test_connection Dominik Csapak
2020-12-02  9:21 ` [pve-devel] [PATCH manager 3/7] status/plugin: extend with add/update/delete hooks Dominik Csapak
2020-12-02  9:21 ` [pve-devel] [PATCH manager 4/7] status/influxdb: implement influxdb 2.x http api Dominik Csapak
2020-12-02  9:21 ` [pve-devel] [PATCH manager 5/7] status/influxdb: remove unnecessary comment Dominik Csapak
2020-12-02  9:21 ` [pve-devel] [PATCH manager 6/7] ui: add necessary fields for influxdb http api Dominik Csapak
2020-12-02  9:21 ` [pve-devel] [PATCH manager 7/7] ui: dc/MetricServerView: add onlineHelp to edit windows Dominik Csapak
2020-12-02  9:21 ` [pve-devel] [PATCH docs 1/1] external metrics server: extend docs to explain http api Dominik Csapak
2021-01-28 16:36 ` [pve-devel] applied-series: [PATCH common/storage/manager/docs] implement http api for influxdb status plugin Thomas Lamprecht

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal