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

From: "Alexander Wels" <awels(a)redhat.com&gt; To: "Itamar Heim" <iheim(a)redhat.com&gt; Cc: "Oved Ourfalli" <ovedo(a)redhat.com&gt;, engine-devel(a)ovirt.org Sent: Thursday, November 21, 2013 3:15:40 PM Subject: Re: [Engine-devel] Weird behavior of multiple SetVmTicket query On Thursday, November 21, 2013 04:01:34 PM Itamar Heim wrote: > On 11/21/2013 03:28 PM, Alexander Wels wrote: > > On Thursday, November 21, 2013 07:18:42 AM Daniel Erez wrote: > >> ----- Original Message ----- > >> > >>> 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: > >>> "engine-devel" <engine-devel(a)ovirt.org&gt; > >>> 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/ > > > > We recently introduced a patch [1] that on the front-end is smart enough > > to > > consolidate multiple query/actions into a single > > runMultiple(Query/Action) > > call to reduce the number http requests to the back-end. In the > > SetVMTicket > > case it combined two actions into one runMultipleAction call and was > > expected that to return once all the actions where complete, which didn't > > happen causing some undesired behavior on the client side. This sparked > > the investigation that ended in this email. > > > > So yes I would definitely like a 'synchronous' version of > > runMultipleAction so the frontend code can set that flag when combining > > multiple actions into a single call. We can only safely do this if we are > > combining several runActions as all of those are 'synchronous'. > > > > [1] http://gerrit.ovirt.org/#/c/17356/ > > btw, how is this going to work with the REST API? > I believe that is one of the outstanding issues with regards to the REST API transition. Before this optimization we already had code calling runMultiple(Query/Action) and we will have to figure something out for the REST API there as well. I think at the point we solve that issue we can decide if we want to keep the optimization or not.

> >>> ---- > >>> 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 > >>> > >>> _______________________________________________ > >>> Engine-devel mailing list > >>> Engine-devel(a)ovirt.org > >>> http://lists.ovirt.org/mailman/listinfo/engine-devel > >> > >> _______________________________________________ > >> Engine-devel mailing list > >> Engine-devel(a)ovirt.org > >> http://lists.ovirt.org/mailman/listinfo/engine-devel > > > > _______________________________________________ > > Engine-devel mailing list > > Engine-devel(a)ovirt.org > > http://lists.ovirt.org/mailman/listinfo/engine-devel _______________________________________________ Engine-devel mailing list Engine-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel