[Engine-devel] Weird behavior of multiple SetVmTicket query

Itamar Heim iheim at redhat.com
Thu Nov 21 14:01:34 UTC 2013 

On 11/21/2013 03:28 PM, Alexander Wels wrote:
> On Thursday, November 21, 2013 07:18:42 AM Daniel Erez wrote:
>> ----- Original Message -----
>>
>>> From: "Einav Cohen" <ecohen at redhat.com>
>>> To: "Livnat Peer" <lpeer at redhat.com>, "Eli Mesika" <emesika at redhat.com>,
>>> "Omer Frenkel" <ofrenkel at redhat.com>, "Doron Fediuck"
>>> <dfediuck at redhat.com>, "Oved Ourfalli" <ovedo at redhat.com> Cc:
>>> "engine-devel" <engine-devel at ovirt.org>
>>> Sent: Monday, November 18, 2013 11:19:23 PM
>>> Subject: Re: [Engine-devel] Weird behavior of multiple SetVmTicket query
>>>
>>> engine-core maintainers - this one is mainly for you - see below.
>>> the most important question (I think) is: Is there any reason
>>> to not introduced a "sync" flavor of RunMultipleActions (i.e.
>>> one that will return from the backend only when all actions have
>>> been completed, similarly to RunAction)?
>>
>> There's a couple of years old patch exactly for that [1].
>> It should allow both synchronous and asynchronous multiple actions according
>> to a flag. Question is when do we want to use the synchronous flavor?
>> The reasoning behind the current implementation is to return a quick
>> response to the client, as executing multiple vdsm commands can potentially
>> require a long wait. Do we need to introduce a sync version just for VM
>> console connection flow? IIRC, the flow actually uses multiple calls to the
>> singular form (RunAction) rather than RunMultipleAction?
>>
>> [1] http://gerrit.ovirt.org/#/c/3210/
>>
>
> We recently introduced a patch [1] that on the front-end is smart enough to
> consolidate multiple query/actions into a single runMultiple(Query/Action)
> call to reduce the number http requests to the back-end. In the SetVMTicket
> case it combined two actions into one runMultipleAction call and was expected
> that to return once all the actions where complete, which didn't happen
> causing some undesired behavior on the client side. This sparked the
> investigation that ended in this email.
>
> So yes I would definitely like a 'synchronous' version of runMultipleAction so
> the frontend code can set that flag when combining multiple actions into a
> single call. We can only safely do this if we are combining several runActions
> as all of those are 'synchronous'.
>
> [1] http://gerrit.ovirt.org/#/c/17356/

btw, how is this going to work with the REST API?

