[pve-devel] [PATCH v2 cluster 5/5] Add cluster join API version check

[Thomas Lamprecht]

On 12/4/19 3:04 PM, Stefan Reiter wrote:
> Adds API call GET /cluster/config/apiversion to retrieve remote clusters
> join-API version (0 is assumed for versions without this endpoint).
> 
> Introduce API_AGE similar to storage plugin API, but with two ages for
> cluster/joinee roles. Currently, all versions are intercompatible.
> 
> For future usage, a new 'addnode' parameter 'apiversion' is introduced,
> to allow introducing API breakages for joining nodes as well.
> 
> As a first compatibility check, use new fallback method only if
> available. This ensures full compatibility between nodes/clusters with
> and without new fallback behaviour.
> 
> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> ---
> 
> Since the new fallback now also has a fallback (fallback-ception?) to use the
> old 'link0' parameter as a remote IP for single-link clusters, this patch is
> entirely optional (i.e. everything else from the series should work without it).
> 
> If we do plan to implement a concept like this it might make sense to implement
> it sooner then later, so when we do need it, the versions are already available
> in all (pre-)join checks.
> 
> 
>  data/PVE/API2/ClusterConfig.pm | 35 +++++++++++++++++++++++++++++++---
>  data/PVE/Cluster/Setup.pm      | 27 ++++++++++++++++++++++----
>  2 files changed, 55 insertions(+), 7 deletions(-)
> 
> diff --git a/data/PVE/API2/ClusterConfig.pm b/data/PVE/API2/ClusterConfig.pm
> index 64bebfe..70faad0 100644
> --- a/data/PVE/API2/ClusterConfig.pm
> +++ b/data/PVE/API2/ClusterConfig.pm
> @@ -58,11 +58,29 @@ __PACKAGE__->register_method({
>  	    { name => 'totem' },
>  	    { name => 'join' },
>  	    { name => 'qdevice' },
> +	    { name => 'apiversion' },
>  	];
>  
>  	return $result;
>      }});
>  
> +__PACKAGE__->register_method ({
> +    name => 'join_api_version',
> +    path => 'apiversion',
> +    method => 'GET',
> +    description => "Return the version of the cluster join API available on this node.",
> +    permissions => {
> +	check => ['perm', '/', [ 'Sys.Audit' ]],
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {},
> +    },
> +    returns => { type => 'integer' },

maybe add minimum 0 and
description => "Cluster Join API version, currently " . PVE::Cluster::Setup::JOIN_API_VERSION

?

> +    code => sub {
> +	return PVE::Cluster::Setup::JOIN_API_VERSION;
> +    }});
> +
>  __PACKAGE__->register_method ({
>      name => 'create',
>      path => '',
> @@ -213,6 +231,11 @@ __PACKAGE__->register_method ({
>  		format => 'ip',
>  		optional => 1,
>  	    },
> +	    apiversion => {
> +		type => 'integer',
> +		description => 'The JOIN_API_VERSION of the new node.',
> +		optional => 1,
> +	    },
>  	}),
>      },
>      returns => {
> @@ -235,6 +258,12 @@ __PACKAGE__->register_method ({
>      code => sub {
>  	my ($param) = @_;
>  
> +	$param->{apiversion} //= 0;
> +	die "unsupported old API version on joining node ($param->{apiversion}),"
> +	    . " please upgrade before joining\n"
> +	    if $param->{apiversion} < (PVE::Cluster::Setup::JOIN_API_VERSION -
> +		PVE::Cluster::Setup::JOIN_API_AGE_AS_CLUSTER);

multiline post-if, also it's always nice to have both in the error message,
is-value and expected-value.

> +
>  	PVE::Cluster::check_cfs_quorum();
>  
>  	my $vc_errors;
> @@ -299,10 +328,10 @@ __PACKAGE__->register_method ({
>  	    } else {
>  		# FIXME: Probably a good idea to remove this once we're sure
>  		# noone would join a node old enough to need this to a recent
> -		# cluster
> +		# cluster, then remove 0 from API_AGE compatible versions
>  		$param->{new_node_ip} = $cluster_links->{0}->{address}
> -		    if !$param->{new_node_ip} && $cluster_links->{0}
> -		    && scalar(%$cluster_links) == 1;
> +		    if !$param->{new_node_ip} && $cluster_links->{0} &&
> +			scalar(%$cluster_links) == 1 && $param->{apiversion} == 0;
>  
>  		# when called without any link parameters, fall back to
>  		# new_node_ip, if all existing nodes only have a single link too
> diff --git a/data/PVE/Cluster/Setup.pm b/data/PVE/Cluster/Setup.pm
> index 6d880dd..fa7a039 100644
> --- a/data/PVE/Cluster/Setup.pm
> +++ b/data/PVE/Cluster/Setup.pm
> @@ -20,6 +20,13 @@ use PVE::Network;
>  use PVE::Tools;
>  use PVE::Certificate;
>  
> +# Only relevant for pre-join checks, after join happened versions can differ
> +use constant JOIN_API_VERSION => 1;
> +# (APIVER-this) is oldest version a new node in addnode can have to be accepted
> +use constant JOIN_API_AGE_AS_CLUSTER => 1;
> +# (APIVER-this) is oldest version a cluster node can have to still try joining
> +use constant JOIN_API_AGE_AS_JOINEE => 1;
> +

isn't it enough if we have the actual version, and the AGE one can join us?
Then all is clear, the third one does not brings additional value or?

>  my $pmxcfs_base_dir = PVE::Cluster::base_dir();
>  my $pmxcfs_auth_dir = PVE::Cluster::auth_dir();
>  
> @@ -664,6 +671,12 @@ sub join {
>      # login raises an exception on failure, so if we get here we're good
>      print "Login succeeded.\n";
>  
> +    # check cluster join API version
> +    my $apiver = eval { $conn->get("/cluster/config/apiversion"); } // 0;

nit, omit the semicolon in the eval for readability.

> +    die "error: incompatible join API version on cluster ($apiver). Make sure"
> +	. " all nodes are up-to-date.\n"
> +	if $apiver < (JOIN_API_VERSION - JOIN_API_AGE_AS_JOINEE);

hard to read, switch to "normal" if.

> +
>      my $args = {};
>      $args->{force} = $param->{force} if defined($param->{force});
>      $args->{nodeid} = $param->{nodeid} if $param->{nodeid};
> @@ -672,11 +685,17 @@ sub join {
>  	$args->{"link$link"} = PVE::Corosync::print_corosync_link($links->{$link});
>      }
>  
> -    # this will be used as fallback if no links are specified
> -    $args->{new_node_ip} = $local_ip_address;
> +    if (!%$links) {
> +	print "No local links given, will attempt fallback to $local_ip_address\n";
>  
> -    print "No local links given, will attempt fallback to $local_ip_address\n"
> -	if !%$links;
> +	# specify fallback if no links are given according to remote api version
> +	$args->{link0} = $local_ip_address if $apiver == 0;
> +	$args->{new_node_ip} = $local_ip_address if $apiver >= 1;
> +    }
> +
> +    if ($apiver >= 1) {
> +	$args->{apiversion} = JOIN_API_VERSION;
> +    }
>  
>      print "Request addition of this node\n";
>      my $res = eval { $conn->post("/cluster/config/nodes/$nodename", $args); };
>