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

Jan Provaznik jprovazn at redhat.com
Thu Feb 2 08:11:02 UTC 2012


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



More information about the aeolus-devel mailing list