>
>>> ----
>>> Thanks,
>>> Einav
>>>
>>> ----- Original Message -----
>>>
>>>> From: "Vojtech Szocs" <vszocs at redhat.com>
>>>> To: awels at redhat.com
>>>> Cc: "engine-devel" <engine-devel at ovirt.org>
>>>> Sent: Tuesday, November 12, 2013 6:40:18 AM
>>>> Subject: Re: [Engine-devel] Weird behavior of multiple SetVmTicket query
>>>>
>>>> Forwarding this email to engine-devel so that backend maintainers are
>>>> aware
>>>> of this issue.
>>>>
>>>> Looking at the code:
>>>>
>>>> - MultipleActionsRunner#Execute first creates and "validates" all
> commands:
>>>>    A. the "for" block which iterates over getParameters()
>>>>
>>>>       1-> validate correlation ID, if OK create and add command,
>>>>       otherwise
>>>>       add
>>>>       returnValue
>>>>
>>>>    B the "if" block which tests getCommands().size()
>>>>
>>>>       1-> if single command to execute, add its "canDoActionOnly"
>>>>       returnValue,
>>>>       which is returnValue with canDoAction but without actual result
>>>>       object
>>>>       2-> if multi commands to execute, execute chunks of max. 10 threads
>>>>       (sequentially, ThreadPoolUtil.invokeAll returns after all threads
>>>>       complete), same returnValue as above
>>>>
>>>>    C. the "if" block which tests canRunActions
>>>>
>>>>       1-> all commands are executed within SINGLE THREAD due to
>>>>       ThreadPoolUtil.execute(Runnable), which is kind of weird comapred
>>>>       to
>>>>       how returnValues are prepared (see B2)
>>>>       2-> when executing command, code DOES NOT CARE about its
>>>>       returnValue,
>>>>       i.e. returnValue was already prepared (see B) and command execution
>>>>       should just update it
>>>>
>>>> The problem (I think) is that C1 starts a different thread (to execute
>>>> all
>>>> commands) and immediately returns, i.e. code doesn't wait for thread to
>>>> complete. This is why returnValues are observed on frontend as
>>>> inconsistent.
>>>>
>>>> Additionally, we're also mixing of two different things: canDoAction
>>>> processing and returnValues processing. IMHO this should be refactored
>>>> to
>>>> make code easier to read.
>>>>
>>>> Changes done by Alex (patch attached):
>>>>
>>>> X1. returnValues changed to Collections.synchronizedList
>>>>
>>>>      -> this means all access to returnValues is now serial
>>>>      -> iteration over synchronizedList should also be enclosed in
>>>>
>>>>      "synchronized (list)" block, so this:
>>>>             for (VdcReturnValueBase value : returnValues) ...
>>>>
>>>>         should be this:
>>>>             synchronized (returnValues) { for (VdcReturnValueBase value :
>>>>             returnValues) ... }
>>>>
>>>> X2. commented-out original command execution via
>>>> ThreadPoolUtil.execute(Runnable)
>>>>
>>>>      -> new RunCommands method invokes all commands each in separate
>>>>      thread
>>>>      via ThreadPoolUtil.invokeAll
>>>>      -> returnValues list is explicitly updated
>>>>
>>>> Guys, what do you think?
>>>>
>>>> Vojtech
>>>>
>>>>
>>>> ----- Original Message -----
>>>>
>>>>> From: "Alexander Wels" <awels at redhat.com>
>>>>> To: "Frantisek Kobzik" <fkobzik at redhat.com>
>>>>> Cc: "Vojtech Szocs" <vszocs at redhat.com>
>>>>> Sent: Monday, November 11, 2013 9:19:08 PM
>>>>> Subject: Re: Weird behavior of multiple SetVmTicket query
>>>>>
>>>>> Hi,
>>>>>
>>>>> I did some debugging, and the problem is a race condition in the
>>>>> MultipleActions class. The whole class seems to have a lot of
>>>>> multi-threading
>>>>> issues but if I modify the code to wait for the results of the actions
>>>>> to
>>>>> return before returning the return value, everything works fine.
>>>>>
>>>>> I am attaching a patch that solves the issue at hand, but should not
>>>>> be
>>>>> considered a real patch. It is just to show the issue is in the
>>>>> back-end
>>>>> not
>>>>> the front-end code. The front-end code is just exposing an issue in
>>>>> the
>>>>> back-
>>>>> end
>>>>>
>>>>> Alexander
>>>>>
>>>>> On Monday, November 11, 2013 09:53:22 AM you wrote:
>>>>>> Ok, thank you very much!
>>>>>>
>>>>>> ----- Original Message -----
>>>>>> From: "Alexander Wels" <awels at redhat.com>
>>>>>> To: "Frantisek Kobzik" <fkobzik at redhat.com>
>>>>>> Sent: Monday, November 11, 2013 2:15:43 PM
>>>>>> Subject: Re: Weird behavior of multiple SetVmTicket query
>>>>>>
>>>>>> Frantisek,
>>>>>>
>>>>>> I had seen this before, let me test and fix it for you, it is very
>>>>>> likely
>>>>>> my
>>>>>> patch broke that.
>>>>>>
>>>>>> Alexander
>>>>>>
>>>>>> On Monday, November 11, 2013 03:43:19 AM you wrote:
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>> recently I noticed problems with invoking console for multiple VMs
>>>>>>> (select
>>>>>>> more VMs in webadmin and then hit the console btn). I was sure it
>>>>>>> worked
>>>>>>> in
>>>>>>> past so i git-bisected the master branch and I discovered that
>>>>>>> this
>>>>>>> problem
>>>>>>> is apparently caused by patch [1]. For single vm console
>>>>>>> invocation
>>>>>>> it
>>>>>>> works fine, but for multiple VMs it doesn't.
>>>>>>>
>>>>>>> I did some closer investigation with a debugger and it seems that
>>>>>>> getSucceeded() on the return val of SetVmTicket command returns
>>>>>>> false
>>>>>>> in
>>>>>>> case of multiple execution despite the fact SetVmTicketCommand
>>>>>>> sets
>>>>>>> this
>>>>>>> value to true. I suppose there is some problem with propagation of
>>>>>>> command
>>>>>>> return value from BE to FE. The same goes for the encapsulated
>>>>>>> returnValue
>>>>>>> attribute (it contains value of the generated ticket). When
>>>>>>> invoking
>>>>>>> multiple consoles it is null (although it has been filled on
>>>>>>> backend).
>>>>>>> Weird.
>>>>>>>
>>>>>>> Tomas told me you did some optimizations for multiple command
>>>>>>> executions,
>>>>>>> do you know it might cause it? Please let me know if you have any
>>>>>>> idea...
>>>>>>>
>>>>>>> Cheers,
>>>>>>> F.
>>>>>>>
>>>>>>> [1]: http://gerrit.ovirt.org/#/c/17356/
>>>>
>>>> _______________________________________________
>>>> Engine-devel mailing list
>>>> Engine-devel at ovirt.org
>>>> http://lists.ovirt.org/mailman/listinfo/engine-devel
>>>
>>> _______________________________________________
>>> Engine-devel mailing list
>>> Engine-devel at ovirt.org
>>> http://lists.ovirt.org/mailman/listinfo/engine-devel
>>
>> _______________________________________________
>> Engine-devel mailing list
>> Engine-devel at ovirt.org
>> http://lists.ovirt.org/mailman/listinfo/engine-devel
>
> _______________________________________________
> Engine-devel mailing list
> Engine-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/engine-devel
>

More information about the Devel mailing list