[PATCH conductor] BZ#783218 added pessimistic database locking for quota to prevent updating with old values

Richard Su rwsu at redhat.com
Mon Feb 6 19:52:36 UTC 2012


On 02/02/2012 12:11 AM, Jan Provaznik wrote:
> On 02/02/2012 05:26 AM, Richard Su wrote:
>> On 01/31/2012 08:08 AM, Tomas Sedovic wrote:
>>> On 01/30/2012 01:53 PM, Tomáš Hrčka wrote:
>>>> ---
>>>> src/app/models/instance_observer.rb | 20 ++++++++++++++------
>>>> 1 files changed, 14 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/src/app/models/instance_observer.rb
>>>> b/src/app/models/instance_observer.rb
>>>> index 9d13fb0..06ba2ef 100644
>>>> --- a/src/app/models/instance_observer.rb
>>>> +++ b/src/app/models/instance_observer.rb
>>>> @@ -65,13 +65,21 @@ class InstanceObserver< ActiveRecord::Observer
>>>> elsif RUNNING_STATES.include?(state_from)&&
>>>> !RUNNING_STATES.include?(state_to)
>>>> quota.running_instances -= 1
>>>> end
>>>> -
>>>> - if !ACTIVE_STATES.include?(state_from)&&
>>>> ACTIVE_STATES.include?(state_to)
>>>> - quota.total_instances += 1
>>>> - elsif ACTIVE_STATES.include?(state_from)&&
>>>> !ACTIVE_STATES.include?(state_to)
>>>> - quota.total_instances -= 1
>>>> - end
>>>> - quota.save!
>>>> + quota.transaction do
>>>> + quota.lock!
>>>> + if !RUNNING_STATES.include?(state_from)&&
>>>> RUNNING_STATES.include?(state_to)
>>>> + quota.running_instances += 1
>>>> + elsif RUNNING_STATES.include?(state_from)&&
>>>> !RUNNING_STATES.include?(state_to)
>>>> + quota.running_instances -= 1
>>>> + end
>>>> +
>>>> + if !ACTIVE_STATES.include?(state_from)&&
>>>> ACTIVE_STATES.include?(state_to)
>>>> + quota.total_instances += 1
>>>> + elsif ACTIVE_STATES.include?(state_from)&&
>>>> !ACTIVE_STATES.include?(state_to)
>>>> + quota.total_instances -= 1
>>>> + end
>>>> + quota.save!
>>>> + end
>>>> end
>>>> end
>>>> end
>>>
>>> I'm willing to ACK this as the code seems to be correct to me.
>>>
>>> However, I'm having trouble reproducing the original issue. If it
>>> fixes it for you, go ahead and push this. We'll see what the person
>>> who reported the bug has to say about it.
>>>
>>> Or if there's anyone else who can repro the bug, do feel free to try
>>> it out.
>>
>> Ack. I'm also not able to reproduce the original issue. Launches work
>> with this patch, I've tried 50 mock instances in a deployment.
>>
>> I'd say push it and let's get more feedback from the performance testing
>> lab.
>>
>>
>
> Reproduction nit:
> What happens here is that both dbomatic and conductor touches quota in 
> the same time, it's probably easier to reproduce on RHEVM because 
> start process is slower than for example for mock, so there is higher 
> chance that a user hits dbomatic 60s refresh cycle.
>
> Because dbomatic forks separate processes for each account, same 
> conflict can be achieved on mock driver by defining 2-3 providers,
> then launch big defployment on each of them at the same time (or try 
> to stop them). At least this worked for me.
>
> Jan

Pushed. Dave Wilson was able to reproduce it in the lab and this patch 
fixes the problem.



More information about the aeolus-devel mailing list