[Engine-devel] Weird behavior of multiple SetVmTicket query

Alexander Wels awels at redhat.com
Thu Nov 21 14:15:40 UTC 2013 

On Thursday, November 21, 2013 04:01:34 PM Itamar Heim wrote:
> 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?
> 

I believe that is one of the outstanding issues with regards to the REST API 
transition. Before this optimization we already had code calling 
runMultiple(Query/Action) and we will have to figure something out for the REST 
API there as well. I think at the point we solve that issue we can decide if 
we want to keep the optimization or not.

> >>> ----
> >>> 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