[pve-devel] [PATCH cluster 5/5] corosync: check if IPs are configured on cluster create

[Thomas Lamprecht]

On 11/20/19 5:43 PM, Stefan Reiter wrote:
> Same as we do in assert_joinable, a cluster with unconfigured IPs will
> fail start after creation anyway.
> 
> Make "check_ip" a standalone sub ("check_ip_configured") and improve
> error messages all around.
> 
> Also move call to create_conf up, so if it fails we exit before SSH and
> auth-key setup.
> 
> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> ---
>  data/PVE/API2/ClusterConfig.pm | 10 ++++-----
>  data/PVE/Cluster/Setup.pm      | 40 ++++++++++------------------------
>  data/PVE/Corosync.pm           | 29 ++++++++++++++++++++++++
>  3 files changed, 45 insertions(+), 34 deletions(-)
> 
> diff --git a/data/PVE/API2/ClusterConfig.pm b/data/PVE/API2/ClusterConfig.pm
> index bff0c48..0454947 100644
> --- a/data/PVE/API2/ClusterConfig.pm
> +++ b/data/PVE/API2/ClusterConfig.pm
> @@ -115,6 +115,11 @@ __PACKAGE__->register_method ({
>  	my $authuser = $rpcenv->get_user();
>  
>  	my $code = sub {
> +	    my $nodename = PVE::INotify::nodename();
> +
> +	    # get the corosync basis config for the new cluster
> +	    my $config = PVE::Corosync::create_conf($nodename, $param);
> +
>  	    STDOUT->autoflush();
>  	    PVE::Cluster::Setup::setup_sshd_config(1);
>  	    PVE::Cluster::Setup::setup_rootsshconfig();
> @@ -124,11 +129,6 @@ __PACKAGE__->register_method ({
>  		if !-f $authfile;
>  	    die "no authentication key available\n" if -f !$authfile;
>  
> -	    my $nodename = PVE::INotify::nodename();
> -
> -	    # get the corosync basis config for the new cluster
> -	    my $config = PVE::Corosync::create_conf($nodename, $param);
> -
>  	    print "Writing corosync config to /etc/pve/corosync.conf\n";
>  	    PVE::Corosync::atomic_write_conf($config);
>  
> diff --git a/data/PVE/Cluster/Setup.pm b/data/PVE/Cluster/Setup.pm
> index c6f6c9e..77d3664 100644
> --- a/data/PVE/Cluster/Setup.pm
> +++ b/data/PVE/Cluster/Setup.pm
> @@ -566,55 +566,37 @@ sub assert_joinable {
>      my ($local_addr, $links, $force) = @_;
>  
>      my $errors = '';
> -    my $error = sub { $errors .= "* $_[0]\n"; };
> +    my $error = sub { $errors .= "* $_[0]"; };
>  
>      if (-f $authfile) {
> -	$error->("authentication key '$authfile' already exists");
> +	$error->("authentication key '$authfile' already exists\n");
>      }
>  
>      if (-f $clusterconf)  {
> -	$error->("cluster config '$clusterconf' already exists");
> +	$error->("cluster config '$clusterconf' already exists\n");
>      }
>  
>      my $vmlist = PVE::Cluster::get_vmlist();
>      if ($vmlist && $vmlist->{ids} && scalar(keys %{$vmlist->{ids}})) {
> -	$error->("this host already contains virtual guests");
> +	$error->("this host already contains virtual guests\n");
>      }

please avoid that manual \n, I did that with reason.. maype use chomp
on the string for your $@ case below (or in the errors sub?

>  
>      if (PVE::Tools::run_command(['corosync-quorumtool', '-l'], noerr => 1, quiet => 1) == 0) {
> -	$error->("corosync is already running, is this node already in a cluster?!");
> -    }
> -
> -    # check if corosync ring IPs are configured on the current nodes interfaces
> -    my $check_ip = sub {
> -	my $ip = shift // return;
> -	my $logid = shift;
> -	if (!PVE::JSONSchema::pve_verify_ip($ip, 1)) {
> -	    my $host = $ip;
> -	    eval { $ip = PVE::Network::get_ip_from_hostname($host); };
> -	    if ($@) {
> -		$error->("$logid: cannot use '$host': $@\n") ;
> -		return;
> -	    }
> -	}
> -
> -	my $cidr = (Net::IP::ip_is_ipv6($ip)) ? "$ip/128" : "$ip/32";
> -	my $configured_ips = PVE::Network::get_local_ip_from_cidr($cidr);
> -
> -	$error->("$logid: cannot use IP '$ip', it must be configured exactly once on local node!\n")
> -	    if (scalar(@$configured_ips) != 1);
> -    };
> +	$error->("corosync is currently running, is this node already in a cluster?\n");
> +    }
>  
> -    $check_ip->($local_addr, 'local node address');
> +    eval { PVE::Corosync::check_ip_configured($local_addr, 'local node address'); };
> +    $error->($@) if $@;
>  
>      if ($links) {
>  	foreach my $link (keys %$links) {
> -	    $check_ip->($links->{$link}->{address}, "link$link");
> +	    eval { PVE::Corosync::check_ip_configured($links->{$link}->{address}, "link$link"); };
> +	    $error->($@) if $@;
>  	}
>      }
>  
>      if ($errors) {
> -	warn "detected the following error(s):\n$errors";
> +	warn "Detected the following error(s):\n$errors";
>  	die "Check if node may join a cluster failed!\n" if !$force;
>      }
>  }
> diff --git a/data/PVE/Corosync.pm b/data/PVE/Corosync.pm
> index 69927ff..19c8976 100644
> --- a/data/PVE/Corosync.pm
> +++ b/data/PVE/Corosync.pm
> @@ -11,6 +11,7 @@ use Socket qw(AF_INET AF_INET6 inet_ntop);
>  
>  use PVE::Cluster;
>  use PVE::JSONSchema;
> +use PVE::Network;
>  use PVE::Tools;
>  use PVE::Tools qw($IPV4RE $IPV6RE);
>  
> @@ -267,6 +268,32 @@ sub atomic_write_conf {
>  	|| die "activating corosync.conf.new failed - $!\n";
>  }
>  
> +# check if an IP address is configured exactly once on the local node, so that
> +# corosync can safely use it as a link
> +sub check_ip_configured {
> +    my ($ip, $logid) = @_;
> +
> +    return if !defined($ip);
> +    $logid //= "error";
> +
> +    if (!PVE::JSONSchema::pve_verify_ip($ip, 1)) {
> +	my $host = $ip;
> +	eval { $ip = PVE::Network::get_ip_from_hostname($host); };
> +	if ($@) {
> +	    die "$logid: cannot use '$host': $@\n";
> +	}
> +    }
> +
> +    my $cidr = (Net::IP::ip_is_ipv6($ip)) ? "$ip/128" : "$ip/32";
> +    my $configured_ips = PVE::Network::get_local_ip_from_cidr($cidr);
> +
> +    die "$logid: cannot use IP '$ip', it is not configured on local node (must exist exactly once)!\n"
> +	if (scalar(@$configured_ips) < 1);
> +
> +    die "$logid: cannot use IP '$ip', it is configured multiple times on local node (must exist exactly once)!\n"
> +	if (scalar(@$configured_ips) > 1);
> +};
> +
>  # for creating a new cluster with the current node
>  # params are those from the API/CLI cluster create call
>  sub create_conf {
> @@ -317,6 +344,8 @@ sub create_conf {
>      foreach my $lnum (keys %$links) {
>  	my $link = $links->{$lnum};
>  
> +	check_ip_configured($link->{address}, "link$lnum");
> +
>  	$totem->{interface}->{$lnum} = { linknumber => $lnum };
>  
>  	my $prio = $link->{priority};
>