Re: [Engine-devel] Weird behavior of multiple SetVmTicket query

From: "Einav Cohen" <ecohen(a)redhat.com&gt; To: "Livnat Peer" <lpeer(a)redhat.com&gt;, "Eli Mesika" <emesika(a)redhat.com&gt;, "Omer Frenkel" <ofrenkel(a)redhat.com&gt;, "Doron Fediuck" <dfediuck(a)redhat.com&gt;, "Oved Ourfalli" <ovedo(a)redhat.com&gt; Cc: awels(a)redhat.com, "engine-devel" <engine-devel(a)ovirt.org&gt;, "Vojtech Szocs" <vszocs(a)redhat.com&gt; Sent: Monday, November 18, 2013 10: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)?

---- Thanks, Einav ----- Original Message ----- > From: "Vojtech Szocs" <vszocs(a)redhat.com&gt; > To: awels(a)redhat.com > Cc: "engine-devel" <engine-devel(a)ovirt.org&gt; > 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(a)redhat.com&gt; > > To: "Frantisek Kobzik" <fkobzik(a)redhat.com&gt; > > Cc: "Vojtech Szocs" <vszocs(a)redhat.com&gt; > > 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(a)redhat.com&gt; > > > To: "Frantisek Kobzik" <fkobzik(a)redhat.com&gt; > > > 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(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/engine-devel >