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..
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
