[pve-devel] [PATCH manager 3/3] Fix #2422: allow multiple Ceph public networks

[Fabian Grünbichler]

On March 11, 2020 4:22 pm, Alwin Antreich wrote:
> Multiple public networks can be defined in the ceph.conf. The networks
> need to be routed to each other.
> 
> On first service start the Ceph MON will register itself with one of the
> IPs configured locally, matching one of the public networks defined in
> the ceph.conf.
> 
> Signed-off-by: Alwin Antreich <a.antreich at proxmox.com>
> ---
>  PVE/API2/Ceph/MON.pm | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
> index 3baeac52..5128fea2 100644
> --- a/PVE/API2/Ceph/MON.pm
> +++ b/PVE/API2/Ceph/MON.pm
> @@ -33,11 +33,19 @@ my $find_mon_ip = sub {
>      }
>      $pubnet //= $cfg->{global}->{public_network};
>  
> +    my $public_nets = [ PVE::Tools::split_list($pubnet) ];
> +    warn "Multiple ceph public networks detected on $node: $pubnet\n".
> +    "Networks must be capable of routing to each other.\n" if scalar(@$public_nets) > 1;
> +

style nit: indentation/alignment, also - postfix if for multiline 
statement is confusing.

>      if (!$pubnet) {
>  	return $overwrite_ip // PVE::Cluster::remote_node_ip($node);
>      }

so here we check whether $pubnet is true, but in the new hunk above we 
already split and print it? works in practice (PVE::Tools::split_list 
handles undef fine, and then $public_nets does not have entries, so the 
warn does not trigger), but is the wrong way round ;)

>  
> -    my $allowed_ips = PVE::Network::get_local_ip_from_cidr($pubnet);
> +    my $allowed_ips;

could possibly be converted to a hash, and make the code using it below 
more readably by getting rid of 'check for duplicate/set membership via grep'

> +    foreach my $net (@$public_nets) {
> +        push @$allowed_ips, @{ PVE::Network::get_local_ip_from_cidr($net) };

would it make sense to catch errors from $net not being a valid CIDR 
string here? so that we get a nice error message, instead of whatever 
'ip' does with the invalid input/command line..

> +    }
> +
>      die "No active IP found for the requested ceph public network '$pubnet' on node '$node'\n"
>  	if scalar(@$allowed_ips) < 1;

further below there is still a code path that treats $pubnet as 
containing a single CIDR string:

62	die "Monitor IP '$overwrite_ip' not in ceph public network '$pubnet'\n"
63	    if !PVE::Network::is_ip_in_cidr($overwrite_ip, $pubnet);

>  
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>