[pve-devel] [PATCH container] Proposed fix for #1326: Added secondary server to graphite plugin as secondary_server parameter. It may be required if graphite data needs to be mirrored as in the case here. Carbon-relay is too much...

[Dominik Csapak]

Hi,

great that you want to improve PVE :)
a few things though:

1. Did you read the http://pve.proxmox.com/wiki/Developer_Documentation ?
specially the part about the license and the Harmony CLA?
if not, please do so

2. Git styling
Please try and keep the subject short and precise,
you can write a longer text in the commit message
the format is

subject of the commit

longer text of the commit
with multiple lines

also if you write "fix #1326" instead of "fix for #1326" our tools can 
detect the patch, and
update the bugtracker semi-automatically

3. Content
The idea in general is very good, but there is a better approach
instead of hardcoding a second server in the graphite plugin,
it would be better to allow defining multiple servers of each type
(so it would work also with influxdb)

the changes would be in PVE/Status/Plugin.pm
where we parse the header section,
here we use the type also as name, but this
can be (backwards compatible) changed so that
you can give the entries names

this would be automatically used in the code
and sent to each one

for an example how to write the parser
see PVE/Storage/Plugin.pm

in the pve-storage package

i also could send this patches for you if you only want the feature and
do not care about that you contributed to the code

happy coding ;)


On 03/30/2017 05:10 PM, Pavel Andreev wrote:
> Signed-off-by: Pavel Andreev <pavel at andreew.spb.ru>
> ---
>  PVE/Status/Graphite.pm |    6 +++++-
>  PVE/Status/Plugin.pm   |    4 ++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/PVE/Status/Graphite.pm b/PVE/Status/Graphite.pm
> index 849930f..50075cc 100644
> --- a/PVE/Status/Graphite.pm
> +++ b/PVE/Status/Graphite.pm
> @@ -7,6 +7,7 @@ use PVE::Status::Plugin;
>  # example config (/etc/pve/status.cfg)
>  #graphite:
>  #	server test
> +#	secondary_server test
>  #	port 2003
>  #	path proxmox.mycluster
>  #	disable 0
> @@ -30,6 +31,7 @@ sub properties {
>  sub options {
>      return {
>  	server => {},
> +        secondary_server => {},
>  	port => { optional => 1 },
>  	path => { optional => 1 },
>  	disable => { optional => 1 },
> @@ -64,7 +66,8 @@ sub update_storage_status {
>  sub write_graphite_hash {
>      my ($plugin_config, $d, $ctime, $object) = @_;
>
> -    my $host = $plugin_config->{server};
> +    my @hosts = ($plugin_config->{server},$plugin_config->{secondary_server});
> +    foreach my $host (@hosts) {
>      my $port = $plugin_config->{port} ? $plugin_config->{port} : 2003;
>      my $path = $plugin_config->{path} ? $plugin_config->{path} : 'proxmox';
>
> @@ -78,6 +81,7 @@ sub write_graphite_hash {
>
>      $carbon_socket->close() if $carbon_socket;
>
> +    }
>  }
>
>  sub write_graphite {
> diff --git a/PVE/Status/Plugin.pm b/PVE/Status/Plugin.pm
> index ff7af89..8e40d58 100644
> --- a/PVE/Status/Plugin.pm
> +++ b/PVE/Status/Plugin.pm
> @@ -30,6 +30,10 @@ my $defaultData = {
>  	    type => 'string', format => 'address',
>  	    description => "server dns name or IP address",
>  	},
> +        secondary_server => {
> +            type => 'string', format => 'address',
> +            description => "server dns name or IP address",
> +        },
>  	port => {
>  	    type => 'integer',
>  	    description => "server network port",
>