[Engine-devel] Weird behavior of multiple SetVmTicket query

[Einav Cohen]

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

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