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.rei...@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@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@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
