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

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?

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