[pve-devel] [PATCH cluster v2 7/8] allow to create a cluster with all possible knet links

[Fabian Grünbichler]

On Tue, Jun 11, 2019 at 07:36:32PM +0200, Thomas Lamprecht wrote:
> This adds basic infrastructure for link[0..7] parameters and allows
> to use it for the create cluster call. Try to be as link independent
> as possible, i.e., no real link0 special handling, it'll only get set
> if there's no single link passed.
> 
> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>

there is still one '[0..1]' in API2/ClusterConfig.pm in
$check_duplicate_addr, and pvecm 'add' only supports two links as well
(even with the changes from #8 included)

> ---
> 
> new in v2
> 
>  data/PVE/API2/ClusterConfig.pm | 10 ++++++++--
>  data/PVE/Cluster.pm            | 18 +++++++++++++++++
>  data/PVE/Corosync.pm           | 36 ++++++++++++++++------------------
>  3 files changed, 43 insertions(+), 21 deletions(-)
> 
> diff --git a/data/PVE/API2/ClusterConfig.pm b/data/PVE/API2/ClusterConfig.pm
> index dabdeb4..e4eeda7 100644
> --- a/data/PVE/API2/ClusterConfig.pm
> +++ b/data/PVE/API2/ClusterConfig.pm
> @@ -26,6 +26,13 @@ my $nodeid_desc = {
>  };
>  PVE::JSONSchema::register_standard_option("corosync-nodeid", $nodeid_desc);
>  
> +
> +my $max_corosync_links = PVE::Cluster::get_max_corosync_links();
> +my $corosync_link_properties = [ map { "link$_" } 0 .. $max_corosync_links-1 ];
> +my $corosync_link_options = {
> +    map { $_ => PVE::JSONSchema::get_standard_option('corosync-link') } @$corosync_link_properties
> +};
> +
>  __PACKAGE__->register_method({
>      name => 'index',
>      path => '',
> @@ -79,8 +86,7 @@ __PACKAGE__->register_method ({
>  		minimum => 1,
>  		optional => 1,
>  	    },
> -	    link0 => get_standard_option('corosync-link'),
> -	    link1 => get_standard_option('corosync-link'),
> +	    %$corosync_link_options,
>  	},
>      },
>      returns => { type => 'string' },
> diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
> index cc6431b..bd7c61b 100644
> --- a/data/PVE/Cluster.pm
> +++ b/data/PVE/Cluster.pm
> @@ -1862,6 +1862,11 @@ my $corosync_link_desc = {
>  };
>  PVE::JSONSchema::register_standard_option("corosync-link", $corosync_link_desc);
>  
> +my $max_corosync_links = 8;
> +sub get_max_corosync_links {
> +    return $max_corosync_links;
> +}
> +
>  sub parse_corosync_link {
>      my ($value) = @_;
>  
> @@ -1870,6 +1875,19 @@ sub parse_corosync_link {
>      return PVE::JSONSchema::parse_property_string($corosync_link_format, $value);
>  }
>  
> +sub parse_corosync_links {
> +    my ($param) = @_;
> +    return undef if !defined($param);
> +
> +    my $res = {};
> +    for my $k (keys %$param) {
> +	next if $k !~ /^link(\d+)$/;

this RE is more lax than necessary ;) it's also used in other parts/#8,
so it's probably a good idea to factor it out into a re-usable, correct
and easily updatable one (e.g., if the number of links gets bumped
further in the future).

> +	$res->{$1} = parse_corosync_link($param->{$k})
> +    }
> +
> +    return $res;
> +}
> +
>  sub assert_joinable {
>      my ($local_addr, $link0, $link1, $force) = @_;
>  
> diff --git a/data/PVE/Corosync.pm b/data/PVE/Corosync.pm
> index e73e51d..063b7d5 100644
> --- a/data/PVE/Corosync.pm
> +++ b/data/PVE/Corosync.pm
> @@ -201,11 +201,6 @@ sub create_conf {
>      my $nodeid = $param{nodeid} || 1;
>      my $votes = $param{votes} || 1;
>  
> -    my $local_ip_address = PVE::Cluster::remote_node_ip($nodename);
> -
> -    my $link0 = PVE::Cluster::parse_corosync_link($param{link0});
> -    $link0->{address} //= $local_ip_address;
> -
>      my $conf = {
>  	totem => {
>  	    version => 2, # protocol version
> @@ -214,9 +209,6 @@ sub create_conf {
>  	    config_version => 0,
>  	    ip_version => 'ipv4-6',
>  	    interface => {
> -		0 => {
> -		    linknumber => 0,
> -		},
>  	    },
>  	},
>  	nodelist => {
> @@ -225,7 +217,6 @@ sub create_conf {
>  		    name => $nodename,
>  		    nodeid => $nodeid,
>  		    quorum_votes => $votes,
> -		    ring0_addr => $link0->{address},
>  		},
>  	    },
>  	},
> @@ -238,19 +229,26 @@ sub create_conf {
>  	},
>      };
>      my $totem = $conf->{totem};
> +    my $node = $conf->{nodelist}->{node}->{$nodename};
>  
> -    $totem->{interface}->{0}->{knet_link_priority} = $link0->{priority}
> -	if defined($link0->{priority});
> +    my $links = PVE::Cluster::parse_corosync_links(\%param);
>  
> -    my $link1 = PVE::Cluster::parse_corosync_link($param{link1});
> -    if ($link1->{address}) {
> -	$conf->{totem}->{interface}->{1} = {
> -	    linknumber => 1,
> -	};
> +    my $linkcount = scalar(keys %$links);
> +    if ($linkcount < 1) {
> +	my $local_ip_address = PVE::Cluster::remote_node_ip($nodename);
> +	$links->{0}->{address} = $local_ip_address;
> +    } elsif ($linkcount > 1) {
>  	$totem->{link_mode} = 'passive';

move this into the constant part? it's the implicit default for
single-interface config files anyway, and we want to use it for
multi-interface configs by default as well..

then this whole thing can become more simplified to

$links->{0}->{address} = PVE::Cluster::remote_node_ip($nodename)
    if (scalar(keys %$links) == 0);

> -	$totem->{interface}->{1}->{knet_link_priority} = $link1->{priority}
> -	    if defined($link1->{priority});
> -	$conf->{nodelist}->{node}->{$nodename}->{ring1_addr} = $link1->{address};
> +    }
> +
> +    for my $id (keys %$links) {
> +	my $l = $links->{$id};
> +
> +	$node->{"ring${id}_addr"} = $l->{address};
> +
> +	$totem->{interface}->{$id}->{linknumber} = $id;
> +	$totem->{interface}->{$id}->{knet_link_priority} = $l->{priority}
> +	    if defined($l->{priority});
>      }
>  
>      return { main => $conf };
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel