[pve-devel] [PATCH cluster 2/5] Enable support for up to 8 corosync links

Fabian Grünbichler f.gruenbichler at proxmox.com
Fri Nov 22 10:54:28 CET 2019


small nit inline

On November 20, 2019 5:43 pm, Stefan Reiter wrote:
> add_corosync_link_properties/extract_corosync_link_args are introduced
> as helpers to avoid hardcoding links in parameters=>properties on
> several occasions, while still providing autocompletion with pvecm by
> being seperate parameters instead of an array.
> 
> Maximum number of links is given as constant MAX_LINK_COUNT, should it
> change in the future.
> 
> All necessary functions have been updated to
> use the new $links array format instead of seperate $link0/$link1
> parameters, and call sites changed accordingly.
> 
> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> ---
> 
> This patch intentionally removes some verification and fallback code to be
> reintroduced in the next one.
> 
>  data/PVE/API2/ClusterConfig.pm | 51 +++++++------------
>  data/PVE/CLI/pvecm.pm          | 20 ++++----
>  data/PVE/Cluster/Setup.pm      | 22 +++++----
>  data/PVE/Corosync.pm           | 89 +++++++++++++++++++++++-----------
>  4 files changed, 100 insertions(+), 82 deletions(-)
> 
> diff --git a/data/PVE/API2/ClusterConfig.pm b/data/PVE/API2/ClusterConfig.pm
> index 3d778b8..241c14e 100644
> --- a/data/PVE/API2/ClusterConfig.pm
> +++ b/data/PVE/API2/ClusterConfig.pm
> @@ -68,10 +68,11 @@ __PACKAGE__->register_method ({
>      path => '',
>      method => 'POST',
>      protected => 1,
> -    description => "Generate new cluster configuration.",
> +    description => "Generate new cluster configuration. If no links given,"
> +		 . " default to local IP address as link0.",
>      parameters => {
>  	additionalProperties => 0,
> -	properties => {
> +	properties => PVE::Corosync::add_corosync_link_properties({
>  	    clustername => {
>  		description => "The name of the cluster.",
>  		type => 'string', format => 'pve-node',
> @@ -84,9 +85,7 @@ __PACKAGE__->register_method ({
>  		minimum => 1,
>  		optional => 1,
>  	    },
> -	    link0 => get_standard_option('corosync-link'),
> -	    link1 => get_standard_option('corosync-link'),
> -	},
> +	}),
>      },
>      returns => { type => 'string' },
>      code => sub {
> @@ -110,7 +109,7 @@ __PACKAGE__->register_method ({
>  	    my $nodename = PVE::INotify::nodename();
>  
>  	    # get the corosync basis config for the new cluster
> -	    my $config = PVE::Corosync::create_conf($nodename, %$param);
> +	    my $config = PVE::Corosync::create_conf($nodename, $param);
>  
>  	    print "Writing corosync config to /etc/pve/corosync.conf\n";
>  	    PVE::Corosync::atomic_write_conf($config);
> @@ -194,7 +193,7 @@ __PACKAGE__->register_method ({
>      description => "Adds a node to the cluster configuration. This call is for internal use.",
>      parameters => {
>  	additionalProperties => 0,
> -	properties => {
> +	properties => PVE::Corosync::add_corosync_link_properties({
>  	    node => get_standard_option('pve-node'),
>  	    nodeid => get_standard_option('corosync-nodeid'),
>  	    votes => {
> @@ -208,9 +207,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'),
> -	},
> +	}),
>      },
>      returns => {
>  	type => "object",
> @@ -256,7 +253,7 @@ __PACKAGE__->register_method ({
>  		while (my ($k, $v) = each %$nodelist) {
>  		    next if $k eq $name; # allows re-adding a node if force is set
>  
> -		    for my $linknumber (0..1) {
> +		    for my $linknumber (0..$PVE::Corosync::max_link_iter) {
>  			my $id = "ring${linknumber}_addr";
>  			next if !defined($v->{$id});
>  
> @@ -266,20 +263,10 @@ __PACKAGE__->register_method ({
>  		}
>  	    };
>  
> -	    my $link0 = PVE::Corosync::parse_corosync_link($param->{link0});
> -	    my $link1 = PVE::Corosync::parse_corosync_link($param->{link1});
> -
> -	    $check_duplicate_addr->($link0);
> -	    $check_duplicate_addr->($link1);
> -
> -	    # FIXME: handle all links (0-7), they're all independent now
> -	    $link0->{address} //= $name if exists($totem_cfg->{interface}->{0});
> -
> -	    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);
> +	    my $links = PVE::Corosync::extract_corosync_link_args($param);
> +	    foreach my $link (values %$links) {
> +		$check_duplicate_addr->($link);
> +	    }
>  
>  	    if (defined(my $res = $nodelist->{$name})) {
>  		$param->{nodeid} = $res->{nodeid} if !$param->{nodeid};
> @@ -317,13 +304,15 @@ __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);
>  	    $nodelist->{$name}->{quorum_votes} = $param->{votes} if $param->{votes};
>  
> +	    foreach my $link (keys %$links) {
> +		$nodelist->{$name}->{"ring${link}_addr"} = $links->{$link}->{address};
> +	    }
> +
>  	    PVE::Cluster::log_msg('notice', 'root at pam', "adding node $name to cluster");
>  
>  	    PVE::Corosync::update_nodelist($conf, $nodelist);
> @@ -495,7 +484,7 @@ __PACKAGE__->register_method ({
>      description => "Joins this node into an existing cluster.",
>      parameters => {
>  	additionalProperties => 0,
> -	properties => {
> +	properties => PVE::Corosync::add_corosync_link_properties({
>  	    hostname => {
>  		type => 'string',
>  		description => "Hostname (or IP) of an existing cluster member."
> @@ -512,17 +501,13 @@ __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'),
>  	    fingerprint => get_standard_option('fingerprint-sha256'),
>  	    password => {
>  		description => "Superuser (root) password of peer node.",
>  		type => 'string',
>  		maxLength => 128,
>  	    },
> -	},
> +	}),
>      },
>      returns => { type => 'string' },
>      code => sub {
> diff --git a/data/PVE/CLI/pvecm.pm b/data/PVE/CLI/pvecm.pm
> index 172ef8c..5856b56 100755
> --- a/data/PVE/CLI/pvecm.pm
> +++ b/data/PVE/CLI/pvecm.pm
> @@ -316,7 +316,7 @@ __PACKAGE__->register_method ({
>      description => "Adds the current node to an existing cluster.",
>      parameters => {
>      	additionalProperties => 0,
> -	properties => {
> +	properties => PVE::Corosync::add_corosync_link_properties({
>  	    hostname => {
>  		type => 'string',
>  		description => "Hostname (or IP) of an existing cluster member."
> @@ -333,8 +333,6 @@ __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'),
>  	    fingerprint => get_standard_option('fingerprint-sha256', {
>  		optional => 1,
>  	    }),
> @@ -343,7 +341,7 @@ __PACKAGE__->register_method ({
>  		description => "Always use SSH to join, even if peer may do it over API.",
>  		optional => 1,
>  	    },
> -	},
> +	}),
>      },
>      returns => { type => 'null' },
>  
> @@ -355,8 +353,7 @@ __PACKAGE__->register_method ({
>  	my $host = $param->{hostname};
>  	my $local_ip_address = PVE::Cluster::remote_node_ip($nodename);
>  
> -	my $link0 = PVE::Corosync::parse_corosync_link($param->{link0});
> -	my $link1 = PVE::Corosync::parse_corosync_link($param->{link1});
> +	my $links = PVE::Corosync::extract_corosync_link_args($param);
>  
>  	my $worker = sub {
>  
> @@ -381,7 +378,7 @@ __PACKAGE__->register_method ({
>  
>  	    # allow fallback to old ssh only join if wished or needed
>  
> -	    PVE::Cluster::Setup::assert_joinable($local_ip_address, $link0, $link1, $param->{force});
> +	    PVE::Cluster::Setup::assert_joinable($local_ip_address, $links, $param->{force});
>  
>  	    PVE::Cluster::Setup::setup_sshd_config();
>  	    PVE::Cluster::Setup::setup_rootsshconfig();
> @@ -399,10 +396,11 @@ __PACKAGE__->register_method ({
>  
>  	    push @$cmd, '--nodeid', $param->{nodeid} if $param->{nodeid};
>  	    push @$cmd, '--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
> -	    push @$cmd, '--link0', $param->{link0} // $local_ip_address;
> -	    push @$cmd, '--link1', $param->{link1} if defined($param->{link1});
> +
> +	    foreach my $link (keys %$links) {
> +		push @$cmd, "--link$link", PVE::JSONSchema::print_property_string(
> +		    $links->{$link}, get_standard_option('corosync-link'));
> +	    }
>  
>  	    if (system (@$cmd) != 0) {
>  		my $cmdtxt = join (' ', @$cmd);
> diff --git a/data/PVE/Cluster/Setup.pm b/data/PVE/Cluster/Setup.pm
> index 5569d6c..e8c5eca 100644
> --- a/data/PVE/Cluster/Setup.pm
> +++ b/data/PVE/Cluster/Setup.pm
> @@ -561,7 +561,7 @@ sub gen_pve_vzdump_files {
>  # join helpers
>  
>  sub assert_joinable {
> -    my ($local_addr, $link0, $link1, $force) = @_;
> +    my ($local_addr, $links, $force) = @_;
>  
>      my $errors = '';
>      my $error = sub { $errors .= "* $_[0]\n"; };
> @@ -604,8 +604,12 @@ 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);
> +
> +    if ($links) {
> +	foreach my $link (keys %$links) {
> +	    $check_ip->($links->{$link}->{address}, "link$link");
> +	}
> +    }
>  
>      if ($errors) {
>  	warn "detected the following error(s):\n$errors";
> @@ -619,11 +623,10 @@ sub join {
>      my $nodename = PVE::INotify::nodename();
>      my $local_ip_address = PVE::Cluster::remote_node_ip($nodename);
>  
> -    my $link0 = PVE::Corosync::parse_corosync_link($param->{link0});
> -    my $link1 = PVE::Corosync::parse_corosync_link($param->{link1});
> +    my $links = PVE::Corosync::extract_corosync_link_args($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();
> @@ -661,10 +664,9 @@ 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});
> -    # 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});
> +    foreach my $link (keys %$links) {
> +	$args->{"link$link"} = PVE::Corosync::print_corosync_link($links->{$link});
> +    }
>  
>      print "Request addition of this node\n";
>      my $res = eval { $conn->post("/cluster/config/nodes/$nodename", $args); };
> diff --git a/data/PVE/Corosync.pm b/data/PVE/Corosync.pm
> index 5973aaf..69927ff 100644
> --- a/data/PVE/Corosync.pm
> +++ b/data/PVE/Corosync.pm
> @@ -35,12 +35,15 @@ my $corosync_link_format = {
>  	minimum => 0,
>  	maximum => 255,
>  	default => 0,
> -	description => "The priority for the link when knet is used in 'passive' mode. Lower value means higher priority.",
> +	description => "The priority for the link when knet is used in 'passive'"
> +		     . " mode (default). Lower value means higher priority. Only"
> +		     . " valid for cluster create, ignored on node add.",
>      },
>  };
>  my $corosync_link_desc = {
>      type => 'string', format => $corosync_link_format,
> -    description => "Address and priority information of a single corosync link.",
> +    description => "Address and priority information of a single corosync link."
> +		 . " (up to 8 links supported; link0..link7)",
>      optional => 1,
>  };
>  PVE::JSONSchema::register_standard_option("corosync-link", $corosync_link_desc);
> @@ -53,6 +56,39 @@ sub parse_corosync_link {
>      return PVE::JSONSchema::parse_property_string($corosync_link_format, $value);
>  }
>  
> +sub print_corosync_link {
> +    my ($link) = @_;
> +
> +    return undef if !defined($link);
> +
> +    return PVE::JSONSchema::print_property_string($link, $corosync_link_format);
> +}
> +
> +use constant MAX_LINK_COUNT => 8;
> +our $max_link_iter = MAX_LINK_COUNT - 1;

why not export a constant of 7 for MAX_LINK_INDEX , since only that ever 
gets used? we know that arrays start their indices at 0 ;)

> +
> +sub add_corosync_link_properties {
> +    my ($prop) = @_;
> +
> +    for my $lnum (0..$max_link_iter) {
> +	$prop->{"link$lnum"} = PVE::JSONSchema::get_standard_option("corosync-link");
> +    }
> +
> +    return $prop;
> +}
> +
> +sub extract_corosync_link_args {
> +    my ($args) = @_;
> +
> +    my $links = {};
> +    for my $lnum (0..$max_link_iter) {
> +	$links->{$lnum} = parse_corosync_link($args->{"link$lnum"})
> +	    if $args->{"link$lnum"};
> +    }
> +
> +    return $links;
> +}
> +
>  # a very simply parser ...
>  sub parse_conf {
>      my ($filename, $raw) = @_;
> @@ -234,16 +270,19 @@ sub atomic_write_conf {
>  # for creating a new cluster with the current node
>  # params are those from the API/CLI cluster create call
>  sub create_conf {
> -    my ($nodename, %param) = @_;
> +    my ($nodename, $param) = @_;
>  
> -    my $clustername = $param{clustername};
> -    my $nodeid = $param{nodeid} || 1;
> -    my $votes = $param{votes} || 1;
> +    my $clustername = $param->{clustername};
> +    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 $links = extract_corosync_link_args($param);
> +
> +    # if no links given, fall back to local IP as link0
> +    $links->{0} = { address => $local_ip_address }
> +	if !%$links;
>  
>      my $conf = {
>  	totem => {
> @@ -252,11 +291,8 @@ sub create_conf {
>  	    cluster_name => $clustername,
>  	    config_version => 0,
>  	    ip_version => 'ipv4-6',
> -	    interface => {
> -		0 => {
> -		    linknumber => 0,
> -		},
> -	    },
> +	    link_mode => 'passive',
> +	    interface => {},
>  	},
>  	nodelist => {
>  	    node => {
> @@ -264,7 +300,6 @@ sub create_conf {
>  		    name => $nodename,
>  		    nodeid => $nodeid,
>  		    quorum_votes => $votes,
> -		    ring0_addr => $link0->{address},
>  		},
>  	    },
>  	},
> @@ -277,19 +312,17 @@ sub create_conf {
>  	},
>      };
>      my $totem = $conf->{totem};
> +    my $node = $conf->{nodelist}->{node}->{$nodename};
> +
> +    foreach my $lnum (keys %$links) {
> +	my $link = $links->{$lnum};
> +
> +	$totem->{interface}->{$lnum} = { linknumber => $lnum };
> +
> +	my $prio = $link->{priority};
> +	$totem->{interface}->{$lnum}->{knet_link_priority} = $prio if $prio;
>  
> -    $totem->{interface}->{0}->{knet_link_priority} = $link0->{priority}
> -	if defined($link0->{priority});
> -
> -    my $link1 = PVE::Cluster::parse_corosync_link($param{link1});
> -    if ($link1->{address}) {
> -	$conf->{totem}->{interface}->{1} = {
> -	    linknumber => 1,
> -	};
> -	$totem->{link_mode} = 'passive';
> -	$totem->{interface}->{1}->{knet_link_priority} = $link1->{priority}
> -	    if defined($link1->{priority});
> -	$conf->{nodelist}->{node}->{$nodename}->{ring1_addr} = $link1->{address};
> +	$node->{"ring${lnum}_addr"} = $link->{address};
>      }
>  
>      return { main => $conf };
> @@ -354,7 +387,7 @@ sub verify_conf {
>      if (%$interfaces) {
>  	# if interfaces are defined, *all* links must have a matching interface
>  	# definition, and vice versa
> -	for my $link (0..1) {
> +	for my $link (0..$max_link_iter) {
>  	    my $have_interface = defined($interfaces->{$link});
>  	    foreach my $node (@node_names) {
>  		my ($optname, $addr) = $find_option->($nodelist->{$node}, $link);
> @@ -372,7 +405,7 @@ sub verify_conf {
>  	}
>      } else {
>  	# without interfaces, only check that links are consistent among nodes
> -	for my $link (0..1) {
> +	for my $link (0..$max_link_iter) {
>  	    my $nodes_with_link = {};
>  	    foreach my $node (@node_names) {
>  		my ($optname, $addr) = $find_option->($nodelist->{$node}, $link);
> -- 
> 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