On 6/12/19 4:34 PM, Fabian Grünbichler wrote:
> On Tue, Jun 11, 2019 at 07:36:32PM +0200, Thomas Lamprecht wrote:
>> This adds basic infrastructure for link[0..7] parameters and allows
>> to use it for the create cluster call. Try to be as link independent
>> as possible, i.e., no real link0 special handling, it'll only get set
>> if there's no single link passed.
>>
>> Signed-off-by: Thomas Lamprecht <t.lampre...@proxmox.com>
>
> there is still one '[0..1]' in API2/ClusterConfig.pm in
> $check_duplicate_addr, and pvecm 'add' only supports two links as well
> (even with the changes from #8 included)
>
>> ---
>>
>> new in v2
>>
>> data/PVE/API2/ClusterConfig.pm | 10 ++++++++--
>> data/PVE/Cluster.pm | 18 +++++++++++++++++
>> data/PVE/Corosync.pm | 36 ++++++++++++++++------------------
>> 3 files changed, 43 insertions(+), 21 deletions(-)
>>
>> diff --git a/data/PVE/API2/ClusterConfig.pm b/data/PVE/API2/ClusterConfig.pm
>> index dabdeb4..e4eeda7 100644
>> --- a/data/PVE/API2/ClusterConfig.pm
>> +++ b/data/PVE/API2/ClusterConfig.pm
>> @@ -26,6 +26,13 @@ my $nodeid_desc = {
>> };
>> PVE::JSONSchema::register_standard_option("corosync-nodeid", $nodeid_desc);
>>
>> +
>> +my $max_corosync_links = PVE::Cluster::get_max_corosync_links();
>> +my $corosync_link_properties = [ map { "link$_" } 0 ..
>> $max_corosync_links-1 ];
>> +my $corosync_link_options = {
>> + map { $_ => PVE::JSONSchema::get_standard_option('corosync-link') }
>> @$corosync_link_properties
>> +};
>> +
>> __PACKAGE__->register_method({
>> name => 'index',
>> path => '',
>> @@ -79,8 +86,7 @@ __PACKAGE__->register_method ({
>> minimum => 1,
>> optional => 1,
>> },
>> - link0 => get_standard_option('corosync-link'),
>> - link1 => get_standard_option('corosync-link'),
>> + %$corosync_link_options,
>> },
>> },
>> returns => { type => 'string' },
>> diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
>> index cc6431b..bd7c61b 100644
>> --- a/data/PVE/Cluster.pm
>> +++ b/data/PVE/Cluster.pm
>> @@ -1862,6 +1862,11 @@ my $corosync_link_desc = {
>> };
>> PVE::JSONSchema::register_standard_option("corosync-link",
>> $corosync_link_desc);
>>
>> +my $max_corosync_links = 8;
>> +sub get_max_corosync_links {
>> + return $max_corosync_links;
>> +}
>> +
>> sub parse_corosync_link {
>> my ($value) = @_;
>>
>> @@ -1870,6 +1875,19 @@ sub parse_corosync_link {
>> return PVE::JSONSchema::parse_property_string($corosync_link_format,
>> $value);
>> }
>>
>> +sub parse_corosync_links {
>> + my ($param) = @_;
>> + return undef if !defined($param);
>> +
>> + my $res = {};
>> + for my $k (keys %$param) {
>> + next if $k !~ /^link(\d+)$/;
>
> this RE is more lax than necessary ;) it's also used in other parts/#8,
> so it's probably a good idea to factor it out into a re-usable, correct
> and easily updatable one (e.g., if the number of links gets bumped
> further in the future).
It _is_ correct if seen with our parameter verification, and being lax
here was by design :-) But yes, having it on one place only make sense,
can make it strict too then.
>
>> + $res->{$1} = parse_corosync_link($param->{$k})
>> + }
>> +
>> + return $res;
>> +}
>> +
>> sub assert_joinable {
>> my ($local_addr, $link0, $link1, $force) = @_;
>>
>> diff --git a/data/PVE/Corosync.pm b/data/PVE/Corosync.pm
>> index e73e51d..063b7d5 100644
>> --- a/data/PVE/Corosync.pm
>> +++ b/data/PVE/Corosync.pm
>> @@ -201,11 +201,6 @@ sub create_conf {
>> 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 $conf = {
>> totem => {
>> version => 2, # protocol version
>> @@ -214,9 +209,6 @@ sub create_conf {
>> config_version => 0,
>> ip_version => 'ipv4-6',
>> interface => {
>> - 0 => {
>> - linknumber => 0,
>> - },
>> },
>> },
>> nodelist => {
>> @@ -225,7 +217,6 @@ sub create_conf {
>> name => $nodename,
>> nodeid => $nodeid,
>> quorum_votes => $votes,
>> - ring0_addr => $link0->{address},
>> },
>> },
>> },
>> @@ -238,19 +229,26 @@ sub create_conf {
>> },
>> };
>> my $totem = $conf->{totem};
>> + my $node = $conf->{nodelist}->{node}->{$nodename};
>>
>> - $totem->{interface}->{0}->{knet_link_priority} = $link0->{priority}
>> - if defined($link0->{priority});
>> + my $links = PVE::Cluster::parse_corosync_links(\%param);
>>
>> - my $link1 = PVE::Cluster::parse_corosync_link($param{link1});
>> - if ($link1->{address}) {
>> - $conf->{totem}->{interface}->{1} = {
>> - linknumber => 1,
>> - };
>> + my $linkcount = scalar(keys %$links);
>> + if ($linkcount < 1) {
>> + my $local_ip_address = PVE::Cluster::remote_node_ip($nodename);
>> + $links->{0}->{address} = $local_ip_address;
>> + } elsif ($linkcount > 1) {
>> $totem->{link_mode} = 'passive';
>
> move this into the constant part? it's the implicit default for
> single-interface config files anyway, and we want to use it for
> multi-interface configs by default as well..
>
> then this whole thing can become more simplified to
>
> $links->{0}->{address} = PVE::Cluster::remote_node_ip($nodename)
> if (scalar(keys %$links) == 0);
OK, makes sense
>
>> - $totem->{interface}->{1}->{knet_link_priority} = $link1->{priority}
>> - if defined($link1->{priority});
>> - $conf->{nodelist}->{node}->{$nodename}->{ring1_addr} =
>> $link1->{address};
>> + }
>> +
>> + for my $id (keys %$links) {
>> + my $l = $links->{$id};
>> +
>> + $node->{"ring${id}_addr"} = $l->{address};
>> +
>> + $totem->{interface}->{$id}->{linknumber} = $id;
>> + $totem->{interface}->{$id}->{knet_link_priority} = $l->{priority}
>> + if defined($l->{priority});
>> }
>>
>> return { main => $conf };
>> --
>> 2.20.1
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
