[pve-devel] [PATCH ha-manager 09/13] Add crm command 'stop'

[Thomas Lamprecht]

On 9/27/19 10:07 AM, Fabian Grünbichler wrote:
>> @@ -651,9 +662,17 @@ sub next_state_started {
>>  		    $haenv->log('info', "$cmd service '$sid' to node '$target'");
>>  		    &$change_service_state($self, $sid, $cmd, node => $sd->{node}, target => $target);
>>  		}
>> +	    } elsif ($cmd eq 'stop') {
>> +		my $timeout = shift @{$sd->{cmd}};
>> +		$haenv->log('info', "$cmd service with timeout '$timeout'");
>> +		&$change_service_state($self, $sid, 'request_stop', timeout => $timeout);
>> +		$haenv->update_service_config(0, 0, $sid, {'state' => 'stopped'});
> this uses the methods introduced in patch #2/3, but is missing the 
> locking that the other call site (and the other read/modify/write blocks 
> for that config file) have.
> 
> either move the locking to PVE::HA::Config::update_resources_config in 
> #2, or add locking here, or re-evaluate whether there is an approach 
> that can skip modifying the config file ;). there are other calls to 
> lock_ha_domain in the CRM command handling flow, so it's probably not 
> that bad to have another few instances here..
> 

That's the "missing locking" issue I mentioned in the cover-letter reply:

> especially at 02/13 (the service-config) changes, which seems to have
> some issues (missing locking) but that should be solvable.
 
If we add it here I'd mark the refactored method as "_nolock"
But IMO it makes never sense to update without lock so it probably
should be always locked in the update_service_config method..