[pve-devel] [RFC cluster 5/6] node join: use new corosync link parameters

[Thomas Lamprecht]

On 5/29/19 10:50 AM, Fabian Grünbichler wrote:
> On Tue, May 28, 2019 at 06:33:12PM +0200, Thomas Lamprecht wrote:
>> Similar to the change to cluster creation use now also the
>> corosync-link definition for the rest of the cluster join/add calls.
>>
>> What could be a bit strange is that bindnet shows here up too in the
>> schema definition, while it's documented that it's only really used
>> for cluster created we may want to "hide" it here, but that's
>> details, and as long as we adapt the docs clearly and users can use
>> the WebUI I think this can stay as is..
>>
>> As join and addnode is quite connected, do all in one patch.
>>
>> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
>> ---
>>  data/PVE/API2/ClusterConfig.pm | 45 ++++++++++++++++++++--------------
>>  data/PVE/CLI/pvecm.pm          | 15 ++++++++----
>>  data/PVE/Cluster.pm            | 18 ++++++++------
>>  3 files changed, 48 insertions(+), 30 deletions(-)
>>
>> diff --git a/data/PVE/API2/ClusterConfig.pm b/data/PVE/API2/ClusterConfig.pm
>> index 8b1fdbe..366f914 100644
>> --- a/data/PVE/API2/ClusterConfig.pm
>> +++ b/data/PVE/API2/ClusterConfig.pm
>> @@ -214,8 +214,8 @@ __PACKAGE__->register_method ({
>>  		description => "Do not throw error if node already exists.",
>>  		optional => 1,
>>  	    },
>> -	    ring0_addr => get_standard_option('corosync-ring0-addr'),
>> -	    ring1_addr => get_standard_option('corosync-ring1-addr'),
>> +	    link0 => get_standard_option('corosync-link'),
>> +	    link1 => get_standard_option('corosync-link'),
>>  	},
>>      },
>>      returns => {
>> @@ -243,27 +243,36 @@ __PACKAGE__->register_method ({
>>  
>>  	    # ensure we do not reuse an address, that can crash the whole cluster!
>>  	    my $check_duplicate_addr = sub {
>> -		my $addr = shift;
>> -		return if !defined($addr);
>> +		my $link = shift;
>> +		return if !defined($link);
>> +		my $addr = $link->{address} // return;
>>  
>>  		while (my ($k, $v) = each %$nodelist) {
>>  		    next if $k eq $name; # allows re-adding a node if force is set
>> -		    if ($v->{ring0_addr} eq $addr || ($v->{ring1_addr} && $v->{ring1_addr} eq $addr)) {
>> -			die "corosync: address '$addr' already defined by node '$k'\n";
>> +
>> +		    for my $linknumber (0..1) {
>> +			my $id = "ring${linknumber}_addr";
>> +			next if !defined($v->{$id});
>> +
>> +			die "corosync $id: address '$addr' already defined by node '$k'\n"
>> +			    if $v->{$id} eq $addr;
>>  		    }
>>  		}
>>  	    };
>>  
>> -	    &$check_duplicate_addr($param->{ring0_addr});
>> -	    &$check_duplicate_addr($param->{ring1_addr});
>> +	    my $link0 = PVE::Cluster::parse_corosync_link($param->{link0});
>> +	    my $link1 = PVE::Cluster::parse_corosync_link($param->{link1});
>>  
>> -	    $param->{ring0_addr} = $name if !$param->{ring0_addr};
>> +	    $check_duplicate_addr->($link0);
>> +	    $check_duplicate_addr->($link1);
>>  
>> -	    die "corosync: using 'ring1_addr' parameter needs a configured ring 1 interface!\n"
>> -		if $param->{ring1_addr} && !defined($totem_cfg->{interface}->{1});
>> +	    $link0->{address} //= $name;
> 
> I wonder whether we should support clusters without link0 as well? might
> happen easily:
> 
> cluster only has link0 configured
> network topology change, add new link1
> deconfigure link0
> 
> cluster joining no longer possible without specifying a bogus link0
> address and manually cleaning up corosync.conf afterwards?
> 
> in general, we probably want to remove any special handling of link0/1,
> and just set fallback defaults on link0 if no other link is specified..

yes, if we want to have support for this, and as people surely want to do
this, as corosync/knet advertises this feature, we have to want...

I'll try to free it from link0 special casing in a reasonable manner, e.g.,
if no link is passed and the config has only one link we want to fallback
to resolving the node name and setting it as address to the used link,
whatever ID it has, else, if more links are used -> error out and tell user
which links need to be passed, semi-intelligent, so to say.

> 
>> -	    die "corosync: ring 1 interface configured but 'ring1_addr' parameter not defined!\n"
>> -		if defined($totem_cfg->{interface}->{1}) && !defined($param->{ring1_addr});
>> +	    die "corosync: using 'link1' parameter needs a interface with linknumber '1' configured!\n"
>> +		if $link1 && !defined($totem_cfg->{interface}->{1});
> 
> for completeness sake: this is no longer true (at least if you don't care about setting
> priorities ;)).
> 
> it still might make sense to keep this semantics/check since it enables
> the reverse safeguard below:
> 
>> +
>> +	    die "corosync: totem interface with linknumber 1 configured but 'link1' parameter not defined!\n"
>> +		if defined($totem_cfg->{interface}->{1}) && !defined($link1);
> 
> ^^^
> 
> that is to say - no additional links without additional interfaces, and
> no nodes without additional links for all configured additional
> interfaces.
> 

yes, agreed, see above