[Engine-devel] Weird behavior of multiple SetVmTicket query

[Vojtech Szocs]

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/
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: thread_wait.patch
Type: text/x-patch
Size: 4373 bytes
Desc: not available
URL: <http://lists.ovirt.org/pipermail/engine-devel/attachments/20131112/f2dfc4bb/attachment.bin>