[pve-devel] [PATCH container] Fix #1326: Multi destinations support.

[Dominik Csapak]

hi,

a few comments inline

On 04/01/2017 02:38 PM, Pavel Andreev wrote:
> Signed-off-by: Pavel Andreev <pavel at andreew.spb.ru>
> ---
>  PVE/Status.pm          |   26 ++++++++++++++++++++++++++
>  PVE/Status/Graphite.pm |    2 +-
>  PVE/Status/InfluxDB.pm |    2 +-
>  PVE/Status/Plugin.pm   |   34 +++++++++++++++++++++++++---------
>  4 files changed, 53 insertions(+), 11 deletions(-)
>  create mode 100644 PVE/Status.pm
>
> diff --git a/PVE/Status.pm b/PVE/Status.pm
> new file mode 100644
> index 0000000..96322fd
> --- /dev/null
> +++ b/PVE/Status.pm
> @@ -0,0 +1,26 @@
> +package PVE::Status;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::Cluster qw(cfs_read_file);
> +
> +sub config {
> +    return cfs_read_file("status.cfg");
> +}
> +
> +sub status_ids {
> +    my ($cfg) = @_;
> +
> +    return keys %{$cfg->{ids}};
> +}
> +
> +sub complete_status_server {
> +    my ($cmdname, $pname, $cvalue) = @_;
> +
> +    my $cfg = PVE::Status::config();
> +
> +    return  $cmdname eq 'add' ? [] : [ PVE::Status::status_ids($cfg) ];
> +}
> +
> +1;

hmm, is there a specific reason you created this file?
if it is just for the 'complete_status_server' sub,
this could be defined in the plugin.pm i guess

anyway, you did not include the file in the makefile, which would not
throw any errors when building, but anyone with a status.cfg
would run into perl errors

> diff --git a/PVE/Status/Graphite.pm b/PVE/Status/Graphite.pm
> index 849930f..f4618b2 100644
> --- a/PVE/Status/Graphite.pm
> +++ b/PVE/Status/Graphite.pm
> @@ -5,7 +5,7 @@ use warnings;
>  use PVE::Status::Plugin;
>
>  # example config (/etc/pve/status.cfg)
> -#graphite:
> +#graphite: graphitename
>  #	server test
>  #	port 2003
>  #	path proxmox.mycluster
> diff --git a/PVE/Status/InfluxDB.pm b/PVE/Status/InfluxDB.pm
> index 7364e57..ed86ae1 100644
> --- a/PVE/Status/InfluxDB.pm
> +++ b/PVE/Status/InfluxDB.pm
> @@ -7,7 +7,7 @@ use Data::Dumper;
>  use PVE::SafeSyslog;
>
>  # example config (/etc/pve/status.cfg)
> -#influxdb:
> +#influxdb: influxname
>  #	server test
>  #	port 8089
>  #	disable 0
> diff --git a/PVE/Status/Plugin.pm b/PVE/Status/Plugin.pm
> index ff7af89..9ac93ae 100644
> --- a/PVE/Status/Plugin.pm
> +++ b/PVE/Status/Plugin.pm
> @@ -4,7 +4,7 @@ use strict;
>  use warnings;
>
>  use PVE::Tools;
> -use PVE::JSONSchema;
> +use PVE::JSONSchema qw(get_standard_option);
>  use PVE::Cluster;
>
>  use Data::Dumper;
> @@ -15,12 +15,27 @@ PVE::Cluster::cfs_register_file('status.cfg',
>  				 sub { __PACKAGE__->parse_config(@_); },
>  				 sub { __PACKAGE__->write_config(@_); });
>
> +PVE::JSONSchema::register_standard_option('pve-status-server-id', {
> +     description => "The storage identifier.",
> +     type => 'string', format => 'pve-status-server-id',
> +});
> +
> +PVE::JSONSchema::register_format('pve-status-server-id', \&parse_status_server_id);
> +sub parse_status_server_id {
> +    my ($serverid, $noerr) = @_;
> +
> +    if ($serverid !~ m/^[a-z][a-z0-9\-\_\.]*[a-z0-9]$/i) {
> +        return undef if $noerr;
> +        die "server ID '$serverid' contains illegal characters\n";
> +    }
> +    return $serverid;
> +}
> +
>  my $defaultData = {
>      propertyList => {
> -	type => {
> -	    description => "Plugin type.",
> -	    type => 'string', format => 'pve-configid',
> -	},

why did you remove the type format?

> +        type => { description => "Plugin type." },
> +             server => get_standard_option('pve-status-server-id',
> +            { completion => \&PVE::Status::complete_status_server }),

since there is no commandline tool for adding/editing/removing the 
status servers, a bash-completion makes really no sense?

also we already have a server property just below the disable property?
so the name would have to be different like "name"

also the indentation is wrong.

>  	disable => {
>  	    description => "Flag to disable the plugin.",
>  	    type => 'boolean',
> @@ -44,13 +59,13 @@ sub private {
>  sub parse_section_header {
>      my ($class, $line) = @_;
>
> -    if ($line =~ m/^(\S+):\s*$/) {
> -	my $type = lc($1);
> +     if ($line =~ m/^(\S+):\s*(\S+)\s*$/) {
> +	my ($type, $serverid) = (lc($1), $2);
>  	my $errmsg = undef; # set if you want to skip whole section
> -	eval { PVE::JSONSchema::pve_verify_configid($type); };
> +	eval { parse_status_server_id($serverid); };

again you remove the check of the type, but this is important

>  	$errmsg = $@ if $@;
>  	my $config = {}; # to return additional attributes
> -	return ($type, $type, $errmsg, $config);
> +	return ($type, $serverid, $errmsg, $config);
>      }
>      return undef;
>  }

this change is sadly not backwards-compatible, because now you have to
define a name, but i guess it should be optional, as to not break the 
old configs (also i guess most people will only ever have 1 influxdb or 
1 graphite export)

> @@ -79,4 +94,5 @@ sub update_storage_status {
>      die "please implement inside plugin";
>  }
>
> +
>  1;
>