[Engine-devel] Weird behavior of multiple SetVmTicket query

Daniel Erez derez at redhat.com
Thu Nov 21 12:18:42 UTC 2013 


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

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

More information about the Engine-devel mailing list