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

2020-04-15 Thread 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 
> ---
>  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@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


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

2020-03-11 Thread Alwin Antreich
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 
---
 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;
+
 if (!$pubnet) {
return $overwrite_ip // PVE::Cluster::remote_node_ip($node);
 }
 
-my $allowed_ips = PVE::Network::get_local_ip_from_cidr($pubnet);
+my $allowed_ips;
+foreach my $net (@$public_nets) {
+push @$allowed_ips, @{ PVE::Network::get_local_ip_from_cidr($net) };
+}
+
 die "No active IP found for the requested ceph public network '$pubnet' on 
node '$node'\n"
if scalar(@$allowed_ips) < 1;
 
-- 
2.20.1


___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel