[pve-devel] [PATCH ha-manager 1/4] allow LRM lock stealing for fenced nodes

[Fabian Grünbichler]

On March 27, 2019 5:42 pm, Thomas Lamprecht wrote:
> We are only allowed to recover (=steal) a service when we have its
> LRMs lock, as this guarantees us that even if said LRM comes up
> again during the steal operation the LRM cannot start the services
> when the service config still belongs to it for a short time.
> 
> This is important, else we have a possible race for the resource
> which can result in a service started on the old (restarted) node
> and the node where the service was recovered too, which is really
> bad!

two very long sentences ;)

In order to recover (=steal) a service from a fenced node, we need to 
own its LRM's lock. Otherwise, the fenced node could start the service 
during the short timespan where it still owns the service config file. 

This requirement prevents a race between the stealing node and the 
fenced node - we never want to start a service on two nodes!

small nits inline, otherwise

Acked-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> ---
>  src/PVE/HA/Env.pm      |  4 ++--
>  src/PVE/HA/Env/PVE2.pm |  4 ++--
>  src/PVE/HA/Sim/Env.pm  | 16 ++++++++++------
>  3 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/src/PVE/HA/Env.pm b/src/PVE/HA/Env.pm
> index bb37486..6b76496 100644
> --- a/src/PVE/HA/Env.pm
> +++ b/src/PVE/HA/Env.pm
> @@ -171,9 +171,9 @@ sub get_ha_agent_lock {
>  # this should only get called if the nodes LRM gracefully shuts down with
>  # all services already cleanly stopped!
>  sub release_ha_agent_lock {
> -    my ($self) = @_;
> +    my ($self, $node) = @_;
>  
> -    return $self->{plug}->release_ha_agent_lock();
> +    return $self->{plug}->release_ha_agent_lock($node);
>  }
>  
>  # return true when cluster is quorate
> diff --git a/src/PVE/HA/Env/PVE2.pm b/src/PVE/HA/Env/PVE2.pm
> index 796acd9..4428ed4 100644
> --- a/src/PVE/HA/Env/PVE2.pm
> +++ b/src/PVE/HA/Env/PVE2.pm
> @@ -309,9 +309,9 @@ sub get_ha_agent_lock {
>  # this should only get called if the nodes LRM gracefully shuts down with
>  # all services already cleanly stopped!

nit: this comment is now no longer true? we also call it after HW fencing, 
which is possibly at the other end of the forceful vs graceful spectrum ;)

nit: I'd actually prefer splitting the release and steal into two separate 
subs, to make it obvious what is happening also at first glance at the 
call site(s). they are short and trivial enough that the duplication
should be harmless, but they can of course also be wrappers around the 
code below.

>  sub release_ha_agent_lock {
> -    my ($self) = @_;
> +    my ($self, $node) = @_;
>  
> -    my $node = $self->nodename();
> +    $node = $node || $self->nodename();
>  
>      return rmdir("$lockdir/ha_agent_${node}_lock");
>  }
> diff --git a/src/PVE/HA/Sim/Env.pm b/src/PVE/HA/Sim/Env.pm
> index 22e13e6..171e486 100644
> --- a/src/PVE/HA/Sim/Env.pm
> +++ b/src/PVE/HA/Sim/Env.pm
> @@ -83,13 +83,17 @@ sub sim_get_lock {
>  	    if (my $d = $data->{$lock_name}) {
>  		my $tdiff = $ctime - $d->{time};
>  
> +		my $manager_node = $data->{'ha_manager_lock'}->{node} || '';
> +
> +		$res = 0;
>  		if ($tdiff > $self->{lock_timeout}) {
>  		    $res = 1;
> -		} elsif (($tdiff <= $self->{lock_timeout}) && ($d->{node} eq $nodename)) {
> -		    delete $data->{$lock_name};
> -		    $res = 1;
>  		} else {
> -		    $res = 0;
> +		    # if we aren't manager we may unlock only *our* lock
> +		    if ($d->{node} eq $nodename || $manager_node eq $nodename) {
> +			delete $data->{$lock_name};
> +			$res = 1;
> +		    }
>  		}
>  	    }
>  
> @@ -342,9 +346,9 @@ sub get_ha_agent_lock {
>  # this should only get called if the nodes LRM gracefully shuts down with
>  # all services already cleanly stopped!
>  sub release_ha_agent_lock {
> -    my ($self) = @_;
> +    my ($self, $node) = @_;
>  
> -    my $node = $self->nodename();
> +    $node = $node || $self->nodename();
>  
>      my $lock = $self->get_ha_agent_lock_name($node);
>      return $self->sim_get_lock($lock, 1);
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>