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