----- Original Message ----- > From: "Einav Cohen" <eco...@redhat.com> > To: "Livnat Peer" <lp...@redhat.com>, "Eli Mesika" <emes...@redhat.com>, > "Omer Frenkel" <ofren...@redhat.com>, "Doron > Fediuck" <dfedi...@redhat.com>, "Oved Ourfalli" <ov...@redhat.com> > Cc: "engine-devel" <engine-devel@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" <vsz...@redhat.com> > > To: aw...@redhat.com > > Cc: "engine-devel" <engine-devel@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" <aw...@redhat.com> > > > To: "Frantisek Kobzik" <fkob...@redhat.com> > > > Cc: "Vojtech Szocs" <vsz...@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" <aw...@redhat.com> > > > > To: "Frantisek Kobzik" <fkob...@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@ovirt.org > > http://lists.ovirt.org/mailman/listinfo/engine-devel > > > _______________________________________________ > Engine-devel mailing list > Engine-devel@ovirt.org > http://lists.ovirt.org/mailman/listinfo/engine-devel > _______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel