[pve-devel] [PATCH cluster v2 8/8] api: join/add: generalize and allow all knet links
Fabian Grünbichler
f.gruenbichler at proxmox.com
Wed Jun 12 16:35:00 CEST 2019
On Tue, Jun 11, 2019 at 07:36:33PM +0200, Thomas Lamprecht wrote:
> allows to join clusters with more than the two links/rings we and
> corosync supported earlier, further it reduced the special treatment
> of link0 a lot.
see previous patch for some RE-related comments
>
> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
> ---
>
> new in v2
>
> data/PVE/API2/ClusterConfig.pm | 37 ++++++++++++++++------------------
> data/PVE/Cluster.pm | 21 +++++++++++--------
> 2 files changed, 30 insertions(+), 28 deletions(-)
>
> diff --git a/data/PVE/API2/ClusterConfig.pm b/data/PVE/API2/ClusterConfig.pm
> index e4eeda7..58982b0 100644
> --- a/data/PVE/API2/ClusterConfig.pm
> +++ b/data/PVE/API2/ClusterConfig.pm
> @@ -204,8 +204,7 @@ __PACKAGE__->register_method ({
> description => "Do not throw error if node already exists.",
> optional => 1,
> },
> - link0 => get_standard_option('corosync-link'),
> - link1 => get_standard_option('corosync-link'),
> + %$corosync_link_options,
> },
> },
> returns => {
> @@ -250,20 +249,20 @@ __PACKAGE__->register_method ({
> }
> };
>
> - my $link0 = PVE::Cluster::parse_corosync_link($param->{link0});
> - my $link1 = PVE::Cluster::parse_corosync_link($param->{link1});
> + my $links = PVE::Cluster::parse_corosync_links($param);
>
> - $check_duplicate_addr->($link0);
> - $check_duplicate_addr->($link1);
> + for my $id (keys %$links) {
> + die "corosync: using 'link$id' parameter requires an interface with linknumber '$id' configured!\n"
> + if !defined($totem_cfg->{interface}->{$id});
> + $check_duplicate_addr->($links->{$id});
it would maybe make sense to change check_duplicate_addr to generate a
list of all currently configured addresses once, then generate another
list of addresses to-be-added here, and then check that those two lists
have no intersection?
otherwise we iterate over potentially hundreds of addresses up to eight
times here ;)
> + }
>
> - # FIXME: handle all links (0-7), they're all independent now
> - $link0->{address} //= $name if exists($totem_cfg->{interface}->{0});
> + for my $id (keys %{$totem_cfg->{interface}}) {
> + die "corosync: totem interface with linknumber $id configured but 'link$id' parameter not defined!\n"
> + if !defined($links->{id});
> + }
>
> - die "corosync: using 'link1' parameter needs a interface with linknumber '1' configured!\n"
> - if $link1 && !defined($totem_cfg->{interface}->{1});
> -
> - die "corosync: totem interface with linknumber 1 configured but 'link1' parameter not defined!\n"
> - if defined($totem_cfg->{interface}->{1}) && !defined($link1);
> + $links->{0}->{address} //= $name if exists($totem_cfg->{interface}->{0});
this is very confusing in combination with the code above - maybe we
should make address non-optional, and to get the behaviour of "just
choose the local node's IP as link0" the user/caller passes no linkX at
all? probably causes some churn in the other patches unfortunately, but
to me it seems like having a link parameter but no address only makes
sense in this one special case..
>
> if (defined(my $res = $nodelist->{$name})) {
> $param->{nodeid} = $res->{nodeid} if !$param->{nodeid};
> @@ -301,11 +300,12 @@ __PACKAGE__->register_method ({
> warn $@ if $@;
>
> $nodelist->{$name} = {
> - ring0_addr => $link0->{address},
> nodeid => $param->{nodeid},
> name => $name,
> };
> - $nodelist->{$name}->{ring1_addr} = $link1->{address} if defined($link1);
> + for my $id (keys %$links) {
> + $nodelist->{$name}->{"ring${id}_addr"} = $links->{$id};
> + }
> $nodelist->{$name}->{quorum_votes} = $param->{votes} if $param->{votes};
>
> PVE::Cluster::log_msg('notice', 'root at pam', "adding node $name to cluster");
> @@ -414,7 +414,7 @@ __PACKAGE__->register_method ({
> properties => {
> name => get_standard_option('pve-node'),
> nodeid => get_standard_option('corosync-nodeid'),
> - link0 => get_standard_option('corosync-link'),
> + %$corosync_link_options,
> quorum_votes => { type => 'integer', minimum => 0 },
> pve_addr => { type => 'string', format => 'ip' },
> pve_fp => get_standard_option('fingerprint-sha256'),
> @@ -484,10 +484,7 @@ __PACKAGE__->register_method ({
> description => "Do not throw error if node already exists.",
> optional => 1,
> },
> - link0 => get_standard_option('corosync-link', {
> - default => "IP resolved by node's hostname",
> - }),
> - link1 => get_standard_option('corosync-link'),
> + %$corosync_link_options,
> fingerprint => get_standard_option('fingerprint-sha256'),
> password => {
> description => "Superuser (root) password of peer node.",
> diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
> index bd7c61b..31a40d3 100644
> --- a/data/PVE/Cluster.pm
> +++ b/data/PVE/Cluster.pm
> @@ -1889,7 +1889,7 @@ sub parse_corosync_links {
> }
>
> sub assert_joinable {
> - my ($local_addr, $link0, $link1, $force) = @_;
> + my ($local_addr, $links, $force) = @_;
>
> my $errors = '';
> my $error = sub { $errors .= "* $_[0]\n"; };
> @@ -1932,8 +1932,10 @@ sub assert_joinable {
> };
>
> $check_ip->($local_addr, 'local node address');
> - $check_ip->($link0->{address}, 'ring0') if defined($link0);
> - $check_ip->($link1->{address}, 'ring1') if defined($link1);
> +
> + for my $id (keys %$links) {
> + $check_ip->($links->{$id}->{address}, "link${id}");
> + }
>
> if ($errors) {
> warn "detected the following error(s):\n$errors";
> @@ -1973,11 +1975,10 @@ sub join {
> my $nodename = PVE::INotify::nodename();
> my $local_ip_address = remote_node_ip($nodename);
>
> - my $link0 = parse_corosync_link($param->{link0});
> - my $link1 = parse_corosync_link($param->{link1});
> + my $links = parse_corosync_links($param);
>
> # check if we can join with the given parameters and current node state
> - assert_joinable($local_ip_address, $link0, $link1, $param->{force});
> + assert_joinable($local_ip_address, $links, $param->{force});
>
> setup_sshd_config();
> setup_rootsshconfig();
> @@ -2017,8 +2018,12 @@ sub join {
> $args->{votes} = $param->{votes} if defined($param->{votes});
> # 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;
> - $args->{link1} = $param->{link1} if defined($param->{link1});
> +
> + $args->{$_} = $param->{$_} for grep { /^link\d+$/ } keys %$param;
> +
> + # only set this if no link was passed, as one could theoretically also
> + # just pass link3 if the cluster has historically changed to use that one
> + $args->{link0} //= $local_ip_address if !grep { /^link\d+$/ } keys %$args;
>
> print "Request addition of this node\n";
> my $res = $conn->post("/cluster/config/nodes/$nodename", $args);
> --
> 2.20.1
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
More information about the pve-devel
mailing list