[pve-devel] [RFC cluster 3/6] cluster create: use new corosync-link format for totem interfaces

[Thomas Lamprecht]

On 5/29/19 10:45 AM, Fabian Grünbichler wrote:
> On Tue, May 28, 2019 at 06:33:10PM +0200, Thomas Lamprecht wrote:
>> Preperation for enhanced compatibillity with new corosync 3/knet
>> transport. Pretty straight forward switch from ringX_addr to links,
>> *but*, for configuration backward compatibillity corosync still uses
>> ringX_addr as "link address", this will surely add confusion"link
>> address", this will surely add confusion ...
>>
>> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
>> ---
>>  data/PVE/API2/ClusterConfig.pm | 16 ++-------------
>>  data/PVE/Corosync.pm           | 37 +++++++++++++++++-----------------
>>  2 files changed, 20 insertions(+), 33 deletions(-)
>>
>> diff --git a/data/PVE/API2/ClusterConfig.pm b/data/PVE/API2/ClusterConfig.pm
>> index e7142b5..8b1fdbe 100644
>> --- a/data/PVE/API2/ClusterConfig.pm
>> +++ b/data/PVE/API2/ClusterConfig.pm
>> @@ -95,20 +95,8 @@ __PACKAGE__->register_method ({
>>  		minimum => 1,
>>  		optional => 1,
>>  	    },
>> -	    bindnet0_addr => {
>> -		type => 'string', format => 'ip',
>> -		description => "This specifies the network address the corosync ring 0".
>> -		    " executive should bind to and defaults to the local IP address of the node.",
>> -		optional => 1,
>> -	    },
>> -	    ring0_addr => get_standard_option('corosync-ring0-addr'),
>> -	    bindnet1_addr => {
>> -		type => 'string', format => 'ip',
>> -		description => "This specifies the network address the corosync ring 1".
>> -		    " executive should bind to and is optional.",
>> -		optional => 1,
>> -	    },
>> -	    ring1_addr => get_standard_option('corosync-ring1-addr'),
>> +	    link0 => get_standard_option('corosync-link'),
>> +	    link1 => get_standard_option('corosync-link'),
>>  	},
>>      },
>>      returns => { type => 'string' },
>> diff --git a/data/PVE/Corosync.pm b/data/PVE/Corosync.pm
>> index fea7258..8752c64 100644
>> --- a/data/PVE/Corosync.pm
>> +++ b/data/PVE/Corosync.pm
>> @@ -202,12 +202,14 @@ sub create_conf {
>>      my $votes = $param{votes} || 1;
>>  
>>      my $local_ip_address = PVE::Cluster::remote_node_ip($nodename);
>> -    my $ring0_addr = $param{ring0_addr} // $local_ip_address;
>> -    my $bindnet0_addr = $param{bindnet0_addr} // $ring0_addr;
>>  
>> -    my $use_ipv6 = ip_is_ipv6($ring0_addr);
>> -    die "ring 0 addresses must be from same IP family!\n"
>> -	if $use_ipv6 != ip_is_ipv6($bindnet0_addr);
>> +    my $link0 = PVE::Cluster::parse_corosync_link($param{link0});
>> +    $link0->{address} //= $local_ip_address;
>> +    $link0->{bindnet} //= $local_ip_address;
> 
> as before, bindnet can be skipped IMHO.
> 
>> +
>> +    my $use_ipv6 = ip_is_ipv6($link0->{address});
>> +    die "link 0 addresses must be from same IP family!\n"
>> +	if $use_ipv6 != ip_is_ipv6($link0->{bindnet});
> 
> same, and I'd argue for dropping $use_ipv6 altogether
> 
>>  
>>      my $conf = {
>>  	totem => {
>> @@ -218,7 +220,7 @@ sub create_conf {
>>  	    ip_version => $use_ipv6 ? 'ipv6' : 'ipv4',
> 
> and instead either set ip_version to 'ipv4-6' (ask DNS, prefer IPv4) or
> leave it out altogether (which defaults to 'ipv6-4' for knet).

OK, we still fallback to the IP resolved by node's hostname, so this
works for default cases too, I mean internally they hopefully don't
try to do a DNS lookup if they get an IP? man page does not specifies
the exact behavior, but it really should do so..

@Wolfgang, any issues you can think of with IPv6-4, i.e., prefer first
IPv6 addr found over IPv4 addr found?

> 
> ip_is_ipv6 only works for actual IPs and not hostnames, so we have the
> following scenarios:
> $link0->{address} is an IPv4 address, we set ip_version to ipv4, but
> there is nothing to resolve
> $link0->{address} is an IPv6 address, we set ip_version to ipv6, but
> there is nothing to resolve
> $link0->{address} is a hostname, we set ip_version to ipv4, but that
> hostname might only have an AAAA record set..



> 
>>  	    interface => {
>>  		0 => {
>> -		    bindnetaddr => $bindnet0_addr,
>> +		    bindnetaddr => $link0->{bindnet},
>>  		    linknumber => 0,
>>  		},
>>  	    },
>> @@ -229,7 +231,7 @@ sub create_conf {
>>  		    name => $nodename,
>>  		    nodeid => $nodeid,
>>  		    quorum_votes => $votes,
>> -		    ring0_addr => $ring0_addr,
>> +		    ring0_addr => $link0->{address},
>>  		},
>>  	    },
>>  	},
>> @@ -242,23 +244,20 @@ sub create_conf {
>>  	},
>>      };
>>  
>> -    die "Param bindnet1_addr set but ring1_addr not specified!\n"
>> -	if (defined($param{bindnet1_addr}) && !defined($param{ring1_addr}));
>> +    my $link1 = PVE::Cluster::parse_corosync_link($param{link1});
>> +    $link1->{bindnet} //= $link1->{address};
>>  
>> -    my $ring1_addr = $param{ring1_addr};
>> -    my $bindnet1_addr = $param{bindnet1_addr} // $param{ring1_addr};
>> -
>> -    if ($bindnet1_addr) {
>> -	die "ring 1 addresses must be from same IP family as ring 0!\n"
>> -	    if $use_ipv6 != ip_is_ipv6($bindnet1_addr) ||
>> -	       $use_ipv6 != ip_is_ipv6($ring1_addr);
>> +    if ($link1->{bindnet}) {
>> +	die "link 1 addresses must be from same IP family as link 0!\n"
>> +	    if $use_ipv6 != ip_is_ipv6($link1->{address}) ||
>> +	       $use_ipv6 != ip_is_ipv6($link1->{bindnet});
> 
> even if we keep bindnet, this is no longer true for knet - the family
> must only match on each link across all nodes, but not across links.

true that!

> 
>>  	$conf->{totem}->{interface}->{1} = {
>> -	    bindnetaddr => $bindnet1_addr,
>> +	    bindnetaddr => $link1->{bindnet},
>>  	    linknumber => 1,
>>  	};
>> -	$conf->{totem}->{rrp_mode} = 'passive';
>> -	$conf->{nodelist}->{node}->{$nodename}->{ring1_addr} = $ring1_addr;
>> +	$conf->{totem}->{link_mode} = 'passive';
>> +	$conf->{nodelist}->{node}->{$nodename}->{ring1_addr} = $link1->{address};
>>      }
>>  
>>      return { main => $conf };
>> -- 
>> 2.20.1