[pve-devel] [PATCH cluster v2 5/8] node join: use new corosync link parameters
Thomas Lamprecht
t.lamprecht at proxmox.com
Thu Jun 13 10:59:44 CEST 2019
On 6/12/19 4:34 PM, Fabian Grünbichler wrote:
> On Tue, Jun 11, 2019 at 07:36:30PM +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.
>>
>> As link0, former ring0, is not special anymore allow that it's not
>> passed and only default back to nodename if it's configured in the
>> totem section of the configuration.
>>
>> As the 'join' and 'addnode' api paths are quite connected, do all in
>> one patch.
>>
>> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
>> ---
>>
>> changes v1 (RFC) -> v2:
>> * only add fallback for link0 if a interface reference in totem exists
>>
>> NOTE: this is a more intermediate patch, will be greatly replaced by the
>> less static "arbitrary-link-handling" later in this series
>>
>> data/PVE/API2/ClusterConfig.pm | 46 +++++++++++++++++++++-------------
>> data/PVE/CLI/pvecm.pm | 15 +++++++----
>> data/PVE/Cluster.pm | 18 +++++++------
>> 3 files changed, 49 insertions(+), 30 deletions(-)
>>
>> diff --git a/data/PVE/API2/ClusterConfig.pm b/data/PVE/API2/ClusterConfig.pm
>> index 8b1fdbe..9c91880 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,37 @@ __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;
>
> nit: inconsistent style
ok fixed for v3
>
>>
>> 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;
>
> might be a nice addition to include the link number as well?
$id _is_ the linknumber, but definitely used in a confusing way, re-worded.
also, you probably meant to also include the link which we currently check,
this works fine here for two links (as the address is printed out) but with
8/8 it surely isn't as obvious, so I'll try to add it there.
> [snip]
>> index 3806416..0a20af3 100755
>> --- a/data/PVE/CLI/pvecm.pm
>> +++ b/data/PVE/CLI/pvecm.pm
>> @@ -334,8 +334,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'),
>> fingerprint => get_standard_option('fingerprint-sha256', {
>> optional => 1,
>> }),
>> @@ -356,7 +356,10 @@ __PACKAGE__->register_method ({
>> my $host = $param->{hostname};
>> my $local_ip_address = PVE::Cluster::remote_node_ip($nodename);
>>
>> - PVE::Cluster::assert_joinable($local_ip_address, $param->{ring0_addr}, $param->{ring1_addr}, $param->{force});
>> + my $link0 = PVE::Cluster::parse_corosync_link($param->{link0});
>> + my $link1 = PVE::Cluster::parse_corosync_link($param->{link1});
>> +
>> + PVE::Cluster::assert_joinable($local_ip_address, $link0, $link1, $param->{force});
>>
>> my $worker = sub {
>>
>> @@ -398,8 +401,10 @@ __PACKAGE__->register_method ({
>>
>> push @$cmd, '--nodeid', $param->{nodeid} if $param->{nodeid};
>> push @$cmd, '--votes', $param->{votes} if defined($param->{votes});
>> - push @$cmd, '--ring0_addr', $param->{ring0_addr} // $local_ip_address;
>> - push @$cmd, '--ring1_addr', $param->{ring1_addr} if defined($param->{ring1_addr});
>> + # just pass the un-parsed string through, or as we've address as
>> + # the default_key, we can just pass the fallback directly too
>> + push @$cmd, '--link0', $param->{link0} // $local_ip_address;
>
> shouldn't this also be conditionalized upon $totem->{interface}->{0}
> existing, like in the addnode API call above?
"use_ssh" was a bit neglected by me here, tbh..
Fact is, I do not have the join info here (yet), so I do not have $totem
info at all. But it probably makes things and checks easier if I'd had it
here, so we probably want to get it, depending on use_ssh either:
* after $api->login in sub join for use_api -> false
* connect over ssh and get the info with pvesh get /cluster/config/join --output-format=json
then we could do those checks and defaults here.
Alternatively we just pass along an additional parameter with the
fallback IP we want to use and let the cluster node which processes the
join request decide if it's needed or not, it would have the info already,
maybe easier for now..
>
>> + push @$cmd, '--link1', $param->{link1} if defined($param->{link1});
>>
>> if (system (@$cmd) != 0) {
>> my $cmdtxt = join (' ', @$cmd);
>> diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
>> index c06b0bf..cc6431b 100644
>> --- a/data/PVE/Cluster.pm
>> +++ b/data/PVE/Cluster.pm
>> @@ -1871,7 +1871,7 @@ sub parse_corosync_link {
>> }
>>
>> sub assert_joinable {
>> - my ($local_addr, $ring0_addr, $ring1_addr, $force) = @_;
>> + my ($local_addr, $link0, $link1, $force) = @_;
>>
>> my $errors = '';
>> my $error = sub { $errors .= "* $_[0]\n"; };
>> @@ -1914,8 +1914,8 @@ sub assert_joinable {
>> };
>>
>> $check_ip->($local_addr, 'local node address');
>> - $check_ip->($ring0_addr, 'ring0');
>> - $check_ip->($ring1_addr, 'ring1');
>> + $check_ip->($link0->{address}, 'ring0') if defined($link0);
>> + $check_ip->($link1->{address}, 'ring1') if defined($link1);
>>
>> if ($errors) {
>> warn "detected the following error(s):\n$errors";
>> @@ -1955,9 +1955,11 @@ sub join {
>> my $nodename = PVE::INotify::nodename();
>> my $local_ip_address = remote_node_ip($nodename);
>>
>> - my ($ring0_addr, $ring1_addr) = $param->@{'ring0_addr', 'ring1_addr'};
>> + my $link0 = parse_corosync_link($param->{link0});
>> + my $link1 = parse_corosync_link($param->{link1});
>> +
>> # check if we can join with the given parameters and current node state
>> - assert_joinable($local_ip_address, $ring0_addr, $ring1_addr, $param->{force});
>> + assert_joinable($local_ip_address, $link0, $link1, $param->{force});
>>
>> setup_sshd_config();
>> setup_rootsshconfig();
>> @@ -1995,8 +1997,10 @@ sub join {
>> $args->{force} = $param->{force} if defined($param->{force});
>> $args->{nodeid} = $param->{nodeid} if $param->{nodeid};
>> $args->{votes} = $param->{votes} if defined($param->{votes});
>> - $args->{ring0_addr} = $ring0_addr // $local_ip_address;
>> - $args->{ring1_addr} = $ring1_addr if defined($ring1_addr);
>> + # just pass the un-parsed string through, or as we've address as the
>> + # default_key, we can just pass the fallback directly too
>> + $args->{link0} = $param->{link0} // $local_ip_address;
>
> same here, but this one gets changed in a later patch..
also same here as above, not in cluster yet, and no join_info (yet)
so no $totem :-) I'll probably add a join_info call here, though.
>
>> + $args->{link1} = $param->{link1} if defined($param->{link1});
>>
>> print "Request addition of this node\n";
>> my $res = $conn->post("/cluster/config/nodes/$nodename", $args);
>> --
>> 2.20.1
>>
More information about the pve-devel
mailing list