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

[Thomas Lamprecht]

On 6/12/19 4:34 PM, Fabian Grünbichler wrote:
> 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).

It _is_ correct if seen with our parameter verification, and being lax
here was by design :-) But yes, having it on one place only make sense,
can make it strict too then.

> 
>> +	$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);

OK, makes sense

> 
>> -	$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